Let the linter raise a message when the add-on manifest is invalid json

src/olympia/devhub/tasks.py

@@ -37,7 +37,8 @@ from olympia.api.models import SYMMETRIC_JWT_TYPE, APIKey
 from olympia.lib.akismet.models import AkismetReport
 from olympia.files.models import File, FileUpload, FileValidation
 from olympia.files.utils import (
-    NoManifestFound, parse_addon, SafeZip, UnsupportedFileType)
+    InvalidManifest, NoManifestFound, parse_addon, SafeZip,
+    UnsupportedFileType)
 from olympia.files.tasks import repack_fileupload
 from olympia.versions.models import Version
 from olympia.devhub import file_validation_annotations as annotations
@@ -157,8 +158,7 @@ def validation_task(fn):
             results = deepcopy(amo.VALIDATOR_SKELETON_RESULTS)
             annotations.insert_validation_message(
                 results, type_='error',
-                message=exc.message, msg_id='unsupported_filetype',
-                compatibility_type=None)
+                message=exc.message, msg_id='unsupported_filetype')
             return results
         except Exception as exc:
             log.exception('Unhandled error during validation: %r' % exc)
@@ -222,6 +222,10 @@ def validate_file_path(path, channel):
         # considered a legacy extension, and the linter will pick it up and
         # will know what message to return to the developer.
         data = {}
+    except InvalidManifest:
+        # Similarly, if we can't parse the manifest, let the linter pick that
+        # up.
+        data = {}
     is_legacy_extension = data.get('is_webextension', None) is False
     is_mozilla_signed = data.get('is_mozilla_signed_extension', None) is True
 

src/olympia/devhub/tests/test_tasks.py

@@ -584,6 +584,18 @@ class TestValidateFilePath(ValidatorTestCase):
             channel=amo.RELEASE_CHANNEL_LISTED)
         assert run_addons_linter_mock.call_count == 1
 
+    @mock.patch('olympia.devhub.tasks.parse_addon')
+    @mock.patch('olympia.devhub.tasks.run_addons_linter')
+    def test_invalid_json_manifest_error(
+            self, run_addons_linter_mock, parse_addon_mock):
+        parse_addon_mock.side_effect = NoManifestFound(message=u'Fôo')
+        # When parse_addon() raises a InvalidManifest error, we should
+        # still call the linter to let it raise the appropriate error message.
+        tasks.validate_file_path(
+            get_addon_file('invalid_manifest_webextension.xpi'),
+            channel=amo.RELEASE_CHANNEL_LISTED)
+        assert run_addons_linter_mock.call_count == 1
+
 
 class TestWebextensionIncompatibilities(UploadTest, ValidatorTestCase):
     fixtures = ['base/addon_3615']

src/olympia/devhub/tests/test_views.py

@@ -15,7 +15,6 @@ from django.utils.translation import trim_whitespace
 import mock
 import pytest
 import responses
-import waffle
 
 from pyquery import PyQuery as pq
 from waffle.testutils import override_switch
@@ -1062,19 +1061,24 @@ class TestUploadDetail(BaseUploadTest):
         assert message == expected
 
     @mock.patch('olympia.devhub.tasks.run_addons_linter')
-    @mock.patch.object(waffle, 'flag_is_active')
-    def test_unparsable_xpi(self, flag_is_active, v):
-        flag_is_active.return_value = True
-        v.return_value = json.dumps(self.validation_ok())
+    def test_not_a_valid_xpi(self, run_addons_linter_mock):
+        run_addons_linter_mock.return_value = json.dumps(self.validation_ok())
         self.upload_file('unopenable.xpi')
+        # We never even reach the linter (we can't: because we're repacking
+        # zip files, we should raise an error if the zip is invalid before
+        # calling the linter, even though the linter has a perfectly good error
+        # message for this kind of situation).
+        assert not run_addons_linter_mock.called
         upload = FileUpload.objects.get()
         response = self.client.get(
             reverse('devhub.upload_detail', args=[upload.uuid.hex, 'json']))
         data = json.loads(force_text(response.content))
         message = [(m['message'], m.get('fatal', False))
                    for m in data['validation']['messages']]
+        # We do raise a specific error message explaining that the archive is
+        # not valid instead of a generic exception.
         assert message == [
-            (u'Sorry, we couldn't load your WebExtension.', True)
+            (u'Invalid or corrupt add-on file.', True)
         ]
 
     @mock.patch('olympia.devhub.tasks.run_addons_linter')

src/olympia/files/utils.py

@@ -144,6 +144,10 @@ class NoManifestFound(forms.ValidationError):
     pass
 
 
+class InvalidManifest(forms.ValidationError):
+    pass
+
+
 class Extractor(object):
     """Extract add-on info from a manifest file."""
     App = collections.namedtuple('App', 'appdata id min max')
@@ -394,7 +398,11 @@ class ManifestJSONExtractor(object):
             if name not in ('blockcomment', 'linecomment'):
                 json_string += token
 
+        try:
             self.data = json.loads(json_string)
+        except Exception:
+            raise InvalidManifest(
+                ugettext('Could not parse the manifest file.'))
 
     def get(self, key, default=None):
         return self.data.get(key, default)
@@ -983,9 +991,13 @@ def parse_xpi(xpi, addon=None, minimal=False, user=None):
         else:
             err, strerror = e.args
         log.error('I/O error({0}): {1}'.format(err, strerror))
+        # Note: we don't really know what happened, so even though we return a
+        # generic message about the manifest, don't raise InvalidManifest. We
+        # want the validation to stop there.
         raise forms.ValidationError(ugettext(
             'Could not parse the manifest file.'))
     except Exception:
+        # As above, don't raise InvalidManifest here.
         log.error('XPI parse error', exc_info=True)
         raise forms.ValidationError(ugettext(
             'Could not parse the manifest file.'))