From 9dcf15f8b50188c592acc4c34e333c738173d516 Mon Sep 17 00:00:00 2001 From: Thiago da Silva Date: Thu, 21 Aug 2014 10:33:30 -0400 Subject: [PATCH] moving object validation checks to top of PUT method This adds a sanity check on x-delete headers as part of check_object_creation method Change-Id: If5069469e433189235b1178ea203b5c8a926f553 Signed-off-by: Thiago da Silva --- swift/common/constraints.py | 69 +++++++++- swift/proxy/controllers/obj.py | 68 ++++------ test/functional/tests.py | 10 ++ test/unit/common/test_constraints.py | 159 +++++++++++++++++++++++- test/unit/proxy/controllers/test_obj.py | 24 +--- 5 files changed, 261 insertions(+), 69 deletions(-) diff --git a/swift/common/constraints.py b/swift/common/constraints.py index 24dcbf034c..20108b36f4 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -15,12 +15,14 @@ import os import urllib +import time from urllib import unquote from ConfigParser import ConfigParser, NoSectionError, NoOptionError from swift.common import utils, exceptions from swift.common.swob import HTTPBadRequest, HTTPLengthRequired, \ - HTTPRequestEntityTooLarge, HTTPPreconditionFailed + HTTPRequestEntityTooLarge, HTTPPreconditionFailed, HTTPNotImplemented, \ + HTTPException MAX_FILE_SIZE = 5368709122 MAX_META_NAME_LENGTH = 128 @@ -154,24 +156,45 @@ def check_object_creation(req, object_name): a chunked request :returns HTTPBadRequest: missing or bad content-type header, or bad metadata + :returns HTTPNotImplemented: unsupported transfer-encoding header value """ - if req.content_length and req.content_length > MAX_FILE_SIZE: + try: + ml = req.message_length() + except ValueError as e: + return HTTPBadRequest(request=req, content_type='text/plain', + body=str(e)) + except AttributeError as e: + return HTTPNotImplemented(request=req, content_type='text/plain', + body=str(e)) + if ml is not None and ml > MAX_FILE_SIZE: return HTTPRequestEntityTooLarge(body='Your request is too large.', request=req, content_type='text/plain') if req.content_length is None and \ req.headers.get('transfer-encoding') != 'chunked': - return HTTPLengthRequired(request=req) + return HTTPLengthRequired(body='Missing Content-Length header.', + request=req, + content_type='text/plain') + if 'X-Copy-From' in req.headers and req.content_length: return HTTPBadRequest(body='Copy requests require a zero byte body', request=req, content_type='text/plain') + if len(object_name) > MAX_OBJECT_NAME_LENGTH: return HTTPBadRequest(body='Object name length of %d longer than %d' % (len(object_name), MAX_OBJECT_NAME_LENGTH), request=req, content_type='text/plain') + if 'Content-Type' not in req.headers: return HTTPBadRequest(request=req, content_type='text/plain', body='No content type') + + try: + req = check_delete_headers(req) + except HTTPException as e: + return HTTPBadRequest(request=req, body=e.body, + content_type='text/plain') + if not check_utf8(req.headers['Content-Type']): return HTTPBadRequest(request=req, body='Invalid Content-Type', content_type='text/plain') @@ -225,6 +248,46 @@ def valid_timestamp(request): content_type='text/plain') +def check_delete_headers(request): + """ + Validate if 'x-delete' headers are have correct values + values should be positive integers and correspond to + a time in the future. + + :param request: the swob request object + + :returns: HTTPBadRequest in case of invalid values + or None if values are ok + """ + if 'x-delete-after' in request.headers: + try: + x_delete_after = int(request.headers['x-delete-after']) + except ValueError: + raise HTTPBadRequest(request=request, + content_type='text/plain', + body='Non-integer X-Delete-After') + actual_del_time = time.time() + x_delete_after + if actual_del_time < time.time(): + raise HTTPBadRequest(request=request, + content_type='text/plain', + body='X-Delete-After in past') + request.headers['x-delete-at'] = utils.normalize_delete_at_timestamp( + actual_del_time) + + if 'x-delete-at' in request.headers: + try: + x_delete_at = int(utils.normalize_delete_at_timestamp( + int(request.headers['x-delete-at']))) + except ValueError: + raise HTTPBadRequest(request=request, content_type='text/plain', + body='Non-integer X-Delete-At') + + if x_delete_at < time.time(): + raise HTTPBadRequest(request=request, content_type='text/plain', + body='X-Delete-At in past') + return request + + def check_utf8(string): """ Validate if a string is valid UTF-8 str or unicode and that it diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index ec45c0739f..5a094e8346 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -55,7 +55,7 @@ from swift.proxy.controllers.base import Controller, delay_denial, \ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPNotFound, \ HTTPPreconditionFailed, HTTPRequestEntityTooLarge, HTTPRequestTimeout, \ HTTPServerError, HTTPServiceUnavailable, Request, \ - HTTPClientDisconnect, HTTPNotImplemented + HTTPClientDisconnect from swift.common.request_helpers import is_sys_or_user_meta, is_sys_meta, \ remove_items, copy_header_subset @@ -425,27 +425,11 @@ class ObjectController(Controller): delete_at_part = None delete_at_nodes = None - if 'x-delete-after' in req.headers: - try: - x_delete_after = int(req.headers['x-delete-after']) - except ValueError: - raise HTTPBadRequest(request=req, content_type='text/plain', - body='Non-integer X-Delete-After') - - req.headers['x-delete-at'] = normalize_delete_at_timestamp( - time.time() + x_delete_after) + req = constraints.check_delete_headers(req) if 'x-delete-at' in req.headers: - try: - x_delete_at = int(normalize_delete_at_timestamp( - int(req.headers['x-delete-at']))) - except ValueError: - raise HTTPBadRequest(request=req, content_type='text/plain', - body='Non-integer X-Delete-At') - - if x_delete_at < time.time(): - raise HTTPBadRequest(request=req, content_type='text/plain', - body='X-Delete-At in past') + x_delete_at = int(normalize_delete_at_timestamp( + int(req.headers['x-delete-at']))) req.environ.setdefault('swift.log_info', []).append( 'x-delete-at:%s' % x_delete_at) @@ -490,16 +474,23 @@ class ObjectController(Controller): if not containers: return HTTPNotFound(request=req) - try: - ml = req.message_length() - except ValueError as e: - return HTTPBadRequest(request=req, content_type='text/plain', - body=str(e)) - except AttributeError as e: - return HTTPNotImplemented(request=req, content_type='text/plain', - body=str(e)) - if ml is not None and ml > constraints.MAX_FILE_SIZE: - return HTTPRequestEntityTooLarge(request=req) + # Sometimes the 'content-type' header exists, but is set to None. + content_type_manually_set = True + detect_content_type = \ + config_true_value(req.headers.get('x-detect-content-type')) + if detect_content_type or not req.headers.get('content-type'): + guessed_type, _junk = mimetypes.guess_type(req.path_info) + req.headers['Content-Type'] = guessed_type or \ + 'application/octet-stream' + if detect_content_type: + req.headers.pop('x-detect-content-type') + else: + content_type_manually_set = False + + error_response = check_object_creation(req, self.object_name) or \ + check_content_type(req) + if error_response: + return error_response partition, nodes = obj_ring.get_nodes( self.account_name, self.container_name, self.object_name) @@ -533,23 +524,6 @@ class ObjectController(Controller): else: req.headers['X-Timestamp'] = Timestamp(time.time()).internal - # Sometimes the 'content-type' header exists, but is set to None. - content_type_manually_set = True - detect_content_type = \ - config_true_value(req.headers.get('x-detect-content-type')) - if detect_content_type or not req.headers.get('content-type'): - guessed_type, _junk = mimetypes.guess_type(req.path_info) - req.headers['Content-Type'] = guessed_type or \ - 'application/octet-stream' - if detect_content_type: - req.headers.pop('x-detect-content-type') - else: - content_type_manually_set = False - - error_response = check_object_creation(req, self.object_name) or \ - check_content_type(req) - if error_response: - return error_response if object_versions and not req.environ.get('swift_versioned_copy'): if hresp.status_int != HTTP_NOT_FOUND: # This is a version manifest and needs to be handled diff --git a/test/functional/tests.py b/test/functional/tests.py index 399200b92e..1bac305101 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -1189,6 +1189,16 @@ class TestFile(Base): cfg={'no_content_length': True}) self.assert_status(400) + # no content-length + self.assertRaises(ResponseError, file_item.write_random, file_length, + cfg={'no_content_length': True}) + self.assert_status(411) + + self.assertRaises(ResponseError, file_item.write_random, file_length, + hdrs={'transfer-encoding': 'gzip,chunked'}, + cfg={'no_content_length': True}) + self.assert_status(501) + # bad request types #for req in ('LICK', 'GETorHEAD_base', 'container_info', # 'best_response'): diff --git a/test/unit/common/test_constraints.py b/test/unit/common/test_constraints.py index 753f858de7..a28de653d1 100644 --- a/test/unit/common/test_constraints.py +++ b/test/unit/common/test_constraints.py @@ -23,7 +23,7 @@ from test.unit import MockTrue from swift.common.swob import HTTPBadRequest, Request, HTTPException from swift.common.http import HTTP_REQUEST_ENTITY_TOO_LARGE, \ - HTTP_BAD_REQUEST, HTTP_LENGTH_REQUIRED + HTTP_BAD_REQUEST, HTTP_LENGTH_REQUIRED, HTTP_NOT_IMPLEMENTED from swift.common import constraints, utils @@ -125,20 +125,68 @@ class TestConstraints(unittest.TestCase): 'Content-Type': 'text/plain'} self.assertEquals(constraints.check_object_creation(Request.blank( '/', headers=headers), 'object_name'), None) + headers = {'Content-Length': str(constraints.MAX_FILE_SIZE + 1), 'Content-Type': 'text/plain'} self.assertEquals(constraints.check_object_creation( Request.blank('/', headers=headers), 'object_name').status_int, HTTP_REQUEST_ENTITY_TOO_LARGE) + headers = {'Transfer-Encoding': 'chunked', 'Content-Type': 'text/plain'} self.assertEquals(constraints.check_object_creation(Request.blank( '/', headers=headers), 'object_name'), None) + + headers = {'Transfer-Encoding': 'gzip', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name').status_int, + HTTP_BAD_REQUEST) + headers = {'Content-Type': 'text/plain'} self.assertEquals(constraints.check_object_creation( Request.blank('/', headers=headers), 'object_name').status_int, HTTP_LENGTH_REQUIRED) + headers = {'Content-Length': 'abc', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name').status_int, + HTTP_BAD_REQUEST) + + headers = {'Transfer-Encoding': 'gzip,chunked', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name').status_int, + HTTP_NOT_IMPLEMENTED) + + def test_check_object_creation_copy(self): + headers = {'Content-Length': '0', + 'X-Copy-From': 'c/o2', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name'), None) + + headers = {'Content-Length': '1', + 'X-Copy-From': 'c/o2', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name').status_int, + HTTP_BAD_REQUEST) + + headers = {'Transfer-Encoding': 'chunked', + 'X-Copy-From': 'c/o2', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name'), None) + + # a content-length header is always required + headers = {'X-Copy-From': 'c/o2', + 'Content-Type': 'text/plain'} + self.assertEquals(constraints.check_object_creation(Request.blank( + '/', headers=headers), 'object_name').status_int, + HTTP_LENGTH_REQUIRED) + def test_check_object_creation_name_length(self): headers = {'Transfer-Encoding': 'chunked', 'Content-Type': 'text/plain'} @@ -168,6 +216,115 @@ class TestConstraints(unittest.TestCase): self.assertEquals(resp.status_int, HTTP_BAD_REQUEST) self.assert_('Content-Type' in resp.body) + def test_check_object_creation_bad_delete_headers(self): + headers = {'Transfer-Encoding': 'chunked', + 'Content-Type': 'text/plain', + 'X-Delete-After': 'abc'} + resp = constraints.check_object_creation( + Request.blank('/', headers=headers), 'object_name') + self.assertEquals(resp.status_int, HTTP_BAD_REQUEST) + self.assert_('Non-integer X-Delete-After' in resp.body) + + t = str(int(time.time() - 60)) + headers = {'Transfer-Encoding': 'chunked', + 'Content-Type': 'text/plain', + 'X-Delete-At': t} + resp = constraints.check_object_creation( + Request.blank('/', headers=headers), 'object_name') + self.assertEquals(resp.status_int, HTTP_BAD_REQUEST) + self.assert_('X-Delete-At in past' in resp.body) + + def test_check_delete_headers(self): + + # X-Delete-After + headers = {'X-Delete-After': '60'} + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + self.assertTrue(isinstance(resp, Request)) + self.assertTrue('x-delete-at' in resp.headers) + + headers = {'X-Delete-After': 'abc'} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('Non-integer X-Delete-After' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + headers = {'X-Delete-After': '60.1'} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('Non-integer X-Delete-After' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + headers = {'X-Delete-After': '-1'} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('X-Delete-After in past' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + # X-Delete-At + t = str(int(time.time() + 100)) + headers = {'X-Delete-At': t} + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + self.assertTrue(isinstance(resp, Request)) + self.assertTrue('x-delete-at' in resp.headers) + self.assertEquals(resp.headers.get('X-Delete-At'), t) + + headers = {'X-Delete-At': 'abc'} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('Non-integer X-Delete-At' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + t = str(int(time.time() + 100)) + '.1' + headers = {'X-Delete-At': t} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('Non-integer X-Delete-At' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + t = str(int(time.time())) + headers = {'X-Delete-At': t} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('X-Delete-At in past' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + + t = str(int(time.time() - 1)) + headers = {'X-Delete-At': t} + try: + resp = constraints.check_delete_headers( + Request.blank('/', headers=headers)) + except HTTPException as e: + self.assertEquals(e.status_int, HTTP_BAD_REQUEST) + self.assertTrue('X-Delete-At in past' in e.body) + else: + self.fail("Should have failed with HTTPBadRequest") + def test_check_mount(self): self.assertFalse(constraints.check_mount('', '')) with mock.patch("swift.common.utils.ismount", MockTrue()): diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py index ac7313502a..199a07cfef 100755 --- a/test/unit/proxy/controllers/test_obj.py +++ b/test/unit/proxy/controllers/test_obj.py @@ -279,10 +279,7 @@ class TestObjController(unittest.TestCase): req = swob.Request.blank('/v1/a/c/o', method='POST', headers={'Content-Type': 'foo/bar', 'X-Delete-After': t}) - x_newest_responses = [200] * self.obj_ring.replicas + \ - [404] * self.obj_ring.max_more_nodes - with set_http_connect(*x_newest_responses): - resp = req.get_response(self.app) + resp = req.get_response(self.app) self.assertEqual(resp.status_int, 400) self.assertEqual('Non-integer X-Delete-After', resp.body) @@ -290,22 +287,16 @@ class TestObjController(unittest.TestCase): req = swob.Request.blank('/v1/a/c/o', method='POST', headers={'Content-Type': 'foo/bar', 'X-Delete-After': '-60'}) - x_newest_responses = [200] * self.obj_ring.replicas + \ - [404] * self.obj_ring.max_more_nodes - with set_http_connect(*x_newest_responses): - resp = req.get_response(self.app) + resp = req.get_response(self.app) self.assertEqual(resp.status_int, 400) - self.assertEqual('X-Delete-At in past', resp.body) + self.assertEqual('X-Delete-After in past', resp.body) def test_POST_delete_at_non_integer(self): t = str(int(time.time() + 100)) + '.1' req = swob.Request.blank('/v1/a/c/o', method='POST', headers={'Content-Type': 'foo/bar', 'X-Delete-At': t}) - x_newest_responses = [200] * self.obj_ring.replicas + \ - [404] * self.obj_ring.max_more_nodes - with set_http_connect(*x_newest_responses): - resp = req.get_response(self.app) + resp = req.get_response(self.app) self.assertEqual(resp.status_int, 400) self.assertEqual('Non-integer X-Delete-At', resp.body) @@ -314,10 +305,7 @@ class TestObjController(unittest.TestCase): req = swob.Request.blank('/v1/a/c/o', method='POST', headers={'Content-Type': 'foo/bar', 'X-Delete-At': t}) - x_newest_responses = [200] * self.obj_ring.replicas + \ - [404] * self.obj_ring.max_more_nodes - with set_http_connect(*x_newest_responses): - resp = req.get_response(self.app) + resp = req.get_response(self.app) self.assertEqual(resp.status_int, 400) self.assertEqual('X-Delete-At in past', resp.body) @@ -363,7 +351,7 @@ class TestObjController(unittest.TestCase): with set_http_connect(): resp = req.get_response(self.app) self.assertEqual(resp.status_int, 400) - self.assertEqual('X-Delete-At in past', resp.body) + self.assertEqual('X-Delete-After in past', resp.body) def test_PUT_delete_at(self): t = str(int(time.time() + 100))