[npm] Consider all installed versions when checking if a dependency is still vulnerable

common/lib/dependabot/dependency.rb

@@ -37,11 +37,11 @@ def self.register_name_normaliser(package_manager, name_builder)
 
     attr_reader :name, :version, :requirements, :package_manager,
                 :previous_version, :previous_requirements,
-                :subdependency_metadata
+                :subdependency_metadata, :metadata
 
     def initialize(name:, requirements:, package_manager:, version: nil,
                    previous_version: nil, previous_requirements: nil,
-                   subdependency_metadata: [], removed: false)
+                   subdependency_metadata: [], removed: false, metadata: {})
       @name = name
       @version = version
       @requirements = requirements.map { |req| symbolize_keys(req) }
@@ -54,6 +54,7 @@ def initialize(name:, requirements:, package_manager:, version: nil,
                                   map { |h| symbolize_keys(h) }
       end
       @removed = removed
+      @metadata = symbolize_keys(metadata || {})
 
       check_values
     end
@@ -105,6 +106,15 @@ def display_name
       display_name_builder.call(name)
     end
 
+    # Returns all detected versions of the dependency. Only ecosystems that
+    # support this feature will return more than the current version.
+    def all_versions
+      all_versions = metadata[:all_versions]
+      return [version].compact unless all_versions
+
+      all_versions.filter_map(&:version)
+    end
+
     def ==(other)
       other.instance_of?(self.class) && to_h == other.to_h
     end

common/lib/dependabot/file_parsers/base/dependency_set.rb

@@ -14,34 +14,42 @@ def initialize(dependencies = [], case_sensitive: false)
             raise ArgumentError, "must be an array of Dependency objects"
           end
 
-          @dependencies = dependencies
           @case_sensitive = case_sensitive
+          @dependencies = Hash.new { |hsh, key| hsh[key] = DependencySlot.new }
+          dependencies.each { |dep| self << dep }
         end
 
-        attr_reader :dependencies
+        def dependencies
+          @dependencies.values.filter_map(&:combined)
+        end
 
         def <<(dep)
           raise ArgumentError, "must be a Dependency object" unless dep.is_a?(Dependency)
 
-          existing_dependency = dependency_for_name(dep.name)
+          @dependencies[key_for_dependency(dep)] << dep
+          self
+        end
 
-          return self if existing_dependency&.to_h == dep.to_h
+        def +(other)
+          raise ArgumentError, "must be a DependencySet" unless other.is_a?(DependencySet)
 
-          if existing_dependency
-            dependencies[dependencies.index(existing_dependency)] =
-              combined_dependency(existing_dependency, dep)
-          else
-            dependencies << dep
+          other_names = other.dependencies.map(&:name)
+          other_names.each do |name|
+            all_versions = other.all_versions_for_name(name)
+            all_versions.each { |dep| self << dep }
           end
 
           self
         end
 
-        def +(other)
-          raise ArgumentError, "must be a DependencySet" unless other.is_a?(DependencySet)
+        def all_versions_for_name(name)
+          key = key_for_name(name)
+          @dependencies.key?(key) ? @dependencies[key].all_versions : []
+        end
 
-          other.dependencies.each { |dep| self << dep }
-          self
+        def dependency_for_name(name)
+          key = key_for_name(name)
+          @dependencies.key?(key) ? @dependencies[key].combined : nil
         end
 
         private
@@ -50,41 +58,98 @@ def case_sensitive?
           @case_sensitive
         end
 
-        def dependency_for_name(name)
-          return dependencies.find { |d| d.name == name } if case_sensitive?
+        def key_for_name(name)
+          case_sensitive? ? name : name.downcase
+        end
 
-          dependencies.find { |d| d.name&.downcase == name&.downcase }
+        def key_for_dependency(dep)
+          key_for_name(dep.name)
         end
 
-        def combined_dependency(old_dep, new_dep)
-          package_manager = old_dep.package_manager
-          v_cls = Utils.version_class_for_package_manager(package_manager)
-
-          # If we already have a requirement use the existing version
-          # (if present). Otherwise, use whatever the lowest version is
-          new_version =
-            if old_dep.requirements.any? then old_dep.version || new_dep.version
-            elsif !v_cls.correct?(new_dep.version) then old_dep.version
-            elsif !v_cls.correct?(old_dep.version) then new_dep.version
-            elsif v_cls.new(new_dep.version) > v_cls.new(old_dep.version)
-              old_dep.version
+        # There can only be one entry per dependency name in a `DependencySet`. Each entry
+        # is assigned a `DependencySlot`.
+        #
+        # In some ecosystems (like `npm_and_yarn`), however, multiple versions of a
+        # dependency may be encountered and added to the set. The `DependencySlot` retains
+        # all added versions and presents a single unified dependency for the entry
+        # that combines the attributes of these versions.
+        #
+        # The combined dependency is accessible via `DependencySet#dependencies` or
+        # `DependencySet#dependency_for_name`. The list of individual versions of the
+        # dependency is accessible via `DependencySet#all_versions_for_name`.
+        class DependencySlot
+          attr_reader :all_versions, :combined
+
+          def initialize
+            @all_versions = []
+            @combined = nil
+          end
+
+          def <<(dep)
+            return self if @all_versions.include?(dep)
+
+            @combined = if @combined
+                          combined_dependency(@combined, dep)
+                        else
+                          Dependency.new(
+                            name: dep.name,
+                            version: dep.version,
+                            requirements: dep.requirements,
+                            package_manager: dep.package_manager,
+                            subdependency_metadata: dep.subdependency_metadata
+                          )
+                        end
+
+            index_of_same_version =
+              @all_versions.find_index { |other| other.version == dep.version }
+
+            if index_of_same_version.nil?
+              @all_versions << dep
             else
-              new_dep.version
+              same_version = @all_versions[index_of_same_version]
+              @all_versions[index_of_same_version] = combined_dependency(same_version, dep)
             end
 
-          subdependency_metadata = (
-            (old_dep.subdependency_metadata || []) +
-            (new_dep.subdependency_metadata || [])
-          ).uniq
-
-          Dependency.new(
-            name: old_dep.name,
-            version: new_version,
-            requirements: (old_dep.requirements + new_dep.requirements).uniq,
-            package_manager: package_manager,
-            subdependency_metadata: subdependency_metadata
-          )
+            self
+          end
+
+          private
+
+          # Produces a new dependency by merging the attributes of `old_dep` with those of
+          # `new_dep`. Requirements and subdependency metadata will be combined and deduped.
+          # The version of the combined dependency is determined by the logic below.
+          def combined_dependency(old_dep, new_dep)
+            version = if old_dep.top_level? # Prefer a direct dependency over a transitive one
+                        old_dep.version || new_dep.version
+                      elsif !version_class.correct?(new_dep.version)
+                        old_dep.version
+                      elsif !version_class.correct?(old_dep.version)
+                        new_dep.version
+                      elsif version_class.new(new_dep.version) > version_class.new(old_dep.version)
+                        old_dep.version
+                      else
+                        new_dep.version
+                      end
+            requirements = (old_dep.requirements + new_dep.requirements).uniq
+            subdependency_metadata = (
+              (old_dep.subdependency_metadata || []) +
+              (new_dep.subdependency_metadata || [])
+            ).uniq
+
+            Dependency.new(
+              name: old_dep.name,
+              version: version,
+              requirements: requirements,
+              package_manager: old_dep.package_manager,
+              subdependency_metadata: subdependency_metadata
+            )
+          end
+
+          def version_class
+            @version_class ||= Utils.version_class_for_package_manager(@combined.package_manager)
+          end
         end
+        private_constant :DependencySlot
       end
     end
   end

common/spec/dependabot/dependency_spec.rb

@@ -270,4 +270,104 @@
       it { is_expected.to eq(nil) }
     end
   end
+
+  describe "#metadata" do
+    it "stores metadata given to initialize" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        metadata: { foo: 42 }
+      )
+      expect(dependency.metadata).to eq(foo: 42)
+    end
+
+    it "is mutable" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        metadata: { foo: 42 }
+      )
+
+      dependency.metadata[:all_versions] = []
+      expect(dependency.metadata).to eq(foo: 42, all_versions: [])
+    end
+
+    it "is not serialized" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        metadata: { foo: 42 }
+      )
+      expect(dependency.to_h.keys).not_to include("metadata")
+    end
+
+    it "isn't utilized by the equality operator" do
+      dependency1 = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        metadata: { foo: 42 }
+      )
+      dependency2 = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        metadata: { foo: 43 }
+      )
+      expect(dependency1).to eq(dependency2)
+    end
+  end
+
+  describe "#all_versions" do
+    it "returns an empty array by default" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy"
+      )
+
+      expect(dependency.all_versions).to eq([])
+    end
+
+    it "returns the dependency version if all_version metadata isn't present" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        version: "1.0.0"
+      )
+
+      expect(dependency.all_versions).to eq(["1.0.0"])
+    end
+
+    it "returns all_version metadata if present" do
+      dependency = described_class.new(
+        name: "dep",
+        requirements: [],
+        package_manager: "dummy",
+        version: "1.0.0",
+        metadata: {
+          all_versions: [
+            described_class.new(
+              name: "dep",
+              requirements: [],
+              package_manager: "dummy",
+              version: "1.0.0"
+            ),
+            described_class.new(
+              name: "dep",
+              requirements: [],
+              package_manager: "dummy",
+              version: "2.0.0"
+            )
+          ]
+        }
+      )
+
+      expect(dependency.all_versions).to eq(["1.0.0", "2.0.0"])
+    end
+  end
 end