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
This commit is contained in:
Kota Tsuyuzaki
parent
9aca9ad780
commit
0e3e7b9b09
@ -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
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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'}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user