Repository navigation
fix(express): bound error redaction cost and contain error-response failures - #10151
Charan-Ratlan wants to merge 1 commit into
Conversation
|
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cbd2ccf to
a24f9e3
Compare
|
@claude review |
…ailures (WAL-2460) problem/goal Every error response runs collectSensitiveRequestValues over the raw request body and then redactSensitiveValues with one split/join pass per collected secret, where each pass rescans the output of the previous passes, including the inserted [REDACTED] tokens. With no minimum length and no cap on collected secrets, an unauthenticated ~1 KB body of short password values chosen as substrings of [REDACTED] makes the redacted message grow exponentially (measured: 75-char message to 15M chars in 283 ms, ~40 MB strings, RangeError or heap exhaustion), freezing the event loop, hanging the request for the 305 s connect timeout, or aborting the process. A throw inside the error handler itself escapes the wrapper's catch block as an unobserved rejection, which produces the same hang through other paths. implementation details - collectSensitiveRequestValues ignores collected values shorter than 4 characters and caps the collected set at 32 secrets. - redactSensitiveValues redacts strings in a single pass: literals are regex-escaped (collected secrets are attacker-influenced), ordered longest-first, and replaced text is never rescanned, bounding the output at roughly 11x the input regardless of secret count or content. - handleRequestHandlerError is wrapped in try/catch: a failure to build the error response now yields a generic 500 {message: 'internal error'} guarded by res.headersSent, and the fallback log prints the error name only, since its message may contain unredacted secret content. verification - modules/express: tsc -p . --noEmit clean; prettier and eslint clean on touched files. - Full express unit suite with BITGOJS_TEST_PASSWORD set: 1998 passing, 0 failing (master baseline: 1991 passing), including new tests for the short-value attack body, mixed junk plus real passphrase, regex metacharacters, longest-first matching, the collection cap, generic 500 on redaction/serialization failure, and the headersSent guard. Ticket: WAL-2460
a24f9e3 to
e3da73f
Compare
|
@claude review |
| : (redactSensitiveValues(err.message, sensitiveValues) as string) || 'local error'; | ||
| let result = exceededSensitiveValues | ||
| ? { error: message } | ||
| : redactSensitiveValues(err.result || { error: message }, sensitiveValues); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Callers get an error field mangled into nested tokens and up to about 100x the size of the input. The PR's "roughly 11x" bound does not cover it. When err.result is absent, { error: message } is built from the already-redacted message and redacted a second time, so it rescans the inserted [REDACTED] tokens. Fix: redact the message exactly once, e.g. err.result ? redactSensitiveValues(err.result, sensitiveValues) : { error: message }. The double pass is pre-existing, but it defeats the bound this change sets out to enforce.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The trigger is any error thrown without err.result, which covers most plain Errors, reaching promiseWrapper or typedPromiseWrapper (clientRoutes.ts:2056, 2073), with a request body holding short sensitive values. At clientRoutes.ts:2004 the message is redacted once. At clientRoutes.ts:2007 { error: message } is passed through redactSensitiveValues again. A secret such as E or D matches inside each inserted "[REDACTED]", so error becomes text like "[R[REDACTED]DACT[REDACTED]D]" while message reads "[REDACTED]". With secrets covering the characters of "[REDACTED]" plus one char of an echoed input, each input character grows to about 100 characters. The comment at clientRoutes.ts:1917 claims about 11x. An error that echoes a few MB of a 20mb body therefore builds a string of hundreds of millions of characters synchronously, or hits RangeError and lands in the generic 500 fallback. The base also double-redacted, but its growth was exponential, so this is the part the fix leaves open. The new tests check only response.message, never response.error.
Verification: In handleRequestHandlerError, the message is redacted first. Then the result is built with redactSensitiveValues(err.result || { error: message }, sensitiveValues). When err.result is absent, the string branch (lines 1905-1962) scans the already-redacted text again, inserted [REDACTED] tokens included. Two passes give up to about 100x. The base ran the same double redaction.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
problem/goal
Every error response runs collectSensitiveRequestValues over the raw request body and then redactSensitiveValues with one split/join pass per collected secret, where each pass rescans the output of the previous passes, including the inserted [REDACTED] tokens. With no minimum length and no cap on collected secrets, an unauthenticated ~1 KB body of short password values chosen as substrings of [REDACTED] makes the redacted message grow exponentially (measured: 75-char message to 15M chars in 283 ms, ~40 MB strings, RangeError or heap exhaustion), freezing the event loop, hanging the request for the 305 s connect timeout, or aborting the process. A throw inside the error handler itself escapes the wrapper's catch block as an unobserved rejection, which produces the same hang through other paths.
implementation details
verification
Ticket: WAL-2460