From 6d681a0be45677276a14ee0515c1d5af3608adee Mon Sep 17 00:00:00 2001 From: David McIntosh <804610+mctofu@users.noreply.github.com> Date: Wed, 5 Oct 2022 14:12:47 -0700 Subject: [PATCH 1/4] Move all_versions retrieval to Dependency Allows reuse outside of the UpdateChecker --- common/lib/dependabot/dependency.rb | 9 ++++ common/spec/dependabot/dependency_spec.rb | 50 +++++++++++++++++++ .../dependabot/npm_and_yarn/update_checker.rb | 6 +-- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/common/lib/dependabot/dependency.rb b/common/lib/dependabot/dependency.rb index 29470d72d3..fc8dcd4848 100644 --- a/common/lib/dependabot/dependency.rb +++ b/common/lib/dependabot/dependency.rb @@ -106,6 +106,15 @@ def display_name display_name_builder.call(name) end + # Returns all detected versions of the dependency. Only ecosystems that + # support this feature will return more than the current version. + def all_versions + all_versions = metadata[:all_versions] + return [version].compact unless all_versions + + all_versions.filter_map(&:version) + end + def ==(other) other.instance_of?(self.class) && to_h == other.to_h end diff --git a/common/spec/dependabot/dependency_spec.rb b/common/spec/dependabot/dependency_spec.rb index 1769c5684e..eabe01b8c8 100644 --- a/common/spec/dependabot/dependency_spec.rb +++ b/common/spec/dependabot/dependency_spec.rb @@ -304,4 +304,54 @@ expect(dependency.to_h.keys).not_to include("metadata") end 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 diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb index 2d4b39d398..8581fe30e2 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb @@ -144,10 +144,8 @@ def vulnerability_audit def vulnerable_versions @vulnerable_versions ||= begin - all_versions = Set.new([ - dependency.version, - *dependency.metadata.fetch(:all_versions, []).filter_map(&:version) - ]).filter_map { |v| version_class.new(v) if version_class.correct?(v) } + all_versions = dependency.all_versions. + filter_map { |v| version_class.new(v) if version_class.correct?(v) } all_versions.select do |v| security_advisories.any? { |advisory| advisory.vulnerable?(v) } From 969dbb8061dc4c0c74e3f2d517d3a3f4b12ec007 Mon Sep 17 00:00:00 2001 From: David McIntosh <804610+mctofu@users.noreply.github.com> Date: Wed, 5 Oct 2022 14:20:41 -0700 Subject: [PATCH 2/4] Job#vulnerable? should consider all versions of the dependency --- updater/lib/dependabot/job.rb | 5 +-- updater/spec/dependabot/job_spec.rb | 47 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index 29c071836d..04b186dd9c 100644 --- a/updater/lib/dependabot/job.rb +++ b/updater/lib/dependabot/job.rb @@ -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) } } end def security_fix?(dependency) diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index 1483037dc5..eb8d1d3a6b 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/updater/spec/dependabot/job_spec.rb @@ -158,6 +158,53 @@ it { is_expected.to eq(true) } end + + context "for a security fix that doesn't apply" do + let(:security_advisories) do + [ + { + "dependency-name" => "business", + "affected-versions" => ["> 1.8.0"], + "patched-versions" => [], + "unaffected-versions" => [] + } + ] + end + + it { is_expected.to eq(false) } + end + + context "for a security fix that doesn't apply to some versions" do + let(:security_advisories) do + [ + { + "dependency-name" => "business", + "affected-versions" => ["> 1.8.0"], + "patched-versions" => [], + "unaffected-versions" => [] + } + ] + end + + it "should be allowed" do + dependency.metadata[:all_versions] = [ + Dependabot::Dependency.new( + name: dependency_name, + package_manager: "bundler", + version: "1.8.0", + requirements: [] + ), + Dependabot::Dependency.new( + name: dependency_name, + package_manager: "bundler", + version: "1.9.0", + requirements: [] + ) + ] + + is_expected.to eq(true) + end + end end context "and a dependency whitelist that includes the dependency" do From e1d3cc42f69546351c1d6828bae8b4f2d76cf81c Mon Sep 17 00:00:00 2001 From: David McIntosh <804610+mctofu@users.noreply.github.com> Date: Wed, 5 Oct 2022 23:15:00 -0700 Subject: [PATCH 3/4] document that dependency metadata doesn't affect equality --- common/spec/dependabot/dependency_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/common/spec/dependabot/dependency_spec.rb b/common/spec/dependabot/dependency_spec.rb index eabe01b8c8..45a1778624 100644 --- a/common/spec/dependabot/dependency_spec.rb +++ b/common/spec/dependabot/dependency_spec.rb @@ -303,6 +303,22 @@ ) 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 end describe "#all_versions" do From 2718fda280f88a168a1a77b09a0a925dfaa8a725 Mon Sep 17 00:00:00 2001 From: David McIntosh <804610+mctofu@users.noreply.github.com> Date: Thu, 6 Oct 2022 11:35:23 -0700 Subject: [PATCH 4/4] Hold off on adjusting conflict explanation for a future PR --- npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb index 8581fe30e2..6ee6910346 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb @@ -118,7 +118,6 @@ def conflicting_dependencies dependency: dependency, target_version: lowest_security_fix_version ) - return conflicts unless defined?(@vulnerability_audit) vulnerable = [vulnerability_audit].select do |hash| !hash["fix_available"] && hash["explanation"]