diff --git a/doc/user/application_security/policies/merge_request_approval_policies.md b/doc/user/application_security/policies/merge_request_approval_policies.md index 2a6570a88052749776a6c9368d938af1ff6ae339..936c336f3732c0035a14f90a58def6d60e94e7e4 100644 --- a/doc/user/application_security/policies/merge_request_approval_policies.md +++ b/doc/user/application_security/policies/merge_request_approval_policies.md @@ -430,6 +430,8 @@ On GitLab Self-Managed, by default the `fallback_behavior` field is available. T ## `policy_tuning` +### `unblock_rules_using_execution_policies` + {{< history >}} - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/498624) support for use in pipeline execution policies in GitLab 17.10 [with a flag](../../../administration/feature_flags/_index.md) named `unblock_rules_using_pipeline_execution_policies`. Enabled by default. @@ -447,9 +449,9 @@ The availability of support for pipeline execution policies is controlled by a f |--------|----------|----------|--------------------|----------------------------------------------------------------------------------------------------------------------| | `unblock_rules_using_execution_policies` | `boolean` | false | `true`, `false` | When enabled, approval rules do not block merge requests when a scan is required by a scan execution policy or a pipeline execution policy but a required scan artifact is missing from the target branch. This option only works when the project or group has an existing scan execution policy or pipeline execution policy with matching scanners. | -### Examples +#### Examples -#### Example of `policy_tuning` with a scan execution policy +##### Example of `policy_tuning` with a scan execution policy You can use this example in a `.gitlab/security-policies/policy.yml` file stored in a [security policy project](enforcement/security_policy_projects.md): @@ -495,7 +497,7 @@ approval_policy: unblock_rules_using_execution_policies: true ``` -#### Example of `policy_tuning` with a pipeline execution policy +##### Example of `policy_tuning` with a pipeline execution policy {{< alert type="warning" >}} @@ -529,7 +531,7 @@ include: - template: Jobs/Dependency-Scanning.gitlab-ci.yml ``` -##### Recreate pipeline execution policies created before GitLab 17.10 +###### Recreate pipeline execution policies created before GitLab 17.10 Pipeline execution policies created before GitLab 17.10 do not contain the data required to use the `policy_tuning` feature. To use this feature with older pipeline execution policies, @@ -552,6 +554,18 @@ To recreate a pipeline execution policy: 1. In the **.YAML mode**, paste the contents of the old policy. 1. Select **Update via merge request** and merge the generated merge request. +### `security_report_time_window` + +{{< history >}} + +- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/525509) in GitLab 18.4 [with a flag](../../../administration/feature_flags/_index.md) named `approval_policy_time_window`. + +{{< /history >}} + +| Field | Type | Required | Possible values | Description | +|--------|----------|----------|--------------------|----------------------------------------------------------------------------------------------------------------------| +| `security_report_time_window` | `integer` | false | 1 to 10080 (7 days) | Specifies the time window in minutes for choosing the target pipeline for the security report comparison. | + ## Policy scope schema To customize policy enforcement, you can define a policy's scope to either include or exclude diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue b/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue index c33227fbbc99ea195469a85f356eb38ac7f2837b..fab9943040fc9f54bda2669cf5161dd2f13bd67a 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue @@ -3,6 +3,8 @@ import { s__ } from '~/locale'; import { UNBLOCK_RULES_KEY, UNBLOCK_RULES_TEXT, + TIME_WINDOW_KEY, + TIME_WINDOW_TEXT, } from '../../policy_editor/scan_result/advanced_settings/constants'; export default { @@ -10,6 +12,7 @@ export default { i18n: { title: s__('SecurityOrchestration|Edge case settings'), [UNBLOCK_RULES_KEY]: UNBLOCK_RULES_TEXT, + [TIME_WINDOW_KEY]: TIME_WINDOW_TEXT, }, props: { settings: { @@ -20,7 +23,12 @@ export default { }, computed: { settingsList() { - return Object.entries(this.settings).map(([key]) => this.$options.i18n[key] || key); + return Object.entries(this.settings).map(([key, value]) => { + if (key === TIME_WINDOW_KEY && typeof value === 'number') { + return `${this.$options.i18n[key]}: ${value} ${value === 1 ? s__('ScanResultPolicy|minute') : s__('ScanResultPolicy|minutes')}`; + } + return this.$options.i18n[key] || key; + }); }, }, }; diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js index 76033508433451928eba406b76513b9cb40d891f..79f82525603e13343055f2473b9fc4d770560453 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js @@ -10,6 +10,16 @@ export const UNBLOCK_RULES_TEXT = s__( 'ScanResultPolicy|Make approval rules optional using execution policies', ); +export const TIME_WINDOW_KEY = 'security_report_time_window'; + +export const TIME_WINDOW_TEXT = s__( + 'ScanResultPolicy|Use time window for target pipeline comparison', +); + +export const TIME_WINDOW_POPOVER_DESC = s__( + 'ScanResultPolicy|This addresses issues in busy projects where the target branch always has a pipeline running. Instead of only using the latest pipeline (which might be pending), the policy will consider security reports from successful pipelines completed within the specified time window.', +); + export const NAME = 'name'; export const PATTERN = 'pattern'; export const SOURCE = 'source'; diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue index 7092830bb54bbe0d365505ffcb9641099b6964d5..5908b5a81b3ea40f585ba0ac32da89e5376e8710 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue @@ -1,26 +1,53 @@ @@ -52,5 +87,40 @@ export default { {{ $options.i18n.popoverDesc }} + +
+ + {{ $options.i18n.TIME_WINDOW_TEXT }} + + + + + {{ $options.i18n.TIME_WINDOW_POPOVER_DESC }} + + +
+ +
+ + {{ $options.i18n.timeWindowUnit }} +
+ +
+
diff --git a/ee/app/controllers/groups/security/policies_controller.rb b/ee/app/controllers/groups/security/policies_controller.rb index a4bb6d798c130560cd32540a2ce575cb7801516f..30a78cfc6486c4553a9525678dd64c95dd764a28 100644 --- a/ee/app/controllers/groups/security/policies_controller.rb +++ b/ee/app/controllers/groups/security/policies_controller.rb @@ -18,6 +18,7 @@ class PoliciesController < Groups::ApplicationController push_frontend_feature_flag(:scheduled_pipeline_execution_policies, group) push_frontend_feature_flag(:flexible_scan_execution_policy, group) push_frontend_feature_flag(:security_policies_combined_list, group) + push_frontend_feature_flag(:approval_policy_time_window, group) end feature_category :security_policy_management diff --git a/ee/app/controllers/projects/security/policies_controller.rb b/ee/app/controllers/projects/security/policies_controller.rb index 24f4398e1d3700bb1666f8cb742cb70eb2504640..d3ad117773a3a7853018e4223d62ceb3796aebdf 100644 --- a/ee/app/controllers/projects/security/policies_controller.rb +++ b/ee/app/controllers/projects/security/policies_controller.rb @@ -18,6 +18,7 @@ class PoliciesController < Projects::ApplicationController push_frontend_feature_flag(:security_policy_approval_warn_mode, project.group) push_frontend_feature_flag(:flexible_scan_execution_policy, project.group) push_frontend_feature_flag(:security_policies_combined_list, project) + push_frontend_feature_flag(:approval_policy_time_window, project) end feature_category :security_policy_management diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index b549fce927c8f430b7badfcaf1138b0021f8517e..4739dd5662d31111b8c77fc29dda59fff6697a8f 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -83,6 +83,12 @@ module ApprovalRuleLike scope :by_report_types, ->(report_types) { where(report_type: report_types) } end + def security_report_time_window + return unless approval_policy_rule + + approval_policy_rule.security_policy.security_report_time_window + end + def vulnerability_attribute_false_positive nil end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index c4a2027f4e9ce87ffa1bce44712ba9b2367231f5..80ab6956855b399d15134a861fc40d1e7ab8d05b 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -10,6 +10,7 @@ module MergeRequest include FromUnion MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON = 10 + MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON_TIME_WINDOW = 1000 prepended do include Elastic::ApplicationVersionedSearch @@ -576,6 +577,23 @@ def latest_scan_finding_comparison_pipeline find_target_branch_pipeline_by_sha_in_order_of_preference(target_shas, :has_security_reports?) end + def target_pipeline_for_approval_policy(approval_rule, report_type) + predicate = report_type == :scan_finding ? :has_security_reports? : :has_sbom_reports? + target_shas = [diff_head_pipeline&.target_sha, diff_base_sha, diff_start_sha] + pipelines = find_target_branch_pipelines_for(target_shas) + + comparison_pipeline = find_pipeline_with_report(pipelines, predicate) + time_window = approval_rule.security_report_time_window + + return comparison_pipeline if comparison_pipeline.present? + + if ::Feature.enabled?(:approval_policy_time_window, project) && time_window.present? + return find_pipeline_with_time_window(pipelines, time_window, report_type) + end + + comparison_pipeline || latest_pipeline_for_target_branch + end + def diff_head_pipeline?(pipeline) pipeline.source_sha == diff_head_sha || pipeline.sha == diff_head_sha end @@ -719,18 +737,43 @@ def security_comparison?(service_class) end def find_target_branch_pipeline_by_sha_in_order_of_preference(shas, predicate) - pipelines = shas - .compact - .lazy - .flat_map do |sha| - target_branch_pipelines_for(sha: sha) - .order(id: :desc) - .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON) - end + pipelines = find_target_branch_pipelines_for(shas) + find_pipeline_with_report(pipelines, predicate) + end + def find_pipeline_with_report(pipelines, predicate) pipelines.find { |pipeline| pipeline.self_and_project_descendants.any?(&predicate) } end + def find_target_branch_pipelines_for(shas) + shas.compact.lazy.flat_map do |sha| + target_branch_pipelines_for(sha: sha) + .order(id: :desc) + .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON) + end + end + + def find_pipeline_with_time_window(pipelines, time_window, report_type) + reports_scope = if report_type == :scan_finding + ::Ci::JobArtifact.security_reports + else + ::Ci::JobArtifact.of_report_type(:sbom) + end + + filtered_pipelines = project + .ci_pipelines + .with_reports(reports_scope) + .created_after(Time.current - (time_window * 60)) + .where(ref: target_branch) + + filtered_pipelines = filtered_pipelines.created_before_id(pipelines.first.id) if pipelines.present? + + filtered_pipelines + .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON_TIME_WINDOW) + .order(id: :desc) + .take(1) + end + def has_approved_license_check? if rule = approval_rules.license_compliance.last ApprovalWrappedRule.wrap(self, rule).approved? diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 67406a33b63370544c56e94f3884d3a49c3f5d51..9185fe7cc2aef02b55608167a769d4033c45908f 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -344,6 +344,10 @@ def supports_policy_rules? Security::PolicyRule::SUPPORTED_POLICY_TYPES.include?(type.to_sym) end + def security_report_time_window + policy_content.dig(:policy_tuning, :security_report_time_window) + end + private def content_by_type diff --git a/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb b/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb index d0c1948a43b05fbd8d6d7770fc36d874449b4098..ef5fa7e8052343e50b2560799a4bd7d74c617ea1 100644 --- a/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb +++ b/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb @@ -23,8 +23,8 @@ def related_pipelines(pipeline) end end - def related_target_pipeline_ids_for_merge_request(merge_request, report_type) - target_pipeline = target_pipeline_for_merge_request(merge_request, report_type) + def related_target_pipeline_ids_for_merge_request(merge_request, report_type, approval_rule) + target_pipeline = target_pipeline_for_merge_request(merge_request, report_type, approval_rule) return [] unless target_pipeline Security::RelatedPipelinesFinder.new(target_pipeline, { @@ -37,14 +37,8 @@ def related_pipeline_sources Enums::Ci::Pipeline.ci_and_security_orchestration_sources.values end - def target_pipeline_for_merge_request(merge_request, report_type) - target_pipeline = if report_type == :scan_finding - merge_request.latest_scan_finding_comparison_pipeline - elsif report_type == :license_scanning - merge_request.latest_comparison_pipeline_with_sbom_reports - end - - target_pipeline || merge_request.latest_pipeline_for_target_branch + def target_pipeline_for_merge_request(merge_request, report_type, approval_rule) + merge_request.target_pipeline_for_approval_policy(approval_rule, report_type) end end 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 ac338f8f20ecc10806776611b9d4d5df1dd67823..1b846456294c5b9aa518178d7e23dda53d86b763 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 @@ -48,8 +48,12 @@ def related_pipeline_with_security_reports_exists? related_pipelines(pipeline).with_reports(::Ci::JobArtifact.security_reports).exists? end - def validation_context - { pipeline_ids: related_source_pipeline_ids, target_pipeline_ids: related_target_pipeline_ids } + def validation_context(approval_rule = nil) + if approval_rule + { pipeline_ids: related_source_pipeline_ids, target_pipeline_ids: related_target_pipeline_ids(approval_rule) } + else + { pipeline_ids: related_source_pipeline_ids } + end end delegate :project, to: :pipeline @@ -73,7 +77,7 @@ def evaluate_rules(approval_rules) evaluation.error!( merge_request_approval_rule, :scan_removed, - context: validation_context, missing_scans: missing_scans(approval_rule) + context: validation_context(approval_rule), missing_scans: missing_scans(approval_rule) ) next true end @@ -140,9 +144,9 @@ def violates_vulnerabilities_allowed?(approval_rule, pipeline_uuids) def missing_scans(approval_rule) scanners = approval_rule.scanners - return (scanners - pipeline_security_scan_types) unless target_pipeline + return (scanners - pipeline_security_scan_types) unless target_pipeline(approval_rule) - scan_types_diff = target_pipeline_security_scan_types - pipeline_security_scan_types + scan_types_diff = target_pipeline_security_scan_types(approval_rule) - pipeline_security_scan_types return scan_types_diff if scanners.empty? @@ -154,15 +158,9 @@ def pipeline_security_scan_types end strong_memoize_attr :pipeline_security_scan_types - def target_pipeline_security_scan_types - security_scan_types(related_target_pipeline_ids) - end - strong_memoize_attr :target_pipeline_security_scan_types - - def target_pipeline - target_pipeline_for_merge_request(merge_request, :scan_finding) + def target_pipeline_security_scan_types(approval_rule) + security_scan_types(related_target_pipeline_ids(approval_rule)) end - strong_memoize_attr :target_pipeline def fail_evaluation_with_data!(rule, newly_detected: nil, previously_existing: nil) evaluation.fail!( @@ -173,7 +171,7 @@ def fail_evaluation_with_data!(rule, newly_detected: nil, previously_existing: n previously_existing: Security::ScanResultPolicyViolation.trim_violations(previously_existing) }.compact_blank }, - context: validation_context + context: validation_context(rule) ) end @@ -185,10 +183,13 @@ def security_scan_types(pipeline_ids) Security::Scan.by_pipeline_ids(pipeline_ids).distinct_scan_types end - def related_target_pipeline_ids - related_target_pipeline_ids_for_merge_request(merge_request, :scan_finding) + def target_pipeline(approval_rule) + target_pipeline_for_merge_request(merge_request, :scan_finding, approval_rule) + end + + def related_target_pipeline_ids(approval_rule) + related_target_pipeline_ids_for_merge_request(merge_request, :scan_finding, approval_rule) end - strong_memoize_attr :related_target_pipeline_ids def related_source_pipeline_ids related_pipeline_ids(pipeline) @@ -196,7 +197,7 @@ def related_source_pipeline_ids strong_memoize_attr :related_source_pipeline_ids def target_pipeline_findings_uuids(approval_rule) - findings_uuids(target_pipeline, approval_rule, related_target_pipeline_ids) + findings_uuids(target_pipeline(approval_rule), approval_rule, related_target_pipeline_ids(approval_rule)) end def pipeline_findings_uuids(approval_rule) diff --git a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb index aaed727b2471dfd5cca8f02e79a4b8abce17d6e5..fa2fb7ee1c80e45cd06aab30205ec66177d94737 100644 --- a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb +++ b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb @@ -31,6 +31,7 @@ def execute return error_with_title(s_('SecurityOrchestration|Timezone is invalid'), field: :timezone) if invalid_timezone? return error_with_title(s_('SecurityOrchestration|Vulnerability age requires previously existing vulnerability states (detected, confirmed, resolved, or dismissed)'), field: :vulnerability_age) if invalid_vulnerability_age? return error_with_title(s_('SecurityOrchestration|Invalid Compliance Framework ID(s)'), field: :compliance_frameworks) if invalid_compliance_framework_ids? + return error_with_title(s_('SecurityOrchestration|Invalid security report time window'), field: :security_report_time_window) if invalid_security_report_time_window? if required_approvals_exceed_eligible_approvers? return errors_with_title(s_('SecurityOrchestration|Required approvals exceed eligible approvers.'), title: s_('SecurityOrchestration|Logic error'), field: :actions, indices: multiple_approvals_failed_action_indices) @@ -293,6 +294,14 @@ def invalid_vulnerability_age? end end + def invalid_security_report_time_window? + return false unless approval_policy? + return false if ::Feature.disabled?(:approval_policy_time_window, container) + + security_report_time_window = policy.dig(:policy_tuning, :security_report_time_window) + security_report_time_window.present? && (security_report_time_window < 1 || security_report_time_window > 10080) + end + def policy_type policy[:type].to_sym end diff --git a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb index 36d23aba587ba1e18fa52fbbe016e902ef583ee0..ff68286e08611b23065a63c8955dea0c0e649215 100644 --- a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb +++ b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb @@ -44,7 +44,7 @@ def update_for_report_type(merge_request, report_type, approval_rules) policy_evaluation = Security::SecurityOrchestrationPolicies::PolicyRuleEvaluationService.new(merge_request) applicable_rules.each do |rule| - policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type)) + policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type, rule)) end policy_evaluation.save @@ -74,11 +74,11 @@ def pipeline_error pipeline&.failed? ? :pipeline_failed : :artifacts_missing end - def validation_context(report_type) + def validation_context(report_type, rule) return if pipeline.nil? { pipeline_ids: related_pipeline_ids(pipeline), - target_pipeline_ids: related_target_pipeline_ids_for_merge_request(merge_request, report_type) } + target_pipeline_ids: related_target_pipeline_ids_for_merge_request(merge_request, report_type, rule) } end def log_message(report_type, message, **attributes) diff --git a/ee/app/validators/json_schemas/approval_policy_content.json b/ee/app/validators/json_schemas/approval_policy_content.json index 04253cbae39a68c7cfe8610a559fca393556a9ee..490648fedf0f3222a314e1f59d10a2b10c25e1d1 100644 --- a/ee/app/validators/json_schemas/approval_policy_content.json +++ b/ee/app/validators/json_schemas/approval_policy_content.json @@ -30,6 +30,12 @@ "unblock_rules_using_execution_policies": { "type": "boolean", "description": "Experimental: Unblock policy approval rules with missing scans of scanners which are enforced by Scan execution policies. If scan does not produce artifacts because it is not applicable to the given project, approval rules are automatically unblocked and do not require approvals." + }, + "security_report_time_window": { + "type": "integer", + "minimum": 1, + "maximum": 10080, + "description": "Specifies the time window in minutes for choosing the target pipeline for the security report comparison." } } }, diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json index 3d4798c61d481f39e8961c1a62d1cbf21feeea35..e81c01332ae8c9d7732e531a007be19ef38b8124 100644 --- a/ee/app/validators/json_schemas/security_orchestration_policy.json +++ b/ee/app/validators/json_schemas/security_orchestration_policy.json @@ -1097,6 +1097,12 @@ "unblock_rules_using_execution_policies": { "type": "boolean", "description": "Experimental: Unblock policy approval rules with missing scans of scanners which are enforced by Scan execution policies. If scan does not produce artifacts because it is not applicable to the given project, approval rules are automatically unblocked and do not require approvals." + }, + "security_report_time_window": { + "type": "integer", + "minimum": 1, + "maximum": 10080, + "description": "Specifies the time window in minutes for choosing the target pipeline for the security report comparison." } } }, diff --git a/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml b/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml new file mode 100644 index 0000000000000000000000000000000000000000..1b5b5126bd77201f0041fe2e70f31d63e60cb1d4 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml @@ -0,0 +1,10 @@ +--- +name: approval_policy_time_window +description: This feature flag will enable MR approval policies to select pipelines with security reports from a time window. +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/525509 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/191739 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/543027 +milestone: '18.4' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js b/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js index f4a8dcf3faf744e3133f3f5e1d1d6fa34972a240..f58d51256ca4dfd623e64380c97a9530d0ef847a 100644 --- a/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js +++ b/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js @@ -2,6 +2,8 @@ import { shallowMount } from '@vue/test-utils'; import { UNBLOCK_RULES_KEY, UNBLOCK_RULES_TEXT, + TIME_WINDOW_KEY, + TIME_WINDOW_TEXT, } from 'ee/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants'; import EdgeCaseSettings from 'ee/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue'; @@ -28,4 +30,24 @@ describe('EdgeCaseSettings', () => { expect(paragraphs.at(0).text()).toBe(UNBLOCK_RULES_TEXT); expect(paragraphs.at(1).text()).toBe('customKey'); }); + + describe('time window formatting', () => { + it('formats time window in minutes', () => { + createComponent({ [TIME_WINDOW_KEY]: 60 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 60 minutes`); + }); + + it('handles plural forms correctly', () => { + createComponent({ [TIME_WINDOW_KEY]: 120 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 120 minutes`); + }); + + it('handles single unit forms correctly', () => { + createComponent({ [TIME_WINDOW_KEY]: 1 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 1 minute`); + }); + }); }); diff --git a/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js b/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js index c376063f857d1606b624b018e976031d79b7c41f..1e5251f86be5314b6270b0b7a347e4db5f3c5af4 100644 --- a/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js +++ b/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js @@ -1,19 +1,25 @@ import { shallowMount } from '@vue/test-utils'; -import { GlFormCheckbox, GlPopover } from '@gitlab/ui'; +import { GlFormCheckbox, GlPopover, GlFormInput, GlFormGroup } from '@gitlab/ui'; import EdgeCasesSection from 'ee/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue'; describe('EdgeCasesSection', () => { let wrapper; - const createComponent = (propsData = {}) => { + const createComponent = (propsData = {}, featureFlags = {}) => { wrapper = shallowMount(EdgeCasesSection, { propsData: { policyTuning: { unblock_rules_using_execution_policies: false }, ...propsData }, + mocks: { + glFeatures: { approvalPolicyTimeWindow: false, ...featureFlags }, + }, }); }; const findCheckbox = () => wrapper.findComponent(GlFormCheckbox); const findPopover = () => wrapper.findComponent(GlPopover); + const findTimeWindowInput = () => wrapper.findComponent(GlFormInput); + const findTimeWindowGroup = () => wrapper.findComponent(GlFormGroup); + describe('default state when edge cases setting is not selected', () => { beforeEach(() => { createComponent(); @@ -44,4 +50,30 @@ describe('EdgeCasesSection', () => { expect(findCheckbox().attributes('checked')).toBe('true'); }); }); + + describe('time window feature', () => { + it('renders time window input when time window is set', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + expect(findTimeWindowInput().exists()).toBe(true); + expect(findTimeWindowGroup().exists()).toBe(true); + expect(findTimeWindowInput().attributes('value')).toBe('60'); + }); + + it('emits when time window input value changes', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + findTimeWindowInput().vm.$emit('input', '120'); + expect(wrapper.emitted('changed')).toEqual([ + ['policy_tuning', { security_report_time_window: 120 }], + ]); + }); + + it('validates time window input range', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + findTimeWindowInput().vm.$emit('input', '0'); + expect(wrapper.emitted('changed')).toBeUndefined(); + + findTimeWindowInput().vm.$emit('input', '10081'); + expect(wrapper.emitted('changed')).toBeUndefined(); + }); + }); }); diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index d3d9ee7cc5972b16c115df9d06c5a8659de1eded..05d09da96ab8516b05e577655599f1e05de680d7 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -3611,4 +3611,107 @@ def stub_foss_conditions_met it_behaves_like 'not editable by user' end end + + describe '#target_pipeline_for_approval_policy' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:target_branch) { 'main' } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project, target_branch: target_branch) } + let_it_be(:merge_request_diff) { create(:merge_request_diff, merge_request: merge_request) } + + let_it_be(:old_pipeline) do + create(:ee_ci_pipeline, + project: project, + ref: target_branch, + created_at: 5.days.ago + ) + end + + let_it_be(:license_scanning_pipeline) do + create(:ee_ci_pipeline, :with_cyclonedx_report, + project: project, + ref: target_branch, + created_at: 2.days.ago + ) + end + + let_it_be(:scan_finding_pipeline) do + create(:ee_ci_pipeline, :with_sast_report, + project: project, + ref: target_branch, + created_at: 1.day.ago + ) + end + + let_it_be(:latest_pipeline) do + create(:ee_ci_pipeline, project: project, ref: target_branch, sha: merge_request.diff_base_sha) + end + + let_it_be(:security_policy) { create(:security_policy, :approval_policy) } + let_it_be(:approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } + let_it_be(:scan_finding_rule) { create(:report_approver_rule, :scan_finding, merge_request: merge_request, approval_policy_rule: approval_policy_rule) } + let_it_be(:license_scanning_rule) { create(:report_approver_rule, :license_scanning, merge_request: merge_request, approval_policy_rule: approval_policy_rule) } + + before do + security_policy.clear_memoization(:policy_content) + end + + shared_examples 'returns correct pipeline for report type' do |report_type, rule| + context "when feature flag is disabled" do + before do + stub_feature_flags(approval_policy_time_window: false) + end + + context 'when there is no pipeline with the correct report type' do + it 'returns the latest pipeline' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(latest_pipeline) + end + end + + context 'when there is a pipeline with the correct report type' do + let_it_be(:pipeline_with_report) do + create(:ee_ci_pipeline, :with_cyclonedx_report, project: project, ref: target_branch, sha: merge_request.diff_base_sha) + end + + it 'returns the pipeline with the correct report type' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(pipeline_with_report) + end + end + end + + context "when time window is set" do + before do + security_policy.update!(content: { policy_tuning: { security_report_time_window: 3 * 24 * 60 } }) + end + + it "returns the first pipeline within the time window with the correct report type" do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(report_type == :scan_finding ? scan_finding_pipeline : license_scanning_pipeline) + end + + context 'when there is a pipeline with the correct report type' do + let_it_be(:pipeline_with_report) do + create(:ee_ci_pipeline, :with_cyclonedx_report, project: project, ref: target_branch, sha: merge_request.diff_base_sha, created_at: 1.hour.ago) + end + + before do + security_policy.update!(content: { policy_tuning: { security_report_time_window: 2 * 60 } }) + end + + it 'returns the pipeline with the correct report type' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(pipeline_with_report) + end + end + end + end + + include_examples 'returns correct pipeline for report type', :scan_finding, :scan_finding_rule + include_examples 'returns correct pipeline for report type', :license_scanning, :license_scanning_rule + end end diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb index 61f97eec160d11a7b37a8e795523964fc9caa1b9..e687622cdce1b2250a95acd7b3239df6e6bccdd6 100644 --- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb +++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb @@ -2523,6 +2523,93 @@ specify { expect(errors).to be_empty } end end + + describe "policy_tuning" do + let(:rule) do + { + type: "scan_finding", + branches: %w[master], + scanners: %w[container_scanning], + vulnerabilities_allowed: 0, + severity_levels: %w[critical], + vulnerability_states: %w[detected] + } + end + + let(:action) do + { + type: "require_approval", + approvals_required: 1, + user_approvers_ids: [42] + } + end + + let(:approval_policy) do + build(:approval_policy, rules: rules, actions: actions, policy_tuning: policy_tuning) + end + + describe "security_report_time_window" do + context 'with valid value' do + let(:policy_tuning) { { security_report_time_window: 60 } } + + specify { expect(errors).to be_empty } + end + + context 'with minimum valid value' do + let(:policy_tuning) { { security_report_time_window: 1 } } + + specify { expect(errors).to be_empty } + end + + context 'with maximum valid value' do + let(:policy_tuning) { { security_report_time_window: 10080 } } + + specify { expect(errors).to be_empty } + end + + context 'when value is below minimum' do + let(:policy_tuning) { { security_report_time_window: 0 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=minimum") + end + end + + context 'when value is above maximum' do + let(:policy_tuning) { { security_report_time_window: 10081 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=maximum") + end + end + + context 'when value is not an integer' do + let(:policy_tuning) { { security_report_time_window: "60" } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is not of type: integer") + end + end + + context 'when value is negative' do + let(:policy_tuning) { { security_report_time_window: -1 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=minimum") + end + end + + context 'with empty policy_tuning object' do + let(:policy_tuning) { {} } + + specify { expect(errors).to be_empty } + end + end + end end shared_examples_for "pipeline_execution_policy_content" do |policy_type| diff --git a/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb b/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb index 4ce73a338ba35b0ec01712ea568b201bcae9429d..e485b5891a146db24c2fc617f7cb1401ff5fb5c8 100644 --- a/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb +++ b/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb @@ -9,6 +9,10 @@ create(:merge_request, :with_merge_request_pipeline, source_project: project) end + let_it_be(:approval_rule) do + create(:approval_project_rule, :scan_finding) + end + let(:class_instance) { subject_class.new } let(:subject_class) do @@ -28,7 +32,9 @@ end describe '#target_pipeline_for_merge_request' do - subject(:target_pipeline) { subject_class.new.target_pipeline_for_merge_request(merge_request, report_type) } + subject(:target_pipeline) do + subject_class.new.target_pipeline_for_merge_request(merge_request, report_type, approval_rule) + end before do merge_request.update_head_pipeline @@ -108,7 +114,7 @@ let(:report_type) { :scan_finding } subject(:related_target_pipeline_ids) do - subject_class.new.related_target_pipeline_ids_for_merge_request(merge_request, report_type) + subject_class.new.related_target_pipeline_ids_for_merge_request(merge_request, report_type, approval_rule) end context 'when there is no pipeline on target branch' do 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 e95fb1f0eca6150c7b59317e6fe557ea90c0b6dc..116db847b742837667d4721205f3c0c48f72076f 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 @@ -193,7 +193,6 @@ merge_request_iid: merge_request.iid, message: 'Evaluating scan_finding rules from approval policies', pipeline_ids: [pipeline.id], - target_pipeline_ids: [target_pipeline.id], project_path: project.full_path ).and_call_original @@ -632,7 +631,6 @@ merge_request_iid: merge_request.iid, message: 'Evaluating scan_finding rules from approval policies', pipeline_ids: [pipeline.id], - target_pipeline_ids: [target_pipeline.id], project_path: project.full_path ).and_call_original diff --git a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb index 03e253975e6c0335dc59c87e3dd17a27e2c110af..10ea80e9e0d04c888c2bb7a32a53616c541707d7 100644 --- a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb @@ -676,6 +676,117 @@ end end + shared_examples 'checks if security_report_time_window is valid' do + context "when policy_type is approval_policy" do + let(:policy_type) { 'approval_policy' } + + before do + stub_feature_flags(approval_policy_time_window: true) + end + + context 'when security_report_time_window is not provided' do + it { expect(result[:status]).to eq(:success) } + end + + context 'when security_report_time_window is provided' do + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: security_report_time_window + } + } + end + + context 'when security_report_time_window is valid' do + where(:security_report_time_window, :status) do + 1 | :success + 60 | :success + 1440 | :success + 10080 | :success + end + + with_them do + it { expect(result[:status]).to eq(status) } + end + end + + context 'when security_report_time_window is invalid' do + where(:security_report_time_window, :status) do + 0 | :error + -1 | :error + 10081 | :error + 20000 | :error + end + + with_them do + it { expect(result[:status]).to eq(status) } + it { expect(result[:message]).to eq('Invalid policy') } + it { expect(result[:details]).to match_array(['Invalid security report time window']) } + + it_behaves_like 'sets validation errors', field: :security_report_time_window, message: 'Invalid security report time window' + end + end + + context 'when security_report_time_window is nil' do + let(:security_report_time_window) { nil } + + it { expect(result[:status]).to eq(:success) } + end + + context 'when security_report_time_window is empty string' do + let(:security_report_time_window) { '' } + + it { expect(result[:status]).to eq(:success) } + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(approval_policy_time_window: false) + end + + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: 0 + } + } + end + + it { expect(result[:status]).to eq(:success) } + end + end + + context "when policy_type is not approval_policy" do + let(:policy_type) { 'scan_execution_policy' } + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: 0 + } + } + end + + before do + stub_feature_flags(approval_policy_time_window: true) + end + + it { expect(result[:status]).to eq(:success) } + end + end + shared_examples 'checks scan execution policy action limit' do let(:action) { { scan: 'container_scanning' } } let(:action_limit) { 2 } @@ -879,6 +990,7 @@ it_behaves_like 'checks if branches are provided in rule' it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' @@ -938,6 +1050,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' it_behaves_like 'checks pipeline execution schedule policy schedule limit' @@ -973,6 +1086,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks if branches exist for the provided branch_type' do where(:policy_type, :branch_type, :status, :expected_error_message) do :scan_execution_policy | 'all' | :success | nil @@ -1039,6 +1153,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' it_behaves_like 'checks pipeline execution schedule policy schedule limit' diff --git a/locale/gitlab.pot b/locale/gitlab.pot index acebe4296602a4b7af80c9ac020b6e226bd8e013..f59559d9cb299e71e8af87a670305cc1c9d26c47 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -56781,9 +56781,18 @@ msgstr "" msgid "ScanResultPolicy|There are no access tokens created" msgstr "" +msgid "ScanResultPolicy|This addresses issues in busy projects where the target branch always has a pipeline running. Instead of only using the latest pipeline (which might be pending), the policy will consider security reports from successful pipelines completed within the specified time window." +msgstr "" + msgid "ScanResultPolicy|Unknown" msgstr "" +msgid "ScanResultPolicy|Use security reports from pipelines completed within the last" +msgstr "" + +msgid "ScanResultPolicy|Use time window for target pipeline comparison" +msgstr "" + msgid "ScanResultPolicy|Users" msgstr "" @@ -56865,6 +56874,12 @@ msgstr "" msgid "ScanResultPolicy|matching type" msgstr "" +msgid "ScanResultPolicy|minute" +msgstr "" + +msgid "ScanResultPolicy|minutes" +msgstr "" + msgid "ScanResultPolicy|scanners" msgstr "" @@ -58699,6 +58714,9 @@ msgstr "" msgid "SecurityOrchestration|Invalid policy type" msgstr "" +msgid "SecurityOrchestration|Invalid security report time window" +msgstr "" + msgid "SecurityOrchestration|Invalid syntax" msgstr ""