Merge branch 'remove_cyclonedx_related_findings_from_finding_map' into 'master'

Remove cyclonedx related findings from

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180470



Merged-by: Michael Becker <mbecker@gitlab.com>
Approved-by: Oscar Tovar <otovar@gitlab.com>
Approved-by: Michael Becker <mbecker@gitlab.com>
Reviewed-by: Oscar Tovar <otovar@gitlab.com>
Co-authored-by: Zamir Martins Filho <zfilho@gitlab.com>

ee/app/models/security/finding.rb

@@ -149,6 +149,10 @@ class Finding < ::Gitlab::Database::SecApplicationRecord
       )
     end
 
+    scope :except_scanners, ->(scanners) do
+      where.not(scanner: scanners)
+    end
+
     delegate :scan_type, :pipeline, :remediations_proxy, to: :scan, allow_nil: true
     delegate :project, :sha, to: :pipeline
 

ee/app/models/vulnerabilities/scanner.rb

@@ -29,6 +29,10 @@ class Scanner < Gitlab::Database::SecApplicationRecord
         .order('"vulnerability_scanners"."external_id" ASC, "report_types"."report_type" ASC')
     end
 
+    def self.sbom_scanner(project_id:)
+      find_by(external_id:  Gitlab::VulnerabilityScanning::SecurityScanner::EXTERNAL_ID, project_id: project_id)
+    end
+
     def report_type
       self[:report_type] || scan_type
     end

ee/app/services/security/ingestion/finding_map_collection.rb

@@ -22,7 +22,7 @@ def each
 
       attr_reader :pipeline, :security_scan
 
-      delegate :findings, :report_findings, to: :security_scan, private: true
+      delegate :findings, :report_findings, :project, to: :security_scan, private: true
 
       def create_finding_map_for(security_finding)
         # For SAST findings, we override the finding UUID with an existing finding UUID
@@ -41,9 +41,20 @@ def report_findings_map
       # We are also sorting by `uuid` to prevent having deadlock errors while
       # ingesting the findings.
       def deduplicated_findings
-        @deduplicated_findings ||= findings.deduplicated.sort do |a, b|
-          [b.overridden_uuid.to_s, b.uuid] <=> [a.overridden_uuid.to_s, a.uuid]
-        end
+        @deduplicated_findings ||= if ::Feature.enabled?(:dependency_scanning_for_pipelines_with_cyclonedx_reports,
+          project)
+                                     findings.deduplicated.except_scanners(sbom_scanner).sort do |a, b|
+                                       [b.overridden_uuid.to_s, b.uuid] <=> [a.overridden_uuid.to_s, a.uuid]
+                                     end
+                                   else
+                                     findings.deduplicated.sort do |a, b|
+                                       [b.overridden_uuid.to_s, b.uuid] <=> [a.overridden_uuid.to_s, a.uuid]
+                                     end
+                                   end
+      end
+
+      def sbom_scanner
+        @sbom_scanner ||= Vulnerabilities::Scanner.sbom_scanner(project_id: project.id)
       end
     end
   end

ee/spec/models/security/finding_spec.rb

@@ -411,6 +411,12 @@
     end
   end
 
+  describe '.except_scanners' do
+    it 'returns findings except the ones associated to scanners as parameter' do
+      expect(described_class.except_scanners(finding_1.scanner)).to contain_exactly(finding_2)
+    end
+  end
+
   describe '#state' do
     subject { finding_1.state }
 

ee/spec/models/vulnerabilities/scanner_spec.rb

@@ -89,4 +89,14 @@
       let_it_be(:model) { create(:vulnerabilities_scanner, project: parent) }
     end
   end
+
+  describe '.sbom_scanner' do
+    let_it_be(:sbom_scanner) { create(:vulnerabilities_scanner, :sbom_scanner) }
+    let_it_be(:sbom_scanner_2) { create(:vulnerabilities_scanner, :sbom_scanner) }
+
+    it 'returns the sbom record related to project' do
+      expect(described_class.sbom_scanner(project_id: sbom_scanner.project_id)).to have_attributes(id: sbom_scanner.id,
+        project_id: sbom_scanner.project_id)
+    end
+  end
 end

ee/spec/services/security/ingestion/finding_map_collection_spec.rb

@@ -43,6 +43,48 @@
         expect(finding_maps).to have_received(:concat).exactly(3).times
         expect(finding_pairs).to eq(expected_finding_pairs)
       end
+
+      context 'with a cyclonedx related security finding' do
+        let_it_be(:sbom_scanner) { create(:vulnerabilities_scanner, :sbom_scanner, project: security_scan.project) }
+        let_it_be(:cyclonedx_finding) do
+          create(
+            :security_finding,
+            :with_finding_data,
+            deduplicated: true,
+            scan: security_scan,
+            scanner: sbom_scanner
+          )
+        end
+
+        it 'does not include cyclonedx related security findings' do
+          run_each_slice
+
+          expect(finding_maps).to have_received(:concat).exactly(3).times
+          expect(finding_pairs).to eq(expected_finding_pairs)
+        end
+
+        context 'with dependency_scanning_for_pipelines_with_cyclonedx_reports FF disabled' do
+          let(:expected_finding_pairs) do
+            [
+              [security_finding_3, report_findings[2]],
+              [security_finding_1, report_findings[0]],
+              [security_finding_2, report_findings[1]],
+              [cyclonedx_finding, nil]
+            ]
+          end
+
+          before do
+            stub_feature_flags(dependency_scanning_for_pipelines_with_cyclonedx_reports: false)
+          end
+
+          it 'includes cyclonedx related security findings' do
+            run_each_slice
+
+            expect(finding_maps).to have_received(:concat).exactly(4).times
+            expect(finding_pairs).to match_array(expected_finding_pairs)
+          end
+        end
+      end
     end
   end
 end

ee/spec/workers/security/store_security_reports_by_project_worker_spec.rb

@@ -331,6 +331,82 @@
         expect(project.vulnerabilities.map(&:resolved_on_default_branch)).not_to include(true)
       end
     end
+
+    context 'with report related findings matching all security findings' do
+      let(:sast_scanner) { create(:vulnerabilities_scanner, project: project) }
+      let(:sbom_scanner) { create(:vulnerabilities_scanner, :sbom_scanner, project: project) }
+      let(:scan_sast) do
+        create(:security_scan, :latest_successful, scan_type: :sast,
+          project: project, build: build_sast)
+      end
+
+      let(:build_sast) do
+        create(
+          :ci_build, :sast, :success,
+          user: project.creator, pipeline: pipeline, project: project)
+      end
+
+      let(:artifact_sast1) do
+        create(:ee_ci_job_artifact, :semgrep_api_vulnerabilities, job: build_sast)
+      end
+
+      before do
+        allow_next_instance_of(Security::Scan) do |instance|
+          allow(instance).to receive(:scanner).and_return(sast_scanner)
+        end
+        stub_licensed_features(
+          sast: true,
+          vulnerability_finding_signatures: true
+        )
+        update_cache(pipeline)
+        scan_sast.reload.report_findings.each do |finding|
+          create(:security_finding, deduplicated: true,
+            scan: scan_sast, scanner: sast_scanner, uuid: finding.uuid)
+        end
+      end
+
+      it 'does not generate errors' do
+        expect { worker.perform(project.id) }.not_to change {
+          scan_sast.reload.info
+        }
+      end
+
+      context 'with an additional cyclonedx related security finding' do
+        let!(:cyclonedx_finding) do
+          create(
+            :security_finding,
+            :with_finding_data,
+            deduplicated: true,
+            scan: scan_sast,
+            scanner: sbom_scanner
+          )
+        end
+
+        it 'does not generate errors' do
+          expect { worker.perform(project.id) }.not_to change {
+            scan_sast.reload.info
+          }
+        end
+
+        context 'with dependency_scanning_for_pipelines_with_cyclonedx_reports FF disabled' do
+          before do
+            stub_feature_flags(dependency_scanning_for_pipelines_with_cyclonedx_reports: false)
+          end
+
+          it 'generates errors' do
+            expect { worker.perform(project.id) }.to change {
+              scan_sast.reload.info
+            }.from({})
+              .to({ "errors" => [
+                {
+                  "message" => "Ingestion failed for some vulnerabilities",
+                  "type" => "IngestionError"
+                }
+              ] })
+          end
+        end
+      end
+    end
   end
 
   def update_cache(pipeline)