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

npm 7 with audit-level set fails the install when audit warnings exist #2715

Closed
ljharb opened this issue Feb 17, 2021 · 17 comments
Closed

npm 7 with audit-level set fails the install when audit warnings exist #2715

ljharb opened this issue Feb 17, 2021 · 17 comments
Labels
Bug thing that needs fixing Needs Discussion is pending a discussion Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@ljharb
Copy link
Collaborator

ljharb commented Feb 17, 2021

In npm 6 npm install was unaffected by the audit-level config setting. It only affected the exit code of npm audit itself. In npm 7 this behavior has been carried over to npm install.

Current Behavior:

If I set audit-level then npm install exits uncleanly if there are vulnerabilities found during install that match that level or higher.

Expected Behavior:

The exit status of npm install should be unaffected by the audit-level setting.

Steps To Reproduce:

Make a new package that depends on "minimist": "~1.1.3".

  • npm install passes in both npm 6 and npm 7.

  • npm audit fails in both npm 6 and npm 7.

  • NPM_CONFIG_AUDIT_LEVEL=low npm install passes in npm 6, but fails in npm 7.

  • NPM_CONFIG_AUDIT_LEVEL=low npm audit fails in npm 6 and npm 7.

Environment:

  • npm: v6.14.11 and v7.5.4
@ljharb ljharb added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Feb 17, 2021
ljharb added a commit to es-shims/RegExp.prototype.flags that referenced this issue Feb 21, 2021
ljharb added a commit to es-shims/Function.prototype.name that referenced this issue Feb 22, 2021
ljharb added a commit to es-shims/Array.prototype.every that referenced this issue Feb 23, 2021
ljharb added a commit to es-shims/Array.prototype.some that referenced this issue Feb 23, 2021
ljharb added a commit to caolan/forms that referenced this issue Feb 24, 2021
@wraithgar
Copy link
Member

There is a low level advisory for minimist at that level, so if you set the audit level to low it will exit w/ a statusCode of 1. If you set the level to info it will exit with a statusCode of 0.

From the readme in npm-audit-report:

level of vulnerability that will trigger a non-zero exit code (set to 'none' to always exit with a 0 status code

This seems to be working as intented. I set my level to info and got an exitCode of 0.

@wraithgar wraithgar removed the Needs Triage needs review for next steps label Mar 23, 2021
@wraithgar
Copy link
Member

Our config page in "using npm" says the same thing:

The minimum level of vulnerability for npm audit to exit with a non-zero exit code.

@ljharb
Copy link
Collaborator Author

ljharb commented Mar 23, 2021

@wraithgar so this was an intentional change in npm 7? can you point me to where that change was decided?

@ljharb
Copy link
Collaborator Author

ljharb commented Mar 23, 2021

Also, when i set it to “info” i get:

audit-level="info" set in environment
npm WARN invalid config Must be one of: low, moderate, high, critical, none, null

none does work tho.

@wraithgar
Copy link
Member

Yep looks like info isn't in the list. will make a PR for that, and leverage our fancy new parameter usage output to add it to the npm usage -h output

@ljharb
Copy link
Collaborator Author

ljharb commented Mar 23, 2021

@wraithgar i still think this should be reopened, since it was an unintentional change in npm 7. The install is not supposed to fail even if there are audit warnings, and that's what's happening here.

@wraithgar
Copy link
Member

Ah I see what you mean, the npm audit behaves as it should but npm install doesn't

@wraithgar wraithgar reopened this Mar 23, 2021
@wraithgar wraithgar added the Priority 2 secondary priority issue label Mar 23, 2021
@ljharb
Copy link
Collaborator Author

ljharb commented Mar 23, 2021

Yes, exactly right.

@wraithgar
Copy link
Member

wraithgar commented Mar 23, 2021

Ok after a bit of debugging I think this is still acting as it should.

If I set my warning level to info (which is the default) it will exit uncleanly because we have a low vulnerability.
If I set my warning level to low it will exit uncleanly because we have a low vulnerability.
If I set my warning level to moderate it will exit cleanly because we do not have any moderate or higher vulnerabilities.

This is the behavior now (in release-next because it has the fix to reinclude info as an alert level

$ node ../cli install --audit-level info;echo $status

up to date, audited 3 packages in 702ms

1 low severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
1
$ node ../cli install --audit-level low;echo $status

up to date, audited 3 packages in 935ms

1 low severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
1
$ node ../cli install --audit-level moderate;echo $status

up to date, audited 3 packages in 536ms

1 low severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
0

@ljharb
Copy link
Collaborator Author

ljharb commented Mar 23, 2021

I still don't think it is. No install should ever fail due to audit warnings, regardless of what the audit level is set to. Only npm audit should fail because of that.

If that was intended to change in npm 7, then that is a pretty major functionality change that was never discussed in an RFC call nor referenced (that I can recall) in a changelog.

Please reopen.

@wraithgar wraithgar added the Needs Discussion is pending a discussion label Mar 23, 2021
@wraithgar wraithgar reopened this Mar 23, 2021
@ljharb
Copy link
Collaborator Author

ljharb commented May 6, 2021

@wraithgar the linked PR appears both merged and released, but I'm still seeing this issue in npm v7.11.2: https://github.com/inspect-js/is-generator-function/runs/2515355693

ljharb added a commit to inspect-js/is-generator-function that referenced this issue May 6, 2021
@wraithgar
Copy link
Member

Can you open a new issue for this @ljharb that clearly outlines the situation?

@ljharb
Copy link
Collaborator Author

ljharb commented May 6, 2021

@wraithgar i can, but the contents would be identical to this - basically, install should never fail due to audit, and currently it does if an explicit audit level is set in config.

@mshima
Copy link

mshima commented May 19, 2021

This errors seems to only happen if there is a lock file.

@ljharb
Copy link
Collaborator Author

ljharb commented May 19, 2021

@mshima i see it explicitly without a lockfile, since i never use lockfiles in any of my published projects.

@mshima
Copy link

mshima commented May 19, 2021

Interesting, at our use case it installs correctly and fails at the second call.

ljharb added a commit to ljharb/json-file-plus that referenced this issue May 22, 2021
ljharb added a commit to ljharb/object.assign that referenced this issue May 24, 2021
@isaacs
Copy link
Contributor

isaacs commented May 26, 2021

Fixed by #3311

@isaacs isaacs closed this as completed May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Discussion is pending a discussion Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

4 participants