Tag values from CSP reports need to run through validation (#4001)

Author: mattrobenolt

src/sentry/coreapi.py

@@ -732,7 +732,6 @@ def validate_data(self, project, data):
             'project': project.id,
             'message': inst.get_message(),
             'culprit': inst.get_culprit(),
-            'tags': inst.get_tags(),
             'release': meta.get('release'),
             inst.get_path(): inst.to_json(),
             # This is a bit weird, since we don't have nearly enough
@@ -759,6 +758,30 @@ def validate_data(self, project, data):
                     'value': data['release'],
                 })
                 del data['release']
+
+        tags = []
+        for k, v in inst.get_tags():
+            if len(v) > MAX_TAG_VALUE_LENGTH:
+                self.log.debug('Discarded invalid tag: %s=%s', k, v)
+                data['errors'].append({
+                    'type': EventError.INVALID_DATA,
+                    'name': 'tags',
+                    'value': (k, v),
+                })
+                continue
+            if not TagValue.is_valid_value(v):
+                self.log.debug('Discard invalid tag value: %s', v)
+                data['errors'].append({
+                    'type': EventError.INVALID_DATA,
+                    'name': 'tags',
+                    'value': (k, v),
+                })
+                continue
+            tags.append((k, v))
+
+        if tags:
+            data['tags'] = tags
+
         return data
 
 

tests/sentry/coreapi/tests.py

@@ -492,7 +492,10 @@ def test_validate_basic(self):
         assert result['errors'] == []
         assert 'message' in result
         assert 'culprit' in result
-        assert 'tags' in result
+        assert result['tags'] == [
+            ('effective-directive', 'img-src'),
+            ('blocked-uri', 'http://google.com'),
+        ]
         assert result['sentry.interfaces.User'] == {'ip_address': '69.69.69.69'}
         assert result['sentry.interfaces.Http'] == {
             'url': 'http://45.55.25.245:8123/csp',
@@ -506,3 +509,58 @@ def test_validate_basic(self):
     def test_validate_raises_invalid_interface(self):
         with self.assertRaises(APIForbidden):
             self.helper.validate_data(self.project, {})
+
+    def test_tags_out_of_bounds(self):
+        report = {
+            "document-uri": "http://45.55.25.245:8123/csp",
+            "referrer": "http://example.com",
+            "violated-directive": "img-src https://45.55.25.245:8123/",
+            "effective-directive": "img-src",
+            "original-policy": "default-src  https://45.55.25.245:8123/; child-src  https://45.55.25.245:8123/; connect-src  https://45.55.25.245:8123/; font-src  https://45.55.25.245:8123/; img-src  https://45.55.25.245:8123/; media-src  https://45.55.25.245:8123/; object-src  https://45.55.25.245:8123/; script-src  https://45.55.25.245:8123/; style-src  https://45.55.25.245:8123/; form-action  https://45.55.25.245:8123/; frame-ancestors 'none'; plugin-types 'none'; report-uri http://45.55.25.245:8123/csp-report?os=OS%20X&device=&browser_version=43.0&browser=chrome&os_version=Lion",
+            "blocked-uri": "v" * 201,
+            "status-code": 200,
+            "_meta": {
+                "release": "abc123",
+            }
+        }
+        result = self.helper.validate_data(self.project, report)
+        assert result['tags'] == [
+            ('effective-directive', 'img-src'),
+        ]
+        assert len(result['errors']) == 1
+
+    def test_tag_value(self):
+        report = {
+            "document-uri": "http://45.55.25.245:8123/csp",
+            "referrer": "http://example.com",
+            "violated-directive": "img-src https://45.55.25.245:8123/",
+            "effective-directive": "img-src",
+            "original-policy": "default-src  https://45.55.25.245:8123/; child-src  https://45.55.25.245:8123/; connect-src  https://45.55.25.245:8123/; font-src  https://45.55.25.245:8123/; img-src  https://45.55.25.245:8123/; media-src  https://45.55.25.245:8123/; object-src  https://45.55.25.245:8123/; script-src  https://45.55.25.245:8123/; style-src  https://45.55.25.245:8123/; form-action  https://45.55.25.245:8123/; frame-ancestors 'none'; plugin-types 'none'; report-uri http://45.55.25.245:8123/csp-report?os=OS%20X&device=&browser_version=43.0&browser=chrome&os_version=Lion",
+            "blocked-uri": "http://google.com\n",
+            "status-code": 200,
+            "_meta": {
+                "release": "abc123",
+            }
+        }
+        result = self.helper.validate_data(self.project, report)
+        assert result['tags'] == [
+            ('effective-directive', 'img-src'),
+        ]
+        assert len(result['errors']) == 1
+
+    def test_no_tags(self):
+        report = {
+            "document-uri": "http://45.55.25.245:8123/csp",
+            "referrer": "http://example.com",
+            "violated-directive": "img-src https://45.55.25.245:8123/",
+            "effective-directive": "v" * 201,
+            "original-policy": "default-src  https://45.55.25.245:8123/; child-src  https://45.55.25.245:8123/; connect-src  https://45.55.25.245:8123/; font-src  https://45.55.25.245:8123/; img-src  https://45.55.25.245:8123/; media-src  https://45.55.25.245:8123/; object-src  https://45.55.25.245:8123/; script-src  https://45.55.25.245:8123/; style-src  https://45.55.25.245:8123/; form-action  https://45.55.25.245:8123/; frame-ancestors 'none'; plugin-types 'none'; report-uri http://45.55.25.245:8123/csp-report?os=OS%20X&device=&browser_version=43.0&browser=chrome&os_version=Lion",
+            "blocked-uri": "http://google.com\n",
+            "status-code": 200,
+            "_meta": {
+                "release": "abc123",
+            }
+        }
+        result = self.helper.validate_data(self.project, report)
+        assert 'tags' not in result
+        assert len(result['errors']) == 2