fix(hooks): stop a printer on one line from blocking secrets on the next - #535
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
The secret-leak detector's `bare $VAR` and `named_dump` patterns used
`[^|;&]*` as the gap between a printing command and a variable. That class
does not exclude newlines, so an `echo` anywhere earlier in a multi-line
command poisoned every later secret expansion in the same command.
The discriminating pair, both with identical token usage:
echo "=== survey ==="
GH_TOKEN="$GH_TOKEN_NOS" gh api repos/x/y # blocked
GH_TOKEN="$GH_TOKEN_NOS" gh api repos/x/y # allowed
Nothing in the second line prints anything. The block came entirely from an
unrelated heading on the line above.
This made per-owner token surveys impractical -- the fleet work needs
`GH_TOKEN_SWM` / `GH_TOKEN_NOS` / `GH_TOKEN_TWM` reads, and those scripts
open with a heading almost every time. A security control that blocks routine
safe work trains people to route around it, which costs more than the false
positives do.
An earlier report attributed this to `for` loops. That was wrong: the loop is
incidental. The trigger is whether a printing command precedes the expansion
anywhere in the command text.
Scope the gap to a single line, while keeping a backslash-newline as the
continuation it is -- `echo \<newline> "$SECRET"` is one logical command and
must still block.
Scoping the gap would have silently dropped one true positive: an unquoted
heredoc (`cat <<EOF` ... `$SECRET`) expands and prints, and was caught only
because the old gap spanned newlines. It now has its own rule, so it blocks
for a stated reason rather than as a side effect of a bug. A quoted delimiter
(`<<'EOF'`) disables expansion and is correctly allowed.
Also unset CDPATH in the test file. With `CDPATH=/Users/andrewrich/Developer`,
`cd "$(dirname ...)/.."` resolves through CDPATH to `Developer/scripts` and
prints that path to stdout, so SCRIPT_DIR captured `pwd` doubled and
newline-joined, HOOK pointed at a nonexistent file, and all 50 checks exited
127. The suite reported 50/50 when invoked directly and 1/50 when its output
was captured -- a false green that depended on how it was called. Every other
suite in scripts/tests already unsets CDPATH; this one had missed the
convention. hook-block-all.sh:6 does too, so production was never affected.
Six tests added: the discriminating pair, the for-loop shape, the
backslash-continuation true positive, and both heredoc forms. 50 pass, up
from 44.
Scope note: this is a usability fix, not a security fix. Measured against
today's transcripts, zero leaks arrived through `.tool_input.command` -- the
only channel this hook inspects. That measurement and its consequences are
being handled separately.
Advances #485.
Claude-Session: https://claude.ai/code/session_01PTUKVrdfDTU3ZH149e9pTY
twistedmelonman
force-pushed
the
claude/fix-secret-leak-multiline-918d6251
branch
from
September 17, 2026 02:31
4ec8e90 to
5a4ac80
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
No BLOCK criteria found. Changes fix a false-positive bug in the gap pattern (now correctly stops at newlines, allowing backslash-continuations). The VERDICT: PASS |
1 task
twistedmelonman
deleted the
claude/fix-secret-leak-multiline-918d6251
branch
September 17, 2026 02:37
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.
The bug
The detector's
bare $VARandnamed_dumppatterns used[^|;&]*for the gapbetween a printing command and a variable. That class does not exclude
newlines, so an
echoanywhere earlier in a multi-line command poisoned everylater secret expansion in the same command.
The discriminating pair — identical token usage, differing only in an
unrelated heading:
Nothing on the second line prints. The block came entirely from the line above.
An earlier report attributed this to
forloops. That was wrong — the loop isincidental. The trigger is whether a printing command precedes the expansion
anywhere in the command text.
Why it matters
The fleet work needs per-owner token reads (
GH_TOKEN_SWM/GH_TOKEN_NOS/GH_TOKEN_TWM), and those survey scripts open with a heading nearly everytime. A control that blocks routine safe work trains people to route around
it, which costs more than the false positives.
The fix
Scope the gap to one line, while keeping a backslash-newline as the
continuation it is —
echo \<newline> "$SECRET"is one logical command andmust still block.
Scoping the gap would have silently dropped one true positive: an unquoted
heredoc (
cat <<EOF…$SECRET) expands and prints, and was caught onlybecause the old gap spanned newlines. It now has its own rule, so it blocks for
a stated reason rather than as a side effect of a bug. A quoted delimiter
(
<<'EOF') disables expansion and is correctly allowed.Bonus: a false green in the test suite
CDPATHis set to/Users/andrewrich/Developerin this environment. With arelative argument,
cdsearchesCDPATHbefore.and prints the resolvedpath to stdout on a match — so
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"landed in
Developer/scripts, capturedpwddoubled and newline-joined, andleft
HOOKpointing at a nonexistent file. All 50 checks exited 127.The suite reported 50/50 when invoked directly and 1/50 when its output was
captured. Invocation-dependent green is the false-green shape this codebase
has been bitten by repeatedly.
Every other suite in
scripts/testsalready unsetsCDPATH; this one hadmissed the convention.
hook-block-all.sh:6does too, so production was neveraffected — only the test.
Tests
50 pass, up from 44. Six added: the discriminating pair, the for-loop shape,
the backslash-continuation true positive, and both heredoc forms.
Full suite: 10/10 suites, 304 assertions, verified under the exact invocation
that previously produced 49 failures.
Scope note
This is a usability fix, not a security fix. Measured against today's
transcripts, zero leaks arrived through
.tool_input.command— the onlychannel this hook inspects. 84 leak sites across 12 distinct values all came
through tool results (69), assistant text (9), and attachments (6).
That measurement, and the output-side filter it points to, are being handled
separately. This PR should not be read as closing the leak.
Advances #485.
https://claude.ai/code/session_01PTUKVrdfDTU3ZH149e9pTY