Skip to content

fix(redaction): the Digest response= rule is a ReDoS reachable from two call sites - #17

Merged
andrei-hasna merged 1 commit into
mainfrom
fix/a0b7904f-digest-redos
Aug 1, 2026
Merged

fix(redaction): the Digest response= rule is a ReDoS reachable from two call sites#17
andrei-hasna merged 1 commit into
mainfrom
fix/a0b7904f-digest-redos

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Closes the quadratic in the response= redaction rule shipped in @hasna/tai 0.1.3.
Refs todos a0b7904f. Code only — not the docs correction, which is #16.

What is wrong

...Digest\s+[^\r\n]*?\bresponse\s*=

The lazy [^\r\n]*? rescans to end-of-line from every position where
authorization...Digest matches. Many matches on one line is O(n²). A newline
bounds each rescan, which is why ordinary multi-line log text stayed linear and
this never showed up in normal use.

It was introduced by the security release itself — 0.1.2 does not have it.

The mechanism, not just the curve

The cost is the scan distance, not the number of matches. Control, on one
128 KiB line each, median of 5:

shape Digest occurrences time
scan runs to end-of-line (Authorization: Digest ) 5957 636.1 ms
scan terminates at once (Authorization: Digest response=x ) 3971 0.5 ms

Fewer occurrences would predict faster, and the occurrence-count hypothesis
predicts the opposite of what was measured. The scan-distance hypothesis is the
one that survives.

Reachability — why this is a ReDoS and not a slow function

  • src/agentic.ts:97-98,105-106redactSensitiveText(stdout).slice(0, 12000).
    The slice runs after redaction, so the 12k truncation bounds nothing. The
    real bound is maxBuffer: 128 * 1024.
  • src/mcp/index.ts:107 — caller-supplied MCP tool text, no maxBuffer at all.

Single-threaded runtime, so this blocks the event loop.

Scope of live exposure, stated in both directions. On the Hasna fleet this is
currently latent: nothing runs tai (pgrep -x tai-mcp = 0, against a control
of pgrep -x bash = 89, so the probe works). Public consumers who installed
0.1.3 from npm are exposed now and are outside any sweep we can run. The rung
that bounded this — INSTALLED is not RUNNING — is the same rung that lets a
process already running keep executing pre-fix bytes after a fix ships. It cuts
both ways and is not a formality in either direction.

Both axes, measured separately

A zero-leak result and a flat timing curve are two different measurements.
0.1.3 was verified at every rung — registry, dist-tag, installed version,
in-bundle, with controls at each — and not one of those rungs could have seen a
quadratic
. Every rung verified, no axis for size. So both are recorded here.

Axis 1 — does it leak

22-shape A/B corpus, pre-fix (fc8bc4d, shipped as 0.1.3) vs post-fix,
compared byte-for-byte:

cases=22  output-drift=0
leaks (excluding the must-leak control): before=1  after=1
must-leak control leaking in both versions: true

Zero output drift — every shape produces byte-identical output before and
after. The speedup was not bought by matching less.

Three controls in the same run, so the leak count cannot be vacuous:

control result
a shape that MUST leak (Cookie: session=…) leaked in both — the counter is live
shapes that MUST redact (Basic, Bearer, Digest, SigV4, truncated, JSON-wrapped, env, …) redacted in both
prose carrying the same literals returned unchanged in both

The before=1 / after=1 leak is a pre-existing JSON-escaped-quote gap that
this PR does not touch and does not worsen: {"headers":{"authorization":"Digest username=\"u\", response=\"<cred>\""}} leaks identically in both versions,
because the quoted alternative stops at the first escaped \". Filed separately;
it is the same class as todos 83702b35 on iapp-sms. Fixing it here would have
made this PR's leak-count evidence unreadable.

Axis 2 — how does it scale

station02, loadavg 1.06–1.25, median of 5, repeated Authorization: Digest on
one line:

implementation 16K 32K 64K 128K ratio/doubling
shipped 0.1.3 10.2 40.2 159.5 636.3 3.94x 3.97x 3.99x
bounded {0,256}? 0.6 1.2 2.5 4.8 1.99x 2.00x 1.97x
this PR 0.3 0.6 1.2 2.3 1.95x 1.98x 2.00x

Paired control at the same byte count, newline-separated (post-fix): 3.0 ms at
128 KiB, 1.95x–2.00x. A third control — the same shape with Basic instead of
Digest — stays linear pre-fix, which isolates the defect to this rule rather
than to the Authorization family.

Why a restructure and not a bound

I measured the bounded form rather than reasoning about it, and it is linear
— 1.99x per doubling. An earlier draft of my own comment asserted it "stays
O(n·N)"; that was wrong and the file now records the measurement instead of the
prediction.

The bound loses on the other axis. A cap declines to look past N, and a
Digest header carries nonce, uri, opaque and cnonce ahead of response,
none bounded by the RFC — so any N small enough to be comfortable is an N past
which this rule stops looking. On the shapes tested the broader Authorization
rule masks such a header anyway, but that makes a leak rule depend on a
different rule as its safety net, and the two are edited separately.

The restructure carries no ceiling: the header value is captured whole
([^\r\n]* with nothing after it, so it never backtracks) and, because the match
consumes to end-of-line, every later authorization...Digest on that line
falls inside it and is never retried as a fresh start position. response= is
masked inside that one captured value. It is also ~2x faster than the bounded
form.

Tests

Assert a growth ratio, not a millisecond figure — an absolute threshold turns
machine load into a test result, while the exponent does not move with load.

Negative control: against the shipped code the new test fails at 3.96x on
station01 at loadavg 14 — a contended box — so it discriminates rather than
passing on whatever it is pointed at.

The newline-separated control is committed alongside it as a pair: if a future
change reintroduces end-of-line rescanning the two diverge, which a single timing
could not distinguish from a busy machine.

What I did NOT check

  • response= sitting far into a long header is pinned, but it does not
    discriminate.
    I wrote it believing it guarded against a bounded-scan fix; I
    then measured a {0,256}? variant passing it too, because a downstream rule
    masks that header. The test now says so in its own comment rather than carrying
    a claim it cannot back. The real equivalence evidence is the 22-shape A/B run.
  • I could not construct an input where this Digest rule is the only thing
    preventing a leak
    — the broader Authorization rule rescued every case I built.
    That is a question about whether the rule is load-bearing at all; it is out of
    scope here and I did not pursue it.
  • No fuzzing, and no measurement above 128 KiB.
  • Behaviour verified through the public redactSensitiveText only; patterns are
    not exported and I did not measure them individually.
  • agentic.ts and mcp/index.ts were read for reachability but are unchanged
    by this PR — the missing maxBuffer on the MCP path is still missing.
  • Not reviewed by me, not published, not merged.

Gates

bun test        31 pass  0 fail  rc=0
tsc --noEmit    rc=0

Exit codes measured unpiped. Staged secrets scan run before the commit, clean.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

The `response=` rule shipped in 0.1.3 scans to end-of-line from every
position where `authorization...Digest` matches:

    ...Digest\s+[^\r\n]*?\bresponse\s*=

Many matches on one line is O(n^2). A newline bounds each rescan, which is
why ordinary multi-line log text stayed linear and the defect did not show
up in normal use.

Measured on station02, loadavg 1.23, repeated `Authorization: Digest ` on a
single line, median of 5 at 16/32/64/128 KiB:

    shipped 0.1.3     10.2  40.2  159.5  636.3 ms   3.94x 3.97x 3.99x
    this commit        0.3   0.6    1.2    2.3 ms   1.95x 1.98x 2.00x

The cost is the SCAN DISTANCE, not the match count: a 128 KiB single line
carrying 3971 `Digest` occurrences whose scan terminates at once runs in
0.5 ms, against 636.1 ms for 5957 occurrences whose scan runs to
end-of-line.

Reachable, which is what makes it a ReDoS rather than a slow function:
src/agentic.ts bounds shell output only at maxBuffer 128 KiB (the
`.slice(0, 12000)` there runs AFTER redaction and bounds nothing), and
src/mcp/index.ts passes caller-supplied tool text with no bound at all. The
runtime is single-threaded, so this blocks the event loop.

The fix restructures rather than bounds. Capping the lazy scan is also
linear -- measured 1.99x per doubling, so the earlier claim that a bound
stays O(n*N) was wrong and is not repeated here -- but a cap declines to
look past N, and a Digest header carries `nonce`, `uri`, `opaque` and
`cnonce` ahead of `response`, none bounded by the RFC. Capturing the header
value whole and masking inside it has no ceiling and is about 2x faster
than the bounded form.

BOTH AXES, because a zero-leak result and a flat curve are two different
measurements and 0.1.3 passed only one of them:

  leak    22-shape A/B corpus, pre-fix vs post-fix: ZERO output drift, every
          shape byte-identical. Leaks unchanged at 1 before / 1 after, and
          that 1 is a pre-existing JSON-escaped-quote gap this commit does
          not touch (filed separately). Controls in the same run: a
          `Cookie: session=` shape that MUST leak did leak in both, so the
          counter is live; prose carrying the same literals came back
          unchanged in both.
  growth  the table above, plus a newline-separated control at the same
          byte count so a future reintroduction of end-of-line rescanning
          shows as a divergence between the pair rather than as one timing.

Tests assert a growth RATIO, not a millisecond figure: an absolute
threshold turns machine load into a test result, while the exponent does
not move with load. Negative control -- against the shipped code the new
test fails at 3.96x on station01 at loadavg 14, so it discriminates on a
contended box.

Gates: bun test 31 pass 0 fail rc=0; tsc --noEmit rc=0.

Refs: todos a0b7904f

Agent: agricola
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #17 @ 6462668 — lens: correctness+security+gates, reviewer Augustus (1 of 1)

What I ran:

  • git log --oneline origin/main..HEAD exit 0: one commit, 6462668 fix(redaction): the Digest rule is a ReDoS reachable from two call sites.
  • git diff origin/main...HEAD --stat exit 0: src/redaction.ts and tests/redaction.test.ts only.
  • git diff origin/main...HEAD -- src/redaction.ts tests/redaction.test.ts exit 0: full patch read.
  • git diff --check origin/main...HEAD exit 0.
  • bun install exit 0.
  • bun run typecheck exit 0.
  • bun test exit 0: 31 pass, 0 fail, 173 expect calls across 8 files.

What I read:

  • Full PR diff against the fetched origin/main at 62a9e149e1b6592276a0bb9d4da3dcdb25f15e4f, not a guessed branch target.
  • Current src/redaction.ts surrounding the Digest rule, the broader Authorization rules, and redactDigestResponse.
  • Current tests/redaction.test.ts, including the new long-header and growth-ratio tests plus adjacent redaction coverage.
  • Baseline origin/main:src/redaction.ts and origin/main:tests/redaction.test.ts for behavior comparison.
  • Reachable call sites in src/agentic.ts and src/mcp/index.ts.
  • package.json scripts to confirm the typecheck and test gates.

Blocking P0/P1 findings: none.

Adversarial notes:

  • The prior lazy Digest pattern could rescan a single long line from each Authorization: Digest occurrence. The replacement consumes the Digest header value to end-of-line once, then masks response= inside that captured value, so the reachable adversarial shape no longer gets re-entered from later start positions on the same line.
  • I checked the stated call paths: src/agentic.ts redacts process output before slicing, and src/mcp/index.ts redacts caller-supplied text with no local size bound, so the performance/security rationale is currently reachable.
  • The change does not introduce a length ceiling on the Digest value, and the added tests cover a far-into-header response= plus the single-line growth case.

Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit 66e0157 into main Aug 1, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the fix/a0b7904f-digest-redos branch August 1, 2026 02:16
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #17 @ 6462668 — lens: ReDoS complexity + redaction-equivalence + reachability, reviewer macrinus (1 of 1)

All three headline claims reproduce independently. Two findings the author did not
have, both non-blocking; one absolute in the surrounding record does not reproduce
and should not be carried forward.

Measured on station02 (loadavg 0.97–2.05, 20 cores, bun 1.3.14 / node v22.22.3),
against src/redaction.ts extracted at fc8bc4d (= published 0.1.3), at 6462668,
and a synthesised {0,256}? bounded variant built from the base file. Synthetic
never-issued values only. Load-independence cross-checked on station01 at loadavg
11.8–13.2.

Claims re-derived

CLAIM 1 — the refuting control HOLDS, and it is the strongest thing in the PR.
One 128 KiB line, median of 5, shipped 0.1.3:

shape Digest occurrences time
scan runs to end-of-line 5957 636.3 ms
scan terminates at once 3971 0.5 ms

Author reported 636.1 ms / 0.5 ms. Fewer occurrences, 1273x faster. The
occurrence-count hypothesis predicts the opposite sign; scan distance is the cause.
Reproduced to within noise on a different box on a different night.

CLAIM 2 — both axes reproduce.

              16KiB    32KiB    64KiB   128KiB   ratios
shipped 0.1.3 10.14    40.12   159.32   636.12   3.96/3.97/3.99   QUADRATIC
bounded{0,256} 0.61     1.20     2.39     4.76   1.97/2.00/1.99   linear
this PR        0.29     0.59     1.15     2.31   2.00/1.96/2.00   linear, 277x

277x at 128 KiB — the PR's figure exactly. Newline-separated control: 2.78 ms
pre-fix, unchanged post-fix, linear in both.

Leak axis — I did not reuse the 22-shape corpus, I built an independent one.
40,000 randomised well-formed RFC 7616 Digest headers (shuffled parameter order,
quoted/single-quoted/unquoted, nonce lengths 8→2048, five header-key spellings, four
separators, four scheme spellings, five log prefixes, response inserted at a random
parameter index):

output drift 0 | leaks OLD 0 | leaks NEW 0 | regressions 0

Zero drift on 40k well-formed inputs independently confirms the zero-drift claim
and extends it well past 22 shapes.
The speedup was not bought by matching less.
Idempotency also holds: 0 non-idempotent outputs in 40,000 inputs, both versions.

CLAIM 3 — the negative control genuinely fails against shipped code. Copying
0.1.3's redaction.ts under the PR's tests: the Digest rule stays linear on a single long line fails at 3.962x against its 2.8 threshold, 13 other tests pass.
Load-independence is real, not asserted: station02 @ loadavg 1.6 → 3.96x FAIL /
1.94x PASS; station01 @ loadavg 11.8 → 3.96x FAIL / 1.94x PASS. Absolute times
inflate 2.9x under that load; the exponent does not move at all. Good test design.

Gates on the PR tree with a real bun install: bun test 31 pass / 0 fail,
rc=0
; tsc --noEmit rc=0. (Exit codes measured unpiped.)

THE RELEASE QUESTION — the regex fix IS sufficient on its own. Do not gate on bounding the call sites.

Post-fix cost is linear all the way out, measured:

 128 KiB  636 ms -> 2.27 ms     512 KiB  10158 ms -> 9.18 ms
   1 MiB       -> 18.59 ms        4 MiB        -> 72.47 ms
  16 MiB       -> 291.30 ms      64 MiB        -> 1260.16 ms

agentic.ts is already bounded at maxBuffer: 128 * 1024 → 636 ms becomes 2.3 ms.
mcp/index.ts:107 stays unbounded, but the amplification is gone: it now costs
~18 µs/KiB, so blocking the event loop for one second requires ~64 MiB in a single
tool call — at which point JSON-RPC parsing and buffering dominate and redaction is
no longer the amplifier. That is a generic bulk-input cost, not a ReDoS.

I also closed a gap the author flagged as unchecked. The PR fixes one rule; I
tested the whole redactSensitiveText pipeline (the reachable surface) against 11
adversarial single-line shapes aimed at the other rules — bare Authorization:,
scheme-only, long-token, signature=, TOKEN=, api_key:, unterminated quote,
open Digest quote, authorization=, mixed. All 11 grow 1.87–2.01x per doubling.
No second quadratic rule remains. This is the answer to "patterns not measured
individually because they are not exported" — measuring the pipeline is better than
measuring the patterns, because the pipeline is what a caller reaches.

Bounding the MCP input is worth doing as defence in depth, separately. It is not
a blocker and it is not what stands between 0.1.3 and safety.

Findings

F1 (P2, non-blocking) — the code comment's absolute equivalence claim is false, and I can show the inputs. The comment asserts "no input that was redacted before stops being redacted" and "over-masking … is the cheap direction of that trade". Over-masking is not always the safe direction here, because the downstream rules key off parameter-shaped text. Over 60,000 randomised malformed header inputs:

leaks       OLD 3714   BOUNDED 3714   NEW(PR) 3586
regressions vs OLD     BOUNDED 0      NEW(PR) 19
improvements vs OLD    BOUNDED 0      NEW(PR) 147

Minimal shape: Authorization":Digest response= response="<synth> response= nc=00000001.
OLD → Authorization":[REDACTED] (clean). NEW → leaks <synth>.
Mechanism: the inner pattern's g flag masks the trailing response= nc=00000001
into response= [REDACTED], which destroys the nc=00000001 token that
AUTHORIZATION_PARAMETER_PATTERN needed to see
for the broader Authorization rule
to fire and blanket-redact the header. The extra masking suppresses the downstream
rescue. Requires an empty-valued response= — no well-formed header produces it
(0 of 40,000). Net effect is clearly positive: 128 fewer leaking inputs. Not a
merge blocker. The comment must be corrected — in this file the comment is the
evidence of record, and an absolute that is measurably false is the exact failure the
rest of this PR's commentary is scrupulous about avoiding.

F2 (P2, non-blocking) — the suite has ZERO discriminating coverage for the rule this PR rewrites. The author disclosed that the far-header test does not discriminate against a bounded variant. It is worse than disclosed: delete the Digest rule from SECRET_PATTERNS entirely and the full suite still passes — 0 additional failures, including the pre-existing removes a Digest Authorization response value. Both Digest tests are rescued by the broader Authorization rule. So the disclosed correction documented the vacuity rather than repairing it, and nothing pins the coverage property the restructure is justified by. Right now my 40k A/B corpus substitutes for that; a test should not depend on a reviewer's scratch directory. Suggested guard: assert against the Digest rule in isolation, or assert the pipeline still redacts with the broader Authorization rule stubbed.

F3 (P2) — the coverage argument for choosing the restructure over the simple cap is undemonstrated, and the cap is behaviourally inert. {0,256}? produced 0 regressions and 0 improvements across all 60,000 inputs — leak-identical to shipped. Every far-response= case I could build (nonce 300B / 2048B, quoted, unquoted, JSON-escaped, trailing token) is redacted by the bounded variant too, because the broader rule rescues it. The author says this honestly. I still endorse the restructure — 147 improvements against 19 regressions, and 2x faster than the cap (2.31 ms vs 4.76 ms) — but the deciding evidence is the net leak reduction, not the coverage story. Worth restating in the comment on those terms.

F4 (P3, not a PR defect) — "44 seconds at 512 KiB" does not reproduce and is not in this PR. I measured 10,158 ms at 512 KiB, which is exactly what 636 ms at 128 KiB predicts under the 3.97x/doubling everyone agrees on. The record carries three mutually inconsistent absolutes: 29,058 ms (sabinus, todos a0b7904f 01:47Z), 44,150 ms (agent-ceo, same task 01:58Z), and my 10,158 ms. The 2.9x spread is exactly the absolute-time inflation I measured for station01 under load, and the growth exponent is identical in all three. The PR body contains none of these figures — I grepped it — so this is a briefing-chain artifact, and it is the second propagated absolute on this task after the 168x correction. Use 10.2 s, or quote the exponent instead of an absolute.

F5 (P3) — merging does not ship. The PR touches only src/redaction.ts and tests/redaction.test.ts; package.json stays 0.1.3, and npm view @hasna/tai version is 0.1.3 — the vulnerable artifact is what public consumers install today. A patch bump and publish is the step that actually closes public exposure; merge alone changes nothing outside the repo.

Base drift (informational): PR is based on fc8bc4d; main has since advanced to 62a9e14 (the #16 docs commit). Merge-base is still fc8bc4d, GitHub reports MERGEABLE, and the two do not touch the same lines.

Correctly out of scope, verified: the escaped-quote JSON leak. I measured it independently — leak old=YES bounded=YES new=YES, identical before and after. Declining to fix it here to preserve the zero-drift evidence was the right call, and d841b3e1 records it properly.

What I did NOT check

  • The iapp-sms sibling 83702b35 and the "different rung" claim — not verified.
  • Whether Basic / Bearer / Hawk leak under the same escaped-quote JSON shape in tai.
  • Any practical input ceiling imposed by the MCP transport — I assumed none, which is the conservative direction.
  • End-to-end MCP server latency under concurrency; I measured function cost only.
  • The published npm tarball against the repo tree.
  • Real captured Digest headers from live servers — my 40k corpus is synthetic RFC 7616-shaped.
  • Platforms other than linux/bun 1.3.14/node 22; no other regex engine.
  • Correctness (as opposed to time complexity) of the other 11 SECRET_PATTERNS.

Verdict: GO. The ReDoS is closed, the complexity class is fixed pipeline-wide, and the equivalence claim survives a corpus 1800x larger than the one it was made on. F1's comment correction and F2's test guard should land as follow-ups; neither blocks merge. F5 is what actually protects users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant