Merge branch...

Merge branch '235164-sidekiq-storesecurityreportsworker-nomethoderror-undefined-method-key-for-nil-nilclass' into 'master'

Add check for identifiers when creating finding

Closes #235164

See merge request gitlab-org/gitlab!39650

ee/app/services/security/store_report_service.rb

@@ -42,7 +42,7 @@ module Security
     end
 
     def create_vulnerability_finding(finding)
-      return if finding.scanner.blank?
+      return if finding.scanner.blank? || finding.primary_identifier.blank?
 
       vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner)
       vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)

ee/changelogs/unreleased/235164-sidekiq-storesecurityreportsworker-nomethoderror-undefined-meth.yml

+---
+title: Add identifier check when creating vulnerability findings
+merge_request: 39650
+author:
+type: fixed

ee/spec/factories/ci/job_artifacts.rb

@@ -179,6 +179,22 @@ FactoryBot.define do
       end
     end
 
+    trait :sast_with_missing_identifiers do
+      file_type { :sast }
+      file_format { :raw }
+
+      after(:build) do |artifact, _|
+        file = fixture_file_upload(
+          Rails.root.join('ee/spec/fixtures/security_reports/master/gl-sast-report.json'), 'application/json')
+        data = Gitlab::Json.parse(file.tempfile.read)['vulnerabilities'].each { |v| v.delete('identifiers') }.to_json
+        output = Tempfile.new("gl-sast-missing-identifiers")
+        output.write(data)
+        artifact.file = fixture_file_upload(output.path, 'application/json')
+        output.close
+        output.unlink
+      end
+    end
+
     trait :license_management do
       to_create { |instance| instance.save!(validate: false) }
 

ee/spec/services/security/store_report_service_spec.rb

@@ -203,6 +203,29 @@ RSpec.describe Security::StoreReportService, '#execute' do
         expect { subject }.not_to raise_error
       end
     end
+
+    context 'when the finding does not include a primary identifier' do
+      let(:bad_project) { bad_artifact.project }
+      let(:bad_pipeline) { bad_artifact.job.pipeline }
+      let!(:bad_artifact) { create(:ee_ci_job_artifact, :sast_with_missing_identifiers) }
+      let(:bad_report) { bad_pipeline.security_reports.get_report(report_type.to_s, bad_artifact) }
+      let(:report_type) { :sast }
+
+      before do
+        bad_project.add_developer(user)
+        allow(bad_pipeline).to receive(:user).and_return(user)
+      end
+
+      subject { described_class.new(bad_pipeline, bad_report).execute }
+
+      it 'does not create a new finding' do
+        expect { subject }.not_to change { Vulnerabilities::Finding.count }
+      end
+
+      it 'does not raise an error' do
+        expect { subject }.not_to raise_error
+      end
+    end
   end
 
   context 'with existing data from same pipeline' do