MR approval policy: leftover rule with approval_policy_rule_id: nil never reconciles, leaving a duplicate report_approver rule stuck at approvals_required: 0

Summary

When a merge request ends up with two report_approver approval rules for what is meant to be a single MR-approval-policy rule — one correctly linked via approval_policy_rule_id, and one legacy/leftover rule with approval_policy_rule_id: nil — the sync logic that evaluates any_merge_request policy violations silently and permanently treats the legacy rule as "unviolated" and forces approvals_required to 0 (overridden: true) on it, while the current rule stays correctly required.

This produces two same-named report_approver rules on the MR with diverging approvals_required/overridden state, and nothing in the sync path ever reconciles or removes the stale one — it recurs on every subsequent sync. Depending on fallback_behavior, this can leave a merge request permanently blocked (detailed_merge_status: security_policy_violations) even after valid approvals are recorded, because approvals landing on the correctly-tracked rule don't satisfy whatever originally created the orphaned duplicate, and vice versa.

Root cause

Security::ScanResultPolicies::SyncAnyMergeRequestRulesService#approval_rules_for_policies matches "which of the MR's any_merge_request rules correspond to a violated policy" strictly by approval_policy_rule_id (when deprecate_scan_result_policies is enabled, which is the default):

# ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb
def approval_rules_for_policies(approval_rules, policy_sources)
  if Feature.enabled?(:deprecate_scan_result_policies, project)
    policy_ids = policy_sources.filter_map(&:approval_policy_rule_id)
    approval_rules.select { |rule| policy_ids.include?(rule.approval_policy_rule_id) }
  else
    policy_ids = policy_sources.filter_map(&:scan_result_policy_id)
    approval_rules.select { |rule| policy_ids.include?(rule.scan_result_policy_id) }
  end
end

A report_approver rule whose approval_policy_rule_id is nil can never appear in policy_ids, so it is unconditionally classified as unviolated — regardless of the policy's actual current state — and gets zeroed out via ApprovalMergeRequestRule.remove_required_approved:

# ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb
def update_required_approvals(violated_rules, unviolated_rules)
  updated_violated_rules = merge_request.reset_required_approvals(violated_rules)
  ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
  [updated_violated_rules, unviolated_rules]
end

Nothing in this path (or in MergeRequest#synchronize_approval_rules_from_target_project / first_for_approval_policy_rule?, which only de-duplicates violation creation, not rule materialization) ever detects that this nil-approval_policy_rule_id rule is a stale duplicate of a currently-linked one and cleans it up. So once such a rule exists on a project (e.g. left over from before a project's approval rules were migrated onto the approval_policy_rule_id-based addressing scheme), it is re-synced onto every merge request indefinitely, always as an incorrectly-optional, overridden: true duplicate.

There is also no database constraint preventing this: approval_project_rules/approval_merge_request_rules only have plain (non-unique) indexes on approval_policy_rule_id, so nothing at the DB layer prevents more than one report_approver rule per project/MR from being associated with (or missing) the same policy rule.

Reproduction

Verified locally with an RSpec regression test against Security::ScanResultPolicies::SyncAnyMergeRequestRulesService:

  1. Set up an any_merge_request MR-approval-policy rule linked to a project (approval_policy_rule_id set, approvals_required: 1).
  2. Add a second report_approver/any_merge_request rule on the same project and merge request, with the same name/config but approval_policy_rule_id: nil (simulating a leftover rule from before the current addressing scheme).
  3. Run SyncAnyMergeRequestRulesService#execute for a merge request whose commits violate the policy.

Result:

  • The correctly-linked rule stays at approvals_required: 1, overridden: false (as expected).
  • The leftover rule is forced to approvals_required: 0, overridden: true — even though its source project rule still requires 1 approval — and stays that way on every subsequent sync.
  • Only one Security::ScanResultPolicyViolation is ever tracked, so the two rules end up representing inconsistent, unreconciled approval state for what should be a single policy requirement.
expect(approver_rule.reload).to have_attributes(approvals_required: 1, overridden?: false)
expect(legacy_approver_rule.reload).to have_attributes(approvals_required: 0, overridden?: true)
expect(merge_request.scan_result_policy_violations.count).to eq(1)

Impact

  • Merge requests can be permanently stuck with detailed_merge_status: security_policy_violations despite having all required approvals, because the duplicate/leftover rule's state never resolves.
  • Conversely, in other orderings this could let a merge request bypass a policy's real approval requirement, since one of the two duplicate rules is always forced to approvals_required: 0.
  • The only documented workaround today (resyncSecurityPolicies) re-runs the same rule-materialization logic and does not remove the stale rule, so it does not reliably fix affected merge requests.

Suggested fix direction

approval_rules_for_policies (and the equivalent matching in MergeRequest#synchronize_approval_rules_from_target_project) should reconcile rather than silently coexist with rules that have no current approval_policy_rule_id for an otherwise-linked policy — e.g. by detecting and removing report_approver project/MR rules whose approval_policy_rule_id no longer corresponds to any currently-linked Security::ApprovalPolicyRule for that policy, instead of only skipping violation creation for them.