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.

Sign up for GitHub

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

Jump to bottom

[BUG] npm audit ambiguously states "found 0 vulnerabilities" #1951

Closed
simonua opened this issue Oct 13, 2020 · 4 comments
Closed

[BUG] npm audit ambiguously states "found 0 vulnerabilities" #1951

simonua opened this issue Oct 13, 2020 · 4 comments
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release

Comments

@simonua
Copy link
Contributor

simonua commented Oct 13, 2020

Current Behavior:

We use Azure Artifacts which is a part of Azure DevOps to host our npm packages. We configured an upstream to the public npm registry to obtain public packages.
When executing npm audit against the public npm registry, we see a proper list of vulnerabilities for a large private package we maintain but when executing the same command against Azure Artifacts, we are ambiguously told "found 0 vulnerabilities".

This isn't new behavior, but it's still evident in npm 7. We have known that Azure Artifacts does not support the endpoint that is called by the audit. I don't recall the response it issues, but I suspect a 4xx of sorts.

This is problematic because it gives a false sense of security to developers, devops engineers, etc.

Expected Behavior:

What would be more appropriate to display would be a message that the audit-related endpoint is not implemented, errored, etc. and that the npm cli was thus unable to determine the status of vulnerabilities.

Steps To Reproduce:

  1. Create a package with dependencies that have known vulnerabilities.
  2. Point the npm cli to Azure Artifacts (set up a free Azure account, if necessary).
  3. Execute npm audit to see the package appears to have zero vulnerabilities.

Environment:

The environment is fairly irrelevant here, but I run NodeJS 14.13.1 and npm 7.0.0.
@simonua simonua 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 Oct 13, 2020
@simonua
Copy link
Contributor Author

simonua commented Oct 13, 2020

@evilpacket, maybe something for your interest, good sir?

@nlf nlf removed the Needs Triage needs review for next steps label Oct 13, 2020
@isaacs
Copy link
Contributor

isaacs commented Oct 13, 2020

I agree, the design intention is to not print audit information at all if the endpoint didn't return anything, so "0 vulnerabilities" is a bug. But I think a message indicating that we tried to audit and failed would be more informative and helpful.

@simonua
Copy link
Contributor Author

simonua commented Oct 13, 2020

Thank you! I'm very happy to debug, if needed, and give you a proper status code, etc. if you give me instructions on how to enable that.

isaacs added a commit that referenced this issue Oct 13, 2020
@isaacs
Handle errors from audit endpoint appropriately
03200bf 
If we're running the 'audit' command, then a failed endpoint means that
the command failed.  Error out in that case.

Otherwise, if it's a quick audit as part of another command, just return
a value to indicate that we should not print audit info.

This avoids showing '0 vulnerabilities found', which, while amusingly
technically correct, is misleading and not very helpful.

Fix: #1951
@isaacs isaacs mentioned this issue Oct 13, 2020
Closed
isaacs added a commit that referenced this issue Oct 13, 2020
@isaacs
Handle errors from audit endpoint appropriately
0dc45c5 
If we're running the 'audit' command, then a failed endpoint means that
the command failed.  Error out in that case.

Otherwise, if it's a quick audit as part of another command, just return
a value to indicate that we should not print audit info.

This avoids showing '0 vulnerabilities found', which, while amusingly
technically correct, is misleading and not very helpful.

Fix: #1951
darcyclarke pushed a commit that referenced this issue Oct 15, 2020
@isaacs @darcyclarke
Handle errors from audit endpoint appropriately
b29cbd8 
If we're running the 'audit' command, then a failed endpoint means that
the command failed.  Error out in that case.

Otherwise, if it's a quick audit as part of another command, just return
a value to indicate that we should not print audit info.

This avoids showing '0 vulnerabilities found', which, while amusingly
technically correct, is misleading and not very helpful.

Fix: #1951

Credit: @isaacs
Close: #1956
Reviewed-by: @darcyclarke
@isaacs isaacs mentioned this issue Oct 15, 2020
Merged
@isaacs isaacs closed this as completed in 2ccb636 Oct 15, 2020
@simonua
Copy link
Contributor Author

simonua commented Oct 16, 2020

Follow-up on what this looks like after installing 7.0.1. I no longer see an ambiguous message upon install, and npm audit correctly tells me about the error that was encountered. Thank you, @isaacs!

image

@MylesBorins MylesBorins mentioned this issue Oct 16, 2020
Closed
@milon27 milon27 mentioned this issue Nov 23, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle errors from audit endpoint appropriately npm/cli
3 participants
@isaacs @nlf @simonua