feat(push): force-lease gate for bare force-push to feature branches (embedder P0, #2677)#23
Merged
Merged
Conversation
…embedder P0, #2677)
A bare `git push --force`/`-f`/`+refspec` to a NON-protected branch can silently
overwrite commits already on the remote (another agent's or session's work). The
push arm's `push_protected_violation` deliberately ignores force ("HOW not WHAT")
and only guards protected refs, so nothing caught this.
Add a `push_force_without_lease_violation` guard, wired into the push arm AFTER
the protected-ref guard: a bare force to a non-protected branch is denied with an
actionable `--force-with-lease` retry sequence; lease forms
(`--force-with-lease`/`--force-if-includes`) are allowed (footgun-removal, not
capability-removal); protected refs stay hard-denied upstream.
Adapted onto agentic-git's existing `parse_push_argv`/`PushArgv` (NOT a copy of
agend-terminal's `force_push.rs`): `PushArgv` gains a `force` flag set by a bare
`--force`/`-f` flag or a `+`-prefixed positional. Force detection is option-aware,
so a `-o +val` push-option VALUE is never mistaken for a force refspec.
Pure deletions are exempt via `is_pure_delete_push`, which is ALL-not-ANY (#2677
F1, a CONFIRMED bypass class): a mixed `git push --force origin :del real` deletes
`:del` AND force-overwrites `real`, so the deletion must NOT exempt the whole
push. Exempt only when `--delete`/`-d` is present or EVERY refspec is a `:<dest>`
colon-deletion.
RED-first: the mixed-refspec cases were verified failing against the buggy
`.any()` variant before landing `.all()`. Unit tests cover bare-force deny,
lease/normal allow, trailing-force-overrides-lease, pure-deletion exempt, the
three mixed forms (incl. `+refspec` and `--` end-of-options variants), and the
flag classifier. A smoke e2e proves the gate is reached in the push arm (denies
the bound-branch bare force with the lease message; a lease force passes through),
with a real origin set up so the fail-closed trust-root guard passes first.
Ports agend-terminal #2677 (force-lease gate) + #2677 F1 (ALL-not-ANY is_delete).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com>
…rce (reviewer4 F1)
git accepts the ATTACHED short push-option form `-o<value>` (`git push -o+force
origin main` = push-option `+force`, NOT a force push). `parse_push_argv` skipped
only the SEPARATE `-o <value>` form, so an attached `-o<val>` whose value contains
`f` (`-o+force`, `-oforce`, `-oci.f`) fell through to `is_bare_force_flag`'s
`contains('f')` and was wrongly flagged as force → a legitimate non-force push was
DENIED. This was a fail-CLOSED false-deny, not a bypass (dev3 VERIFIED no bare
force slips through).
Fix: skip the attached glued-value token `-o<val>` (`is_attached_short_value_opt`)
before force/delete classification — one token consumed whole, so its value is
never read as force/delete. `-o` is the only short value-option `git push`
accepts; long options use `--opt[=val]` (already `--`-excluded). Genuine force is
unaffected: `-f`/`-uf`/`-fu`/`--force`/`+refspec` don't start with `-o`, so they
still deny — no reverse fail-open (verified by RED controls, the risk the lead
flagged).
F2 withdrawn by reviewer4 (verified non-bug): in `git push --repo=origin --force
:dead` the positional `:dead` OVERRIDES `--repo=` and is consumed as the
repository, so git never performs a dead-on-origin delete; the shim not exempting
it is harmless. No parser change for F2.
Also document the deliberate fail-closed over-deny for a clustered `-df`
(force+delete of a non-colon ref) — denied not exempted, never fail-open.
RED-first: the option-value cases were verified failing (wrongly denied) against
the unfixed parser before landing the skip; the genuine-force controls stay denied
throughout.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 0 — force-lease gate in agentic-git's push arm (embedder migration, ports #2677)
Closes the first of the two security regressions the embedder migration depends on. agentic-git repo only — the agend-terminal daemon is untouched, so this is independently mergeable / revertable.
What
A bare
git push --force/-f/+refspecto a non-protected branch can silently overwrite commits already on the remote (another agent's or session's work).push_protected_violationdeliberately ignores force ("HOW not WHAT") and only guards protected refs, so nothing caught this.New guard
push_force_without_lease_violation, wired into the push arm after the protected-ref guard:--force-with-leaseretry sequence;--force-with-lease/--force-if-includes→ allow (footgun-removal, not capability-removal);How it's adapted (not a copy-paste)
Built onto agentic-git's own
parse_push_argv/PushArgv, not agend-terminal'sforce_push.rshelpers.PushArgvgains aforceflag set by a bare--force/-fflag or a+-prefixed positional. Detection is option-aware (a-o +valpush-option value is never mistaken for a force refspec).#2677 F1 — ALL-not-ANY (the bypass class)
is_pure_delete_pushexempts a push only when--delete/-dis present or EVERY refspec is a:<dest>colon-deletion. A mixedgit push --force origin :del realdeletes:deland force-overwritesreal→ stays gated. RED-first: the mixed cases were verified failing against a buggy.any()variant before landing.all().Tests
src/tests.rs): bare-force deny (incl. bundled-uf,+refspec,--end-of-options), lease/normal allow, trailing---force-overrides-lease, pure-deletion exempt, the three mixed forms,is_pure_delete_pushALL-not-ANY,is_bare_force_flagclassifier.tests/smoke.rs): proves the gate is reached in the push arm — denies a bound-branch bare force with the lease message; a lease force passes through. A realoriginis set up so the fail-closed trust-root guard passes first (otherwise it would mask the gate).cargo test+cargo clippy --workspace --all-targets -D warnings: green.For DUAL bypass-attack review
Please attack the bypass surface: bundled short flags (
-fu/-uf/-df),+refspecvariants,--end-of-options,--force=-style and flag-form selectors, and mixed delete+overwrite orderings. Known deliberate over-deny (fail-closed, not a bypass): a clustered-df(force+delete) of a non-colon ref is denied rather than exempted — documented inis_pure_delete_push.Base:
d789c83.