Commit 057fef27 authored by Saikat Sarkar's avatar Saikat Sarkar

Add vulnerability flags in Vulnerability Findings API

parent d5aba68e
......@@ -61,6 +61,11 @@ module Security
finding.project = project
finding.sha = pipeline.sha
finding.scanner = security_finding.scanner
if calculate_false_positive?
finding.vulnerability_flags = existing_vulnerability_flags.fetch(security_finding.uuid, [])
end
finding.identifiers = identifiers
finding.signatures = signatures
end
......@@ -74,6 +79,14 @@ module Security
existing_vulnerabilities.dig(security_finding.scan.scan_type, security_finding.project_fingerprint)&.first
end
def existing_vulnerability_flags
@existing_vulnerability_flags ||= project.vulnerability_flags_for(security_findings.map(&:uuid))
end
def calculate_false_positive?
::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
end
def existing_vulnerabilities
@existing_vulnerabilities ||= begin
project.vulnerabilities
......
......@@ -33,7 +33,9 @@ module Security
normalized_findings = normalize_report_findings(
report.findings,
vulnerabilities_by_finding_fingerprint(report))
vulnerabilities_by_finding_fingerprint(report),
existing_vulnerability_flags_for(report))
filtered_findings = filter(normalized_findings)
findings.concat(filtered_findings)
......@@ -74,12 +76,16 @@ module Security
.select(:vulnerability_id, :project_fingerprint)
end
def existing_vulnerability_flags_for(report)
pipeline.project.vulnerability_flags_for(report.findings.map(&:uuid))
end
# This finder is used for fetching vulnerabilities for any pipeline, if we used it to fetch
# vulnerabilities for a non-default-branch, the findings will be unpersisted, so we
# coerce the POROs into unpersisted AR records to give them a common object.
# See https://gitlab.com/gitlab-org/gitlab/issues/33588#note_291849433 for more context
# on why this happens.
def normalize_report_findings(report_findings, vulnerabilities)
def normalize_report_findings(report_findings, vulnerabilities, vulnerability_flags)
report_findings.map do |report_finding|
finding_hash = report_finding.to_hash
.except(:compare_key, :identifiers, :location, :scanner, :links, :signatures)
......@@ -101,10 +107,19 @@ module Security
Vulnerabilities::FindingSignature.new(signature.to_hash)
end
if calculate_false_positive?
finding.vulnerability_flags = vulnerability_flags.fetch(finding.uuid, [])
end
finding
end
end
def calculate_false_positive?
project = pipeline.project
::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
end
def filter(findings)
findings.select do |finding|
next unless in_selected_state?(finding)
......
# frozen_string_literal: true
module EE
module VulnerabilityFlagHelpers
def vulnerability_flags_for(uuids)
vulnerability_findings
.by_uuid(uuids)
.eager_load_vulnerability_flags
.to_h { |finding| [finding.uuid, finding.vulnerability_flags] }
end
end
end
......@@ -21,6 +21,7 @@ module EE
include DeprecatedApprovalsBeforeMerge
include UsageStatistics
include ProjectSecurityScannersInformation
include VulnerabilityFlagHelpers
before_save :set_override_pull_mirror_available, unless: -> { ::Gitlab::CurrentSettings.mirror_available }
before_save :set_next_execution_timestamp_to_now, if: ->(project) { project.mirror? && project.mirror_changed? && project.import_state }
......
......@@ -87,6 +87,7 @@ module Vulnerabilities
scope :by_severities, -> (values) { where(severity: values) }
scope :by_confidences, -> (values) { where(confidence: values) }
scope :by_project_fingerprints, -> (values) { where(project_fingerprint: values) }
scope :by_uuid, -> (uuids) { where(uuid: uuids) }
scope :all_preloaded, -> do
preload(:scanner, :identifiers, project: [:namespace, :project_feature])
......
......@@ -12,6 +12,9 @@ class Vulnerabilities::FindingEntity < Grape::Entity
expose :create_jira_issue_url do |occurrence|
create_jira_issue_url_for(occurrence)
end
expose :false_positive, if: -> (_, _) { expose_false_positive? } do |occurrence|
occurrence.vulnerability_flags.any?(&:false_positive?)
end
expose :create_vulnerability_feedback_issue_path do |occurrence|
create_vulnerability_feedback_issue_path(occurrence.project)
end
......@@ -54,6 +57,13 @@ class Vulnerabilities::FindingEntity < Grape::Entity
def current_user
return request.current_user if request.respond_to?(:current_user)
end
private
def expose_false_positive?
project = occurrence.project
::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
end
end
Vulnerabilities::FindingEntity.include_mod_with('ProjectsHelper')
---
name: vulnerability_flags
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66775
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336630
milestone: '14.2'
type: development
group: group::static analysis
default_enabled: false
......@@ -59,6 +59,8 @@ RSpec.describe Security::FindingsFinder do
deduplicated: true,
position: index,
scan: scan)
create(:vulnerabilities_finding, uuid: finding.uuid, project: pipeline.project)
end
end
......@@ -76,8 +78,20 @@ RSpec.describe Security::FindingsFinder do
stub_licensed_features(sast: true, dependency_scanning: true)
end
it 'does not cause N+1 queries' do
expect { finder_result }.not_to exceed_query_limit(8)
context 'N+1 queries' do
it 'does not cause N+1 queries' do
expect { finder_result }.not_to exceed_query_limit(10)
end
context 'with vulnerability_flags disabled' do
before do
stub_feature_flags(vulnerability_flags: false)
end
it 'does not cause N+1 queries' do
expect { finder_result }.not_to exceed_query_limit(8)
end
end
end
describe '#current_page' do
......@@ -169,6 +183,35 @@ RSpec.describe Security::FindingsFinder do
end
end
describe '#vulnerability_flags' do
before do
stub_licensed_features(sast_fp_reduction: true)
end
context 'with no vulnerability flags present' do
it 'does not have any vulnerability flag' do
expect(finder_result.findings).to all(have_attributes(vulnerability_flags: be_empty))
end
end
context 'with some vulnerability flags present' do
before do
create(:vulnerabilities_flag, finding: pipeline.project.vulnerability_findings.first)
create(:vulnerabilities_flag, finding: pipeline.project.vulnerability_findings.last)
end
it 'has some vulnerability_findings with vulnerability flag' do
expect(finder_result.findings).to include(have_attributes(vulnerability_flags: be_present))
end
it 'does not have any vulnerability_flag if license is not available' do
stub_licensed_features(sast_fp_reduction: false)
expect(finder_result.findings).to all(have_attributes(vulnerability_flags: be_empty))
end
end
end
describe '#findings' do
subject { finder_result.findings.map(&:project_fingerprint) }
......
......@@ -38,7 +38,7 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
end
before do
stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, dast: true)
stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, dast: true, sast_fp_reduction: true)
# Stub out deduplication, if not done the expectations will vary based on the fixtures (which may/may not have duplicates)
disable_deduplication
end
......@@ -137,6 +137,33 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
expect(subject.findings.map(&:uuid)).to match_array(sast_report_uuids)
expect(subject.findings.count).to eq(sast_count)
end
context "false-positive" do
before do
vulnerability_finding = create(:vulnerabilities_finding, uuid: sast_report_uuids.first, project: pipeline.project)
create(:vulnerabilities_flag, finding: vulnerability_finding)
end
it 'includes findings with false-positive' do
expect(subject.findings.flat_map(&:vulnerability_flags)).to be_present
end
it 'does not include findings with false-positive if license is not available' do
stub_licensed_features(sast_fp_reduction: false)
expect(subject.findings).to all(have_attributes(vulnerability_flags: be_empty))
end
context 'with vulnerability_flags FF disabled' do
before do
stub_feature_flags(vulnerability_flags: false)
end
it 'does not include findings with false-positive' do
expect(subject.findings).to all(have_attributes(vulnerability_flags: be_empty))
end
end
end
end
context 'when dependency_scanning' do
......
......@@ -69,7 +69,8 @@ RSpec.describe API::VulnerabilityFindings do
# Threshold is required for the extra query performed in Security::PipelineVulnerabilitiesFinder to load
# the Vulnerabilities providing computed states for the associated Vulnerability::Findings
expect { get api(project_vulnerability_findings_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1)
# and all associated vulnerability_flags
expect { get api(project_vulnerability_findings_path, user) }.not_to exceed_query_limit(control_count).with_threshold(3)
end
describe 'using different finders' do
......
......@@ -23,10 +23,17 @@ RSpec.describe Vulnerabilities::FindingEntity do
scanner: scanner,
scan: scan,
project: project,
identifiers: identifiers
identifiers: identifiers,
vulnerability_flags: flags
)
end
let(:flags) do
[
build(:vulnerabilities_flag)
]
end
let(:dismiss_feedback) do
build(:vulnerability_feedback, :sast, :dismissal,
project: project, project_fingerprint: occurrence.project_fingerprint)
......@@ -47,6 +54,7 @@ RSpec.describe Vulnerabilities::FindingEntity do
subject { entity.as_json }
before do
stub_licensed_features(sast_fp_reduction: true)
allow(request).to receive(:current_user).and_return(user)
end
......@@ -58,11 +66,30 @@ RSpec.describe Vulnerabilities::FindingEntity do
expect(subject).to include(:description, :links, :location, :remediations, :solution, :evidence)
expect(subject).to include(:blob_path, :request, :response)
expect(subject).to include(:scan)
expect(subject).to include(:false_positive)
expect(subject).to include(:assets, :evidence_source, :supporting_messages)
expect(subject).to include(:uuid)
expect(subject).to include(:details)
end
context 'false-positive' do
it 'finds the vulnerability_finding as false_positive' do
expect(subject[:false_positive]).to be(true)
end
it 'does not contain false_positive field if feature_flag is disabled' do
stub_feature_flags(vulnerability_flags: false)
expect(subject).not_to include(:false_positive)
end
it 'does not contain false_positive field if license is not available' do
stub_licensed_features(sast_fp_reduction: false)
expect(subject).not_to include(:false_positive)
end
end
context 'when not allowed to admin vulnerability feedback' do
before do
project.add_guest(user)
......
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