Fix(dependency-firewall): filter DF policies out of approval-policy push-bypass paths

Michael Eddingtonrequested to merge
mikeeddington/gitlab:fix/df-type-filter-push-bypass into master

Summary

Every git push to a project with an active Dependency Firewall (DF) policy currently returns:

remote: GitLab: 500 Internal Server Error
! [remote rejected] main -> main (pre-receive hook declined)

This is a hard blocker for all git activity on such a project — CLI/SSH push, MR creation, CI pipelines pushing built artifacts, imports.

Related issue: gitlab-org/gitlab#606449

Root cause

Security::Policy.with_bypass_settings is polymorphic over Security::Policy#type and today includes dependency-firewall policies alongside approval policies.

Two call sites feed that scope directly into approval-policy bypass code paths:

  • ee/lib/security/scan_result_policies/push_bypass_checker.rb:30 (invoked from the /api/v4/internal/allowed pre-receive path)
  • ee/app/models/ee/merge_request.rb:573 (MergeRequest#security_policies_with_bypass_settings)

The approval-policy bypass checkers (UserBypassChecker, BotBypassChecker) call default_roles / service_account_ids / etc. on the policy's bypass_settings. Security::DependencyFirewallPolicies::BypassSettings deliberately implements only user_bypassed? / access_token_bypassed? — the surface actually used by Security::DependencyFirewall::PolicyEvaluator — so the approval-policy checker stack crashes with NoMethodError.

Beyond crashing, PushBypassChecker#check_bypass! OR-aggregates across every policy the scope returns:

policies.any? { |policy| bypass_allowed?(policy) }

Once a DF policy declared a bypass actor in bypass_settings.users / bypass_settings.access_tokens, that actor would silently grant a true on the shared push-bypass checker, effectively bypassing an unrelated approval-policy's push/branch protections — a cross-policy privilege escalation, not just a crash.

What this MR does

Switches both call sites from project.security_policies (all policy types) to the existing project.approval_policies scope, so DF policies never enter the approval-policy bypass code paths:

# ee/lib/security/scan_result_policies/push_bypass_checker.rb
def filtered_policies
-  project.security_policies.with_bypass_settings
+  project.approval_policies.with_bypass_settings
end

# ee/app/models/ee/merge_request.rb
def security_policies_with_bypass_settings
-  project.security_policies.with_bypass_settings
+  project.approval_policies.with_bypass_settings
     .preload(...)
     .joins(...)
end

Project#approval_policies is already defined in ee/app/models/ee/project.rb:170 as has_many :approval_policies, -> { type_approval_policy }, ... and is the standard idiom for approval-only lookups in this codebase. No new scope, index, or migration is introduced.

Behavioural impact

  • git push on a DF-policy project: stops returning 500. PushBypassChecker no longer sees DF policies, returns false cleanly, and the push proceeds normally.
  • MergeRequest#security_policies_with_bypass_settings: now returns only approval policies (which is what its single consumer, check_security_policy_violations_service.rb, and the GraphQL surface both expect — DF policies have no approval_policy_rules join anyway, so this is a semantic tightening, not a behavioural break).
  • DF enforcement (npm/pypi/maven/etc. package downloads and uploads): unchanged. Security::DependencyFirewall::PolicyEvaluator#evaluate_policies calls user_bypassed? / access_token_bypassed? on the DF BypassSettings directly, never through the approval-policy checker stack.
  • Approval-policy push bypass: unchanged for approval policies (they still enter the checker exactly as before).
  • Cross-policy privilege escalation: eliminated. DF bypass_settings cannot influence approval-policy SCM controls.

Specs

Adds regression coverage at both call sites:

  • push_bypass_checker_spec.rb: a DF policy with bypass_settings.access_tokens for the acting user's PAT is ignored — check_bypass! returns false and PolicyBypassChecker is never instantiated for the DF policy.
  • merge_request_spec.rb: #security_policies_with_bypass_settings excludes DF policies with bypass_settings.users.

Relationship to !246423 (merged)

This MR supersedes the earlier interim additive fix (!246423 (merged)), which extended Security::DependencyFirewallPolicies::BypassSettings to implement the full approval-policy BypassSettings interface. That approach fixed the crash but left the cross-policy escalation risk in place. The type-filter is the correct long-term fix, as noted by @hbakergitlab in review. !246423 (merged) should be closed in favour of this MR.

Database considerations

security_policies is documented as table_size: small in db/docs/security_policies.yml. GitLab's database review guidelines require a database review for schema changes, migrations, or "queries beyond the obvious"; a trivial equality predicate on an enum column of a small table falls into the "obvious" category. The Project#approval_policies scope this MR now uses is already the standard idiom for approval-only lookups (see ee/app/models/ee/project.rb:170):

has_many :approval_policies, -> { type_approval_policy },
  class_name: 'Security::Policy', through: :security_policy_project_links, source: :security_policy

Even so, raw SQL and query plans for both updated queries are provided below for reviewer convenience.

PushBypassChecker#filtered_policies (ee/lib/security/scan_result_policies/push_bypass_checker.rb)

SQL before:

SELECT "security_policies".*
FROM "security_policies"
INNER JOIN "security_policy_project_links"
  ON "security_policies"."id" = "security_policy_project_links"."security_policy_id"
WHERE "security_policy_project_links"."project_id" = $1
  AND (security_policies.content->'bypass_settings' IS NOT NULL)
  AND (security_policies.content->'bypass_settings' <> '{}');

SQL after:

SELECT "security_policies".*
FROM "security_policies"
INNER JOIN "security_policy_project_links"
  ON "security_policies"."id" = "security_policy_project_links"."security_policy_id"
WHERE "security_policy_project_links"."project_id" = $1
  AND "security_policies"."type" = 0                        -- <-- added predicate (approval_policy)
  AND (security_policies.content->'bypass_settings' IS NOT NULL)
  AND (security_policies.content->'bypass_settings' <> '{}');

EXPLAIN before (local dev DB):

 Hash Join  (cost=1.09..2.22 rows=1 width=457)
   Hash Cond: (security_policies.id = security_policy_project_links.security_policy_id)
   ->  Seq Scan on security_policies  (cost=0.00..1.10 rows=5 width=457)
         Filter: (((content -> 'bypass_settings'::text) IS NOT NULL) AND ((content -> 'bypass_settings'::text) <> '{}'::jsonb))
   ->  Hash  (cost=1.07..1.07 rows=1 width=8)
         ->  Seq Scan on security_policy_project_links  (cost=0.00..1.07 rows=1 width=8)
               Filter: (project_id = 1)

EXPLAIN after (local dev DB):

 Nested Loop  (cost=0.00..2.21 rows=1 width=457)
   Join Filter: (security_policies.id = security_policy_project_links.security_policy_id)
   ->  Seq Scan on security_policies  (cost=0.00..1.12 rows=1 width=457)
         Filter: (((content -> 'bypass_settings'::text) IS NOT NULL) AND (type = 0) AND ((content -> 'bypass_settings'::text) <> '{}'::jsonb))
   ->  Seq Scan on security_policy_project_links  (cost=0.00..1.07 rows=1 width=8)
         Filter: (project_id = 1)

Costs are essentially identical (2.22 → 2.21). The seq-scans on security_policies reflect the fact that a small table has too few rows for the planner to prefer an index; this is normal and expected for tables of this size and is unchanged by the additional predicate. In production, the same query is already governed by the unique index index_security_policies_on_unique_config_type_policy_index (security_orchestration_policy_configuration_id, type, policy_index), so the type predicate falls on an indexed column.

MergeRequest#security_policies_with_bypass_settings (ee/app/models/ee/merge_request.rb)

SQL before:

SELECT "security_policies".*
FROM "security_policies"
INNER JOIN "security_policy_project_links"
  ON "security_policies"."id" = "security_policy_project_links"."security_policy_id"
INNER JOIN "approval_policy_rules"
  ON "approval_policy_rules"."security_policy_id" = "security_policies"."id"
WHERE "security_policy_project_links"."project_id" = $1
  AND (security_policies.content->'bypass_settings' IS NOT NULL)
  AND (security_policies.content->'bypass_settings' <> '{}')
  AND "approval_policy_rules"."id" IN (
    SELECT "approval_merge_request_rules"."approval_policy_rule_id"
    FROM "approval_merge_request_rules"
    WHERE "approval_merge_request_rules"."merge_request_id" = $2
      AND "approval_merge_request_rules"."approval_policy_rule_id" IS NOT NULL
  );

SQL after: (adds AND "security_policies"."type" = 0 alongside the existing predicates)

SELECT "security_policies".*
FROM "security_policies"
INNER JOIN "security_policy_project_links"
  ON "security_policies"."id" = "security_policy_project_links"."security_policy_id"
INNER JOIN "approval_policy_rules"
  ON "approval_policy_rules"."security_policy_id" = "security_policies"."id"
WHERE "security_policy_project_links"."project_id" = $1
  AND "security_policies"."type" = 0                        -- <-- added predicate (approval_policy)
  AND (security_policies.content->'bypass_settings' IS NOT NULL)
  AND (security_policies.content->'bypass_settings' <> '{}')
  AND "approval_policy_rules"."id" IN (
    SELECT "approval_merge_request_rules"."approval_policy_rule_id"
    FROM "approval_merge_request_rules"
    WHERE "approval_merge_request_rules"."merge_request_id" = $2
      AND "approval_merge_request_rules"."approval_policy_rule_id" IS NOT NULL
  );

EXPLAIN before (local dev DB):

 Nested Loop  (cost=1.43..3.54 rows=1 width=457)
   Join Filter: (security_policies.id = security_policy_project_links.security_policy_id)
   ->  Nested Loop  (cost=1.30..3.37 rows=1 width=465)
         ->  Nested Loop  (cost=1.16..3.20 rows=1 width=8)
               ->  HashAggregate  (cost=1.01..1.02 rows=1 width=8)
                     Group Key: approval_merge_request_rules.approval_policy_rule_id
                     ->  Seq Scan on approval_merge_request_rules  (cost=0.00..1.01 rows=1 width=8)
                           Filter: ((approval_policy_rule_id IS NOT NULL) AND (merge_request_id = 1))
               ->  Index Scan using approval_policy_rules_pkey on approval_policy_rules  (cost=0.15..2.17 rows=1 width=16)
                     Index Cond: (id = approval_merge_request_rules.approval_policy_rule_id)
         ->  Index Scan using security_policies_pkey on security_policies  (cost=0.13..0.16 rows=1 width=457)
               Index Cond: (id = approval_policy_rules.security_policy_id)
               Filter: (((content -> 'bypass_settings'::text) IS NOT NULL) AND ((content -> 'bypass_settings'::text) <> '{}'::jsonb))
   ->  Index Only Scan using index_security_policy_project_links_on_project_and_policy on security_policy_project_links  (cost=0.13..0.16 rows=1 width=8)
         Index Cond: ((security_policy_id = approval_policy_rules.security_policy_id) AND (project_id = 1))

EXPLAIN after (local dev DB):

 Nested Loop  (cost=1.43..3.55 rows=1 width=457)
   Join Filter: (security_policies.id = security_policy_project_links.security_policy_id)
   ->  Nested Loop  (cost=1.30..3.38 rows=1 width=16)
         ->  Nested Loop  (cost=1.16..3.20 rows=1 width=8)
               ->  HashAggregate  (cost=1.01..1.02 rows=1 width=8)
                     Group Key: approval_merge_request_rules.approval_policy_rule_id
                     ->  Seq Scan on approval_merge_request_rules  (cost=0.00..1.01 rows=1 width=8)
                           Filter: ((approval_policy_rule_id IS NOT NULL) AND (merge_request_id = 1))
               ->  Index Scan using approval_policy_rules_pkey on approval_policy_rules  (cost=0.15..2.17 rows=1 width=16)
                     Index Cond: (id = approval_merge_request_rules.approval_policy_rule_id)
         ->  Index Only Scan using index_security_policy_project_links_on_project_and_policy on security_policy_project_links  (cost=0.13..0.16 rows=1 width=8)
               Index Cond: ((security_policy_id = approval_policy_rules.security_policy_id) AND (project_id = 1))
   ->  Index Scan using security_policies_pkey on security_policies  (cost=0.13..0.16 rows=1 width=457)
         Index Cond: (id = approval_policy_rules.security_policy_id)
         Filter: (((content -> 'bypass_settings'::text) IS NOT NULL) AND (type = 0) AND ((content -> 'bypass_settings'::text) <> '{}'::jsonb))

Cost is 3.54 → 3.55 — a 0.3% variation, within measurement noise. The plan uses index scans on every join, and the added type = 0 predicate is applied as a residual filter on the already-narrowed security_policies row. No sequential scan is introduced.

Verification plan

  • Local rubocop clean on all touched files.
  • CI (backend, backend-ee) will exercise:
    • New specs in push_bypass_checker_spec.rb and merge_request_spec.rb.
    • Existing coverage in check_security_policy_violations_service_spec.rb and merge-request GraphQL type specs.
  • Re-run the original reproduction on gdk-in-a-box (Ultimate, dependency_firewall_phase1 FF, DF policy attached): git push should return 200 instead of 500, and production.log / exceptions_json.log should be free of NoMethodError at user_bypass_checker.rb:42.

Reviewer notes

  • No DB migrations, no schema changes, no public API changes.
  • Fix is confined to the EE tree.
  • Uses the existing Project#approval_policies scope (per @sashi_kumar's review suggestion); no new scope is introduced.
Edited by Michael Eddington

Merge request reports