Repository navigation
Close bypasses in the git-command guard hook - #26
Merged
Merged
Conversation
A Claude Security scan of 151e3ef rejected two candidates against .claude/hooks/guard-bash.sh as not exploitable, since the agent the hook constrains can run arbitrary shell anyway. It did confirm that the guard missed blocked commands written in other forms, and that its "never a false allow" comment was wrong. Segments are now normalized before matching: backslash-continued lines are joined, quotes and backslashes stripped and redirections dropped, and the command word is found behind variable assignments, a path, shell keywords and wrappers such as env, sudo, sh -c and xargs. Short options are read inside clusters and long options by the abbreviations git accepts. A push after a checkout of main in the same command line counts as a push from main, and @ counts as HEAD. The guard also blocks git push --all/--mirror, signing and hooksPath overrides through --config-env and GIT_CONFIG_* variables, any git -c commit.gpgsign value other than true, and git config --unset commit.gpgsign. The header now states the remaining limit: a string filter cannot see commands assembled at runtime, so the hook stays an advisory layer behind the rulesets and CI.
The cases that verified the guard fix lived only in a scratch directory, so a later edit could reopen a bypass unnoticed and a reviewer could not rerun them. The maintainer approved adding them as a check (design gate: new check, no new CI job). scripts/testdata/guard-bash-cases.tsv holds 107 block/allow rows, each with the branch the command runs on. scripts/test-guard-bash.sh feeds each row to the hook as Claude Code's PreToolUse JSON from a scratch repository on that branch and fails on any mismatch, unexpected exit status or empty table. It runs as make test-hooks, as part of make verify, and as a step in the CI lint job. Without jq the hook is inert, so a local run skips the table with a warning, while CI fails.
The independent review of d3db75a found 12 commands that the old guard blocked and the new one allowed. Dropping redirections and joining continued lines ignored quoting, so a quoted ">" or "<", an escaped backslash or a backslash in a comment could hide a word or a whole command. That is a weakened guard. Each command line is now read twice: split as written, and normalized (continued lines joined, comments cut, redirections dropped). It is blocked if either reading blocks it. The normalized reading keeps a word glued to a redirection (-f>/dev/null) instead of dropping it. Across the committed table and the review's case files (421 rows), no command the old hook blocks is allowed. The other findings are fixed as well: - git push --m, --b and --branches resolve to --mirror and --all, so the minimum abbreviation length is now 3. - GIT_CONFIG_PARAMETERS with several entries is searched as a whole. - Values of -o, --push-option, --repo, --receive-pack and --exec, and comments, no longer count as refspecs in the push-from-main check. - git config setting commit.gpgsign to an empty value, and removing or renaming the commit or core section, are blocked. - The whole command line is searched for a Co-Authored-By trailer whenever it runs git commit, so heredocs and multi-line messages are covered. - More wrappers are recognized, and every git or gh word after a wrapper is checked (sudo -u git git push). - $'...' and $"..." quoting, checkout of @{-N} and --track */main. - --no-verify and --no-gpg-sign on every git command; heads/main, matching (:), wildcard and brace-expanded refspecs; git -c alias, remote.*.mirror, remote.*.push and push.default; gh --admin=...; git --attr-source and --super-prefix values. - Short-option clusters that git would reject, such as -inf in a commit message, no longer block. -U takes a value. - A checkout that creates another branch, or checks out paths after --, no longer leaves the guard assuming main. It never overrides the branch the hook sees toward "not main". - gh pr merge must be the first two positional words, so a PR body mentioning it is not blocked. The table grows to 263 rows, including the remaining known false blocks (quoted text is not parsed). It passes under bash 5.3 and bash 3.2.57.
Owner
Author
Review of d3db75a: findings and how each was handledAn independent reviewer agent reviewed d3db75a against the AGENTS.md Review checklist and requested changes. 5b56a61 addresses the findings. Every case the reviewer wrote is now a row in
Also checked:
The fix commit goes back to the same reviewer. The reviewed SHA will be recorded here. |
The re-review of 5b56a61 approved with findings to fix or answer. - A bare git -c remote.<name>.mirror is a boolean true and forces a mirror push; the key without a value is now blocked. - One key check (check_ref_config) covers aliases, mirror remotes, push refspecs and push.default=matching for git -c, the GIT_CONFIG_* variables and git config writes. Reads stay allowed, and push.default=current is no longer blocked. - The Co-Authored-By search also runs on the command line without quotes and backslashes, which can split the word. - More wrappers: gtimeout, caffeinate, xcrun, arch, chrt, taskset, watch, runuser, systemd-run and ssh. - A branch name glued to -b/-B/-c/-C or given as --orphan=NAME is read. - gh alias set with --admin and git send-pack are blocked. - A wildcard refspec is blocked only when a side matches main, so git push origin "feature/*" is allowed. - --super-prefix is not skipped as a value-taking option: git 2.53 rejects it. - shellcheck is clean. The table grows to 311 rows. Among the committed table and the review's case files (626 rows), the only commands the old hook blocks and the new one allows are five that git or gh rejects, or where HEAD is a push option value; they are recorded as allow rows.
Owner
Author
Re-review of 5b56a61: findings and how each was handledThe same reviewer re-reviewed 5b56a61, including a full review of 3d7115a. The verdict was approve, with findings to fix or answer. 6fdb713 handles them:
Notes on 3d7115a:
The fix commit goes back to the same reviewer. |
The third review of 6fdb713 approved with findings to fix or answer. - push.default=upstream (tracking) pushes the current branch to its upstream, which is origin/main for a branch made from it, so it is blocked along with matching. - Splitting on $( and backticks cut the words after a substitution off from their command in both readings, so git commit -m "$(cat <<EOF ...)" --no-verify got through (the old hook had the same gap). A line with a substitution is now read twice more with each substitution replaced by one word (awk, nesting counted). - git config skips the value of --comment, --file, --blob, --type, --default and --value, so a value such as --list cannot pass for a read. Unsets are treated as reads for the alias and push-refspec checks. - git remote --mirror is blocked. - A wildcard refspec is checked on its destination side (or the source when there is none), so refs/heads/*:refs/heads/backup/* is allowed. - git switch --create=NAME and --force-create=NAME are read. While timing the new readings, a command with 2,000 git commit segments did not finish in 300 s. The whole-line Co-Authored-By search added in 5b56a61 ran once per commit segment, which made the hook quadratic in the command length, and a hook that times out lets the command through. The search now runs once per command: 2,000 segments take 2.9 s. The table grows to 372 rows and passes under bash 5.3.9 and 3.2.57; shellcheck is clean.
Owner
Author
Third review of 6fdb713: findings and how each was handledThe reviewer re-reviewed 6fdb713 and approved, with findings to fix or answer. 31f0131 handles them:
Found while testing: a command with 2,000
Stopping rule, set by the maintainer: this is the last fix round. In the final review of this commit, critical, high and medium findings are fixed. Low findings and nits are answered in the PR as known limits of a string filter, with no further commit. The header and the table's known-gaps and known-false-blocks sections record those limits. |
The final review of 31f0131 requested changes for two medium findings. The substitution pass made the hook quadratic: it read the whole command as one awk record and appended to its output one character at a time, which BWK awk (macOS), mawk and busybox awk handle in quadratic time, so a large heredoc with a backtick ran past the 15 s timeout, and a hook that times out lets the command through. Both awk passes now read one line per record, carry their state across lines, split each line into characters once and print as they go. Their output is identical under BWK awk, mawk, gawk and busybox awk. A ( ) ; & | inside quoted text cut the words after it off from their command in every reading, so git commit -m "feat(proto): x" --no-verify got through (the old hook had the same gap). A line with quotes is now read twice more with the separators inside quotes turned into spaces. To keep the six readings inside the timeout, a segment that names neither git, gh nor GIT_CONFIG_ is skipped before it is split into words, and the branch HEAD is looked up once. A 1 MB heredoc command takes 4.4 s (31f0131: 33 s with mawk); 90 KB on one line takes 3.7 s (18 s in the BWK substitution pass before). Also from the review: process substitution (<(...), >(...)) is flattened like $(...), and abbreviations of git remote --mirror are blocked. The table grows to 394 rows; three answered rows are recorded as known gaps.
Owner
Author
Final review of 31f0131: findings and how each was handledThe final review of 31f0131 requested changes for two medium findings. Under the maintainer's stopping rule, the mediums are fixed in 8733b5c and the lows are answered. Results for 8733b5c:
Timings, with mawk unless noted:
This commit goes back to the reviewer as a fix verification. Only critical, high or medium findings in it lead to another change. |
The prefilter added in 8733b5c removed quotes and backslashes before looking for git, gh or GIT_CONFIG_, but the word processing also drops the $ of $"..." and $'...', so gi$"t" push -f origin x (which bash runs as git) was skipped by the prefilter while 31f0131 blocked it. The prefilter now removes $ as well, so it keeps every segment the word processing would read as git or gh. An automated review also flagged the prefilter for matching case- sensitively. That does not lose anything: the rules match git, gh and GIT_CONFIG_ case-sensitively too, as the old hook did.
On bash 3.2 (macOS /bin/bash), ${var//pattern/repl} slows down sharply
on long strings with many matches. The segment prefilter, the
per-token quote stripping and the line-continuation join used it, so a
command with a 12 KB one-line JSON payload and a force push ran past
the 15 s hook timeout (origin/main's hook blocks it at once). The fix
review of 45f7758 found and tested this change:
- the prefilter reads a second copy of the segments with quotes,
backslashes and $ already removed by tr, in step with the first;
- tokens longer than 256 bytes are stripped with sed and tr;
- continued lines are joined with awk.
Under bash 3.2 that command is now blocked in under a second (45f7758:
about 60 s). Under bash 5 a 1 MB heredoc command takes 7.8 s (45f7758:
4.3 s), inside the 15 s timeout. The table (397 rows) passes under
bash 5.3.9 and 3.2.57, with no row changing against 45f7758.
Owner
Author
Fix verification of 45f7758: last roundThe reviewer verified 8733b5c and 45f7758. At the maintainer's request, the run was stopped before the full fuzz of 45f7758. The checks it did not get to are listed below.
One medium finding (fixed in a02f2e5): on bash 3.2 (macOS
Low findings, answered as known limits with no further commit, per the maintainer's stopping rule:
Checks the reviewer did not get to:
Review of the last commit: this is the last change to this PR. The maintainer reviews the final commit's 8-line diff ( |
5 of 6 tasks
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
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.
Summary
A Claude Security scan of
151e3ef(whole repository,medium) reportedno vulnerabilities. Its panel rejected two candidates against
.claude/hooks/guard-bash.shas not exploitable, since the agent the hookconstrains can run arbitrary shell anyway, but confirmed that the guard
missed blocked commands written in other forms, and that the comment
claiming over-splitting "can only cause a rare false block, never a false
allow" was wrong. This PR closes those gaps and corrects the comment. The
hook is still an advisory layer; rulesets and CI stay authoritative.
Recognized before matching (previously allowed, now blocked):
GIT_TERMINAL_PROMPT=0 git ...), apath (
/usr/bin/git,\git), shell keywords (if,then,!, ...)or a wrapper that runs a command (
env,command,exec,eval,sudo,nice,timeout,nohup,time,xargs,sh -c/bash -c,...). Behind a wrapper the next git/gh word is taken, so wrapper options
need no parsing.
"git",origin "main","--admin"): quotes andbackslashes are stripped.
git commit -nm,-anm,git push -uf,-fu.The cluster is read up to the first option that takes a value, so
git commit -mn(message "n") stays allowed.--dry-run):--no-verif,--no-g,--force-w,--m(--mirror),and so on. Ambiguous prefixes such as
--forcare blocked too, althoughgit rejects them.
@as HEAD (git push origin @on main).git push origin 2>&1on main counted2>as a secondpush argument and slipped past the "pushing while on main" check.
git push \+ newline +--force).git checkout main/git switch main/git checkout -in the same command line (the branch was read before the command ran).
Newly blocked:
git push --all/--mirror(they push main too).core.hooksPathoverrides through--config-env(bothforms) and
GIT_CONFIG_PARAMETERS/GIT_CONFIG_KEY_nvariables (theenvironment form of
git -c).git -c commit.gpgsign=value other than an explicit true (an emptyvalue also disables signing).
git config --unset commit.gpgsign.After the independent review (5b56a61): see the review reply comment
for each finding. In short:
(continued lines joined, comments cut, redirections dropped, keeping a
word glued to a redirection). It is blocked if either reading blocks it,
so normalization cannot hide a command the old hook blocked.
--no-verify/--no-gpg-signon every git command,--branchesand--m, matching/wildcard/brace refspecs andheads/main,git -calias/push-refspec/push.defaultdefinitions,gh … --admin=true, an emptycommit.gpgsign, and removing orrenaming the
commit/coreconfig sections. It searches the wholecommand line for a Co-Authored-By trailer whenever it runs
git commit.-inf), a PRbody that mentions
gh pr merge --admin, and a checkout that createsanother branch or checks out paths.
Still out of reach, stated in the header: commands assembled at
runtime (scripts, aliases, variables, exported config), and literal forms
the hook does not model. Quoted text is not parsed, so a message word like
-nanstill blocks;git commit -F FILEavoids it.Testing
scripts/testdata/guard-bash-cases.tsvholds 397 rows (block/allow, andthe branch the command runs on), including every case from the review.
scripts/test-guard-bash.shfeeds eachrow to the hook as Claude Code's PreToolUse JSON, from a scratch repository
on that branch. It runs as
make test-hooks, insidemake verify, and asa step in the CI
lintjob. Withoutjqthe hook is inert, so a local runskips the table with a warning, while CI fails. The maintainer approved
adding this check (design gate: a new check, but no new CI job; required
check names are unchanged).
(
bash:3.2Docker image).main): every mismatch is a row that should be blocked, except five rows git or gh rejects (recorded as allow rows), all of them rows that should beblocked; it passes every allow row, so the expected values match its
intent.
table and the reviewer's case files (421 rows).
git push -u origin <branch>,git push origin vX.Y.Zon main,gh pr merge --merge --delete-branch,git switch -c x origin/main && git push -u origin x,git checkout main && git pull) and the header's examples.gh pr create --bodycommand takes about 0.1 s; a 254 KBheredoc command about 2.4 s (hook timeout: 15 s).
git show origin/main:.claude/hooks/guard-bash.sh > /tmp/old.sh && ./scripts/test-guard-bash.sh /tmp/old.shChecklist
make verifypasses locally (build + lint + tests)make integrationpasses (required whenclient.go/conn.go/commands.go/internal/proto//docker/changed). Not run: none of these paths changed.README.mdandREADME.ja.mdupdated together, or the change touches neitherCHANGELOG.mdupdated under[Unreleased]for user-visible changes