Skip to content

Add more helpful error messaging when a vulnerable dependency cannot be upgraded#5542

Merged
Nishnha merged 8 commits intomainfrom
nishnha/npm-helpful-errors
Sep 1, 2022
Merged

Add more helpful error messaging when a vulnerable dependency cannot be upgraded#5542
Nishnha merged 8 commits intomainfrom
nishnha/npm-helpful-errors

Conversation

@Nishnha
Copy link
Copy Markdown
Member

@Nishnha Nishnha commented Aug 16, 2022

This PR adds a new method, #explain_fix_unavailable, which adds an explanation key to the fix_unavailable object when the UpdateChecker returns an update_not_possible error.

This explanation key is also used in the ConflictingDependencies check which is printed out after a failed job run and also shows up in the Dependabot security alert view.

The UpdateChecker should always raise with an :update_not_possible by this point since the call from #latest_version_resolvable_with_full_unlock? will not have any viable audit results and thus no fix available

@Nishnha Nishnha requested a review from a team as a code owner August 16, 2022 00:50
@Nishnha Nishnha marked this pull request as draft August 17, 2022 15:35
@Nishnha Nishnha force-pushed the nishnha/npm-helpful-errors branch 3 times, most recently from c3df4e5 to 425c391 Compare August 23, 2022 20:19
@Nishnha Nishnha marked this pull request as ready for review August 23, 2022 20:19
@Nishnha
Copy link
Copy Markdown
Member Author

Nishnha commented Aug 23, 2022

🙃
I'm getting an error from the "behaves like an update checker" test:
https://github.com/dependabot/dependabot-core/runs/7982211556?check_suite_focus=true#step:5:267

I'm pretty sure the UpdateChecker#vulnerability_audit method this PR introduces will need to be public to be accessed from the Updater (we include Dependabot Core as a gem there, so we should only access the public interface)

For #conflicting_dependencies, we added as a method to the UpdateChecker base class even though it's only used in the bundler and npm_and_yarn ecosystems
https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/update_checkers/base.rb#L97-L105

I think the best course of action for the #vulnerability_audit method is to also add it to the UpdateChecker base class and note that it's used for npm transitive dependency updates.

Does anyone have other suggestions?

@mctofu
Copy link
Copy Markdown
Contributor

mctofu commented Aug 24, 2022

🙃 I'm getting an error from the "behaves like an update checker" test: https://github.com/dependabot/dependabot-core/runs/7982211556?check_suite_focus=true#step:5:267

I'm pretty sure the UpdateChecker#vulnerability_audit method this PR introduces will need to be public to be accessed from the Updater (we include Dependabot Core as a gem there, so we should only access the public interface)

For #conflicting_dependencies, we added as a method to the UpdateChecker base class even though it's only used in the bundler and npm_and_yarn ecosystems https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/update_checkers/base.rb#L97-L105

I think the best course of action for the #vulnerability_audit method is to also add it to the UpdateChecker base class and note that it's used for npm transitive dependency updates.

Does anyone have other suggestions?

Can you represent the error message in the #conflicting_dependencies result? #conflicting_dependencies is fairly generic and could be implemented by more ecosystems if we had the time to do so. #vulnerability_audit seems much more specific to our npm implementation so it doesn't seem as likely we'd expose that for any other ecosystem.

@Nishnha Nishnha force-pushed the nishnha/npm-helpful-errors branch 4 times, most recently from b51fc84 to 13258dd Compare August 26, 2022 18:56
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I'm not really well versed into how npm updates work, but this looks good to me!

@Nishnha Nishnha force-pushed the nishnha/npm-helpful-errors branch from 23bdd59 to 6204913 Compare August 31, 2022 20:49
@Nishnha Nishnha merged commit 039ce94 into main Sep 1, 2022
@Nishnha Nishnha deleted the nishnha/npm-helpful-errors branch September 1, 2022 15:19
@Nishnha
Copy link
Copy Markdown
Member Author

Nishnha commented Sep 1, 2022

Reverting this in #5613 because of a TypeError. Will fix, add tests, and re-deploy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants