From 36adcb6c66bf0c0093ead1080078ca8154b0d158 Mon Sep 17 00:00:00 2001 From: anc Date: Tue, 18 Feb 2014 14:07:37 +0000 Subject: [PATCH] Fix invalid account acl generating 500 response. Sending an account POST with an X-Account-Access-Control header value that is valid json but not a valid ACL was causing a 500 Internal Error if the value did not parse to a dict due to an exception being raised in tempauth.py. This patch modifies acl.py to check that the header value is both json and parses to a dict. The existing tests are extended to cover these invalid header values. This patch also enables json encoded dicts with whitespace (e.g. '{ }') to be accepted as a value for X-Account-Access-Control in the same way that '{}' is. These previously resulted in a 400 response. Closes-bug: 1281626 Change-Id: Ia06ba9c9d16f749f801a8158e73d3898c4a42888 --- swift/common/middleware/acl.py | 16 +++++++----- swift/common/middleware/tempauth.py | 2 +- test/unit/common/middleware/test_acl.py | 11 ++++++++ test/unit/common/middleware/test_tempauth.py | 27 +++++++++++++++++++- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/swift/common/middleware/acl.py b/swift/common/middleware/acl.py index 15bb67395c..2aed09dd2c 100644 --- a/swift/common/middleware/acl.py +++ b/swift/common/middleware/acl.py @@ -207,17 +207,21 @@ def parse_acl_v2(data): Parses a version-2 Swift ACL string and returns a dict of ACL info. :param data: string containing the ACL data in JSON format - :returns: A dict containing ACL info, e.g.: + :returns: A dict (possibly empty) containing ACL info, e.g.: {"groups": [...], "referrers": [...]} - :returns: None if data is None - :returns: empty dictionary if data does not parse as valid JSON + :returns: None if data is None, is not valid JSON or does not parse + as a dict + :returns: empty dictionary if data is an empty string """ if data is None: return None - try: - return json.loads(data) - except ValueError: + if data is '': return {} + try: + result = json.loads(data) + return (result if type(result) is dict else None) + except ValueError: + return None def parse_acl(*args, **kwargs): diff --git a/swift/common/middleware/tempauth.py b/swift/common/middleware/tempauth.py index fea26e1f1d..4f756247b9 100644 --- a/swift/common/middleware/tempauth.py +++ b/swift/common/middleware/tempauth.py @@ -324,7 +324,7 @@ class TempAuth(object): acl_header = 'x-account-access-control' acl_data = req.headers.get(acl_header) result = parse_acl(version=2, data=acl_data) - if (not result and acl_data not in ('', '{}')): + if result is None: return 'Syntax error in input (%r)' % acl_data tempauth_acl_keys = 'admin read-write read-only'.split() diff --git a/test/unit/common/middleware/test_acl.py b/test/unit/common/middleware/test_acl.py index 14ab6b4eb4..e74044d405 100644 --- a/test/unit/common/middleware/test_acl.py +++ b/test/unit/common/middleware/test_acl.py @@ -90,6 +90,17 @@ class TestACL(unittest.TestCase): # No header "hdr" exists -- should return None ({}, None), ({'junk': 'junk'}, None), + + # Empty ACLs should return empty dict + ({'hdr': ''}, {}), + ({'hdr': '{}'}, {}), + ({'hdr': '{ }'}, {}), + + # Bad input -- should return None + ({'hdr': '["array"]'}, None), + ({'hdr': 'null'}, None), + ({'hdr': '"some_string"'}, None), + ({'hdr': '123'}, None), ] for hdrs_in, expected in tests: diff --git a/test/unit/common/middleware/test_tempauth.py b/test/unit/common/middleware/test_tempauth.py index 2f1b718fb2..414f5035d1 100644 --- a/test/unit/common/middleware/test_tempauth.py +++ b/test/unit/common/middleware/test_tempauth.py @@ -1016,13 +1016,16 @@ class TestAccountAcls(unittest.TestCase): def test_acl_syntax_verification(self): test_auth = auth.filter_factory({'user_admin_user': 'testing'})( - FakeApp(iter(NO_CONTENT_RESP * 3))) + FakeApp(iter(NO_CONTENT_RESP * 5))) good_headers = {'X-Auth-Token': 'AUTH_t'} good_acl = '{"read-only":["a","b"]}' bad_acl = 'syntactically invalid acl -- this does not parse as JSON' wrong_acl = '{"other-auth-system":["valid","json","but","wrong"]}' bad_value_acl = '{"read-write":["fine"],"admin":"should be a list"}' + not_dict_acl = '["read-only"]' + not_dict_acl2 = 1 + empty_acls = ['{}', '', '{ }'] target = '/v1/AUTH_firstacct' # no acls -- no problem! @@ -1036,6 +1039,14 @@ class TestAccountAcls(unittest.TestCase): resp = req.get_response(test_auth) self.assertEquals(resp.status_int, 204) + # syntactically valid empty acls should go through + for acl in empty_acls: + update = {'x-account-access-control': acl} + req = self._make_request(target, + headers=dict(good_headers, **update)) + resp = req.get_response(test_auth) + self.assertEquals(resp.status_int, 204) + errmsg = 'X-Account-Access-Control invalid: %s' # syntactically invalid acls get a 400 update = {'x-account-access-control': bad_acl} @@ -1058,6 +1069,20 @@ class TestAccountAcls(unittest.TestCase): self.assertEquals(resp.status_int, 400) self.assertEquals(errmsg % "Value", resp.body[:39]) + # acls with wrong json structure also get a 400 + update = {'x-account-access-control': not_dict_acl} + req = self._make_request(target, headers=dict(good_headers, **update)) + resp = req.get_response(test_auth) + self.assertEquals(resp.status_int, 400) + self.assertEquals(errmsg % "Syntax error", resp.body[:46]) + + # acls with wrong json structure also get a 400 + update = {'x-account-access-control': not_dict_acl2} + req = self._make_request(target, headers=dict(good_headers, **update)) + resp = req.get_response(test_auth) + self.assertEquals(resp.status_int, 400) + self.assertEquals(errmsg % "Syntax error", resp.body[:46]) + def test_acls_propagate_to_sysmeta(self): test_auth = auth.filter_factory({'user_admin_user': 'testing'})( FakeApp(iter(NO_CONTENT_RESP * 3)))