From 9b8793313c3349368f7d75ef67910cfd19b605e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Thu, 16 Oct 2025 20:17:43 +0200 Subject: [PATCH 1/4] Enforce presence of target scans with merge request approval policies If the enforced scans are not present in the target pipeline, evaluate the approval rules as errored. --- ee/app/models/concerns/approval_rule_like.rb | 5 + .../security/scan_result_policy_violation.rb | 1 + .../fallback_behavior_tracking_service.rb | 2 +- .../update_approvals_service.rb | 80 ++++--- ...approval_policies_enforce_target_scans.yml | 10 + .../policy_violation_details.rb | 5 +- .../policy_violation_details_spec.rb | 14 ++ .../concerns/approval_rule_like_spec.rb | 16 ++ ...fallback_behavior_tracking_service_spec.rb | 20 +- .../update_approvals_service_spec.rb | 197 ++++++++++++++---- 10 files changed, 276 insertions(+), 74 deletions(-) create mode 100644 ee/config/feature_flags/gitlab_com_derisk/approval_policies_enforce_target_scans.yml diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index b55c942498d1f8..292e7d7f933165 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -173,6 +173,11 @@ def warn_mode_policy? approval_policy_rule&.security_policy&.warn_mode? || false end + def scanners_with_default_fallback + # `scanners: []` means that all supported scanners are used + scanners.presence || ApprovalProjectRule::SUPPORTED_SCANNERS + end + private def relation_exists?(relation, column:, value:) diff --git a/ee/app/models/security/scan_result_policy_violation.rb b/ee/app/models/security/scan_result_policy_violation.rb index db9fd9f959aa9b..8ba30175199279 100644 --- a/ee/app/models/security/scan_result_policy_violation.rb +++ b/ee/app/models/security/scan_result_policy_violation.rb @@ -54,6 +54,7 @@ class ScanResultPolicyViolation < ApplicationRecord ERRORS = { scan_removed: 'SCAN_REMOVED', + target_scan_missing: 'TARGET_SCAN_MISSING', target_pipeline_missing: 'TARGET_PIPELINE_MISSING', artifacts_missing: 'ARTIFACTS_MISSING', evaluation_skipped: 'EVALUATION_SKIPPED', diff --git a/ee/app/services/security/scan_result_policies/fallback_behavior_tracking_service.rb b/ee/app/services/security/scan_result_policies/fallback_behavior_tracking_service.rb index 5016e735fe5a2f..addfe1e94aa5c5 100644 --- a/ee/app/services/security/scan_result_policies/fallback_behavior_tracking_service.rb +++ b/ee/app/services/security/scan_result_policies/fallback_behavior_tracking_service.rb @@ -60,7 +60,7 @@ def rule_failed_open?(approval_rule) # * there can't be any security scans # * the rule relates to a security scan present for the target branch, but absent for the source branch return true unless head_pipeline&.can_store_security_reports? - return true if approval_rule.scan_finding? && update_approvals_service.scan_removed?(approval_rule) + return true if approval_rule.scan_finding? && update_approvals_service.scan_missing?(approval_rule) false end diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb index 8c16768bf044c2..cc4bc867764908 100644 --- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb +++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb @@ -31,8 +31,8 @@ def execute evaluation.save end - def scan_removed?(approval_rule) - missing_scans(approval_rule).any? + def scan_missing?(approval_rule) + missing_source_scans(approval_rule).any? || missing_target_scans(approval_rule).any? end private @@ -62,34 +62,17 @@ def evaluate_rules(approval_rules) log_update_approval_rule('Evaluating scan_finding rules from approval policies', **validation_context) approval_rules.each do |merge_request_approval_rule| - approval_rule = merge_request_approval_rule.try(:source_rule) || merge_request_approval_rule - - if scan_removed?(approval_rule) - unless fail_open?(approval_rule) - log_update_approval_rule( - 'Updating MR approval rule', - reason: 'Scanner removed by MR', - approval_rule_id: approval_rule.id, - approval_rule_name: approval_rule.name, - missing_scans: missing_scans(approval_rule) - ) - end - - evaluation.error!( - merge_request_approval_rule, :scan_removed, - context: validation_context(approval_rule), missing_scans: missing_scans(approval_rule) - ) - next true - end + next if enforce_scans_presence!(merge_request_approval_rule) + approval_rule = merge_request_approval_rule.try(:source_rule) || merge_request_approval_rule violation_result = violates_approval_rule?(approval_rule) if violation_result.violated log_update_approval_rule( 'Updating MR approval rule', reason: 'scan_finding rule violated', - approval_rule_id: approval_rule.id, - approval_rule_name: approval_rule.name + approval_rule_id: merge_request_approval_rule.id, + approval_rule_name: merge_request_approval_rule.name ) fail_evaluation_with_data!(merge_request_approval_rule, newly_detected: violation_result.newly_detected, @@ -101,6 +84,42 @@ def evaluate_rules(approval_rules) end end + def enforce_scans_presence!(approval_rule) + source_scans_diff = missing_source_scans(approval_rule) + if source_scans_diff.any? + handle_scanner_mismatch_error( + approval_rule, :scan_removed, 'Scanner removed by MR', source_scans_diff + ) + return true + end + + target_scans_diff = missing_target_scans(approval_rule) + if target_scans_diff.any? + handle_scanner_mismatch_error( + approval_rule, :target_scan_missing, 'Enforced scanner missing on target branch', target_scans_diff + ) + return true + end + + false + end + + def handle_scanner_mismatch_error(approval_rule, reason, reason_msg, missing_scans) + unless fail_open?(approval_rule) + log_update_approval_rule( + 'Updating MR approval rule', + reason: reason_msg, + approval_rule_id: approval_rule.id, + approval_rule_name: approval_rule.name, + missing_scans: missing_scans + ) + end + + evaluation.error!( + approval_rule, reason, context: validation_context(approval_rule), missing_scans: missing_scans + ) + end + def log_update_approval_rule(message, **attributes) log_policy_evaluation('update_approvals', message, project: project, merge_request_id: merge_request.id, merge_request_iid: merge_request.iid, **attributes) @@ -142,15 +161,22 @@ def violates_vulnerabilities_allowed?(approval_rule, pipeline_uuids) ) end - def missing_scans(approval_rule) - scanners = approval_rule.scanners + def missing_source_scans(approval_rule) + scanners = approval_rule.scanners_with_default_fallback + # No target pipeline, report specified scanners that are not on the source branch as missing return (scanners - pipeline_security_scan_types) unless target_pipeline(approval_rule) + # Target pipeline has scans, but some may be missing on the source branch scan_types_diff = target_pipeline_security_scan_types(approval_rule) - pipeline_security_scan_types + # Return the diff for specified scanners + scan_types_diff & scanners + end - return scan_types_diff if scanners.empty? + def missing_target_scans(approval_rule) + return [] if ::Feature.disabled?(:approval_policies_enforce_target_scans, project) - scan_types_diff & scanners + scanners = approval_rule.scanners_with_default_fallback + scanners - target_pipeline_security_scan_types(approval_rule) end def pipeline_security_scan_types diff --git a/ee/config/feature_flags/gitlab_com_derisk/approval_policies_enforce_target_scans.yml b/ee/config/feature_flags/gitlab_com_derisk/approval_policies_enforce_target_scans.yml new file mode 100644 index 00000000000000..5663b8a2ed7e59 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/approval_policies_enforce_target_scans.yml @@ -0,0 +1,10 @@ +--- +name: approval_policies_enforce_target_scans +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514201 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209314 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/577681 +milestone: '18.6' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/lib/security/scan_result_policies/policy_violation_details.rb b/ee/lib/security/scan_result_policies/policy_violation_details.rb index b41bb26e5979b2..677aa698d27a08 100644 --- a/ee/lib/security/scan_result_policies/policy_violation_details.rb +++ b/ee/lib/security/scan_result_policies/policy_violation_details.rb @@ -20,6 +20,8 @@ class PolicyViolationDetails 'UNKNOWN' => 'Unknown error: %{error}', 'SCAN_REMOVED' => 'There is a mismatch between the scans of the source and target pipelines. ' \ 'The following scans are missing: %{scans}', + 'TARGET_SCAN_MISSING' => 'The enforced scans could not be found in the target pipelines. ' \ + 'The following scans are missing: %{scans}', 'TARGET_PIPELINE_MISSING' => 'Pipeline configuration error: SBOM reports required by policy `%{policy}` ' \ 'could not be found on the target branch.', 'ARTIFACTS_MISSING' => { @@ -40,6 +42,7 @@ class PolicyViolationDetails # These messages correspond to the possible errors above and are shown when a policy is configured to fail open FAIL_OPEN_MESSAGES = { 'SCAN_REMOVED' => 'Confirm that all scanners from the target branch are present on the source branch.', + 'TARGET_SCAN_MISSING' => 'Confirm that all enforced scanners are present on the target branch.', 'TARGET_PIPELINE_MISSING' => 'Confirm that dependency scanning is properly configured on the target branch ' \ 'pipeline and is producing results. License scanning depends on SBOM reports.', 'ARTIFACTS_MISSING' => { @@ -336,7 +339,7 @@ def license_spdx(licenses) def error_message(violation, error) error_key = error['error'] params = case error_key - when 'SCAN_REMOVED' + when 'SCAN_REMOVED', 'TARGET_SCAN_MISSING' { scans: error['missing_scans']&.map(&:humanize)&.join(', ') } when 'ARTIFACTS_MISSING' { policy: violation.name, report_type: violation.report_type } diff --git a/ee/spec/lib/security/scan_result_policies/policy_violation_details_spec.rb b/ee/spec/lib/security/scan_result_policies/policy_violation_details_spec.rb index 19d95eeca28013..bf560bf7c0e04f 100644 --- a/ee/spec/lib/security/scan_result_policies/policy_violation_details_spec.rb +++ b/ee/spec/lib/security/scan_result_policies/policy_violation_details_spec.rb @@ -974,6 +974,20 @@ def build_violation_details(policy, data, status = :failed) end end + context 'with TARGET_SCAN_MISSING error' do + let_it_be(:violation1) do + build_violation_with_error(policy1, + Security::ScanResultPolicyViolation::ERRORS[:target_scan_missing], 'missing_scans' => %w[secret_detection]) + end + + it 'returns associated error messages' do + expect(errors.pluck(:message)).to contain_exactly( + 'The enforced scans could not be found in the target pipelines. ' \ + 'The following scans are missing: Secret detection' + ) + end + end + context 'with TARGET_PIPELINE_MISSING error' do let_it_be(:violation1) do build_violation_with_error(policy1, Security::ScanResultPolicyViolation::ERRORS[:target_pipeline_missing]) diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index b606d8c9be52d6..96927a2cffd468 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -399,6 +399,22 @@ def self.scope(*); end end end + describe '#scanners_with_default_fallback' do + let(:specified_scanners) { %w[dependency_scanning secret_detection] } + + before do + subject.scanners = specified_scanners + end + + it { expect(subject.scanners_with_default_fallback).to eq(specified_scanners) } + + context 'when scanners are empty' do + let(:specified_scanners) { [] } + + it { expect(subject.scanners_with_default_fallback).to eq(ApprovalProjectRule::SUPPORTED_SCANNERS) } + end + end + describe 'validation' do context 'when value is too big' do it 'is invalid' do diff --git a/ee/spec/services/security/scan_result_policies/fallback_behavior_tracking_service_spec.rb b/ee/spec/services/security/scan_result_policies/fallback_behavior_tracking_service_spec.rb index 6ae5cbbeb20d30..76fdf02c268e0f 100644 --- a/ee/spec/services/security/scan_result_policies/fallback_behavior_tracking_service_spec.rb +++ b/ee/spec/services/security/scan_result_policies/fallback_behavior_tracking_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Security::ScanResultPolicies::FallbackBehaviorTrackingService, "#execute", feature_category: :security_policy_management do - let_it_be(:project) { create(:project) } + let_it_be(:project) { create(:project, :repository) } let(:service) { described_class.new(merge_request) } let_it_be(:merge_request) { create(:merge_request, source_project: project) } let_it_be(:user) { create(:user) } @@ -49,17 +49,27 @@ end shared_context "with report" do - let_it_be(:pipeline) do + let_it_be(:source_pipeline) do create(:ee_ci_pipeline, :success, :with_dependency_scanning_report, project: project, ref: merge_request.source_branch, + sha: merge_request.diff_head_sha, merge_requests_as_head_pipeline: [merge_request]) end - let_it_be(:scan) do - create(:security_scan, scan_type: 'dependency_scanning', pipeline: pipeline) + let_it_be_with_refind(:target_pipeline) do + create(:ee_ci_pipeline, :success, :with_dependency_scanning_report, project: project, + ref: merge_request.target_branch, sha: merge_request.diff_base_sha) + end + + let_it_be(:source_scan) do + create(:security_scan, scan_type: 'dependency_scanning', pipeline: source_pipeline) + end + + let_it_be(:target_scan) do + create(:security_scan, scan_type: 'dependency_scanning', pipeline: target_pipeline) end before do @@ -144,7 +154,7 @@ before do allow_next_instance_of(Security::ScanResultPolicies::UpdateApprovalsService) do |service| - allow(service).to receive(:scan_removed?).with(rule).and_return(true) + allow(service).to receive(:scan_missing?).with(rule).and_return(true) end end diff --git a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb index 116db847b74283..08f68bf591d3b5 100644 --- a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb +++ b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb @@ -124,6 +124,20 @@ end end + RSpec.shared_examples_for 'persists error in violation details' do + let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [target_pipeline.id] } } + + it 'persists violation details' do + execute + + expect(last_violation.violation_data) + .to match( + 'errors' => [expected_error], + 'context' => expected_context + ) + end + end + context 'without persisted policy' do let!(:report_approver_rule) { create(:report_approver_rule, :scan_finding, merge_request: merge_request) } @@ -214,16 +228,13 @@ execute end - it 'persists the error in violation data' do - execute - - expect(last_violation.violation_data) - .to eq('errors' => [{ + it_behaves_like 'persists error in violation details' do + let(:expected_error) do + { 'error' => Security::ScanResultPolicyViolation::ERRORS[:scan_removed], 'missing_scans' => ['dependency_scanning'] - }], 'context' => { - 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [target_pipeline.id] - }) + } + end end context 'when policy fails open' do @@ -300,6 +311,14 @@ context 'when scan type does not match the approval rule scanners' do let(:scanners) { %w[container_scanning] } + let_it_be(:target_scan_container_scanning) do + create(:security_scan, :succeeded, + project: project, + pipeline: target_pipeline, + scan_type: 'container_scanning' + ) + end + it_behaves_like 'sets approvals_required to 0' it_behaves_like 'triggers policy bot comment', false end @@ -383,31 +402,28 @@ it_behaves_like 'does not update approvals_required' it_behaves_like 'triggers policy bot comment', true - it_behaves_like 'persists violation details' do - let(:expected_violations) { { 'newly_detected' => array_including(uuids) } } + it_behaves_like 'persists error in violation details' do let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } } - end - - context 'when there are no newly detected findings' do - let_it_be_with_refind(:pipeline) do - create(:ee_ci_pipeline, :with_dependency_scanning_report, - project: project, - ref: merge_request.source_branch, - sha: merge_request.diff_head_sha) + let(:expected_error) do + { + 'error' => Security::ScanResultPolicyViolation::ERRORS[:target_scan_missing], + 'missing_scans' => ['dependency_scanning'] + } end + end + context 'when feature flag "approval_policies_enforce_target_scans" is disabled' do before do - create(:security_scan, :succeeded, - project: project, - pipeline: pipeline, - scan_type: 'dependency_scanning' - ) + stub_feature_flags(approval_policies_enforce_target_scans: false) end - it_behaves_like 'sets approvals_required to 0' + it_behaves_like 'persists violation details' do + let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } } + let(:expected_violations) { { 'newly_detected' => array_including(uuids) } } + end end - context 'with missing scan' do + context 'with missing scan in the source pipeline' do before do report_approver_rule.update!(scanners: %i[container_scanning]) end @@ -415,16 +431,14 @@ it_behaves_like 'does not update approvals_required' it_behaves_like 'triggers policy bot comment', true - it 'persists the error in violation data' do - execute - - expect(last_violation.violation_data) - .to eq('errors' => [{ + it_behaves_like 'persists error in violation details' do + let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } } + let(:expected_error) do + { 'error' => Security::ScanResultPolicyViolation::ERRORS[:scan_removed], 'missing_scans' => ['container_scanning'] - }], - 'context' => { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } - ) + } + end end end end @@ -842,16 +856,32 @@ end context 'when target pipeline is nil' do - let_it_be(:merge_request) do + let_it_be_with_refind(:merge_request) do create(:merge_request, source_project: project, target_project: project, source_branch: 'feature', target_branch: 'target-branch') end it_behaves_like 'does not update approvals_required' it_behaves_like 'triggers policy bot comment', true - it_behaves_like 'persists violation details' do - let(:expected_violations) { { 'newly_detected' => array_including(uuids) } } + it_behaves_like 'persists error in violation details' do let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } } + let(:expected_error) do + { + 'error' => Security::ScanResultPolicyViolation::ERRORS[:target_scan_missing], + 'missing_scans' => ['dependency_scanning'] + } + end + end + + context 'when feature flag "approval_policies_enforce_target_scans" is disabled' do + before do + stub_feature_flags(approval_policies_enforce_target_scans: false) + end + + it_behaves_like 'persists violation details' do + let(:expected_violations) { { 'newly_detected' => array_including(uuids) } } + let(:expected_context) { { 'pipeline_ids' => [pipeline.id], 'target_pipeline_ids' => [] } } + end end end end @@ -979,12 +1009,13 @@ create(:scan_result_policy_read, project: project, vulnerability_attributes: { fix_available: true }) end - let_it_be(:approval_rule) do - create(:approval_project_rule, :scan_finding, project: project, scan_result_policy_read: policy) + let!(:approval_rule) do + create(:approval_project_rule, :scan_finding, project: project, scanners: scanners, + scan_result_policy_read: policy) end - let_it_be(:mr_rule) do - create(:approval_merge_request_rule, :scan_finding, merge_request: merge_request, + let!(:mr_rule) do + create(:approval_merge_request_rule, :scan_finding, scanners: scanners, merge_request: merge_request, approval_project_rule: approval_rule) end @@ -1016,6 +1047,92 @@ end end + describe '#scan_missing?' do + let(:scanners) { %w[dependency_scanning] } + + let_it_be(:project) { create(:project, :repository) } + let_it_be_with_refind(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:pipeline) do + create(:ee_ci_pipeline, :success, :with_dependency_scanning_report, project: project, + ref: merge_request.source_branch, sha: merge_request.diff_head_sha) + end + + let_it_be_with_refind(:target_pipeline) do + create(:ee_ci_pipeline, :success, :with_dependency_scanning_report, project: project, + ref: merge_request.target_branch, sha: merge_request.diff_base_sha) + end + + let_it_be_with_refind(:source_scan) do + create(:security_scan, :succeeded, project: project, pipeline: pipeline, scan_type: 'dependency_scanning') + end + + let_it_be_with_refind(:target_scan) do + create(:security_scan, :succeeded, + project: project, + pipeline: target_pipeline, + scan_type: 'dependency_scanning') + end + + let!(:approval_rule) do + create(:report_approver_rule, :scan_finding, merge_request: merge_request, scanners: scanners) + end + + subject(:scan_missing?) do + described_class.new(merge_request: merge_request, pipeline: pipeline).scan_missing?(approval_rule) + end + + context 'with both source and target pipeline scans present' do + it { is_expected.to be(false) } + end + + context 'with only source pipeline scan' do + before do + target_scan.destroy! + end + + it { is_expected.to be(true) } + + context 'when feature flag "approval_policies_enforce_target_scans" is disabled' do + before do + stub_feature_flags(approval_policies_enforce_target_scans: false) + end + + it { is_expected.to be(false) } + end + end + + context 'with only target pipeline scan' do + before do + source_scan.destroy! + end + + it { is_expected.to be(true) } + end + + context 'when more scanners are enforced' do + let(:scanners) { %w[dependency_scanning container_scanning] } + + it { is_expected.to be(true) } + + context 'when feature flag "approval_policies_enforce_target_scans" is disabled' do + before do + stub_feature_flags(approval_policies_enforce_target_scans: false) + end + + it { is_expected.to be(false) } + end + end + + context 'without any scans in the source or target pipeline' do + before do + target_scan.destroy! + source_scan.destroy! + end + + it { is_expected.to be(true) } + end + end + def create_findings_with_vulnerabilities(scan, uuids) uuids.each do |uuid| create(:security_finding, scan: scan, scanner: scanner, severity: 'high', uuid: uuid) -- GitLab From a1eca3f83e76cfab2c0f167a6f8b0981d07483d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Mon, 20 Oct 2025 14:42:45 +0200 Subject: [PATCH 2/4] Rename method to make it clearer --- .../scan_result_policies/update_approvals_service.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb index cc4bc867764908..51ad643174c51b 100644 --- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb +++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb @@ -32,7 +32,7 @@ def execute end def scan_missing?(approval_rule) - missing_source_scans(approval_rule).any? || missing_target_scans(approval_rule).any? + target_scans_missing_in_source(approval_rule).any? || missing_target_scans(approval_rule).any? end private @@ -85,7 +85,7 @@ def evaluate_rules(approval_rules) end def enforce_scans_presence!(approval_rule) - source_scans_diff = missing_source_scans(approval_rule) + source_scans_diff = target_scans_missing_in_source(approval_rule) if source_scans_diff.any? handle_scanner_mismatch_error( approval_rule, :scan_removed, 'Scanner removed by MR', source_scans_diff @@ -161,7 +161,7 @@ def violates_vulnerabilities_allowed?(approval_rule, pipeline_uuids) ) end - def missing_source_scans(approval_rule) + def target_scans_missing_in_source(approval_rule) scanners = approval_rule.scanners_with_default_fallback # No target pipeline, report specified scanners that are not on the source branch as missing return (scanners - pipeline_security_scan_types) unless target_pipeline(approval_rule) -- GitLab From 3130b5522f5ff7ed77bf92269e0bf86fdf6682c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Wed, 22 Oct 2025 17:57:12 +0200 Subject: [PATCH 3/4] Fix QA specs --- ...scan_result_policy_vulnerabilities_spec.rb | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/qa/qa/specs/features/ee/browser_ui/18_security_risk_management/scan_result_policy_vulnerabilities_spec.rb b/qa/qa/specs/features/ee/browser_ui/18_security_risk_management/scan_result_policy_vulnerabilities_spec.rb index ac356c0f3cad73..0ab45e43d9940a 100644 --- a/qa/qa/specs/features/ee/browser_ui/18_security_risk_management/scan_result_policy_vulnerabilities_spec.rb +++ b/qa/qa/specs/features/ee/browser_ui/18_security_risk_management/scan_result_policy_vulnerabilities_spec.rb @@ -50,6 +50,14 @@ module QA end end + let(:target_branch_report_commit) do + create_commit(branch_name: project.default_branch, actions: [ + ci_file(report_name: premade_report_name, action: 'create'), + report_file(report_name: premade_report_name, report_path: premade_report_path, severity: 'High', + action: 'create') + ]) + end + before do project.add_member(approver) QA::Support::Waiter.wait_until(sleep_interval: 1, @@ -62,6 +70,10 @@ module QA end Flow::Login.sign_in + + # Create a target branch report for the approval policy to match against + target_branch_report_commit + project.visit! end @@ -81,8 +93,10 @@ module QA create_scan_result_policy # Create a branch and a commit to trigger a pipeline to generate container scanning findings - create_commit(branch_name: commit_branch, report_name: premade_report_name, - report_path: premade_report_path, severity: "Critical") + create_commit(branch_name: commit_branch, actions: [ + report_file(report_name: premade_report_name, report_path: premade_report_path, + severity: 'Critical', action: 'update') + ]) merge_request = create_test_mr Flow::Pipeline.wait_for_latest_pipeline(status: 'Passed', wait: 90) @@ -103,8 +117,10 @@ module QA create_scan_result_policy # Create a branch and a commit to trigger a pipeline to generate container scanning findings - create_commit(branch_name: commit_branch, report_name: premade_report_name, - report_path: premade_report_path, severity: "High") + create_commit(branch_name: commit_branch, actions: [ + report_file(report_name: premade_report_name, report_path: premade_report_path, + severity: 'Medium', action: 'update') + ]) merge_request = create_test_mr Flow::Pipeline.wait_for_latest_pipeline(status: 'Passed') @@ -116,9 +132,9 @@ module QA end end - def ci_file(report_name) + def ci_file(report_name:, action:) { - action: 'create', + action: action, file_path: '.gitlab-ci.yml', content: <<~YAML include: @@ -156,31 +172,29 @@ def create_test_mr source_branch: commit_branch) end - def report_file(report_name:, report_path:, severity:) + def report_file(report_name:, report_path:, severity:, action:) { - action: 'create', + action: action, file_path: report_name.to_s, content: container_scanning_report_content(report_path, severity) } end def container_scanning_report_content(report_path, severity) - if severity == "High" + if severity != "Critical" File.read(report_path.to_s).gsub("Critical", severity) else File.read(report_path.to_s) end end - def create_commit(branch_name:, report_name:, report_path:, severity:) + def create_commit(branch_name:, actions:) create(:commit, project: project, start_branch: project.default_branch, branch: branch_name, commit_message: 'Add premade container scanning report', - actions: [ - ci_file(report_name), report_file(report_name: report_name, report_path: report_path, severity: severity) - ]) + actions: actions) end end end -- GitLab From d8bf7a108be461b6968e4509f5c116aa27c0dea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Wed, 22 Oct 2025 19:35:56 +0200 Subject: [PATCH 4/4] Fix feature specs --- ...er_sees_security_policy_rules_scan_findings_spec.rb | 10 +++++++++- .../helpers/features/security_policy_helpers.rb | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/ee/spec/features/merge_request/user_sees_security_policy_rules_scan_findings_spec.rb b/ee/spec/features/merge_request/user_sees_security_policy_rules_scan_findings_spec.rb index 8a32cfc97591dd..50e29f145c168f 100644 --- a/ee/spec/features/merge_request/user_sees_security_policy_rules_scan_findings_spec.rb +++ b/ee/spec/features/merge_request/user_sees_security_policy_rules_scan_findings_spec.rb @@ -32,7 +32,6 @@ let_it_be(:approver) { create(:user, maintainer_of: project) } let_it_be(:approver_roles) { ['maintainer'] } let!(:protected_branch) { create(:protected_branch, project: project, name: merge_request.target_branch) } - let!(:pipeline) { nil } let(:vuln_states) { [] } let(:policy_rule) do { @@ -66,6 +65,15 @@ end end + let!(:target_pipeline) do + create(:ee_ci_pipeline, :success, :with_sast_report, + project: project, ref: project.default_branch, sha: merge_request.diff_base_sha) + end + + let!(:target_pipeline_scan) do + create(:security_scan, :succeeded, project: project, pipeline: target_pipeline, scan_type: 'sast') + end + before do sign_in(user) end diff --git a/ee/spec/support/helpers/features/security_policy_helpers.rb b/ee/spec/support/helpers/features/security_policy_helpers.rb index a98f4151073502..d0fa9800a0bd6a 100644 --- a/ee/spec/support/helpers/features/security_policy_helpers.rb +++ b/ee/spec/support/helpers/features/security_policy_helpers.rb @@ -11,7 +11,6 @@ def create_security_policy end def create_policy_setup - stub_feature_flags(custom_software_license: false) stub_licensed_features(security_dashboard: true, multiple_approval_rules: true, sast: true, report_approver_rules: true, -- GitLab