From 441cad8a96aadf07a82fc8bd1a8b6a8f3dcdf544 Mon Sep 17 00:00:00 2001
From: Mehmet Emin INAC <minac@gitlab.com>
Date: Mon, 12 Dec 2022 14:51:47 +0000
Subject: [PATCH] Use `finding_data` to generate response
This commit introduces a new finder class to be used while we are
transitioning from using artifacts to utilizing data stored in the DB.
This new finder class will mainly use the data stored in the database
except the remediations are still read from report artifacts by using
the `multi_read` method.
With this change, we will also no longer show the findings of purged
security scans.
Changelog: fixed
EE: true
---
app/uploaders/gitlab_uploader.rb | 9 +
ee/app/finders/security/findings_finder.rb | 5 +-
.../finders/security/pure_findings_finder.rb | 39 ++
ee/app/models/security/finding.rb | 71 +++-
ee/app/models/security/remediations_proxy.rb | 30 ++
ee/app/models/security/scan.rb | 4 +
ee/app/models/vulnerabilities/finding.rb | 2 +-
ee/app/models/vulnerabilities/read.rb | 6 +
.../vulnerabilities/finding_entity.rb | 6 +-
.../development/utilize_finding_data.yml | 8 +
ee/lib/api/vulnerability_findings.rb | 33 +-
ee/spec/factories/security_scans.rb | 21 +
.../finders/security/findings_finder_spec.rb | 381 ++----------------
.../security/pure_findings_finder_spec.rb | 22 +
ee/spec/models/security/finding_spec.rb | 138 ++++++-
.../security/remediations_proxy_spec.rb | 55 +++
ee/spec/models/security/scan_spec.rb | 17 +-
ee/spec/models/vulnerabilities/read_spec.rb | 3 +-
.../api/vulnerability_findings_spec.rb | 92 +++--
.../findings_finder_shared_examples.rb | 343 ++++++++++++++++
spec/uploaders/gitlab_uploader_spec.rb | 15 +-
21 files changed, 868 insertions(+), 432 deletions(-)
create mode 100644 ee/app/finders/security/pure_findings_finder.rb
create mode 100644 ee/app/models/security/remediations_proxy.rb
create mode 100644 ee/config/feature_flags/development/utilize_finding_data.yml
create mode 100644 ee/spec/finders/security/pure_findings_finder_spec.rb
create mode 100644 ee/spec/models/security/remediations_proxy_spec.rb
create mode 100644 ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index 29091eb93ef1..62024bff4c08 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/app/uploaders/gitlab_uploader.rb
@@ -115,6 +115,15 @@ def open
end
end
+ def multi_read(offsets)
+ open do |stream|
+ offsets.map do |start_offset, end_offset|
+ stream.seek(start_offset)
+ stream.read(end_offset - start_offset + 1)
+ end
+ end
+ end
+
# Used to replace an existing upload with another +file+ without modifying stored metadata
# Use this method only to repair/replace an existing upload, or to upload to a Geo secondary node
#
diff --git a/ee/app/finders/security/findings_finder.rb b/ee/app/finders/security/findings_finder.rb
index 19f127fa01f7..7846422a8b5c 100644
--- a/ee/app/finders/security/findings_finder.rb
+++ b/ee/app/finders/security/findings_finder.rb
@@ -19,7 +19,8 @@ class FindingsFinder
include ::VulnerabilityFindingHelpers
ResultSet = Struct.new(:relation, :findings) do
- delegate :current_page, :limit_value, :total_pages, :total_count, :next_page, :prev_page, to: :relation
+ delegate :current_page, :limit_value, :total_pages, :total_count,
+ :next_page, :prev_page, :page, :empty?, to: :relation
end
DEFAULT_PAGE = 1
@@ -31,7 +32,7 @@ def initialize(pipeline, params: {})
end
def execute
- return unless has_security_findings?
+ return ResultSet.new(Security::Finding.none.page(DEFAULT_PAGE), []) unless has_security_findings?
ResultSet.new(security_findings, findings)
end
diff --git a/ee/app/finders/security/pure_findings_finder.rb b/ee/app/finders/security/pure_findings_finder.rb
new file mode 100644
index 000000000000..b123a0b4a06c
--- /dev/null
+++ b/ee/app/finders/security/pure_findings_finder.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+# Security::PureFindingsFinder
+#
+# This finder returns Active Record relation of `Security::Finding` model
+# which is different than the other finders maintained by threat insights.
+# It's called pure because it does not depend on report artifacts(mostly)
+# and uses the data stored in the `Security::Finding` model.
+#
+# Arguments:
+# pipeline - object to filter findings
+# params:
+# severity: Array<String>
+# confidence: Array<String>
+# report_type: Array<String>
+# scope: String
+# page: Int
+# per_page: Int
+
+module Security
+ class PureFindingsFinder < FindingsFinder
+ def execute
+ security_findings.tap do |findings|
+ findings.each(&:remediations) # initiates Batchloader
+ end
+ end
+
+ def available?
+ pipeline.security_findings.first&.finding_data.present?
+ end
+
+ private
+
+ def security_findings
+ super.with_feedbacks
+ .with_vulnerability
+ end
+ end
+end
diff --git a/ee/app/models/security/finding.rb b/ee/app/models/security/finding.rb
index e12f1d68f654..e61b4855f881 100644
--- a/ee/app/models/security/finding.rb
+++ b/ee/app/models/security/finding.rb
@@ -10,9 +10,12 @@
module Security
class Finding < ApplicationRecord
include EachBatch
+ include Presentable
include PartitionedTable
MAX_PARTITION_SIZE = 10.gigabyte
+ ATTRIBUTES_DELEGATED_TO_FINDING_DATA = %i[name description solution location identifiers links false_positive?
+ assets evidence details remediation_byte_offsets].freeze
self.table_name = 'security_findings'
self.primary_key = :id # As ActiveRecord does not support compound PKs
@@ -26,8 +29,15 @@ class Finding < ApplicationRecord
belongs_to :scan, inverse_of: :findings, optional: false
belongs_to :scanner, class_name: 'Vulnerabilities::Scanner', inverse_of: :security_findings, optional: false
+ belongs_to :vulnerability_read,
+ class_name: 'Vulnerabilities::Read',
+ primary_key: :uuid,
+ foreign_key: :uuid,
+ inverse_of: :security_findings
has_one :build, through: :scan, disable_joins: true
+ has_one :vulnerability, through: :vulnerability_read
+
has_many :feedbacks,
class_name: 'Vulnerabilities::Feedback',
inverse_of: :security_finding,
@@ -62,15 +72,17 @@ class Finding < ApplicationRecord
.where('vulnerability_feedback.finding_uuid = security_findings.uuid'))
end
scope :latest, -> { joins(:scan).merge(Security::Scan.latest_successful) }
- scope :ordered, -> { order(severity: :desc, confidence: :desc, id: :asc) }
+ scope :ordered, -> { order(severity: :desc, id: :asc) }
scope :with_pipeline_entities, -> { preload(build: [:job_artifacts, :pipeline]) }
- scope :with_scan, -> { includes(:scan) }
+ scope :with_scan, -> { preload(:scan) }
scope :with_scanner, -> { includes(:scanner) }
+ scope :with_feedbacks, -> { includes(:feedbacks) }
+ scope :with_vulnerability, -> { includes(:vulnerability) }
scope :deduplicated, -> { where(deduplicated: true) }
scope :grouped_by_scan_type, -> { joins(:scan).group('security_scans.scan_type') }
- delegate :scan_type, :pipeline, to: :scan, allow_nil: true
- delegate :project, to: :pipeline
+ delegate :scan_type, :pipeline, :remediations_proxy, to: :scan, allow_nil: true
+ delegate :project, :sha, to: :pipeline
class << self
def count_by_scan_type
@@ -103,5 +115,56 @@ def last_finding_in_partition(partition_number)
where(partition_number: partition_number).last
end
end
+
+ # Following alias attributes as used by `Vulnerabilities::FindingEntity`
+ alias_attribute :raw_metadata, :finding_data
+ alias_attribute :report_type, :scan_type
+
+ def dismissal_feedback
+ feedbacks.find(&:for_dismissal?)
+ end
+
+ def issue_feedback
+ feedbacks.find(&:for_issue?)
+ end
+
+ def merge_request_feedback
+ feedbacks.find(&:for_merge_request?)
+ end
+
+ def state
+ return vulnerability.state if vulnerability
+
+ dismissal_feedback ? 'dismissed' : 'detected'
+ end
+
+ # Symbolizing the hash keys is important as Grape entity
+ # works with symbolized keys only.
+ # See https://github.com/ruby-grape/grape-entity/issues/223
+ def symbolized_finding_data
+ @symbolized_finding_data ||= finding_data.deep_symbolize_keys
+ end
+
+ def finding_data=(value)
+ super
+ ensure
+ @symbolized_finding_data = nil
+ end
+
+ # Defines methods for the keys exist in `finding_data` to support the same
+ # interface with `Vulnerabilities::Finding` model as these methods are used
+ # by `Vulnerabilities::FindingEntity`.
+ ATTRIBUTES_DELEGATED_TO_FINDING_DATA.each do |delegated_attribute|
+ define_method(delegated_attribute) do
+ symbolized_finding_data.fetch(delegated_attribute)
+ end
+ end
+
+ def remediations
+ return [] unless symbolized_finding_data[:remediation_byte_offsets]
+
+ symbolized_finding_data[:remediation_byte_offsets].map { |offset| offset.values_at(:start_byte, :end_byte) }
+ .then { remediations_proxy.by_byte_offsets(_1) }
+ end
end
end
diff --git a/ee/app/models/security/remediations_proxy.rb b/ee/app/models/security/remediations_proxy.rb
new file mode 100644
index 000000000000..0c5f12726401
--- /dev/null
+++ b/ee/app/models/security/remediations_proxy.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Security
+ class RemediationsProxy
+ attr_reader :file
+
+ def initialize(file)
+ @file = file
+ end
+
+ def by_byte_offsets(byte_offsets)
+ byte_offsets.map do |offsets|
+ batch_load(offsets)
+ end
+ end
+
+ private
+
+ def batch_load(offsets)
+ BatchLoader.for(offsets).batch(key: object_id) do |byte_offsets, loader|
+ byte_offsets.uniq
+ .sort
+ .then { |offsets| file.multi_read(offsets) }
+ .map { |data| Gitlab::Json.parse(data) }
+ .zip(byte_offsets)
+ .each { |remediation, offsets| loader.call(offsets, remediation) }
+ end
+ end
+ end
+end
diff --git a/ee/app/models/security/scan.rb b/ee/app/models/security/scan.rb
index 88a9417c6db9..756573f56a2f 100644
--- a/ee/app/models/security/scan.rb
+++ b/ee/app/models/security/scan.rb
@@ -107,6 +107,10 @@ def report_primary_identifiers
@report_primary_identifiers ||= security_report&.primary_identifiers
end
+ def remediations_proxy
+ @remediations_proxy ||= RemediationsProxy.new(job_artifact.file)
+ end
+
private
def security_report
diff --git a/ee/app/models/vulnerabilities/finding.rb b/ee/app/models/vulnerabilities/finding.rb
index 9f97eaff4a53..fccbbf8bc516 100644
--- a/ee/app/models/vulnerabilities/finding.rb
+++ b/ee/app/models/vulnerabilities/finding.rb
@@ -380,7 +380,7 @@ def pipeline_branch
end
def false_positive?
- vulnerability_flags.false_positive.any?
+ vulnerability_flags.any?(&:false_positive?)
end
def first_finding_pipeline
diff --git a/ee/app/models/vulnerabilities/read.rb b/ee/app/models/vulnerabilities/read.rb
index 5a244bde35b4..f4299fcb56d4 100644
--- a/ee/app/models/vulnerabilities/read.rb
+++ b/ee/app/models/vulnerabilities/read.rb
@@ -12,6 +12,12 @@ class Read < ApplicationRecord
belongs_to :project
belongs_to :scanner, class_name: 'Vulnerabilities::Scanner'
+ has_many :security_findings,
+ class_name: 'Security::Finding',
+ primary_key: :uuid,
+ foreign_key: :uuid,
+ inverse_of: :vulnerability_read
+
validates :vulnerability_id, uniqueness: true, presence: true
validates :project_id, presence: true
validates :scanner_id, presence: true
diff --git a/ee/app/serializers/vulnerabilities/finding_entity.rb b/ee/app/serializers/vulnerabilities/finding_entity.rb
index f878020ed1e9..ae5ec53908ef 100644
--- a/ee/app/serializers/vulnerabilities/finding_entity.rb
+++ b/ee/app/serializers/vulnerabilities/finding_entity.rb
@@ -13,9 +13,7 @@ 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 :false_positive?, as: :false_positive, if: -> (_, _) { expose_false_positive? }
expose :create_vulnerability_feedback_issue_path do |occurrence|
create_vulnerability_feedback_issue_path(occurrence.project)
end
@@ -53,7 +51,7 @@ class Vulnerabilities::FindingEntity < Grape::Entity
expose :scan
expose :blob_path do |occurrence|
- occurrence.present.blob_path
+ occurrence.present(presenter_class: Vulnerabilities::FindingPresenter).blob_path
end
alias_method :occurrence, :object
diff --git a/ee/config/feature_flags/development/utilize_finding_data.yml b/ee/config/feature_flags/development/utilize_finding_data.yml
new file mode 100644
index 000000000000..336bdce5ad09
--- /dev/null
+++ b/ee/config/feature_flags/development/utilize_finding_data.yml
@@ -0,0 +1,8 @@
+---
+name: utilize_finding_data
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/103652
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/383430
+milestone: '15.7'
+type: development
+group: group::threat insights
+default_enabled: false
diff --git a/ee/lib/api/vulnerability_findings.rb b/ee/lib/api/vulnerability_findings.rb
index 38f49165c42b..67d998917437 100644
--- a/ee/lib/api/vulnerability_findings.rb
+++ b/ee/lib/api/vulnerability_findings.rb
@@ -23,31 +23,26 @@ def vulnerability_findings
@vulnerability_findings ||= begin
return paginate(Kaminari.paginate_array([])) unless pipeline
- with_adaptive_finder || with_vulnerabilities_finder
+ with_pure_finder || with_adaptive_finder
end
end
- # We are using this fallback to provide an early response to the users
- # until the `security_findings` are stored.
- # We will remove this fallback by https://gitlab.com/gitlab-org/gitlab/-/issues/334488
- def with_vulnerabilities_finder
- aggregated_report = Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: declared_params).execute
-
- # We might have to add rubocop:disable annotation here in case
- # https://gitlab.com/gitlab-org/gitlab/issues/32763 happens, using
- # Kaminari.paginate_array here is correct
- # See https://gitlab.com/gitlab-org/gitlab/issues/33588#note_291849433
- # for discussion
- paginate(Kaminari.paginate_array(aggregated_report.findings))
- rescue Security::PipelineVulnerabilitiesFinder::ParseError
- paginate(Kaminari.paginate_array([]))
+ def with_pure_finder
+ pure_finder = Security::PureFindingsFinder.new(pipeline, params: declared_params)
+ return unless Feature.enabled?(:utilize_finding_data, pipeline.project) && pure_finder.available?
+
+ paginate(pure_finder.execute)
end
def with_adaptive_finder
result = Security::FindingsFinder.new(pipeline, params: declared_params).execute
- return unless result
+ paginate(result)
+
+ if Feature.disabled?(:deprecate_vulnerabilities_feedback, user_project)
+ Gitlab::Vulnerabilities::FindingsPreloader.preload_feedback!(result.findings)
+ end
- paginate(result).findings
+ result.findings
end
end
@@ -101,10 +96,6 @@ def with_adaptive_finder
get ':id/vulnerability_findings' do
authorize! :read_security_resource, user_project
- if Feature.disabled?(:deprecate_vulnerabilities_feedback, user_project)
- Gitlab::Vulnerabilities::FindingsPreloader.preload_feedback!(vulnerability_findings)
- end
-
present vulnerability_findings,
with: ::Vulnerabilities::FindingEntity,
request: GrapeRequestProxy.new(request, current_user)
diff --git a/ee/spec/factories/security_scans.rb b/ee/spec/factories/security_scans.rb
index 08d5a741a330..2362ca73ec36 100644
--- a/ee/spec/factories/security_scans.rb
+++ b/ee/spec/factories/security_scans.rb
@@ -23,5 +23,26 @@
trait :purged do
status { :purged }
end
+
+ trait :with_findings do
+ # rubocop:disable RSpec/FactoryBot/StrategyInCallback
+ after(:create) do |scan|
+ artifact = create(:ee_ci_job_artifact, scan.scan_type, job: scan.build, project: scan.project)
+ report = create(:ci_reports_security_report, pipeline: scan.pipeline, type: scan.scan_type)
+
+ Gitlab::Ci::Parsers.fabricate!(scan.scan_type, artifact.file.read, report).parse!
+ report = Security::MergeReportsService.new(report).execute
+
+ report.findings.each do |finding|
+ create(:security_finding,
+ severity: finding.severity,
+ confidence: finding.confidence,
+ uuid: finding.uuid,
+ deduplicated: true,
+ scan: scan)
+ end
+ end
+ # rubocop:enable RSpec/FactoryBot/StrategyInCallback
+ end
end
end
diff --git a/ee/spec/finders/security/findings_finder_spec.rb b/ee/spec/finders/security/findings_finder_spec.rb
index 2310652e48de..dbf94aeb4c1f 100644
--- a/ee/spec/finders/security/findings_finder_spec.rb
+++ b/ee/spec/finders/security/findings_finder_spec.rb
@@ -2,377 +2,56 @@
require 'spec_helper'
-RSpec.describe Security::FindingsFinder do
- let_it_be(:pipeline) { create(:ci_pipeline) }
- let_it_be(:build_1) { create(:ci_build, :success, name: 'dependency_scanning', pipeline: pipeline) }
- let_it_be(:build_2) { create(:ci_build, :success, name: 'sast', pipeline: pipeline) }
- let_it_be(:artifact_ds) { create(:ee_ci_job_artifact, :dependency_scanning, job: build_1) }
- let_it_be(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_2) }
- let_it_be(:report_ds) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) }
- let_it_be(:report_sast) { create(:ci_reports_security_report, pipeline: pipeline, type: :sast) }
+RSpec.describe Security::FindingsFinder, feature_category: :vulnerability_management do
+ it_behaves_like 'security findings finder' do
+ let(:findings) { service_object.execute.findings }
+ let(:query_limit) { 11 }
- let(:severity_levels) { nil }
- let(:confidence_levels) { nil }
- let(:report_types) { nil }
- let(:scope) { nil }
- let(:page) { nil }
- let(:per_page) { nil }
- let(:service_object) { described_class.new(pipeline, params: params) }
- let(:params) do
- {
- severity: severity_levels,
- confidence: confidence_levels,
- report_type: report_types,
- scope: scope,
- page: page,
- per_page: per_page
- }
- end
-
- describe '#execute' do
- context 'when the pipeline does not have security findings' do
- subject { service_object.execute }
-
- it { is_expected.to be_nil }
- end
-
- context 'when the pipeline has security findings' do
- let(:finder_result) { service_object.execute }
-
- before(:all) do
- ds_content = File.read(artifact_ds.file.path)
- Gitlab::Ci::Parsers::Security::DependencyScanning.parse!(ds_content, report_ds)
- report_ds.merge!(report_ds)
- sast_content = File.read(artifact_sast.file.path)
- Gitlab::Ci::Parsers::Security::Sast.parse!(sast_content, report_sast)
- report_sast.merge!(report_sast)
-
- findings = { artifact_ds => report_ds, artifact_sast => report_sast }.collect do |artifact, report|
- scan = create(:security_scan, :latest_successful, scan_type: artifact.job.name, build: artifact.job)
-
- report.findings.collect do |finding, index|
- create(:security_finding,
- severity: finding.severity,
- confidence: finding.confidence,
- uuid: finding.uuid,
- deduplicated: true,
- scan: scan)
- end
- end.flatten
-
- findings.second.update!(deduplicated: false)
- end
-
- before do
- stub_licensed_features(sast: true, dependency_scanning: true)
- end
-
- context 'N+1 queries' do
- it 'does not cause N+1 queries' do
- expect { finder_result }.not_to exceed_query_limit(11)
- end
+ context 'when the `security_findings` records have `overridden_uuid`s' do
+ let(:security_findings) { Security::Finding.by_build_ids(build_1) }
+ let(:expected_uuids) do
+ Security::Finding.where(overridden_uuid: nil).pluck(:uuid)
+ .concat(Security::Finding.where.not(overridden_uuid: nil).pluck(:overridden_uuid)) -
+ [Security::Finding.second[:overridden_uuid]]
end
- describe '#current_page' do
- subject { finder_result.current_page }
-
- context 'when the page is not provided' do
- it { is_expected.to be(1) }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
-
- it { is_expected.to be(2) }
- end
- end
-
- describe '#limit_value' do
- subject { finder_result.limit_value }
-
- context 'when the per_page is not provided' do
- it { is_expected.to be(20) }
- end
-
- context 'when the per_page is provided' do
- let(:per_page) { 100 }
-
- it { is_expected.to be(100) }
- end
- end
-
- describe '#total_pages' do
- subject { finder_result.total_pages }
-
- context 'when the per_page is not provided' do
- it { is_expected.to be(1) }
- end
+ subject { findings.map(&:uuid) }
- context 'when the per_page is provided' do
- let(:per_page) { 3 }
-
- it { is_expected.to be(3) }
- end
- end
-
- describe '#total_count' do
- subject { finder_result.total_count }
-
- context 'when the scope is not provided' do
- let(:finding_to_dismiss) { Security::Finding.first }
-
- before do
- stub_feature_flags(deprecate_vulnerabilities_feedback: deprecate_vulnerabilities_feedback?)
- end
-
- context 'when the `deprecate_vulnerabilities_feedback` feature is enabled' do
- let(:deprecate_vulnerabilities_feedback?) { true }
-
- before do
- create(:vulnerabilities_finding, :dismissed, uuid: finding_to_dismiss.uuid)
- end
-
- it { is_expected.to be(7) }
- end
-
- context 'when the `deprecate_vulnerabilities_feedback` feature is disabled' do
- let(:deprecate_vulnerabilities_feedback?) { false }
-
- before do
- create(:vulnerability_feedback,
- :dismissal,
- project: pipeline.project,
- category: :dependency_scanning,
- finding_uuid: finding_to_dismiss.uuid)
- end
-
- it { is_expected.to be(7) }
- end
- end
-
- context 'when the scope is provided as `all`' do
- let(:scope) { 'all' }
-
- it { is_expected.to be(8) }
+ before do
+ security_findings.each do |security_finding|
+ security_finding.update!(overridden_uuid: security_finding.uuid, uuid: SecureRandom.uuid)
end
end
- describe '#next_page' do
- subject { finder_result.next_page }
-
- context 'when the page is not provided' do
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
-
- it { is_expected.to be(2) }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
+ it { is_expected.to match_array(expected_uuids) }
+ end
- it { is_expected.to be_nil }
- end
+ describe '#vulnerability_flags' do
+ before do
+ stub_licensed_features(sast_fp_reduction: true)
end
- describe '#prev_page' do
- subject { finder_result.prev_page }
-
- context 'when the page is not provided' do
- it { is_expected.to be_nil }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
-
- it { is_expected.to be(1) }
+ context 'with no vulnerability flags present' do
+ it 'does not have any vulnerability flag' do
+ expect(findings).to all(have_attributes(vulnerability_flags: be_empty))
end
end
- describe '#vulnerability_flags' do
+ context 'with some vulnerability flags present' 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
- allow_next_instance_of(Gitlab::Ci::Reports::Security::Finding) do |finding|
- allow(finding).to receive(:flags).and_return([create(:ci_reports_security_flag)]) if finding.report_type == 'sast'
- end
- 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(&:uuid) }
-
- context 'with the default parameters' do
- let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the uuid is provided' do
- let(:uuid) { Security::Finding.pick(:uuid) }
- let(:params) do
- {
- uuid: uuid
- }
- end
-
- it { is_expected.to match_array([uuid]) }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
- let(:expected_uuids) { Security::Finding.pluck(:uuid)[4..5] }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the per_page is provided' do
- let(:per_page) { 1 }
- let(:expected_uuids) { [Security::Finding.pick(:uuid)] }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the `severity_levels` is provided' do
- let(:severity_levels) { [:medium] }
- let(:expected_uuids) { Security::Finding.where(severity: 'medium').pluck(:uuid) }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the `confidence_levels` is provided' do
- let(:confidence_levels) { [:low] }
- let(:expected_uuids) { Security::Finding.where(confidence: 'low' ).pluck(:uuid) }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the `report_types` is provided' do
- let(:report_types) { :dependency_scanning }
- let(:expected_uuids) do
- Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) -
- [Security::Finding.second[:uuid]]
+ allow_next_instance_of(Gitlab::Ci::Reports::Security::Finding) do |finding|
+ allow(finding).to receive(:flags).and_return([create(:ci_reports_security_flag)]) if finding.report_type == 'sast'
end
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the `scope` is provided as `all`' do
- let(:scope) { 'all' }
-
- let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] }
-
- it { is_expected.to match_array(expected_uuids) }
end
- context 'when there is a retried build' do
- let(:retried_build) { create(:ci_build, :success, :retried, name: 'dependency_scanning', pipeline: pipeline) }
- let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning, job: retried_build) }
- let(:report) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) }
- let(:report_types) { :dependency_scanning }
- let(:expected_uuids) do
- Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) -
- [Security::Finding.second[:uuid]]
- end
-
- before do
- retried_content = File.read(artifact.file.path)
- Gitlab::Ci::Parsers::Security::DependencyScanning.parse!(retried_content, report)
- report.merge!(report)
-
- scan = create(:security_scan, scan_type: retried_build.name, build: retried_build, latest: false)
-
- report.findings.each_with_index do |finding, index|
- create(:security_finding,
- severity: finding.severity,
- confidence: finding.confidence,
- uuid: finding.uuid,
- deduplicated: true,
- scan: scan)
- end
- end
-
- it { is_expected.to match_array(expected_uuids) }
+ it 'has some vulnerability_findings with vulnerability flag' do
+ expect(findings).to include(have_attributes(vulnerability_flags: be_present))
end
- context 'when the `security_findings` records have `overridden_uuid`s' do
- let(:security_findings) { Security::Finding.by_build_ids(build_1) }
-
- let(:expected_uuids) do
- Security::Finding.where(overridden_uuid: nil).pluck(:uuid)
- .concat(Security::Finding.where.not(overridden_uuid: nil).pluck(:overridden_uuid)) -
- [Security::Finding.second[:overridden_uuid]]
- end
+ it 'does not have any vulnerability_flag if license is not available' do
+ stub_licensed_features(sast_fp_reduction: false)
- before do
- security_findings.each do |security_finding|
- security_finding.update!(overridden_uuid: security_finding.uuid, uuid: SecureRandom.uuid)
- end
- end
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when a build has more than one security report artifacts' do
- let(:report_types) { :secret_detection }
- let(:secret_detection_report) { create(:ci_reports_security_report, pipeline: pipeline, type: :secret_detection) }
- let(:expected_uuids) { secret_detection_report.findings.map(&:uuid) }
-
- before do
- scan = create(:security_scan, :latest_successful, scan_type: :secret_detection, build: build_2)
- artifact = create(:ee_ci_job_artifact, :secret_detection, job: build_2)
- report_content = File.read(artifact.file.path)
-
- Gitlab::Ci::Parsers::Security::SecretDetection.parse!(report_content, secret_detection_report)
-
- secret_detection_report.findings.each_with_index do |finding, index|
- create(:security_finding,
- severity: finding.severity,
- confidence: finding.confidence,
- uuid: finding.uuid,
- deduplicated: true,
- scan: scan)
- end
- end
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when a vulnerability already exist for a security finding' do
- let(:vulnerability) { create(:vulnerability, :with_finding, project: pipeline.project) }
-
- subject { finder_result.findings.map(&:vulnerability).first }
-
- before do
- vulnerability.finding.update_attribute(:uuid, Security::Finding.first.uuid)
- end
-
- describe 'the vulnerability is included in results' do
- it { is_expected.to eq(vulnerability) }
- end
+ expect(findings).to all(have_attributes(vulnerability_flags: be_empty))
end
end
end
diff --git a/ee/spec/finders/security/pure_findings_finder_spec.rb b/ee/spec/finders/security/pure_findings_finder_spec.rb
new file mode 100644
index 000000000000..c0228596584d
--- /dev/null
+++ b/ee/spec/finders/security/pure_findings_finder_spec.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::PureFindingsFinder, feature_category: :vulnerability_management do
+ it_behaves_like 'security findings finder' do
+ let(:findings) { finder_result.to_a }
+ let(:query_limit) { 8 }
+
+ describe 'parsing artifacts' do
+ before do
+ allow(::Gitlab::Ci::Parsers).to receive(:fabricate!)
+ end
+
+ it 'does not parse artifacts' do
+ service_object.execute
+
+ expect(::Gitlab::Ci::Parsers).not_to have_received(:fabricate!)
+ end
+ end
+ end
+end
diff --git a/ee/spec/models/security/finding_spec.rb b/ee/spec/models/security/finding_spec.rb
index 90fbe64923df..60a12d287582 100644
--- a/ee/spec/models/security/finding_spec.rb
+++ b/ee/spec/models/security/finding_spec.rb
@@ -2,16 +2,18 @@
require 'spec_helper'
-RSpec.describe Security::Finding do
+RSpec.describe Security::Finding, feature_category: :vulnerability_management do
let_it_be(:scan_1) { create(:security_scan, :latest_successful, scan_type: :sast) }
let_it_be(:scan_2) { create(:security_scan, :latest_successful, scan_type: :dast) }
- let_it_be(:finding_1) { create(:security_finding, scan: scan_1) }
- let_it_be(:finding_2) { create(:security_finding, scan: scan_2) }
+ let_it_be(:finding_1, refind: true) { create(:security_finding, scan: scan_1) }
+ let_it_be(:finding_2, refind: true) { create(:security_finding, scan: scan_2) }
describe 'associations' do
it { is_expected.to belong_to(:scan).required }
it { is_expected.to belong_to(:scanner).required }
+ it { is_expected.to belong_to(:vulnerability_read).class_name('Vulnerabilities::Read') }
it { is_expected.to have_one(:build).through(:scan) }
+ it { is_expected.to have_one(:vulnerability).through(:vulnerability_read) }
it {
is_expected.to have_many(:feedbacks)
@@ -148,16 +150,16 @@
end
describe '.ordered' do
- let_it_be(:finding_3) { create(:security_finding, severity: :critical, confidence: :confirmed) }
- let_it_be(:finding_4) { create(:security_finding, severity: :critical, confidence: :high) }
+ let_it_be(:finding_3) { create(:security_finding, severity: :critical) }
+ let_it_be(:finding_4) { create(:security_finding, severity: :critical) }
let(:expected_findings) { [finding_3, finding_4, finding_1, finding_2] }
subject { described_class.ordered }
before do
- finding_1.update!(severity: :high, confidence: :unknown)
- finding_2.update!(severity: :low, confidence: :confirmed)
+ finding_1.update!(severity: :high)
+ finding_2.update!(severity: :low)
end
it { is_expected.to eq(expected_findings) }
@@ -287,4 +289,126 @@
it { is_expected.to match(1) }
end
end
+
+ describe '#state' do
+ subject { finding_1.state }
+
+ context 'when there is no associated vulnerability' do
+ context 'when there is no associated dismissal feedback' do
+ it { is_expected.to eq('detected') }
+ end
+
+ context 'when there is an associated dismissal feedback' do
+ before do
+ create(:vulnerability_feedback, :dismissal, finding_uuid: finding_1.uuid)
+ end
+
+ it { is_expected.to eq('dismissed') }
+ end
+ end
+
+ context 'when there is an associated vulnerability' do
+ where(:state) { %i[detected confirmed dismissed resolved] }
+
+ before do
+ create(:vulnerabilities_finding, state, uuid: finding_1.uuid)
+ end
+
+ with_them do
+ it { is_expected.to eq(state.to_s) }
+ end
+ end
+ end
+
+ describe 'feedback accessors' do
+ shared_examples_for 'has feedback method for' do |type|
+ context 'when there is no associated dismissal feedback' do
+ it { is_expected.to be_nil }
+ end
+
+ context 'when there is an associated dismissal feedback' do
+ let!(:feedback) { create(:vulnerability_feedback, type, finding_uuid: finding_1.uuid) }
+
+ it { is_expected.to eq(feedback) }
+ end
+ end
+
+ describe '#dismissal_feedback' do
+ it_behaves_like 'has feedback method for', :dismissal do
+ subject { finding_1.dismissal_feedback }
+ end
+ end
+
+ describe '#issue_feedback' do
+ it_behaves_like 'has feedback method for', :issue do
+ subject { finding_1.issue_feedback }
+ end
+ end
+
+ describe '#merge_request_feedback' do
+ it_behaves_like 'has feedback method for', :merge_request do
+ subject { finding_1.merge_request_feedback }
+ end
+ end
+ end
+
+ describe 'attributes delegated to `finding_data`' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:attribute, :expected_value) do
+ :name | 'Test finding'
+ :description | 'Test description'
+ :solution | 'Test solution'
+ :location | 'Test location'
+ :identifiers | ['Test identifier']
+ :links | ['Test link']
+ :false_positive? | false
+ :assets | ['Test asset']
+ :evidence | {}
+ :details | []
+ :remediation_byte_offsets | { start_byte: 0, end_byte: 1 }
+ end
+
+ with_them do
+ let(:finding) { build(:security_finding) }
+
+ subject { finding.send(attribute) }
+
+ before do
+ finding.finding_data[attribute] = expected_value
+ end
+
+ it { is_expected.to eq(expected_value) }
+ end
+ end
+
+ describe '#remediations', :aggregate_failures do
+ let(:finding) { create(:security_finding, finding_data: finding_data) }
+ let(:mock_remediations) { [Object.new] }
+ let(:mock_proxy) { instance_double(Security::RemediationsProxy, by_byte_offsets: mock_remediations) }
+
+ subject(:remediations) { finding.remediations }
+
+ before do
+ allow(finding.scan).to receive(:remediations_proxy).and_return(mock_proxy)
+ end
+
+ context 'when the remediation byte offsets do not exist' do
+ let(:finding_data) { {} }
+
+ it 'does not call the proxy and returns an empty array' do
+ expect(remediations).to be_empty
+ expect(mock_proxy).not_to have_received(:by_byte_offsets)
+ end
+ end
+
+ context 'when the remediation byte offsets exist' do
+ let(:finding_data) { { remediation_byte_offsets: [{ start_byte: 0, end_byte: 10 }] } }
+
+ it 'delegates the call to the proxy' do
+ expect(remediations).to eq(mock_remediations)
+ expect(mock_proxy).to have_received(:by_byte_offsets)
+ end
+ end
+ end
end
diff --git a/ee/spec/models/security/remediations_proxy_spec.rb b/ee/spec/models/security/remediations_proxy_spec.rb
new file mode 100644
index 000000000000..97984c661f7b
--- /dev/null
+++ b/ee/spec/models/security/remediations_proxy_spec.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::RemediationsProxy, feature_category: :vulnerability_management do
+ let(:dast_artifact) { create(:ci_job_artifact, :common_security_report) }
+ let(:file) { dast_artifact.file }
+ let(:model) { described_class.new(file) }
+
+ describe '#by_byte_offsets' do
+ # The following byte offsets are collected by parsing the related artifact with Oj::Introspect.
+ # If the specs fail due to a change in the related artifact, you can collect them again by parsing
+ # the artifact again and checking the `:__oj_introspect` keys for remediations.
+ let(:remediation_1_byte_offsets) { [11842, 12008] }
+ let(:remediation_2_byte_offsets) { [12015, 12181] }
+ let(:remediation_3_byte_offsets) { remediation_2_byte_offsets }
+
+ subject(:data_fragments) do
+ model.by_byte_offsets([remediation_1_byte_offsets, remediation_2_byte_offsets, remediation_2_byte_offsets])
+ .map(&:deep_symbolize_keys)
+ end
+
+ before do
+ allow(file).to receive(:multi_read).and_call_original
+ end
+
+ it 'returns remediations by given byte offsets' do
+ expect(data_fragments).to eq(
+ [
+ {
+ diff: 'dG90YWxseSBsZWdpdCBkaWZm',
+ summary: 'this remediates CVE-2137',
+ fixes: [{ cve: 'CVE-2137' }]
+ },
+ {
+ diff: 'dG90YWxseSBsZWdpdCBkaWZm',
+ summary: 'this remediates CVE-2138',
+ fixes: [{ cve: 'CVE-2138' }]
+ },
+ {
+ diff: 'dG90YWxseSBsZWdpdCBkaWZm',
+ summary: 'this remediates CVE-2138',
+ fixes: [{ cve: 'CVE-2138' }]
+ }
+ ]
+ )
+ end
+
+ it 'delegates the call to GitlabUploader#multi_read with unique offsets' do
+ data_fragments
+
+ expect(file).to have_received(:multi_read).once.with([remediation_1_byte_offsets, remediation_2_byte_offsets])
+ end
+ end
+end
diff --git a/ee/spec/models/security/scan_spec.rb b/ee/spec/models/security/scan_spec.rb
index 46fc5ff80129..ed8688698be6 100644
--- a/ee/spec/models/security/scan_spec.rb
+++ b/ee/spec/models/security/scan_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Security::Scan do
+RSpec.describe Security::Scan, feature_category: :vulnerability_management do
it_behaves_like 'cleanup by a loose foreign key' do
let!(:model) { create(:security_scan) }
let(:parent) { model.build }
@@ -399,6 +399,21 @@
end
end
+ describe '#remediations_proxy' do
+ let(:mock_file) { instance_double(JobArtifactUploader) }
+ let(:scan) { create(:security_scan, :with_findings) }
+
+ subject { scan.remediations_proxy }
+
+ before do
+ allow_next_found_instance_of(Ci::JobArtifact) do |artifact|
+ allow(artifact).to receive(:file).and_return(mock_file)
+ end
+ end
+
+ it { is_expected.to be_an_instance_of(Security::RemediationsProxy).and have_attributes(file: mock_file) }
+ end
+
it_behaves_like 'having unique enum values'
it 'sets `project_id` and `pipeline_id` before save' do
diff --git a/ee/spec/models/vulnerabilities/read_spec.rb b/ee/spec/models/vulnerabilities/read_spec.rb
index 4817b082b0cd..55c400b3fa7a 100644
--- a/ee/spec/models/vulnerabilities/read_spec.rb
+++ b/ee/spec/models/vulnerabilities/read_spec.rb
@@ -2,13 +2,14 @@
require 'spec_helper'
-RSpec.describe Vulnerabilities::Read, type: :model do
+RSpec.describe Vulnerabilities::Read, type: :model, feature_category: :vulnerability_management do
let_it_be(:project) { create(:project) }
describe 'associations' do
it { is_expected.to belong_to(:vulnerability) }
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:scanner).class_name('Vulnerabilities::Scanner') }
+ it { is_expected.to have_many(:security_findings).class_name('Security::Finding') }
end
describe 'validations' do
diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb
index 1fc73e7889dc..8c377633f898 100644
--- a/ee/spec/requests/api/vulnerability_findings_spec.rb
+++ b/ee/spec/requests/api/vulnerability_findings_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe API::VulnerabilityFindings do
+RSpec.describe API::VulnerabilityFindings, feature_category: :vulnerability_management do
include AccessMatchersForRequest
let_it_be(:project) { create(:project, :public) }
@@ -11,24 +11,23 @@
describe 'GET /projects/:id/vulnerability_findings' do
let(:project_vulnerability_findings_path) { "/projects/#{project.id}/vulnerability_findings" }
- before do
- stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true)
-
- create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds, project: project)
- create(:ee_ci_job_artifact, :sast, job: build_sast, project: project)
- dismissal
- end
-
- let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) }
- let(:pipeline_without_vulnerabilities) { create(:ci_pipeline, status: :created, project: project) }
+ let_it_be(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) }
+ let_it_be(:pipeline_without_vulnerabilities) { create(:ci_pipeline, status: :created, project: project) }
- let(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) }
- let(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) }
+ let_it_be(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) }
+ let_it_be(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) }
let(:ds_report) { pipeline.security_reports.reports['dependency_scanning'] }
let(:sast_report) { pipeline.security_reports.reports['sast'] }
- let(:dismissal) do
+ before(:all) do
+ create(:security_scan, :latest_successful, :with_findings, scan_type: :dependency_scanning, build: build_ds)
+ create(:security_scan, :latest_successful, :with_findings, scan_type: :sast, build: build_sast)
+ end
+
+ before do
+ stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true)
+
create(:vulnerability_feedback, :dismissal, :sast,
project: project,
pipeline: pipeline,
@@ -36,6 +35,8 @@
vulnerability_data: sast_report.findings.first.raw_metadata,
finding_uuid: sast_report.findings.first.uuid
)
+
+ create(:vulnerabilities_finding, :dismissed, uuid: sast_report.findings.first.uuid)
end
context 'with an authorized user with proper permissions' do
@@ -80,45 +81,58 @@
end
describe 'using different finders' do
+ let(:utilize_finding_data?) { true }
+ let(:mock_findings_finder) { instance_double(Security::FindingsFinder, execute: []) }
+ let(:mock_pure_findings_finder) do
+ instance_double(Security::PureFindingsFinder,
+ execute: [], available?: pure_finder_available?)
+ end
+
before do
- allow(Security::PipelineVulnerabilitiesFinder).to receive(:new).and_call_original
+ stub_feature_flags(utilize_finding_data: utilize_finding_data?)
- allow_next_instance_of(Security::FindingsFinder) do |finder|
- allow(finder).to receive(:execute).and_return(mock_result)
- end
- end
+ allow(Security::FindingsFinder).to receive(:new).and_return(mock_findings_finder)
+ allow(Security::PureFindingsFinder).to receive(:new).and_return(mock_pure_findings_finder)
- context 'when the project uses `security_findings`' do
- let(:finding) { create(:vulnerability_finding) }
- let(:mock_result) { double(findings: [finding]) }
+ get api(project_vulnerability_findings_path, user), params: pagination
+ end
- it 'does not use `Security::PipelineVulnerabilitiesFinder`' do
- get api(project_vulnerability_findings_path, user), params: pagination
+ context 'when the `Security::PureFindingsFinder` is not available' do
+ let(:pure_finder_available?) { false }
- expect(Security::PipelineVulnerabilitiesFinder).not_to have_received(:new)
+ it 'uses the `Security::FindingsFinder`' do
+ expect(mock_pure_findings_finder).not_to have_received(:execute)
+ expect(mock_findings_finder).to have_received(:execute)
end
end
- context 'when the project does not use `security_findings`' do
- let(:mock_result) { nil }
+ context 'when the `Security::PureFindingsFinder` is available' do
+ let(:pure_finder_available?) { true }
- it 'fallsback to `Security::PipelineVulnerabilitiesFinder`' do
- get api(project_vulnerability_findings_path, user), params: pagination
-
- expect(Security::PipelineVulnerabilitiesFinder).to have_received(:new)
+ it 'uses the `Security::FindingsFinder`' do
+ expect(mock_pure_findings_finder).to have_received(:execute)
+ expect(mock_findings_finder).not_to have_received(:execute)
end
+ end
+
+ context 'when the `utilize_finding_data` feature flag is turned off' do
+ let(:utilize_finding_data?) { false }
- context 'when the `Security::PipelineVulnerabilitiesFinder` raises exception' do
- before do
- allow_next_instance_of(Security::PipelineVulnerabilitiesFinder) do |finder|
- allow(finder).to receive(:execute).and_raise(Security::PipelineVulnerabilitiesFinder::ParseError.new('failed'))
- end
+ context 'when the `Security::PureFindingsFinder` is not available' do
+ let(:pure_finder_available?) { false }
+
+ it 'uses the `Security::FindingsFinder`' do
+ expect(mock_pure_findings_finder).not_to have_received(:execute)
+ expect(mock_findings_finder).to have_received(:execute)
end
+ end
- it 'does not propagate the error to the client' do
- get api(project_vulnerability_findings_path, user), params: pagination
+ context 'when the `Security::PureFindingsFinder` is available' do
+ let(:pure_finder_available?) { true }
- expect(response).to have_gitlab_http_status(:ok)
+ it 'uses the `Security::FindingsFinder`' do
+ expect(mock_pure_findings_finder).not_to have_received(:execute)
+ expect(mock_findings_finder).to have_received(:execute)
end
end
end
diff --git a/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb b/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
new file mode 100644
index 000000000000..c126c5748e7e
--- /dev/null
+++ b/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
@@ -0,0 +1,343 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.shared_examples 'security findings finder' do
+ let_it_be(:pipeline) { create(:ci_pipeline) }
+ let_it_be(:build_1) { create(:ci_build, :success, name: 'dependency_scanning', pipeline: pipeline) }
+ let_it_be(:build_2) { create(:ci_build, :success, name: 'sast', pipeline: pipeline) }
+ let_it_be(:artifact_ds) { create(:ee_ci_job_artifact, :dependency_scanning, job: build_1) }
+ let_it_be(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_2) }
+ let_it_be(:report_ds) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) }
+ let_it_be(:report_sast) { create(:ci_reports_security_report, pipeline: pipeline, type: :sast) }
+
+ let(:severity_levels) { nil }
+ let(:confidence_levels) { nil }
+ let(:report_types) { nil }
+ let(:scope) { nil }
+ let(:page) { nil }
+ let(:per_page) { nil }
+ let(:service_object) { described_class.new(pipeline, params: params) }
+ let(:params) do
+ {
+ severity: severity_levels,
+ confidence: confidence_levels,
+ report_type: report_types,
+ scope: scope,
+ page: page,
+ per_page: per_page
+ }
+ end
+
+ describe '#execute' do
+ context 'when the pipeline does not have security findings' do
+ subject { service_object.execute }
+
+ it { is_expected.to be_empty }
+ end
+
+ context 'when the pipeline has security findings' do
+ let(:finder_result) { service_object.execute }
+
+ before(:all) do
+ ds_content = File.read(artifact_ds.file.path)
+ Gitlab::Ci::Parsers::Security::DependencyScanning.parse!(ds_content, report_ds)
+ report_ds.merge!(report_ds)
+ sast_content = File.read(artifact_sast.file.path)
+ Gitlab::Ci::Parsers::Security::Sast.parse!(sast_content, report_sast)
+ report_sast.merge!(report_sast)
+
+ findings = { artifact_ds => report_ds, artifact_sast => report_sast }.collect do |artifact, report|
+ scan = create(:security_scan, :latest_successful, scan_type: artifact.job.name, build: artifact.job)
+
+ report.findings.collect do |finding, index|
+ create(:security_finding,
+ severity: finding.severity,
+ confidence: finding.confidence,
+ uuid: finding.uuid,
+ deduplicated: true,
+ scan: scan)
+ end
+ end.flatten
+
+ findings.second.update!(deduplicated: false)
+
+ create(:vulnerability_feedback,
+ :dismissal,
+ project: pipeline.project,
+ category: :sast,
+ finding_uuid: findings.first.uuid)
+ end
+
+ before do
+ stub_licensed_features(sast: true, dependency_scanning: true)
+ end
+
+ describe 'N+1 queries' do
+ it 'does not cause N+1 queries' do
+ expect { finder_result }.not_to exceed_query_limit(query_limit)
+ end
+ end
+
+ describe '#current_page' do
+ subject { finder_result.current_page }
+
+ context 'when the page is not provided' do
+ it { is_expected.to be(1) }
+ end
+
+ context 'when the page is provided' do
+ let(:page) { 2 }
+
+ it { is_expected.to be(2) }
+ end
+ end
+
+ describe '#limit_value' do
+ subject { finder_result.limit_value }
+
+ context 'when the per_page is not provided' do
+ it { is_expected.to be(20) }
+ end
+
+ context 'when the per_page is provided' do
+ let(:per_page) { 100 }
+
+ it { is_expected.to be(100) }
+ end
+ end
+
+ describe '#total_pages' do
+ subject { finder_result.total_pages }
+
+ context 'when the per_page is not provided' do
+ it { is_expected.to be(1) }
+ end
+
+ context 'when the per_page is provided' do
+ let(:per_page) { 3 }
+
+ it { is_expected.to be(3) }
+ end
+ end
+
+ describe '#total_count' do
+ subject { finder_result.total_count }
+
+ context 'when the scope is not provided' do
+ let(:finding_to_dismiss) { Security::Finding.first }
+
+ before do
+ stub_feature_flags(deprecate_vulnerabilities_feedback: deprecate_vulnerabilities_feedback?)
+ end
+
+ context 'when the `deprecate_vulnerabilities_feedback` feature is enabled' do
+ let(:deprecate_vulnerabilities_feedback?) { true }
+
+ before do
+ create(:vulnerabilities_finding, :dismissed, uuid: finding_to_dismiss.uuid)
+ end
+
+ it { is_expected.to be(7) }
+ end
+
+ context 'when the `deprecate_vulnerabilities_feedback` feature is disabled' do
+ let(:deprecate_vulnerabilities_feedback?) { false }
+
+ before do
+ create(:vulnerability_feedback,
+ :dismissal,
+ project: pipeline.project,
+ category: :dependency_scanning,
+ finding_uuid: finding_to_dismiss.uuid)
+ end
+
+ it { is_expected.to be(7) }
+ end
+ end
+
+ context 'when the scope is provided as `all`' do
+ let(:scope) { 'all' }
+
+ it { is_expected.to be(8) }
+ end
+ end
+
+ describe '#next_page' do
+ subject { finder_result.next_page }
+
+ context 'when the page is not provided' do
+ # Limit per_page to force pagination on smaller dataset
+ let(:per_page) { 2 }
+
+ it { is_expected.to be(2) }
+ end
+
+ context 'when the page is provided' do
+ let(:page) { 2 }
+
+ it { is_expected.to be_nil }
+ end
+ end
+
+ describe '#prev_page' do
+ subject { finder_result.prev_page }
+
+ context 'when the page is not provided' do
+ it { is_expected.to be_nil }
+ end
+
+ context 'when the page is provided' do
+ let(:page) { 2 }
+ # Limit per_page to force pagination on smaller dataset
+ let(:per_page) { 2 }
+
+ it { is_expected.to be(1) }
+ end
+ end
+
+ describe '#findings' do
+ subject { findings.map(&:uuid) }
+
+ context 'with the default parameters' do
+ let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the uuid is provided' do
+ let(:uuid) { Security::Finding.pick(:uuid) }
+ let(:params) do
+ {
+ uuid: uuid
+ }
+ end
+
+ it { is_expected.to match_array([uuid]) }
+ end
+
+ context 'when the page is provided' do
+ let(:page) { 2 }
+ # Limit per_page to force pagination on smaller dataset
+ let(:per_page) { 2 }
+ let(:expected_uuids) { Security::Finding.pluck(:uuid)[4..5] }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the per_page is provided' do
+ let(:per_page) { 1 }
+ let(:expected_uuids) { [Security::Finding.pick(:uuid)] }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the `severity_levels` is provided' do
+ let(:severity_levels) { [:medium] }
+ let(:expected_uuids) { Security::Finding.where(severity: 'medium').pluck(:uuid) }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the `confidence_levels` is provided' do
+ let(:confidence_levels) { [:low] }
+ let(:expected_uuids) { Security::Finding.where(confidence: 'low').pluck(:uuid) }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the `report_types` is provided' do
+ let(:report_types) { :dependency_scanning }
+ let(:expected_uuids) do
+ Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) -
+ [Security::Finding.second[:uuid]]
+ end
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when the `scope` is provided as `all`' do
+ let(:scope) { 'all' }
+
+ let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] }
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when there is a retried build' do
+ let(:retried_build) { create(:ci_build, :success, :retried, name: 'dependency_scanning', pipeline: pipeline) }
+ let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning, job: retried_build) }
+ let(:report) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) }
+ let(:report_types) { :dependency_scanning }
+ let(:expected_uuids) do
+ Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) -
+ [Security::Finding.second[:uuid]]
+ end
+
+ before do
+ retried_content = File.read(artifact.file.path)
+ Gitlab::Ci::Parsers::Security::DependencyScanning.parse!(retried_content, report)
+ report.merge!(report)
+
+ scan = create(:security_scan, scan_type: retried_build.name, build: retried_build, latest: false)
+
+ report.findings.each_with_index do |finding, index|
+ create(:security_finding,
+ severity: finding.severity,
+ confidence: finding.confidence,
+ uuid: finding.uuid,
+ deduplicated: true,
+ scan: scan)
+ end
+ end
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when a build has more than one security report artifacts' do
+ let(:report_types) { :secret_detection }
+ let(:expected_uuids) { secret_detection_report.findings.map(&:uuid) }
+ let(:secret_detection_report) do
+ create(:ci_reports_security_report,
+ pipeline: pipeline,
+ type: :secret_detection)
+ end
+
+ before do
+ scan = create(:security_scan, :latest_successful, scan_type: :secret_detection, build: build_2)
+ artifact = create(:ee_ci_job_artifact, :secret_detection, job: build_2)
+ report_content = File.read(artifact.file.path)
+
+ Gitlab::Ci::Parsers::Security::SecretDetection.parse!(report_content, secret_detection_report)
+
+ secret_detection_report.findings.each_with_index do |finding, index|
+ create(:security_finding,
+ severity: finding.severity,
+ confidence: finding.confidence,
+ uuid: finding.uuid,
+ deduplicated: true,
+ scan: scan)
+ end
+ end
+
+ it { is_expected.to match_array(expected_uuids) }
+ end
+
+ context 'when a vulnerability already exist for a security finding' do
+ let!(:vulnerability_finding) do
+ create(:vulnerabilities_finding,
+ :detected,
+ uuid: Security::Finding.first.uuid,
+ project: pipeline.project)
+ end
+
+ subject { findings.map(&:vulnerability).first }
+
+ describe 'the vulnerability is included in results' do
+ it { is_expected.to eq(vulnerability_finding.vulnerability) }
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb
index db70441aaf56..f62ab7266319 100644
--- a/spec/uploaders/gitlab_uploader_spec.rb
+++ b/spec/uploaders/gitlab_uploader_spec.rb
@@ -6,7 +6,7 @@
RSpec.describe GitlabUploader do
let(:uploader_class) { Class.new(described_class) }
- subject { uploader_class.new(double) }
+ subject(:uploader) { uploader_class.new(double) }
describe '#file_storage?' do
context 'when file storage is used' do
@@ -161,6 +161,19 @@
end
end
+ describe '#multi_read' do
+ let(:file) { fixture_file_upload('spec/fixtures/trace/sample_trace', 'text/plain') }
+ let(:byte_offsets) { [[4, 10], [17, 29]] }
+
+ subject { uploader.multi_read(byte_offsets) }
+
+ before do
+ uploader.store!(file)
+ end
+
+ it { is_expected.to eq(%w[Running gitlab-runner]) }
+ end
+
describe '.version' do
subject { uploader_class.version }
--
GitLab