CodeIssuesPull RequestsActionsSecurityInsights
✨ AI
More
Settings

feat(protection): branch protection is enforced at push time — the checkboxes stop lying #5546

MergedXSccantynz wants to mergefeat/enforce-branch-protectionmainopened 6d ago
ccantynzcommented 6d ago

Flow audit critical #2: require_pull_request / allow_force_push / allow_deletion were written by bootstrap, rendered as configured on the gates page, and read by nothing in the push path — git push --force main sailed through "protected" branches.

What enforces what, and where:

  • Deletion and require-PR are decidable from ref names alone → enforced in evaluatePushPolicy (the existing pre-receive policy layer, same 403 path as protected tags). Branch creation is always allowed — a fresh repo's first git push -u origin main must never bounce.
  • Force-push needs the pushed commits (still quarantined at policy time) → enforced inside the pack-inspection pre-receive hook via merge-base --is-ancestor. The protected-branch list is resolved in TypeScript through matchProtection (exact-beats-glob preserved) and handed to bash as exact names only, so glob semantics can't drift between the two languages.
  • PR merges unaffected: the merge chain updates refs via git update-ref, never through receive-pack.

The honest reset (migration 0130): every existing require_pull_request=true row predates its own enforcement — bootstrap wrote it, nobody chose it, and enforcing it retroactively would reject every solo developer's direct push, including this platform's own deploy-by-push flow. Reset to false; bootstrap default flips to false; owners opt in deliberately and from today the checkbox does what it says. allow_force_push/allow_deletion keep their false values — blocking history rewrites and deletions is what those owners plausibly wanted, and it breaks no normal push.

Known gap, documented in-code: the SSH path installs the hook before refs are knowable, so enforcement doesn't run there yet — SSH is disabled in production (SSH_PORT=0); the comment names the fix required before re-enabling.

New suite branch-protection-push-enforcement.test.ts (9 tests: deletion/require-PR/creation semantics, fail-open on DB errors, hook script + no-force-push file materialization, newline-smuggling guard). Typecheck clean; protection + push-policy suites 35 pass.

ccantynzAI Reviewcommented 6d ago

AI Triage

(no summary)

Priority: medium Risk area: mixed

Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)

Suggestions only — nothing has been applied. The PR author stays in control.

gluecron[bot]🤖 botAI Reviewcommented 6d ago

AI review unavailable

The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.

Cross-repo impact

See what breaks downstream if this PR merges.

Analyze →
⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts