Add dependency path parsing

Issue https://gitlab.com/gitlab-org/gitlab/-/issues/229472

We add a list of ancestors in shortest path of dependencies
For all vulnerable dependencies. We don't parse this path
for all items because everything is parsed on the fly and
for large projects it could be resource concuming.

ee/changelogs/unreleased/229472-parse-dep-path.yml

+---
+title: Parse dependency path, and present it to the frontend
+merge_request: 42596
+author:
+type: added

ee/lib/gitlab/ci/parsers/security/formatters/dependency_list.rb

@@ -13,7 +13,8 @@ module Gitlab
 
             def format(dependency, package_manager, file_path, vulnerabilities = [])
               {
-                name:     dependency['package']['name'],
+                name: dependency['package']['name'],
+                iid: dependency['iid'],
                 packager: packager(package_manager),
                 package_manager: package_manager,
                 location: formatted_location(dependency, file_path),
@@ -59,23 +60,20 @@ module Gitlab
               }
 
               return base_location if Feature.disabled?(:path_to_vulnerable_dependency, project)
+              return base_location unless dependency['iid']
 
-              # TODO: update this code before https://gitlab.com/gitlab-org/gitlab/-/issues/229472 is closed
-              # We temporary return test dependency path to get a PoC with integration to frontend
               base_location.merge({
-                                    ancestors:
-                                      [{
-                                         name: 'dep1',
-                                         version: '1.2'
-                                       },
-                                       {
-                                         name: 'dep2',
-                                         version: '10.11'
-                                       }],
-                                    top_level: false
+                                    ancestors: formatted_dependency_path(dependency['dependency_path']),
+                                    top_level: !!dependency['direct']
                                   })
             end
 
+            def formatted_dependency_path(dependency_path)
+              return unless dependency_path
+
+              dependency_path.map { |path| path.with_indifferent_access }
+            end
+
             # we know that Parsers::Security::DependencyList parses one vulnerability at a time
             # however, to keep interface compability with rest of the code and have MVC we return array
             # even tough we know that array's size will be 1

ee/lib/gitlab/ci/reports/dependency_list/dependency.rb

@@ -5,10 +5,11 @@ module Gitlab
     module Reports
       module DependencyList
         class Dependency
-          attr_reader :name, :packager, :package_manager, :location, :version, :licenses, :vulnerabilities
+          attr_reader :name, :iid, :packager, :package_manager, :location, :version, :licenses, :vulnerabilities
 
           def initialize(params = {})
             @name = params.fetch(:name)
+            @iid = params.fetch(:iid, nil)
             @packager = params.fetch(:packager)
             @package_manager = params.fetch(:package_manager)
             @location = params.fetch(:location)

ee/lib/gitlab/ci/reports/dependency_list/report.rb

@@ -5,17 +5,27 @@ module Gitlab
     module Reports
       module DependencyList
         class Report
+          # Stores dependency_path info in Hash of Hashes
+          # where keys of external Hash are path to dependency files
+          # and keys of internal Hashes are iid of dependencies
+          attr_reader :dependency_map
+
           def initialize
             @dependencies = {}
+            @dependency_map = {}
           end
 
           def dependencies
+            augment_ancestors!
             @dependencies.values.map(&:to_hash)
           end
 
           def add_dependency(dependency)
             dep = Dependency.new(dependency)
             key = dep.composite_key
+
+            store_dependency_path_info(dep) if dep.iid
+
             if @dependencies.has_key?(key)
               existing_dependency = @dependencies[key]
               existing_dependency.update_dependency(dependency)
@@ -36,6 +46,36 @@ module Gitlab
           def dependencies_with_licenses
             dependencies.select { |dependency| dependency[:licenses].any? }
           end
+
+          private
+
+          def augment_ancestors!
+            @dependencies.each_value do |dep|
+              next unless dep.iid
+              next if dep.location[:top_level]
+
+              if dep.vulnerabilities.empty?
+                dep.location.except!(:ancestors)
+              else
+                dependency_file = dep.location[:path]
+                dependencies_by_iid = dependency_map[dependency_file]
+
+                dep.location[:ancestors].map! do |ancestor|
+                  next ancestor unless ancestor.fetch(:iid, false)
+
+                  dependencies_by_iid[ancestor[:iid]]
+                end
+              end
+            end
+          end
+
+          def store_dependency_path_info(dependency)
+            dependency_file = dependency.location[:path]
+
+            dependency_map[dependency_file] ||= {}
+
+            dependency_map[dependency_file][dependency.iid] = { name: dependency.name, version: dependency.version }
+          end
         end
       end
     end

ee/spec/factories/dependencies.rb

@@ -42,6 +42,7 @@ FactoryBot.define do
     end
 
     trait :indirect do
+      iid { 42 }
       location do
         {
           blob_path: '/some_project/path/package_file.lock',
@@ -61,12 +62,12 @@ FactoryBot.define do
     end
 
     trait :direct do
+      iid { 24 }
       location do
         {
           blob_path: '/some_project/path/package_file.lock',
           path: 'package_file.lock',
-          ancestors:
-            [],
+          ancestors: nil,
           top_level: true
         }
       end

ee/spec/fixtures/security_reports/dependency_list/gl-dependency-scanning-report.json

@@ -306,115 +306,231 @@
           "package": {
             "name": "async"
           },
-          "version": "0.2.10"
+          "version": "0.2.10",
+          "iid": 1,
+          "direct":true
         },
         {
           "package": {
             "name": "async"
           },
-          "version": "1.5.2"
+          "version": "1.5.2",
+          "iid": 2,
+          "dependency_path": [
+            {
+              "iid": 7
+            },
+            {
+              "iid": 16
+            }
+          ]
         },
         {
           "package": {
             "name": "debug"
           },
-          "version": "1.0.5"
+          "version": "1.0.5",
+          "iid": 3,
+          "direct":true
         },
         {
           "package": {
             "name": "ejs"
           },
-          "version": "0.8.8"
+          "version": "0.8.8",
+          "iid": 4,
+          "direct":true
         },
         {
           "package": {
             "name": "ms"
           },
-          "version": "2.0.0"
+          "version": "2.0.0",
+          "iid": 5,
+          "dependency_path": [
+            {
+              "iid": 3
+            },
+            {
+              "iid": 17
+            }
+          ]
         },
         {
           "package": {
             "name": "node-forge"
           },
-          "version": "0.2.24"
+          "version": "0.2.24",
+          "iid": 6,
+          "direct":true
         },
         {
           "package": {
             "name": "saml2-js"
           },
-          "version": "1.5.0"
+          "version": "1.5.0",
+          "iid": 7,
+          "direct":true
         },
         {
           "package": {
             "name": "sax"
           },
-          "version": "1.2.4"
+          "version": "1.2.4",
+          "iid": 8,
+          "dependency_path": [
+            {
+              "iid": 12
+            },
+            {
+              "iid": 11
+            },
+            {
+              "iid": 9
+            }
+          ]
         },
         {
           "package": {
             "name": "underscore"
           },
-          "version": "1.9.1"
+          "version": "1.9.1",
+          "iid": 9,
+          "dependency_path": [
+            {
+              "iid": 12
+            },
+            {
+              "iid": 11
+            }
+          ]
         },
         {
           "package": {
             "name": "underscore"
           },
-          "version": "1.6.0"
+          "version": "1.6.0",
+          "iid": 10,
+          "direct":true
         },
         {
           "package": {
             "name": "xml-crypto"
           },
-          "version": "0.8.5"
+          "version": "0.8.5",
+          "iid": 11,
+          "dependency_path": [
+            {
+              "iid": 12
+            }
+          ]
         },
         {
           "package": {
             "name": "xml-encryption"
           },
-          "version": "0.7.4"
+          "version": "0.7.4",
+          "iid": 12,
+          "direct":true
         },
         {
           "package": {
             "name": "xml2js"
           },
-          "version": "0.4.19"
+          "version":"0.4.19",
+          "iid": 13,
+          "dependency_path": [
+            {
+              "iid": 7
+            }
+          ]
         },
         {
           "package": {
             "name": "xmlbuilder"
           },
-          "version": "2.1.0"
+          "version": "2.1.0",
+          "iid": 14,
+          "dependency_path":[
+            {
+              "iid": 12
+            },
+            {
+              "iid": 11
+            },
+            {
+              "iid": 9
+            }
+          ]
         },
         {
           "package": {
             "name": "xmlbuilder"
           },
-          "version": "9.0.7"
+          "version": "9.0.7",
+          "iid": 15,
+          "dependency_path": [
+            {
+              "iid": 12
+            },
+            {
+              "iid": 11
+            },
+            {
+              "iid": 9
+            }
+          ]
         },
         {
           "package": {
             "name": "xmldom"
           },
-          "version": "0.1.19"
+          "version": "0.1.19",
+          "iid": 16,
+          "dependency_path":
+          [
+            {
+              "iid": 7
+            }
+          ]
         },
         {
           "package": {
             "name": "xmldom"
           },
-          "version": "0.1.27"
+          "version": "0.1.27",
+          "iid": 17,
+          "dependency_path": [
+            {
+              "iid": 3
+            }
+          ]
         },
         {
           "package": {
             "name": "xpath.js"
           },
-          "version": "1.1.0"
+          "version": "1.1.0",
+          "iid": 18,
+          "dependency_path": [
+            {
+              "iid": 3
+            },
+            {
+              "iid": 17
+            },
+            {
+              "iid": 5
+            }
+          ]
         },
         {
           "package": {
             "name": "xpath"
           },
-          "version": "0.0.5"
+          "version": "0.0.5",
+          "iid": 19,
+          "direct":true
         }
       ]
     }

ee/spec/lib/gitlab/ci/parsers/security/dependency_list_spec.rb

@@ -25,9 +25,19 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyList do
         expect(report.dependencies[0][:name]).to eq('mini_portile2')
         expect(report.dependencies[0][:version]).to eq('2.2.0')
         expect(report.dependencies[0][:packager]).to eq('Ruby (Bundler)')
-        expect(report.dependencies[12][:packager]).to eq('JavaScript (Yarn)')
         expect(report.dependencies[0][:location][:path]).to eq('rails/Gemfile.lock')
+
+        expect(report.dependencies[12][:name]).to eq('xml-crypto')
+        expect(report.dependencies[12][:version]).to eq('0.8.5')
+        expect(report.dependencies[12][:packager]).to eq('JavaScript (Yarn)')
         expect(report.dependencies[12][:location][:blob_path]).to eq(blob_path)
+        expect(report.dependencies[12][:location][:top_level]).to be_falsey
+        expect(report.dependencies[12][:location][:ancestors]).to be_nil
+
+        expect(report.dependencies[13][:name]).to eq('xml-encryption')
+        expect(report.dependencies[13][:version]).to eq('0.7.4')
+        expect(report.dependencies[13][:location][:top_level]).to be_truthy
+        expect(report.dependencies[13][:location][:ancestors]).to be_nil
       end
 
       it 'merge vulnerabilities data' do

ee/spec/lib/gitlab/ci/parsers/security/formatters/dependency_list_spec.rb

@@ -16,26 +16,54 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Formatters::DependencyList do
   end
 
   describe '#format' do
-    let(:dependency) { parsed_report['dependency_files'][0]['dependencies'][0] }
     let(:package_manager) { 'bundler' }
-    let(:file_path) { 'rails/Gemfile.lock' }
+    let(:file_path) { 'file.path' }
     let(:data) { formatter.format(dependency, package_manager, file_path) }
-    let(:blob_path) { "/#{project.full_path}/-/blob/#{sha}/rails/Gemfile.lock" }
+    let(:blob_path) { "/#{project.full_path}/-/blob/#{sha}/file.path" }
 
     context 'with secure dependency' do
-      let(:dependency) { parsed_report['dependency_files'][0]['dependencies'][0] }
+      context 'with top-level dependency' do
+        let(:dependency) { parsed_report['dependency_files'][1]['dependencies'][0] }
+
+        it 'formats the dependency' do
+          expect(data[:name]).to eq('async')
+          expect(data[:iid]).to eq(1)
+          expect(data[:location][:blob_path]).to eq(blob_path)
+          expect(data[:location][:path]).to eq('file.path')
+          expect(data[:location][:top_level]).to be_truthy
+          expect(data[:location][:ancestors]).to be_nil
+        end
+      end
+
+      context 'with dependency path included' do
+        let(:dependency) { parsed_report['dependency_files'][1]['dependencies'][4] }
+
+        it 'formats the dependency' do
+          expect(data[:name]).to eq('ms')
+          expect(data[:iid]).to eq(5)
+          expect(data[:location][:blob_path]).to eq(blob_path)
+          expect(data[:location][:path]).to eq('file.path')
+          expect(data[:location][:top_level]).to be_falsey
+          expect(data[:location][:ancestors][0][:iid]).to eq(3)
+        end
+      end
+
+      context 'without dependency path' do
+        let(:dependency) { parsed_report['dependency_files'][0]['dependencies'][0] }
 
-      it 'format report into a right format' do
-        expect(data[:name]).to eq('mini_portile2')
-        expect(data[:packager]).to eq('Ruby (Bundler)')
-        expect(data[:package_manager]).to eq('bundler')
-        expect(data[:location][:blob_path]).to eq(blob_path)
-        expect(data[:location][:path]).to eq('rails/Gemfile.lock')
-        expect(data[:location][:top_level]).to be_falsey
-        expect(data[:location][:ancestors].first[:name]).to eq('dep1')
-        expect(data[:version]).to eq('2.2.0')
-        expect(data[:vulnerabilities]).to be_empty
-        expect(data[:licenses]).to be_empty
+        it 'formats the dependency' do
+          expect(data[:name]).to eq('mini_portile2')
+          expect(data[:iid]).to be_nil
+          expect(data[:packager]).to eq('Ruby (Bundler)')
+          expect(data[:package_manager]).to eq('bundler')
+          expect(data[:location][:blob_path]).to eq(blob_path)
+          expect(data[:location][:path]).to eq('file.path')
+          expect(data[:location][:top_level]).to be_nil
+          expect(data[:location][:ancestors]).to be_nil
+          expect(data[:version]).to eq('2.2.0')
+          expect(data[:vulnerabilities]).to be_empty
+          expect(data[:licenses]).to be_empty
+        end
       end
     end
 
@@ -49,7 +77,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Formatters::DependencyList do
 
       it { expect(location[:top_level]).to be_nil }
       it { expect(location[:ancestors]).to be_nil }
-      it { expect(location[:path]).to eq('rails/Gemfile.lock') }
+      it { expect(location[:path]).to eq('file.path') }
     end
 
     context 'with vulnerable dependency' do

ee/spec/lib/gitlab/ci/reports/dependency_list/dependency_spec.rb

@@ -3,23 +3,44 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Reports::DependencyList::Dependency do
-  let(:dependency_nokogiri) { build(:dependency, :nokogiri, :with_vulnerabilities) }
+  let(:dependency_nokogiri) do
+    {
+      name: 'nokogiri',
+      version: '1.8.0',
+      packager: 'Ruby (Bundler)',
+      package_manager: 'Ruby (Bundler)',
+      location: {
+        blob_path: '/some_project/path/package_file.lock',
+        path: 'package_file.lock',
+        ancestors: nil,
+        top_level: true
+      },
+      licenses: [],
+      vulnerabilities: [{
+                          name: 'DDoS',
+                          severity: 'high'
+                        },
+                        {
+                          name: 'XSS vulnerability',
+                          severity: 'low'
+                        }]
+    }
+  end
 
   context 'initialize' do
-    specify do
-      dependency_nokogiri[:location] = { blob_path: 'path/File_21.lock', path: 'File_21.lock' }
+    it 'sets all required properties' do
       dep = described_class.new(dependency_nokogiri)
 
       expect(dep.to_hash).to eq({ name: 'nokogiri',
                                   packager: 'Ruby (Bundler)',
                                   package_manager: 'Ruby (Bundler)',
-                                  location: { blob_path: 'path/File_21.lock', path: 'File_21.lock' },
+                                  location: { blob_path: '/some_project/path/package_file.lock', path: 'package_file.lock', top_level: true, ancestors: nil },
                                   version: '1.8.0',
                                   licenses: [],
-                                   vulnerabilities: [{ name: 'DDoS', severity: 'high' }, { name: 'XSS vulnerability', severity: 'low' }] })
+                                  vulnerabilities: [{ name: 'DDoS', severity: 'high' }, { name: 'XSS vulnerability', severity: 'low' }] })
     end
 
-    specify do
+    it 'keeps vulnerabilities that are not duplicates' do
       dependency_nokogiri[:vulnerabilities] << { name: 'problem', severity: 'high' }
       dep = described_class.new(dependency_nokogiri)
 
@@ -28,7 +49,7 @@ RSpec.describe Gitlab::Ci::Reports::DependencyList::Dependency do
                                                              { name: 'problem', severity: 'high' }])
     end
 
-    specify do
+    it 'removes vulnerability duplicates' do
       dependency_nokogiri[:vulnerabilities] << { name: 'DDoS', severity: 'high' }
       dep = described_class.new(dependency_nokogiri)
 

ee/spec/lib/gitlab/ci/reports/dependency_list/report_spec.rb

@@ -6,23 +6,109 @@ require 'benchmark/ips'
 RSpec.describe Gitlab::Ci::Reports::DependencyList::Report do
   let(:report) { described_class.new }
 
-  describe '#add_dependency' do
-    let(:dependency) do
-      {
-        name: 'gitlab',
-        packager: '',
-        package_manager: 'bundler',
-        location: { blob_path: '', path: 'Gemfile' },
-        version: '0.2.10',
-        vulnerabilities: [],
-        licenses: []
-      }
+  describe '#dependencies' do
+    subject(:dependencies) { report.dependencies }
+
+    context 'without dependency path information' do
+      let(:dependency) { build :dependency }
+
+      before do
+        report.add_dependency(dependency)
+      end
+
+      it 'returns array of hashes' do
+        expect(dependencies).to be_an(Array)
+        expect(dependencies.first).to be_a(Hash)
+      end
+
+      it 'does not contain dependency path' do
+        expect(dependencies.first[:location][:ancestors]).to be_nil
+      end
     end
 
-    subject { report.add_dependency(dependency) }
+    context 'with dependency path information' do
+      let(:indirect) { build :dependency, :with_vulnerabilities, iid: 32 }
+      let(:direct) { build :dependency, :direct, :with_vulnerabilities }
+
+      before do
+        indirect[:location][:top_level] = false
+        indirect[:location][:ancestors] = [{ iid: direct[:iid] }]
+
+        report.add_dependency(direct)
+        report.add_dependency(indirect)
+      end
+
+      it 'generates the dependency path' do
+        ancestors = dependencies.second[:location][:ancestors]
+
+        expect(ancestors.last).to eq({ name: direct[:name], version: direct[:version] })
+      end
+
+      context 'with multiple dependency files matching same package manager' do
+        let(:indirect_other) { build :dependency, :with_vulnerabilities, iid: 32 }
+        let(:direct_other) { build :dependency, :direct, :with_vulnerabilities }
+
+        before do
+          indirect_other[:location][:top_level] = false
+          indirect_other[:location][:ancestors] = [{ iid: direct[:iid] }]
+          indirect_other[:location][:path] = 'other_path.lock'
+          direct_other[:location][:path] = 'other_path.lock'
+
+          report.add_dependency(direct_other)
+          report.add_dependency(indirect_other)
+        end
+
+        it 'generates the dependency path' do
+          ancestors = dependencies.second[:location][:ancestors]
+          ancestors_other = dependencies.last[:location][:ancestors]
+
+          expect(ancestors.last).to eq({ name: direct[:name], version: direct[:version] })
+          expect(ancestors_other.last).to eq({ name: direct_other[:name], version: direct_other[:version] })
+        end
+      end
+
+      context 'when method is called more than once' do
+        it 'generates path only once' do
+          dependency_before = report.dependencies.last
+
+          expect(dependency_before[:name]).to eq(indirect[:name])
+          expect(dependency_before[:location][:ancestors].size).to eq(1)
+          expect(dependency_before[:location][:ancestors].last[:name]).to eq(direct[:name])
+
+          dependency_after = report.dependencies.last
+
+          expect(dependency_after[:name]).to eq(indirect[:name])
+          expect(dependency_after[:location][:ancestors].size).to eq(1)
+          expect(dependency_after[:location][:ancestors].last[:name]).to eq(direct[:name])
+        end
+      end
+
+      context 'with not vulnerable dependencies' do
+        let(:indirect_secure) { build :dependency, iid: 13 }
+
+        before do
+          indirect_secure[:location][:top_level] = false
+          indirect_secure[:location][:ancestors] = [{ iid: direct[:iid] }]
+
+          report.add_dependency(indirect_secure)
+        end
+
+        it 'augment dependency path only for vulnerable dependencies' do
+          vulnerable = dependencies.find { |dep| dep[:name] == indirect[:name] }
+          secure = dependencies.find { |dep| dep[:name] == indirect_secure[:name] }
+
+          expect(vulnerable[:location][:ancestors].last[:name]).to eq(direct[:name])
+          expect(secure[:location][:ancestors]).to be_nil
+        end
+      end
+    end
+  end
+
+  describe '#add_dependency' do
+    let(:dependency) { build :dependency }
 
     it 'stores given dependency params in the map' do
-      subject
+      report.add_dependency(dependency)
 
       expect(report.dependencies).to eq([dependency])
     end
@@ -44,7 +130,7 @@ RSpec.describe Gitlab::Ci::Reports::DependencyList::Report do
       expect(report.dependencies.first.dig(:vulnerabilities)).to eq(vulnerabilities)
     end
 
-    it 'updates dependency' do
+    it 'stores a dependency' do
       dependency[:packager] = 'Ruby (Bundler)'
       dependency[:vulnerabilities] = [{ name: 'abc', severity: 'high' }]
 

ee/spec/serializers/dependency_entity_spec.rb

@@ -28,7 +28,9 @@ RSpec.describe DependencyEntity do
           project.add_developer(user)
         end
 
-        it { is_expected.to eq(dependency.except(:package_manager)) }
+        it 'includes license info and vulnerabilities' do
+          is_expected.to eq(dependency.except(:package_manager, :iid))
+        end
       end
 
       context 'with reporter' do
@@ -37,7 +39,7 @@ RSpec.describe DependencyEntity do
         end
 
         it 'includes license info and not vulnerabilities' do
-          is_expected.to eq(dependency.except(:vulnerabilities, :package_manager))
+          is_expected.to eq(dependency.except(:vulnerabilities, :package_manager, :iid))
         end
       end
     end
@@ -48,7 +50,7 @@ RSpec.describe DependencyEntity do
       end
 
       it 'does not include licenses and vulnerabilities' do
-        is_expected.to eq(dependency.except(:vulnerabilities, :licenses, :package_manager))
+        is_expected.to eq(dependency.except(:vulnerabilities, :licenses, :package_manager, :iid))
       end
     end