Merge pull request #6361 from dependabot/mctofu/go-transitive-securit…

…y-updates

[gomod] Support updating indirect dependencies

go_modules/lib/dependabot/go_modules/file_parser.rb

@@ -53,7 +53,7 @@ def dependency_from_details(details)
         Dependency.new(
           name: details["Path"],
           version: version,
-          requirements: details["Indirect"] || dependency_is_replaced(details) ? [] : reqs,
+          requirements: details["Indirect"] ? [] : reqs,
           package_manager: "go_modules"
         )
       end
@@ -155,14 +155,13 @@ def git_revision(dep)
       end
 
       def skip_dependency?(dep)
-        return true if dep["Indirect"]
+        # Updating replaced dependencies is not supported
+        return true if dependency_is_replaced(dep)
 
-        begin
-          path_uri = URI.parse("https://#{dep['Path']}")
-          !path_uri.host.include?(".")
-        rescue URI::InvalidURIError
-          false
-        end
+        path_uri = URI.parse("https://#{dep['Path']}")
+        !path_uri.host.include?(".")
+      rescue URI::InvalidURIError
+        false
       end
 
       def dependency_is_replaced(details)

go_modules/lib/dependabot/go_modules/file_updater.rb

@@ -28,7 +28,7 @@ def self.updated_files_regex
       def updated_dependency_files
         updated_files = []
 
-        if go_mod && file_changed?(go_mod)
+        if go_mod && dependency_changed?(go_mod)
           updated_files <<
             updated_file(
               file: go_mod,
@@ -56,6 +56,11 @@ def updated_dependency_files
 
       private
 
+      def dependency_changed?(go_mod)
+        # file_changed? only checks for changed requirements. Need to check for indirect dep version changes too.
+        file_changed?(go_mod) || dependencies.any? { |dep| dep.previous_version != dep.version }
+      end
+
       def check_required_files
         return if go_mod
 

go_modules/lib/dependabot/go_modules/update_checker.rb

@@ -13,17 +13,6 @@ class UpdateChecker < Dependabot::UpdateCheckers::Base
       require_relative "update_checker/latest_version_finder"
 
       def latest_resolvable_version
-        # We don't yet support updating indirect dependencies for go_modules
-        #
-        # To update indirect dependencies we'll need to promote the indirect
-        # dependency to the go.mod file forcing the resolver to pick this
-        # version (possibly as `// indirect`)
-        unless dependency.top_level?
-          return unless dependency.version
-
-          return current_version
-        end
-
         latest_version_finder.latest_version
       end
 
@@ -37,12 +26,6 @@ def latest_version
       def lowest_resolvable_security_fix_version
         raise "Dependency not vulnerable!" unless vulnerable?
 
-        unless dependency.top_level?
-          return unless dependency.version
-
-          return current_version
-        end
-
         lowest_security_fix_version
       end
 

go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb

@@ -75,6 +75,10 @@ def fetch_lowest_security_fix_version
         end
 
         def available_versions
+          @available_versions ||= fetch_available_versions
+        end
+
+        def fetch_available_versions
           SharedHelpers.in_a_temporary_directory do
             SharedHelpers.with_git_configured(credentials: credentials) do
               manifest = parse_manifest

go_modules/spec/dependabot/go_modules/file_parser_spec.rb

@@ -40,7 +40,7 @@
   describe "parse" do
     subject(:dependencies) { parser.parse }
 
-    its(:length) { is_expected.to eq(3) }
+    its(:length) { is_expected.to eq(4) }
 
     describe "top level dependencies" do
       subject(:dependencies) do
@@ -130,16 +130,34 @@
       end
     end
 
+    describe "indirect dependencies" do
+      subject(:dependencies) do
+        parser.parse.reject(&:top_level?)
+      end
+
+      its(:length) { is_expected.to eq(2) }
+
+      describe "a dependency that uses go modules" do
+        subject(:dependency) do
+          dependencies.find { |d| d.name == "github.com/mattn/go-isatty" }
+        end
+
+        it "has the right details" do
+          expect(dependency).to be_a(Dependabot::Dependency)
+          expect(dependency.name).to eq("github.com/mattn/go-isatty")
+          expect(dependency.version).to eq("0.0.4")
+          expect(dependency.requirements).to be_empty
+        end
+      end
+    end
+
     describe "a dependency that is replaced" do
       subject(:dependency) do
         dependencies.find { |d| d.name == "rsc.io/qr" }
       end
 
-      it "has the right details" do
-        expect(dependency).to be_a(Dependabot::Dependency)
-        expect(dependency.name).to eq("rsc.io/qr")
-        expect(dependency.version).to eq("0.1.0")
-        expect(dependency.requirements).to eq([])
+      it "is skipped as unsupported" do
+        expect(dependency).to be_nil
       end
     end
 
@@ -274,9 +292,8 @@
         dependencies.find { |d| d.name == "rsc.io/qr" }
       end
 
-      it "has the right details" do
-        expect(dependency).to be_a(Dependabot::Dependency)
-        expect(dependency.name).to eq("rsc.io/qr")
+      it "is skipped as unsupported" do
+        expect(dependency).to be_nil
       end
     end
 
@@ -325,7 +342,6 @@
       it "parses root file" do
         expect(dependencies.map(&:name)).
           to eq(%w(
-            github.com/dependabot/vgotest/common
             rsc.io/qr
           ))
       end
@@ -337,7 +353,6 @@
         it "parses nested file" do
           expect(dependencies.map(&:name)).
             to eq(%w(
-              github.com/dependabot/vgotest/common
               rsc.io/qr
             ))
         end

go_modules/spec/dependabot/go_modules/file_updater_spec.rb

@@ -84,6 +84,19 @@
       expect(updated_files.find { |f| f.name == "go.sum" }).to_not be_nil
     end
 
+    context "with an indirect dependency update" do
+      let(:requirements) { [] }
+      let(:previous_requirements) { [] }
+
+      it "includes an updated go.mod" do
+        expect(updated_files.find { |f| f.name == "go.mod" }).to_not be_nil
+      end
+
+      it "includes an updated go.sum" do
+        expect(updated_files.find { |f| f.name == "go.sum" }).to_not be_nil
+      end
+    end
+
     context "with an invalid module path" do
       let(:stderr) do
         <<~STDERR

go_modules/spec/dependabot/go_modules/update_checker_spec.rb

@@ -63,12 +63,11 @@ module foobar
       end
     end
 
-    context "doesn't update indirect dependencies (not supported)" do
+    context "updates indirect dependencies" do
       let(:requirements) { [] }
-      it do
-        is_expected.to eq(
-          Dependabot::GoModules::Version.new(dependency.version)
-        )
+
+      it "updates to the newer version" do
+        is_expected.to eq(Dependabot::GoModules::Version.new("1.1.0"))
       end
     end
 
@@ -123,12 +122,11 @@ module foobar
       end
     end
 
-    context "doesn't update indirect dependencies (not supported)" do
+    context "updates indirect dependencies" do
       let(:requirements) { [] }
-      it do
-        is_expected.to eq(
-          Dependabot::GoModules::Version.new(dependency.version)
-        )
+
+      it "updates to the least new supported version" do
+        is_expected.to eq(Dependabot::GoModules::Version.new("1.0.5"))
       end
     end