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

rpl · 2022-07-05T13:20:18Z

Fixes #4367
(also fixes #4521, 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).

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).

rpl · 2022-07-05T13:24:48Z

src/schema/validator.js

+    const schemaObject = JSON.parse(
+      JSON.stringify(this.schemaObject, replacer, 2)
+    );


@willdurand this is currently traversing the JSONSchema data by using JSON.stringify, which is likely not the best way to traverse the json objects given that it is stringifying all data and then parsing it again, I used it mainly as a quick way to give the approach a try and confirm if that would be a possible way to avoid these unexpected errors.

Besides the JSON.stringify trick, as I also mentioned in the PR description comment 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).

We may also consider looking into that approach now and see if we can get an alternative patch without a too big of an effort.

Let me know wdyt.

(in the PR description I tried to explain in a bit more detail what is going on in practice, but I'm happy to have a chat if that description didn't yet help you to get a good picture of the underlying issue).

codecov · 2022-07-05T13:31:14Z

Codecov Report

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

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%)`	⬇️
src/schema/validator.js	`99.28% <100.00%> (+0.12%)`	⬆️

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

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

rpl · 2022-07-11T13:22:34Z

@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 · 2022-07-14T14:52:49Z

@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 · 2022-07-14T19:01:51Z

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 · 2022-09-19T08:56:54Z

As per our last discussion, the idea was to:

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
switch back to the old patch (with the replacer function) to fix Host permissions regarded as invalid in permissions if other invalid permissions are present #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 · 2022-10-13T09:07:54Z

[...] 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 · 2022-10-13T09:11:00Z

I filed #4512 and #4513

rpl · 2022-10-14T13:36:26Z

@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.

src/schema/validator.js

…arned on in mv2 extensions

…on validation 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).

rpl · 2022-10-24T13:53:20Z

@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 · 2022-10-24T14:15:35Z

(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.

rpl · 2022-10-24T14:30:32Z

(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.

willdurand

Looking good, a few questions and typos.

src/schema/validator.js

tests/unit/parsers/test.manifestjson.js

tests/unit/schema/test.schema.js

…wedManifestVersionsRange

rpl · 2022-10-27T10:38:01Z

@willdurand I've just updated this PR based on your last round of review comments (a14d501), this should be ready for a final review pass.

wpplumber · 2023-03-27T09:23:12Z

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

rpl commented Jul 5, 2022

View reviewed changes

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from 1cf744b to 4ee032b Compare July 5, 2022 13:28

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from 58676a5 to 79fb846 Compare July 11, 2022 13:12

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from 79fb846 to ed1cf84 Compare October 14, 2022 13:32

rpl requested a review from willdurand October 14, 2022 13:33

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from ed1cf84 to 9575b85 Compare October 14, 2022 13:35

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from 9575b85 to da1a7bf Compare October 14, 2022 15:03

willdurand reviewed Oct 14, 2022

View reviewed changes

src/schema/validator.js Outdated Show resolved Hide resolved

test: added coverage for valid host permission expected to don't be w…

ca11040

…arned on in mv2 extensions

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from da1a7bf to edf8e6c Compare October 24, 2022 13:45

rpl added 2 commits October 24, 2022 15:47

fix: Fixed typo in isRelevantError schema validator helper function

0447648

rpl force-pushed the fix/valid-mv2-host-permission-marked-invalid branch from edf8e6c to 363727b Compare October 24, 2022 13:48

rpl requested a review from willdurand October 24, 2022 13:49

willdurand requested changes Oct 24, 2022

View reviewed changes

chore: Fixed typos and rename addon to currentAddon in validator allo…

a14d501

…wedManifestVersionsRange

rpl requested a review from willdurand October 27, 2022 10:38

chore: Turn addonManifestVersion into a Validator class getter

3c2a0a7

willdurand approved these changes Oct 27, 2022

View reviewed changes

rpl merged commit 9295bec into master Oct 27, 2022

rpl deleted the fix/valid-mv2-host-permission-marked-invalid branch October 27, 2022 12:39

willdurand mentioned this pull request Nov 3, 2022

Omit linting warnings and errors related to manifest_version 3 extension when the addons-linter max allowed manifest version is set to 2 #3737

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rpl commented Jul 5, 2022 •

edited

rpl Jul 5, 2022

codecov bot commented Jul 5, 2022 •

edited

rpl commented Jul 11, 2022

rpl commented Jul 14, 2022

willdurand commented Jul 14, 2022

willdurand commented Sep 19, 2022

willdurand commented Oct 13, 2022

willdurand commented Oct 13, 2022

rpl commented Oct 14, 2022

rpl commented Oct 24, 2022

willdurand commented Oct 24, 2022

rpl commented Oct 24, 2022

willdurand left a comment

rpl commented Oct 27, 2022

wpplumber commented Mar 27, 2023

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

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

Conversation

rpl commented Jul 5, 2022 • edited

rpl Jul 5, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2022 • edited

Codecov Report

rpl commented Jul 11, 2022

rpl commented Jul 14, 2022

willdurand commented Jul 14, 2022

willdurand commented Sep 19, 2022

willdurand commented Oct 13, 2022

willdurand commented Oct 13, 2022

rpl commented Oct 14, 2022

rpl commented Oct 24, 2022

willdurand commented Oct 24, 2022

rpl commented Oct 24, 2022

willdurand left a comment

Choose a reason for hiding this comment

rpl commented Oct 27, 2022

wpplumber commented Mar 27, 2023

rpl commented Jul 5, 2022 •

edited

codecov bot commented Jul 5, 2022 •

edited