Support transitive dependency removal

bin/dry-run.rb

@@ -731,7 +731,10 @@ def security_fix?(dependency)
     next
   end
 
-  updater = file_updater_for(updated_deps)
+  # Removal is only supported for transitive dependencies which are removed as a
+  # side effect of the parent update
+  deps_to_update = updated_deps.reject{ |d| d.removed? }
+  updater = file_updater_for(deps_to_update)
   updated_files = updater.updated_dependency_files
 
   # Currently unused but used to create pull requests (from the updater)

common/lib/dependabot/dependency.rb

@@ -61,6 +61,10 @@ def top_level?
       requirements.any?
     end
 
+    def removed?
+      version == ""
+    end
+
     def to_h
       {
         "name" => name,
@@ -114,9 +118,7 @@ def eql?(other)
     private
 
     def check_values
-      if [version, previous_version].any? { |v| v == "" }
-        raise ArgumentError, "blank strings must not be provided as versions"
-      end
+      raise ArgumentError, "previous version must not be a blank string" if previous_version == ""
 
       check_requirement_fields
       check_subdependency_metadata

common/lib/dependabot/pull_request_creator/message_builder.rb

@@ -293,10 +293,14 @@ def metadata_links
         return metadata_links_for_dep(dependencies.first) if dependencies.count == 1
 
         dependencies.map do |dep|
-          "\n\nUpdates `#{dep.display_name}` "\
-          "#{from_version_msg(previous_version(dep))}to "\
-          "#{new_version(dep)}"\
-          "#{metadata_links_for_dep(dep)}"
+          if dep.removed?
+            "\n\nRemoves `#{dep.display_name}`"
+          else
+            "\n\nUpdates `#{dep.display_name}` "\
+            "#{from_version_msg(previous_version(dep))}to "\
+            "#{new_version(dep)}"\
+            "#{metadata_links_for_dep(dep)}"
+          end
         end.join
       end
 
@@ -313,9 +317,13 @@ def metadata_cascades
         return metadata_cascades_for_dep(dependencies.first) if dependencies.one?
 
         dependencies.map do |dep|
-          msg = "\nUpdates `#{dep.display_name}` "\
-                "#{from_version_msg(previous_version(dep))}"\
-                "to #{new_version(dep)}"
+          msg = if dep.removed?
+                  "\nRemoves `#{dep.display_name}`"
+                else
+                  "\nUpdates `#{dep.display_name}` "\
+                        "#{from_version_msg(previous_version(dep))}"\
+                        "to #{new_version(dep)}"
+                end
 
           if vulnerabilities_fixed[dep.name]&.one?
             msg += " **This update includes a security fix.**"
@@ -328,6 +336,8 @@ def metadata_cascades
       end
 
       def metadata_cascades_for_dep(dependency)
+        return "" if dependency.removed?
+
         MetadataPresenter.new(
           dependency: dependency,
           source: source,

common/lib/dependabot/security_advisory.rb

@@ -62,6 +62,9 @@ def fixed_by?(dependency)
       # Ignore deps that weren't previously vulnerable
       return false unless affects_version?(dependency.previous_version)
 
+      # Removing a dependency is a way to fix the vulnerability
+      return true if dependency.removed?
+
       # Select deps that are now fixed
       !affects_version?(dependency.version)
     end

common/spec/dependabot/pull_request_creator/message_builder_spec.rb

@@ -1531,6 +1531,45 @@ def commits_details(base:, head:)
         end
       end
 
+      context "removing a transitive dependency" do
+        let(:dependencies) { [removed_dependency, dependency] }
+        let(:removed_dependency) do
+          Dependabot::Dependency.new(
+            name: "statesman",
+            version: "",
+            previous_version: "1.6.0",
+            package_manager: "dummy",
+            requirements: [],
+            previous_requirements: []
+          )
+        end
+
+        it "includes details of both dependencies" do
+          expect(pr_message).
+            to eq(
+              "Bumps [statesman](https://github.com/gocardless/statesman) "\
+              "and [business](https://github.com/gocardless/business). "\
+              "These dependencies needed to be updated together.\n"\
+              "Removes `statesman`\n"\
+              "Updates `business` from 1.4.0 to 1.5.0\n"\
+              "<details>\n"\
+              "<summary>Changelog</summary>\n"\
+              "<p><em>Sourced from <a href=\"https://github.com/gocardless/"\
+              "business/blob/master/CHANGELOG.md\">"\
+              "business's changelog</a>.</em></p>\n"\
+              "<blockquote>\n"\
+              "<h2>1.5.0 - June 2, 2015</h2>\n"\
+              "<ul>\n"\
+              "<li>Add 2016 holiday definitions</li>\n"\
+              "</ul>\n"\
+              "</blockquote>\n"\
+              "</details>\n"\
+              "#{commits_details(base: 'v1.4.0', head: 'v1.5.0')}"\
+              "<br />\n"
+            )
+        end
+      end
+
       context "with multiple git source requirements", :vcr do
         include_context "with multiple git sources"
 

common/spec/dependabot/security_advisory_spec.rb

@@ -183,6 +183,11 @@
         it { is_expected.to eq(false) }
       end
     end
+
+    context "with a removed dependency" do
+      let(:dependency_version) { "" }
+      it { is_expected.to eq(true) }
+    end
   end
 
   describe "#affects_version?" do

npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb

@@ -111,7 +111,8 @@ def vulnerability_audit
         @vulnerability_audit ||=
           VulnerabilityAuditor.new(
             dependency_files: dependency_files,
-            credentials: credentials
+            credentials: credentials,
+            allow_removal: @options.key?(:npm_transitive_dependency_removal)
           ).audit(
             dependency: dependency,
             security_advisories: security_advisories

npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/vulnerability_auditor.rb

@@ -15,9 +15,10 @@ module Dependabot
   module NpmAndYarn
     class UpdateChecker < Dependabot::UpdateCheckers::Base
       class VulnerabilityAuditor
-        def initialize(dependency_files:, credentials:)
+        def initialize(dependency_files:, credentials:, allow_removal: false)
           @dependency_files = dependency_files
           @credentials = credentials
+          @allow_removal = allow_removal
         end
 
         # Finds any dependencies in the `package-lock.json` or `npm-shrinkwrap.json` that have
@@ -96,7 +97,7 @@ def viable_audit_result?(audit_result, security_advisories)
 
         def validate_audit_result(audit_result, security_advisories)
           return :fix_unavailable unless audit_result["fix_available"]
-          return :vulnerable_dependency_removed if vulnerable_dependency_removed?(audit_result)
+          return :vulnerable_dependency_removed if !@allow_removal && vulnerable_dependency_removed?(audit_result)
           return :dependency_still_vulnerable if dependency_still_vulnerable?(audit_result, security_advisories)
           return :downgrades_dependencies if downgrades_dependencies?(audit_result)
 
@@ -108,6 +109,9 @@ def vulnerable_dependency_removed?(audit_result)
         end
 
         def dependency_still_vulnerable?(audit_result, security_advisories)
+          # vulnerable depenendency is removed if the target version is nil
+          return false unless audit_result["target_version"]
+
           version = Version.new(audit_result["target_version"])
           security_advisories.any? { |a| a.vulnerable?(version) }
         end
@@ -121,6 +125,8 @@ def downgrades_dependencies?(audit_result)
         end
 
         def downgrades_version?(current_version, target_version)
+          return false unless target_version
+
           current = Version.new(current_version)
           target = Version.new(target_version)
           current > target

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/vulnerability_auditor_spec.rb

@@ -16,9 +16,10 @@
       "password" => "token"
     }]
   end
+  let(:allow_removal) { true }
 
   subject do
-    described_class.new(dependency_files: dependency_files, credentials: credentials)
+    described_class.new(dependency_files: dependency_files, credentials: credentials, allow_removal: allow_removal)
   end
 
   describe "#audit" do
@@ -69,7 +70,7 @@
     context "when a fix removes the vulnerable dependency" do
       let(:dependency_files) { project_dependency_files("npm8/locked_transitive_dependency_removed") }
 
-      it "returns fix_available => false" do
+      it "omits target_version to indicate removal" do
         security_advisories = [
           Dependabot::SecurityAdvisory.new(
             dependency_name: dependency.name,
@@ -79,9 +80,37 @@
           )
         ]
 
-        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(
+            "dependency_name" => dependency.name,
+            "current_version" => "1.0.0",
+            "fix_available" => true,
+            "fix_updates" => [{
+              "dependency_name" => "@dependabot-fixtures/npm-remove-dependency",
+              "current_version" => "10.0.0",
+              "target_version" => "10.0.1",
+              "top_level_ancestors" => []
+            }],
+            "top_level_ancestors" => ["@dependabot-fixtures/npm-remove-dependency"]
+          )
+      end
+
+      context "when removal is disabled" do
+        let(:allow_removal) { false }
+
+        it "returns fix_available => false" do
+          security_advisories = [
+            Dependabot::SecurityAdvisory.new(
+              dependency_name: dependency.name,
+              package_manager: "npm_and_yarn",
+              vulnerable_versions: ["<1.0.1"],
+              safe_versions: ["1.0.1"]
+            )
+          ]
+
+          expect(subject.audit(dependency: dependency, security_advisories: security_advisories)).
+            to include("fix_available" => false)
+        end
       end
     end
 

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb

@@ -204,7 +204,7 @@
           let(:dependency_files) { project_dependency_files("npm8/locked_transitive_dependency_removed") }
           let(:registry_listing_url) { "https://registry.npmjs.org/locked_transitive_dependency_removed" }
 
-          it "doesn't allow an update yet" do
+          it "doesn't allow an update because removal has not been enabled" do
             expect(checker.can_update?(requirements_to_unlock: :all)).to eq(false)
           end
         end
@@ -1443,6 +1443,51 @@
           )
         end
       end
+
+      context "when the vulnerable transitive dependency is removed as a result of updating its parent" do
+        let(:dependency_files) { project_dependency_files("npm8/locked_transitive_dependency_removed") }
+        let(:registry_listing_url) { "https://registry.npmjs.org/locked-transitive-dependency-removed" }
+        let(:options) do
+          {
+            npm_transitive_security_updates: true,
+            npm_transitive_dependency_removal: true
+          }
+        end
+
+        it "correctly updates the parent dependency and removes the transitive because removal is enabled" do
+          expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly(
+            Dependabot::Dependency.new(
+              name: "@dependabot-fixtures/npm-transitive-dependency",
+              package_manager: "npm_and_yarn",
+              previous_requirements: [],
+              previous_version: "1.0.0",
+              requirements: [],
+              version: ""
+            ),
+            Dependabot::Dependency.new(
+              name: "@dependabot-fixtures/npm-remove-dependency",
+              package_manager: "npm_and_yarn",
+              previous_requirements: [{
+                requirement: "10.0.0",
+                file: "package.json",
+                groups: ["dependencies"],
+                source: {
+                  type: "registry",
+                  url: "https://registry.npmjs.org"
+                }
+              }],
+              previous_version: "10.0.0",
+              requirements: [{
+                requirement: "10.0.1",
+                file: "package.json",
+                groups: ["dependencies"],
+                source: nil
+              }],
+              version: "10.0.1"
+            )
+          )
+        end
+      end
     end
   end