diff --git a/ee/app/services/security/store_report_service.rb b/ee/app/services/security/store_report_service.rb
index a37148922d0bb2c073494f08ca895dfbcfd8d4c6..efc745177a1e8ee87509782fac52a6e5bda1bf13 100644
--- a/ee/app/services/security/store_report_service.rb
+++ b/ee/app/services/security/store_report_service.rb
@@ -42,7 +42,10 @@ def mark_as_resolved_except(vulnerability_ids)
end
def create_vulnerability_finding(finding)
- return if finding.scanner.blank? || finding.primary_identifier.blank?
+ unless finding.valid?
+ put_warning_for(finding)
+ return
+ end
vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner)
vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
@@ -172,5 +175,9 @@ def existing_identifiers_objects
end.to_h
end
end
+
+ def put_warning_for(finding)
+ Gitlab::AppLogger.warn(message: "Invalid vulnerability finding record found", finding: finding.to_hash)
+ end
end
end
diff --git a/ee/lib/gitlab/ci/reports/security/finding.rb b/ee/lib/gitlab/ci/reports/security/finding.rb
index 1aa565c63c59769260cfc35d38b16f5cb4f75cf0..baf44ee4b87b05461fe83b706f9bb6be899a5d14 100644
--- a/ee/lib/gitlab/ci/reports/security/finding.rb
+++ b/ee/lib/gitlab/ci/reports/security/finding.rb
@@ -81,6 +81,10 @@ def hash
report_type.hash ^ location.fingerprint.hash ^ primary_identifier.fingerprint.hash
end
+ def valid?
+ scanner.present? && primary_identifier.present? && location.present?
+ end
+
private
def generate_project_fingerprint
diff --git a/ee/spec/factories/ci/job_artifacts.rb b/ee/spec/factories/ci/job_artifacts.rb
index ef821a2c18eedc0208bc363b03878936a0ee2a5d..84e8de6901d49603eee251c82eb237d3451278f9 100644
--- a/ee/spec/factories/ci/job_artifacts.rb
+++ b/ee/spec/factories/ci/job_artifacts.rb
@@ -179,22 +179,6 @@
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) }
diff --git a/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb b/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb
index 5ebda2926c70cb5beba799645bd70a3df742d96c..6255e984ef26048e5fb289d30fbfc2e025e05416 100644
--- a/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb
+++ b/ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb
@@ -228,4 +228,42 @@
end
end
end
+
+ describe '#valid?' do
+ let(:scanner) { build(:ci_reports_security_scanner) }
+ let(:identifiers) { [build(:ci_reports_security_identifier)] }
+ let(:location) { build(:ci_reports_security_locations_sast) }
+
+ let(:finding) do
+ build(:ci_reports_security_finding,
+ scanner: scanner,
+ identifiers: identifiers,
+ location: location,
+ compare_key: '')
+ end
+
+ subject { finding.valid? }
+
+ context 'when the scanner is missing' do
+ let(:scanner) { nil }
+
+ it { is_expected.to be_falsey }
+ end
+
+ context 'when there is no identifier' do
+ let(:identifiers) { [] }
+
+ it { is_expected.to be_falsey }
+ end
+
+ context 'when the location is missing' do
+ let(:location) { nil }
+
+ it { is_expected.to be_falsey }
+ end
+
+ context 'when all required attributes present' do
+ it { is_expected.to be_truthy }
+ end
+ end
end
diff --git a/ee/spec/services/security/store_report_service_spec.rb b/ee/spec/services/security/store_report_service_spec.rb
index e5d916ed6f67b85d1447c4025e1ca607df7b38cb..fa1001a8125a71e3996bda4fd4078beb3ac49c72 100644
--- a/ee/spec/services/security/store_report_service_spec.rb
+++ b/ee/spec/services/security/store_report_service_spec.rb
@@ -181,20 +181,14 @@
end
end
- context 'when the finding does not include a scanner' do
- let(:bad_pipeline) { create(:ci_pipeline, project: project) }
- let(:bad_build) { create(:ci_build, pipeline: bad_pipeline) }
- let!(:bad_artifact) { create(:ee_ci_job_artifact, :sast_with_missing_scanner, job: bad_build) }
- let(:bad_report) { bad_pipeline.security_reports.get_report(report_type.to_s, bad_artifact) }
- let(:report_type) { :sast }
-
+ context 'when the finding is not valid' do
before do
- project.add_developer(user)
- allow(bad_pipeline).to receive(:user).and_return(user)
+ allow(Gitlab::AppLogger).to receive(:warn)
+ allow_next_instance_of(::Gitlab::Ci::Reports::Security::Finding) do |finding|
+ allow(finding).to receive(:valid?).and_return(false)
+ end
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
@@ -202,28 +196,11 @@
it 'does not raise an error' 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 'puts a warning log' do
+ subject
- 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
+ expect(Gitlab::AppLogger).to have_received(:warn).exactly(new_report.findings.length).times
end
end
end