fix(redaction): stop the quoted-value scan terminating on an escaped quote - #24
Conversation
…quote
`tai.redact` — the tool whose only job is redaction — returned the credential
with a `[REDACTED]` marker beside it. The rule engaged and ended in the wrong
place, which is worse than no rule: the next reader greps for the marker, finds
it, and stops looking.
Reachable from an ordinary `JSON.stringify(req.headers)`. No contrivance and no
attacker: RFC 7616 Digest and Hawk carry quoted parameters natively, so one
plain serialization already produces
{"headers":{"authorization":"Digest username=\"u\", response=\"<cred>\""}}
The quoted-value body `(?:(?!\2)[^\r\n])*` stops at the first quote CHARACTER
regardless of the backslash escaping it, so the rule masked the four characters
before that inner quote and emitted
`"authorization":"[REDACTED]"u\", response=\"<cred>\""`.
Pre-existing and byte-identical in 0.1.3, 0.1.4 and main, so there is nothing to
revert.
THE FIX IS ONE MECHANISM, NOT A LIST OF SHAPES. The escape-aware body is now a
single shared constant used by all eight rules that model a quoted value, rather
than eight copies of a quoting model — eight copies of a subtly wrong model is
what this defect was, and this file has already grown four list-shaped guards.
The branches are deliberately DISJOINT on their first character. The obvious
form `(?:\\.|(?!\2)[^\r\n])*` lets a backslash be consumed by either branch,
which is a ReDoS wherever the closing quote is required and the branch can fail:
measured against a mutant carrying it, 0.25/0.57/1.54/3.96/10.35/27.33 ms at
N=18..28 backslashes (~1.6^N) versus a flat ~0.001 ms for the shipped form. This
file has shipped two quadratic rules already; a third arriving through the fix
for a leak would be a poor trade.
Evidence, synthetic canary throughout — no real credential used or rendered:
4 schemes x JSON nesting depth 0/1/2, both controls firing in every run
before 6 of 12 cells leak (depth-1 Digest and Hawk misleading)
after 4 of 12, all of them the depth-2 rung left open below
end-to-end through the MCP tool `tai.redact` on the built dist
installed 0.1.5 depth-1 Digest MISLEADING, Hawk MISLEADING
this build both ok, must-redact control ok, must-leak control leaks
base-vs-patched drift over 191 inputs
10 outputs changed, all 10 strictly safer, 0 needing review, 0 non-idempotent
The canary matches no provider-prefix rule in this file, and that is
load-bearing: an `sk-`-prefixed canary is masked by the prefix rule while the
structural rule is still broken, so the probe passes for the wrong reason. That
is how a sibling implementation was read as clean on shapes it in fact leaks on.
STILL OPEN, recorded in docs rather than silently left: headers serialized into
a JSON string field (two `JSON.stringify` levels) leak for all four schemes.
That is the rung above this one — the backslash sits between `authorization` and
its `:`, so no rule engages and no marker is printed. Closing it means teaching
the key prefix to cross escaping, which is adjacent to the normalisation-layer
decision ruled a documented won't-fix, and is kept out so this change stays one
mechanism wide.
Tests introduce the axis rather than extending the list: the corpus carried 21
`digest` fixtures but zero escaped quotes and zero Hawk, so no amount of adding
cases along the existing axes could have expressed the shape. The two defect
tests fail on unfixed main and pass here.
Refs: todos d841b3e1
Agent: atticus
|
[REVIEW] GO — #24 @ eac8401 — lens: credential-leak, reviewer Lucretius (1 of 1) Independent review. Everything below was re-measured from the two commits, not read from the PR body. Synthetic canary throughout ( Premise: CONFIRMED, not assumedThe leak reproduces on the base commit The marker sits beside the surviving credential. The pathology is exactly as filed. Matrix — 4 schemes x JSON nesting depth 0/1/2, both builds
Leaking cells: BASE 6/12 -> HEAD 4/12. Zero regressions. This reproduces the PR's own table exactly. Both controls fired in both builds — must-redact fires, must-leak stays visible — so the probe is not vacuous in either direction. 1. Scope discipline — HELDThe two cells that changed are exactly the two that emit a marker (rule engaged, terminated wrong). All four marker-less depth-2 cells are byte-unchanged base->head. The change did not wander into 2. Over-redaction — the trap this class of fix falls into: PASSEDI ran a 15-case corpus using neighbour sentinels — legitimate non-credential fields placed after a credential field, which vanish if the scan over-consumes. Two independent properties per case: canary absent (leak), sentinel present (over-redaction). Shipped fix: zero over-redactions introduced. Ordinary prose carrying the literal stays visible; escaped quotes in non-credential JSON return byte-identical; Proving the control can actually fail — I built a deliberate over-masking mutant ( The over-masker scores the same 4/12 on the leak matrix as the shipped fix — a leak count alone cannot tell a correct fix from this bug — but the sentinel detector fires on 5 cases the shipped fix passes. The fix is genuinely not an over-masker. 3. Test axis — genuinely INTRODUCEDBase 4. Sabotage — fails, and for the RIGHT reasonReverting only the terminator (old body restored, new tests intact) fails exactly the two defect tests, each with its own named assertion: Not incidental failures — the second asserts the marker-beside-credential pathology directly. 5. ReDoS claim — independently verifiedThe disjoint-branch design is load-bearing, not stylistic. Failing suffix, N backslashes:
Exponential versus flat, matching the PR's measurements in shape and order of magnitude. Gates
Findings — none blockingF1 (P1, non-blocking follow-up) — the negative control cannot fail on over-masking. F2 (P2, pre-existing, NOT caused by this PR) — allowlisted non-secret values are masked. F3 (P2, pre-existing, disclosed) — perf flakiness is slightly worse than stated. F4 (P3, accuracy nit) — the body says "21 What I did NOT check
Verdict: GO. The fix is one mechanism wide, correct, scope-respecting, and closes a live fleet-wide leak in published 0.1.5 where the marker was actively telling readers the line was clean. F1 should land as a follow-up before the next edit to |
|
[REVIEW] GO — #24 @ eac8401 — lens: correctness+security+gates, reviewer Augustus (1 of 1) What I ran:
What I read:
Blocking P0/P1 findings: none. Non-blocking follow-ups:
|
0.1.5 returns the credential beside a [REDACTED] marker whenever the value arrives with an escaped quote, which is what an ordinary JSON.stringify(req.headers) produces for RFC 7616 Digest and Hawk. Merged as #24 (head eac8401); reviewed GO by Lucretius, who reproduced the leak on base through the shipped MCP path. Ships the merged fix. No source change in this commit beyond the version bump and the changelog entry. Agent: Papinian
1 participant
Fixes the live credential leak in published
@hasna/tai0.1.5, installed on every station. Tracked as todosd841b3e1.Do not merge this on my say-so — I am the author and this is security work. A reviewer is being arranged separately.
The defect
tai.redact— the tool whose only job is redaction — returns the credential with a[REDACTED]marker beside it. The rule engages and terminates in the wrong place, which is worse than no rule at all: the next reader greps for the marker, finds it, and stops looking.It is reachable from an ordinary
JSON.stringify(req.headers). No contrivance, no attacker. RFC 7616 Digest and Hawk carry quoted parameters natively, so one plain serialization already produces the shape:The quoted-value body
(?:(?!\2)[^\r\n])*stops at the first quote character, regardless of the backslash escaping it. So it masked the four characters before that inner quote and emitted"authorization":"[REDACTED]"u\", response=\"<cred>\"".Pre-existing and byte-identical across 0.1.3, 0.1.4 and main — nobody regressed anything and there is no revert to reach for.
The fix is one mechanism, not a list of shapes
The escape-aware value body is a single shared constant used by all eight rules that model a quoted value. Eight copies of a subtly wrong quoting model is exactly what this defect was, and this file has already grown four list-shaped guards.
The two branches are deliberately disjoint on their first character, and that is load-bearing. The obvious form
(?:\\.|(?!\2)[^\r\n])*lets a backslash be consumed by either branch — a ReDoS wherever the closing quote is required and the branch can fail. Measured against a mutant carrying it:Roughly
1.6^N. This file has shipped two quadratic rules already; a third arriving through the fix for a leak would be a poor trade.Evidence — both directions
Synthetic canary throughout. No real credential was used or rendered at any point.
4 schemes x JSON nesting depth 0/1/2, both controls firing in every run:
06cc7de)End-to-end through the MCP tool
tai.redact(JSON-RPC stdio against the built dist, request serialized byjson.dumpsso the escaping is a real serializer's):Negative control — base-vs-patched output drift over 191 inputs: 10 outputs changed, all 10 strictly safer (canary survived on base, masked here), 0 needing review, 0 non-idempotent. A change that redacts everything scores zero leaks and is a different bug; this one changes nothing else. Ordinary prose carrying the same literal stays visible, escaped quotes in non-credential JSON come back byte-identical, and neighbouring audit fields beside
authorization=deniedsurvive.The canary matches no provider-prefix rule in this file, and that is the whole point. An
sk--prefixed canary is masked by the prefix rule while the structural rule is still broken — the probe passes for the wrong reason. Varying only the canary on one fixed shape:sk-prefixedsms_prefixedConsequence, and it reverses the brief I was given:
iapp-smsis not clean on these shapes and was not transplanted. On a plainAuthorization: Digest ... response="<tok>"header it returnsAuthorization: [REDACTED], realm="r", ... response="<tok>"wheretaireturnsAuthorization: [REDACTED]—taihas a dedicated Digest rule thatiapp-smslacks. Across the same 12-cell matrixiapp-smsleaked 8 cells totai's 6. Copying it wholesale would have regressed this repo. Recorded indocs/redaction.md.Tests introduce the axis
The corpus carried 21
digestfixtures, zero escaped quotes, zero Hawk (verified with a positive control). The suite's fixtures could not express this shape, so adding cases along existing axes would never have found it.The two defect tests fail on unfixed main and pass here:
The other four added tests are controls and guards, not regression tests, and pass on both sides by design — stated so nobody reads six passing tests as six proofs.
Gates
Deliberately NOT fixed, and recorded rather than left silent
Headers serialized into a JSON string field (two
JSON.stringifylevels — pino/winston/axios-error) still leak for all four schemes. That is the rung above this one: the backslash sits betweenauthorizationand its:, so no rule engages and no marker is printed. Closing it means teaching the key prefix to cross escaping, which is adjacent to the normalisation-layer decision already ruled a documented won't-fix on todos4afd4361. Kept out so this change stays one mechanism wide. Added as a row indocs/redaction.md.Pre-existing flaky tests, unrelated to this change
The perf-growth family flakes on a loaded box. Measured on the unmodified base at loadavg ~18: 2 of 5 suite runs failed (
the cookie rule stays linear on a cookie-dense single line,the Digest rule stays linear on a single long line); this branch passed 5 of 5. An interleaved A/B of the specific assertion gave base 1.99 and patched 2.00 against a 2.8 threshold — identical. Not caused by this PR; filed separately rather than fixed here, so this PR's evidence stays about one thing.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.