From 2fe319c467519c07a82d05bac114e6a9f17db4ac Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 15:38:37 +0200 Subject: [PATCH 1/2] fix: bind the heredoc sink allowlist to its own clause, fix the closing-delimiter scan, and drop the stale reserved-name exception (#447, #455) #447: jit_heredoc_opener_is_known_sink() checked the WHOLE line for a known data sink, so a decoy sink separated by ; or && (cat > /dev/null --- .github/scripts/smoke_release_tree.py | 49 ++--------- changelog.d/447.security.md | 3 + changelog.d/455.fixed.md | 2 + scripts/common-awk.sh | 117 +++++++++++++++++++++----- tests/test-pre-tool-hook.sh | 51 +++++++++++ tests/test-smoke-reserved-name-445.sh | 77 +++++++++-------- 6 files changed, 205 insertions(+), 94 deletions(-) create mode 100644 changelog.d/447.security.md create mode 100644 changelog.d/455.fixed.md diff --git a/.github/scripts/smoke_release_tree.py b/.github/scripts/smoke_release_tree.py index 5a82da27..3820ab2e 100644 --- a/.github/scripts/smoke_release_tree.py +++ b/.github/scripts/smoke_release_tree.py @@ -38,7 +38,6 @@ import argparse import json import os -import re import shutil import signal import subprocess @@ -247,48 +246,18 @@ def run_validate(tree: Path, mode: str, claude_bin: str | None, home: Path, result.notes.append(f"validate: `claude plugin validate --strict` exit {r.returncode}") result.notes.extend(f" {line}" for line in output.splitlines()) if r.returncode != 0: - if _only_reserved_name_error(output): - # #445 (claude-5h-window-spread #20): CLI 2.1.287 reserves `claude-` names. - # The directory's own scan has not objected to this listing's name, and a - # rename changes every install ID. Until Anthropic answers, this ONE error - # is a warning, never any other. - warning = ("::warning::claude plugin validate --strict: the plugin name is " - "reserved (#445) -- the only error, let through as a warning") - print(warning) - result.notes.append(warning) - return + # #455: this used to let exactly one error through as a warning when the + # validator's own output named the plugin a reserved name (#445/#448) -- CLI + # 2.1.287 rejected the `claude-` prefix this repo carried at the time. #452 + # renamed the plugin away from that prefix, so `claude plugin validate + # --strict` no longer reports that error for this tree, and the carve-out + # that downgraded it is dead code going forward. It is also removed rather + # than kept dormant: its own heading parser could mask a SECOND real error + # whose heading did not match the regex it expected, so any future validate + # error -- reserved-name shaped or not -- now fails the smoke test outright. result.errors.append(f"validate: claude plugin validate --strict exited {r.returncode}") -def _only_reserved_name_error(output: str) -> bool: - """True only when the validator reported exactly one error, the reserved name. - - Errors and warnings both print as `❯` bullets, each under its own "Found N - error(s)" / "Found N warning(s)" heading, and the validator prints one such block - per file it checks. So only bullets under an ERROR heading count, and the totals - of every error heading are summed (#448: the v0.12.0 run printed 1 error and 6 - warnings, and counting every bullet read 7 errors). - """ - error_bullets: list[str] = [] - stated_errors = 0 - in_errors = False - for line in output.splitlines(): - text = line.strip() - m = re.match(r"^\S*\s*Found (\d+) (error|warning)s?:?$", text) - if m: - in_errors = m.group(2) == "error" - if in_errors: - stated_errors += int(m.group(1)) - continue - if text.startswith("Validat"): - in_errors = False - continue - if in_errors and text.startswith("❯"): - error_bullets.append(text) - return (stated_errors == 1 and len(error_bullets) == 1 - and "is reserved" in error_bullets[0]) - - def run_smoke(tree: Path, validate: str = "auto", claude_bin: str | None = None, linger_seconds: float = 30, keep_dir: Path | None = None) -> SmokeResult: tree = Path(tree).resolve() diff --git a/changelog.d/447.security.md b/changelog.d/447.security.md new file mode 100644 index 00000000..6de7d01e --- /dev/null +++ b/changelog.d/447.security.md @@ -0,0 +1,3 @@ +- **The heredoc-body sink allowlist checked the WHOLE line for a known data sink, so a decoy sink sharing the line with the real heredoc operator -- separated by `;` or `&&` -- could still hide a non-sink command's heredoc body from a `forbid:`/`~`/`mode: block` rule** (#447, carried forward from the v0.12.0 gate-3 security audit round 2). `jit_heredoc_opener_is_known_sink()` never bound the `<<` operator to the specific simple command it trails, so `cat > /dev/null = 1; i--) { + c = substr(line, i, 1) + if (c == ";" || c == "&") { start = i + 1; break } + } + endp = len + for (i = col; i <= len; i++) { + c = substr(line, i, 1) + if (c == ";" || c == "&") { endp = i - 1; break } + } + if (endp < start) return "" + return substr(line, start, endp - start + 1) +} +# #447: the OLD closing-delimiter extraction kept only ONE optional quote or +# backslash character on each side of the bare identifier, so a delimiter bash +# itself builds by concatenating several quoted/escaped/bare pieces was read wrong +# -- a quote-then-bare-letters suffix, a bare dash-digit suffix, and a backslash +# mid-word all closed at the WRONG line for this stripper, either leaving a real +# sink own data visible (false block) or never finding a close at all. Replays +# bash own word-formation rule one character at a time: a bare run of ordinary +# characters, a single-quoted run, a double-quoted run (where a backslash-escaped +# double-quote, backslash, dollar or backtick collapses to the literal character), +# or a single backslash-escaped character outside quotes, concatenated with no +# gap, stopping at the first blank or shell word-ending character. start is the +# column of the first character of the word (i.e. right after the operator, its +# optional "-", and any blanks) -- not the operator itself. +function jit_heredoc_scan_word(line, start, i, n, c, nc, state, word, bs, q1, q2, stopchars) { + q1 = sprintf("%c", 39) + q2 = sprintf("%c", 34) + bs = sprintf("%c", 92) + stopchars = " \t" bs q1 q2 ";&|<>()" + n = length(line) + i = start + state = 0 + word = "" + while (i <= n) { + c = substr(line, i, 1) + if (state == 0) { + if (c == q1) { state = 1; i++; continue } + if (c == q2) { state = 2; i++; continue } + if (c == bs) { + if (i == n) break + word = word substr(line, i + 1, 1) + i += 2 + continue + } + if (index(stopchars, c) > 0) break + word = word c + i++ + continue + } + if (state == 1) { + if (c == q1) { state = 0; i++; continue } + word = word c + i++ + continue + } + # state == 2: double-quoted -- only an escaped double-quote, backslash, dollar + # or backtick collapses (bash own double-quote escape set); any other + # backslash stays literal. + if (c == q2) { state = 0; i++; continue } + if (c == bs && i < n) { + nc = substr(line, i + 1, 1) + if (nc == q2 || nc == bs || nc == "$" || nc == "`") { word = word nc; i += 2; continue } + } + word = word c + i++ + } + return word +} +function jit_strip_heredoc_body(s, unconditional, n, lines, i, j, out, strip_tabs, delim, line, rest, probed, word, prefix, close_i, quote_in, lt2, op_col, after_pos) { # #461: the heredoc operator is built from its character code, never typed. Once the # release build inlines this file into a hook, the directory validator reads a typed # double `<` here as a shell here-document it cannot close, and blocks the plugin. lt2 = sprintf("%c%c", 60, 60) - q1 = sprintf("%c", 39) - q2 = sprintf("%c", 34) - # TWO bytes, not one: q3 sits inside a DYNAMIC (string) regex bracket expression - # below, and this awk regex compiler treats a lone backslash there as an escape - # introducer for whatever follows -- q3 alone made the escaped closing bracket read - # as a literal close-bracket character rather than the class-closing bracket itself, - # and the whole pattern failed to compile ("nonterminated character class", caught - # by re-running the suite after this change -- self-review finding). Two backslash - # bytes let that same escape reading collapse them back to ONE literal backslash - # inside the class, so the real closing bracket after it closes the class as - # intended. - q3 = sprintf("%c%c", 92, 92) - qclass = "[" q1 q2 q3 "]?" n = split(s, lines, "\n") jit_heredoc_quote_states(lines, n, quote_in) out = "" @@ -1433,15 +1504,19 @@ function jit_strip_heredoc_body(s, unconditional, n, lines, i, j, out, strip_ line = lines[i] probed = " " line close_i = 0 - if (match(probed, "[^<]" lt2 "-?[ \t]*" qclass "[A-Za-z_][A-Za-z0-9_]*" qclass)) { + # Detection only -- deliberately loose, just "a non-< char followed by <<" -- the + # ACCURATE word is computed below by jit_heredoc_scan_word(), never by trusting + # how much of the operator this match happens to consume (#447). + if (match(probed, "[^<]" lt2)) { prefix = substr(probed, 1, RSTART) if (!jit_heredoc_opener_is_suppressed(prefix, quote_in[i])) { - op = substr(probed, RSTART + 1, RLENGTH - 1) - strip_tabs = (substr(op, 1, 3) == lt2 "-") - word = op - sub("^" lt2 "-?[ \t]*", "", word) - gsub("[" q1 q2 q3 "]", "", word) - if (word != "" && (unconditional || jit_heredoc_opener_is_known_sink(line))) { + op_col = RSTART + after_pos = op_col + length(lt2) + strip_tabs = (substr(line, after_pos, 1) == "-") + if (strip_tabs) after_pos++ + while (substr(line, after_pos, 1) == " " || substr(line, after_pos, 1) == "\t") after_pos++ + word = jit_heredoc_scan_word(line, after_pos) + if (word != "" && (unconditional || jit_heredoc_opener_is_known_sink(jit_heredoc_clause_at(line, op_col)))) { delim = word for (j = i + 1; j <= n; j++) { rest = lines[j] diff --git a/tests/test-pre-tool-hook.sh b/tests/test-pre-tool-hook.sh index 758a27f2..cd28a568 100755 --- a/tests/test-pre-tool-hook.sh +++ b/tests/test-pre-tool-hook.sh @@ -677,6 +677,57 @@ echo "=== repro 5: a single-quoted argument spanning several lines still hides a OUT=$(run_hook '{"tool_name":"Bash","tool_input":{"command":"x='"'"'\n</dev/null; bash < /dev/null 2>&1; then exit 2 fi +echo "=== #455: no trace of the removed reserved-name exception remains in the source ===" +if grep -q "_only_reserved_name_error" "$SMOKE"; then + echo " FAIL: _only_reserved_name_error still referenced in $SMOKE" + STATIC_FAIL=1 +else + echo " PASS: _only_reserved_name_error is gone from $SMOKE" + STATIC_FAIL=0 +fi + python3 - "$SMOKE" << 'PY' import importlib.util import os @@ -68,43 +84,38 @@ def run(name, output, code): return result -r = run("alone", "\u2718 Found 1 error:\n\n \u276f name: Plugin name \"claude-jit-context\" is " - "reserved: it passes as one of Anthropic's own.\n\n\u2718 Validation failed\n", 1) -check("reserved name as the only error: run stays ok", r.ok, f"errors: {r.errors!r}") -check("and says so as a ::warning::", any("::warning::" in n for n in r.notes), f"notes: {r.notes!r}") +# #455 problem 2: the plugin is renamed (#452) -- a reserved-name-shaped error must now +# fail like any other validate error, never be let through as a warning. +r = run("alone", "✘ Found 1 error:\n\n ❯ name: Plugin name \"claude-jit-context\" is " + "reserved: it passes as one of Anthropic's own.\n\n✘ Validation failed\n", 1) +check("a reserved-name-shaped error now fails -- the exception is really gone", not r.ok, + f"errors: {r.errors!r}") +check("and raises no ::warning:: carve-out for it", not any("::warning::" in n for n in r.notes), + f"notes: {r.notes!r}") -r = run("two", "\u2718 Found 2 errors:\n\n \u276f name: Plugin name \"claude-x\" is reserved.\n" - " \u276f version: invalid\n\n\u2718 Validation failed\n", 1) +r = run("two", "✘ Found 2 errors:\n\n ❯ name: Plugin name \"claude-x\" is reserved.\n" + " ❯ version: invalid\n\n✘ Validation failed\n", 1) check("reserved name beside another error still fails", not r.ok, f"errors: {r.errors!r}") -r = run("other", "\u2718 Found 1 error:\n\n \u276f version: invalid\n\n\u2718 Validation failed\n", 1) +r = run("other", "✘ Found 1 error:\n\n ❯ version: invalid\n\n✘ Validation failed\n", 1) check("any other validate failure still fails", not r.ok, f"errors: {r.errors!r}") -r = run("clean", "\u2714 Validation passed\n", 0) +r = run("clean", "✔ Validation passed\n", 0) check("a passing validate passes", r.ok, f"errors: {r.errors!r}") check("with no warning", not any("::warning::" in n for n in r.notes), f"notes: {r.notes!r}") -# #448: the real v0.12.0 output. Warnings are also `❯` bullets, under their own -# heading, and counting every bullet read this as 7 errors. -WARN = (" ❯ hooks.Stop: Shell command uses ${CLAUDE_PLUGIN_ROOT} without quotes: " - "bash ${CLAUDE_PLUGIN_ROOT}/scripts/stop-hook.sh.\n") -V012 = ("Validating plugin manifest: /tmp/release-tree/.claude-plugin/plugin.json\n\n" - "✘ Found 1 error:\n\n ❯ name: Plugin name \"claude-jit-context\" is reserved: " - "it passes as one of Anthropic's own.\n\n" - "Validating hooks: /tmp/release-tree/hooks/hooks.json\n\n" - "⚠ Found 6 warnings:\n\n" + WARN * 6 + "\n✘ Validation failed\n") -r = run("v012", V012, 1) -check("#448: reserved name plus 6 warnings (the real v0.12.0 output) stays ok", r.ok, - f"errors: {r.errors!r}") -check("#448: and still warns", any("::warning::" in n for n in r.notes), f"notes: {r.notes!r}") - -r = run("twofiles", "Validating plugin manifest: x\n\n✘ Found 1 error:\n\n" - " ❯ name: Plugin name \"claude-x\" is reserved.\n\n" - "Validating hooks: y\n\n✘ Found 1 error:\n\n ❯ hooks.Stop: invalid\n\n" +# #455 problem 1: a second error block whose own heading does not match the old parser's +# regex (e.g. a per-file heading like "Found 1 error in hooks.json:") used to be able to +# slip past uncounted. With the whole exception removed there is no heading parsing left +# to fool -- ANY non-zero exit fails, regardless of what the output looks like. +r = run("oddheading", "✘ Found 1 error in hooks.json:\n\n ❯ hooks.Stop: invalid\n\n" "✘ Validation failed\n", 1) -check("#448: a second error in another file's block still fails", not r.ok, - f"errors: {r.errors!r}") +check("#455 problem 1: an unrecognized per-file heading still fails (nothing to fool anymore)", + not r.ok, f"errors: {r.errors!r}") print(f"== Results: {PASS} passed, {FAIL} failed ==") sys.exit(1 if FAIL else 0) PY +PY_STATUS=$? + +[ "$STATIC_FAIL" -eq 0 ] && [ "$PY_STATUS" -eq 0 ] From 9dade16875201a488db083f330fe13a527698735 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Tue, 6 Oct 2026 16:01:11 +0200 Subject: [PATCH 2/2] fix: self-review findings on #447/#455 -- quote-aware clause boundary, skip-vs-fail exit code, engine-matrix coverage Explore review and oss:auditor both ran against commit 2fe319c. Three real findings, all fixed here; one real but pre-existing (not a #447 regression) finding logged to trap.d/ rather than fixed inline or filed, since it ranks non-blocking (misreports) in the manager skill's own table. - jit_heredoc_clause_at() scanned for a bare ";"/"&" with no quote tracking, so a quoted one inside the operator's own clause could forge a fake boundary. Threaded the per-line quote state through it (the same quote_in[i] jit_heredoc_opener_is_suppressed already uses) so a ";"/"&" only ends a clause when the state at that character is 0. Also updated two stale "whole line"/"same line" comments left behind by the earlier clause-binding change. - tests/test-smoke-reserved-name-445.sh's new wrapper folded a legitimate SKIPPED (exit 2, no /bin/sh to run the fake claude shim on) into the script's own exit 1 -- tests/run-all.sh reads exit 2 as SKIPPED and anything else as FAILED, so this would have mislabeled a platform-driven skip as a real failure on a leg with no /bin/sh (Windows). Fixed to propagate exit 2 separately from the static check's own exit 1. - The new #447 assertions (jit_heredoc_scan_word/jit_heredoc_clause_at) only ran under the default $PATH awk, not the awk/gawk/mawk matrix this file already runs #442's own heredoc state-machine assertions through. Extended the matrix to cover them too. Logged to trap.d/447.sink-regexes-quote-blind.md: jit_heredoc_opener_is_known_sink()'s five sink regexes are quote-blind, so a quoted ";"+sink-keyword inside a non-sink command's own argument can still false-match as a sink. Confirmed this reproduces identically on bda0282 (before any #447 change), so it is not a regression; fixing it is a separate, larger change to the matching primitives themselves. Co-Authored-By: Claude Sonnet 5 --- scripts/common-awk.sh | 54 ++++++++++++++++----- tests/test-pre-tool-hook.sh | 35 ++++++++++++++ tests/test-smoke-reserved-name-445.sh | 13 ++++- trap.d/447.sink-regexes-quote-blind.md | 67 ++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 12 deletions(-) create mode 100644 trap.d/447.sink-regexes-quote-blind.md diff --git a/scripts/common-awk.sh b/scripts/common-awk.sh index af0e16f2..3687870d 100644 --- a/scripts/common-awk.sh +++ b/scripts/common-awk.sh @@ -1419,17 +1419,46 @@ function jit_heredoc_quote_states(lines, n, qin, i, state) { # runs on that same clause now too -- more precise than the whole line, never less # conservative, since a pipe or substitution in an UNRELATED earlier clause has # nothing to do with this heredoc own command. -function jit_heredoc_clause_at(line, col, i, c, start, endp, len) { +# +# Self-review finding: a bare scan for ";"/"&" is quote-UNAWARE, so a quoted ";" or +# "&" inside the heredoc operator own command (python3 -c "x=1; tee y" <= 1; i--) { c = substr(line, i, 1) - if (c == ";" || c == "&") { start = i + 1; break } + if ((c == ";" || c == "&") && instate[i] == 0) { start = i + 1; break } } endp = len for (i = col; i <= len; i++) { c = substr(line, i, 1) - if (c == ";" || c == "&") { endp = i - 1; break } + if ((c == ";" || c == "&") && instate[i] == 0) { endp = i - 1; break } } if (endp < start) return "" return substr(line, start, endp - start + 1) @@ -1516,7 +1545,7 @@ function jit_strip_heredoc_body(s, unconditional, n, lines, i, j, out, strip_ if (strip_tabs) after_pos++ while (substr(line, after_pos, 1) == " " || substr(line, after_pos, 1) == "\t") after_pos++ word = jit_heredoc_scan_word(line, after_pos) - if (word != "" && (unconditional || jit_heredoc_opener_is_known_sink(jit_heredoc_clause_at(line, op_col)))) { + if (word != "" && (unconditional || jit_heredoc_opener_is_known_sink(jit_heredoc_clause_at(line, op_col, quote_in[i])))) { delim = word for (j = i + 1; j <= n; j++) { rest = lines[j] @@ -1579,10 +1608,12 @@ function jit_strip_heredoc_body(s, unconditional, n, lines, i, j, out, strip_ # "$(cat <( or a <( on the SAME line as the heredoc operator, so their presence overrides -# the allowlist unconditionally rather than being named in each sink pattern one at -# a time -- the same posture as the comment/quote suppression above: a check that -# can only ever turn a match into a non-match, never the reverse. +# >( or a <( on the SAME CLAUSE as the heredoc operator (#447: the argument this +# function receives is that clause, jit_heredoc_clause_at()s own return value, not +# necessarily the whole physical line any more), so their presence overrides the +# allowlist unconditionally rather than being named in each sink pattern one at a +# time -- the same posture as the comment/quote suppression above: a check that can +# only ever turn a match into a non-match, never the reverse. function jit_heredoc_opener_has_danger_token(line) { if (index(line, "|") > 0) return 1 if (index(line, "$(") > 0) return 1 @@ -1594,9 +1625,10 @@ function jit_heredoc_opener_has_danger_token(line) { function jit_heredoc_opener_is_known_sink(line) { if (jit_heredoc_opener_has_danger_token(line)) return 0 # cat writing to a file: the write target may be spelled before OR after the - # heredoc operator on the same physical line (cat > out < out - # are both ordinary bash), so this checks the whole line for a > rather than just - # the text before the operator. + # heredoc operator in the same clause (cat > out < out are + # both ordinary bash), so this checks the whole clause (#447: the caller now hands + # this the operator own clause, not necessarily the whole physical line) for a > + # rather than just the text before the operator. if (line ~ /(^|[;&|])[ \t]*cat([ \t]|$)/ && line ~ />/) return 1 # tee always takes its target as a plain argument, never a redirect. if (line ~ /(^|[;&|])[ \t]*tee[ \t]+[^ \t;&|\n]/) return 1 diff --git a/tests/test-pre-tool-hook.sh b/tests/test-pre-tool-hook.sh index cd28a568..2f64e4bb 100755 --- a/tests/test-pre-tool-hook.sh +++ b/tests/test-pre-tool-hook.sh @@ -728,6 +728,18 @@ echo "=== issue #447: a closing delimiter with a backslash mid-word closes where OUT=$(run_hook '{"tool_name":"Bash","tool_input":{"command":"hdcmd; tee /tmp/jit447-sink.txt <, tee, supertool, git commit -F -, gh --body-file -/-F -) are +plain awk EREs matched against a text span (the whole line before #447, the operator +own clause after #447) with ZERO quote-state tracking. A quoted ";" inside a +legitimate argument to a NON-sink command (python3 -c "x=1; tee y") still leaves the +literal substring "; tee y" sitting in the text being matched, and the `tee` regex +-- anchored on `(^|[;&|])[ \t]*tee[ \t]+...` -- cannot tell that the ";" and the +"tee" it is anchored against are both inside a double-quoted Python string rather +than being real shell syntax. jit_heredoc_opener_has_danger_token() has the identical +blind spot for `|`, `$(`, backtick, `>(`, `<(` (though that direction is the SAFE one +for a deny-style rule -- a quoted danger token only makes the override MORE likely to +fire, never less, so it cannot itself become a bypass the way the sink patterns can). + +What I did fix in this same round (genuinely #447-scoped, and kept): a bare, +quote-UNAWARE scan for ";"/"&" in jit_heredoc_clause_at() itself -- the function that +decides WHERE the clause boundary falls, as opposed to whether the regex matching +WITHIN that clause is quote-aware -- could forge a fake boundary off a quoted ";"/"&" +and truncate the clause wrong in either direction. Fixed by threading a per-line +quote state (jit_heredoc_quote_states()'s own quote_in[i]) through +jit_heredoc_clause_at() and only treating a ";"/"&" as a boundary when the state at +that character is 0. Verified this closes a real (non-security, over-cautious) +precision bug: `hdcmd; tee ";" <