Parse vulnerability field in addition to dependency_list

Only gemnasium DS generates dependency_list field in report
Old parser finds dependency names from dependency_list field
and adds vulnerability data based on that. However, not all analyzers
produce that field.
For example, Retire.js only reports vulnerabilities.
This change modifies parser so that it will check vulnerabilities field
in generated report to create dependency list
and populate dependencies with vulnerabilities.

ee/changelogs/unreleased/parse-vulnerability-info.yml

+---
+title: Dependency List shows only vulnerabilities from Gemnasium analyzer instead
+  of all Dependency Scanning reports
+merge_request: 32560
+author:
+type: fixed

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

@@ -11,16 +11,26 @@ module Gitlab
 
           def parse!(json_data, report)
             report_data = Gitlab::Json.parse(json_data)
+            parse_dependency_names(report_data, report)
+            parse_vulnerabilities(report_data, report)
+          end
+
+          def parse_dependency_names(report_data, report)
             report_data.fetch('dependency_files', []).each do |file|
               file['dependencies'].each do |dependency|
-                report.add_dependency(formatter.format(dependency,
-                                                       file['package_manager'],
-                                                       file['path'],
-                                                       report_data['vulnerabilities']))
+                report.add_dependency(formatter.format(dependency, file['package_manager'], file['path']))
               end
             end
           end
 
+          def parse_vulnerabilities(report_data, report)
+            report_data.fetch('vulnerabilities', []).each do |vulnerability|
+              dependency = vulnerability.dig("location", "dependency")
+              file = vulnerability.dig("location", "file")
+              report.add_dependency(formatter.format(dependency, '', file, vulnerability))
+            end
+          end
+
           def parse_licenses!(json_data, report)
             license_report = ::Gitlab::Ci::Reports::LicenseScanning::Report.parse_from(json_data)
             license_report.licenses.each do |license|

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

@@ -20,7 +20,7 @@ module Gitlab
                   path:      file_path
                 },
                 version:  dependency['version'],
-                vulnerabilities: collect_vulnerabilities(vulnerabilities, dependency, file_path),
+                vulnerabilities: formatted_vulnerabilities(vulnerabilities),
                 licenses: []
               }
             end
@@ -67,19 +67,13 @@ module Gitlab
               end
             end
 
-            def collect_vulnerabilities(vulnerabilities, dependency, file_path)
-              dependency_location = location(dependency, file_path)
+            # 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
+            def formatted_vulnerabilities(vulnerabilities)
+              return [] if vulnerabilities.blank?
 
-              vulnerabilities
-                .select { |vulnerability| vulnerability['location'] == dependency_location }
-                .map { |vulnerability| formatted_vulnerability(vulnerability) }
-            end
-
-            def formatted_vulnerability(vulnerability)
-              {
-                name: vulnerability['message'],
-                severity: vulnerability['severity'].downcase
-              }
+              [{ name: vulnerabilities['message'], severity: vulnerabilities['severity'].downcase }]
             end
           end
         end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module DependencyList
+        class Dependency
+          attr_reader :name, :packager, :package_manager, :location, :version, :licenses, :vulnerabilities
+
+          def initialize(params = {})
+            @name = params.fetch(:name)
+            @packager = params.fetch(:packager)
+            @package_manager = params.fetch(:package_manager)
+            @location = params.fetch(:location)
+            @version = params.fetch(:version)
+            @licenses = params.fetch(:licenses)
+            @vulnerabilities = unique_vulnerabilities(params.fetch(:vulnerabilities, []))
+          end
+
+          def update_dependency(dependency)
+            if self.packager.empty? && !dependency.fetch(:packager).empty?
+              @packager = dependency.fetch(:packager)
+            end
+
+            new_vulns = dependency.fetch(:vulnerabilities)
+            new_vulns.each { |v| add_vulnerability(v) }
+          end
+
+          def composite_key
+            data = [self.name, self.version, self.location.fetch(:path)].compact.join
+            Digest::SHA2.hexdigest(data)
+          end
+
+          def to_hash
+            {
+              name: self.name,
+              packager: self.packager,
+              package_manager: self.package_manager,
+              location: self.location,
+              version: self.version,
+              licenses: self.licenses,
+              vulnerabilities: self.vulnerabilities.to_a.map(&:to_hash)
+            }
+          end
+
+          private
+
+          def add_vulnerability(vulnerability)
+            return if vulnerability.empty?
+
+            @vulnerabilities.add(Vulnerability.new(vulnerability))
+          end
+
+          def unique_vulnerabilities(vulnerabilities)
+            return Set.new if vulnerabilities.empty?
+
+            unique_vulnerabilities = Set.new
+            vulnerabilities.each do |v|
+              next if v.empty?
+
+              unique_vulnerabilities.add(Vulnerability.new(v))
+            end
+
+            unique_vulnerabilities
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -5,14 +5,23 @@ module Gitlab
     module Reports
       module DependencyList
         class Report
-          attr_accessor :dependencies
-
           def initialize
-            @dependencies = []
+            @dependencies = {}
+          end
+
+          def dependencies
+            @dependencies.values.map(&:to_hash)
           end
 
           def add_dependency(dependency)
-            dependencies << dependency
+            dep = Dependency.new(dependency)
+            key = dep.composite_key
+            if @dependencies.has_key?(key)
+              existing_dependency = @dependencies[key]
+              existing_dependency.update_dependency(dependency)
+            else
+              @dependencies[key] = dep
+            end
           end
 
           def apply_license(license)

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module DependencyList
+        class Vulnerability
+          attr_reader :name, :severity
+
+          def initialize(params)
+            @name = params.fetch(:name)
+            @severity = params.fetch(:severity)
+          end
+
+          def ==(other)
+            self.to_hash == other.to_hash
+          end
+
+          def hash
+            name.hash ^ severity.hash
+          end
+
+          def to_hash
+            {
+              name: self.name,
+              severity: self.severity
+            }
+          end
+
+          alias_method :eql?, :==
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/projects/dependencies_controller_spec.rb

@@ -195,7 +195,7 @@ describe Projects::DependenciesController do
           end
         end
 
-        context 'when report doesn\'t have dependency list' do
+        context 'when report doesn\'t have dependency list field' do
           let(:user) { developer }
           let!(:pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) }
 
@@ -203,8 +203,12 @@ describe Projects::DependenciesController do
             get :index, params: params, format: :json
           end
 
-          it 'returns no_dependencies status' do
-            expect(json_response['report']['status']).to eq('no_dependencies')
+          it 'returns dependencies with vulnerabilities' do
+            expect(json_response['dependencies'].count).to eq(4)
+            django = json_response['dependencies'].find { |d| d['name'] == 'Django' }
+            expect(django).not_to be_nil
+            expect(django['vulnerabilities']).to eq([{ "name" => "Possible XSS in traceback section of technical 500 debug page", "severity" => "unknown" }])
+            expect(json_response['report']['status']).to eq('ok')
           end
         end
 

ee/spec/factories/dependencies.rb

@@ -4,8 +4,10 @@ FactoryBot.define do
   factory :dependency, class: 'Hash' do
     sequence(:name) { |n| "library#{n}" }
     packager { 'Ruby (Bundler)' }
+    package_manager { 'Ruby (Bundler)' }
     version { '1.8.0' }
     licenses { [] }
+    vulnerabilities { [] }
     sequence(:location) do |n|
       {
         blob_path: "/some_project/path/File_#{n}.lock",

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

@@ -44,11 +44,11 @@ describe Gitlab::Ci::Parsers::Security::DependencyList do
       end
     end
 
-    context 'with old dependency scanning artifact' do
+    context 'with dependency scanning artifact without dependency_list' do
       let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning) }
 
-      it 'returns empty list of dependencies' do
-        expect(report.dependencies.size).to eq(0)
+      it 'list of dependencies with vulnerabilities' do
+        expect(report.dependencies.size).to eq(4)
       end
     end
   end

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

@@ -19,7 +19,7 @@ describe Gitlab::Ci::Parsers::Security::Formatters::DependencyList do
     let(:dependency) { parsed_report['dependency_files'][0]['dependencies'][0] }
     let(:package_manager) { 'bundler' }
     let(:file_path) { 'rails/Gemfile.lock' }
-    let(:data) { formatter.format(dependency, package_manager, file_path, parsed_report['vulnerabilities']) }
+    let(:data) { formatter.format(dependency, package_manager, file_path) }
     let(:blob_path) { "/#{project.full_path}/-/blob/#{sha}/rails/Gemfile.lock" }
 
     context 'with secure dependency' do
@@ -38,15 +38,14 @@ describe Gitlab::Ci::Parsers::Security::Formatters::DependencyList do
     end
 
     context 'with vulnerable dependency' do
+      let(:data) { formatter.format(dependency, package_manager, file_path, parsed_report['vulnerabilities'].first) }
       let(:dependency) { parsed_report['dependency_files'][0]['dependencies'][1] }
 
       it 'merge vulnerabilities data' do
         vulnerabilities = data[:vulnerabilities]
 
-        expect(vulnerabilities.size).to eq(4)
-        expect(vulnerabilities[0][:name]).to eq('Vulnerabilities in libxml2 in nokogiri')
-        expect(vulnerabilities[3][:name]).to eq('Bypass of a protection mechanism in libxslt in nokogiri')
-        expect(vulnerabilities[0][:severity]).to eq('high')
+        expect(vulnerabilities.first[:name]).to eq('Vulnerabilities in libxml2 in nokogiri')
+        expect(vulnerabilities.first[:severity]).to eq('high')
       end
     end
   end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Reports::DependencyList::Dependency do
+  let(:dependency_nokogiri) { build(:dependency, :nokogiri, :with_vulnerabilities) }
+
+  context 'initialize' do
+    specify do
+      dependency_nokogiri[:location] = { blob_path: 'path/File_21.lock', path: 'File_21.lock' }
+      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' },
+                                  version: '1.8.0',
+                                  licenses: [],
+                                   vulnerabilities: [{ name: 'DDoS', severity: 'high' }, { name: 'XSS vulnerability', severity: 'low' }] })
+    end
+
+    specify do
+      dependency_nokogiri[:vulnerabilities] << { name: 'problem', severity: 'high' }
+      dep = described_class.new(dependency_nokogiri)
+
+      expect(dep.vulnerabilities.to_a.map(&:to_hash)).to eq([{ name: 'DDoS', severity: 'high' },
+                                                             { name: 'XSS vulnerability', severity: 'low' },
+                                                             { name: 'problem', severity: 'high' }])
+    end
+
+    specify do
+      dependency_nokogiri[:vulnerabilities] << { name: 'DDoS', severity: 'high' }
+      dep = described_class.new(dependency_nokogiri)
+
+      expect(dep.vulnerabilities.to_a.map(&:to_hash)).to eq([{ name: 'DDoS', severity: 'high' },
+                                                             { name: 'XSS vulnerability', severity: 'low' }])
+    end
+  end
+
+  context 'update dependency' do
+    specify do
+      dependency_nokogiri[:vulnerabilities] << { name: 'DDoS', severity: 'high' } << { name: 'problem', severity: 'high' }
+      dep = described_class.new(dependency_nokogiri)
+
+      expect(dep.vulnerabilities.to_a.map(&:to_hash)).to eq([{ name: 'DDoS', severity: 'high' },
+                                                             { name: 'XSS vulnerability', severity: 'low' },
+                                                             { name: 'problem', severity: 'high' }])
+    end
+  end
+end

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

 # frozen_string_literal: true
 
 require 'spec_helper'
+require 'benchmark/ips'
 
 describe Gitlab::Ci::Reports::DependencyList::Report do
   let(:report) { described_class.new }
 
   describe '#add_dependency' do
-    let(:dependency) { { name: 'gitlab', version: '12.0' } }
+    let(:dependency) do
+      {
+        name: 'gitlab',
+        packager: '',
+        package_manager: 'bundler',
+        location: { blob_path: '', path: 'Gemfile' },
+        version: '0.2.10',
+        vulnerabilities: [],
+        licenses: []
+      }
+    end
 
     subject { report.add_dependency(dependency) }
 
@@ -15,6 +26,33 @@ describe Gitlab::Ci::Reports::DependencyList::Report do
 
       expect(report.dependencies).to eq([dependency])
     end
+
+    it 'does not duplicate same dependency' do
+      report.add_dependency(dependency)
+      report.add_dependency(dependency.dup)
+
+      expect(report.dependencies.first).to eq(dependency)
+    end
+
+    it 'does not duplicate same vulnerability for dependency' do
+      vulnerabilities = [{ name: 'problem', severity: 'high' }, { name: 'problem2', severity: 'medium' }]
+      dependency[:vulnerabilities] = [vulnerabilities.first]
+      with_extra_vuln_from_another_report = dependency.dup.merge(vulnerabilities: vulnerabilities)
+
+      report.add_dependency(dependency)
+      report.add_dependency(with_extra_vuln_from_another_report)
+      expect(report.dependencies.first.dig(:vulnerabilities)).to eq(vulnerabilities)
+    end
+
+    it 'updates dependency' do
+      dependency[:packager] = 'Ruby (Bundler)'
+      dependency[:vulnerabilities] = [{ name: 'abc', severity: 'high' }]
+
+      report.add_dependency(dependency)
+      expect(report.dependencies.size).to eq(1)
+      expect(report.dependencies.first[:packager]).to eq('Ruby (Bundler)')
+      expect(report.dependencies.first[:vulnerabilities]).to eq([{ name: 'abc', severity: 'high' }])
+    end
   end
 
   describe '#apply_license' do

ee/spec/models/ci/build_spec.rb

@@ -316,6 +316,28 @@ describe Ci::Build do
       end
     end
 
+    context 'with different report format' do
+      let!(:dl_artifact) { create(:ee_ci_job_artifact, :dependency_scanning, job: job, project: job.project) }
+      let(:dependency_list_report) { Gitlab::Ci::Reports::DependencyList::Report.new }
+
+      before do
+        stub_licensed_features(dependency_scanning: true)
+      end
+
+      subject { job.collect_dependency_list_reports!(dependency_list_report) }
+
+      it 'parses blobs and add the results to the report' do
+        subject
+        blob_path = "/#{project.full_path}/-/blob/#{job.sha}/sast-sample-rails/Gemfile.lock"
+        netty = dependency_list_report.dependencies.first
+        ffi = dependency_list_report.dependencies.last
+
+        expect(dependency_list_report.dependencies.count).to eq(4)
+        expect(netty[:name]).to eq('io.netty/netty')
+        expect(ffi[:location][:blob_path]).to eq(blob_path)
+      end
+    end
+
     context 'with disabled licensed feature' do
       it 'does NOT parse dependency list report' do
         subject

ee/spec/models/ci/pipeline_spec.rb

@@ -288,17 +288,21 @@ describe Ci::Pipeline do
 
     context 'when pipeline has a build with dependency list reports' do
       let!(:build) { create(:ee_ci_build, :success, :dependency_list, pipeline: pipeline, project: project) }
+      let!(:build1) { create(:ee_ci_build, :success, :dependency_scanning, pipeline: pipeline, project: project) }
       let!(:build2) { create(:ee_ci_build, :success, :license_scanning, pipeline: pipeline, project: project) }
 
       it 'returns a dependency list report with collected data' do
-        expect(subject.dependencies.count).to eq(21)
-        expect(subject.dependencies[0][:name]).to eq('mini_portile2')
-        expect(subject.dependencies[0][:licenses]).not_to be_empty
+        mini_portile2 = subject.dependencies.find { |x| x[:name] == 'mini_portile2' }
+
+        expect(subject.dependencies.count).to eq(24)
+        expect(mini_portile2[:name]).not_to be_empty
+        expect(mini_portile2[:licenses]).not_to be_empty
       end
 
       context 'when builds are retried' do
         before do
           build.update(retried: true)
+          build1.update(retried: true)
         end
 
         it 'does not take retried builds into account' do

ee/spec/serializers/dependency_entity_spec.rb

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