Repository navigation
gate: error-render cannot see a caller-controlled string reaching a cause:, and three log-injection holes shipped under it #97
Description
Found during the enterprise-readiness sweep. bun run error-render reported clean across 3,323 files while three real log-injection holes were live in packages/auth alone.
What the check actually covers
scripts/error-render.ts refuses a parameter typed unknown/any reaching a cause:/fix: through ${x}, JSON.stringify(x) or String(x). Its own header says so. That rule exists because the bug it names shipped three times, and it is a good rule.
It does not cover a value that is already typed string and is nonetheless caller-controlled. A string cannot throw on render, so the check has nothing to object to — but it can still carry a newline, and a newline in a cause: writes a second log line the operator reads as genuine.
The three that shipped under a green check
| Site | Value | Reachable by |
|---|---|---|
packages/auth/src/id-token.ts:153 |
claims.iss — a field of the JWT the caller just presented |
an unauthenticated stranger with one crafted token |
packages/auth/src/oauth-exchange.ts providerDetail() |
a remote token endpoint's error_description / raw body |
a compromised, impersonated or proxied IdP |
packages/auth/src/errors.ts accountLocked() |
key, from ipKey(ip) — whatever address string the caller passed |
anyone who can influence the forwarded IP |
The first also hand-wrote its quote pair, so a forged iss could close the sentence and add a line.
All three are fixed in the enterprise-readiness branch, each with a test that feeds a value carrying "\n and asserts the rendered cause contains no raw newline. The fixes were reverted and re-run to prove the tests fail without them.
Why this is worth an issue rather than just the three fixes
The repo believes a green error-render means cause strings are safe. It means something narrower. packages/auth/CLAUDE.md now records the rule locally — which values are foreign, that they are rendered at their source, and that the gate cannot see any of it — but that is one package's discipline, not a mechanism, and axiom 3 says a convention that is not a build error does not exist.
Options, roughly in cost order
- Do nothing but document it. Cheapest, and already half done. The risk is exactly what happened here: the next reader trusts the green.
- Flag
${…}of any non-literal binding inside acause:, with an opt-out comment. Crude, and it would fire on a lot of authored prose — but the opt-out makes each exception a decision someone wrote down, which is the property that actually prevents recurrence.packages/auth's deliberate${provider}exclusion is a worked example of what such a comment says. - Render at the boundary instead of at the call site — make the logger and problem-document serialisers escape control characters in
cause, so no call site can get it wrong. Strictly better if it is feasible; it is one place instead of every place. Needs a check that it does not mangle the many causes that are legitimately multi-clause prose.
Option 3 looks like the real answer and was out of scope for the sweep.
Related, same family
A second blind spot found in the same pass: checkErrorFixes only scans literal fix: properties, so packages/db's SQLSTATE_FIXES lookup table is invisible to it. Those six fix strings were verified by hand against BANNED_PHRASES/COMMAND_TOKENS; nothing would catch a seventh.
Activity
sebyx07 commentedon Aug 16, 2026
One refinement from the agent that did the
packages/authsweep, worth capturing before this is scoped.The two failures are different, and only one is mechanised.
error-renderguards against a factory that throws while formatting — anunknownwhosetoStringexplodes. What shipped here is hostile text that formats fine and forges a log line. Same file, same interpolation, unrelated failure modes. Conflating them is why the green read as coverage.The useful artefact is a placement rule, not a checker. From the fix that worked:
Concretely,
oauth-exchange.ts'sproviderDetail()is where a remote token endpoint's bytes enter the package, so rendering happens there — and every current and future caller inherits it. Rendering at theoauthExchangeFailedfactory instead would have quoted the prose the package authors ('the response carried no access_token') and made every readable message unreadable, which is how a safety measure gets reverted six months later.If a check is built, the cheap honest increment is probably not taint analysis but a narrower rule: flag a
cause:/fix:interpolation of a binding read off aResponse, a parsed JSON payload, or a parameter the package has declared foreign. That is a fraction of the cost of option 3 in the description and catches the class that actually shipped.Worth noting the counter-example too, because a checker will fire on it:
packages/authdeliberately does not render${provider}. On every shipped path it isproviderFor(id).idor a segment already validated against the registry — boot config, not request data — and rendering it would manglerestartAt's fix line intoGET /auth/oauth/"github", trading a runnable instruction for a theoretical hole. Any rule needs an opt-out, and the opt-out comment is the part that prevents recurrence, because an unexplained exception gets "fixed" by the next reader and the fix line breaks.developerz-ai commentedon Aug 16, 2026
I couldn't reliably classify or scope this report right now, so I haven't guessed or handed it off. A maintainer will review it.
🤖 Posted by developerz.ai — the maintainer agent, not a human.
sebyx07 commentedon Aug 16, 2026
A second blind spot in the same family, found while fixing the reference app for #98.
scanFixes(packages/cli/src/ts-scan.ts:164) only readsfix:object keys. So everyfixpassed as a positional argument is unchecked — which is howinvariant()andassert()take theirs.Concretely, these were all wrong and all invisible to the
errorsstep:examples/dummy/apps/web/shared/rows.ts:17x db queryinvariant()'s 4th positional argexamples/dummy/scripts/test-setup.ts:94x db seed --listassert()'s positional arg and outside the scanned globsThe first was found only because a human-directed sweep grepped for it; the second is still unfixed.
So the
errorsstep's coverage is narrower than it reads in two independent ways, and this issue now covers both:error-renderrefuses anunknownreaching acause:, and is blind to a caller-controlledstring(the original report — three log-injection holes shipped under it).scanFixesvalidatesfix:object keys only, and is blind to every positionalfixin the repo.They share a shape: the check is real and correct, and its coverage is assumed to be total when it is not. Whoever scopes a fix should probably do both at once, because closing one and leaving the other reproduces the same false confidence.
Also surfaced and needing a separate decision — the reference app cites two commands that exist in neither
COREnorPLANNED_COMMANDS:x setup --json—examples/dummy/bin/setup:7x db seed—examples/dummy/package.json:23,CLAUDE.md:25,README.md:15,packages/db/CLAUDE.md:29,packages/db/README.md:57, and thetest-setup.ts:94fix aboveThat is the app's documented setup flow referring to commands the CLI does not ship. The repair is either shipping
x db seed/x setup, or rewriting the flow to what actually works — a framework decision, not a doc fix, so it was deliberately left alone rather than widening #98.sebyx07 commentedon Aug 17, 2026
Live confirmation of this gap, found by review on #131 rather than by the gate.
Seven
fix:sites inpackages/money/src/errors.tsemitted unparsable JavaScript for legal inputs. A currency name ofO'Reilly— a legalregisterCurrencyargument — produced:bun run error-renderpassed all seven, because the values are declaredstring, notunknown. That is precisely the blind spot this issue names.A second route with no quote character involved.
fromDecimaltrims before parsing but passes the untrimmed value to the error, sofromDecimal('1.2345\n', 'USD')put a raw line break inside a single-quoted literal — a broken fix line, and a broken 3-lineformat()render, from a value containing no quote at all. Any repo-wide rule needs to catch control characters too, not just quotes.The exposure is wider than "internally validated values are safe". Every one of those factories is a public export, so the argument is whatever an app passes — not only what the package's own validated paths produce. Several sites would have been safe by construction (their callers had already run
assertCurrency) and were still reachable through the export.One design note for whoever mechanises this: escaping alone is not sufficient. Escaping
O'Reillyyields a parsableassertCurrency("O'Reilly")— which throws the same error when pasted. Where the value has a known shape, the fix must gate to a usable example ('XXX') rather than faithfully echo an unusable one.packages/money/src/errors.tsalready stated that rule in prose: an instruction that raises the error it is answering is not an instruction.Fixed in #131 for
packages/moneyonly, with a test that extracts eachfix:line, tracks quoting, and compiles the result withnew Function('if (false) { … }')— parsed, never executed — across five hostile values × sixteen factories. It guards one package.The repo-wide close is to teach
scripts/error-render.tsto flag any${…}landing inside a quoted literal in afix:template regardless of the parameter's declared type, plus a control-character check. That is the same shape as the existingunknownrule, which shipped only after the bug shipped three times.fix: review findings — a drop that could delete a database we never c…
fix(cli,money)!: two shipped fix: lines that resolved and still did t…
sebyx07 commentedon Aug 20, 2026
Not built, and the evidence is a clean negative rather than an opinion.
The proposal is to extend
error-renderso it sees acatch (error)binding, since it inspects annotated parameters and a catch binding is inference-typed — which is howString(error)shipped green for three releases.A census of
packages/cli— the package most likely to be full of them — found:catchbindings inpackages/cli/src;String(err);mcp-host.ts:195andprerender.ts:138containString(error)only inside the comment explaining whyrenderThrowableis used instead — both already call it. The third,verify-floor.ts:46, iserror instanceof Error ? error.message : String(error): guarded, the value goes into aproblems: string[]and never acause:/fix:, and the unguarded branch is unreachable becauseJSON.parseonly throwsSyntaxError.renderThrowablecall sites. That package has already absorbed the lesson.A gate that reports one finding and is dismissed on review is worse than no gate.
What would change the decision: a census of a package that has not absorbed it, showing a real count. That was not run, so nothing here overturns the
cliresult — the issue stays open on that question.Worth recording, because #219 found the same defect class in a different place: two of the new gates shipped a
fix:built from a template literal, and the fix-line contract reads those statically, soscripts/${SCRIPT}.tsrendered asscripts/<value>.tsand named no file. Same family — a value hidden inside a template literal that a static reader cannot see — caught by a contract that already existed. That is evidence thefix:half of this problem is covered; thecause:/catch-binding half is what remains unmeasured.fix(core,schema,cli): address review — six applied, one rejected as f…
fix(core,schema,http,mcp,cli): escape line breaks where the 3-line fo…
developerz-ai commentedon Aug 20, 2026
🤖 developerz.ai maintainer bot — developerz-ai[bot]
Tracking pull request #229 for this issue.
feat(entity,testing)!: the subscribe fixture drives a whole sync node…