Merge branch 'parse-vulnerability-info' into 'master'

Update dependency list parser to parse vulnerability field in addition do dependency_list field

See merge request gitlab-org/gitlab!32560

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,14 +11,24 @@ 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)

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