From fa889358acf7675efba2de095777886b4c1ff7f8 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Wed, 20 Nov 2024 10:59:43 -0600 Subject: [PATCH] Ensure correct content-type in container HEAD response A failing CORS test in the gate discovered that we were responding application/json to ?format=txt requests (which is maybe not even a valid value for that qs param?), but only when running with eventlet==0.38.0 This avoids the problem of backend container server HEADs no longer having 'Content-Length: 0' by fixing the client HEAD resp headers before we check for chunked-transfer resp. Drive-By: refactor listing_formats to use HeaderKeyDict and always set Content-Length explicitly Co-Authored-By: Alistair Coles Co-Authored-By: Matthew Oliver Change-Id: If724485e1425d1481d10b9255436301e346f07e8 --- swift/common/middleware/listing_formats.py | 66 ++++++-------- test/functional/test_account.py | 41 ++++++++- test/functional/test_container.py | 66 +++++++++++++- .../common/middleware/test_listing_formats.py | 89 +++++++++++++++++-- 4 files changed, 213 insertions(+), 49 deletions(-) diff --git a/swift/common/middleware/listing_formats.py b/swift/common/middleware/listing_formats.py index bf402ff75e..643341afb7 100644 --- a/swift/common/middleware/listing_formats.py +++ b/swift/common/middleware/listing_formats.py @@ -18,6 +18,7 @@ import six from xml.etree.cElementTree import Element, SubElement, tostring from swift.common.constraints import valid_api_version +from swift.common.header_key_dict import HeaderKeyDict from swift.common.http import HTTP_NO_CONTENT from swift.common.request_helpers import get_param from swift.common.swob import HTTPException, HTTPNotAcceptable, Request, \ @@ -178,52 +179,39 @@ class ListingFilter(object): start_response(status, headers) return resp_iter - header_to_index = {} - resp_content_type = resp_length = None - for i, (header, value) in enumerate(headers): - header = header.lower() - if header == 'content-type': - header_to_index[header] = i - resp_content_type = value.partition(';')[0] - elif header == 'content-length': - header_to_index[header] = i - resp_length = int(value) - elif header == 'vary': - header_to_index[header] = i - if not status.startswith(('200 ', '204 ')): start_response(status, headers) return resp_iter + headers_dict = HeaderKeyDict(headers) + resp_content_type = headers_dict.get( + 'content-type', '').partition(';')[0] + resp_length = headers_dict.get('content-length') + if can_vary: - if 'vary' in header_to_index: - value = headers[header_to_index['vary']][1] + if 'vary' in headers_dict: + value = headers_dict['vary'] if 'accept' not in list_from_csv(value.lower()): - headers[header_to_index['vary']] = ( - 'Vary', value + ', Accept') + headers_dict['vary'] = value + ', Accept' else: - headers.append(('Vary', 'Accept')) + headers_dict['vary'] = 'Accept' if resp_content_type != 'application/json': - start_response(status, headers) + start_response(status, list(headers_dict.items())) + return resp_iter + + if req.method == 'HEAD': + headers_dict['content-type'] = out_content_type + '; charset=utf-8' + # proxy logging (and maybe other mw?) seem to be good about + # sticking this on HEAD/204 but we do it here to be responsible + # and explicit + headers_dict['content-length'] = 0 + start_response(status, list(headers_dict.items())) return resp_iter if resp_length is None or \ - resp_length > MAX_CONTAINER_LISTING_CONTENT_LENGTH: - start_response(status, headers) - return resp_iter - - def set_header(header, value): - if value is None: - del headers[header_to_index[header]] - else: - headers[header_to_index[header]] = ( - headers[header_to_index[header]][0], str(value)) - - if req.method == 'HEAD': - set_header('content-type', out_content_type + '; charset=utf-8') - set_header('content-length', None) # don't know, can't determine - start_response(status, headers) + int(resp_length) > MAX_CONTAINER_LISTING_CONTENT_LENGTH: + start_response(status, list(headers_dict.items())) return resp_iter body = b''.join(resp_iter) @@ -237,7 +225,7 @@ class ListingFilter(object): except ValueError: # Static web listing that's returning invalid JSON? # Just pass it straight through; that's about all we *can* do. - start_response(status, headers) + start_response(status, list(headers_dict.items())) return [body] if not req.allow_reserved_names: @@ -257,16 +245,16 @@ class ListingFilter(object): body = json.dumps(listing).encode('ascii') except KeyError: # listing was in a bad format -- funky static web listing?? - start_response(status, headers) + start_response(status, list(headers_dict.items())) return [body] if not body: status = '%s %s' % (HTTP_NO_CONTENT, RESPONSE_REASONS[HTTP_NO_CONTENT][0]) - set_header('content-type', out_content_type + '; charset=utf-8') - set_header('content-length', len(body)) - start_response(status, headers) + headers_dict['content-type'] = out_content_type + '; charset=utf-8' + headers_dict['content-length'] = len(body) + start_response(status, list(headers_dict.items())) return [body] diff --git a/test/functional/test_account.py b/test/functional/test_account.py index bd28d9dbbb..dab6ad9908 100644 --- a/test/functional/test_account.py +++ b/test/functional/test_account.py @@ -21,7 +21,7 @@ from uuid import uuid4 from string import ascii_letters import six -from six.moves import range +from six.moves import range, urllib from swift.common.middleware.acl import format_acl from swift.common.utils import distribute_evenly @@ -99,6 +99,45 @@ class TestAccount(unittest.TestCase): new_metadata = self.get_meta().keys() self.clear_meta(new_metadata) + def test_GET_HEAD_content_type(self): + + def send_req(url, token, parsed, conn, method, params): + qs = '?%s' % urllib.parse.urlencode(params) if params else '' + conn.request(method, parsed.path + qs, '', {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(send_req, 'GET', {}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + resp = retry(send_req, 'HEAD', {}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + resp = retry(send_req, 'GET', {'format': 'json'}) + self.assertEqual(resp.status, 200) + self.assertEqual(resp.getheader('Content-Type'), + 'application/json; charset=utf-8') + resp = retry(send_req, 'HEAD', {'format': 'json'}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'application/json; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + resp = retry(send_req, 'GET', {'format': 'xml'}) + self.assertEqual(resp.status, 200) + self.assertEqual(resp.getheader('Content-Type'), + 'application/xml; charset=utf-8') + resp = retry(send_req, 'HEAD', {'format': 'xml'}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'application/xml; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + def test_metadata(self): if tf.skip: raise SkipTest diff --git a/test/functional/test_container.py b/test/functional/test_container.py index b6a4a2384a..879bd0b06e 100644 --- a/test/functional/test_container.py +++ b/test/functional/test_container.py @@ -24,7 +24,7 @@ from test.functional import check_response, cluster_info, retry, \ import test.functional as tf import six -from six.moves import range +from six.moves import range, urllib def setUpModule(): @@ -111,6 +111,70 @@ class TestContainer(unittest.TestCase): # retry despite the request having been successfully processed. self.assertIn(resp.status, (204, 404)) + def test_GET_HEAD_content_type(self): + + def send_req(url, token, parsed, conn, method, container, params): + qs = '?%s' % urllib.parse.urlencode(params) if params else '' + conn.request(method, parsed.path + '/' + container + qs, '', + {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(send_req, 'GET', self.name, {}) + # GET is still 204 if there's no objects!? + self.assertEqual(resp.status, 204) + # we respond text/plain by default + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + resp = retry(send_req, 'HEAD', self.name, {}) + # HEAD will *always* 204 + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + def put_object(url, token, parsed, conn, container, obj_name): + conn.request('PUT', '/'.join((parsed.path, container, obj_name)), + '', {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(put_object, self.name, 'obj1') + self.assertEqual(resp.status, 201) + + resp = retry(send_req, 'GET', self.name, {}) + self.assertEqual(resp.status, 200) + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + + resp = retry(send_req, 'HEAD', self.name, {}) + # HEAD will *always* 204 + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'text/plain; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + # and we can ask for our preferred encoding format + resp = retry(send_req, 'GET', self.name, {'format': 'json'}) + self.assertEqual(resp.status, 200) + self.assertEqual(resp.getheader('Content-Type'), + 'application/json; charset=utf-8') + resp = retry(send_req, 'HEAD', self.name, {'format': 'json'}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'application/json; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + + resp = retry(send_req, 'GET', self.name, {'format': 'xml'}) + self.assertEqual(resp.status, 200) + self.assertEqual(resp.getheader('Content-Type'), + 'application/xml; charset=utf-8') + resp = retry(send_req, 'HEAD', self.name, {'format': 'xml'}) + self.assertEqual(resp.status, 204) + self.assertEqual(resp.getheader('Content-Type'), + 'application/xml; charset=utf-8') + self.assertEqual(resp.getheader('Content-Length'), '0') + def test_multi_metadata(self): if tf.skip: raise SkipTest diff --git a/test/unit/common/middleware/test_listing_formats.py b/test/unit/common/middleware/test_listing_formats.py index a183e9a512..e68bbf6928 100644 --- a/test/unit/common/middleware/test_listing_formats.py +++ b/test/unit/common/middleware/test_listing_formats.py @@ -16,6 +16,7 @@ import json import unittest +from swift.common.header_key_dict import HeaderKeyDict from swift.common.swob import Request, HTTPOk, HTTPNoContent from swift.common.middleware import listing_formats from swift.common.request_helpers import get_reserved_name @@ -108,8 +109,8 @@ class TestListingFormats(unittest.TestCase): def test_valid_content_type_on_txt_head(self): self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { - 'Content-Length': str(len(self.fake_account_listing)), - 'Content-Type': 'application/json'}, self.fake_account_listing) + 'Content-Length': '0', + 'Content-Type': 'application/json'}, b'') req = Request.blank('/v1/a', method='HEAD') resp = req.get_response(self.app) self.assertEqual(resp.body, b'') @@ -122,10 +123,48 @@ class TestListingFormats(unittest.TestCase): self.assertEqual(self.fake_swift.calls[-1], ( 'HEAD', '/v1/a?format=json')) + def test_text_content_type_on_invalid_format_qs(self): + self.fake_swift.register('HEAD', '/v1/a/c', HTTPNoContent, { + 'Content-Type': 'application/json'}, b'') + req = Request.blank('/v1/a/c?format=foo', method='HEAD') + resp = req.get_response(self.app) + self.assertEqual(resp.body, b'') + self.assertEqual(resp.headers['Content-Length'], '0') + self.assertEqual(resp.headers['Content-Type'], + 'text/plain; charset=utf-8') + self.assertEqual(self.fake_swift.calls[-1], ( + 'HEAD', '/v1/a/c?format=json')) + + def test_accept_content_type_on_missing_qs(self): + self.fake_swift.register('HEAD', '/v1/a/c', HTTPNoContent, { + 'Content-Type': 'application/json'}, b'') + req = Request.blank('/v1/a/c', method='HEAD', + headers={'Accept': 'application/xml'}) + resp = req.get_response(self.app) + self.assertEqual(resp.body, b'') + self.assertEqual(resp.headers['Content-Length'], '0') + self.assertEqual(resp.headers['Content-Type'], + 'application/xml; charset=utf-8') + self.assertEqual(self.fake_swift.calls[-1], ( + 'HEAD', '/v1/a/c?format=json')) + + def test_accept_ignored_on_invalid_qs(self): + self.fake_swift.register('HEAD', '/v1/a/c', HTTPNoContent, { + 'Content-Type': 'application/json'}, b'') + req = Request.blank('/v1/a/c?format=foo', method='HEAD', + headers={'Accept': 'application/xml'}) + resp = req.get_response(self.app) + self.assertEqual(resp.body, b'') + self.assertEqual(resp.headers['Content-Length'], '0') + self.assertEqual(resp.headers['Content-Type'], + 'text/plain; charset=utf-8') + self.assertEqual(self.fake_swift.calls[-1], ( + 'HEAD', '/v1/a/c?format=json')) + def test_valid_content_type_on_xml_head(self): self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { - 'Content-Length': str(len(self.fake_account_listing)), - 'Content-Type': 'application/json'}, self.fake_account_listing) + 'Content-Length': '0', + 'Content-Type': 'application/json'}, b'') req = Request.blank('/v1/a?format=xml', method='HEAD') resp = req.get_response(self.app) self.assertEqual(resp.body, b'') @@ -136,11 +175,28 @@ class TestListingFormats(unittest.TestCase): self.assertEqual(self.fake_swift.calls[-1], ( 'HEAD', '/v1/a?format=json')) + def test_valid_content_type_on_xml_head_with_no_content_length(self): + # note: eventlet 0.38.0 stopped including content-length with 204 + # responses + self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { + 'Content-Type': 'application/json'}, b'') + req = Request.blank('/v1/a?format=xml', method='HEAD') + status, headers, body = req.call_application(self.app) + self.assertEqual(b''.join(body), b'') + headers_dict = dict(headers) + self.assertEqual(headers_dict.get('Content-Length'), '0', headers) + self.assertEqual(headers_dict['Content-Type'], + 'application/xml; charset=utf-8') + # query param overrides header, so it won't vary + self.assertNotIn('Vary', HeaderKeyDict(headers_dict)) + self.assertEqual(self.fake_swift.calls[-1], ( + 'HEAD', '/v1/a?format=json')) + def test_update_vary_if_present(self): self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { - 'Content-Length': str(len(self.fake_account_listing)), + 'Content-Length': '0', 'Content-Type': 'application/json', - 'Vary': 'Origin'}, self.fake_account_listing) + 'Vary': 'Origin'}, b'') req = Request.blank('/v1/a', method='HEAD') resp = req.get_response(self.app) self.assertEqual(resp.body, b'') @@ -150,11 +206,28 @@ class TestListingFormats(unittest.TestCase): self.assertEqual(self.fake_swift.calls[-1], ( 'HEAD', '/v1/a?format=json')) + def test_add_vary_when_content_type_not_json(self): + self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { + 'Content-Length': '0', + 'Content-Type': 'text/plain'}, b'') + req = Request.blank('/v1/a', method='HEAD') + resp = req.get_response(self.app) + self.assertEqual(resp.body, b'') + # We actually returned early, we didn't change things in the + # request, but added the vary to let the cache know this + # request could vary based on Accept as we didn't pass in + # a format. + self.assertEqual(resp.headers['Content-Type'], + 'text/plain') + self.assertEqual(resp.headers['Vary'], 'Accept') + self.assertEqual(self.fake_swift.calls[-1], ( + 'HEAD', '/v1/a?format=json')) + def test_update_vary_does_not_duplicate(self): self.fake_swift.register('HEAD', '/v1/a', HTTPNoContent, { - 'Content-Length': str(len(self.fake_account_listing)), + 'Content-Length': '0', 'Content-Type': 'application/json', - 'Vary': 'Accept'}, self.fake_account_listing) + 'Vary': 'Accept'}, b'') req = Request.blank('/v1/a', method='HEAD') resp = req.get_response(self.app) self.assertEqual(resp.body, b'')