From f63dc07b9da568c4bbe206278334fa1fb8a13be9 Mon Sep 17 00:00:00 2001
From: gholt <z-launchpad@brim.net>
Date: Fri, 19 Apr 2013 09:43:31 +0000
Subject: [PATCH] Extra safety on account-level DELETE

I just noticed tonight when adding a bunch of stuff to Swiftly that
the Bulk Delete middleware uses an account-level DELETE request,
albeit with a query parameter of bulk-delete. But, one typo and,
assuming the cluster supports it and you have access, whoops, you
just marked the account for deletion!

I put a bit of extra safety on the account deletion by requiring it
to have an empty query string.

Change-Id: Ib5df11193b04eff69d14185bd9d0607169131e7f
---
 swift/proxy/controllers/account.py |  5 +++++
 test/unit/proxy/test_server.py     | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/swift/proxy/controllers/account.py b/swift/proxy/controllers/account.py
index ee5995f1eb..f3ff6458fc 100644
--- a/swift/proxy/controllers/account.py
+++ b/swift/proxy/controllers/account.py
@@ -139,6 +139,11 @@ class AccountController(Controller):
     @public
     def DELETE(self, req):
         """HTTP DELETE request handler."""
+        # Extra safety in case someone typos a query string for an
+        # account-level DELETE request that was really meant to be caught by
+        # some middleware.
+        if req.query_string:
+            return HTTPBadRequest(request=req)
         if not self.app.allow_account_management:
             return HTTPMethodNotAllowed(
                 request=req,
diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py
index 497136f5dc..adea5e54ff 100644
--- a/test/unit/proxy/test_server.py
+++ b/test/unit/proxy/test_server.py
@@ -5564,6 +5564,29 @@ class TestAccountController(unittest.TestCase):
             test_status_map((201, 500, 500), 503)
             test_status_map((204, 500, 404), 503)
 
+    def test_DELETE_with_query_string(self):
+        # Extra safety in case someone typos a query string for an
+        # account-level DELETE request that was really meant to be caught by
+        # some middleware.
+        with save_globals():
+            controller = proxy_server.AccountController(self.app, 'account')
+
+            def test_status_map(statuses, expected, **kwargs):
+                set_http_connect(*statuses, **kwargs)
+                self.app.memcache.store = {}
+                req = Request.blank('/a?whoops', {'REQUEST_METHOD': 'DELETE'})
+                req.content_length = 0
+                self.app.update_request(req)
+                res = controller.DELETE(req)
+                expected = str(expected)
+                self.assertEquals(res.status[:len(expected)], expected)
+            test_status_map((201, 201, 201), 400)
+            self.app.allow_account_management = True
+            test_status_map((201, 201, 201), 400)
+            test_status_map((201, 201, 500), 400)
+            test_status_map((201, 500, 500), 400)
+            test_status_map((204, 500, 404), 400)
+
 
 class FakeObjectController(object):