diff --git a/swift/account/server.py b/swift/account/server.py index 53889a16f1..9a5084d157 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -153,7 +153,7 @@ class AccountController(object): for key, value in req.headers.iteritems() if is_sys_or_user_meta('account', key)) if metadata: - broker.update_metadata(metadata) + broker.update_metadata(metadata, validate_metadata=True) if created: return HTTPCreated(request=req) else: @@ -259,7 +259,7 @@ class AccountController(object): for key, value in req.headers.iteritems() if is_sys_or_user_meta('account', key)) if metadata: - broker.update_metadata(metadata) + broker.update_metadata(metadata, validate_metadata=True) return HTTPNoContent(request=req) def __call__(self, env, start_response): diff --git a/swift/common/constraints.py b/swift/common/constraints.py index 7a480eaec0..3495e72bdb 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -92,7 +92,10 @@ FORMAT2CONTENT_TYPE = {'plain': 'text/plain', 'json': 'application/json', def check_metadata(req, target_type): """ - Check metadata sent in the request headers. + Check metadata sent in the request headers. This should only check + that the metadata in the request given is valid. Checks against + account/container overall metadata should be forwarded on to its + respective server to be checked. :param req: request object :param target_type: str: one of: object, container, or account: indicates diff --git a/swift/common/db.py b/swift/common/db.py index 192d17d371..7abdbe3d81 100644 --- a/swift/common/db.py +++ b/swift/common/db.py @@ -31,7 +31,9 @@ import sqlite3 from swift.common.utils import json, normalize_timestamp, renamer, \ mkdirs, lock_parent_directory, fallocate +from swift.common.constraints import MAX_META_COUNT, MAX_META_OVERALL_SIZE from swift.common.exceptions import LockTimeout +from swift.common.swob import HTTPBadRequest #: Whether calls will be made to preallocate disk space for database files. @@ -643,7 +645,35 @@ class DatabaseBroker(object): metadata = {} return metadata - def update_metadata(self, metadata_updates): + @staticmethod + def validate_metadata(metadata): + """ + Validates that metadata_falls within acceptable limits. + + :param metadata: to be validated + :raises: HTTPBadRequest if MAX_META_COUNT or MAX_META_OVERALL_SIZE + is exceeded + """ + meta_count = 0 + meta_size = 0 + for key, (value, timestamp) in metadata.iteritems(): + key = key.lower() + if value != '' and (key.startswith('x-account-meta') or + key.startswith('x-container-meta')): + prefix = 'x-account-meta-' + if key.startswith('x-container-meta-'): + prefix = 'x-container-meta-' + key = key[len(prefix):] + meta_count = meta_count + 1 + meta_size = meta_size + len(key) + len(value) + if meta_count > MAX_META_COUNT: + raise HTTPBadRequest('Too many metadata items; max %d' + % MAX_META_COUNT) + if meta_size > MAX_META_OVERALL_SIZE: + raise HTTPBadRequest('Total metadata too large; max %d' + % MAX_META_OVERALL_SIZE) + + def update_metadata(self, metadata_updates, validate_metadata=False): """ Updates the metadata dict for the database. The metadata dict values are tuples of (value, timestamp) where the timestamp indicates when @@ -676,6 +706,8 @@ class DatabaseBroker(object): value, timestamp = value_timestamp if key not in md or timestamp > md[key][1]: md[key] = value_timestamp + if validate_metadata: + DatabaseBroker.validate_metadata(md) conn.execute('UPDATE %s_stat SET metadata = ?' % self.db_type, (json.dumps(md),)) conn.commit() diff --git a/swift/container/server.py b/swift/container/server.py index ebf729597a..ec4139670f 100644 --- a/swift/container/server.py +++ b/swift/container/server.py @@ -286,7 +286,7 @@ class ContainerController(object): metadata['X-Container-Sync-To'][0] != \ broker.metadata['X-Container-Sync-To'][0]: broker.set_x_container_sync_points(-1, -1) - broker.update_metadata(metadata) + broker.update_metadata(metadata, validate_metadata=True) resp = self.account_update(req, account, container, broker) if resp: return resp @@ -473,7 +473,7 @@ class ContainerController(object): metadata['X-Container-Sync-To'][0] != \ broker.metadata['X-Container-Sync-To'][0]: broker.set_x_container_sync_points(-1, -1) - broker.update_metadata(metadata) + broker.update_metadata(metadata, validate_metadata=True) return HTTPNoContent(request=req) def __call__(self, env, start_response): diff --git a/test/functional/test_account.py b/test/functional/test_account.py index 30cef31ed0..1cc61bc63f 100755 --- a/test/functional/test_account.py +++ b/test/functional/test_account.py @@ -32,6 +32,42 @@ from test.functional.tests import load_constraint class TestAccount(unittest.TestCase): + def setUp(self): + self.max_meta_count = load_constraint('max_meta_count') + self.max_meta_name_length = load_constraint('max_meta_name_length') + self.max_meta_overall_size = load_constraint('max_meta_overall_size') + self.max_meta_value_length = load_constraint('max_meta_value_length') + + def head(url, token, parsed, conn): + conn.request('HEAD', parsed.path, '', {'X-Auth-Token': token}) + return check_response(conn) + resp = retry(head) + self.existing_metadata = set([ + k for k, v in resp.getheaders() if + k.lower().startswith('x-account-meta')]) + + def tearDown(self): + def head(url, token, parsed, conn): + conn.request('HEAD', parsed.path, '', {'X-Auth-Token': token}) + return check_response(conn) + resp = retry(head) + resp.read() + new_metadata = set( + [k for k, v in resp.getheaders() if + k.lower().startswith('x-account-meta')]) + + def clear_meta(url, token, parsed, conn, remove_metadata_keys): + headers = {'X-Auth-Token': token} + headers.update((k, '') for k in remove_metadata_keys) + conn.request('POST', parsed.path, '', headers) + return check_response(conn) + extra_metadata = list(self.existing_metadata ^ new_metadata) + for i in range(0, len(extra_metadata), 90): + batch = extra_metadata[i:i + 90] + resp = retry(clear_meta, batch) + resp.read() + self.assertEqual(resp.status // 100, 2) + def test_metadata(self): if skip: raise SkipTest @@ -733,6 +769,21 @@ class TestAccount(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_bad_metadata2(self): + if skip: + raise SkipTest + + def post(url, token, parsed, conn, extra_headers): + headers = {'X-Auth-Token': token} + headers.update(extra_headers) + conn.request('POST', parsed.path, '', headers) + return check_response(conn) + + # TODO: Find the test that adds these and remove them. + headers = {'x-remove-account-meta-temp-url-key': 'remove', + 'x-remove-account-meta-temp-url-key-2': 'remove'} + resp = retry(post, headers) + headers = {} for x in xrange(MAX_META_COUNT): headers['X-Account-Meta-%d' % x] = 'v' @@ -746,6 +797,21 @@ class TestAccount(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_bad_metadata3(self): + if skip: + raise SkipTest + + def post(url, token, parsed, conn, extra_headers): + headers = {'X-Auth-Token': token} + headers.update(extra_headers) + conn.request('POST', parsed.path, '', headers) + return check_response(conn) + + # TODO: Find the test that adds these and remove them. + headers = {'x-remove-account-meta-temp-url-key': 'remove', + 'x-remove-account-meta-temp-url-key-2': 'remove'} + resp = retry(post, headers) + headers = {} header_value = 'k' * MAX_META_VALUE_LENGTH size = 0 diff --git a/test/functional/test_container.py b/test/functional/test_container.py index 7c0fd3e45c..91702e9892 100755 --- a/test/functional/test_container.py +++ b/test/functional/test_container.py @@ -382,6 +382,16 @@ class TestContainer(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_POST_bad_metadata2(self): + if skip: + raise SkipTest + + def post(url, token, parsed, conn, extra_headers): + headers = {'X-Auth-Token': token} + headers.update(extra_headers) + conn.request('POST', parsed.path + '/' + self.name, '', headers) + return check_response(conn) + headers = {} for x in xrange(MAX_META_COUNT): headers['X-Container-Meta-%d' % x] = 'v' @@ -395,6 +405,16 @@ class TestContainer(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_POST_bad_metadata3(self): + if skip: + raise SkipTest + + def post(url, token, parsed, conn, extra_headers): + headers = {'X-Auth-Token': token} + headers.update(extra_headers) + conn.request('POST', parsed.path + '/' + self.name, '', headers) + return check_response(conn) + headers = {} header_value = 'k' * MAX_META_VALUE_LENGTH size = 0 diff --git a/test/unit/common/test_db.py b/test/unit/common/test_db.py index aa9ae11da4..d62a1d0d3c 100644 --- a/test/unit/common/test_db.py +++ b/test/unit/common/test_db.py @@ -28,11 +28,14 @@ from mock import patch, MagicMock from eventlet.timeout import Timeout import swift.common.db +from swift.common.constraints import \ + MAX_META_VALUE_LENGTH, MAX_META_COUNT, MAX_META_OVERALL_SIZE from swift.common.db import chexor, dict_factory, get_db_connection, \ DatabaseBroker, DatabaseConnectionError, DatabaseAlreadyExists, \ GreenDBConnection from swift.common.utils import normalize_timestamp, mkdirs from swift.common.exceptions import LockTimeout +from swift.common.swob import HTTPException class TestDatabaseConnectionError(unittest.TestCase): @@ -230,7 +233,7 @@ class TestDatabaseBroker(unittest.TestCase): conn.execute('CREATE TABLE test (one TEXT)') conn.execute('CREATE TABLE test_stat (id TEXT)') conn.execute('INSERT INTO test_stat (id) VALUES (?)', - (str(uuid4),)) + (str(uuid4),)) conn.execute('INSERT INTO test (one) VALUES ("1")') conn.commit() stub_called = [False] @@ -679,6 +682,91 @@ class TestDatabaseBroker(unittest.TestCase): [first_value, first_timestamp]) self.assert_('Second' not in broker.metadata) + @patch.object(DatabaseBroker, 'validate_metadata') + def test_validate_metadata_is_called_from_update_metadata(self, mock): + broker = self.get_replication_info_tester(metadata=True) + first_timestamp = normalize_timestamp(1) + first_value = '1' + metadata = {'First': [first_value, first_timestamp]} + broker.update_metadata(metadata, validate_metadata=True) + self.assertTrue(mock.called) + + @patch.object(DatabaseBroker, 'validate_metadata') + def test_validate_metadata_is_not_called_from_update_metadata(self, mock): + broker = self.get_replication_info_tester(metadata=True) + first_timestamp = normalize_timestamp(1) + first_value = '1' + metadata = {'First': [first_value, first_timestamp]} + broker.update_metadata(metadata) + self.assertFalse(mock.called) + + def test_metadata_with_max_count(self): + metadata = {} + for c in xrange(MAX_META_COUNT): + key = 'X-Account-Meta-F{0}'.format(c) + metadata[key] = ('B', normalize_timestamp(1)) + key = 'X-Account-Meta-Foo'.format(c) + metadata[key] = ('', normalize_timestamp(1)) + try: + DatabaseBroker.validate_metadata(metadata) + except HTTPException: + self.fail('Unexpected HTTPException') + + def test_metadata_raises_exception_over_max_count(self): + metadata = {} + for c in xrange(MAX_META_COUNT + 1): + key = 'X-Account-Meta-F{0}'.format(c) + metadata[key] = ('B', normalize_timestamp(1)) + message = '' + try: + DatabaseBroker.validate_metadata(metadata) + except HTTPException as e: + message = str(e) + self.assertEqual(message, '400 Bad Request') + + def test_metadata_with_max_overall_size(self): + metadata = {} + metadata_value = 'v' * MAX_META_VALUE_LENGTH + size = 0 + x = 0 + while size < (MAX_META_OVERALL_SIZE - 4 + - MAX_META_VALUE_LENGTH): + size += 4 + MAX_META_VALUE_LENGTH + metadata['X-Account-Meta-%04d' % x] = (metadata_value, + normalize_timestamp(1)) + x += 1 + if MAX_META_OVERALL_SIZE - size > 1: + metadata['X-Account-Meta-k'] = ( + 'v' * (MAX_META_OVERALL_SIZE - size - 1), + normalize_timestamp(1)) + try: + DatabaseBroker.validate_metadata(metadata) + except HTTPException: + self.fail('Unexpected HTTPException') + + def test_metadata_raises_exception_over_max_overall_size(self): + metadata = {} + metadata_value = 'k' * MAX_META_VALUE_LENGTH + size = 0 + x = 0 + while size < (MAX_META_OVERALL_SIZE - 4 + - MAX_META_VALUE_LENGTH): + size += 4 + MAX_META_VALUE_LENGTH + metadata['X-Account-Meta-%04d' % x] = (metadata_value, + normalize_timestamp(1)) + x += 1 + if MAX_META_OVERALL_SIZE - size > 1: + metadata['X-Account-Meta-k'] = ( + 'v' * (MAX_META_OVERALL_SIZE - size - 1), + normalize_timestamp(1)) + metadata['X-Account-Meta-k2'] = ('v', normalize_timestamp(1)) + message = '' + try: + DatabaseBroker.validate_metadata(metadata) + except HTTPException as e: + message = str(e) + self.assertEqual(message, '400 Bad Request') + if __name__ == '__main__': unittest.main()