Avoid raising exceptions when parsing invalid manifests

src/olympia/files/tests/test_utils.py

@@ -66,12 +66,48 @@ def create_webext_default_versions(cls):
 
 
 class TestManifestJSONExtractor(AppVersionsMixin, TestCase):
+    def test_valid_json(self):
+        assert self.parse({})
+
     def test_not_dict(self):
         with self.assertRaises(utils.InvalidManifest) as exc:
             utils.ManifestJSONExtractor('"[email protected]"').parse()
         assert isinstance(exc.exception, forms.ValidationError)
         assert exc.exception.message == 'Could not parse the manifest file.'
 
+    def test_parse_with_fields_that_should_be_dicts(self):
+        # parse() will get the value for these fields and therefore should
+        # raise the InvalidManifest exception if they are not dicts, so that
+        # the linter can display the proper error.
+        with self.assertRaises(utils.InvalidManifest) as exc:
+            self.parse({'browser_specific_settings': []})
+        assert isinstance(exc.exception, forms.ValidationError)
+        assert exc.exception.message == 'Could not parse the manifest file.'
+
+        with self.assertRaises(utils.InvalidManifest) as exc:
+            self.parse({'browser_specific_settings': {'gecko': 'lmao'}})
+        assert isinstance(exc.exception, forms.ValidationError)
+        assert exc.exception.message == 'Could not parse the manifest file.'
+
+        with self.assertRaises(utils.InvalidManifest) as exc:
+            self.parse({'developer': 'rotfl'})
+        assert isinstance(exc.exception, forms.ValidationError)
+        assert exc.exception.message == 'Could not parse the manifest file.'
+
+    def test_parse_data_collection_permissions_should_be_dict(self):
+        # As previous, this is automatically parsed for extensions and should
+        # raise InvalidManifest if not a dict.
+        with self.assertRaises(utils.InvalidManifest) as exc:
+            self.parse(
+                {
+                    'browser_specific_settings': {
+                        'gecko': {'data_collection_permissions': []}
+                    }
+                }
+            )
+        assert isinstance(exc.exception, forms.ValidationError)
+        assert exc.exception.message == 'Could not parse the manifest file.'
+
     def test_parse_xpi_no_manifest(self):
         fake_zip = utils.make_xpi({'dummy': 'dummy'})
 
@@ -886,6 +922,10 @@ def parse(self, base_data):
     def test_type(self):
         assert self.parse({})['type'] == amo.ADDON_STATICTHEME
 
+    def test_parse_data_collection_permissions_should_be_dict(self):
+        # Overridden because themes don't support data collection permissions.
+        pass
+
     def test_apps_use_default_versions_if_applications_is_omitted(self):
         """
         Override this because static themes have a higher default version.

src/olympia/files/utils.py

@@ -1,6 +1,7 @@
 import contextlib
 import errno
 import fcntl
+import functools
 import hashlib
 import io
 import json
@@ -13,6 +14,7 @@
 import tarfile
 import tempfile
 import zipfile
+from collections.abc import Mapping
 from types import MappingProxyType
 
 from django import forms
@@ -152,6 +154,17 @@ def get_simple_version(version_string):
     return VersionString(re.sub('[<=>]', '', version_string))
 
 
+def raise_invalid_manifest_if_not_mapping(f):
+    @functools.wraps(f)
+    def wrapper(self):
+        rval = f(self)
+        if not isinstance(rval, Mapping):
+            raise InvalidManifest(gettext('Could not parse the manifest file.'))
+        return rval
+
+    return wrapper
+
+
 class ManifestJSONExtractor:
     """Extract add-on info from a manifest file."""
 
@@ -189,11 +202,16 @@ def __init__(self, manifest_data, *, certinfo=None):
     def get(self, key, default=None):
         return self.data.get(key, default)
 
+    @property
+    @raise_invalid_manifest_if_not_mapping
+    def developer(self):
+        return self.get('developer', EMPTY_FALLBACK_DICT)
+
     @property
     def homepage(self):
         homepage_url = self.get('homepage_url')
         # `developer.url` in the manifest overrides `homepage_url`.
-        return self.get('developer', EMPTY_FALLBACK_DICT).get('url', homepage_url)
+        return self.developer.get('url', homepage_url)
 
     @property
     def is_experiment(self):
@@ -206,20 +224,29 @@ def is_experiment(self):
         return any(bool(self.get(key)) for key in experiment_keys)
 
     @property
+    @raise_invalid_manifest_if_not_mapping
+    def browser_specific_settings(self):
+        return self.get(
+            'browser_specific_settings', self.get('applications', EMPTY_FALLBACK_DICT)
+        )
+
+    @property
+    @raise_invalid_manifest_if_not_mapping
     def gecko(self):
         """Return the "applications|browser_specific_settings["gecko"]" part
         of the manifest."""
-        parent_block = self.get(
-            'browser_specific_settings', self.get('applications', EMPTY_FALLBACK_DICT)
-        )
-        return parent_block.get('gecko', EMPTY_FALLBACK_DICT)
+        return self.browser_specific_settings.get('gecko', EMPTY_FALLBACK_DICT)
 
     @property
+    @raise_invalid_manifest_if_not_mapping
     def gecko_android(self):
         """Return `browser_specific_settings.gecko_android` if present."""
-        return self.get('browser_specific_settings', EMPTY_FALLBACK_DICT).get(
-            'gecko_android', EMPTY_FALLBACK_DICT
-        )
+        return self.browser_specific_settings.get('gecko_android', EMPTY_FALLBACK_DICT)
+
+    @property
+    @raise_invalid_manifest_if_not_mapping
+    def data_collection_permissions(self):
+        return self.gecko.get('data_collection_permissions', EMPTY_FALLBACK_DICT)
 
     @property
     def guid(self):
@@ -441,12 +468,12 @@ def parse(self, minimal=False):
                         'permissions': self.get('permissions', []),
                         'host_permissions': self.get('host_permissions', []),
                         'content_scripts': self.get('content_scripts', []),
-                        'data_collection_permissions': self.gecko.get(
-                            'data_collection_permissions', {}
-                        ).get('required', []),
-                        'optional_data_collection_permissions': self.gecko.get(
-                            'data_collection_permissions', {}
-                        ).get('optional', []),
+                        'data_collection_permissions': (
+                            self.data_collection_permissions.get('required', [])
+                        ),
+                        'optional_data_collection_permissions': (
+                            self.data_collection_permissions.get('optional', [])
+                        ),
                     }
                 )