diff --git a/swift/account/server.py b/swift/account/server.py index 3bdf37f156..cf160e9e21 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -156,7 +156,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: @@ -249,7 +249,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 b41239e6f3..eacc6c8d32 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -101,7 +101,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 daad589ed4..4c325b1ed7 100644 --- a/swift/common/db.py +++ b/swift/common/db.py @@ -30,9 +30,11 @@ from tempfile import mkstemp from eventlet import sleep, Timeout import sqlite3 +from swift.common.constraints import MAX_META_COUNT, MAX_META_OVERALL_SIZE from swift.common.utils import json, Timestamp, renamer, \ mkdirs, lock_parent_directory, fallocate from swift.common.exceptions import LockTimeout +from swift.common.swob import HTTPBadRequest #: Whether calls will be made to preallocate disk space for database files. @@ -719,7 +721,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 @@ -752,6 +782,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 76b7771ac4..6ef713188d 100644 --- a/swift/container/server.py +++ b/swift/container/server.py @@ -374,7 +374,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 @@ -551,7 +551,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 a739259fca..30a8e74184 100755 --- a/test/functional/test_account.py +++ b/test/functional/test_account.py @@ -774,6 +774,21 @@ class TestAccount(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_bad_metadata2(self): + if tf.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(self.max_meta_count): headers['X-Account-Meta-%d' % x] = 'v' @@ -787,6 +802,16 @@ class TestAccount(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_bad_metadata3(self): + if tf.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) + headers = {} header_value = 'k' * self.max_meta_value_length size = 0 diff --git a/test/functional/test_container.py b/test/functional/test_container.py index bfd41fb104..d7896a42e7 100755 --- a/test/functional/test_container.py +++ b/test/functional/test_container.py @@ -401,6 +401,16 @@ class TestContainer(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_POST_bad_metadata2(self): + if tf.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(self.max_meta_count): headers['X-Container-Meta-%d' % x] = 'v' @@ -414,6 +424,16 @@ class TestContainer(unittest.TestCase): resp.read() self.assertEqual(resp.status, 400) + def test_POST_bad_metadata3(self): + if tf.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' * self.max_meta_value_length size = 0 diff --git a/test/unit/common/test_db.py b/test/unit/common/test_db.py index e0c164de9b..d4f9828548 100644 --- a/test/unit/common/test_db.py +++ b/test/unit/common/test_db.py @@ -32,11 +32,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, PICKLE_PROTOCOL from swift.common.utils import normalize_timestamp, mkdirs, json, Timestamp from swift.common.exceptions import LockTimeout +from swift.common.swob import HTTPException from test.unit import with_tempdir @@ -655,7 +658,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] @@ -1107,6 +1110,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()