Support for transitive dependency removal

bin/dry-run.rb

@@ -732,7 +732,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

@@ -41,7 +41,7 @@ def self.register_name_normaliser(package_manager, name_builder)
 
     def initialize(name:, requirements:, package_manager:, version: nil,
                    previous_version: nil, previous_requirements: nil,
-                   subdependency_metadata: [])
+                   subdependency_metadata: [], removed: false)
       @name = name
       @version = version
       @requirements = requirements.map { |req| symbolize_keys(req) }
@@ -53,6 +53,7 @@ def initialize(name:, requirements:, package_manager:, version: nil,
         @subdependency_metadata = subdependency_metadata&.
                                   map { |h| symbolize_keys(h) }
       end
+      @removed = removed
 
       check_values
     end
@@ -61,6 +62,10 @@ def top_level?
       requirements.any?
     end
 
+    def removed?
+      @removed
+    end
+
     def to_h
       {
         "name" => name,
@@ -69,7 +74,8 @@ def to_h
         "previous_version" => previous_version,
         "previous_requirements" => previous_requirements,
         "package_manager" => package_manager,
-        "subdependency_metadata" => subdependency_metadata
+        "subdependency_metadata" => subdependency_metadata,
+        "removed" => removed? ? true : nil
       }.compact
     end
 

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/dependency_spec.rb

@@ -217,6 +217,27 @@
         is_expected.to eq(expected)
       end
     end
+
+    context "when removed" do
+      let(:dependency_args) do
+        {
+          name: "dep",
+          requirements: [],
+          package_manager: "dummy",
+          removed: true
+        }
+      end
+
+      it do
+        expected = {
+          "name" => "dep",
+          "package_manager" => "dummy",
+          "requirements" => [],
+          "removed" => true
+        }
+        is_expected.to eq(expected)
+      end
+    end
   end
 
   describe "#subdependency_metadata" do

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",
+            previous_version: "1.6.0",
+            package_manager: "dummy",
+            requirements: [],
+            previous_requirements: [],
+            removed: true
+          )
+        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

@@ -131,7 +131,8 @@
         version: dependency_version,
         previous_version: dependency_previous_version,
         requirements: [],
-        previous_requirements: []
+        previous_requirements: [],
+        removed: removed
       )
     end
     let(:package_manager) { "dummy" }
@@ -140,6 +141,7 @@
     let(:safe_versions) { [Gem::Requirement.new("~> 1.11.0")] }
     let(:dependency_version) { "1.11.1" }
     let(:dependency_previous_version) { "0.7.1" }
+    let(:removed) { false }
 
     it { is_expected.to eq(true) }
 
@@ -183,6 +185,12 @@
         it { is_expected.to eq(false) }
       end
     end
+
+    context "with a removed dependency" do
+      let(:dependency_version) { nil }
+      let(:removed) { true }
+      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
@@ -141,34 +142,37 @@ def updated_dependencies_after_full_unlock
           map { |update_details| build_updated_dependency(update_details) }
       end
 
+      # rubocop:disable Metrics/AbcSize
       def conflicting_updated_dependencies
         top_level_dependencies = top_level_dependency_lookup
 
         updated_deps = []
         vulnerability_audit["fix_updates"].each do |update|
           dependency_name = update["dependency_name"]
           requirements = top_level_dependencies[dependency_name]&.requirements || []
-          conflicting_dep = Dependency.new(
-            name: dependency_name,
-            package_manager: "npm_and_yarn",
-            requirements: requirements
-          )
 
           updated_deps << build_updated_dependency(
-            dependency: conflicting_dep,
+            dependency: Dependency.new(
+              name: dependency_name,
+              package_manager: "npm_and_yarn",
+              requirements: requirements
+            ),
             version: update["target_version"],
             previous_version: update["current_version"]
           )
         end
+        # rubocop:enable Metrics/AbcSize
 
         # We don't need to update this but need to include it so it's described
         # in the PR and we'll pass validation that this dependency is at a
         # non-vulnerable version.
         if updated_deps.none? { |dep| dep.name == dependency.name }
+          target_version = vulnerability_audit["target_version"]
           updated_deps << build_updated_dependency(
             dependency: dependency,
-            version: vulnerability_audit["target_version"],
-            previous_version: dependency.version
+            version: target_version,
+            previous_version: dependency.version,
+            removed: target_version.nil?
           )
         end
 
@@ -189,7 +193,8 @@ def top_level_dependency_lookup
 
       def build_updated_dependency(update_details)
         original_dep = update_details.fetch(:dependency)
-        version = update_details.fetch(:version).to_s
+        removed = update_details.fetch(:removed, false)
+        version = update_details.fetch(:version).to_s unless removed
         previous_version = update_details.fetch(:previous_version)&.to_s
 
         Dependency.new(
@@ -203,7 +208,8 @@ def build_updated_dependency(update_details)
           ).updated_requirements,
           previous_version: previous_version,
           previous_requirements: original_dep.requirements,
-          package_manager: original_dep.package_manager
+          package_manager: original_dep.package_manager,
+          removed: removed
         )
       end
 

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,38 @@
           )
         ]
 
-        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(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)
+        end
       end
     end