fix(hooks): continuation-line reserved word and fully \u-encoded command bypasses - #3
Merged
Merged
Conversation
… fully \u-encoded command to the walker Two bypasses that v1.0.7 shipped, both verified end-to-end against a git argv shim under bash 4+. A backslash-newline joins the next line onto the previous word, so a `case` (or a split `ca`+`se`) at column 1 of a continuation line inside `$(...)` is the reserved word `case`. The walker skipped scanning a word at column 1 of a continuation line, so `case` was never counted, its pattern `)` was read as the substitution's closing paren, and everything after it, a force-push, a `--no-verify`, or a `git config user.name`, was discarded from the stripped text. The walker now scans that word and carries a word split across the backslash-newline, so the reserved word is seen and the command after the pattern stays visible. The fast path routed a command to the walker on a JSON `\u` escape only when a literal git/commit/push word still remained after dequoting, so a command whose every such word was `\u`-encoded slipped through unread. jq decodes a `\u` escape, so the presence of the escape alone now routes the command to the walker. An ANSI-C numeric escape is decoded by the shell at run time, not jq, so it still routes on a residual word; a command whose every such word is ANSI-C-encoded stays the documented splice limit. Guard tests 585 to 594.
…does not continue it The first cut of the continuation fix carried a word fragment across a backslash-newline but cleared it only at end of line. A fragment could survive an intervening continuation that neither continued nor ended it (`foo\` then `; \` then `case`), and was then wrongly glued onto a later column-1 `case`, making `foocase`, so the reserved word was not counted and the case-pattern `)` popped the substitution frame again. A `git push --force`, `--no-verify`, or committer write after the pattern was discarded and ran under bash 4+. A carried fragment now continues only when the next joined line begins with a word character. Otherwise the deferred word was complete, so it is classified and dropped rather than glued onto a later word. The keyword and command-position logic is factored into one `classify` helper used by both the inline word scan and this resolve. A bare `\` intermediate line (`foo\` then `\` then `case`) joins with no gap into `foocase`, so bash never runs the command after it; that stays allowed, confirmed against bash 5.
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
Two force-push /
--no-verify/ committer bypasses that v1.0.7 shipped, both verified end-to-end against agitargv shim under bash 4/5. They live in the same construct the guard exists to model, so this is a patch release.case(or a splitca+se) at column 1 of a continuation line inside$(...)is the reserved wordcase. The walker skipped scanning a word at column 1 of a continuation line, socasewas never counted intocasec, its pattern)was read as the substitution's closing paren, the$(frame popped early, and everything after the pattern, agit push --force, agit commit --no-verify, or agit config user.name, was swallowed as string data and never checked. The walker now scans that word, carries a word split across the backslash-newline, and resolves a carried fragment the moment a continuation line does not continue it (sofoo\+; \+caseno longer glues intofoocaseand hides the push). A no-gap join that bash never executes (foo\+\+case, which isfoocase) stays allowed, confirmed against bash 5.\u-encoded command. The fast path routed a command to the walker on a JSON\uescape only when a literal git/commit/push word still remained after dequoting, so a command whose every such word was\u-encoded slipped through unread. jq decodes a\uescape, so the escape alone now routes the command to the walker. An ANSI-C numeric escape is decoded by the shell at run time, not jq, so it still routes on a residual word; a command whose every such word is ANSI-C-encoded stays the documented splice limit.The reserved-word / command-position tracking is factored into one
classify()helper used by both the inline word scan and the fragment resolve.\u-encoded forms, each verified to run (or not run) the dangerous command under bash 5 before being pinned.Deferred / out of scope
\u00XXescape now reaches jq even when it is not a git command, so it needs jq (a documented hard dependency); a fully\u-encoded git command has no literal word to match, so routing on the escape alone is the only way to close the bypass, and failing closed on a missing jq is the safe direction. Noted in the fast-path comment.Test plan
gitshim under dockerbash:5) before the fix; blocked afterbash tests/hooks/test-guard-commit.sh(597 passed),bash tests/hooks/test-format.sh(4 passed)foocasejoin confirmed not to execute under bash 5, so it stays allowed