Comments (7)
Hi there,
Thanks for this great writeup!
I wholeheartedly agree that we should at least a flag to enforce this behaviour. I will work on that soon 😄
I agree that --fail-not-found
is an appropriate flag name. --fail-whitelist-not-found
is more verbose, but it is more clear.
The other option we have is to introduce a breaking change v3.0.0
(perhaps release along-side #131) and enforce this behaviour by default with a flag to disable the behaviour. The flag could be named --pass-whitelist-not-found
. We use the --pass-enoaudit
flag currently, so --pass-*
is appropriate, and --pass-fail-not-found
is confusing.
Introducing this behaviour as default would most likely break some CIs, but this is appropriate for most environments IMO.
Would love some more feedback! CC: @andy-patt
from audit-ci.
Thanks Quinn.
Personally, I'd welcome the breaking change to have fail-by-default behaviour because
i) It means I won't forget to add the flag!
ii) I don't think anyone would intentionally whitelist items that are not relevant
from audit-ci.
I think not failing by default is reasonable as the impact of the original CVE, which was whitelisted, hasn't changed now that a fix has been made available. IMO keeping dependencies always up to date is out of scope for this tool and should be handled with other tooling. That being said, it's not a problem to use the default being discussed here - just providing my opinion.
This potentially becomes an issue in cases where the CVE reappears in the future, maybe in a different library, or maybe in the same one, but after the use of that library has materially changed. In these cases the CVE should be reassessed, but because the whitelist lingered in the config if is missed.
Can this actually happen? I would think a new CVE would have to be created. For example, CVEs always include the year. If this were to re-appear a year later, I would think the year would be updated.
I think the use case of re-assessing after the use of a library has changed is relevant, but not entirely solved by the change being discussed. Perhaps adding an expiry to the whitelist could also help.
from audit-ci.
I missed that we have the flag --show-not-found
in the current project. An additional flag --pass-whitelist-not-found
will introduce overlap that may lead to confusion regarding the mix of these flags.
I propose to merge them into a single flag that allows users to specify their desired behaviour. The configurable behaviours would be something along the lines of: ignore
, warn
(print to console in yellow), and error
.
Some ideas for the flag name are:
--redundant-whitelist <option>
--excess-whitelist <option>
To exhaust all possibilities for this configuration, there's also the option of having per-package overrides for the ignore
, warn
, and error
. For example, we frequently get the request to ignore devDependencies. I could see users wanting to warn
/ignore
for all devDependencies, but error
on all dependencies. If they used the whitelist
config option and listed all devDependencies to make it so that they don't break the build (not recommended, but it happens), any non-vulnerable packages with the error
flag would break the build.
To me, it seems like more work as the end-user to handle manually configuring which not-found vulnerable dependencies to ignore
/warn
/error
than to simply remove them from the whitelist. For this reason, I don't think it's appropriate to support the per-package behaviour.
from audit-ci.
Can this actually happen? I would think a new CVE would have to be created. For example, CVEs always include the year. If this were to re-appear a year later, I would think the year would be updated.
So, probably more into than anyone needs to read because I'm happy with the flag suggestions above, but just to clarify my edge case:
The CVE 'reappearance' would be if the 'unpatched' version of a transitive dependency that was previously 'fixed' for one library, appears as a transitive dependency in another library.
Say some-lib
has a transitive dependency ropey-lib
and ropey-lib
v2.1 has a CVE against it. It's in our dev dependencies and not a big risk so we whitelist the CVE.
Time passes...
some-lib
replaces it's dependency on ropey-lib
with something else in its next major version. The CVE no longer applies to some-lib
Time passes
A develop adds cool-new-lib
to the production dependencies, but cool-new-lib
uses the old version of some-lib
which has ropey-lib
at v2.1, only now it's in production there's a potential vulnerability. Unfortunately, we don't spot it because we left the CVE in the whitelist.
And if you followed that, well done! It's niche, I admit.
from audit-ci.
TL;DR: the print
options seems to be a reasonable default in all workflows if you use the various whitelist options mindfully.
Let's consider some workflows:
If a user is using a lock file (yarn.lock
or package-lock.json
), dependencies (including transitive) will not be updated until users (or a bot) manually perform a {npm|yarn} update
. Upon an update, existing whitelists will be redundant. From a workflow perspective, it seems reasonable to address the redundant whitelist for cleanliness purposes, but keeping them there doesn't introduce a vulnerability unless they cover an unknown vulnerability (as per @dgjbge's use-case). Either the print
option or the error
option seem to be a reasonable default in this case.
If a user is not using a lock file, the transitive dependencies will be updated automatically. The whitelist will be redundant, but in an intrusive manner. The build will break because a vulnerability has been fixed automatically. IMO, the print
option seems to be the most reasonable default in this case.
There are three types of whitelists that users can perform with this tool: --whitelist [packages]
, --advisories [advisory numbers]
, and --path-whitelist [paths]
.
In the case that @dgjbge provided, this would be a non-issue if the original whitelist was performed using --path-whitelist
(e.g. 123|some-lib>ropey-lib
). The newly introduced cool-new-lib
would not be within the whitelisted paths (it would need to use 123|cool-new-lib>ropey-lib
), so the audit will fail correctly. If a different prototype pollution existed in the future for ropey-lib
, it would get a new advisory, so it's still "future-proof". If a whitelist is redundant with the usage of a path whitelist, there's no reason to error
since it's inherently not an error, just outdated/redundant. Thus, print
is desirable here.
However, if the author used either the advisories
option or the whitelist
option then a new vulnerability could slide through unnoticed. Using the whitelist
option is inherently unsafe as you're saying ignore all advisories with this package, so I am not concerned with this workflow being "future-proofed".
Thus, this problem comes down to the usage of the advisories
option. In general, there is greater risk of transitive vulnerabilities being introduced if the user decides to use advisories
instead of path-whitelists
. This risk should be clearly communicated to users. Both print
and error
are reasonable in this case (print
if it's a devDependency probably, error
if it's a production dependency).
This package doesn't make distinctions between dependencies and devDependencies for many reasons. All in all, the print
options seems to be a reasonable default in all workflows if you use the various whitelist options mindfully.
from audit-ci.
Just a note on the module path whitelist as sometimes it's not practical.
Last month we many hundreds of reported vulns in per project due to a CVE in "acorn" that was included many times in the many dependencies of jest and webpack.
So, while path whitelists might be the 'right way', I don't think anyone is going to add 100's of paths over multiple repos!
from audit-ci.
Related Issues (20)
- Drop support for Node <12 HOT 1
- Long summary output for only one vulnerable advisory HOT 6
- Cannot convert undefined or null to object Exiting HOT 9
- Support allowlisting private packages by module HOT 7
- Recommend pinning to commit SHA or release tag HOT 3
- Add expiration time for allow list items HOT 1
- Allow notes for allowlist items HOT 2
- [Feature] Support Gitlab SAST report-type HOT 2
- Let the severity level influence the json output HOT 1
- Fail on unmatched ignores HOT 1
- Invalid JSON config file when using new allowlist NSPRecord syntax HOT 3
- Add support for registry flag for PNPM HOT 1
- Support Yarn's `--exclude` HOT 2
- Handle errors from Yarn Berry more gracefully HOT 2
- Tests should include all major Yarn versions HOT 1
- packages starting with "@" are not working in allowlist HOT 2
- Replace event-stream with something secure and supported HOT 3
- The audit report format changed? HOT 2
- CI commands fail because no version 7
- audit-ci is Incompatible with yarn version 4 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from audit-ci.