Merge lp:~david-goetz/swift/invalid_url_bug into lp:~hudson-openstack/swift/trunk

[David Goetz]

catching invalid urls and adding tests

[Chuck Thier (cthier)]

Looks good to me

review: Approve

[Chuck Thier (cthier)]

btw, I added another note to the ticket for his issue so that we can track back porting it to the 1.1 series.

[Chuck Thier (cthier)]

Ok looks good now as well, thanks!

review: Approve

=== modified file 'swift/account/server.py'
--- swift/account/server.py	2010-09-22 19:34:52 +0000
+++ swift/account/server.py	2010-11-01 22:14:58 +0000
@@ -77,7 +77,12 @@
 
     def PUT(self, req):
         """Handle HTTP PUT request."""
-        drive, part, account, container = split_path(unquote(req.path), 3, 4)
+        try:
+            drive, part, account, container = split_path(unquote(req.path),
+                                                         3, 4)
+        except ValueError, err:
+            return HTTPBadRequest(body=str(err), content_type='text/plain',
+                                  request=req)
         if self.mount_check and not check_mount(self.root, drive):
             return Response(status='507 %s is not mounted' % drive)
         broker = self._get_account_broker(drive, part, account)

=== modified file 'swift/auth/server.py'
--- swift/auth/server.py	2010-10-19 22:35:18 +0000
+++ swift/auth/server.py	2010-11-01 22:14:58 +0000
@@ -516,8 +516,11 @@
 
         :param request: A webob.Request instance.
         """
-        pathsegs = \
-            split_path(request.path, minsegs=1, maxsegs=3, rest_with_last=True)
+        try:
+            pathsegs = split_path(request.path, minsegs=1, maxsegs=3,
+                                  rest_with_last=True)
+        except ValueError:
+            return HTTPBadRequest()
         if pathsegs[0] == 'v1' and pathsegs[2] == 'auth':
             account = pathsegs[1]
             user = request.headers.get('x-storage-user')

=== modified file 'swift/common/middleware/auth.py'
--- swift/common/middleware/auth.py	2010-09-17 18:26:30 +0000
+++ swift/common/middleware/auth.py	2010-11-01 22:14:58 +0000
@@ -16,7 +16,7 @@
 from time import time
 
 from eventlet.timeout import Timeout
-from webob.exc import HTTPForbidden, HTTPUnauthorized
+from webob.exc import HTTPForbidden, HTTPUnauthorized, HTTPNotFound
 
 from swift.common.bufferedhttp import http_connect_raw as http_connect
 from swift.common.middleware.acl import clean_acl, parse_acl, referrer_allowed
@@ -82,8 +82,11 @@
                 # With a non-empty reseller_prefix, I would like to be called
                 # back for anonymous access to accounts I know I'm the
                 # definitive auth for.
-                version, rest = split_path(env.get('PATH_INFO', ''),
-                                           1, 2, True)
+                try:
+                    version, rest = split_path(env.get('PATH_INFO', ''),
+                                               1, 2, True)
+                except ValueError:
+                    return HTTPNotFound()(env, start_response)
                 if rest and rest.startswith(self.reseller_prefix):
                     # Handle anonymous access to accounts I'm the definitive
                     # auth for.
@@ -103,10 +106,10 @@
     def get_groups(self, token, memcache_client=None):
         """
         Get groups for the given token.
-        
+
         If memcache_client is set, token credentials will be cached
         appropriately.
-        
+
         With a cache miss, or no memcache_client, the configurated external
         authentication server will be queried for the group information.
 
@@ -146,7 +149,10 @@
         Returns None if the request is authorized to continue or a standard
         WSGI response callable if not.
         """
-        version, account, container, obj = split_path(req.path, 1, 4, True)
+        try:
+            version, account, container, obj = split_path(req.path, 1, 4, True)
+        except ValueError:
+            return HTTPNotFound(request=req)
         if not account or not account.startswith(self.reseller_prefix):
             return self.denied_response(req)
         user_groups = (req.remote_user or '').split(',')

=== modified file 'swift/common/middleware/ratelimit.py'
--- swift/common/middleware/ratelimit.py	2010-10-25 20:42:28 +0000
+++ swift/common/middleware/ratelimit.py	2010-11-01 22:14:58 +0000
@@ -14,6 +14,7 @@
 import time
 import eventlet
 from webob import Request, Response
+from webob.exc import HTTPNotFound
 
 from swift.common.utils import split_path, cache_from_env, get_logger
 from swift.proxy.server import get_container_memcache_key
@@ -204,7 +205,10 @@
         req = Request(env)
         if self.memcache_client is None:
             self.memcache_client = cache_from_env(env)
-        version, account, container, obj = split_path(req.path, 1, 4, True)
+        try:
+            version, account, container, obj = split_path(req.path, 1, 4, True)
+        except ValueError:
+            return HTTPNotFound()(env, start_response)
         ratelimit_resp = self.handle_ratelimit(req, account, container, obj)
         if ratelimit_resp is None:
             return self.app(env, start_response)

=== modified file 'swift/common/utils.py'
--- swift/common/utils.py	2010-10-21 18:32:10 +0000
+++ swift/common/utils.py	2010-11-01 22:14:58 +0000
@@ -208,6 +208,7 @@
                            trailing data, raises ValueError.
     :returns: list of segments with a length of maxsegs (non-existant
               segments will return as None)
+    :raises: ValueError if given an invalid path
     """
     if not maxsegs:
         maxsegs = minsegs
@@ -622,6 +623,7 @@
         os.fsync(fd)
         renamer(tmppath, dest)
 
+
 def audit_location_generator(devices, datadir, mount_check=True, logger=None):
     '''
     Given a devices path and a data directory, yield (path, device,

=== modified file 'swift/obj/server.py'
--- swift/obj/server.py	2010-10-19 01:05:54 +0000
+++ swift/obj/server.py	2010-11-01 22:14:58 +0000
@@ -534,8 +534,12 @@
         Handle REPLICATE requests for the Swift Object Server.  This is used
         by the object replicator to get hashes for directories.
         """
-        device, partition, suffix = split_path(
-            unquote(request.path), 2, 3, True)
+        try:
+            device, partition, suffix = split_path(
+                unquote(request.path), 2, 3, True)
+        except ValueError, e:
+            return HTTPBadRequest(body=str(e), request=request,
+                                  content_type='text/plain')
         if self.mount_check and not check_mount(self.devices, device):
             return Response(status='507 %s is not mounted' % device)
         path = os.path.join(self.devices, device, DATADIR, partition)

=== modified file 'swift/proxy/server.py'
--- swift/proxy/server.py	2010-10-27 15:52:00 +0000
+++ swift/proxy/server.py	2010-11-01 22:14:58 +0000
@@ -1259,6 +1259,8 @@
 
         :param path: path from request
         :returns: tuple of (controller class, path dictionary)
+
+        :raises: ValueError (thrown by split_path) id given invalid path
         """
         version, account, container, obj = split_path(path, 1, 4, True)
         d = dict(version=version,

=== modified file 'swift/stats/access_processor.py'
--- swift/stats/access_processor.py	2010-10-08 01:56:31 +0000
+++ swift/stats/access_processor.py	2010-11-01 22:14:58 +0000
@@ -66,10 +66,13 @@
             self.logger.debug('Bad server name: found "%s" expected "%s"' \
                                % (server, self.server_name))
             return {}
-        (version,
-        account,
-        container_name,
-        object_name) = split_path(request, 2, 4, True)
+        try:
+            (version, account, container_name, object_name) = \
+                split_path(request, 2, 4, True)
+        except ValueError, e:
+            self.logger.debug(
+                'Invalid path: %s from data: %s' % (e, repr(raw_log)))
+            return {}
         if container_name is not None:
             container_name = container_name.split('?', 1)[0]
         if object_name is not None:

=== modified file 'test/functional/tests.py'
--- test/functional/tests.py	2010-09-10 20:40:43 +0000
+++ test/functional/tests.py	2010-11-01 22:14:58 +0000
@@ -170,6 +170,15 @@
         self.assert_status(412)
         self.assert_body('Bad URL')
 
+    def testInvalidPath(self):
+        was_url = self.env.account.conn.storage_url
+        self.env.account.conn.storage_url = "/%s" % was_url
+        self.env.account.conn.make_request('GET')
+        try:
+            self.assert_status(404)
+        finally:
+            self.env.account.conn.storage_url = was_url
+
     def testPUT(self):
         self.env.account.conn.make_request('PUT')
         self.assert_status([403, 405])

=== modified file 'test/unit/auth/test_server.py'
--- test/unit/auth/test_server.py	2010-09-16 21:44:44 +0000
+++ test/unit/auth/test_server.py	2010-11-01 22:14:58 +0000
@@ -241,8 +241,9 @@
             len(set(repr(a) for a in cfaccounts) - set(failed)), 2)
 
     def test_auth_bad_path(self):
-        self.assertRaises(ValueError, self.controller.handle_auth,
+        res = self.controller.handle_auth(
             Request.blank('', environ={'REQUEST_METHOD': 'GET'}))
+        self.assertEquals(res.status_int, 400)
         res = self.controller.handle_auth(Request.blank('/bad',
                 environ={'REQUEST_METHOD': 'GET'}))
         self.assertEquals(res.status_int, 400)

=== modified file 'test/unit/common/middleware/test_ratelimit.py'
--- test/unit/common/middleware/test_ratelimit.py	2010-10-25 20:42:28 +0000
+++ test/unit/common/middleware/test_ratelimit.py	2010-11-01 22:14:58 +0000
@@ -366,6 +366,26 @@
         time_took = time.time() - begin
         self.assert_(round(time_took, 1) == .4)
 
+    def test_call_invalid_path(self):
+        env = {'REQUEST_METHOD': 'GET',
+               'SCRIPT_NAME': '',
+               'PATH_INFO': '//v1/AUTH_1234567890',
+               'SERVER_NAME': '127.0.0.1',
+               'SERVER_PORT': '80',
+               'swift.cache': FakeMemcache(),
+               'SERVER_PROTOCOL': 'HTTP/1.0'}
+
+        app = lambda *args, **kwargs: None
+        rate_mid = ratelimit.RateLimitMiddleware(app, {},
+                                                 logger=FakeLogger())
+
+        class a_callable(object):
+
+            def __call__(self, *args, **kwargs):
+                pass
+        resp = rate_mid.__call__(env, a_callable())
+        self.assert_('404 Not Found' in resp[0])
+
 
 if __name__ == '__main__':
     unittest.main()