Fix approval policy bypass after silent merge request retarget

Sashi Kumar Kumaresanrequested to merge
601181-fix-approval-policy-bypass-on-retarget into master

What does this MR do and why?

A merge request covered by a merge request approval policy could merge without the required approvals after being retargeted to a protected branch when its dependency-chain parent merged (three customer-reported occurrences in #601181 (closed)).

This MR makes the security policy mergeability check fail closed: when the project has policy approval rules applicable to the merge request's current target branch but those rules were never copied onto the merge request, the merge is blocked (checking status) and an idempotent worker re-syncs the rules and re-evaluates the policies. The state converges to correct enforcement without user intervention.

The detection is stateless (it compares persisted project rules against persisted MR rules), so it also self-heals any other lost-sync scenario (for example a crash during the MR-creation sync or the reopen sync), not just retargets.

Guarded by the new security_policy_target_branch_desync_detection feature flag (disabled by default, project actor). No changelog entry because the change is behind a default-off flag.

Root cause

When MR1 (branch-1 → main) merges with "delete source branch", MR2 (branch-2 → branch-1) is retargeted to main via DeleteSourceBranchWorker → RetargetChainService → MergeRequests::UpdateService. The target_branch change is committed in its own transaction (IssuableBaseService#update), while every side effect — the system note, the policy approval-rule re-sync, retargeted = true, head_pipeline_id = nil, and the mergeability re-check — runs after the commit, outside any transaction.

If the Sidekiq process dies (deploy, OOM) or raises in that window, the MR is silently retargeted. The worker's retry cannot recover: RetargetChainService looks up MRs still targeting branch-1, and MR2 already targets main, so the hooks are never re-run. This matches the customer reports: no "changed target branch" system note, and "it almost seems like it was targeted main from the beginning".

Policy approval rules are only kept on MRs whose target branch the policy applies to (ApprovalProjectRule#update_report_approver_rule_for_merge_request deletes them otherwise, and policy rules never apply to unprotected target branches). MR2 therefore had no policy rules and no violations while targeting branch-1, and after the silent retarget:

  • CheckSecurityPolicyViolationsService: no policy rules on the MR → inactive
  • CheckApprovedService: no rules → approved

so the merge proceeded without approvals.

sequenceDiagram
    participant U as User
    participant MS as MergeService (MR1)
    participant W as DeleteSourceBranchWorker
    participant RC as RetargetChainService
    participant US as MergeRequests::UpdateService
    participant DB as PostgreSQL

    U->>MS: Merge MR1 (branch-1 into main, delete source branch)
    MS->>W: perform_async
    W->>RC: execute(MR1)
    RC->>US: execute(MR2, target_branch: main)
    US->>DB: COMMIT target_branch = main
    Note over US: crash / exception window
    US--xUS: post-commit hooks lost:<br/>no system note<br/>no policy rule re-sync<br/>no retargeted flag or pipeline reset
    W->>RC: Sidekiq retry
    RC--xRC: no open MRs target branch-1 anymore,<br/>hooks are never re-run
    Note over DB: MR2 targets main with no policy rules:<br/>mergeable without approvals

How the fix works

CheckSecurityPolicyViolationsService runs on every mergeability evaluation (widget poll and the merge attempt itself) and is not cacheable, which makes it the right enforcement point.

flowchart TD
    A["CheckSecurityPolicyViolationsService#execute"] --> B{"security_orchestration_policies<br/>licensed?"}
    B -- no --> INACTIVE[inactive]
    B -- yes --> C{"security_policy_target_branch_desync_detection<br/>enabled?"}
    C -- no --> EXISTING[existing behavior]
    C -- yes --> D{"Applicable policy project rules<br/>missing from the merge request?<br/>(MergeRequest#missing_policy_approval_rules?)"}
    D -- no --> EXISTING
    D -- yes --> E["return checking: merge blocked"]
    E --> F["Schedule ResyncMergeRequestRulesWorker<br/>(1-minute exclusive lease debounce,<br/>structured log event)"]
    F --> G["Worker: SyncReportApproverApprovalRules copies rules<br/>and bootstraps running violations, then<br/>schedule_policy_synchronization re-evaluates"]
    G -. next evaluation .-> A

Convergence: the re-sync copies the rules and creates running violations, so the check transitions from the desync checking into the existing running-violations checking, and finally to success/failure with the correct approvals_required once the evaluation workers finish. If no policy applies to the target branch, the detection is inert — no behavior change for MRs targeting unprotected branches, and none for already-synced MRs.

Framework compatibility: the comparison is keyed on approval_merge_request_rule_sources.approval_project_rule_id and reuses the same "actively linked policy rules" filter as the sync itself (extracted into MergeRequest#linked_report_approver_project_rules), so it works for both the legacy (scan_result_policy_read) and current (approval_policy_rule) frameworks, and does not flag rules that are intentionally skipped mid-unlink.

What this MR intentionally does not cover (follow-ups)

  1. CE hardening: persist retargeted: true and head_pipeline_id = nil atomically with the target_branch change instead of in post-commit hooks (separate MR for group::code review workflow).
  2. Durable retarget side effects: the "changed target branch" system note is still lost in the crash window.
  3. Residual gap: a protected → protected retarget with lost hooks leaves rules present but evaluated against the old target branch; presence-based detection cannot see this. Needs evaluation-context stamping (follow-up issue).

Database / performance notes

No schema changes and no migrations. When licensed, the check adds one indexed query for the linked policy project rules (with protected_branches and project preloads), one pluck of the MR's synced rule source IDs, and in-memory branch matching. This is the same order of cost as the existing queries in this non-cacheable check. The resync worker is idempotent!, deduplicated until_executed, urgency :low, concurrency-limited, and defers on database health signals.

References

How to set up and validate locally

  1. On an Ultimate GDK, create a project, protect the main branch, and add a merge request approval policy requiring 2 approvals that targets all protected branches (Secure → Policies).

  2. Enable the feature flag in a Rails console:

    Feature.enable(:security_policy_target_branch_desync_detection, Project.find(<project_id>))

  3. Create branch-1 from main and open MR1 (branch-1 → main). Create branch-2 from branch-1 and open MR2 (branch-2 → branch-1). MR2 shows no policy approvals because its target branch is unprotected — expected.

  4. Simulate the production failure (a silent retarget that bypasses every hook):

    mr2 = Project.find(<project_id>).merge_requests.opened.find_by!(source_branch: 'branch-2')
mr2.update_column(:target_branch, 'main')

  5. Without this change (or with the flag disabled), MR2 is immediately mergeable without approvals — the bug.

  6. With this change and the flag enabled, the merge widget reports the merge as blocked while checking security policies; the resync worker then creates the policy approval rules with running violations, and once evaluated MR2 requires 2 approvals:

    mr2.reload.mergeable? # => false until approved
mr2.approval_rules.report_approver.pluck(:name, :approvals_required)

MR acceptance checklist

Evaluated against the MR acceptance checklist.

Merge request reports