Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Invalid host_permissions should be reported as warning in MV3 and filtered out in MV2 #3891

Merged
merged 2 commits into from
Sep 7, 2021

Conversation

rpl
Copy link
Member

@rpl rpl commented Sep 3, 2021

Followup to #3873, Fixes #3888

This PR includes the following changes:

  • two new rules code: MANIFEST_HOST_PERMISSIONS (used to report invalid permission as a warning) and MANIFEST_BAD_HOST_PERMISSION (reported as error, only used when a permission doesn't have the expected type string), as the ones we already have for the permissions and optional_permissions manifest keys
  • ensure that invalid host_permissions (the ones that are still in the expected string form) are reported as warnings (as we do already for permissions and optional_permissions)
  • filter out the additional linting error that the top level anyOf schema entry (recently introduced because of the difference in the schema definition for the "permissions" manifest key in manifest_version 2 and manifest_version 3), because that is redundant (the schema entries nested into it have already reported their validation errors) and it would trigger a JSON_INVALID that is reported as an error even when the nested schema entries reported only warnings
  • changes to the tests to properly cover the expected behaviors

As a side note, the ManifestJSONParser's errorLookup method could use a refactoring, it has never been that much readable, but it is also increasing in complexity and it would be good to reorganize it to make it easier to read and maintain over the time, but it may be reasonable to defer that to a separate follow up (e.g. to keep this incremental change to fix the permissions validations smaller).

@rpl rpl requested a review from willdurand September 3, 2021 12:13
@rpl rpl added component:MV3 Issues related to Manifest Version 3 component:schema priority:p1 labels Sep 3, 2021
@rpl rpl force-pushed the fix/warn-on-invalid-host-permissions branch from 1893ae8 to 3948963 Compare September 3, 2021 12:14
@rpl rpl mentioned this pull request Sep 3, 2021
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.

An additional question: should we update the (generated) docs?

src/messages/manifestjson.js Outdated Show resolved Hide resolved
src/messages/manifestjson.js Outdated Show resolved Hide resolved
src/parsers/manifestjson.js Outdated Show resolved Hide resolved
tests/unit/parsers/test.manifestjson.js Outdated Show resolved Hide resolved
@rpl
Copy link
Member Author

rpl commented Sep 6, 2021

Looks good, thanks.

An additional question: should we update the (generated) docs?

@willdurand eh that's a good point, they should... which is making me think: shouldn't we check as part of the CI job that the rules docs are updated with the content of rules.md and there is no pending change that should have been part of the PR?

@willdurand
Copy link
Member

@willdurand eh that's a good point, they should... which is making me think: shouldn't we check as part of the CI job that the rules docs are updated with the content of rules.md and there is no pending change that should have been part of the PR?

I thought that was the case already...

@rpl
Copy link
Member Author

rpl commented Sep 6, 2021

@willdurand eh that's a good point, they should... which is making me think: shouldn't we check as part of the CI job that the rules docs are updated with the content of rules.md and there is no pending change that should have been part of the PR?

I thought that was the case already...

Looking to the CI job, that should be the case:

npm run build-rules
# This will return a non-zero exit code if content in the docs/
# folder has changed. When this fails, run the following command
# and commit the changes:
#
# npm run build-rules
#
git diff --name-only --exit-code docs

@willdurand I'll double-check why that step isn't making this PR to fail as we assume it should.

@rpl
Copy link
Member Author

rpl commented Sep 6, 2021

@willdurand eh that's a good point, they should... which is making me think: shouldn't we check as part of the CI job that the rules docs are updated with the content of rules.md and there is no pending change that should have been part of the PR?

I thought that was the case already...

Looking to the CI job, that should be the case:

npm run build-rules
# This will return a non-zero exit code if content in the docs/
# folder has changed. When this fails, run the following command
# and commit the changes:
#
# npm run build-rules
#
git diff --name-only --exit-code docs

@willdurand I'll double-check why that step isn't making this PR to fail as we assume it should.

bah, if I run the job locally (using circleci cli tools and docker) it does fail as expected if changes to docs/index.html were not committed. I'm going to giving another look into why it doesn't seem to fail on circleci as part of the jobs triggered by the pull requests.

@rpl rpl force-pushed the fix/warn-on-invalid-host-permissions branch 2 times, most recently from 52a7e65 to 2e5c512 Compare September 6, 2021 17:16
@rpl rpl requested a review from willdurand September 6, 2021 17:17
@rpl rpl force-pushed the fix/warn-on-invalid-host-permissions branch from ebc16ec to 2e5c512 Compare September 6, 2021 17:33
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

src/messages/manifestjson.js Outdated Show resolved Hide resolved
@rpl rpl force-pushed the fix/warn-on-invalid-host-permissions branch from 2e5c512 to 134c23e Compare September 6, 2021 19:33
@rpl rpl merged commit 65b4ed7 into mozilla:master Sep 7, 2021
@willdurand willdurand removed priority:p1 component:schema component:MV3 Issues related to Manifest Version 3 labels Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants