From 5c81485a7fc2078d15af03fec4db7d125bd571a8 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 2 Aug 2018 13:06:58 +0200 Subject: [PATCH 1/3] Always validate language packs if langpack_id is present in manifest Previously we would only validate them if the --langpack flag was passed. --- README.md | 1 - src/linter.js | 1 - src/parsers/manifestjson.js | 4 ++-- src/yargs-options.js | 5 ----- tests/setup.js | 1 - tests/unit/parsers/test.manifestjson.js | 2 -- tests/unit/test.cli.js | 5 ----- tests/unit/test.linter.js | 23 ----------------------- 8 files changed, 2 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 1a720e69..de46767b 100644 --- a/README.md +++ b/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/...', ...] // diff --git a/src/linter.js b/src/linter.js index ec7a133c..2744462f 100644 --- a/src/linter.js +++ b/src/linter.js @@ -251,7 +251,6 @@ export default class Linter { this.collector, { selfHosted: this.config.selfHosted, - isLanguagePack: this.config.langpack, io: this.io, }, ); diff --git a/src/parsers/manifestjson.js b/src/parsers/manifestjson.js index e6f1dc2b..5393c45f 100644 --- a/src/parsers/manifestjson.js +++ b/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; diff --git a/src/yargs-options.js b/src/yargs-options.js index 75f1a69f..a75f6676 100644 --- a/src/yargs-options.js +++ b/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', diff --git a/tests/setup.js b/tests/setup.js index fad60894..17bc699f 100644 --- a/tests/setup.js +++ b/tests/setup.js @@ -16,7 +16,6 @@ jest.mock('cli', () => { getConfig: () => ({ argv: { selfHosted: false, - langpack: false, }, }), terminalWidth: () => 78, diff --git a/tests/unit/parsers/test.manifestjson.js b/tests/unit/parsers/test.manifestjson.js index 5e58b316..7bc8e772 100644 --- a/tests/unit/parsers/test.manifestjson.js +++ b/tests/unit/parsers/test.manifestjson.js @@ -1072,7 +1072,6 @@ describe('ManifestJSONParser', () => { const json = validLangpackManifestJSON(); const manifestJSONParser = new ManifestJSONParser( json, linter.collector, { - isLanguagePack: true, io: { files: {} }, } ); @@ -1084,7 +1083,6 @@ describe('ManifestJSONParser', () => { const json = validLangpackManifestJSON({ langpack_id: null }); const manifestJSONParser = new ManifestJSONParser( json, linter.collector, { - isLanguagePack: true, io: { files: {} }, } ); diff --git a/tests/unit/test.cli.js b/tests/unit/test.cli.js index aef431f8..1932d2d6 100644 --- a/tests/unit/test.cli.js +++ b/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( diff --git a/tests/unit/test.linter.js b/tests/unit/test.linter.js index 937a2d8f..ad8e9503 100644 --- a/tests/unit/test.linter.js +++ b/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 = { From 9622916d1c000e263e5ee2cc74c63630cb3a2465 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 2 Aug 2018 13:17:30 +0200 Subject: [PATCH 2/3] Do not warn on strict_max_version for langpacks --- src/parsers/manifestjson.js | 3 ++- tests/unit/helpers.js | 2 +- tests/unit/parsers/test.manifestjson.js | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/parsers/manifestjson.js b/src/parsers/manifestjson.js index 5393c45f..04a278b3 100644 --- a/src/parsers/manifestjson.js +++ b/src/parsers/manifestjson.js @@ -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); diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index 1986606f..6b667851 100644 --- a/tests/unit/helpers.js +++ b/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: { diff --git a/tests/unit/parsers/test.manifestjson.js b/tests/unit/parsers/test.manifestjson.js index 7bc8e772..3bc13220 100644 --- a/tests/unit/parsers/test.manifestjson.js +++ b/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', () => { From a07476696d3e1d06e54b082691a5e88014e0dd21 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 2 Aug 2018 13:40:57 +0200 Subject: [PATCH 3/3] Prevent language packs from having additional webextension properties --- src/schema/validator.js | 42 +++++++++++++++++-------- tests/unit/parsers/test.manifestjson.js | 16 ++++++++++ 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/schema/validator.js b/src/schema/validator.js index 73f53ee5..2ff56cab 100644 --- a/src/schema/validator.js +++ b/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', diff --git a/tests/unit/parsers/test.manifestjson.js b/tests/unit/parsers/test.manifestjson.js index 3bc13220..ccd72345 100644 --- a/tests/unit/parsers/test.manifestjson.js +++ b/tests/unit/parsers/test.manifestjson.js @@ -1104,6 +1104,22 @@ describe('ManifestJSONParser', () => { ); 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', () => {