Fix positional JSON key confusion in the hook dispatch loops, and cap JIT_MISSING_REQUIRES - #430
Merged
Merged
Conversation
added 2 commits
September 19, 2026 14:16
…o byte cap (#426, #427) pre-tool-hook.sh/pre-path-hook.sh/post-tool-hook.sh read their JSON payload positionally: every quoted field at an even split position was treated as a candidate key with no check that it sat at key position and no brace-depth tracking, so a tool_input STRING VALUE equal to tool_name/command/file_path/ pattern/skill/subagent_type could repoint the field it named -- last-wins, byte-identical to a genuine non-match -- bypassing every mode: block rule (#426). jit_hook_fields() (common.sh) replaces the positional read with a structural one: a string is a key only when followed by a colon, and a value is read only at the correct depth. Separately, JIT_MISSING_REQUIRES was the one exec-crossing list in common.sh with no byte cap, unlike every sibling that crosses the same pre-tool-hook.sh exec boundary -- an oversized committed requires: value could trip E2BIG and refuse (or, on an unwritable TMPDIR, silently pass) every tool call in a session (#427). jit_missing_requires() now refuses a non-bare-name value outright and caps the accumulated list with a reported truncation; rebuild-tsv.sh applies the same bare-name check at index time. Found by a Claude Security whole-repository scan; both confirmed by reading the awk/bash source rather than executed. Co-Authored-By: Max <noreply>
…lds' real one Self-review finding on #426: jit_hook_fields() (common.sh) locked ti_depth onto the FIRST key anywhere in the payload spelled "tool_input", regardless of what depth that key itself sat at -- unlike the top-level TOP path, which already hardcodes depth == 1. A JSON payload carrying an earlier, unrelated top-level object with its own nested key coincidentally spelled "tool_input" would hijack every subsequent TI[...] read for the rest of the object it wraps, silently discarding the real top-level tool_input's own fields under first-wins. Fixed by tracking the depth a candidate key was itself read at (pending_key_depth) and only accepting it to set ti_depth when that key sat at depth 1 -- the genuine top level, mirroring TOP's own hardcoded check. Not known to be triggerable against Claude Code's actual PreToolUse payload shape today (every field preceding the real tool_input key is runner-written), but the code did not enforce that assumption, and the new-code comments and changelog fragment both claimed a guarantee this did not actually provide. Co-Authored-By: Max <noreply>
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.
Two AI-generated findings from a Claude Security whole-repository scan (report
CLAUDE-SECURITY-20260919-101841/, findings F1/F2/F4 for #426, F3 for #427), reasoned from source reading rather than executed by the issue author; independently confirmed by reading the code before implementing.#426 -- a tool_input string value can pose as a JSON key
pre-tool-hook.sh,pre-path-hook.shandpost-tool-hook.shparsed their JSON payload positionally viajit_json_fields()(common.sh): any quoted field at an even split position was treated as a candidate key, with no check it sat at real key position (followed by:) and no brace-depth tracking. Atool_inputSTRING VALUE equal totool_name,command,file_path,pattern,skillorsubagent_typecould therefore repoint the field it named, last-wins, defeating everymode: block/require/forbidrule byte-identically to a genuine non-match.Fixed with a new
jit_hook_fields()(common.sh) that reads structurally: a string is a key only when the next raw piece begins with:, and brace depth is tracked so a value is only read at the correct nesting level -- top level (depth 1) fortool_name, directly inside the top-leveltool_inputobject for everything else -- first-wins throughout. All three hooks' dispatch loops were rewritten to call it.A self-review finding (Explore reviewer spawn) caught a second-order variant of the same bug in the fix itself:
ti_depthwas locked onto the FIRST key anywhere in the payload spelledtool_input, regardless of what depth that key itself sat at, so a decoy nested object also namedtool_inputcould hijack it. Fixed by tracking the depth at which the candidate key was itself read (pending_key_depth) and requiring it equal 1 before accepting -- mirroring the TOP path's own hardcoded check. Landed as a separate, reviewable commit.Closes #426
#427 -- JIT_MISSING_REQUIRES had no byte cap
JIT_MISSING_REQUIRES(built byjit_missing_requires(),common.sh) was the one list crossing an exec boundary (pre-tool-hook.shhands it to awk as a single-vargument) with no byte cap, unlike five sibling lists. An oversized or malformed committedrequires:value could trip E2BIG and refuse -- or, on an unwritable TMPDIR, silently pass -- every tool call in a session.Fixed:
jit_missing_requires()now refuses anyrequires:value that is not a bare binary name (^[A-Za-z0-9._+-]{1,255}$) before adding it, and caps the accumulated list at 4096 bytes with a one-time truncation sentinel.rebuild-tsv.shapplies the same check at index time, refusing to index a malformed row.Closes #427
Tests
Red confirmed before each fix (isolated by surgical reverse-patches, not a full-file stash, to avoid breaking unrelated parts of the same diff), green after. Final state: test-pre-tool-hook.sh (336/336), test-pre-path-hook.sh (101/101), test-post-tool-hook.sh (61/61), test-security.sh (108/108), test-requires-field.sh (25/25) -- 631 assertions, 0 failures. Collateral-checked (not part of red/green): test-arg-flag-values.sh, test-line-citations.sh, test-dogfood-entries.sh, all green.
bash tests/run-all.shdeliberately not run locally -- CI is the authority for the whole matrix.Review
Self-reviewed by two concurrent spawns (Explore, oss:auditor) against the committed diff. Explore found the
ti_depthhijack above (fixed, separate commit). oss:auditor returned no findings, but flagged one non-blocking maintainability note, judged below the intake bar rather than fixed: jit_missing_requires()'s bash case/${#bin} check re-implements JIT_VALID_REQUIRES_RE's policy rather than invoking the regex itself, so a future edit to the regex alone could silently desync the two. The two checks are character-for-character equivalent today; this is left as-is rather than refactored under this fix.Docs
README.md checked against both matching sections (host-support matrix, writing-entries.md pointer) -- neither documents the JSON-parsing internals or the requires: value format in a way this fix changes; no change needed.
Tooling friction (adjacent, non-blocking)
Several supertool edit calls landed in the main clone instead of the worktree despite a cd-worktree-root prefix on every call, recovered with git diff/git apply/git checkout, no data lost -- a tooling-improvement note for the maintainer, not a blocker on this pull request.
[AI-generated]