Commit a1623133 authored by Michał Zając's avatar Michał Zając

Lookup findings using UUIDv5 before find_params

Looking up by UUIDv5 should be more stable in detecting duplicates.

Changes:
 - Use sequence to simulate project_id in Ci::Reports::Security::Finding
   factory
 - Return vulnerability_finding in case of duplicates
 - Check if UUIDv4 gets updated to UUIDv5
parent 6fccc84a
......@@ -80,15 +80,25 @@ module Security
}
begin
vulnerability_finding = project
.vulnerability_findings
# Look for existing Findings using UUID
vulnerability_finding = project.vulnerability_findings.find_by(uuid: finding.uuid)
# If there's no Finding then we're dealing with one of two cases:
# 1. The Finding is a new one
# 2. The Finding is already saved but has UUIDv4
unless vulnerability_finding
vulnerability_finding = project.vulnerability_findings
.create_with(create_params)
.find_or_initialize_by(find_params)
vulnerability_finding.uuid = finding.uuid
end
vulnerability_finding.save!
vulnerability_finding
rescue ActiveRecord::RecordNotUnique
project.vulnerability_findings.find_by!(find_params)
rescue ActiveRecord::RecordNotUnique => e
Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
vulnerability_finding
rescue ActiveRecord::RecordInvalid => e
Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
end
......
......@@ -31,7 +31,9 @@ FactoryBot.define do
scanner factory: :ci_reports_security_scanner
severity { :high }
scan factory: :ci_reports_security_scan
sequence(:uuid) { generate(:vulnerability_finding_uuid) }
sequence(:uuid) do |n|
Gitlab::UUID.v5("#{report_type}-#{identifiers.first&.fingerprint}-#{location.fingerprint}-#{n}")
end
skip_create
......
# frozen_string_literal: true
FactoryBot.define do
sequence :vulnerability_finding_uuid do |n|
SecureRandom.uuid
end
factory :vulnerabilities_finding_with_remediation, parent: :vulnerabilities_finding do
transient do
summary { nil }
......@@ -47,11 +43,13 @@ FactoryBot.define do
factory :vulnerabilities_finding, class: 'Vulnerabilities::Finding' do
name { 'Cipher with no integrity' }
project
sequence(:uuid) { generate(:vulnerability_finding_uuid) }
project_fingerprint { generate(:project_fingerprint) }
primary_identifier factory: :vulnerabilities_identifier
location_fingerprint { '4e5b6966dd100170b4b1ad599c7058cce91b57b4' }
location_fingerprint { SecureRandom.hex(20) }
report_type { :sast }
sequence(:uuid) do
Gitlab::UUID.v5("#{report_type}-#{primary_identifier.fingerprint}-#{location_fingerprint}-#{project_id}")
end
severity { :high }
confidence { :medium }
scanner factory: :vulnerabilities_scanner
......
......@@ -124,9 +124,16 @@ RSpec.describe Security::StoreReportService, '#execute' do
primary_identifier: identifier,
scanner: scanner,
project: project,
uuid: "80571acf-8660-4bc8-811a-1d8dec9ab6f4",
location_fingerprint: 'd869ba3f0b3347eb2749135a437dc07c8ae0f420')
end
let(:uuid_v5_components) do
"#{finding.report_type}-#{finding.primary_identifier.fingerprint}-#{finding.location_fingerprint}-#{finding.project_id}"
end
let(:desired_uuid) { Gitlab::UUID.v5(uuid_v5_components) }
let!(:vulnerability) { create(:vulnerability, findings: [finding], project: project) }
before do
......@@ -136,6 +143,12 @@ RSpec.describe Security::StoreReportService, '#execute' do
subject { described_class.new(new_pipeline, new_report).execute }
it 'updates UUIDv4 to UUIDv5' do
subject
expect(finding.reload.uuid).to eq(desired_uuid)
end
it 'inserts only new scanners and reuse existing ones' do
expect { subject }.to change { Vulnerabilities::Scanner.count }.by(2)
end
......@@ -158,11 +171,13 @@ RSpec.describe Security::StoreReportService, '#execute' do
it 'updates existing findings with new data' do
subject
expect(finding.reload).to have_attributes(severity: 'medium', name: 'Probable insecure usage of temp file/directory.')
end
it 'updates existing vulnerability with new data' do
subject
expect(vulnerability.reload).to have_attributes(severity: 'medium', title: 'Probable insecure usage of temp file/directory.', title_html: 'Probable insecure usage of temp file/directory.')
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment