From fe70898daced5f5fd698dd27098451439fdd8d08 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Sun, 28 Feb 2016 01:18:07 +0000 Subject: [PATCH] Require account/container metadata be UTF-8 Otherwise, we get a UnicodeDecodeError when we call json.dumps() Change-Id: Ie200d029e1fd7f0ff0956c8ced98207e11ef9080 --- swift/common/constraints.py | 8 +++++++- swift/common/db.py | 11 +++++++--- test/unit/account/test_server.py | 23 +++++++++++++++++++++ test/unit/common/test_constraints.py | 18 ++++++++++++++--- test/unit/common/test_db.py | 12 +++++++++++ test/unit/container/test_server.py | 30 ++++++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 7 deletions(-) diff --git a/swift/common/constraints.py b/swift/common/constraints.py index 451e7458bf..abfab4bb9e 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -129,7 +129,8 @@ def check_metadata(req, target_type): which type the target storage for the metadata is :returns: HTTPBadRequest with bad metadata otherwise None """ - prefix = 'x-%s-meta-' % target_type.lower() + target_type = target_type.lower() + prefix = 'x-%s-meta-' % target_type meta_count = 0 meta_size = 0 for key, value in req.headers.items(): @@ -145,6 +146,11 @@ def check_metadata(req, target_type): if not key: return HTTPBadRequest(body='Metadata name cannot be empty', request=req, content_type='text/plain') + bad_key = not check_utf8(key) + bad_value = value and not check_utf8(value) + if target_type in ('account', 'container') and (bad_key or bad_value): + return HTTPBadRequest(body='Metadata must be valid UTF-8', + request=req, content_type='text/plain') meta_count += 1 meta_size += len(key) + len(value) if len(key) > MAX_META_NAME_LENGTH: diff --git a/swift/common/db.py b/swift/common/db.py index cead803375..092018d6b3 100644 --- a/swift/common/db.py +++ b/swift/common/db.py @@ -32,7 +32,8 @@ 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.constraints import MAX_META_COUNT, MAX_META_OVERALL_SIZE, \ + check_utf8 from swift.common.utils import Timestamp, renamer, \ mkdirs, lock_parent_directory, fallocate from swift.common.exceptions import LockTimeout @@ -729,11 +730,11 @@ class DatabaseBroker(object): @staticmethod def validate_metadata(metadata): """ - Validates that metadata_falls within acceptable limits. + 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 + is exceeded, or if metadata contains non-UTF-8 data """ meta_count = 0 meta_size = 0 @@ -747,6 +748,10 @@ class DatabaseBroker(object): key = key[len(prefix):] meta_count = meta_count + 1 meta_size = meta_size + len(key) + len(value) + bad_key = key and not check_utf8(key) + bad_value = value and not check_utf8(value) + if bad_key or bad_value: + raise HTTPBadRequest('Metadata must be valid UTF-8') if meta_count > MAX_META_COUNT: raise HTTPBadRequest('Too many metadata items; max %d' % MAX_META_COUNT) diff --git a/test/unit/account/test_server.py b/test/unit/account/test_server.py index db4f09cc1f..d2e7c087ef 100644 --- a/test/unit/account/test_server.py +++ b/test/unit/account/test_server.py @@ -383,6 +383,29 @@ class TestAccountController(unittest.TestCase): self.assertEqual(resp.body, 'Recently deleted') self.assertEqual(resp.headers['X-Account-Status'], 'Deleted') + def test_PUT_non_utf8_metadata(self): + # Set metadata header + req = Request.blank( + '/sda1/p/a', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(1), + 'X-Account-Meta-Test': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 400) + # Set sysmeta header + req = Request.blank( + '/sda1/p/a', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(1), + 'X-Account-Sysmeta-Access-Control': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 400) + # Send other + req = Request.blank( + '/sda1/p/a', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': normalize_timestamp(1), + 'X-Will-Not-Be-Saved': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 202) + def test_PUT_GET_metadata(self): # Set metadata header req = Request.blank( diff --git a/test/unit/common/test_constraints.py b/test/unit/common/test_constraints.py index b250907527..2f7fb85d9b 100644 --- a/test/unit/common/test_constraints.py +++ b/test/unit/common/test_constraints.py @@ -22,7 +22,7 @@ from six.moves import range from test import safe_repr from test.unit import MockTrue -from swift.common.swob import HTTPBadRequest, Request, HTTPException +from swift.common.swob import Request, HTTPException from swift.common.http import HTTP_REQUEST_ENTITY_TOO_LARGE, \ HTTP_BAD_REQUEST, HTTP_LENGTH_REQUIRED, HTTP_NOT_IMPLEMENTED from swift.common import constraints, utils @@ -49,8 +49,20 @@ class TestConstraints(unittest.TestCase): def test_check_metadata_empty_name(self): headers = {'X-Object-Meta-': 'Value'} - self.assertTrue(constraints.check_metadata(Request.blank( - '/', headers=headers), 'object'), HTTPBadRequest) + self.assertEqual(constraints.check_metadata(Request.blank( + '/', headers=headers), 'object').status_int, HTTP_BAD_REQUEST) + + def test_check_metadata_non_utf8(self): + headers = {'X-Account-Meta-Foo': b'\xff'} + self.assertEqual(constraints.check_metadata(Request.blank( + '/', headers=headers), 'account').status_int, HTTP_BAD_REQUEST) + headers = {b'X-Container-Meta-\xff': 'foo'} + self.assertEqual(constraints.check_metadata(Request.blank( + '/', headers=headers), 'container').status_int, HTTP_BAD_REQUEST) + # Object's OK; its metadata isn't serialized as JSON + headers = {'X-Object-Meta-Foo': b'\xff'} + self.assertIsNone(constraints.check_metadata(Request.blank( + '/', headers=headers), 'object')) def test_check_metadata_name_length(self): name = 'a' * constraints.MAX_META_NAME_LENGTH diff --git a/test/unit/common/test_db.py b/test/unit/common/test_db.py index 925e71438a..7e7660e77c 100644 --- a/test/unit/common/test_db.py +++ b/test/unit/common/test_db.py @@ -1147,6 +1147,18 @@ class TestDatabaseBroker(unittest.TestCase): except HTTPException: self.fail('Unexpected HTTPException') + def test_metadata_raises_exception_on_non_utf8(self): + def try_validate(metadata): + try: + DatabaseBroker.validate_metadata(metadata) + except HTTPException as e: + self.assertEqual(str(e), '400 Bad Request') + else: + self.fail('HTTPException not raised') + ts = normalize_timestamp(1) + try_validate({'X-Account-Meta-Foo': (b'\xff', ts)}) + try_validate({b'X-Container-Meta-\xff': ('bar', ts)}) + def test_metadata_raises_exception_over_max_count(self): metadata = {} for c in range(MAX_META_COUNT + 1): diff --git a/test/unit/container/test_server.py b/test/unit/container/test_server.py index 22e0f00c41..4646b87fae 100644 --- a/test/unit/container/test_server.py +++ b/test/unit/container/test_server.py @@ -632,6 +632,36 @@ class TestContainerController(unittest.TestCase): self.assertEqual(resp.headers['X-Backend-Storage-Policy-Index'], str(non_default_policy.idx)) + def test_PUT_non_utf8_metadata(self): + # Set metadata header + req = Request.blank( + '/sda1/p/a/c', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': Timestamp(1).internal, + 'X-Container-Meta-Test': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 400) + # Set sysmeta header + req = Request.blank( + '/sda1/p/a/c', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': Timestamp(1).internal, + 'X-Container-Sysmeta-Test': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 400) + # Set ACL + req = Request.blank( + '/sda1/p/a/c', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': Timestamp(1).internal, + 'X-Container-Read': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 400) + # Send other + req = Request.blank( + '/sda1/p/a/c', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': Timestamp(1).internal, + 'X-Will-Not-Be-Saved': b'\xff'}) + resp = req.get_response(self.controller) + self.assertEqual(resp.status_int, 202) + def test_PUT_GET_metadata(self): # Set metadata header req = Request.blank(