fix(guardrails): close git-alias-chain re-expansion bypass of both git guards#1085
fix(guardrails): close git-alias-chain re-expansion bypass of both git guards#1085kyle-sexton wants to merge 1 commit into
Conversation
…t guards
git chains aliases (an expansion whose first word is itself an alias is
expanded again), but both guards' inline/persisted alias re-expansion had
two coupled defects that let a dangerous op or non-canonical commit reached
through a SECOND alias hop pass (verified rc=0 -> fail open):
- The nested re-parse splice `${w[@]:0:gi+1}` dropped every command-line
`-c`/`--config`/`--config-env` global between `git` and the subcommand, so
the nested hop saw empty config (no second-hop alias, no `--config-env`
shape to refuse). Widened to `${w[@]:0:sub_idx}` so all globals ride into
every hop; the value-blind `--config-env` shape refusal and the
plain/`.command` max-danger union then fire at every depth.
- The one-level `HOOK_NO_ALIAS` cap (git-alias + persisted branches; the `!`
shell-alias path is unchanged) is replaced by a save/restore seen-set of
resolved subcommand names: recursion follows the chain to the real op, a
repeat is git's own alias-loop stop (nothing runs -> allow-safe), and the
finite set of distinct alias keys guarantees termination.
Guard-local only (no hook-utils.sh change, no cross-plugin sync). Both test
matrices extended: two-/three-hop chains, `--config-env` and `.command`
second-hop variants, persisted-config alias chain (fixture repo), and benign
controls (safe multi-hop chain allowed; alias cycle terminates without hang).
guardrails 0.12.1 -> 0.12.2.
Closes #964.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baa459b422
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| saved_seen=(${HOOK_ALIAS_SEEN[@]+"${HOOK_ALIAS_SEEN[@]}"}) | ||
| HOOK_ALIAS_SEEN+=("$sub") | ||
| for exp in ${HOOK_GIT_ALIAS_EXPS[@]+"${HOOK_GIT_ALIAS_EXPS[@]}"}; do | ||
| [[ -n "$exp" ]] || continue | ||
| if [[ "$exp" == '!'* ]]; then |
There was a problem hiding this comment.
Because the current alias is added to HOOK_ALIAS_SEEN before the ! shell-alias branch, the reparse of the shell body inherits a loop state that Git itself would not carry into the fresh git process launched by the shell alias. For example, this hook returns 0 for git -c alias.a='!git -c alias.a="reset --hard" a' a, but in a real repo the inner Git invocation expands a and runs reset --hard, so the dangerous operation bypasses the guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| # | Finding | Classification | Evidence | Reacted |
|---|---|---|---|---|
| 1 | Alias-loop state carried into ! shell-alias reparse |
VALID — unfixed this pass | At baa459b, block-dangerous-git.sh:284 appends $sub to the global HOOK_ALIAS_SEEN before the ! branch at L287. For git -c alias.a='!git -c alias.a="reset --hard" a' a, the reparsed inner a hits seen_hit=1 (L271–277) and is skipped → hook returns 0, while real git runs the ! body as a fresh process (empty alias-loop guard) and executes reset --hard. The L269–270 comment already asserts the shell path "is unbounded by this set" — but the code does not implement that. Confirmed bypass. |
👍 |
Fix direction: the ! shell-alias branch must reparse under a fresh/empty HOOK_ALIAS_SEEN (save → empty → reparse → restore), since a shell alias spawns a new git process whose loop-guard starts empty; the git-alias branch correctly keeps the set (git chains aliases in-process). The P2 finding on block-noncanonical-commit.sh is the same structure and needs the same one-line pattern.
Not fixed in this pass: safe-tier babysit worker, guardrails plugin is out of my mutation scope — surfaced for the author. Thread left unresolved.
| saved_seen=(${HOOK_ALIAS_SEEN[@]+"${HOOK_ALIAS_SEEN[@]}"}) | ||
| HOOK_ALIAS_SEEN+=("$sub") |
There was a problem hiding this comment.
This seen-set is also inherited by ! shell aliases even though their body runs as a separate shell command with a fresh Git alias-loop guard. With git -c alias.a='!git -c alias.a="commit --allow-empty -m bypass" a' a, the hook returns 0 while real Git creates a non-canonical -m commit, because the inner a is treated as already seen instead of being expanded and checked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| # | Finding | Classification | Evidence | Reacted |
|---|---|---|---|---|
| 1 | Alias-loop state reused for ! shell-alias body |
VALID — unfixed this pass | At baa459b, block-noncanonical-commit.sh:258 appends $sub to the global HOOK_ALIAS_SEEN before the ! branch at L268. For git -c alias.a='!git -c alias.a="commit --allow-empty -m bypass" a' a, the reparsed inner a hits seen_hit=1 (L250–256) and is skipped → hook returns 0, while real git runs the ! body as a fresh process and creates the non-canonical -m commit. Confirmed bypass. |
👍 |
Fix direction: identical to the P1 finding on block-dangerous-git.sh — the ! shell-alias branch must reparse under a fresh/empty HOOK_ALIAS_SEEN (save → empty → reparse → restore), because a shell alias spawns a new git process; the in-process git-alias branch correctly keeps the set. Fix both files consistently.
Not fixed in this pass: safe-tier babysit worker, guardrails plugin is out of my mutation scope — surfaced for the author. Thread left unresolved.
|
🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier). |
|
TOWER-MANAGED (control tower holds the #964 claim — issue comment 5051750345). Lanes: hands off; do not resolve threads, do not merge. Tower will drive review rounds and merge under decide-and-log once checks + reviews are clean. Fixer's out-of-scope finding (third guard, same defect class, lower severity) is filed as #1089 — deferred with classification on the issue; not a blocker for this PR. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
kyle-sexton
added
the
needs-human
1 participant
Summary
gitchains aliases — an alias whose expansion's first word is itself an alias is expanded again, until a non-alias subcommand is reached or git detects an alias loop and runs nothing. Both git guards (block-dangerous-git,block-noncanonical-commit) re-expand an inline (-c/--config/--config-env) or persisted-config alias to re-check the REAL subcommand, but two coupled defects in that re-expansion let a dangerous op or a non-canonical commit reached through a second alias hop pass the guard (verified rc=0 → fail open). Both are closed here; the change is guard-local.Root cause (both defects live in the alias re-expansion branch of both guards):
The nested re-parse dropped the command-line globals. The recursive
check_segmentsplice was${w[@]:0:gi+1}(wrappers +git) + expansion + trailing args, which drops everything betweengitand the subcommand — the-c/--config/--config-envoptions. So the nested hop saw empty config: no second-hop alias definition to resolve, and no--config-envshape to refuse. A chain likegit -c alias.rh=foo --config-env=alias.foo=AV rh(AV='reset --hard') reached ALLOW.Re-expansion was capped at one level (
HOOK_NO_ALIAS) on the false premise, stated in the old comments, that git does not chain aliases. It does. A plain two-hop inline chaingit -c alias.rh=foo -c alias.foo='reset --hard' rhreached ALLOW.Fix:
${w[@]:0:sub_idx}so wrappers +git+ every command-line global ride into each nested hop.sub_idx ≥ gi+1always, so this never drops thegittoken and is a no-op when there are no globals. The already-value-blind--config-envshape refusal and the plain/.commandmax-danger union now fire at every depth (covers the--config-env-second-hop manifestation and its.command-spelled variant by construction).HOOK_ALIAS_SEEN) of resolved subcommand names. Recursion follows the chain to the real op; a repeat is git's own alias-loop stop (nothing runs → allow-safe); termination is guaranteed by the finite set of distinct alias keys. The set is initialized at guard entry (a multi-command line invokes the callback once per top-level segment) and saved/restored around each recursion so sibling segments and unwound hops start clean. The!shell-alias re-parse path is unchanged. In the commit guard this collapses the inline and persisted-config branches under a single seen-guard.No
lib/hook-utils.shchange, no cross-plugin sync (hook-utils.shstays IDENTICAL across all carrying plugins). guardrails0.12.1→0.12.2with a CHANGELOG entry.Test plan
Both contract suites extended with the full #964 matrix and run single-threaded:
block-dangerous-git.test.sh— PASS=258 FAIL=0block-noncanonical-commit.test.sh— 98 passed, 0 failedNew #964 cases (blocking = exit 2): two-hop inline chain (case C),
--config-envsecond hop before and after the-c(H1/H2), three-hop inline chain,alias.<sub>.command-spelled second hop, commit-guard inline twin, and a persisted-config alias chain in a fixture repo (alias.c → x → commit). Benign controls (exit 0, verified no hang): a safe multi-hop chain, and an alias cycle (a → b → a) that terminates via git's alias-loop stop.Repo checks green:
shellcheck --rcfile=.shellcheckrc,shfmt -d(clean),check-changelog-parity.sh --check/--check-bump origin/main,check-cross-plugin-source-drift.sh(no new drift),validate-plugins.sh.Related
Closes #964.
🤖 Generated with Claude Code
Co-authored-by: Claude Opus 4.8 noreply@anthropic.com