search

mozilla / web-ext

A command line tool to help build, run, and test web extensions
Mozilla Public License 2.0
2.71k stars 338 forks source link

Ignore 3rd party libraries during linting? #1376

Open pdehaan opened 6 years ago

pdehaan commented 6 years ago

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)

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
rpl commented 6 years ago

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

pdehaan commented 6 years ago

@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-tabs@0.1.0 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.
pdehaan commented 6 years ago

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 git@github.com: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.'
rpl commented 6 years ago

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

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:

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)

smashedr commented 4 months ago

Upstream issue has been closed for a year now. Is there any plans to fix this? Or should we just remove web-ext lint from ci?

enchanted-sword commented 2 months ago

Bumping this again, it's been a constant annoyance as long as I've worked on extensions.