Finding builder errors should be recoverable

Make errors thrown by finding builders recoverable in the vulnerability creation service

Changelog: fixed

ee/app/services/security/vulnerability_scanning/create_vulnerability_service.rb

@@ -64,18 +64,20 @@ def finding_for_affected_component(affected_component)
           advisory: advisory,
           affected_component: affected_component
         ).finding
+      rescue ::Gitlab::VulnerabilityScanning::FindingBuilder::Error => error
+        process_recoverable_error(error, affected_component.project.id)
+        nil
       end
 
       def finding_maps
         maps = affected_components.filter_map do |affected_component|
-          if affected_component.pipeline.user.nil?
-            track_missing_author_error_for_project_id(affected_component.project.id)
-            next
-          end
-
-          track_upsert_for_project_id(affected_component.project.id)
           pipeline = affected_component.pipeline
+
           finding = finding_for_affected_component(affected_component)
+          next if finding.nil?
+
+          track_upsert_for_project_id(affected_component.project.id)
+
           scanner_id = scanner_for_project(affected_component.project).id
           FindingMap.new(pipeline: pipeline, report_finding: finding, scanner_id: scanner_id)
         end
@@ -92,10 +94,10 @@ def track_upsert_for_project_id(project_id)
         project_ids_with_upsert.add(project_id)
       end
 
-      def track_missing_author_error_for_project_id(project_id)
+      def process_recoverable_error(error, project_id)
         ::Gitlab::ErrorTracking.track_exception(
-          ArgumentError.new('Pipeline must have a corresponding user to use as vulnerability author'),
-          message: 'Skipping vulnerability creation for project',
+          error,
+          message: 'Skipping vulnerability creation for an affected component',
           project_id: project_id,
           advisory_xid: advisory.xid)
 

ee/lib/gitlab/vulnerability_scanning/container_scanning/finding_builder.rb

@@ -6,14 +6,14 @@ module ContainerScanning
       class FindingBuilder < VulnerabilityScanning::FindingBuilder
         include Gitlab::Utils::StrongMemoize
 
-        MissingPropertiesError = Class.new(StandardError)
-
         private
 
         def validate!
+          super
+
           return unless sbom_source.image_name.nil? || sbom_source.image_tag.nil?
 
-          raise MissingPropertiesError,
+          raise Gitlab::VulnerabilityScanning::FindingBuilder::MissingPropertiesError,
             'Missing required gitlab:container_scanning CycloneDX properties'
         end
 

ee/lib/gitlab/vulnerability_scanning/dependency_scanning/finding_builder.rb

@@ -6,14 +6,14 @@ module DependencyScanning
       class FindingBuilder < VulnerabilityScanning::FindingBuilder
         include Gitlab::Utils::StrongMemoize
 
-        MissingPropertiesError = Class.new(StandardError)
-
         private
 
         def validate!
+          super
+
           return unless input_file.nil?
 
-          raise MissingPropertiesError,
+          raise Gitlab::VulnerabilityScanning::FindingBuilder::MissingPropertiesError,
             'Missing required gitlab:dependency_scanning CycloneDX properties'
         end
 

ee/lib/gitlab/vulnerability_scanning/finding_builder.rb

@@ -3,6 +3,10 @@
 module Gitlab
   module VulnerabilityScanning
     class FindingBuilder
+      Error = Class.new(StandardError)
+      ArgumentError = Class.new(Error)
+      MissingPropertiesError = Class.new(Error)
+
       include Gitlab::Utils::StrongMemoize
 
       def self.for_report_type(report_type)
@@ -77,7 +81,11 @@ def finding
       attr_reader :project, :pipeline, :sbom_source, :scanner, :advisory, :affected_component
 
       def validate!
-        raise NoMethodError, "#{self.class}#validate! is not implemented"
+        raise ArgumentError, 'Missing sbom source argument' if sbom_source.nil?
+
+        return unless pipeline.user.nil?
+
+        raise ArgumentError, 'Pipeline must have a corresponding user to use as vulnerability author'
       end
 
       def report_type

ee/spec/lib/gitlab/vulnerability_scanning/container_scanning/finding_builder_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe Gitlab::VulnerabilityScanning::ContainerScanning::FindingBuilder, feature_category: :software_composition_analysis do
   let(:now) { Time.zone.now }
-  let(:ci_build) { build(:ci_build) }
+  let(:ci_build) { build(:ci_build, pipeline: build(:ci_pipeline, user: build(:user))) }
   let(:sbom_source) { build(:ci_reports_sbom_source) }
   let(:security_scanner) { Gitlab::VulnerabilityScanning::SecurityScanner.fabricate }
 
@@ -104,7 +104,7 @@
         it "adds an error to the generated report" do
           expect do
             builder.finding
-          end.to raise_error(described_class::MissingPropertiesError,
+          end.to raise_error(Gitlab::VulnerabilityScanning::FindingBuilder::MissingPropertiesError,
             "Missing required gitlab:container_scanning CycloneDX properties")
         end
       end

ee/spec/lib/gitlab/vulnerability_scanning/dependency_scanning/finding_builder_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe Gitlab::VulnerabilityScanning::DependencyScanning::FindingBuilder, feature_category: :software_composition_analysis do
   let(:now) { Time.zone.now }
-  let(:ci_build) { build(:ci_build) }
+  let(:ci_build) { build(:ci_build, pipeline: build(:ci_pipeline, user: build(:user))) }
   let(:sbom_source) { build(:ci_reports_sbom_source) }
   let(:security_scanner) { Gitlab::VulnerabilityScanning::SecurityScanner.fabricate }
 
@@ -90,7 +90,7 @@
         expect do
           builder.finding
         end.to raise_error(
-          Gitlab::VulnerabilityScanning::DependencyScanning::FindingBuilder::MissingPropertiesError,
+          Gitlab::VulnerabilityScanning::FindingBuilder::MissingPropertiesError,
           "Missing required gitlab:dependency_scanning CycloneDX properties")
       end
     end

ee/spec/lib/gitlab/vulnerability_scanning/finding_builder_spec.rb

@@ -3,7 +3,6 @@
 require "spec_helper"
 
 RSpec.describe Gitlab::VulnerabilityScanning::FindingBuilder, feature_category: :software_composition_analysis do
-  let_it_be(:pipeline) { build(:ci_pipeline) }
   let_it_be(:sbom_source) { build(:ci_reports_sbom_source) }
   let_it_be(:scanner) { Gitlab::VulnerabilityScanning::SecurityScanner.fabricate }
   let_it_be(:location) { build(:ci_reports_security_locations_sast) }
@@ -25,6 +24,9 @@
 
   let_it_be(:affected_component) { build(:vs_possibly_affected_component) }
 
+  let(:user) { build(:user) }
+  let(:pipeline) { build(:ci_pipeline, user: user) }
+
   subject(:builder) do
     described_class.new(project: pipeline.project, pipeline: pipeline,
       sbom_source: sbom_source, scanner: scanner, advisory: advisory, affected_component: affected_component)
@@ -32,17 +34,8 @@
 
   describe "#finding" do
     context "when abstract methods have not been implemented" do
-      context "when validation has not been implemented" do
-        before do
-          allow(builder).to receive(:report_type).and_return("dependency_scanning")
-        end
-
-        it { expect { builder.finding }.to raise_error(NoMethodError, /#validate! is not implemented/) }
-      end
-
       context "when report_type has not been implemented" do
         before do
-          allow(builder).to receive(:validate!).and_return(nil)
           allow(builder).to receive(:location_fingerprint).and_return("01234567890abcdef")
           allow(builder).to receive(:location).and_return(location)
           allow(builder).to receive(:original_data).and_return({})
@@ -53,7 +46,6 @@
 
       context "when location has not been implemented" do
         before do
-          allow(builder).to receive(:validate!).and_return(nil)
           allow(builder).to receive(:report_type).and_return("dependency_scanning")
         end
 
@@ -62,7 +54,6 @@
 
       context "when original_data has not been implemented" do
         before do
-          allow(builder).to receive(:validate!).and_return(nil)
           allow(builder).to receive(:report_type).and_return("dependency_scanning")
           allow(builder).to receive(:location).and_return(location)
         end
@@ -83,7 +74,6 @@
     context "when creating uuid" do
       before do
         allow(Gitlab::AppJsonLogger).to receive(:warn).and_call_original
-        allow(builder).to receive(:validate!).and_return(nil)
         allow(builder).to receive(:report_type).and_return("dependency_scanning")
         allow(builder).to receive(:location_fingerprint).and_return("01234567890abcdef")
         allow(builder).to receive(:location).and_return(location)
@@ -117,7 +107,6 @@
 
     context "when creating finding name" do
       before do
-        allow(builder).to receive(:validate!).and_return(nil)
         allow(builder).to receive(:report_type).and_return("dependency_scanning")
         allow(builder).to receive(:original_data).and_return({})
       end
@@ -138,6 +127,33 @@
         it { expect(builder.finding.name).to eq("CVE-2018-1000538") }
       end
     end
+
+    context 'when invalid arguments' do
+      before do
+        allow(builder).to receive(:report_type).and_return("dependency_scanning")
+        allow(builder).to receive(:location).and_return(nil)
+        allow(builder).to receive(:original_data).and_return({})
+      end
+
+      context 'if no sbom source' do
+        let_it_be(:sbom_source) { nil }
+
+        specify do
+          expect { builder.finding }.to raise_error(described_class::ArgumentError, 'Missing sbom source argument')
+        end
+      end
+
+      context 'if no pipeline author' do
+        let(:user) { nil }
+
+        specify do
+          expect do
+            builder.finding
+          end.to raise_error(described_class::ArgumentError,
+            'Pipeline must have a corresponding user to use as vulnerability author')
+        end
+      end
+    end
   end
 
   describe '.for_report_type' do

ee/spec/lib/gitlab/vulnerability_scanning/security_report_builder_spec.rb

@@ -5,7 +5,7 @@
 RSpec.describe Gitlab::VulnerabilityScanning::SecurityReportBuilder, feature_category: :software_composition_analysis do
   let(:sbom) { build(:ci_reports_sbom_report, source: sbom_source) }
 
-  let_it_be(:ci_build) { build(:ci_build) }
+  let_it_be(:ci_build) { build(:ci_build, pipeline: build(:ci_pipeline, user: build(:user))) }
 
   let(:expected) { Gitlab::Ci::Reports::Security::Report.new(report_type, ci_build.pipeline, created_at) }
   let(:created_at) { Time.zone.now }

ee/spec/services/security/vulnerability_scanning/create_vulnerability_service_spec.rb

@@ -8,7 +8,8 @@
   let(:sbom_source) { build(:ci_reports_sbom_source) }
   let(:affected_components) do
     [
-      build(:vs_possibly_affected_component, purl_type: 'npm', pipeline: pipeline, project: pipeline.project)
+      build(:vs_possibly_affected_component, purl_type: 'npm', pipeline: pipeline, project: pipeline.project,
+        source: sbom_source)
     ]
   end
 
@@ -69,32 +70,55 @@
       end
     end
 
-    context 'when the pipeline has no associated user' do
-      let_it_be(:pipeline) { create(:ci_pipeline) }
-
-      it 'fails to create a new vulnerability' do
-        expect(Gitlab::ErrorTracking)
-          .to receive(:track_exception)
-          .with(ArgumentError.new('Pipeline must have a corresponding user to use as vulnerability author'),
-            message: 'Skipping vulnerability creation for project',
-            project_id: pipeline.project.id,
-            advisory_xid: advisory.xid)
-
-        expect(Gitlab::ErrorTracking)
-          .to receive(:track_exception)
-          .with(anything,
-            message: 'Continuous vulnerability scanning failed to create vulnerabilities',
-            project_ids_with_upsert: [],
-            project_ids_with_error: [pipeline.project.id],
-            advisory_xid: advisory.xid).once
+    context 'when an error is thrown' do
+      context 'and it is not recoverable' do
+        it 'captures the error and halts execution' do
+          allow_next_instance_of(::Gitlab::VulnerabilityScanning::FindingBuilder) do |instance|
+            allow(instance).to receive(:finding).and_raise(StandardError)
+          end
 
-        expect(service_response.success?).to be(false)
-        expect(service_response.payload[:vulnerability_ids]).to be_nil
-        expect(service_response.payload[:error]).not_to be_nil
-        expect(service_response.payload[:error]).to be_a_kind_of(StandardError)
-        expect(service_response.payload[:error].message).to eq(
-          'No vulnerability scanning finding maps could be created'
-        )
+          expect(Gitlab::ErrorTracking).to receive(:track_exception)
+            .with(an_instance_of(StandardError),
+              message: "Continuous vulnerability scanning failed to create vulnerabilities",
+              project_ids_with_upsert: [],
+              project_ids_with_error: [],
+              advisory_xid: advisory.xid)
+
+          expect(service_response.success?).to be(false)
+          expect(service_response.message).to eq('Vulnerabilities were not created')
+          expect(service_response.payload[:vulnerability_ids]).to be_nil
+          expect(service_response.payload[:error]).not_to be_nil
+          expect(service_response.payload[:error]).to be_a_kind_of(StandardError)
+          expect(service_response.payload[:error]).to be_kind_of(StandardError)
+        end
+      end
+
+      context 'and it is a recoverable finding builder error' do
+        let(:valid_component) do
+          build(:vs_possibly_affected_component, purl_type: 'npm', pipeline: pipeline, project: pipeline.project,
+            source: sbom_source)
+        end
+
+        let(:invalid_source_component) do
+          build(:vs_possibly_affected_component, purl_type: 'npm', pipeline: pipeline, project: pipeline.project,
+            source: nil)
+        end
+
+        let(:affected_components) { [valid_component, invalid_source_component] }
+
+        it 'captures and tracks the error' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception)
+                  .with(an_instance_of(::Gitlab::VulnerabilityScanning::FindingBuilder::ArgumentError),
+                    message: 'Skipping vulnerability creation for an affected component',
+                    project_id: pipeline.project.id,
+                    advisory_xid: advisory.xid)
+
+          expect(service_response.success?).to be(true)
+          expect(service_response.payload[:vulnerability_ids].size).to eq(1)
+          expect(service_response.payload[:project_ids_with_upsert]).to eq([pipeline.project.id])
+          expect(service_response.payload[:project_ids_with_error]).to eq([pipeline.project.id])
+          expect(service_response.payload[:error]).to be_nil
+        end
       end
     end
   end