Merge pull request #7446 from sigurdm/pub_smallest_update

Pub smallest update

pub/helpers/build

@@ -10,7 +10,7 @@ if [ -z "$DEPENDABOT_NATIVE_HELPERS_PATH" ]; then
 fi
 
 # Retrieve the dependencies
-dart pub get -C "$DEPENDABOT_NATIVE_HELPERS_PATH/pub/helpers"
+dart pub upgrade -C "$DEPENDABOT_NATIVE_HELPERS_PATH/pub/helpers"
 
 # Compile the helpers
 dart compile exe "$DEPENDABOT_NATIVE_HELPERS_PATH/pub/helpers/bin/dependency_services.dart" -o "$DEPENDABOT_NATIVE_HELPERS_PATH/pub/dependency_services"

pub/helpers/pubspec.lock

@@ -181,8 +181,8 @@ packages:
     dependency: "direct main"
     description:
       path: "."
-      ref: a16763a93b5050c6a9d917fca5a132cfcb00f1a9
-      resolved-ref: a16763a93b5050c6a9d917fca5a132cfcb00f1a9
+      ref: a42800e5a2f539dd5d86fdc3a6f3beefc971c753
+      resolved-ref: a42800e5a2f539dd5d86fdc3a6f3beefc971c753
       url: "https://github.com/dart-lang/pub"
     source: git
     version: "0.0.0"

pub/helpers/pubspec.yaml

@@ -12,4 +12,4 @@ dependencies:
   pub:
     git:
      url: https://github.com/dart-lang/pub
-     ref: a16763a93b5050c6a9d917fca5a132cfcb00f1a9
+     ref: a42800e5a2f539dd5d86fdc3a6f3beefc971c753

pub/lib/dependabot/pub/helpers.rb

@@ -35,6 +35,60 @@ def dependency_services_list
         JSON.parse(run_dependency_services("list"))["dependencies"]
       end
 
+      def repository_url(dependency)
+        source = dependency.requirements&.first&.dig(:source)
+        source&.dig("description", "url") || options[:pub_hosted_url] || "https://pub.dev"
+      end
+
+      def fetch_package_listing(dependency)
+        # Because we get the security_advisories as a set of constraints, we
+        # fetch the list of all versions and filter them to a list of vulnerable
+        # versions.
+        #
+        # Ideally we would like the helper to be the only one doing requests to
+        # the repository. But this should work for now:
+        response = Dependabot::RegistryClient.get(url: "#{repository_url(dependency)}/api/packages/#{dependency.name}")
+        JSON.parse(response.body)
+      end
+
+      def available_versions(dependency)
+        fetch_package_listing(dependency)["versions"].map do |v|
+          Dependabot::Pub::Version.new(v["version"])
+        end
+      end
+
+      def dependency_services_smallest_update
+        return @smallest_update if @smallest_update
+
+        security_advisories.each do |a|
+          # Sanity check, that we only get the advisories for a single package
+          # at a time. If we got all advisories for all current dependencies,
+          # the helper would be able to handle it, but we would need a better
+          # way to find the repository url.
+          if a.dependency_name != dependency.name
+            raise "Only expected advisories for #{dependency.name} got for #{a.dependency_name}"
+          end
+        end
+        vulnerable_versions = available_versions(dependency).select do |v|
+          security_advisories.any? { |a| a.vulnerable?(v) }
+        end
+        input = {
+          # For "smallest update" we don't cache the report to be shared between
+          # dependencies, but run a specific report for the current dependency.
+          target: dependency.name,
+          disallowed:
+            [
+              {
+                name: dependency.name,
+                url: repository_url(dependency),
+                versions: vulnerable_versions.map { |v| { range: v.to_s } }
+              }
+            ]
+        }
+        report = JSON.parse(run_dependency_services("report", stdin_data: JSON.generate(input)))["dependencies"]
+        @smallest_update = report.find { |d| d["name"] == dependency.name }["smallestUpdate"]
+      end
+
       def dependency_services_report
         sha256 = Digest::SHA256.new
         dependency_files.each do |f|
@@ -45,7 +99,7 @@ def dependency_services_report
         cache_file = "/tmp/report-#{hash}-pid-#{Process.pid}.json"
         return JSON.parse(File.read(cache_file)) if File.file?(cache_file)
 
-        report = JSON.parse(run_dependency_services("report"))["dependencies"]
+        report = JSON.parse(run_dependency_services("report", stdin_data: ""))["dependencies"]
         File.write(cache_file, JSON.generate(report))
         report
       end

pub/lib/dependabot/pub/requirement.rb

@@ -52,7 +52,7 @@ def initialize(*requirements, raw_constraint: nil)
 
       def to_s
         if @raw_constraint.nil?
-          as_list.join ", "
+          as_list.join " "
         else
           @raw_constraint
         end

pub/lib/dependabot/pub/update_checker.rb

@@ -42,26 +42,37 @@ def lowest_resolvable_security_fix_version
       end
 
       def lowest_security_fix_version
-        # TODO: Pub lacks a lowest-non-vulnerable version strategy, for now we simply bump to latest resolvable:
-        # https://github.com/dependabot/dependabot-core/issues/5391
-        relevant_version = latest_resolvable_version
-        return unless relevant_version
-
-        # NOTE: in other ecosystems, the native helpers return a list of possible versions, to which we apply
-        # post-filtering. Ideally we move toward a world where we hand the native helper a list of ignored versions
-        # and possibly a flag indicating "use min version rather than max". The pub team is interested in supporting
-        # that. But in the meantime for internal consistency with other dependabot ecosystem implementations I kept
-        # `relevant_versions` as an array.
-        relevant_versions = [relevant_version]
-        relevant_versions = Dependabot::UpdateCheckers::VersionFilters.filter_vulnerable_versions(relevant_versions,
-                                                                                                  security_advisories)
-        relevant_versions.min
+        # Don't attempt to do security updates for git dependencies.
+        return nil if git_revision? dependency.version
+        # If the current version is not vulnerable, we stay on it.
+        return version_unless_ignored dependency.version unless vulnerable?
+
+        e = dependency_services_smallest_update
+        return nil if e.nil?
+
+        upgrade = e.find { |u| u["name"] == dependency.name }
+
+        version = upgrade["version"]
+        version_unless_ignored(version)
       end
 
       def updated_requirements
         # Requirements that need to be changed, if obtain:
-        # latest_resolvable_version
-        entry = current_report["singleBreaking"].find { |d| d["name"] == dependency.name }
+        # latest_resolvable_version or lowest_security_fix_version
+        entry = if vulnerable?
+                  updates = dependency_services_smallest_update
+
+                  # Ideally we would like to do any upgrade that migrates away from the vulnerability
+                  # but this method can only return a single requirement udate.
+                  breaking_changes = updates.filter { |d| d["previousConstraint"] != d["constraintBumpedIfNeeded"] }
+                  if breaking_changes.size > 1
+                    raise "Cannot upgrade from vulnerability without unlocking other packages."
+                  end
+
+                  updates.find { |u| u["name"] == dependency.name }
+                else
+                  current_report["singleBreaking"].find { |d| d["name"] == dependency.name }
+                end
         return unless entry
 
         parse_updated_dependency(entry, requirements_update_strategy: resolved_requirements_update_strategy).
@@ -108,8 +119,13 @@ def latest_version_resolvable_with_full_unlock?
       end
 
       def updated_dependencies_after_full_unlock
+        report_section = if vulnerable?
+                           dependency_services_smallest_update
+                         else
+                           current_report["multiBreaking"]
+                         end
         # We only expose non-transitive dependencies here...
-        direct_deps = current_report["multiBreaking"].reject do |d|
+        direct_deps = report_section.reject do |d|
           d["kind"] == "transitive"
         end
         direct_deps.map do |d|

pub/spec/dependabot/pub/metadata_finder_spec.rb

@@ -61,7 +61,7 @@
   describe "#source_url" do
     let(:dependency) do
       Dependabot::Dependency.new(
-        name: "protobuf",
+        name: "old_protobuf",
         version: "2.0.1",
         requirements: [{
           file: "pubspec.yaml",

pub/spec/dependabot/pub/update_checker_spec.rb

@@ -406,8 +406,8 @@
         expect(updated_dependencies).to eq [
           {
             "name" => "protobuf",
-            "version" => "2.0.0",
-            "requirements" => [{ requirement: "2.0.0", groups: ["direct"], source: nil, file: "pubspec.yaml" }],
+            "version" => "2.1.0",
+            "requirements" => [{ requirement: "2.1.0", groups: ["direct"], source: nil, file: "pubspec.yaml" }],
             "previous_version" => "1.1.4",
             "previous_requirements" => [{
               requirement: "1.1.4", groups: ["direct"], source: nil, file: "pubspec.yaml"
@@ -423,6 +423,16 @@
               requirement: "0.10.11", groups: ["direct"], source: nil, file: "pubspec.yaml"
             }],
             "package_manager" => "pub"
+          },
+          {
+            "name" => "collection",
+            "package_manager" => "pub",
+            "previous_requirements" => [{ file: "pubspec.yaml", groups: ["direct"], requirement: "^1.14.13",
+                                          source: nil }],
+            "previous_version" => "1.14.13",
+            "requirements" => [{ file: "pubspec.yaml", groups: ["direct"], requirement: "^1.16.0",
+                                 source: nil }],
+            "version" => "1.16.0"
           }
 
         ]
@@ -480,16 +490,63 @@
         )
       ]
     end
-
+    before do
+      # Allow network. We use it to install flutter.
+      WebMock.allow_net_connect!
+      # To find the vulnerable versions we do a package listing before invoking the helper.
+      # Stub this out here:
+      stub_request(:get, "http://localhost:#{@server[:Port]}/api/packages/#{dependency.name}").to_return(
+        status: 200,
+        body: fixture("pub_dev_responses/simple/#{dependency.name}.json"),
+        headers: {}
+      )
+    end
     context "when a newer non-vulnerable version is available" do
-      # TODO: Implement https://github.com/dependabot/dependabot-core/issues/5391, then flip "highest" to "lowest"
-      it "updates to the highest non-vulnerable version" do
-        is_expected.to eq(Gem::Version.new("3.1.0"))
+      it "updates to the lowest non-vulnerable version" do
+        is_expected.to eq(Gem::Version.new("3.0.0"))
       end
     end
 
-    # TODO: should it update indirect deps for security vulnerabilities? I assume Pub has these?
-    # examples of how to write tests in go_modules/update_checker_spec
+    context "Can unlock transitive deps" do
+      let(:requirements_to_unlock) { :all }
+      let(:dependency_name) { "protobuf" }
+      let(:dependency_version) { "1.1.4" }
+      let(:security_advisories) do
+        [
+          Dependabot::SecurityAdvisory.new(
+            dependency_name: dependency_name,
+            package_manager: "pub",
+            vulnerable_versions: ["1.1.4"]
+          )
+        ]
+      end
+      it "can update" do
+        expect(checker.vulnerable?).to be_truthy
+        expect(checker.lowest_resolvable_security_fix_version).to eq("2.0.0")
+        expect(updated_dependencies).to eq [
+          {
+            "name" => "protobuf",
+            "version" => "2.0.0",
+            "requirements" => [{ requirement: "2.0.0", groups: ["direct"], source: nil, file: "pubspec.yaml" }],
+            "previous_version" => "1.1.4",
+            "previous_requirements" => [{
+              requirement: "1.1.4", groups: ["direct"], source: nil, file: "pubspec.yaml"
+            }],
+            "package_manager" => "pub"
+          },
+          {
+            "name" => "fixnum",
+            "version" => "1.0.0",
+            "requirements" => [{ requirement: "1.0.0", groups: ["direct"], source: nil, file: "pubspec.yaml" }],
+            "previous_version" => "0.10.11",
+            "previous_requirements" => [{
+              requirement: "0.10.11", groups: ["direct"], source: nil, file: "pubspec.yaml"
+            }],
+            "package_manager" => "pub"
+          }
+        ]
+      end
+    end
 
     context "when the current version is not newest but also not vulnerable" do
       let(:dependency_version) { "3.0.0" } # 3.1.0 is latest
@@ -502,12 +559,24 @@
   end
 
   describe "#lowest_security_fix_version" do
+    before do
+      # Allow network. We use it to install flutter.
+      WebMock.allow_net_connect!
+      # To find the vulnerable versions we do a package listing before invoking the helper.
+      # Stub this out here:
+      stub_request(:get, "http://localhost:#{@server[:Port]}/api/packages/#{dependency.name}").to_return(
+        status: 200,
+        body: fixture("pub_dev_responses/simple/#{dependency.name}.json"),
+        headers: {}
+      )
+    end
     subject(:lowest_security_fix_version) { checker.lowest_security_fix_version }
     let(:dependency_name) { "retry" }
+    let(:dependency_version) { "2.0.0" }
 
     # TODO: Implement https://github.com/dependabot/dependabot-core/issues/5391, then flip "highest" to "lowest"
-    it "finds the highest available non-vulnerable version" do
-      is_expected.to eq(Gem::Version.new("3.1.0"))
+    it "keeps current version if it is not vulnerable" do
+      is_expected.to eq(Gem::Version.new("2.0.0"))
     end
 
     context "with a security vulnerability on older versions" do
@@ -521,9 +590,8 @@
         ]
       end
 
-      # TODO: Implement https://github.com/dependabot/dependabot-core/issues/5391, then flip "highest" to "lowest"
-      it "finds the highest available non-vulnerable version" do
-        is_expected.to eq(Gem::Version.new("3.1.0"))
+      it "finds the lowest available non-vulnerable version" do
+        is_expected.to eq(Gem::Version.new("3.0.0"))
       end
 
       # it "returns nil for git versions" # tested elsewhere under `context "With a git dependency"`
@@ -609,6 +677,21 @@
     let(:foo_pubspec) { File.join(git_dir, "pubspec.yaml") }
 
     let(:dependency_name) { "foo" }
+    let(:requirements) do
+      [{
+        file: "pubspec.yaml",
+        requirement: "~3.0.0",
+        groups: [],
+        source: {
+          "type" => "git",
+          "description" => {
+            "url" => git_dir,
+            "path" => "foo",
+            "ref" => "1adc00411d4e1184d248d0147de3348a287f2fea"
+          }
+        }
+      }]
+    end
     let(:dependency_version) do
       FileUtils.mkdir_p git_dir
       run_git ["init"], git_dir

pub/spec/fixtures/projects/can_update/pubspec.yaml

@@ -4,6 +4,6 @@ environment:
 dependencies:
   collection: ^1.14.13 # Locked to 1.14.13, can update with no unlock.
   retry: ^2.0.0 # Can update with updated constraint, no further constraints.
-  protobuf: 1.1.4 # Can update with updated constraint, only together with fixnum.
+  protobuf: 1.1.4 # Can update with updated constraint, only together with fixnum to 2.0.0 or with fixnum and collection to 2.1.0.
   fixnum: 0.10.11
   path: 1.8.0 # Already at latest