mozilla
Ignore 3rd party libraries during linting?
Is this a feature request or a bug?
Feature request maybe?
What is the current behavior?
Running $(npm bin)/web-ext lint -s dist gives me all sorts of warnings about some minified 3rd party code that we're using in an extension:
WARNINGS:
Code Message Description File Line Column
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to Due to both security and performance concerns, this content/parser.js 589 3
innerHTML may not be set using dynamic values which have not
been adequately sanitized. This can lead to security
issues or fairly serious performance degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to Due to both security and performance concerns, this lib/purify.js 411 7
outerHTML may not be set using dynamic values which have not
been adequately sanitized. This can lead to security
issues or fairly serious performance degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe call to Due to both security and performance concerns, this lib/purify.js 538 11
insertAdjacentHTML may not be set using dynamic values which have not
been adequately sanitized. This can lead to security
...
I'd like $ web-ext lint to ignore any files in the dist/lib/*.js directory since those are 3rd party libraries that I have zero control over.
It seems like I can do that w/ the --ignore-files flag, but then it looks like web-ext promotes those warnings to errors since it considers all those dist/lib/.js as "_FILE_NOT_FOUND" errors:
$(npm bin)/web-ext lint -s dist --ignore-files lib/*.js
Applying config file: ./package.json
Validation Summary:
errors 4
notices 1
warnings 1
ERRORS:
Code Message Description File Line Column
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script Background script could not be found at manifest.json
defined in the manifest "lib/localforage.js".
could not be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined Content script defined in the manifest could not be manifest.json
OT_FOUND in the manifest could not found at "lib/d3.js".
be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined Content script defined in the manifest could not be manifest.json
OT_FOUND in the manifest could not found at "lib/parsimmon-browserified.js".
be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined Content script defined in the manifest could not be manifest.json
OT_FOUND in the manifest could not found at "lib/purify.js".
be found.
NOTICES:
Code Message Description File Line Column
MOZILLA_COND_OF_USE Violation of Mozilla Words found that violate the Mozilla conditions of page/fbpac-targets.json
conditions of use. use. See
https://www.mozilla.org/en-US/about/legal/acceptabl…
e-use/ for more details.
WARNINGS:
Code Message Description File Line Column
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to Due to both security and performance concerns, this content/parser.js 589 3
innerHTML may not be set using dynamic values which have not
been adequately sanitized. This can lead to security
issues or fairly serious performance degradation.
What is the expected or desired behavior?
Since I'm just doing a $ web-ext lint, I want the --ignore-files flag to just ignore any errors/warnings from those specified files, but it shouldn't cause a bunch of MANIFEST_CONTENT_SCRIPT_FILE_NOT_FOUND errors. It seems like the linter is a bit over-eager with how it's ignoring those files.
Or not sure if I need a new/different flag for this use case.
Version information (for bug reports)
- Firefox version:
- Your OS and version:
- Paste the output of these commands:
node --version && npm --version && web-ext --version
$ node --version # v10.10.0
$ npm --version # 6.4.1
$ $(npm bin)/web-ext --version # 2.9.1
@pdehaan I briefly looked into this and the --ignore-files cli option should be excluding the "files that match the list of ignore patterns" from the addons-linter validation results, I'm wondering if the reason why is not working with the above command line is that the shell is expanding the globs, do you mind to try to explicitly quote the pattern? as in
web-ext lint -s dist --ignore-files "lib/*.js"
(or to add it to the config file, so that the shell can't silently expand the globs for us)
@rpl: I'm still able to reproduce it with the quoted glob using the latest web-ext (v2.9.1):
> npm info web-ext version # 2.9.1 (latest published to npm)
> $(npm bin)/web-ext --version # 2.9.1 (what I have installed locally in ./node_modules/.bin)
I can't remember which repo my original manifest.json is from, but I can still reproduce w/ the email-tabs manifest.json in https://github.com/mozilla/email-tabs/issues/79:
Even with a quoted $ web-ext lint -s addon --ignore-files='build/*.js' --self-hosted, the output is saying it cannot find any of the manifest background files (since the vendored code matches the glob ignore pattern):
Background script could not be found at "build/testpilot-ga.js".
$ npm run lint:addon
> [email protected] lint:addon /Users/pdehaan/dev/github/mozilla/email-tabs
> web-ext lint -s addon --ignore-files='build/*.js' --self-hosted || true
Applying config file: ./package.json
Validation Summary:
errors 5
notices 0
warnings 4
ERRORS:
Code Message Description File Line Column
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest Background script could not be found at "build/testpilot-ga.js". manifest.json
could not be found.
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest Background script could not be found at "build/react.production.min.js". manifest.json
could not be found.
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest Background script could not be found at "build/react-dom-server.browser.production.min.js". manifest.json
could not be found.
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest Background script could not be found at "build/purify.min.js". manifest.json
could not be found.
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest Background script could not be found at "build/emailTemplates.js". manifest.json
could not be found.
WARNINGS:
Code Message Description File Line Column
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which background.js 210 3
have not been adequately sanitized. This can lead to security issues or fairly serious
performance degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which set-html-email.js 74 7
have not been adequately sanitized. This can lead to security issues or fairly serious
performance degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which set-html-email.js 95 7
have not been adequately sanitized. This can lead to security issues or fairly serious
performance degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which set-html-email.js 189 7
have not been adequately sanitized. This can lead to security issues or fairly serious
performance degradation.
I can't remember which repo my original manifest.json is from
Actually, I just remembered, and the repo is now public: https://github.com/mozilla/fb-online-targeting
SET UP:
$ git clone [email protected]:mozilla/ad-analysis-for-facebook.git
$ cd ad-*
$ npm i
$ npm run build
But actually, this gets interesting. I am getting different output w/ --ignore-files='' and without the flag (which is expected, but not entirely):
> $(npm bin)/web-ext lint -s dist --ignore-files='lib/*.js'
Applying config files: ./package.json, ./web-ext-config.js
Validation Summary:
errors 4
notices 1
warnings 1
ERRORS:
Code Message Description File Line Column
MANIFEST_BACKGROUND_FILE_NOT_FOUND A background script defined in the manifest could Background script could not be found at "lib/localforage.js". manifest.json
not be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined in the manifest could not Content script defined in the manifest could not be found at "lib/d3.js". manifest.json
OT_FOUND be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined in the manifest could not Content script defined in the manifest could not be found at "lib/parsimmon-browserified.js". manifest.json
OT_FOUND be found.
MANIFEST_CONTENT_SCRIPT_FILE_N… A content script defined in the manifest could not Content script defined in the manifest could not be found at "lib/purify.js". manifest.json
OT_FOUND be found.
NOTICES:
Code Message Description File Line Column
MOZILLA_COND_OF_USE Violation of Mozilla conditions of use. Words found that violate the Mozilla conditions of use. See info/fbpac-targets.json
https://www.mozilla.org/en-US/about/legal/acceptable-use/ for more details.
WARNINGS:
Code Message Description File Line Column
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have content/parser.js 1 6632
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
So even though I'm ignoring lib/*.js, I'm still seeing a bunch of errors from that file.
But if I omit that --ignore-files flag and run the command again, I get lots more warnings. So I think it is almost working, but only filtering warnings and not errors properly:
> $(npm bin)/web-ext lint -s dist
Applying config files: ./package.json, ./web-ext-config.js
Validation Summary:
errors 0
notices 1
warnings 8
NOTICES:
Code Message Description File Line Column
MOZILLA_COND_OF_USE Violation of Mozilla conditions of use. Words found that violate the Mozilla conditions of use. See info/fbpac-targets.json
https://www.mozilla.org/en-US/about/legal/acceptable-use/ for more details.
WARNINGS:
Code Message Description File Line Column
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have content/parser.js 1 6632
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to outerHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/purify.js 1 3384
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe call to insertAdjacentHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/purify.js 1 4609
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/purify.js 1 4832
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/purify.js 1 4877
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/d3.js 2 17413
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
UNSAFE_VAR_ASSIGNMENT Unsafe assignment to innerHTML Due to both security and performance concerns, this may not be set using dynamic values which have lib/d3.js 2 17473
not been adequately sanitized. This can lead to security issues or fairly serious performance
degradation.
DANGEROUS_EVAL The Function constructor is eval. Evaluation of strings as code can lead to security vulnerabilities and performance issues, even in lib/d3.js 2 63804
the most innocuous of circumstances. Please avoid using `eval` and the `Function` constructor when at
all possible.'
@pdehaan ah, thanks for the additional details, I see now what it is actually happening! (and now I'm wondering how I missed it when I looked at the linting output included in the issue description :-( )
At a first glance all those validations look like they are related to the ignored files, but if we look at the File column in the linting output... they are actually related to the manifest.json file (which is not ignored as we would expect based on the --ignore-files patterns).
And so web-ext is actually making the addons-linter to ignore the expected set of files, but as a side-effect the linting rules that are processing the manifest.json file (on the addons-linter side) are not able to find the files and then those validation messages are added to the manifest.json.
The part of the addons-linter that implements those validations messages is the following one:
- https://github.com/mozilla/addons-linter/blob/9f410673d792478d43372a27b9e32dfaa178c771/src/parsers/manifestjson.js#L395-L405
which doesn't check if the file exists on disk, but it check that it is included in the files included in the scan, and the ignored files are also not part of the files:
- https://github.com/mozilla/addons-linter/blob/225c9130dff8f246ea72f1c4ac549080b4a8d57d/src/io/directory.js#L22-L24
I filed this issue in the addons-linter repo as mozilla/addons-linter#2269 (but I'll keep this issue open and marked as blocked on upstream fix)