fix(hooks): commit-guard heredoc false positive, and a one-model data strip - #2
Merged
Merged
Conversation
…hat mentions git flags is not a command A Bash heredoc that wrote a doc mentioning git commit and a shell wrapper was blocked. Heredoc bodies are data bash never executes, so they are now dropped line by line up to the terminator before the quote strip runs. A <<WORD marker counts only when it sits outside quotes on its line (even number of unescaped double and single quotes before it); a marker inside a string is text, and stripping past it would let a real command hide behind a fake terminator line. Nine cases added: four allowed forms (quoted and unquoted markers, commit -F - <<EOF, <<- with a tab-indented end) and five that must still block (a real command after the terminator, fake markers inside double and single quotes, a here-string).
The first heredoc strip judged quote parity per line, so a marker on line 2 of a multi-line quoted message was honored and everything after it, including a chained force-push, was stripped. Replaced the awk heredoc pass plus the sed quote strip with a single character walk that keeps state across lines: single and double quotes with backslash escapes, $( ) and backtick substitutions that reopen code inside double quotes, comments, and heredoc bodies with any delimiter, an EOF) terminator inside $( ), <<- tab stripping, and unterminated bodies running to end of input as bash does. Here-strings are no longer mistaken for markers. Ten cases added (92 total), eight of them bypass attempts from review.
…mments after ), ANSI-C strings; fail closed if the strip fails Security review of the character walk found three ways an innocuous first line hid a plain force-push: $((1<<2)) read as a heredoc marker, a # comment directly after ) not recognized, and an escaped quote inside $'...' flipping the quote state for the rest of the input. Arithmetic frames ($(( )), (( )), $[ ]) now suppress marker detection, the comment-start class includes ) and backtick, and $'...' is its own state with backslash escapes. The strip runs under pipefail and an awk failure blocks; LC_ALL=C keeps every tool byte-oriented so an invalid UTF-8 byte cannot drop the rest of the input; commands over 256 KB are refused. Nine cases added (101 total).
…continuations inside the walker; parens inside arithmetic Second code review found two regressions against main: bash expands $(...) and backticks inside an unquoted-delimiter heredoc body, and the global backslash-newline join ran before the walker knew which lines were a body, so a body line ending in a backslash could swallow the terminator. Unquoted bodies are now walked in a data mode that reopens code for substitutions; quoted bodies stay inert; the continuation join happens in the walker for code only. A paren nested inside $(( )) or (( )) keeps the arithmetic context, and a quoted delimiter may contain spaces. Ten cases added (111 total); the suite passes on macOS awk, mawk 1.3.4, and busybox awk.
…[ ]; case patterns; continuations joined with nothing; delimiter continuation refused
Security re-audit found four more places the walker took bash code for data: a # directly after $(...) or a backtick is part of the word, not a comment; a backslash-newline inside a heredoc delimiter was eaten as an escape and armed a phantom body; << inside ${...} and inside an array subscript was read as a heredoc marker. Continuations are now joined with nothing, as bash does, and the fast path joins JSON-escaped continuations before matching, so a word or flag split across lines cannot dodge it. case ... esac is tracked so a pattern's ) no longer closes an enclosing $(...); that limit is gone from the header. The fast path fails closed if sed or grep errors, and pipefail is on for the whole hook. Fourteen cases added (125 total); green on macOS awk, mawk, busybox awk.
…ne; nested and same-line markers fail closed; segment cap Third code review: an EOF) terminator followed by a quote or an operator was not recognized, a marker registered inside a body's substitution was queued behind the outer terminator, and a body started at the end of the marker line even when a quote or continuation was still open. Now a terminator may be followed by ) and the rest of that line is walked as code; a body starts only at a newline reached in code state with no continuation; frames left open inside a body die with it; a marker nested inside a body, or one registered inside a substitution that closes on the same line, makes the walk fail closed. A backslash before a flag character is dropped as bash does. The per-segment loop refuses over 500 separators so it cannot outlast the hook timeout, and a missing awk is tested to fail closed. Fourteen cases added (139 total); green on macOS awk, mawk, busybox awk.
… frame it closes; no comments inside ${ }; open frame or quote at end fails closed; here-strings instead of pipes
Final security pass: the word case anywhere (docs/use-case.md inside a substitution) stuck the pattern counter, so the ) closing $( ) never popped and the closing quote read as an opening one, hiding a chained force-push; an unbalanced [ or ${ leaked the same way; # inside ${ } was taken as a comment; and global pipefail turned grep -q's early exit into a SIGPIPE upstream, silently disabling the secrets scan on staged diffs over a pipe buffer. Now case and esac count only in command position, a ) the top frame cannot close unwinds to the substitution it does close, [ frames die at command boundaries, comments are not recognized inside expansion frames, a frame or quote still open at end of input fails closed (which also covers a case with no esac), and every whole-input check reads through a here-string. The fast path joins continuations with a bash substitution instead of sed. Fifteen cases added (154 total), including a secret at the top of a 200 KB staged diff and 80 KB of padding before a wrapper and a hooksPath override; green on macOS awk, mawk, busybox awk.
…s; # after (( )) is a comment; secrets scan reads every added line
Fifth code review: a ; & or | inside ${x:-;} split git push from --force across segments, and # directly after an ((...)) command was not a comment though bash treats it as one. $(( is now a distinct frame from ((, separators inside ${ }, (( )), $(( )) and their parens are emitted as text, and the comment rule accepts the )) of an arithmetic command but not of an expansion. The secrets scan drops -q and excludes only the +++ b/ and +++ /dev/null headers, so an added line that itself starts with ++ is scanned. Seven cases added (161 total); green on macOS awk, mawk, busybox awk.
…th retries dequoted; every tool status-checked; heredoc queue capped; function definitions and coproc set command position Final security audit: the fast path could be dodged by splitting the subcommand word itself (git pu"sh", git pus\h, git $'push'), which made every later check moot; a missing tr failed open; a pending-heredoc queue with thousands of terminators was quadratic and could outlast the hook timeout; and case after a function definition or coproc was not in command position, which on bash 4+ let a pattern's ) close the enclosing substitution. A quoted span with no whitespace inside is now glued back into its word as bash does, which also closes the old --no-veri"fy", $'--no-verify', and push "-f" limits; the fast path retries with quotes, backslashes, $ and continuations removed when the plain grep finds nothing, refusing payloads over 512 KB; tr and wc are status-checked and the secrets pipeline inspects every stage's status; more than sixteen pending heredocs fails closed. Eighteen cases added (179 total); green on macOS awk, mawk, busybox awk.
…losed; linear fast-path retry; -F is not a force flag Sixth code review: a one-word quoted message ending in ; was glued into its word and then split by the segment loop, moving --no-verify or --force into a git-less segment; $'\x70ush' was glued verbatim where bash decodes it; and the fast-path retry used bash substitutions that are quadratic on bash 3.2, stalling unrelated quote-dense commands. The glue rule now treats separators, newlines, and any substitution inside the span as data, a numeric escape inside $'...' fails closed in the walker and routes to it from the fast path, the retry runs sed once and only when the payload contains the word git, and the short force flag is matched case-sensitively so git commit -F is not a force-push. Eleven cases added (190 total); green on macOS awk, mawk, busybox awk.
… walker refusal names its construct Seventh code review: gating the retry on the substring git meant that splitting the word git itself (g"it" push --force) skipped every later check, a regression from the previous commit. The retry now runs whenever the plain grep finds nothing; sed is linear so the gate bought nothing. Each fail-closed exit in the walker prints which construct it refused, so an agent that hits one (a numeric escape in $'...', a nested heredoc, an open quote) knows what to rewrite. Seven cases added (197 total); green on macOS awk, mawk, busybox awk.
…nuation state carries into the next line; command position after coproc and time; quoted hooksPath; every grep and git call status-checked Closing security audit and eighth code review. The walker printed a newline at the end of every line, but inside $(...) that newline does not end the enclosing command, so the repo's own multi-line heredoc commit message followed by --no-verify split into separate segments and passed; and a continuation line starting with # was read as a comment. A newline is now a segment boundary only in code at depth zero, a space inside a substitution, and nothing inside a quoted span; continuation state persists into the next line. Also: coproc NAME and time -p keep the following word in command position; a quoted core.hooksPath value with a space is caught on the raw command; the fast path flattens pretty-printed payloads and routes JSON unicode escapes to the walker; the ANSI-C router no longer stops at an escaped quote; the committer name is sanitized before it reaches the model; every grep and git call treats a tool failure as a block; the untracked scan is case-insensitive, NUL-safe, and fails closed on an unreadable file; a commit with -i, -o, or a pathspec-looking token scans the working tree; the refusal test asserts the reason text. Twenty-one cases added (218 total); green on macOS awk, mawk, busybox awk.
… absolute-path shell wrappers; --no-color on the diff; symlinks, $'EOF' delimiters, and numeric escapes without git Ninth code review: a ; or | inside a $(...) used as an argument (git push origin "$(git branch --show-current | tr -d ' ')" --force) split the enclosing command so the flag lost its git; /bin/sh -c was not recognized as a wrapper; color.ui=always hid the + prefix from the secrets scan. Separators and newlines inside a substitution now become spaces (a newline in a bare subshell stays a separator), the wrapper class admits an absolute path, git diff runs with --no-color, the untracked scan skips symlinks and non-regular files and matches exclusions on the whole name, a $'EOF' delimiter is quoted, an ANSI-C numeric escape is refused only when git, commit, or push is also mentioned, stdin read failure blocks, and has/hasc use a local status. Fifteen cases added (233 total); green on macOS awk, mawk, busybox awk.
…xpansion frame; the secrets diff is read as text without textconv or relative paths; wrapper flags before -c
Tenth code review: a newline inside ${...}, $((...)) or $[...] split the enclosing command so a flag lost its git; a -diff attribute or a NUL byte made git call the file binary and the secrets scan saw no + lines; bash -x -c and ksh -c were not recognized as wrappers. A newline is now a command boundary only when every open frame is a bare subshell; $[ has its own frame kind so it survives the end-of-line test-bracket drop; the guard's git diff runs with --text --no-textconv --no-color --no-relative and a diff over 16 MB fails closed; the untracked scan reads binaries; the wrapper regex accepts flags before -c and ksh, csh, tcsh, fish. Header lists interpreter wrappers, filter.*.clean, and the -n-inside-substitution false block. Twelve cases added (245 total); green on macOS awk, mawk, busybox awk.
…cess substitution is a word; added lines come from git's own indicator Eleventh code review and third security audit: a shell fed on stdin (bash <<'EOF', bash <<<, ... | sh, bash -s) was not a wrapper so its body was stripped as data; bash -cx and -ceu clusters slipped the -c test; trap runs text at exit; ash, mksh, yash were unlisted. Process substitution <(...) was pushed as a bare subshell, so a separator inside it split the enclosing command and detached --force or --no-verify from its git; it is now its own frame that merges like $(...). A staged line whose content starts with ++ b/ evaded the diff-header filter; git now marks added lines with its own --output-indicator-new, so header shape is irrelevant, and the secrets scan is one grep. Twenty-two cases added (267 total); green on macOS awk, mawk, busybox awk.
… aliases refused; env -S; git 2.28 floor; JSON-escape tests really contain escapes Twelfth code review: the stdin-fed-shell detection caught a few shapes and missed many (bash --norc <<EOF, bash 0<<EOF, | bash;, a pipe into bash on the next line, exec-redirected stdin then bare bash, bash <(...), source <(...)); a git alias defined in the same call (git -c alias.fp=push fp --force) renamed the subcommand out of reach of every check; env -S splits a string into a command; and the two JSON unicode-escape tests had lost their backslashes in transit and asserted nothing. The wrapper check now runs per segment with one rule: a shell by name or path is a wrapper when given leading options, when anything is redirected into it, or when it has no arguments at all; a shell given a script path and the script's own arguments is not. Process substitution keeps its < in the stripped text. alias. anywhere in a call that commits or pushes is refused. The header states the git 2.28 floor. Nineteen cases added (286 total); green on macOS awk, mawk, busybox awk.
…operators and escaped separators are text; wrappers next to parens, after redirections, and glued to redirects; env options before -S; alias rule on stripped text Fourth security audit and thirteenth code review. The stripped text reused ; & | as its own segment delimiter, so an escaped \; or the & of 2>&1 became a command boundary bash never has and a flag after it lost its git. The walker now emits a \001 byte for a real boundary, escaped separators and redirection operators are text, and segments split only on the sentinel. $( and ( emit a trailing space so a wrapper right after them is at a word start (a regression from the previous commit); the wrapper rule adopts a reviewer-verified regex set: a bare shell followed only by redirections, a redirect glued to the shell word, env options before -S, and a path prefix that ignores shebang writes and assignments. The alias rule reads the stripped text plus the inline -c form, so a message mentioning alias.md is data. Any bare token after commit that is not a message-like option value triggers the working-tree scan. Patterns are passed to grep with -e. The suite pins /bin/bash when present. Fifty-six cases added (342 total); green on macOS awk, mawk, busybox awk.
…boundary byte A quoted span with whitespace inside is data and is dropped, but it was dropped without a trace, so `-m "fix: two words" tracked.txt` made the pathspec look like the message value and skipped the working-tree secret scan. A dropped span now leaves an empty "" in its place; an empty pair spliced into a word (commi""t) still vanishes, as in bash. The pathspec loop now consumes exactly the next token after a value-taking option, whatever its shape (-m -F file), no longer treats -S/--gpg-sign as value-taking (they are not), and skips redirection targets (2>&1). Globbing is off for the whole hook so an unquoted token can never expand against the working directory. The walk refuses a raw \001 byte in the command (its own segment mark). env -C/-u/-P/-a/--unset/--chdir/--argv0 with a separate argument, and versioned shell names (ksh93, bash5), are recognized as wrappers.
…tterns and identity overrides are checked
An array subscript inside an expansion or arithmetic (${a[1|2]}) was
treated as a test bracket, so its `|` or `&` split the command where
bash does not; the pieces then dodged every per-segment check. The walk
now opens a subscript frame there, whose separators are text.
A word holding an expansion may be nothing at all when bash runs it, so
`bash $a -c "..."` was not seen as a wrapper. The walk now leaves a `$`
(or backtick) in every such word and the wrapper rule steps over them.
An unquoted `*`, `?`, `[`, or `{a,b}` in a commit or push is expanded at
run time into words the walk never sees (`-*` with a file named
`--no-verify`; `--{force,}`), so it is refused; a quoted or escaped one
is data.
The committer check read only the repo config. It now also reads an
override on the command itself: -c user.name=, GIT_COMMITTER_NAME=,
GIT_AUTHOR_NAME=, and --author.
The two secrets greps take their pattern through -e like the rest.
…annot classify; note the git 2.28 floor
…ide command position is a pattern
A `)` inside `${...}` unwound the frame without restoring the quote it
was opened in, so `: "${x:-)}" ; git push -f origin main #"` hid the
push as quoted data. Inside `${...}` a `)` is now text, the unwind
refuses to drop a frame opened in a quote, and a `${ cmd; }` or a
substitution inside `${...}` is refused rather than guessed.
The interior of a `${...}` word is dropped and a lone `$` stands for it,
in code and in double quotes, so `bash ${a[@]} -c`, `bash "${@}" -c`,
and `bash `echo` -c` are seen as wrappers, and `${BRANCH##*/}` no longer
trips the pattern refusal. A dropped quoted span that held a
substitution leaves `"$"`, for the same reason.
A `[` opens a test bracket only where a command starts; glued to a word
it is a subscript, and anywhere else it is a pattern character, so
`-[n]` is refused like `-*`.
The committer override check reads the stripped text as well as the raw
one (`user.name=Cla'ude'`, `$'Claude'`) and no longer fires on a quoted
message that mentions `user.name=`. Config passed through the
environment (`--config-env`, `GIT_CONFIG_*`) in a commit call is refused.
A short cluster ending in an option that takes a value (`-qm fix`) no
longer makes the value a pathspec; parentheses and angle brackets are
no longer glued back into a word.
… is refused
A lone `[` in a `${...}` value (`${x:-[}`) opened a subscript frame that
swallowed the word's own `}`, so the word stayed open across real
command separators, and a later `]}` truncated away every command bash
had run in between. Inside `${...}` a `[` is now text, as `)` already
was; a `}` that arrives with a paren or bracket still open inside the
braces is refused rather than absorbed.
…at their `)`; config through the environment is one rule
A `${...}` word spanning lines printed its earlier lines as plain
tokens, so a three-line `bash ${a-` ... `} -c` was not seen as a
wrapper. Every line of such a word is now cut back to the word's mark.
A case pattern's `)` is a command boundary, so the pattern no longer
lands in the next command's segment and trips the pattern refusal.
The pathspec loop skips the words of a `$(...)` or backtick
substitution, a `-S<keyid>` consumes nothing, and a short cluster is
read as value-taking only when its other letters are no-value flags
(`-SDEADBEEF` is a key, not `-F`). A dropped quoted span holding a
substitution emits nothing, since the substitution's own mark is
already there. The `-a/-i/-o` check is case-sensitive.
Config passed through the environment (`--config-env`, `GIT_CONFIG_*`)
is refused by one rule for commits and pushes alike, ahead of the
hooks-path rule, which now names only that key. The quoted path
refuses a substitution inside `${...}` with the same reason as code.
README names what the guard refuses instead of a claim it did not keep.
…icks and redirects in the pathspec loop
Bash keeps reading a `${...}` word across newlines and starts a heredoc
body only after the line holding the `}`; the walk entered the body at
the first newline, then cut the following lines to the word's mark, so
a command bash ran on one of them reached no rule. A heredoc marker on
a line that ends inside `${...}` is now refused.
In the pathspec loop backticks are their own flag, toggled by an odd
count in a token, so a `)` inside a backtick pair is no longer counted
twice and the pathspec after it is scanned. A redirection glued to a
word (`tracked.txt>log`, `tracked.txt2>&1`) leaves the word an
argument; `--pathspec-from-file`, `-p`, `--patch`, and `--interactive`
commit working-tree content and scan it. Header and README wording
follow the code.
…ted committer, empty-arg and literal-mark pathspecs
A `<<WORD` marker registered before a `$(...)`, backtick, or `<(...)`
that opens on the same line let bash defer the body past the
substitution's close and run the lines between as its commands, which
the walk dropped as data. That interleave is now refused, alongside the
`${...}` case it mirrors.
The committer override check now also catches a key quoted with its
whole argument (`-c "user.name=Claude Bot"`, `"--author=Claude Bot"`)
and a bot name with a trailing digit.
In the pathspec loop a `$(` mark is matched exactly, so a literal
`x$(y` word is not mistaken for a substitution and no longer hides the
pathspec after it; a quoted backtick is dropped as data so it cannot do
the same; and an empty quoted argument that is a whole word
(`-m "" file`) is kept as one, so its pathspec is still scanned.
Config read from a file the guard cannot see (`include.path`,
`includeIf.`, `GIT_CONFIG_GLOBAL`/`SYSTEM`, `HOME=`) in a commit or
push call is refused, the same as the environment routes.
…d; XDG_CONFIG_HOME is a config route; anchor includeIf The empty-argument fix from the previous commit emitted a placeholder for a quote at the start of a word, but bash elides an empty quote glued to the characters after it (`git ""commit` is `git commit`, `push ""--force` is `push --force`). The placeholder was then a phantom only the guard saw, and it broke every word-anchored check: a two-byte `""` prefix on a subcommand, flag, or shell name slipped a force-push, `--no-verify`, a secret-bearing or bot-authored commit, or a `bash -c` wrapper straight through. The placeholder is now emitted only when the quote is a whole word, proven by the character before it and a one-char lookahead at the character after. XDG_CONFIG_HOME points git at a config file the guard cannot read, the same hooks-path bypass as HOME, so it is refused in a commit or push call. The includeIf config route now requires its `.path=` so a file literally named includeIf.md is not blocked. The header names the secret-scan reach of the expansion limit.
…empty one cannot glue to a flag
A braced `${x}`, a substitution, and an empty quote all leave a `$` or a
space in the walk, so a flag or subcommand after them keeps its word
boundary. A bare unbraced `$x` did not: it fell through to the default
copy and was emitted verbatim. Unset, bash drops it and applies the
literal next to it, so `git push origin $x--force` force-pushed while
the guard read `$x--force` as one unknown word and allowed it. The same
held for `$x--no-verify`, `$x-n`, and a `$x`-prefixed shell wrapper.
A bare `$name`, `$1`, or `$@` now emits a `$` mark set off by spaces,
the same treatment `${...}` already gets, so the boundary is restored
and the flag is seen. Also add XDG_CONFIG_HOME to the header's
false-block list for parity with the rule.
…ue to or hide a flag
The empty-expansion fix reached code state but not the inside of a double
quote, so `git push origin "$x--force"` and `"${x}--force"` (x unset)
still force-pushed: the walk kept `$x` glued to the flag, and a `${x}`
dropped the literal after it, so the flag was invisible to the anchored
checks. The same hid `-f`, `--no-verify`, `-n`, and a shell wrapper
(`bash "$a" -c`).
The double-quote walk now treats a substitution and a possibly-empty
parameter differently. A substitution keeps its marks; a bare `$name`,
`$1`, `$@`, or `${...}` contributes nothing to the word, so a flag glued
after it stays a visible, bounded token, and an expansion-only span
collapses to a single `$` word: spaced at a word start so a following
flag still shows, glued after `=` so `--author=$x` stays one token. A
substitution in the span no longer leaves a phantom empty argument, so a
`$(...)` or `${...}` message is not mistaken for a pathspec.
A committer or author name split across two concatenated quoted spans
(`--author="$x""Claude"`) stays a documented limit: closing it would
block ordinary commit messages that mention these config keys, and the
enforced committer from repo config is still checked.
A `${x:---force}` or `${x:=--no-verify}` expands to that flag when the
parameter is unset, which is the default state, so unlike variable
indirection it needs no prior assignment. The walk drops a `${...}`
interior, so it never saw the flag, and `git push origin "${x:---force}"`
force-pushed while the guard allowed it, quoted or not.
A `${...}` whose `:-`, `-`, `:=`, or `=` default begins with a dash is
now refused: the walk cannot tell whether it expands to that flag. A
normal default is a value, not a flag, so `${VERSION:-0.0.0}`,
`${BRANCH:-main}`, `${x:-HEAD}`, a length `${#x}`, and a replacement
`${x//a/b}` are unaffected.
…nd a space before the dash
The previous refuse caught only `:-`, `-`, `:=`, `=`. It missed the
alternate operators `:+` and `+`, which produce their value when the
parameter is set: `${HOME:+--force}` fires deterministically, since HOME
is always set, with no prior assignment. It also missed a space between
the operator and the dash (`${x:- --force}`), which an unquoted
expansion word-splits into the flag. Both are now refused, and an array
subscript before the operator (`${a[0]:---force}`) is handled.
A `${x/pat/--force}` replacement producing a flag needs the variable set
to a controlled value first, so it is folded into the documented
variable-indirection limit rather than guessed at.
The concatenated-quoted-span identity form (`--author="$x""Claude"`)
moves from the false-block list to the accepted-limits list: it is a
false allow mitigated by the enforced repo-config committer check, not
an over-block, and the header now says so.
…nfig/identity token is refused
A brace default whose value the walk drops was only partly covered. The
value could be quoted or escaped past the check (`${x:-"--force"}`,
`${x:-\-\-force}`), so the dash test now steps over a run of
non-alphanumerics before the flag. The value could also be a `+refspec`,
the other force-push form (`git push origin ${x:-+main}`), so the value
class is now a dash or a plus.
The same dropped interior hid a config or identity token fed to a slot
that only a raw-command check guards: `git -c ${x:-core.hooksPath=/tmp}`
disabled the repo hooks, and `git -c ${x:-user.name=Claude}`,
`git -c user.name=${x:-Claude}`, `GIT_AUTHOR_NAME=${x:-Claude}`,
`--author=${x:-Claude}`, and `core.hooksPath=$HOME` set a non-human
committer or re-pointed the hooks path, all while the guard allowed it.
A `-c`, `--config`, committer, author, or hooksPath value taken from a
shell expansion is now refused, since the walk cannot see it. A literal
override (`-c user.name="Dennis"`) is unaffected.
This also closes the concatenated-quoted-span author form
(`--author="$x""Claude"`), which was a documented limit: its `--author="$`
is a value from an expansion, so it is now caught rather than tolerated.
…ght even when the key is split
The previous refuse anchored the expansion to the character right after
-c or after the key's =, so a value built by concatenation slipped past:
git -c c"$(printf 'ore.hooksPath=/tmp')" commit set core.hooksPath (an
attacker-writable hooks path is code execution), and
GIT_AUTHOR_NAME=C"$(printf laude)" set a non-human committer, both while
the guard allowed them. The literal key itself was split across a
literal prefix and a substitution, so no contiguous key matched either.
Three checks replace the one anchored rule:
- A brace default or alternate whose value carries an `=` is a possible
config token and is refused (`${x:-user.name=Claude}`), alongside the
existing flag and +refspec values.
- On the stripped text, an identity, author, or hooksPath `KEY=` value
that carries any expansion is refused. The stripped text has already
dropped a quoted message, so a commit message that mentions such a key
is exempt; a real override keeps its literal key and the value's mark.
- In the segment loop, a config-position `-c`/`--config` (before the
subcommand) whose value carries an expansion is refused, which catches
a key split across a substitution. A reuse-message `commit -c $sha`
(value after the subcommand) and a `-C <dir>` change-directory
(uppercase) are not that and stay allowed.
…sed, mirroring the hooksPath rule The committer check reads the repo config as it stands before the command runs. A `git config user.name Claude && git commit` in the same call sets the bot identity first, so the commit lands as Claude while the guard, having read the pre-execution human name, allowed it. The same-call `core.hooksPath` write was already refused; this is its identity twin, and it was the last unguarded §2 committer channel. A `git config` (with any flags, or the `set` subcommand) that writes user.name or user.email to a known bot identity, in a call that also commits, is now refused. A literal human name (`git config user.name "Dennis" && git commit`) and an unrelated key (`core.editor`) are unaffected. As with the other identity checks, a commit message that quotes such a config line is a documented false block. The header now records that a brace default carrying `=` is refused as a possible config token, and that a case-insensitive `-c` key from an expansion is caught by the config-position refuse.
…uses a value built from an expansion The same-call config-identity refuse only read the raw command and needed a contiguous bot name, so two shapes slipped through: a split key (`git config user.na"me" Claude`, whose key glues back only in the stripped text) and a value built from an expansion (`git config user.name C"$(printf laude)"`, `git config user.name $BOT`), where the bot name is never a literal. The literal-identity check now also scans the stripped text, so a split key is seen. And a config write of user.name or user.email whose value carries any expansion is refused, since the walk cannot read what it becomes. A quoted commit message is already dropped in the stripped text, so a prose mention stays exempt, and a config write of an unrelated key from a variable (`core.editor $EDITOR`) is unaffected.
…git config -f <file> user.name) A git config write can carry its target file with the short -f flag, not only --file, so the skip between config and the key now covers a short flag and its argument as well as long flags and set. A -f write of user.name or user.email to a bot is refused; a -f write of an unrelated key stays allowed.
…ion flag (git config --type=path user.name) A git config flag can attach its value with = (`--type=path`, `--file=cfg`), which left the =value between the flag and the key and broke the prefix, so `git config --type=path user.name Claude` set a bot committer while the guard allowed it. The skipped flag token now takes an optional =value, so the equals-glued forms are covered for both the literal bot name and a value built from an expansion; an equals-glued flag on an unrelated key (`--file=.git/config core.editor`) stays allowed.
… like git config keys git treats a config key case-insensitively (User.name == user.name), and the literal identity check already matched case-insensitively, but the expansion-value refuse did not, so git config User.name C$(printf laude) set a bot committer while the guard allowed it. That refuse now matches case-insensitively too. The key scoping to user.name/user.email is unchanged, so an uppercase unrelated key (Core.editor $EDITOR) stays allowed. The config-position -c refuse stays case-sensitive on purpose, to tell -c (config) from -C (chdir).
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.
Summary
v1.0.6's commit guard blocked a Bash heredoc that wrote a file whose body mentioned
git commit, a shell wrapper, andgit push -f. Heredoc bodies are not quoted, so the old quote strip left them in and the flag checks fired on prose.The first fix (a per-line heredoc pass in front of the sed quote strip) did not survive adversarial review: two quote models with per-line state opened several ways to hide a real force-push behind an innocuous first line. This PR replaces both with one model, then closes every divergence between that model and bash that repeated review and security audit could produce.
$(...)and backticks that reopen code inside double quotes and unquoted heredoc bodies;${...}parameters, reduced to a single mark because bash may replace them with nothing;$((...))/((...))/$[...]arithmetic and array subscripts, where<<is a shift and|/&are operators;[ ... ]test brackets in command position;#comments only where a word starts; heredoc bodies with any delimiter,<<-,EOF), several per line, unterminated to end of input. Real command boundaries are emitted as a\001byte, so a separator that is data never splits a segment.${ cmd; }funsub, a substitution inside${...}, an unquoted*/?/[/{a,b}in a commit or push, a raw control byte, any shell-wrapper shape, a heredoc marker before a same-line substitution, a parameter default that is a flag or+refspec, git config from the environment or an unreadable file, a-c/committer/author/hooksPath value built from an expansion, and a same-callgit config user.name/emailset to a non-human.LC_ALL=C,set -f, every grep pattern via-ewith status over 1 treated as failure, here-strings instead of pipes so SIGPIPE cannot read as "no match", the diff read with--text --no-textconv --no-relative --no-color --output-indicator-new, caps at 256 KB / 16 MB / 500 segments / 16 heredocs.Deferred / out of scope
git pu${x}sh,--for$(echo c)e), a${x/pat/--force}replacement (needs the variable set first), a script run by name, another interpreter. The false-block list is the price of the invariant and is documented item by item.-c user.email=/ envGIT_COMMITTER_EMAIL=bot-address form is left uncovered on purpose: the committer name still comes from repo config and is checked, so no non-human-named commit lands, and email substring matching is noisy.Test plan
gitshim) before its fixbash tests/hooks/test-guard-commit.sh(585 passed),bash tests/hooks/test-format.sh(4 passed)/bin/bash3.2.57; three awks via docker; per-call cost about 60 msgitargv shim; every finding closed with a pinned case