Merge pull request #2123 from diox/validate-langpacks-without-command-line-flag

Validate langpacks without command line flag

README.md

@@ -69,7 +69,6 @@ const linter = linter.createInstance({
     output: 'none',
     boring: false,
     selfHosted: false,
-    langpack: false,
     // Lint only the selected files
     //   scanFile: ['path/...', ...]
     //

src/linter.js

@@ -251,7 +251,6 @@ export default class Linter {
         this.collector,
         {
           selfHosted: this.config.selfHosted,
-          isLanguagePack: this.config.langpack,
           io: this.io,
         },
       );

src/parsers/manifestjson.js

@@ -39,7 +39,6 @@ export default class ManifestJSONParser extends JSONParser {
   constructor(jsonString, collector, {
     filename = MANIFEST_JSON, RelaxedJSON = RJSON,
     selfHosted = getDefaultConfigValue('self-hosted'),
-    isLanguagePack = getDefaultConfigValue('langpack'),
     io = null,
   } = {}) {
     super(jsonString, collector, { filename });
@@ -57,7 +56,8 @@ export default class ManifestJSONParser extends JSONParser {
     } else {
       // We've parsed the JSON; now we can validate the manifest.
       this.selfHosted = selfHosted;
-      this.isLanguagePack = isLanguagePack;
+      this.isLanguagePack = Object.prototype.hasOwnProperty.call(
+        this.parsedJSON, 'langpack_id');
       this.isStaticTheme = Object.prototype.hasOwnProperty.call(
         this.parsedJSON, 'theme');
       this.io = io;
@@ -192,7 +192,8 @@ export default class ManifestJSONParser extends JSONParser {
       this.isValid = false;
     }
 
-    if (this.parsedJSON.applications &&
+    if (!this.isLanguagePack &&
+        this.parsedJSON.applications &&
         this.parsedJSON.applications.gecko &&
         this.parsedJSON.applications.gecko.strict_max_version) {
       this.collector.addNotice(messages.STRICT_MAX_VERSION);

src/schema/validator.js

@@ -69,21 +69,9 @@ export const validateAddon = (...args) => {
   return isValid;
 };
 
-const _validateLangPack = validator.compile({
-  ...schemaObject,
-  id: 'langpack-manifest',
-  $ref: '#/types/WebExtensionLangpackManifest',
-});
-
-export const validateLangPack = (...args) => {
-  const isValid = _validateLangPack(...args);
-  validateLangPack.errors = filterErrors(_validateLangPack.errors);
-  return isValid;
-};
-
 // Create a new schema object that merges theme.json and the regular
 // manifest.json schema.
-// Then modify the result of that to allow `additionalProperties = false`
+// Then modify the result of that to set `additionalProperties = false`
 // so that additional properties are not allowed for themes.
 // We have to use deepmerge here to make sure we can overwrite the nested
 // structure and can use object-destructuring at the root level
@@ -113,6 +101,34 @@ export const validateStaticTheme = (...args) => {
   return isValid;
 };
 
+// Like with static themes, we don't want additional properties in langpacks.
+// The only difference is, this time, there is no additional schema file, we
+// just need to reference WebExtensionLangpackManifest and merge it with the
+// object that has additionalProperties: false.
+const _validateLangPack = validator.compile({
+  ...merge(
+    schemaObject, {
+      types: {
+        WebExtensionLangpackManifest: {
+          $merge: {
+            with: {
+              additionalProperties: false,
+            },
+          },
+        },
+      },
+    }
+  ),
+  id: 'langpack-manifest',
+  $ref: '#/types/WebExtensionLangpackManifest',
+});
+
+export const validateLangPack = (...args) => {
+  const isValid = _validateLangPack(...args);
+  validateLangPack.errors = filterErrors(_validateLangPack.errors);
+  return isValid;
+};
+
 const _validateLocaleMessages = validator.compile({
   ...messagesSchemaObject,
   id: 'messages',

src/yargs-options.js

@@ -42,11 +42,6 @@ const options = {
     type: 'boolean',
     default: false,
   },
-  langpack: {
-    describe: 'Treat this package as a WebExtension Language Pack.',
-    type: 'boolean',
-    default: false,
-  },
   'scan-file': {
     alias: ['f'],
     describe: 'Scan a selected file',

tests/setup.js

@@ -16,7 +16,6 @@ jest.mock('cli', () => {
     getConfig: () => ({
       argv: {
         selfHosted: false,
-        langpack: false,
       },
     }),
     terminalWidth: () => 78,

tests/unit/helpers.js

@@ -93,7 +93,7 @@ export function validLangpackManifestJSON(extra) {
   return JSON.stringify(Object.assign({}, {
     manifest_version: 2,
     name: 'My Language Pack',
-    version: '57.0a1',
+    version: '57.0',
     langpack_id: 'de',
     languages: {
       de: {

tests/unit/parsers/test.manifestjson.js

@@ -340,6 +340,22 @@ describe('ManifestJSONParser', () => {
       expect(notices[0].code).toEqual(messages.STRICT_MAX_VERSION.code);
       expect(notices[0].message).toContain('strict_max_version');
     });
+
+    it('does not warn on strict_max_version in language packs', () => {
+      const addonLinter = new Linter({ _: ['bar'] });
+      const json = validLangpackManifestJSON({
+        applications: {
+          gecko: {
+            strict_max_version: '58.0',
+          },
+        },
+      });
+      const manifestJSONParser = new ManifestJSONParser(json,
+        addonLinter.collector);
+      console.log(addonLinter.collector.notices);
+      expect(manifestJSONParser.isValid).toEqual(true);
+      expect(addonLinter.collector.notices.length).toEqual(0);
+    });
   });
 
   describe('content security policy', () => {
@@ -1072,7 +1088,6 @@ describe('ManifestJSONParser', () => {
       const json = validLangpackManifestJSON();
       const manifestJSONParser = new ManifestJSONParser(
         json, linter.collector, {
-          isLanguagePack: true,
           io: { files: {} },
         }
       );
@@ -1084,12 +1099,27 @@ describe('ManifestJSONParser', () => {
       const json = validLangpackManifestJSON({ langpack_id: null });
       const manifestJSONParser = new ManifestJSONParser(
         json, linter.collector, {
-          isLanguagePack: true,
           io: { files: {} },
         }
       );
       expect(manifestJSONParser.isValid).toEqual(false);
     });
+
+    it('throws warning on additional properties', () => {
+      const linter = new Linter({ _: ['bar'] });
+      const json = validLangpackManifestJSON({ content_scripts: ['foo.js'] });
+      const manifestJSONParser = new ManifestJSONParser(
+        json, linter.collector, {
+          io: { files: {} },
+        }
+      );
+      expect(manifestJSONParser.isValid).toEqual(false);
+      assertHasMatchingError(linter.collector.errors, {
+        code: messages.JSON_INVALID.code,
+        message: '"/content_scripts" is an invalid additional property',
+        description: 'Your JSON file could not be parsed.',
+      });
+    });
   });
 
   describe('static theme', () => {

tests/unit/test.cli.js

@@ -66,11 +66,6 @@ describe('Basic CLI tests', function cliCallback() {
     expect(args.warningsAsErrors).toEqual(false);
   });
 
-  it('should default langpack to false', () => {
-    const args = cli.parse(['foo/bar.zip']);
-    expect(args.langpack).toEqual(false);
-  });
-
   it('should show error on missing xpi', () => {
     cli.parse([]);
     expect(this.fakeFail.calledWithMatch(

tests/unit/test.linter.js

@@ -674,29 +674,6 @@ describe('Linter.getAddonMetadata()', () => {
     expect(FakeManifestParser.firstCall.args[2].selfHosted).toEqual(true);
   });
 
-  it('should pass isLanguagePack flag to ManifestJSONParser', async () => {
-    const addonLinter = new Linter({ _: ['bar'], langpack: true });
-    addonLinter.io = {
-      getFiles: async () => {
-        return {
-          'manifest.json': {},
-        };
-      },
-      getFileAsString: async () => {
-        return validManifestJSON({});
-      },
-    };
-
-    const FakeManifestParser = sinon.spy(ManifestJSONParser);
-    await addonLinter.getAddonMetadata({
-      ManifestJSONParser: FakeManifestParser,
-    });
-    sinon.assert.calledOnce(FakeManifestParser);
-    expect(
-      FakeManifestParser.firstCall.args[2].isLanguagePack).toEqual(true);
-  });
-
-
   it('should collect notices if no manifest', async () => {
     const addonLinter = new Linter({ _: ['bar'] });
     addonLinter.io = {