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

audit: env selection for report #125

Closed
wants to merge 2 commits into from
Closed

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Dec 17, 2018

Select dependency environments with --only, --also and --production for
reports as well, instead of just for audit fix. Still reports the
filtered advisories, but changes the exit code (as is done with
advisories below --audit-level). Tests need updating due to the new way
of counting vulnerabilities.

See https://npm.community/t/3959

Select dependency environments with --only, --also and --production for 
reports as well, instead of just for audit fix. Still reports the 
filtered advisories, but changes the exit code (as is done with 
advisories below --audit-level). Tests need updating due to the new way 
of counting vulnerabilities.

See https://npm.community/t/3959
@larsgw larsgw requested a review from a team as a code owner December 17, 2018 20:56
@sneakypete81
Copy link

Thanks very much for doing this.

Still reports the filtered advisories

Maybe I've misunderstood, but if I run npm audit --only=prod I wouldn't expect to see any advisories from dev dependencies.

@larsgw
Copy link
Contributor Author

larsgw commented Dec 17, 2018

Maybe I've misunderstood, but if I run npm audit --only=prod I wouldn't expect to see any advisories from dev dependencies.

@sneakypete81 I know, but that requires either modifying the audit report, which could have unintended side effects, or changing all the reporters. Since --audit-level also reports advisories under the set severity (correct me if I'm wrong), this seemed like warranted behavior.

@mikecbrant
Copy link

Anxiously awaiting merge. Thanks, @larsgw

@zkat zkat added semver:minor new backwards-compatible feature needs-discussion labels Jan 7, 2019
@evilpacket
Copy link

I did a quick test of --audit-level and it worked as I would intend it to.

I would expect these flags to be passed along to the reporters and for them to act appropriately, such as @sneakypete81 suggests when I say --only=prod to not show advisories that pertain to dev dependencies.

Looks like npm audit help would also need some updating to represent the new flags but I'm not sure the standard that's been set forth by the cli team for these things.

@iarna
Copy link
Contributor

iarna commented Jan 8, 2019

As Adam suggests, docs should be added to doc/cli/npm-audit.md and doc/misc/npm-config.md

Reporter filters go in https://github.com/npm/npm-audit-report/pulls and should be PRed there (please add a note here when they are)

@larsgw
Copy link
Contributor Author

larsgw commented Jan 17, 2019

I added the docs. --audit-level doesn't filter the reports on my end (v6.5.0), should I add that as well as the env filters?

$ npm ini -y
$ npm i underscore.string@3.3.4
$ npm audit
                                                                                
                       === npm audit security report ===                        
                                                                                
# Run  npm install underscore.string@3.3.5  to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/745                       │
└───────────────┴──────────────────────────────────────────────────────────────┘


found 1 moderate severity vulnerability in 3 scanned packages
  run `npm audit fix` to fix 1 of them.

$ echo $?
1

$ npm audit --audit-level high
                                                                                
                       === npm audit security report ===                        
                                                                                
# Run  npm install underscore.string@3.3.5  to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/745                       │
└───────────────┴──────────────────────────────────────────────────────────────┘


found 1 moderate severity vulnerability in 3 scanned packages
  run `npm audit fix` to fix 1 of them.

$ echo $?
0

@zkat zkat force-pushed the release-next branch 5 times, most recently from db63b89 to b09bc8c Compare January 23, 2019 18:36
DrSensor added a commit to DrSensor/bot-byte that referenced this pull request Jan 30, 2019
mischah added a commit to micromata/Baumeister that referenced this pull request Feb 7, 2019
The security script might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/Baumeister that referenced this pull request Feb 7, 2019
The security script might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/Baumeister that referenced this pull request Feb 8, 2019
The security script might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/cli-error-notifier that referenced this pull request Feb 11, 2019
The security script might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/cli-error-notifier that referenced this pull request Feb 11, 2019
The security script might come back when this is merged and released:
npm/cli#125
@perrosen
Copy link

Any movement on this? Is there something I can do to help speeding this up? This is one of my most wanted features for npm audit since launch. Seeing CI builds fail because of dev dependencies is becoming a real annoyance.

mischah added a commit to micromata/generator-baumeister that referenced this pull request Feb 18, 2019
The security check might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/generator-baumeister that referenced this pull request Feb 18, 2019
The security script might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/generator-baumeister that referenced this pull request Feb 18, 2019
…d and gone

The security check might come back when this is merged and released:
npm/cli#125
mischah added a commit to micromata/generator-baumeister that referenced this pull request Feb 18, 2019
The security script might come back when this is merged and released:
npm/cli#125
@MNF
Copy link

MNF commented Dec 8, 2019

Is it similar to implemented “Enable production flag for npm audit #202 ”?

@IPWright83
Copy link

Are there updates on this at all? I've got builds failing on our pipeline (and similarly don't want to auto fix them during the build), but it's all because of jest dependencies - which really don't matter at runtime.

@lucasfevi
Copy link

@IPWright83 there is now a production flag for npm audit on npm version v6.10+. Check the release notes: https://github.com/npm/cli/releases/tag/v6.10.0

@darcyclarke darcyclarke added the Priority Backlog a "backlogged" item that will be tracked in a Project Board label Mar 10, 2020
@SoerenHenning
Copy link

Even though the --production flag is a nice improvement, we could only take full advantage of it if accompanied by a --development flag or so. Our use case is as follows: We would like to run npm audit as part of our build pipeline for both production and development dependencies. However, the build process should only fail for vulnerabilities in production. For vulnerabilities in dev dependencies, only a warning should be generated.

@darcyclarke darcyclarke added Release 6.x work is associated with a specific npm 6 release and removed Priority Backlog a "backlogged" item that will be tracked in a Project Board labels Oct 1, 2020
@darcyclarke darcyclarke modified the milestone: OSS - Sprint 17 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.