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

Prevent blowing up on audit malformed response #42

Merged
merged 4 commits into from
Aug 20, 2018
Merged

Prevent blowing up on audit malformed response #42

merged 4 commits into from
Aug 20, 2018

Conversation

framp
Copy link
Contributor

@framp framp commented Aug 8, 2018 •
edited
Loading

npm is failing our builds with:

npm ERR! Cannot read property 'totalDependencies' of undefined

which it's coming out of auditing

There must be something wrong in whatever code is generating the response - this only prevents blowing up a valid installation because of this.

@framp framp requested a review from a team as a code owner August 8, 2018 16:23
ljharb
ljharb approved these changes Aug 8, 2018
View reviewed changes
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More defensive programming in general would help use with things like artifactory, that may not meet the CLI’s implicit assumptions about data format.

@zkat zkat added semver:patch semver patch level for changes security labels Aug 9, 2018
zkat
zkat suggested changes Aug 13, 2018
View reviewed changes
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine, but I'd like to have a warning if metadata is missing like this. There's definitely a bug in our services somewhere if that happens (or in npm-audit-report, so I'd like to make sure we don't just hide the problem.

lib/install.js
@@ -834,7 +834,7 @@ Installer.prototype.printInstalledForHuman = function (diffs, auditResult) {
if (removed) actions.push('removed ' + packages(removed))
if (updated) actions.push('updated ' + packages(updated))
if (moved) actions.push('moved ' + packages(moved))
if (auditResult && auditResult.metadata.totalDependencies) {
if (auditResult && auditResult.metadata && auditResult.metadata.totalDependencies) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is missing, there's definitely an issue with the endpoint. I'd rather spit out a warning here if this happens, so we can make sure to track it down. In the future, it might also be good to have npm-audit-report verify the data we get from the server and warn accordingly, and we can remove the warning from lib/install.js at that point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could also be a bug in a non-npm registry - like artifactory - if they implemented audit support but did it incorrectly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the intention is also to cover non-npm registries, but I didn't think this was necessarily an artifactory issue. If so, then yeah, they should fix their stuff.

@framp
Copy link
Contributor Author

framp commented Aug 14, 2018

I added a warn using npmlog - hope I didn't miss some internal conventions

zkat
zkat approved these changes Aug 14, 2018
View reviewed changes
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

I poked at it a little, but this works :)

@zkat zkat changed the base branch from latest to release-next August 20, 2018 22:28
zkat
zkat approved these changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks awesome. Thank you!

@zkat zkat merged commit 8d1b322 into npm:release-next Aug 20, 2018
zkat pushed a commit that referenced this pull request Aug 20, 2018
@framp @zkat
install: Prevent blowing up on audit malformed response (#42)
4bd40f5 
PR-URL: #42
Credit: @framp
Reviewed-By: @zkat
@mattrussell-sonocent mattrussell-sonocent mentioned this pull request May 7, 2019
Closed
@varmakarthik12 varmakarthik12 mentioned this pull request Nov 30, 2023
Open
antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
@github-actions
chore(main): release 3.0.1 (npm#42)
492a1be 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@zkat zkat zkat approved these changes

Assignees
No one assigned
Labels
semver:patch semver patch level for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@framp @zkat @ljharb