From 0e3e7b9b0953baaf7a0686881fd2348fde7d1e59 Mon Sep 17 00:00:00 2001 From: Kota Tsuyuzaki Date: Thu, 22 Mar 2018 19:26:24 +0900 Subject: [PATCH] Fix versioned writes error with url-encoded object name With url encoded object name like '%25ff' that can be url-encoded value after decoded can cause 412 Precondition Failed. And more, that can do nothing (no versioned object creation) even it returns a successful response. The root causes are in versioned_writes middleware as follows: A. unnecessary unquote in object_request method B. incorrect use of make_pre_authed_request that takes 'quoted' path in the args. That is described at [1] explicitely. This patch resolved those 2 bugs at once, and then, now we can create %25ff versioned object reported in the launchpad with this patch. Perhaps, more tests would be nice to have. This patch added a few test cases on that. 1: https://github.com/openstack/swift/blob/master/swift/common/wsgi.py#L1174 Note that make_subrequest and its caller should have *quoted* path but make_env should *NOT*. That might make us confused. Closes-Bug: #1755554 Change-Id: Ibcd90cc633c68973929ee5249c6598c22b342e3e --- swift/common/middleware/versioned_writes.py | 44 +++++---- test/functional/test_versioned_writes.py | 99 ++++++++++++++++++- .../middleware/test_versioned_writes.py | 24 +++++ 3 files changed, 147 insertions(+), 20 deletions(-) diff --git a/swift/common/middleware/versioned_writes.py b/swift/common/middleware/versioned_writes.py index ee834fc413..25c7eee927 100644 --- a/swift/common/middleware/versioned_writes.py +++ b/swift/common/middleware/versioned_writes.py @@ -372,7 +372,7 @@ class VersionedWritesContext(WSGIContext): # to container, but not READ. This was allowed in previous version # (i.e., before middleware) so keeping the same behavior here get_req = make_pre_authed_request( - req.environ, path=path_info, + req.environ, path=quote(path_info), headers={'X-Newest': 'True'}, method='GET', swift_source='VW') source_resp = get_req.get_response(self.app) @@ -387,7 +387,7 @@ class VersionedWritesContext(WSGIContext): # Create a new Request object to PUT to the versions container, copying # all headers from the source object apart from x-timestamp. put_req = make_pre_authed_request( - req.environ, path=put_path_info, method='PUT', + req.environ, path=quote(put_path_info), method='PUT', swift_source='VW') copy_header_subset(source_resp, put_req, lambda k: k.lower() != 'x-timestamp') @@ -506,7 +506,7 @@ class VersionedWritesContext(WSGIContext): 'content-length': '0', 'x-auth-token': req.headers.get('x-auth-token')} marker_req = make_pre_authed_request( - req.environ, path=marker_path, + req.environ, path=quote(marker_path), headers=marker_headers, method='PUT', swift_source='VW') marker_req.environ['swift.content_type_overridden'] = True marker_resp = marker_req.get_response(self.app) @@ -579,7 +579,7 @@ class VersionedWritesContext(WSGIContext): obj_head_headers = {'X-Newest': 'True'} obj_head_headers.update(auth_token_header) head_req = make_pre_authed_request( - req.environ, path=req.path_info, method='HEAD', + req.environ, path=quote(req.path_info), method='HEAD', headers=obj_head_headers, swift_source='VW') hresp = head_req.get_response(self.app) close_if_possible(hresp.app_iter) @@ -604,8 +604,9 @@ class VersionedWritesContext(WSGIContext): continue old_del_req = make_pre_authed_request( - req.environ, path=restored_path, method='DELETE', - headers=auth_token_header, swift_source='VW') + req.environ, path=quote(restored_path), + method='DELETE', headers=auth_token_header, + swift_source='VW') del_resp = old_del_req.get_response(self.app) close_if_possible(del_resp.app_iter) if del_resp.status_int != HTTP_NOT_FOUND: @@ -618,7 +619,7 @@ class VersionedWritesContext(WSGIContext): previous_version['name'].encode('utf-8')) # done restoring, redirect the delete to the marker req = make_pre_authed_request( - req.environ, path=marker_path, method='DELETE', + req.environ, path=quote(marker_path), method='DELETE', headers=auth_token_header, swift_source='VW') else: # there are older versions so copy the previous version to the @@ -634,7 +635,7 @@ class VersionedWritesContext(WSGIContext): # version object - we already auth'd original req so make a # pre-authed request req = make_pre_authed_request( - req.environ, path=restored_path, method='DELETE', + req.environ, path=quote(restored_path), method='DELETE', headers=auth_token_header, swift_source='VW') # remove 'X-If-Delete-At', since it is not for the older copy @@ -749,9 +750,18 @@ class VersionedWritesMiddleware(object): def object_request(self, req, api_version, account, container, obj, allow_versioned_writes): - account_name = unquote(account) - container_name = unquote(container) - object_name = unquote(obj) + """ + Handle request for object resource. + + Note that account, container, obj should be unquoted by caller + if the url path is under url encoding (e.g. %FF) + + :param req: swift.common.swob.Request instance + :param api_version: should be v1 unless swift bumps api version + :param account: account name string + :param container: container name string + :param object: object name string + """ resp = None is_enabled = config_true_value(allow_versioned_writes) container_info = get_container_info( @@ -779,17 +789,17 @@ class VersionedWritesMiddleware(object): vw_ctx = VersionedWritesContext(self.app, self.logger) if req.method == 'PUT': resp = vw_ctx.handle_obj_versions_put( - req, versions_cont, api_version, account_name, - object_name) + req, versions_cont, api_version, account, + obj) # handle DELETE elif versioning_mode == 'history': resp = vw_ctx.handle_obj_versions_delete_push( - req, versions_cont, api_version, account_name, - container_name, object_name) + req, versions_cont, api_version, account, + container, obj) else: resp = vw_ctx.handle_obj_versions_delete_pop( - req, versions_cont, api_version, account_name, - container_name, object_name) + req, versions_cont, api_version, account, + container, obj) if resp: return resp diff --git a/test/functional/test_versioned_writes.py b/test/functional/test_versioned_writes.py index 0242722355..a4d13a7708 100644 --- a/test/functional/test_versioned_writes.py +++ b/test/functional/test_versioned_writes.py @@ -237,13 +237,13 @@ class TestObjectVersioning(Base): self.assertEqual(self.env.container.info()['versions'], self.env.versions_container.name) - def _test_overwriting_setup(self): + def _test_overwriting_setup(self, obj_name=None): container = self.env.container versions_container = self.env.versions_container cont_info = container.info() self.assertEqual(cont_info['versions'], versions_container.name) expected_content_types = [] - obj_name = Utils.create_name() + obj_name = obj_name or Utils.create_name() versioned_obj = container.file(obj_name) put_headers = {'Content-Type': 'text/jibberish01', @@ -291,11 +291,11 @@ class TestObjectVersioning(Base): # check that POST does not create a new version versioned_obj.sync_metadata(metadata={'fu': 'baz'}) self.assertEqual(1, versions_container.info()['object_count']) - expected_content_types.append('text/jibberish02') # if we overwrite it again, there are two versions versioned_obj.write("ccccc") self.assertEqual(2, versions_container.info()['object_count']) + expected_content_types.append('text/jibberish02') versioned_obj_name = versions_container.files()[1] prev_version = versions_container.file(versioned_obj_name) prev_version.initialize() @@ -371,6 +371,48 @@ class TestObjectVersioning(Base): versioned_obj.delete() self.assertRaises(ResponseError, versioned_obj.read) + def test_overwriting_with_url_encoded_object_name(self): + versions_container = self.env.versions_container + obj_name = Utils.create_name() + '%25ff' + versioned_obj, expected_headers, expected_content_types = \ + self._test_overwriting_setup(obj_name) + + # pop one for the current version + expected_content_types.pop() + self.assertEqual(expected_content_types, [ + o['content_type'] for o in versions_container.files( + parms={'format': 'json'})]) + + # test delete + versioned_obj.delete() + self.assertEqual("ccccc", versioned_obj.read()) + expected_content_types.pop() + self.assertEqual(expected_content_types, [ + o['content_type'] for o in versions_container.files( + parms={'format': 'json'})]) + + versioned_obj.delete() + self.assertEqual("bbbbb", versioned_obj.read()) + expected_content_types.pop() + self.assertEqual(expected_content_types, [ + o['content_type'] for o in versions_container.files( + parms={'format': 'json'})]) + + versioned_obj.delete() + self.assertEqual("aaaaa", versioned_obj.read()) + self.assertEqual(0, versions_container.info()['object_count']) + + # verify that all the original object headers have been copied back + obj_info = versioned_obj.info() + self.assertEqual('text/jibberish01', obj_info['content_type']) + resp_headers = dict(versioned_obj.conn.response.getheaders()) + for k, v in expected_headers.items(): + self.assertIn(k.lower(), resp_headers) + self.assertEqual(v, resp_headers[k.lower()]) + + versioned_obj.delete() + self.assertRaises(ResponseError, versioned_obj.read) + def assert_most_recent_version(self, obj_name, content, should_be_dlo=False): archive_versions = self.env.versions_container.files(parms={ @@ -784,6 +826,57 @@ class TestObjectVersioningHistoryMode(TestObjectVersioning): self.assertEqual(404, cm.exception.status) self.assertEqual(11, versions_container.info()['object_count']) + def test_overwriting_with_url_encoded_object_name(self): + versions_container = self.env.versions_container + obj_name = Utils.create_name() + '%25ff' + versioned_obj, expected_headers, expected_content_types = \ + self._test_overwriting_setup(obj_name) + + # test delete + # at first, delete will succeed with 204 + versioned_obj.delete() + expected_content_types.append( + 'application/x-deleted;swift_versions_deleted=1') + # after that, any time the delete doesn't restore the old version + # and we will get 404 NotFound + for x in range(3): + with self.assertRaises(ResponseError) as cm: + versioned_obj.delete() + self.assertEqual(404, cm.exception.status) + expected_content_types.append( + 'application/x-deleted;swift_versions_deleted=1') + # finally, we have 4 versioned items and 4 delete markers total in + # the versions container + self.assertEqual(8, versions_container.info()['object_count']) + self.assertEqual(expected_content_types, [ + o['content_type'] for o in versions_container.files( + parms={'format': 'json'})]) + + # update versioned_obj + versioned_obj.write("eeee", hdrs={'Content-Type': 'text/thanksgiving', + 'X-Object-Meta-Bar': 'foo'}) + # verify the PUT object is kept successfully + obj_info = versioned_obj.info() + self.assertEqual('text/thanksgiving', obj_info['content_type']) + + # we still have delete-marker there + self.assertEqual(8, versions_container.info()['object_count']) + + # update versioned_obj + versioned_obj.write("ffff", hdrs={'Content-Type': 'text/teriyaki', + 'X-Object-Meta-Food': 'chickin'}) + # verify the PUT object is kept successfully + obj_info = versioned_obj.info() + self.assertEqual('text/teriyaki', obj_info['content_type']) + + # new obj will be inserted after delete-marker there + self.assertEqual(9, versions_container.info()['object_count']) + + versioned_obj.delete() + with self.assertRaises(ResponseError) as cm: + versioned_obj.read() + self.assertEqual(404, cm.exception.status) + def test_versioning_dlo(self): obj_name, man_file = \ self._test_versioning_dlo_setup() diff --git a/test/unit/common/middleware/test_versioned_writes.py b/test/unit/common/middleware/test_versioned_writes.py index 4346d7f340..ea11168bc1 100644 --- a/test/unit/common/middleware/test_versioned_writes.py +++ b/test/unit/common/middleware/test_versioned_writes.py @@ -354,6 +354,30 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): self.assertEqual(['VW', None], self.app.swift_sources) self.assertEqual({'fake_trans_id'}, set(self.app.txn_ids)) + def test_put_versioned_object_including_url_encoded_name_success(self): + self.app.register( + 'PUT', '/v1/a/c/%ff', swob.HTTPOk, {}, 'passed') + self.app.register( + 'GET', '/v1/a/c/%ff', swob.HTTPNotFound, {}, None) + + cache = FakeCache({'sysmeta': {'versions-location': 'ver_cont'}}) + req = Request.blank( + '/v1/a/c/%25ff', + environ={'REQUEST_METHOD': 'PUT', 'swift.cache': cache, + 'CONTENT_LENGTH': '100', + 'swift.trans_id': 'fake_trans_id'}) + status, headers, body = self.call_vw(req) + self.assertEqual(status, '200 OK') + self.assertEqual(len(self.authorized), 2) + # Versioned writes middleware now calls auth on the incoming request + # before we try the GET and then at the proxy, so there are 2 + # atuhorized for the same request. + self.assertRequestEqual(req, self.authorized[0]) + self.assertRequestEqual(req, self.authorized[1]) + self.assertEqual(2, self.app.call_count) + self.assertEqual(['VW', None], self.app.swift_sources) + self.assertEqual({'fake_trans_id'}, set(self.app.txn_ids)) + def test_put_object_no_versioning_with_container_config_true(self): # set False to versions_write and expect no GET occurred self.vw.conf = {'allow_versioned_writes': 'false'}