Merge branch '32765-vuln-check-approvals-required' into 'master'

Resolve sentry errors related to parsing security reports

See merge request gitlab-org/gitlab!18423

ee/app/services/security/sync_reports_to_approval_rules_service.rb

@@ -37,12 +37,13 @@ module Security
     end
 
     def sync_vulnerability_rules
-      reports = pipeline.security_reports.reports
-      safe = reports.any? && reports.none? do |_report_type, report|
-        report.unsafe_severity?
-      end
+      reports = pipeline.security_reports
+      # If we have some reports, then we want to sync them early;
+      # If we don't have reports, then we should wait until pipeline stops.
+      return if reports.empty? && !pipeline.complete?
+      return if reports.violates_default_policy?
 
-      remove_required_approvals_for(ApprovalMergeRequestRule.security_report) if safe
+      remove_required_approvals_for(ApprovalMergeRequestRule.security_report)
     end
 
     def remove_required_approvals_for(rules)

ee/changelogs/unreleased/32765-vuln-check-approvals-required.yml

+---
+title: Add default empty values to prevent parser errors from approving the Vulnerability-Check rule
+merge_request: 18423
+author:
+type: fixed

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

@@ -29,7 +29,7 @@ module Gitlab
 
           # map remediations to relevant vulnerabilities
           def collate_remediations(report_data)
-            return report_data["vulnerabilities"] unless report_data["remediations"]
+            return report_data["vulnerabilities"] || [] unless report_data["remediations"]
 
             report_data["vulnerabilities"].map do |vulnerability|
               # Grab the first available remediation.
@@ -49,8 +49,8 @@ module Gitlab
                 uuid: SecureRandom.uuid,
                 report_type: report.type,
                 name: data['message'],
-                compare_key: data['cve'],
-                location: create_location(data['location']),
+                compare_key: data['cve'] || '',
+                location: create_location(data['location'] || {}),
                 severity: parse_level(data['severity']),
                 confidence: parse_level(data['confidence']),
                 scanner: scanner,

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

@@ -54,7 +54,12 @@ module Gitlab
           end
 
           def unsafe_severity?
-            occurrences.any? { |occurrence| UNSAFE_SEVERITIES.include?(occurrence.severity) }
+            !safe?
+          end
+
+          def safe?
+            severities = occurrences.map(&:severity).compact.map(&:downcase)
+            (severities & UNSAFE_SEVERITIES).empty?
           end
         end
       end

ee/lib/gitlab/ci/reports/security/reports.rb

@@ -7,6 +7,8 @@ module Gitlab
         class Reports
           attr_reader :reports, :commit_sha
 
+          delegate :empty?, to: :reports
+
           def initialize(commit_sha)
             @reports = {}
             @commit_sha = commit_sha
@@ -15,6 +17,10 @@ module Gitlab
           def get_report(report_type)
             reports[report_type] ||= Report.new(report_type, commit_sha)
           end
+
+          def violates_default_policy?
+            reports.values.any? { |report| report.unsafe_severity? }
+          end
         end
       end
     end

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

@@ -49,6 +49,26 @@ describe Gitlab::Ci::Parsers::Security::DependencyScanning do
       end
     end
 
+    context "when parsing a vulnerability with a missing location" do
+      let(:report_hash) { JSON.parse(fixture_file('security_reports/master/gl-sast-report.json', dir: 'ee'), symbolize_names: true) }
+
+      before do
+        report_hash[:vulnerabilities][0][:location] = nil
+      end
+
+      it { expect { parser.parse!(report_hash.to_json, report) }.not_to raise_error }
+    end
+
+    context "when parsing a vulnerability with a missing cve" do
+      let(:report_hash) { JSON.parse(fixture_file('security_reports/master/gl-sast-report.json', dir: 'ee'), symbolize_names: true) }
+
+      before do
+        report_hash[:vulnerabilities][0][:cve] = nil
+      end
+
+      it { expect { parser.parse!(report_hash.to_json, report) }.not_to raise_error }
+    end
+
     context "when vulnerabilities have remediations" do
       let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning_remediation) }
 

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

@@ -4,44 +4,53 @@ require 'spec_helper'
 
 describe Gitlab::Ci::Parsers::Security::Sast do
   describe '#parse!' do
-    let(:project) { artifact.project }
-    let(:pipeline) { artifact.job.pipeline }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha) }
-    let(:parser) { described_class.new }
+    subject(:parser) { described_class.new }
 
-    where(report_format: %i(sast sast_deprecated))
+    let(:commit_sha) { "d8978e74745e18ce44d88814004d4255ac6a65bb" }
 
-    with_them do
-      let(:artifact) { create(:ee_ci_job_artifact, report_format) }
+    context "when parsing valid reports" do
+      where(report_format: %i(sast sast_deprecated))
 
-      before do
-        artifact.each_blob do |blob|
-          parser.parse!(blob, report)
+      with_them do
+        let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, commit_sha) }
+        let(:artifact) { create(:ee_ci_job_artifact, report_format) }
+
+        before do
+          artifact.each_blob do |blob|
+            parser.parse!(blob, report)
+          end
         end
-      end
 
-      it "parses all identifiers and occurrences" do
-        expect(report.occurrences.length).to eq(33)
-        expect(report.identifiers.length).to eq(17)
-        expect(report.scanners.length).to eq(3)
-      end
+        it "parses all identifiers and occurrences" do
+          expect(report.occurrences.length).to eq(33)
+          expect(report.identifiers.length).to eq(17)
+          expect(report.scanners.length).to eq(3)
+        end
 
-      it 'generates expected location' do
-        location = report.occurrences.first.location
-
-        expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::Sast)
-        expect(location).to have_attributes(
-          file_path: 'python/hardcoded/hardcoded-tmp.py',
-          start_line: 1,
-          end_line: 1,
-          class_name: nil,
-          method_name: nil
-        )
-      end
+        it 'generates expected location' do
+          location = report.occurrences.first.location
+
+          expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::Sast)
+          expect(location).to have_attributes(
+            file_path: 'python/hardcoded/hardcoded-tmp.py',
+            start_line: 1,
+            end_line: 1,
+            class_name: nil,
+            method_name: nil
+          )
+        end
 
-      it "generates expected metadata_version" do
-        expect(report.occurrences.first.metadata_version).to eq('1.2')
+        it "generates expected metadata_version" do
+          expect(report.occurrences.first.metadata_version).to eq('1.2')
+        end
       end
     end
+
+    context "when parsing an empty report" do
+      let(:report) { Gitlab::Ci::Reports::Security::Report.new('sast', commit_sha) }
+      let(:blob) { JSON.generate({}) }
+
+      it { expect(parser.parse!(blob, report)).to be_empty }
+    end
   end
 end

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

@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Reports::Security::Report do
-  let(:pipeline) { create(:ci_pipeline) }
-  let(:report) { described_class.new('sast', pipeline.sha) }
+  let(:report) { described_class.new('sast', commit_sha) }
+  let(:commit_sha) { "d8978e74745e18ce44d88814004d4255ac6a65bb" }
 
   it { expect(report.type).to eq('sast') }
 
@@ -111,7 +111,7 @@ describe Gitlab::Ci::Reports::Security::Report do
       allow(report).to receive(:replace_with!)
     end
 
-    subject { report.merge!(described_class.new('sast', pipeline.sha)) }
+    subject { report.merge!(described_class.new('sast', commit_sha)) }
 
     it 'invokes the merge with other report and then replaces this report contents by merge result' do
       subject
@@ -119,4 +119,63 @@ describe Gitlab::Ci::Reports::Security::Report do
       expect(report).to have_received(:replace_with!).with(merged_report)
     end
   end
+
+  describe "#safe?" do
+    subject { described_class.new('sast', commit_sha) }
+
+    context "when the sast report has an unsafe vulnerability" do
+      where(severity: %w[unknown Unknown high High critical Critical])
+      with_them do
+        let(:occurrence) { build(:ci_reports_security_occurrence, severity: severity) }
+
+        before do
+          subject.add_occurrence(occurrence)
+        end
+
+        it { expect(subject.unsafe_severity?).to be(true) }
+        it { expect(subject).not_to be_safe }
+      end
+    end
+
+    context "when the sast report has a medium to low severity vulnerability" do
+      where(severity: %w[medium Medium low Low])
+      with_them do
+        let(:occurrence) { build(:ci_reports_security_occurrence, severity: severity) }
+
+        before do
+          subject.add_occurrence(occurrence)
+        end
+
+        it { expect(subject.unsafe_severity?).to be(false) }
+        it { expect(subject).to be_safe }
+      end
+    end
+
+    context "when the sast report has a vulnerability with a `nil` severity" do
+      let(:occurrence) { build(:ci_reports_security_occurrence, severity: nil) }
+
+      before do
+        subject.add_occurrence(occurrence)
+      end
+
+      it { expect(subject.unsafe_severity?).to be(false) }
+      it { expect(subject).to be_safe }
+    end
+
+    context "when the sast report has a vulnerability with a blank severity" do
+      let(:occurrence) { build(:ci_reports_security_occurrence, severity: '') }
+
+      before do
+        subject.add_occurrence(occurrence)
+      end
+
+      it { expect(subject.unsafe_severity?).to be(false) }
+      it { expect(subject).to be_safe }
+    end
+
+    context "when the sast report has zero vulnerabilities" do
+      it { expect(subject.unsafe_severity?).to be(false) }
+      it { expect(subject).to be_safe }
+    end
+  end
 end

ee/spec/lib/gitlab/ci/reports/security/reports_spec.rb

@@ -35,4 +35,29 @@ describe Gitlab::Ci::Reports::Security::Reports do
       end
     end
   end
+
+  describe "#violates_default_policy?" do
+    subject { described_class.new(commit_sha) }
+
+    let(:low_severity) { build(:ci_reports_security_occurrence, severity: 'low') }
+    let(:high_severity) { build(:ci_reports_security_occurrence, severity: 'high') }
+
+    context "when a report has a high severity vulnerability" do
+      before do
+        subject.get_report('sast').add_occurrence(high_severity)
+        subject.get_report('dependency_scanning').add_occurrence(low_severity)
+      end
+
+      it { expect(subject.violates_default_policy?).to be(true) }
+    end
+
+    context "when none of the reports have a high severity vulnerability" do
+      before do
+        subject.get_report('sast').add_occurrence(low_severity)
+        subject.get_report('dependency_scanning').add_occurrence(low_severity)
+      end
+
+      it { expect(subject.violates_default_policy?).to be(false) }
+    end
+  end
 end

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

@@ -131,13 +131,14 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
   end
 
   context 'without reports' do
+    let(:pipeline) { create(:ci_pipeline, :running, project: project, merge_requests_as_head_pipeline: [merge_request]) }
+
     it "won't change approvals_required count" do
       expect { subject }
         .not_to change { report_approver_rule.reload.approvals_required }
     end
 
     context "license compliance policy" do
-      let(:pipeline) { create(:ee_ci_pipeline, :running, project: project, merge_requests_as_head_pipeline: [merge_request]) }
       let!(:software_license_policy) { create(:software_license_policy, :blacklist, project: project, software_license: blacklisted_license) }
       let!(:license_compliance_rule) { create(:report_approver_rule, :license_management, merge_request: merge_request, approvals_required: 1) }
       let!(:blacklisted_license) { create(:software_license) }