diff --git a/src/olympia/devhub/tasks.py b/src/olympia/devhub/tasks.py index a6673fee6f..3c98faad65 100644 --- a/src/olympia/devhub/tasks.py +++ b/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 diff --git a/src/olympia/devhub/tests/addons/invalid_manifest_webextension.xpi b/src/olympia/devhub/tests/addons/invalid_manifest_webextension.xpi new file mode 100644 index 0000000000..cad52ae3c2 Binary files /dev/null and b/src/olympia/devhub/tests/addons/invalid_manifest_webextension.xpi differ diff --git a/src/olympia/devhub/tests/test_tasks.py b/src/olympia/devhub/tests/test_tasks.py index 8886aa37ca..f9268838ae 100644 --- a/src/olympia/devhub/tests/test_tasks.py +++ b/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'] diff --git a/src/olympia/devhub/tests/test_views.py b/src/olympia/devhub/tests/test_views.py index 1765079079..5115d0a84e 100644 --- a/src/olympia/devhub/tests/test_views.py +++ b/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') diff --git a/src/olympia/files/utils.py b/src/olympia/files/utils.py index 1a660c7935..3a16e3d223 100644 --- a/src/olympia/files/utils.py +++ b/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 - self.data = json.loads(json_string) + 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.'))