-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Consider all dependency versions in Job.vulnerable? #5837
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.
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
Changes from all commits
6d681a0
969dbb8
e1d3cc4
2718fda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -303,5 +303,71 @@ | |
| ) | ||
| expect(dependency.to_h.keys).not_to include("metadata") | ||
| end | ||
|
|
||
| it "isn't utilized by the equality operator" do | ||
| dependency1 = described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| metadata: { foo: 42 } | ||
| ) | ||
| dependency2 = described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| metadata: { foo: 43 } | ||
| ) | ||
| expect(dependency1).to eq(dependency2) | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thinking |
||
| end | ||
|
|
||
| describe "#all_versions" do | ||
| it "returns an empty array by default" do | ||
| dependency = described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy" | ||
| ) | ||
|
|
||
| expect(dependency.all_versions).to eq([]) | ||
| end | ||
|
|
||
| it "returns the dependency version if all_version metadata isn't present" do | ||
| dependency = described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| version: "1.0.0" | ||
| ) | ||
|
|
||
| expect(dependency.all_versions).to eq(["1.0.0"]) | ||
| end | ||
|
|
||
| it "returns all_version metadata if present" do | ||
| dependency = described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| version: "1.0.0", | ||
| metadata: { | ||
| all_versions: [ | ||
| described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| version: "1.0.0" | ||
| ), | ||
| described_class.new( | ||
| name: "dep", | ||
| requirements: [], | ||
| package_manager: "dummy", | ||
| version: "2.0.0" | ||
| ) | ||
| ] | ||
| } | ||
| ) | ||
|
|
||
| expect(dependency.all_versions).to eq(["1.0.0", "2.0.0"]) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,8 +112,9 @@ def vulnerable?(dependency) | |
| version_class_for_package_manager(dependency.package_manager) | ||
| return false unless version_class.correct?(dependency.version) | ||
|
|
||
| version = version_class.new(dependency.version) | ||
| 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) } } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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) }
end
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| end | ||
|
|
||
| def security_fix?(dependency) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌