Consider branch rules on merge request approval rule and policy violation registration

What does this MR do and why?

Security approval policies can be configured with branch rules to target specific branches (e.g., branches: ['feature/*']). Currently, when a merge request is created, approval rules and violations are registered for all policies, regardless of whether the policy's branch rules match the MR's target branch. Filtering only occurs later during evaluation in ApprovalState#wrapped_rules.

This causes several issues:

  • Policy Bot comments may show false positive violations for policies that do not apply to the target branch. ( fixed in #553183 (closed) )
  • Unnecessary violation records are created in the database.
  • UI inconsistencies, where non-applicable policies appear as unviolated (e.g., in the Approval Widget).
  • False positive audit events which are generated based on policy violations

This MR introduces pre-filtering based on branch rules before registering policy violations and creating merge request rules.

When the security_policy_approval_rule_branch_filtering feature flag is enabled:

  • For scan_result_policies, ApprovalProjectRule checks applies_to_branch? before creating the merge request approval rule and violation.
  • If a policy does not apply to the target branch, no approval rule or violation is created.
  • If an existing rule or violation becomes applicable or inapplicable (e.g., if the target branch changes), it is created or deleted accordingly.

Side effects

This is a backend change and the merge request approval rules as well as the security policies feature are expected to work as before. However, there is one side affect of this change.

Approval overrides are applied based on persisted violations. Previously, overrides were applied regardless of the target branch because violations were registered even when policies did not apply.

This MR corrects that behavior. Approval overrides are now enforced only when the policy applies to the merge request’s target branch according to its branch rules.

See the discussion here for more on this: #553189 (comment 2903710412)

Spec changes

The ApprovalProjectRule#apply_report_approver_rules_to method now checks whether the approval rule applies to the merge request's target branch before creating or updating the MR approval rule.

If the branch is not protected or the rule doesn't apply to that branch, the method deletes any existing rules/violations and returns nil instead of a rule object.

This change impacts all services that eventually call apply_report_approver_rules_to:

  • UpdateApprovalsServicePolicyRuleEvaluationService#saveMergeRequest#reset_required_approvalsapply_report_approver_rules_to
  • GroupProjectTransferWorkerLinkProjectServiceSyncMergeRequestsServiceapply_report_approver_rules_to
  • MergeRequests::ReopenServiceresync_policiesMergeRequest#reset_required_approvalsapply_report_approver_rules_to
  • MergeRequests::UpdateServiceschedule_policy_synchronizationMergeRequest#sync_project_approval_rules_for_*apply_report_approver_rules_to
  • SyncReportApproverApprovalRulesMergeRequest#synchronize_approval_rules_from_target_projectapply_report_approver_rules_to
  • SyncMergeRequestsServicesync_merge_requestMergeRequest#sync_project_approval_rules_for_approval_policy_rulesapply_report_approver_rules_to

All these services share a common downstream dependency on apply_report_approver_rules_to. To ensure tests pass, each affected spec was updated to:

  1. Create a protected_branch matching the merge request's target branch
  2. Add the :for_all_protected_branches trait to approval_project_rule factories

Screenshots or screen recordings

Before After
image image

Database Queries

New queries:

Check if any existing violations:

SELECT 1 AS one FROM "scan_result_policy_violations" INNER JOIN "scan_result_policies" ON "scan_result_policy_violations"."scan_result_policy_id" = "scan_result_policies"."id" WHERE "scan_result_policies"."id" = 23 AND "scan_result_policy_violations"."merge_request_id" = 140 LIMIT 1

explain: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/49749/commands/148126

Deleting Violations:

DELETE FROM "scan_result_policy_violations"
WHERE "scan_result_policy_violations"."id" IN (
  SELECT "scan_result_policy_violations"."id"
  FROM "scan_result_policy_violations"
  INNER JOIN "scan_result_policies"
    ON "scan_result_policy_violations"."scan_result_policy_id" = "scan_result_policies"."id"
  WHERE "scan_result_policies"."id" = 23
    AND "scan_result_policy_violations"."merge_request_id" = 140
);

Explain: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/49749/commands/148125 (It returns 0 rows, as we can't know the scan_result_policies.id since it is not exposed through APIs)

Delete Approval Rules

DELETE FROM "approval_merge_request_rules"
WHERE ("approval_merge_request_rules"."id")
    IN (
        SELECT
            "approval_merge_request_rules"."id"
        FROM
            "approval_merge_request_rules"
            INNER JOIN "approval_merge_request_rule_sources" ON "approval_merge_request_rule_sources"."approval_merge_request_rule_id" = "approval_merge_request_rules"."id"
        WHERE
            "approval_merge_request_rules"."merge_request_id" = 16248692
            AND "approval_merge_request_rules"."rule_type" = 3
            AND "approval_merge_request_rule_sources"."approval_project_rule_id" = 47696360)

Query plan: https://console.postgres.aiundefined/shared/322fbb4b-07b8-459e-a879-ce6536d7dea6

References

Spike issue #553189 (closed)

#583612

How to set up and validate locally

  1. Enable the feature flag by running:
bin/rails runner "Feature.enable(:security_policy_approval_rule_branch_filtering)"
  1. Create a project with security policies
  • Secret detection - protected branch
  • SAST scan - feature branch
Policy YAML

.gitlab/security-policies/policy.yml

---
approval_policy:
- name: SAST scan - feature branch
  description: ''
  enabled: true
  rules:
  - type: scan_finding
    scanners:
    - sast
    vulnerabilities_allowed: 0
    severity_levels: []
    vulnerability_states: []
    branches:
    - feature/*
  actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
    - developer
    - maintainer
    - owner
  - type: send_bot_message
    enabled: true
  approval_settings:
    block_branch_modification: false
    prevent_pushing_and_force_pushing: false
    prevent_approval_by_author: true
    prevent_approval_by_commit_author: false
    remove_approvals_with_new_commit: false
    require_password_to_approve: false
  fallback_behavior:
    fail: closed
- name: Secret Detection - Protected Branch
  description: ''
  enabled: true
  enforcement_type: enforce
  rules:
  - type: scan_finding
    scanners:
    - type: secret_detection
      vulnerabilities_allowed: 0
      severity_levels:
      - critical
      - high
      vulnerability_states:
      - new_needs_triage
    vulnerabilities_allowed: 0
    severity_levels: []
    vulnerability_states: []
    branch_type: protected
  actions:
  - type: require_approval
    approvals_required: 1
    group_approvers_ids:
    - 24
  - type: send_bot_message
    enabled: true
  approval_settings:
    block_branch_modification: false
    prevent_pushing_and_force_pushing: false
    prevent_approval_by_author: false
    prevent_approval_by_commit_author: false
    remove_approvals_with_new_commit: false
    require_password_to_approve: false
  fallback_behavior:
    fail: closed

.gitlab-ci.yml

image: busybox:latest
include:
  - template: 'Jobs/Secret-Detection.gitlab-ci.yml'
  - template: 'Jobs/SAST.gitlab-ci.yml'
stages:
  - test

test_job:
  stage: test
  script:
    - echo "Running tests..."

variables:
  AST_ENABLE_MR_PIPELINES: "true"
  1. Navigate to project -> settings -> Protected Branches and add a protected branch feature/*

  2. Add the following 2 files in a branch and create MR targeting main branch

env.sample

AWS_TOKEN=AKIAZYONPI3G4JNCCWGQ

test.rb

job = params[:job]
eval(job)

Observation: only the Secret detection - protected branch is violated. Approval rules and policy bot comment is recorded accordingly.

  1. Create a feature branch (feature/test-branch) in the repository and Change the MR target branch to it

Observation: Both (Secret detection - protected branch and SAST scan - feature branch ) policy is violated. Approval rules and policy bot comment is updated accordingly

  1. To test, approval rule overwrites are working correctly
  • Navigate to project -\> settings -\> Merge requests -\> Merge request approvals : Uncheck all boxes
  • The MR is not allowed to be approved by merge request author when Secret detection - protected branch is violated since the policy has prevent_approval_by_author: true overwrite.
  • Change the target branch to main , now the MR is allowed to be approved by merge request author.
  1. Code coverage rules are working as expected.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #583612

Edited by Imam Hossain

Merge request reports

Loading