Merge branch '229472-parse-dep-path' into 'master'

Parse dependency path

See merge request gitlab-org/gitlab!42596

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