Skip to content

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

Merged
Nishnha merged 20 commits intomainfrom
nishnha/npm-fix-type-error
Sep 7, 2022
Merged

Add more helpful error messaging when a vulnerable dependency cannot be upgraded#5645
Nishnha merged 20 commits intomainfrom
nishnha/npm-fix-type-error

Conversation

@Nishnha
Copy link
Copy Markdown
Member

@Nishnha Nishnha commented Sep 7, 2022

This PR fixes the TypeError that was present in #5542 and re-introduces the commits from the original PR since they were reverted.

@Nishnha Nishnha requested a review from a team as a code owner September 7, 2022 01:38
expect(conflicting_dependencies_result.last).
to eq(
"dependency_name" => "@dependabot-fixtures/npm-transitive-dependency",
"explanation" => "No patched version available for @dependabot-fixtures/npm-transitive-dependency",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What would the explanation be if it's one of the parents that doesn't have an update to a non locking version? I think that'd be the primary use case of this error as we might not get here if the vulnerable dependency doesn't have a fix available.

Copy link
Copy Markdown
Member Author

@Nishnha Nishnha Sep 7, 2022

Choose a reason for hiding this comment

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

I think that's captured by this test in the vulnerability auditor https://github.com/dependabot/dependabot-core/pull/5645/files#diff-f0c4f0847c642ca1ba8627d73ce6dfc76f107816ac9916ef12d3ae5c4edce665R122-R149

I used a vulnerable range without a fixed version to make sure the vulnerability auditor would error, but I can update the test to #5645 (comment)

Then the resulting error from the ConflictingDependencyResolver and VulnerabilityAuditor is

(ruby) conflicting_dependencies_result
[{"explanation"=>
   "@dependabot-fixtures/npm-parent-dependency@2.0.2 requires @dependabot-fixtures/npm-transitive-dependency@^1.0.0 via @dependabot-fixtures/npm-intermediate-dependency@0.0.2",
  "name"=>"@dependabot-fixtures/npm-intermediate-dependency",
  "version"=>"0.0.2",
  "requirement"=>"^1.0.0"},
 {"dependency_name"=>"@dependabot-fixtures/npm-transitive-dependency",
  "fix_available"=>false,
  "fix_updates"=>[],
  "top_level_ancestors"=>[],
  "explanation"=>"No patched version available for @dependabot-fixtures/npm-transitive-dependency"}]

The error message from the vulnerability auditor is the same though. Would you expect it to list out the locking parent too?

Copy link
Copy Markdown
Member Author

@Nishnha Nishnha Sep 7, 2022

Choose a reason for hiding this comment

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

I removed the test for "with an unsatisfiable vulnerability" in bf62aaf because I don't think we can reach that code without also having conflicting dependencies.

I updated the test with both "conflicting dependencies and an unsatisfiable vulnerability" to use an invalid version range for the vulnerability in a8d6b3a.

Our @dependabot-fixtures setup is not complex enough to test this scenario using valid vulnerability ranges, but it does seem like a case that can occur in a project, so I want to keep test coverage around it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I opened #5672 to create and test a scenario where a valid vulnerable version range is blocked by a locking parent dependency

@Nishnha Nishnha force-pushed the nishnha/npm-fix-type-error branch from 4500c2c to a8d6b3a Compare September 7, 2022 21:09
@Nishnha Nishnha enabled auto-merge September 7, 2022 21:49
@Nishnha Nishnha merged commit 0bfb007 into main Sep 7, 2022
@Nishnha Nishnha deleted the nishnha/npm-fix-type-error branch September 7, 2022 21:58
@pavera pavera mentioned this pull request Oct 31, 2022
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.

3 participants