fix: Prevent mv3 permissions errors to be wrongly reported in mv2 addon validation

[rpl]

Fixes #4367

The unexpected validation warning reported for a valid host permission (filed as addons-linter issue #4367) seems to be due to the fact that both manifest_version 2 and manifest_version 3 definitions for the permissions manifest field are being processed.

The current implementation is trying to make sure to filter out the errors not relevant for the current add-on based on the error.parentSchema property (See isRelevantError helper function defined in src/schema/validator.js).

Unfortunately this approach is unable to filter out all irrelevant errors, in particular for JSONSchema types that are referenced in a min/max_manifest_version conditioned entry with more then 1 level deep nesting, the error.parentSchema will not be the one explicitly including the min/max_manifest_version property and this is what makes the error triggered by the min_manifest_version 3 version of the "permissions" to be wrongly reported as part of a manifest_version 2 add-on validation.

This patch is currently filtering the schema data at "JSONSchema data compile"-time and omitting from the schema data all entries with min/max_manifest_version that do not match the minimum and maximum manifest_version set per schema validator config or if they don't apply to the actual add-on manifest_version.

NOTE: Ideally a better (and less expesive at runtime) approach may be to this kind of JSONSchema data filtering at import time (by creating a different set of JSONSchema data per manifest_version).

[codecov[bot]]

Codecov Report

Base: 98.72% // Head: 98.74% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (3c2a0a7) compared to base (300ad70). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4397      +/-   ##
==========================================
+ Coverage   98.72%   98.74%   +0.01%     
==========================================
  Files          54       54              
  Lines        2832     2866      +34     
  Branches      849      865      +16     
==========================================
+ Hits         2796     2830      +34     
  Misses         36       36              

Impacted Files	Coverage Δ
src/parsers/manifestjson.js	99.21% <ø> (-0.02%)	:arrow_down:
src/schema/validator.js	99.28% <100.00%> (+0.12%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[rpl]

@willdurand I've updated this PR with some more exploratory changes for the other approach we discussed a few days ago (filtering the manifest specific anyOf entries at import time instead of doing it at runtime as in the initial version of this PR).

While I was looking into that I did notice that it seems we lost a piece of the work done in #4363, related to rework the way we get the list of the schema updates to apply on the imported data, I have added it back as 78230b7 but we may likely want to move that in a separate PR and merge that part sooner.

We should discuss about this new version of the PR, in particular I'm a bit on the fence about what to do with the fact that the schema data is currently imported statically:

• to keep this first attempt simpler, at the moment I opted to define two static imports one for the mv2 data and another one with the mv3 data, then pick the right one based on the manifest version from the addon being validated
• browser-apis.js also import the schema data with a static import, at the moment I left it to refer to the unfiltered imported data (which means that as is the npm package will need to keep basically 3 copies of the schema data)
• another option would be to rework validator.js and browser-apis.js to import the schema data lazily using a dynamic import

Let's touch base about this points in the next few days, e.g. after you got a chance to take a look to this version.

[rpl]

@willdurand I cherry-picked 78230b7 from this PR to the PR to refresh the JSONSchema data from Firefox 103.0b9 (#4407), I'll rebase this patch and remove that from this PR once #4407 lands on the main branch.

[willdurand]

Let's touch base about this points in the next few days, e.g. after you got a chance to take a look to this version.

Yeah, that's actually something I forgot to let you know earlier today.. I read this new patch and your comments/concerns. We should discuss some time next week!

[willdurand]

As per our last discussion, the idea was to:

1. move these changes to a new PR with a different goal: we eventually want to simplify the JSON schema in the linter so that will be a first step (having two sets of schema for mv2 and mv3). Later, we'll merge all files to have a JSON file per manifest version. After that we'll try to simplify the structure of the JSON schema
2. switch back to the old patch (with the replacer function) to fix https://github.com/mozilla/addons-linter/issues/4367. The unproven fear was that it would impact the perfs negatively but we came to the conclusion that this was unproven and rather unlikely

[willdurand]

[...] Later, we'll merge all files to have a JSON file per manifest version [...]

I filed #4511 for that, let's file issues for:

[...] we eventually want to simplify the JSON schema in the linter so that will be a first step (having two sets of schema for mv2 and mv3) [...]

and

[...] After that we'll try to simplify the structure of the JSON schema [...]

[willdurand]

I filed https://github.com/mozilla/addons-linter/issues/4512 and https://github.com/mozilla/addons-linter/issues/4513

[rpl]

@willdurand I have restored the initial version of this PR as we agreed on, then added the test case introduced along with the other approach to provide explicit test coverage around the issue this is meant to fix.

[rpl]

@willdurand this PR is now ready for another review pass from your perspective. As I mentioned in the updated description associated to this PR I've also added a fix for the pre-existing typo in isRelevantError and added explicit test coverage for it in test.schema.js and so #4523 should now be also superseded by this PR).

[willdurand]

(also fixes #4523, including the fix for the typo in isRelevantError helper function and additional test coverage added to test.schema.js to cover the fixed typo with an explicit test case).

That fixes https://github.com/mozilla/addons-linter/issues/4521, not the PR.

[rpl]

(also fixes #4523, including the fix for the typo in isRelevantError helper function and additional test coverage added to test.schema.js to cover the fixed typo with an explicit test case).

That fixes #4521, not the PR.

right, reference to the right issue number fixed in the PR description.

[rpl]

@willdurand I've just updated this PR based on your last round of review comments (https://github.com/mozilla/addons-linter/pull/4397/commits/a14d5017569935f2d6f52251109091f2e5f6b052), this should be ready for a final review pass.

[tarikrital]

Does this fixed now? I still get errors with V3!