Merge pull request #142 from mozilla-services/131_ujson_escaping

Fix ujson escaping (fixes #131)

cliquet/__init__.py

@@ -16,6 +16,7 @@
 from cornice import Service
 from pyramid.events import NewRequest, NewResponse
 from pyramid.httpexceptions import HTTPTemporaryRedirect, HTTPGone
+from pyramid.renderers import JSON as JSONRenderer
 from pyramid.security import NO_PERMISSION_REQUIRED
 from pyramid_multiauth import MultiAuthenticationPolicy
 from pyramid.settings import asbool, aslist
@@ -80,10 +81,8 @@ def monkey_patch_json(config):
     requests.models.json = utils.json
 
     # Override json renderer using ujson
-    # deactivated, see https://github.com/mozilla-services/cliquet/pull/132
-    # from pyramid.renderers import JSON as JSONRenderer
-    # renderer = JSONRenderer(serializer=lambda v, **kw: utils.json.dumps(v))
-    # config.add_renderer('json', renderer)
+    renderer = JSONRenderer(serializer=lambda v, **kw: utils.json.dumps(v))
+    config.add_renderer('json', renderer)
 
 
 def load_default_settings(config):

cliquet/logs.py

@@ -84,10 +84,10 @@ def __init__(self, settings):
 
     def __call__(self, logger, name, event_dict):
         if 'path' in event_dict:
-            pattern = ('"{method: <5} {path}{querystring}" {code} ({t} ms)'
+            pattern = (u'"{method: <5} {path}{querystring}" {code} ({t} ms)'
                        ' {event} {context}')
         else:
-            pattern = '{event} {context}'
+            pattern = u'{event} {context}'
 
         output = {}
         for field in ['method', 'path', 'code', 't', 'event']:

cliquet/storage/cloud_storage.py

@@ -29,11 +29,13 @@ def wrapped(*args, **kwargs):
         try:
             return func(*args, **kwargs)
         except RequestException as e:
+            status_code = body = None
             if e.response is not None:
                 status_code = e.response.status_code
-                body = e.response.json()
-            else:
-                status_code = body = None
+                try:
+                    body = e.response.json()
+                except ValueError:
+                    body = e.response.content
 
             if status_code == 404:
                 record_id = '?'
@@ -173,6 +175,18 @@ def delete_all(self, resource, user_id, filters=None):
                                    headers=self._build_headers(resource))
         resp.raise_for_status()
 
+    def _filters_as_params(self, filters):
+        params = []
+        for k, v, op in filters:
+            if isinstance(v, six.string_types):
+                # Literals '\' should be preserved in querystring
+                v = v.replace("\\", "\\\\")
+            params.append(("%s%s" % (FILTERS[op], k), v))
+        return params
+
+    def _params_as_querystring(self, params):
+        return '&'.join(["%s=%s" % (p, v) for p, v in params])
+
     @wrap_http_error
     def get_all(self, resource, user_id, filters=None, sorting=None,
                 pagination_rules=None, limit=None, include_deleted=False):
@@ -190,8 +204,7 @@ def get_all(self, resource, user_id, filters=None, sorting=None,
             params += [("_sort", ','.join(sort_fields))]
 
         if filters:
-            params += [("%s%s" % (FILTERS[op], k), v)
-                       for k, v, op in filters]
+            params += self._filters_as_params(filters)
 
         if limit:
             params.append(("_limit", limit))
@@ -208,17 +221,16 @@ def get_all(self, resource, user_id, filters=None, sorting=None,
         else:
             batch_payload = {'defaults': {'body': {}}, 'requests': []}
 
-            querystring = '&'.join(['%s=%s' % (p, v) for p, v in params])
+            querystring = self._params_as_querystring(params)
             batch_payload['requests'].append({
                 'method': 'HEAD',
                 'path': url + '?%s' % querystring,
             })
 
             for filters in pagination_rules:
                 params_ = list(params)
-                params_ += [("%s%s" % (FILTERS[op], k), v)
-                            for k, v, op in filters]
-                querystring = '&'.join(['%s=%s' % (p, v) for p, v in params_])
+                params_ += self._filters_as_params(filters)
+                querystring = self._params_as_querystring(params_)
                 batch_payload['requests'].append({
                     'path': url + '?%s' % querystring,
                 })

cliquet/storage/postgresql/__init__.py

@@ -466,7 +466,8 @@ def _format_conditions(self, resource, filters, prefix='filters'):
                 # If field is missing, we default to ''.
                 sql_field = "coalesce(data->>%%(%s)s, '')" % field_holder
                 # JSON-ify the native value (e.g. True -> 'true')
-                value = json.dumps(filtr.value).strip('"')
+                if not isinstance(filtr.value, six.string_types):
+                    value = json.dumps(filtr.value).strip('"')
 
             # Safely escape value
             value_holder = '%s_value_%s' % (prefix, i)

cliquet/tests/test_logging.py

@@ -86,6 +86,10 @@ def test_every_event_dict_entry_appears_in_log_message(self):
         self.assertEqual(('"GET   /v1/?_sort=field" 200 (32 ms)'
                           ' app.event nb_records=5'), value)
 
+    def test_fields_values_support_unicode(self):
+        value = self.renderer(self.logger, 'critical', {'value': u'\u2014'})
+        self.assertIn(u'\u2014', value)
+
 
 class MozillaHekaRendererTest(unittest.TestCase):
     def setUp(self):

cliquet/tests/test_storage.py

@@ -383,6 +383,23 @@ def test_updating_raises_unicity_error(self):
                           record['id'],
                           {'phone': 'number'})
 
+    def test_unicity_detection_supports_special_characters(self):
+        record = self.create_record()
+        values = ['b', 'http://moz.org', u"#131 \u2014 ujson",
+                  "C:\\\\win32\\hosts"]
+        for value in values:
+            self.create_record({'phone': value})
+            try:
+                error = None
+                self.storage.update(self.resource,
+                                    self.user_id,
+                                    record['id'],
+                                    {'phone': value})
+            except exceptions.UnicityError as e:
+                error = e
+            msg = 'UnicityError not raised with %s' % value
+            self.assertIsNotNone(error, msg)
+
 
 class DeletedRecordsTest(object):
     def _get_last_modified_filters(self):
@@ -841,3 +858,15 @@ def setUp(self):
             self.storage._client,
             'request',
             side_effect=requests.ConnectionError)
+
+    def test_raises_backenderror_when_remote_returns_500(self):
+        with mock.patch.object(self.storage._client, 'request') as mocked:
+            error_response = requests.models.Response()
+            error_response.status_code = 500
+            error_response._content_consumed = True
+            error_response._content = u'Internal Error'.encode('utf8')
+            mocked.return_value = error_response
+            self.assertRaises(exceptions.BackendError,
+                              self.storage.get_all,
+                              self.resource,
+                              self.user_id)

cliquet/tests/test_views_batch.py

@@ -283,7 +283,7 @@ def test_subrequests_body_have_utf8_charset(self):
         self.post({'requests': [request]})
         subrequest, = self.request.invoke_subrequest.call_args[0]
         self.assertIn('charset=utf-8', subrequest.headers['Content-Type'])
-        wanted = {"json": u"\ud83d\ude02"}
+        wanted = {"json": u"😂"}
         self.assertEqual(subrequest.body.decode('utf8'),
                          json.dumps(wanted))
 

cliquet/utils.py

@@ -5,10 +5,7 @@
 from base64 import b64decode, b64encode
 from binascii import hexlify
 
-# XXX see https://github.com/mozilla-services/cliquet/issues/131
-# import ujson as json
-import json
-
+import ujson as json
 from cornice import cors
 from colander import null