From d3c9c5f88e9956e18ad6bcab6bdd35d87cb7c5e2 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 17 Aug 2022 11:33:53 -0400 Subject: [PATCH 1/8] Add an explanation when the vulnerablility auditor fix is unavailable instead of raising --- .../update_checker/vulnerability_auditor.rb | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb index 2c722842497..d3e0a677373 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb @@ -41,6 +41,7 @@ def initialize(dependency_files:, credentials:, allow_removal: false) # dependency on the blocking dependency # * :top_level_ancestors [Array] the names of all top-level dependencies with a transitive # dependency on the dependency + # * :explanation [String] an explanation for why the project failed the vulnerability auditor run def audit(dependency:, security_advisories:) fix_unavailable = { "dependency_name" => dependency.name, @@ -60,7 +61,7 @@ def audit(dependency:, security_advisories:) # `npm-shrinkwrap.js`, if present, takes precedence over `package-lock.js`. # Both files use the same format. See https://bit.ly/3lDIAJV for more. lockfile = (dependency_files_builder.shrinkwraps + dependency_files_builder.package_locks).first - return fix_unavailable unless lockfile + return explain_fix_unavailable(:no_lockfile, fix_unavailable) unless lockfile vuln_versions = security_advisories.map do |a| { @@ -74,7 +75,11 @@ def audit(dependency:, security_advisories:) function: "npm:vulnerabilityAuditor", args: [Dir.pwd, vuln_versions] ) - return fix_unavailable unless viable_audit_result?(audit_result, security_advisories) + + validation_result = validate_audit_result(audit_result, security_advisories) + unless viable_audit_result?(validation_result) + return explain_fix_unavailable(validation_result, fix_unavailable) + end audit_result end @@ -87,8 +92,22 @@ def audit(dependency:, security_advisories:) attr_reader :dependency_files, :credentials - def viable_audit_result?(audit_result, security_advisories) - validation_result = validate_audit_result(audit_result, security_advisories) + def explain_fix_unavailable(validation_result, fix_unavailable) + fix_unavailable["explanation"] = + case validation_result + when :no_lockfile + "No lockfile detected" + when :fix_unavailable, :dependency_still_vulnerable, :downgrades_dependencies + "No patched version available for #{fix_unavailable['dependency_name']}" + when :vulnerable_dependency_removed + "#{fix_unavailable['dependency_name']} was removed in the update. Dependabot is unable to " \ + "open a pull request. Please upgrade manually" + end + + fix_unavailable + end + + def viable_audit_result?(validation_result) return true if validation_result == :viable Dependabot.logger.info("VulnerabilityAuditor: audit result not viable: #{validation_result}") From b49a2da62f7ede924032ab01b7b4c9a6472939ed Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Tue, 23 Aug 2022 16:11:57 -0400 Subject: [PATCH 2/8] Add tests for vulnerability auditor explanation --- .../vulnerability_auditor_spec.rb | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb index 83de366e4c0..5b21a810256 100644 --- a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb +++ b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb @@ -110,7 +110,11 @@ expect(Dependabot.logger).to receive(:info).with(/audit result not viable: vulnerable_dependency_removed/i) expect(subject.audit(dependency: dependency, security_advisories: security_advisories)). - to include("fix_available" => false) + to include( + "fix_available" => false, + "explanation" => "#{dependency.name} was removed in the update. "\ + "Dependabot is unable to open a pull request. Please upgrade manually" + ) end end end @@ -137,7 +141,10 @@ expect(Dependabot.logger).to receive(:info).with(/audit result not viable: dependency_still_vulnerable/i) expect(subject.audit(dependency: dependency, security_advisories: security_advisories)). - to include("fix_available" => false) + to include( + "fix_available" => false, + "explanation" => "No patched version available for #{dependency.name}" + ) end end @@ -172,7 +179,10 @@ expect(Dependabot.logger).to receive(:info).with(/audit result not viable: downgrades_dependencies/i) expect(subject.audit(dependency: dependency, security_advisories: security_advisories)). - to include("fix_available" => false) + to include( + "fix_available" => false, + "explanation" => "No patched version available for #{dependency.name}" + ) end end @@ -181,7 +191,10 @@ it "returns fix_available => false" do expect(subject.audit(dependency: dependency, security_advisories: [])). - to include("fix_available" => false) + to include( + "fix_available" => false, + "explanation" => "No lockfile detected" + ) end end From f7db7c820efcbac751fdea22b1226ce5f53b82ab Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 25 Aug 2022 15:08:25 -0400 Subject: [PATCH 3/8] Undo lockfile error message changes --- .../npm_and_yarn/update_checker/vulnerability_auditor.rb | 4 +--- .../update_checker/vulnerability_auditor_spec.rb | 5 +---- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb index d3e0a677373..ce02c4758cc 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb @@ -61,7 +61,7 @@ def audit(dependency:, security_advisories:) # `npm-shrinkwrap.js`, if present, takes precedence over `package-lock.js`. # Both files use the same format. See https://bit.ly/3lDIAJV for more. lockfile = (dependency_files_builder.shrinkwraps + dependency_files_builder.package_locks).first - return explain_fix_unavailable(:no_lockfile, fix_unavailable) unless lockfile + return fix_unavailable unless lockfile vuln_versions = security_advisories.map do |a| { @@ -95,8 +95,6 @@ def audit(dependency:, security_advisories:) def explain_fix_unavailable(validation_result, fix_unavailable) fix_unavailable["explanation"] = case validation_result - when :no_lockfile - "No lockfile detected" when :fix_unavailable, :dependency_still_vulnerable, :downgrades_dependencies "No patched version available for #{fix_unavailable['dependency_name']}" when :vulnerable_dependency_removed diff --git a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb index 5b21a810256..1d69587719a 100644 --- a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb +++ b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb @@ -191,10 +191,7 @@ it "returns fix_available => false" do expect(subject.audit(dependency: dependency, security_advisories: [])). - to include( - "fix_available" => false, - "explanation" => "No lockfile detected" - ) + to include("fix_available" => false) end end From 1a8f15450072e34533a8945d9c32a28dbd5d626d Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Thu, 25 Aug 2022 15:41:54 -0400 Subject: [PATCH 4/8] conflicting_dependencies also returns vulnerablilty_audit --- .../lib/dependabot/npm_and_yarn/update_checker.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 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 13ed0a3e3f4..624278dbdae 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 @@ -96,13 +96,20 @@ def requirements_update_strategy end def conflicting_dependencies - ConflictingDependencyResolver.new( + conflicts = ConflictingDependencyResolver.new( dependency_files: dependency_files, credentials: credentials ).conflicting_dependencies( dependency: dependency, target_version: lowest_security_fix_version ) + + vulnerable = vulnerability_audit.select do |v| + v["fix_available"] == false && + v["explanation"] + end + + conflicts + vulnerable end private From bc7c46fb50c3f34c59e2dce67e265688871c562f Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Mon, 29 Aug 2022 15:17:55 -0400 Subject: [PATCH 5/8] Improve removed dependency error messaging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David Rodríguez --- .../npm_and_yarn/update_checker/vulnerability_auditor.rb | 4 ++-- .../npm_and_yarn/update_checker/vulnerability_auditor_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb index ce02c4758cc..b23037835aa 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb @@ -98,8 +98,8 @@ def explain_fix_unavailable(validation_result, fix_unavailable) when :fix_unavailable, :dependency_still_vulnerable, :downgrades_dependencies "No patched version available for #{fix_unavailable['dependency_name']}" when :vulnerable_dependency_removed - "#{fix_unavailable['dependency_name']} was removed in the update. Dependabot is unable to " \ - "open a pull request. Please upgrade manually" + "#{fix_unavailable['dependency_name']} was removed in the update. Dependabot is not able to " \ + "deal with this yet, but you can still upgrade manually." end fix_unavailable diff --git a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb index 1d69587719a..733565d249d 100644 --- a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb +++ b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb @@ -113,7 +113,7 @@ to include( "fix_available" => false, "explanation" => "#{dependency.name} was removed in the update. "\ - "Dependabot is unable to open a pull request. Please upgrade manually" + "Dependabot is not able to deal with this yet, but you can still upgrade manually." ) end end From 07238efabb91adb036f23c9d1cacc7143d390886 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 31 Aug 2022 14:17:21 -0400 Subject: [PATCH 6/8] Check fix_available key instead of its value --- npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 624278dbdae..b2f3907879a 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 @@ -105,8 +105,7 @@ def conflicting_dependencies ) vulnerable = vulnerability_audit.select do |v| - v["fix_available"] == false && - v["explanation"] + !v["fix_available"] && v["explanation"] end conflicts + vulnerable From 620491327d8a6e9bf9624ffec771108d53e16ee3 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 31 Aug 2022 14:17:54 -0400 Subject: [PATCH 7/8] explain_fix_unavailable returns explanation string instead of fix_unavailable object --- .../update_checker/vulnerability_auditor.rb | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb index b23037835aa..059a472df5c 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb @@ -78,7 +78,8 @@ def audit(dependency:, security_advisories:) validation_result = validate_audit_result(audit_result, security_advisories) unless viable_audit_result?(validation_result) - return explain_fix_unavailable(validation_result, fix_unavailable) + fix_unavailable["explanation"] = explain_fix_unavailable(validation_result, dependency) + return fix_unavailable end audit_result @@ -92,17 +93,14 @@ def audit(dependency:, security_advisories:) attr_reader :dependency_files, :credentials - def explain_fix_unavailable(validation_result, fix_unavailable) - fix_unavailable["explanation"] = - case validation_result - when :fix_unavailable, :dependency_still_vulnerable, :downgrades_dependencies - "No patched version available for #{fix_unavailable['dependency_name']}" - when :vulnerable_dependency_removed - "#{fix_unavailable['dependency_name']} was removed in the update. Dependabot is not able to " \ - "deal with this yet, but you can still upgrade manually." - end - - fix_unavailable + def explain_fix_unavailable(validation_result, dependency) + case validation_result + when :fix_unavailable, :dependency_still_vulnerable, :downgrades_dependencies + "No patched version available for #{dependency.name}" + when :vulnerable_dependency_removed + "#{dependency.name} was removed in the update. Dependabot is not able to " \ + "deal with this yet, but you can still upgrade manually." + end end def viable_audit_result?(validation_result) From 17ce181f3d7029e1b6252cf5b3f0c746d0515a62 Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Wed, 31 Aug 2022 17:05:15 -0400 Subject: [PATCH 8/8] Rubocop method length --- .../npm_and_yarn/update_checker/vulnerability_auditor.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb index 059a472df5c..1fb5f6b8702 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb +++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb @@ -21,6 +21,7 @@ def initialize(dependency_files:, credentials:, allow_removal: false) @allow_removal = allow_removal end + # rubocop:disable Metrics/MethodLength # Finds any dependencies in the `package-lock.json` or `npm-shrinkwrap.json` that have # a subdependency on the given dependency that is locked to a vuln version range. # @@ -88,6 +89,7 @@ def audit(dependency:, security_advisories:) log_helper_subprocess_failure(dependency, e) fix_unavailable end + # rubocop:enable Metrics/MethodLength private