No 4 byte unicode allowed in image parameters

Image create or update with params containing 4 byte unicode characters
results in 400 server error code (instead of former 500).
Image name/location/tags/properties are validated this way

Change-Id: Ib0fcf2456f0996e8235983b361d1ee5f66136375
Closes-bug: #1370954

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)})

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:

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

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)

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
+    return _image_update(image, values, values.pop('properties', {}))
-    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
 
 
 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)

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()

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)

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"""