diff --git a/glance/api/v2/image_tags.py b/glance/api/v2/image_tags.py index 5ce10fb289..b3124efa3f 100644 --- a/glance/api/v2/image_tags.py +++ b/glance/api/v2/image_tags.py @@ -55,6 +55,10 @@ class Controller(object): msg = _("Not allowed to update tags for image %s.") % image_id LOG.warning(msg) raise webob.exc.HTTPForbidden(explanation=msg) + except exception.Invalid as e: + msg = _("Could not update image: %s") % utils.exception_to_str(e) + LOG.warning(msg) + raise webob.exc.HTTPBadRequest(explanation=msg) except exception.ImageTagLimitExceeded as e: msg = (_("Image tag limit exceeded for image %(id)s: %(e)s:") % {"id": image_id, "e": utils.exception_to_str(e)}) diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 87f1d878b9..11966a4303 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -65,6 +65,8 @@ class ImagesController(object): image_repo.add(image) except exception.DuplicateLocation as dup: raise webob.exc.HTTPBadRequest(explanation=dup.msg) + except exception.Invalid as e: + raise webob.exc.HTTPBadRequest(explanation=e.msg) except exception.Forbidden as e: raise webob.exc.HTTPForbidden(explanation=e.msg) except exception.InvalidParameterValue as e: @@ -138,6 +140,8 @@ class ImagesController(object): image_repo.save(image) except exception.NotFound as e: raise webob.exc.HTTPNotFound(explanation=e.msg) + except exception.Invalid as e: + raise webob.exc.HTTPBadRequest(explanation=e.msg) except exception.Forbidden as e: raise webob.exc.HTTPForbidden(explanation=e.msg) except exception.InvalidParameterValue as e: diff --git a/glance/common/utils.py b/glance/common/utils.py index 3757a965b4..cbfa09b392 100644 --- a/glance/common/utils.py +++ b/glance/common/utils.py @@ -627,3 +627,49 @@ def exception_to_str(exc): error = ("Caught '%(exception)s' exception." % {"exception": exc.__class__.__name__}) return encodeutils.safe_encode(error, errors='ignore') + + +try: + REGEX_4BYTE_UNICODE = re.compile(u'[\U00010000-\U0010ffff]') +except re.error: + # UCS-2 build case + REGEX_4BYTE_UNICODE = re.compile(u'[\uD800-\uDBFF][\uDC00-\uDFFF]') + + +def no_4byte_params(f): + """ + Checks that no 4 byte unicode characters are allowed + in dicts' keys/values and string's parameters + """ + def wrapper(*args, **kwargs): + + def _is_match(some_str): + return (isinstance(some_str, unicode) and + REGEX_4BYTE_UNICODE.findall(some_str) != []) + + def _check_dict(data_dict): + # a dict of dicts has to be checked recursively + for key, value in data_dict.iteritems(): + if isinstance(value, dict): + _check_dict(value) + else: + if _is_match(key): + msg = _("Property names can't contain 4 byte unicode.") + raise exception.Invalid(msg) + if _is_match(value): + msg = (_("%s can't contain 4 byte unicode characters.") + % key.title()) + raise exception.Invalid(msg) + + for data_dict in [arg for arg in args if isinstance(arg, dict)]: + _check_dict(data_dict) + # now check args for str values + for arg in args: + if _is_match(arg): + msg = _("Param values can't contain 4 byte unicode.") + raise exception.Invalid(msg) + # check kwargs as well, as params are passed as kwargs via + # registry calls + _check_dict(kwargs) + return f(*args, **kwargs) + return wrapper diff --git a/glance/db/registry/api.py b/glance/db/registry/api.py index 0cc0cc5b01..dc947d5341 100644 --- a/glance/db/registry/api.py +++ b/glance/db/registry/api.py @@ -233,6 +233,12 @@ def image_location_delete(client, image_id, location_id, status, session=None): status=status) +@_get_client +def image_location_update(client, image_id, location, session=None): + """Update image location.""" + client.image_location_update(image_id=image_id, location=location) + + @_get_client def user_get_storage_usage(client, owner_id, image_id=None, session=None): return client.user_get_storage_usage(owner_id=owner_id, image_id=image_id) diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index 356ff74c81..2b3ddef486 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -22,10 +22,10 @@ from oslo.utils import timeutils import six from glance.common import exception +from glance.common import utils from glance import i18n import glance.openstack.common.log as logging - LOG = logging.getLogger(__name__) _ = i18n._ _LI = i18n._LI @@ -94,6 +94,7 @@ def _get_session(): return DATA +@utils.no_4byte_params def _image_location_format(image_id, value, meta_data, status, deleted=False): dt = timeutils.utcnow() return { @@ -179,6 +180,20 @@ def _task_info_format(task_id, **values): return task_info +@utils.no_4byte_params +def _image_update(image, values, properties): + # NOTE(bcwaldon): store properties as a list to match sqlalchemy driver + properties = [{'name': k, + 'value': v, + 'image_id': image['id'], + 'deleted': False} for k, v in properties.items()] + if 'properties' not in image.keys(): + image['properties'] = [] + image['properties'].extend(properties) + image.update(values) + return image + + def _image_format(image_id, **values): dt = timeutils.utcnow() image = { @@ -214,16 +229,7 @@ def _image_format(image_id, **values): image['locations'].append(location_ref) DATA['locations'].append(location_ref) - # NOTE(bcwaldon): store properties as a list to match sqlalchemy driver - properties = values.pop('properties', {}) - properties = [{'name': k, - 'value': v, - 'image_id': image_id, - 'deleted': False} for k, v in properties.items()] - image['properties'] = properties - - image.update(values) - return image + return _image_update(image, values, values.pop('properties', {})) def _filter_images(images, filters, context, @@ -488,6 +494,7 @@ def image_member_delete(context, member_id): @log_call +@utils.no_4byte_params def image_location_add(context, image_id, location): deleted = location['status'] in ('deleted', 'pending_delete') location_ref = _image_location_format(image_id, @@ -501,6 +508,7 @@ def image_location_add(context, image_id, location): @log_call +@utils.no_4byte_params def image_location_update(context, image_id, location): loc_id = location.get('id') if loc_id is None: @@ -659,13 +667,8 @@ def image_update(context, image_id, image_values, purge_props=False, # this matches weirdness in the sqlalchemy api prop['deleted'] = True - # add in any completely new properties - image['properties'].extend([{'name': k, 'value': v, - 'image_id': image_id, 'deleted': False} - for k, v in new_properties.items()]) - image['updated_at'] = timeutils.utcnow() - image.update(image_values) + _image_update(image, image_values, new_properties) DATA['images'][image_id] = image return _normalize_locations(copy.deepcopy(image)) @@ -724,6 +727,7 @@ def image_tag_set_all(context, image_id, values): @log_call +@utils.no_4byte_params def image_tag_create(context, image_id, value): global DATA DATA['tags'][image_id].append(value) diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index 831c545065..53a4443e68 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -35,6 +35,7 @@ import sqlalchemy.orm as sa_orm import sqlalchemy.sql as sa_sql from glance.common import exception +from glance.common import utils from glance.db.sqlalchemy.metadef_api import namespace as metadef_namespace_api from glance.db.sqlalchemy.metadef_api import object as metadef_object_api from glance.db.sqlalchemy.metadef_api import property as metadef_property_api @@ -663,6 +664,7 @@ def _update_values(image_ref, values): @retry(retry_on_exception=_retry_on_deadlock, wait_fixed=500, stop_max_attempt_number=50) +@utils.no_4byte_params def _image_update(context, values, image_id, purge_props=False, from_state=None): """ @@ -773,6 +775,7 @@ def _image_update(context, values, image_id, purge_props=False, return image_get(context, image_ref.id) +@utils.no_4byte_params def image_location_add(context, image_id, location, session=None): deleted = location['status'] in ('deleted', 'pending_delete') delete_time = timeutils.utcnow() if deleted else None @@ -786,6 +789,7 @@ def image_location_add(context, image_id, location, session=None): location_ref.save(session=session) +@utils.no_4byte_params def image_location_update(context, image_id, location, session=None): loc_id = location.get('id') if loc_id is None: @@ -874,6 +878,7 @@ def _image_locations_delete_all(context, image_id, delete_time=delete_time, session=session) +@utils.no_4byte_params def _set_properties_for_image(context, image_ref, properties, purge_props=False, session=None): """ @@ -1126,6 +1131,7 @@ def image_tag_set_all(context, image_id, tags): image_tag_delete(context, image_id, tag, session) +@utils.no_4byte_params def image_tag_create(context, image_id, value, session=None): """Create an image tag.""" session = session or get_session() diff --git a/glance/tests/functional/db/base.py b/glance/tests/functional/db/base.py index eaf53771a5..8099eb174d 100644 --- a/glance/tests/functional/db/base.py +++ b/glance/tests/functional/db/base.py @@ -206,6 +206,32 @@ class DriverTests(object): self.assertRaises(exception.Invalid, self.db_api.image_create, self.context, fixture) + def test_image_create_bad_name(self): + bad_name = u'A name with forbidden symbol \U0001f62a' + fixture = {'name': bad_name, 'size': 12, 'status': 'queued'} + self.assertRaises(exception.Invalid, self.db_api.image_create, + self.context, fixture) + + def test_image_create_bad_property(self): + # bad value + fixture = {'status': 'queued', + 'properties': {'bad': u'Bad \U0001f62a'}} + self.assertRaises(exception.Invalid, self.db_api.image_create, + self.context, fixture) + # bad property names are also not allowed + fixture = {'status': 'queued', 'properties': {u'Bad \U0001f62a': 'ok'}} + self.assertRaises(exception.Invalid, self.db_api.image_create, + self.context, fixture) + + def test_image_create_bad_location(self): + location_data = [{'url': 'a', 'metadata': {'key': 'value'}, + 'status': 'active'}, + {'url': u'Bad \U0001f60a', 'metadata': {}, + 'status': 'active'}] + fixture = {'status': 'queued', 'locations': location_data} + self.assertRaises(exception.Invalid, self.db_api.image_create, + self.context, fixture) + def test_image_update_core_attribute(self): fixture = {'status': 'queued'} image = self.db_api.image_update(self.adm_context, UUID3, fixture) @@ -272,6 +298,51 @@ class DriverTests(object): self.assertEqual('bar', properties['foo']['value']) self.assertTrue(properties['foo']['deleted']) + def test_image_update_bad_name(self): + fixture = {'name': u'A new name with forbidden symbol \U0001f62a'} + self.assertRaises(exception.Invalid, self.db_api.image_update, + self.adm_context, UUID1, fixture) + + def test_image_update_bad_property(self): + # bad value + fixture = {'status': 'queued', + 'properties': {'bad': u'Bad \U0001f62a'}} + self.assertRaises(exception.Invalid, self.db_api.image_update, + self.adm_context, UUID1, fixture) + # bad property names are also not allowed + fixture = {'status': 'queued', 'properties': {u'Bad \U0001f62a': 'ok'}} + self.assertRaises(exception.Invalid, self.db_api.image_update, + self.adm_context, UUID1, fixture) + + def test_image_update_bad_location(self): + location_data = [{'url': 'a', 'metadata': {'key': 'value'}, + 'status': 'active'}, + {'url': u'Bad \U0001f60a', 'metadata': {}, + 'status': 'active'}] + fixture = {'status': 'queued', 'locations': location_data} + self.assertRaises(exception.Invalid, self.db_api.image_update, + self.adm_context, UUID1, fixture) + + def test_update_locations_direct(self): + """ + For some reasons update_locations can be called directly + (not via image_update), so better check that everything is ok if passed + 4 byte unicode characters + """ + # update locations correctly first to retrieve existing location id + location_data = [{'url': 'a', 'metadata': {'key': 'value'}, + 'status': 'active'}] + fixture = {'locations': location_data} + image = self.db_api.image_update(self.adm_context, UUID1, fixture) + self.assertEqual(1, len(image['locations'])) + self.assertIn('id', image['locations'][0]) + loc_id = image['locations'][0].pop('id') + bad_location = {'url': u'Bad \U0001f60a', 'metadata': {}, + 'status': 'active', 'id': loc_id} + self.assertRaises(exception.Invalid, + self.db_api.image_location_update, + self.adm_context, UUID1, bad_location) + def test_image_property_delete(self): fixture = {'name': 'ping', 'value': 'pong', 'image_id': UUID1} prop = self.db_api.image_property_create(self.context, fixture) @@ -971,6 +1042,11 @@ class DriverTests(object): tag = self.db_api.image_tag_create(self.context, UUID1, 'snap') self.assertEqual('snap', tag) + def test_image_tag_create_bad_value(self): + self.assertRaises(exception.Invalid, + self.db_api.image_tag_create, self.context, + UUID1, u'Bad \U0001f62a') + def test_image_tag_set_all(self): tags = self.db_api.image_tag_get_all(self.context, UUID1) self.assertEqual([], tags) diff --git a/glance/tests/unit/v2/test_registry_api.py b/glance/tests/unit/v2/test_registry_api.py index 3266100ebb..7cd0aa0630 100644 --- a/glance/tests/unit/v2/test_registry_api.py +++ b/glance/tests/unit/v2/test_registry_api.py @@ -1306,6 +1306,85 @@ class TestRegistryRPC(base.IsolatedUnitTest): for k, v in six.iteritems(fixture): self.assertEqual(v, res_dict[k]) + def _send_request(self, command, kwargs, method): + req = webob.Request.blank('/rpc') + req.method = method + cmd = [{'command': command, 'kwargs': kwargs}] + req.body = jsonutils.dumps(cmd) + res = req.get_response(self.api) + res_dict = jsonutils.loads(res.body)[0] + return res.status_int, res_dict + + def _expect_fail(self, command, kwargs, error_cls, method='POST'): + # on any exception status_int is always 200, so have to check _error + # dict + code, res_dict = self._send_request(command, kwargs, method) + self.assertTrue('_error' in res_dict) + self.assertEqual(error_cls, res_dict['_error']['cls']) + return res_dict + + def _expect_ok(self, command, kwargs, method, expected_status=200): + code, res_dict = self._send_request(command, kwargs) + self.assertEqual(expected_status, code) + return res_dict + + def test_create_image_bad_name(self): + fixture = {'name': u'A bad name \U0001fff2', 'status': 'queued'} + self._expect_fail('image_create', + {'values': fixture}, + 'glance.common.exception.Invalid') + + def test_create_image_bad_location(self): + fixture = {'status': 'queued', + 'locations': [{'url': u'file:///tmp/tests/\U0001fee2', + 'metadata': {}, + 'status': 'active'}]} + self._expect_fail('image_create', + {'values': fixture}, + 'glance.common.exception.Invalid') + + def test_create_image_bad_property(self): + fixture = {'status': 'queued', + 'properties': {'ok key': u' bad value \U0001f2aa'}} + self._expect_fail('image_create', + {'values': fixture}, + 'glance.common.exception.Invalid') + fixture = {'status': 'queued', + 'properties': {u'invalid key \U00010020': 'ok value'}} + self._expect_fail('image_create', + {'values': fixture}, + 'glance.common.exception.Invalid') + + def test_update_image_bad_tag(self): + self._expect_fail('image_tag_create', + {'value': u'\U0001fff2', 'image_id': UUID2}, + 'glance.common.exception.Invalid') + + def test_update_image_bad_name(self): + fixture = {'name': u'A bad name \U0001fff2'} + self._expect_fail('image_update', + {'values': fixture, 'image_id': UUID1}, + 'glance.common.exception.Invalid') + + def test_update_image_bad_location(self): + fixture = {'locations': + [{'url': u'file:///tmp/glance-tests/\U0001fee2', + 'metadata': {}, + 'status': 'active'}]} + self._expect_fail('image_update', + {'values': fixture, 'image_id': UUID1}, + 'glance.common.exception.Invalid') + + def test_update_bad_property(self): + fixture = {'properties': {'ok key': u' bad value \U0001f2aa'}} + self._expect_fail('image_update', + {'values': fixture, 'image_id': UUID2}, + 'glance.common.exception.Invalid') + fixture = {'properties': {u'invalid key \U00010020': 'ok value'}} + self._expect_fail('image_update', + {'values': fixture, 'image_id': UUID2}, + 'glance.common.exception.Invalid') + def test_delete_image(self): """Tests that the registry API deletes the image"""