naugtur / npm-audit-resolver

Apache License 2.0
121 stars 28 forks source link

Feature requests #1

Closed naugtur closed 3 years ago

naugtur commented 6 years ago

Request features in comments, I'll add it to the list if I accept the request.

Planned

Possible

CADBOT commented 6 years ago

The ability to specify a different registry than the .npmrc file. Mine specifies a custom registry, but I'd still like to use the normal one when running audit-resolve and check.

naugtur commented 6 years ago

@CADBOT interesting. This uses npm under the hood and it's intended (I started the process) as a future npm audit feature, so not sure where to go from here. I'll see if npm accepts a cli argument to override repo url, and I'll pass it through in that case. Got other thoughts?

CADBOT commented 6 years ago

There is a registry flag that can be used: npm run audit --registry https://registry.npmjs.org/ at which point the registry configured in the rc file is overridden.

CADBOT commented 6 years ago

Wrote up a quick PR that has a working example

joh-klein commented 5 years ago

Is there a chance to skip/ignore all DevDependencies? I know there is one for npm audit, but only when you do npm audit fix

naugtur commented 5 years ago

currently you can ignore low severity automatically, but not dev dependencies. I didn't put that in because with the data I get as input, I wasn't sure what would happen if something is both in deps and devDeps

I'd consider adding this as an option defined in the audit-resolve file. How would you want to ignore them? In resolve-audit or in check-audit, or both?

joh-klein commented 5 years ago

I thought both, to be honest. And why not do it as an cli argument?

naugtur commented 5 years ago

audit-resolve file is where this tool puts your decisions about what's safe to ignore etc. so it made sense to me to keep it there. Any reasons skipping dev dependencies should be treated as unrelated? Should a run with dev and without dev share the same audit-resolve file in your opinion? I'm not sure if I see a situation where that'd be useful, but happy to get feedback!

stewartduffy commented 5 years ago

Any chance for an option for longer remind timeframe. 24 hours can be difficult to deal with. Propose

naugtur commented 5 years ago

Public announcement: Remind option will not change, but I'm planning to add expiry to ignore.

naugtur commented 5 years ago

@stewartduffy npm-audit-resolver@next supports expiring ignores, which effectively does what you wanted.

Undistraction commented 5 years ago

FWIW I definitely like the idea of being able to ignore all advisories for dev dependencies. How about allowing certain packages to be ignored? Guess you have the same problem again of how to handle advisories that apply both to ignored packages and not-ignored packages.

stewartduffy commented 5 years ago

Hey @naugtur sorry I dropped out of this conversation for ages - I got pulled on a number of different projects and got busy. I will update out app to use npm-audit-resolver@next today and give feedback on how we go. Thanks for this! Sounds like it will do just the trick.

stewartduffy commented 5 years ago

@naugtur I just tried it with the expiring ignore for 1 month. Works well thanks!

jansu76 commented 5 years ago

Suggestion: possibility to have check-audit fail only if some high or critical issues were found. Not when there there were low issues.

Basically a --ignoreLow parameter for check-audit, would do what I am after. (by the way, I did not get resolve-audit --ignoreLow to work. It claimed ✔ automatically ignore low severity issue, but ignores did not appear in audit-resolve.json)

Reasoning: we currently ignore all low severity vulnerabilities. We have e.g. a dev dependency @vue/cli-plugin-unit-jest that has a lot (tens) of low severity issues. Having all those issues configured in audit-resolve.json makes the configuration file very noisy. Especially since there is no way to see which entry ignores a low issue and which entry ignores a critical issue.

Having an audit-resolve.json that contains many low severity issues makes people not care about it, and not care about changing it, so much. If it contained only serious issues, changing it would be a thing that requires careful consideration.

Kind of a duplicate of the earlier request about ignoring devDependencies. I think that also for that use case it might be better to keep dev dependencies out of audit-resolve.json, and instead add the ability to limit checking in check-audit to ignore alerts from dev dependencies.

 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runtime>jest-snapshot>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-snapshot>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-runtime>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runtime>jest-message-util>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-config>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-config>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-runtime>jest-config>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runtime>jest-config>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-haste-map>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-haste-map>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-runtime>jest-haste-map>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runtime>jest-haste-map>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runner>jest-runtime>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>jest-runtime>micromatch>braces
 -  devDependencies: @vue/cli-plugin-unit-jest>jest>jest-cli>micromatch>braces
--------------------------------------------------
 😱   Unresolved issues found!
naugtur commented 5 years ago

Yeah, I might have broken the ignoreLow param in the big rewrite for v2. If you look at the new audit file format it has the rules section in there. It's meant to host features like defining the severity below which ignores should be automated. Please create a new issue on the repo and write down how you see it. We'll collaborate on the idea a bit. I'd rather build something better than just fix the ignoreLow switch.

joebowbeer commented 4 years ago

Recent versions of npm audit can ignore devDependencies using the --production flag.

I'd like audit-check to be at least as capable as npm audit.

Can I pass-through options such as --production from audit-check to npm audit?

naugtur commented 4 years ago

@joebowbeer not implemented. Agree it's necessary. I need to think if this should be a single flag that's passed or if something more generic makes sense since we're now supporting npm and yarn.

Could you open an issue? PR welcome - I can guarantee I won't be able to get to implement it till Tuesday at least

joebowbeer commented 4 years ago

I created https://github.com/naugtur/npm-audit-resolver/issues/18

coreysnyder commented 4 years ago

Is there any way to define which audit-resolve.json is used? I have a micro-service architecture and 27 web repos all pull in a node_module ui-scaffolding which has 99% of the vulnerabilities that come up. I am looking for a way for all of my web projects to reference/mix-in the audit-resolve.json in that library and apply any web-project specific ignores. That way when I fix an ignore in ui-scaffolding, I don't have to open up 27 PRs to do the same for all the top level libraries that use it.

Top Level Project

naugtur commented 4 years ago

@coreysnyder that's definitely interesting. For now I believe you can only use symlinks.

But there's a few ways to solve this. Let's pop out to an issue just for this and discuss. Please open the issue on the audit-resolver-core repo.

I'd like to think about ways we could reference other files with rules. One difficulty is paths differ between installation so if depth of sub dependency differs, decision from resolve file doesn't apply.

We'll discuss more in the issue.

Thanks for bringing this up!

ghost commented 4 years ago

Feature request: calling "audit-resolve" (or "audit-resolve --clean") removes decisions from the json that are no longer relevant. Example After an "npm audit fix" i want to see the whitelist get shorter, i do not want to update all decision timestamps, which happens if i empty the file and call audit-resolve again.

naugtur commented 4 years ago

@ion2s-JM I can relate. Good idea. Please create a separate issue where we could discuss details. I'd suggest a PR too, but it might require changes to audit-resolver-core as well. let's talk more in the issue.

jmac105 commented 4 years ago

Feature request: Make 'resolve-audit' exit with a non-zero exit code if it is run from a CI server, potentially by using https://www.npmjs.com/package/is-ci or similar.

I've noticed that if a CI server is incorrectly setup to call 'resolve-audit' rather than 'check-audit', then it will exit with a 0 exit code regardless of whether or not there vulnerabilities that needed to be addressed. This means the audit will appear to pass.

I can't think of any use case where you would deliberately run 'resolve-audit' from a Ci server. Happy to try raising a PR for this if you give the go ahead.

naugtur commented 4 years ago

@jmac105 interesting idea. Instead of pulling in a new dependency, I'd rather exit(1) every time. What do you think?

jmac105 commented 4 years ago

Potentially, looking again at the output from the CI server I wonder if there's an exit condition check that could be added somewhere? I'm interested to know what's causing it to exit here and no just hang waiting for input that will never come?

[13:34:38]  Step 3/10: NPM Audit (Node.js NPM) (7s)
[13:34:38]  [Step 3/10] Executing npm via wrapping shell script
[13:34:38]  [Step 3/10] Starting: /data/teamcity-agent/temp/agentTmp/wrapper53360257518953549.sh run audit
[13:34:38]  [Step 3/10] in directory: /data/teamcity-agent/work/34ba38c1bb7b80/sourceCode
[13:34:38]  [Step 3/10] npm run audit (7s)
[13:34:38]  [npm run audit] 
[13:34:38]  [npm run audit] > internal-package@1.0.16 audit /data/teamcity-agent/work/34ba38c1bb7b80/sourceCode
[13:34:38]  [npm run audit] > resolve-audit
[13:34:38]  [npm run audit] 
[13:34:38]  [npm run audit] >>>> npm audit --json 
[13:34:45]  [npm run audit] >>>> exit: 1
[13:34:45]  [npm run audit] 
[13:34:45]  [npm run audit] --------------------------------------------------
[13:34:45]  [npm run audit]  minimist needs your attention.
[13:34:45]  [npm run audit] 
[13:34:45]  [npm run audit] [ moderate ] Prototype Pollution
[13:34:45]  [npm run audit]  vulnerable versions <1.2.3 found in:
[13:34:45]  [npm run audit]  - dependencies: another-internal-package>etl>csv-parser>minimist
[13:34:45]  [npm run audit]  - dependencies: etl>csv-parser>minimist
[13:34:45]  [npm run audit] _
[13:34:45]  [npm run audit]  d) show more details and ask me again
[13:34:45]  [npm run audit]  r) remind me in 24h
[13:34:45]  [npm run audit]  i) ignore paths
[13:34:45]  [npm run audit]  del) Remove all listed dependency paths
[13:34:45]  [npm run audit]  s) Skip this
[13:34:45]  [npm run audit]  q) Quit
[13:34:45]  [npm run audit] What would you like to do? 
[13:34:45]  [Step 3/10] Process exited with code 0

If I run resolve-audit locally and ctrl-c out of it when it prompts me for input it recognises that and exits with code 2, hard to replicate the behaviour seen on the ci server

naugtur commented 4 years ago

please move to a separate issue and we can discuss in more detail there

zcabjro commented 4 years ago

Small but nice usability improvement would be to use a human-readable date for expires. Would make reviewing audit-resolve.json easier for team members that would otherwise have to verify or trust that the epoch time is both desirable and correct.

Undistraction commented 4 years ago

Yarn now offers a --groups flag which allows you to choose not to audit development dependencies. It would be really helpful if this could be supported.

https://classic.yarnpkg.com/en/docs/cli/audit/

naugtur commented 4 years ago

@Undistraction all flags you set on check-audit are passed down to yarn (except for the yarn flag itself :) )

Undistraction commented 4 years ago

@naugtur Ah! Amazing. Thank you.

emilong commented 3 years ago

I'd love to be able to add a comment about why I chose to ignore a warning for both myself and teammates who might wonder about it in the future. :)

naugtur commented 3 years ago

@emilong That's one of the oldest pending features. I think I added the reason field to the JSON schema already, but didn't come up with a good idea how to take the input without making the process longer for ppeople not using it.

drswobodziczka commented 3 years ago

Hello @naugtur !

It would be nice to have the resolver just to forward exit status received from npm-audit.

Namely suppose the following npm script:

"security:check": "node node_modules/npm-audit-resolver/check.js --audit-level=high"

^ it just fails for any not yet resolved issue (however logs exit(0) status from npm audit). I would rather expect it to log all the _lower_thanhigh ones -- but PASS in the same time, respecting params given to npm-audit.

naugtur commented 3 years ago

The whole point of check-audit is to only return 0 when npm audit returns positive. What exactly would it do in all cases? When something is found, when something should be ignored etc.

Want to work on listing a sort of user story with all permutations?