fix(hooks): recognize git -C <path> in guard-git.sh#1004
fix(hooks): recognize git -C <path> in guard-git.sh#1004carlos-alm wants to merge 7 commits intomainfrom
git -C <path> in guard-git.sh#1004Conversation
The subcommand grep patterns (`git\s+push`, `git\s+commit`, `git\s+reset`, etc.) did not match when a global option like `-C <path>` appeared between `git` and the subcommand, so `git -C <worktree> push` bypassed both branch validation and the destructive-command blocklist. Parallel-session agents hit this when pushing from a cwd on a non-conforming branch to a PR worktree — the workaround `git -C <pr-worktree> push` succeeded only because the hook failed to recognize it as a push. Fix: - Normalize `$COMMAND` into `$NCOMMAND` with `git -C <path>` stripped, and run all subcommand greps against the normalized form. - Extend `detect_work_dir` to extract the `-C` target (in addition to the existing `cd "<dir>" &&` form) so `validate_branch_name` reads the target worktree's branch, not the cwd's. - Apply the same detection to the commit edit-log check so `git -C <worktree> commit` reads session-edits.log and `diff --cached` from the target repo. Verified with a local test harness covering 18 cases: plain / `cd ... &&` / `git -C ...` forms of push, destructive blocks (add ., reset, clean, stash), and safe forms (restore --staged, log, gh pr list).
|
Claude finished @carlos-alm's task —— View job Reviewing PR: fix(hooks): recognize
|
Greptile SummaryThis PR closes a meaningful security gap in Confidence Score: 4/5Safe to merge; all prior P0/P1 bypass vectors are closed. Two P2 portability nits remain. All previously flagged bypass issues (multi-C chain, cd/C precedence, wrong worktree on chained &&) are correctly resolved. The remaining findings are a BSD awk RS portability concern and an overly-broad -C grep guard — both P2 with no security impact in the current agent-generated-command context. .claude/hooks/guard-git.sh — awk segment-splitting portability (line 109) and -C detection pattern breadth (line 120). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Raw $COMMAND"] --> B["NCOMMAND: strip 'git -C path'
(2-pass sed, quoted + unquoted)"]
B --> C{Blocklist checks
git add/reset/clean
checkout -- / stash / restore}
C -->|match| D["deny()"]
C -->|no match| E{git push
or gh pr create?}
E -->|yes| F["validate_branch_name(subcmd)"]
F --> G["detect_work_dir(subcmd)"]
G --> H{awk: find &&-segment
matching subcmd}
H -->|segment found| I["search_str = segment"]
H -->|not found| J["search_str = $COMMAND"]
I --> K{git -C in
search_str?}
J --> K
K -->|yes| L["sed greedy .*-C → LAST -C path"]
K -->|no| M{cd prefix
in $COMMAND?}
M -->|yes| N["sed → cd target"]
M -->|no| O["work_dir = '' (use cwd)"]
L --> P["git -C work_dir rev-parse HEAD"]
N --> P
O --> Q["git rev-parse HEAD"]
P --> R{branch matches pattern?}
Q --> R
R -->|no| D
R -->|yes| S["exit 0 (allow)"]
E -->|git commit| T["detect_work_dir(commit)"]
T --> G
T --> U["git -C WORK_DIR diff --cached"]
U --> V{staged files in
session-edits.log?}
V -->|unexpected files| D
V -->|ok| S
Reviews (6): Last reviewed commit: "fix(hooks): resolve per-subcommand work ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Normalize: strip `git -C "<path>"` / `git -C <path>` so downstream subcommand | ||
| # patterns (git\s+push, git\s+commit, …) match regardless of whether `-C` is | ||
| # present. detect_work_dir still inspects the raw $COMMAND to find the target. | ||
| NCOMMAND=$(echo "$COMMAND" | sed -E 's/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+"[^"]+"/\1git/g; s/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+[^[:space:]]+/\1git/g') |
There was a problem hiding this comment.
-C options leave NCOMMAND partially un-normalized
The two sed substitutions run sequentially on the same line. Each runs with /g, but after the first match replaces git -C /path1 with git, the remainder of the string is -C /path2 push — there is no second git token for the pattern to re-anchor on. So git -C /path1 -C /path2 push (two unquoted -C options) becomes git -C /path2 push in NCOMMAND, which does not match git\s+push, silently bypassing branch validation.
A second pass of the same sed expression on the already-normalized string would collapse the leftover -C option:
| NCOMMAND=$(echo "$COMMAND" | sed -E 's/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+"[^"]+"/\1git/g; s/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+[^[:space:]]+/\1git/g') | |
| NCOMMAND=$(echo "$COMMAND" | sed -E 's/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+"[^"]+"/\1git/g; s/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+[^[:space:]]+/\1git/g') | |
| NCOMMAND=$(echo "$NCOMMAND" | sed -E 's/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+"[^"]+"/\1git/g; s/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+[^[:space:]]+/\1git/g') |
git -C with multiple flags is rare in agent-generated commands, but closing the gap keeps the bypass surface minimal.
There was a problem hiding this comment.
Fixed — added a second sed pass so the -C normalization re-anchors on git after the first replacement. Also tightened the unquoted pattern ([^"[:space:]][^[:space:]]*) so it no longer mis-matches the opening " of a quoted path on the second pass (which would have left a trailing path" in NCOMMAND). Local smoke test covers git -C /a -C /b push (unquoted) and git -C "/p a/b" -C "/p c/d" push (quoted) — both collapse to git push and match git\s+push.
| if [ -z "$work_dir" ] && echo "$COMMAND" | grep -qE 'git\s+-C\s+'; then | ||
| work_dir=$(echo "$COMMAND" | sed -nE 's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p') | ||
| fi |
There was a problem hiding this comment.
cd X && git -C Y resolves to the cd target, not the -C target
detect_work_dir prefers the cd "..." path and skips the -C check when a cd prefix is present (if [ -z "$work_dir" ]). For a command like cd /tmp && git -C /worktree push, the function returns /tmp, but git will operate on /worktree. Branch validation would then read the HEAD from /tmp rather than the actual target worktree.
This combination is unusual in practice, but for completeness the -C path should take precedence when both are present (it is the explicit git-level override):
detect_work_dir() {
local work_dir=""
# git -C takes explicit precedence over any ambient cd prefix
if echo "$COMMAND" | grep -qE 'git\s+-C\s+'; then
work_dir=$(echo "$COMMAND" | sed -nE 's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p')
fi
if [ -z "$work_dir" ] && echo "$COMMAND" | grep -qE '^\s*cd\s+'; then
work_dir=$(echo "$COMMAND" | sed -nE 's/^\s*cd\s+"?([^"&]+)"?\s*&&.*/\1/p')
fi
work_dir="${work_dir%"${work_dir##*[![:space:]]}"}"
echo "$work_dir"
}There was a problem hiding this comment.
Fixed — detect_work_dir now checks git -C first and only falls back to the cd prefix when no -C is present. -C is the explicit git-level override, so cd /tmp && git -C /worktree push now correctly returns /worktree. Smoke tested all four combinations (only-cd, only-C, both, neither).
…work-dir Impact: 4 functions changed, 5 affected
- Normalize NCOMMAND twice so multi-`-C` chains (e.g. `git -C /a -C /b push`) collapse fully; with a single pass, the pattern cannot re-anchor on `git` after the first replacement, leaving a residual `-C /b` that would bypass subcommand matching. - Unquoted pattern now requires a non-quote first char so it does not mis-match the opening `"` of a quoted path; the previous pattern could swallow `"/p` and leave a trailing `path"` in NCOMMAND. - `detect_work_dir` gives `git -C` precedence over `cd`: `-C` is an explicit git-level override, so `cd /tmp && git -C /worktree push` now correctly reports /worktree (was /tmp).
| if echo "$COMMAND" | grep -qE 'git\s+-C\s+'; then | ||
| work_dir=$(echo "$COMMAND" | sed -nE 's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p') | ||
| fi |
There was a problem hiding this comment.
detect_work_dir extracts the first -C path, not the last
The greedy .*git anchor in the sed pattern causes it to capture the first -C path when multiple -C options are chained. For git -C /conforming-wt -C /bad-branch-wt push, detect_work_dir returns /conforming-wt while git actually operates on /bad-branch-wt. validate_branch_name then reads the HEAD of the wrong worktree and approves the push — a branch-validation bypass using a prefix -C that points to any conforming worktree.
NCOMMAND collapses the chain to git push correctly (two-pass sed with /g), so the subcommand is detected and validate_branch_name is called, but the worktree resolution is wrong.
A one-pass strip of NCOMMAND-style normalization removes the first -C and leaves only the last one, which can then be extracted:
if echo "$COMMAND" | grep -qE 'git\s+-C\s+'; then
local cmd_last_c
cmd_last_c=$(echo "$COMMAND" | \
sed -E 's/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+"[^"]+"/\1git/g; s/(^|\s|&&\s*)git[[:space:]]+-C[[:space:]]+[^"[:space:]][^[:space:]]*/\1git/g')
if echo "$cmd_last_c" | grep -qE 'git\s+-C\s+'; then
work_dir=$(echo "$cmd_last_c" | sed -nE \
's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p')
else
work_dir=$(echo "$COMMAND" | sed -nE \
's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p')
fi
fiThis keeps a single--C command working while correctly picking the last path for multi--C chains.
There was a problem hiding this comment.
Fixed — detect_work_dir now accepts an optional subcommand hint and, within the chosen segment, uses greedy .*-C (anchoring on the LAST -C) rather than .*git\s+-C (anchoring on the first). So git -C /ok -C /bad push now resolves to /bad, matching git's cumulative -C semantics. Verified with a regression harness: single -C, multi -C (last wins), triple -C, and mixed quoted/unquoted paths all resolve correctly. The multi--C bypass is closed — e.g. git -C /conforming-wt -C /bad-branch-wt push is now blocked by validate_branch_name.
| if echo "$COMMAND" | grep -qE 'git\s+-C\s+'; then | ||
| work_dir=$(echo "$COMMAND" | sed -nE 's/.*git[[:space:]]+-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*git[[:space:]]+-C[[:space:]]+([^[:space:]]+).*/\1/p') |
There was a problem hiding this comment.
detect_work_dir targets the last git -C in a chained command, bypassing push branch validation
The sed pattern uses a greedy .*git anchor, so it always extracts the -C path from the last git token in the string. For a chained command like git -C /wt-bad-branch push && git -C /wt-good-branch commit -m "msg", both the push and commit checks fire on NCOMMAND, but detect_work_dir returns /wt-good-branch (from the trailing commit). validate_branch_name then reads the HEAD of the wrong worktree and approves the push — a branch-validation bypass whenever the last sub-command targets a conforming worktree.
validate_branch_name needs the -C path of the first git token (the one driving the push), while the commit-validation block needs the last. One approach is to pass a hint to detect_work_dir so each caller can anchor to the right sub-command, or strip all sub-commands after the first && before calling detect_work_dir from within validate_branch_name.
There was a problem hiding this comment.
Fixed — detect_work_dir now takes a subcommand hint (push, commit) and narrows to the &&-separated segment whose git invocation runs that subcommand before extracting -C. For git -C /wt-bad push && git -C /wt-good commit -m "msg", the push block calls validate_branch_name push which resolves to /wt-bad (correctly blocking the non-conforming branch), and the commit block calls detect_work_dir commit which resolves to /wt-good for the edit-log check. Regression test suite covers this exact chained scenario and confirms push/commit each see their own worktree.
Chained commands like 'git -C /a push && git -C /b commit' and multi-C invocations like 'git -C /ok -C /bad push' escaped branch validation because detect_work_dir extracted the first -C path via greedy anchor on '.*git', not the effective -C git would honor. - detect_work_dir now accepts an optional subcommand hint (push, commit). When given, it narrows to the &&-separated segment whose git invocation runs that subcommand. - Within the chosen segment the LAST -C wins — git's -C is cumulative, so the final -C is the effective CWD. - validate_branch_name and the commit-edit-log block pass the appropriate hint (push/commit) so each sees its own target worktree. Regression tests cover: single -C, multi -C (last wins), chained push/commit in different worktrees, quoted paths, cd-only, mixed precedence, and the original 18-case harness from the PR.
1 participant
Summary
git\s+push,git\s+commit,git\s+reset, etc.) did not matchgit -C <path> <subcmd>— the-Cglobal option betweengitand the subcommand prevented the regex from firing.git -C <worktree> pushbypassed both branch-name validation and the destructive-command blocklist (add ., reset, clean, stash, restore, checkout --). Parallel-session agents hit this when a plaingit pushfrom a non-conforming cwd was denied and they switched togit -C <pr-worktree> pushas a workaround — the workaround succeeded only because the hook couldn't recognize it as a push.git -C <path>before subcommand matching, and extendsdetect_work_dirto extract the-Ctarget sovalidate_branch_nameand the commit edit-log check read from the correct worktree.Changes
NCOMMANDderived from$COMMANDwithgit -C "<path>"/git -C <path>stripped — all subcommand greps now run against$NCOMMAND.detect_work_dirrecognizes bothcd "<dir>" &&(existing) andgit -C "<dir>"(new) forms and returns the target worktree path.session-edits.logandgit diff --cachedfrom the target worktree when-Cis present.Test plan
Local harness covering 18 cases (all pass):
cd <dir> &&/git -C <dir>forms, both conforming and non-conforming target branches.git reset,git clean -fd,git add .,git stash— all blocked whether or not-Cis used (bypass closed).git restore --staged,git log,gh pr list, including-Cvariants.