Consider all dependency versions in Job.vulnerable? #5837
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows reuse outside of the UpdateChecker
mctofu
force-pushed
the
mctofu/2855-vuln-multi-version
branch
from
fcb97a8 to
e1d3cc4
Compare
bdragon
approved these changes
Oct 6, 2022
| return [version].compact unless all_versions | ||
|
|
||
| all_versions.filter_map(&:version) | ||
| end |
| security_advisories.any? { |a| a.vulnerable?(version) } | ||
| all_versions = dependency.all_versions. | ||
| filter_map { |v| version_class.new(v) if version_class.correct?(v) } | ||
| security_advisories.any? { |a| all_versions.any? { |v| a.vulnerable?(v) } } |
There was a problem hiding this comment.
I've noticed this code
version_class = Utils.version_class_for_package_manager(package_manager)
version_class.new(version) if version_class.correct?(version)in a lot of places. Looking at it once again here, I'm wondering if it wouldn't make sense to put this in Dependency. It would be a reasonable argument to say it's too much for that class to know about, but it would remove some boilerplate. Maybe something like this (but probably with better naming)
def correct_version
version_class = Utils.version_class_for_package_manager(package_manager)
version_class.new(version) if version_class.correct?(version)
end
def all_correct_versions
version_class = Utils.version_class_for_package_manager(package_manager)
all_versions.filter_map { |v| version_class.new(v) if version_class.correct?(v) }
endThere was a problem hiding this comment.
Yeah, that's a good thought! There's still some future changes needed to get a successful PR through. If that ends up duplicating this again I think that'd be good opportunity to dry it up?
| metadata: { foo: 43 } | ||
| ) | ||
| expect(dependency1).to eq(dependency2) | ||
| end |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In testing #5801 via the updater we found there's an additional spot we need to consider all versions of a dependency when doing a vulnerability check. This is to prevent Dependabot from returning a confusing error message that all versions have been ignored.
This code path is invoked from the updater at
dependabot-core/updater/lib/dependabot/updater.rb
Line 231 in 97ae98c
Part of the allowed update check is to see if the dependency is actually still vulnerable:
dependabot-core/updater/lib/dependabot/job.rb
Lines 77 to 78 in 97ae98c
I considered removing this check as it's a duplicate of this check in the create pr flow:
dependabot-core/updater/lib/dependabot/updater.rb
Lines 221 to 222 in 97ae98c
However, it's also checked in the update pr flow so it would have taken more changes to try and sort that out so I'm leaving that for a future cleanup:
dependabot-core/updater/lib/dependabot/updater.rb
Lines 140 to 141 in 97ae98c