Skip to content

fix(express): bound error redaction cost and contain error-response failures #10151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Charan-Ratlan wants to merge 1 commit into
base: master
Choose a base branch
from
+350 −36
Open

fix(express): bound error redaction cost and contain error-response failures #10151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 131 additions & 36 deletions modules/express/src/clientRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1858,26 +1858,43 @@ function isTraversableObject(value: unknown): value is Record<string, unknown> |
return value !== null && typeof value === 'object';
}

/**
* Maximum number of distinct sensitive values per request whose redaction the
* error path can guarantee. Requests carrying more than this many cannot be
* verified exhaustively, so their error responses fail closed.
*/
const MAX_SENSITIVE_VALUES = 64;

interface CollectedSensitiveValues {
values: Set<string>;
/** True when the request contained more distinct sensitive values than can be guaranteed redactable. */
exceeded: boolean;
}

function collectSensitiveRequestValues(
value: unknown,
values = new Set<string>(),
collected: CollectedSensitiveValues = { values: new Set<string>(), exceeded: false },
seen = new WeakSet<Record<string, unknown> | unknown[]>()
): Set<string> {
): CollectedSensitiveValues {
if (!isTraversableObject(value) || seen.has(value)) {
return values;
return collected;
}
seen.add(value);
if (Array.isArray(value)) {
value.forEach((item) => collectSensitiveRequestValues(item, values, seen));
value.forEach((item) => collectSensitiveRequestValues(item, collected, seen));
} else {
Object.entries(value).forEach(([key, item]) => {
if (SENSITIVE_REQUEST_KEYS.has(key.toLowerCase()) && typeof item === 'string' && item.length > 0) {
values.add(item);
if (collected.values.size < MAX_SENSITIVE_VALUES) {
collected.values.add(item);
} else {
collected.exceeded = true;
}
}
collectSensitiveRequestValues(item, values, seen);
collectSensitiveRequestValues(item, collected, seen);
});
}
return values;
return collected;
}

function redactSensitiveValues(
Expand All @@ -1886,7 +1903,63 @@ function redactSensitiveValues(
ancestors = new WeakSet<Record<string, unknown> | unknown[]>()
): unknown {
if (typeof value === 'string') {
return [...sensitiveValues].reduce((redacted, secret) => redacted.split(secret).join('[REDACTED]'), value);
if (sensitiveValues.size === 0) {
return value;
}
// Single scan over the input:
// - every secret keeps a cached next-match position that only moves
// forward, so each secret scans the string about once and the total
// cost is linear in the input length and the number of secrets, even
// when one secret matches thousands of times while another never does;
// - when matches overlap, the union of their ranges is replaced, so no
// part of any matched secret can remain in the output;
// - replaced text is never rescanned, so output length stays bounded at
// roughly 11x the input regardless of the number or content of the
// collected secrets. (The previous chained split/join passes re-matched
// the inserted [REDACTED] tokens and grew the output exponentially for
// attacker-chosen values.)
const secrets = [...sensitiveValues];
const notSearched = -2;
const nextMatch = new Array<number>(secrets.length).fill(notSearched);
let redacted = '';
let fromIndex = 0;
while (fromIndex < value.length) {
let matchIndex = -1;
let matchEnd = -1;
for (let i = 0; i < secrets.length; i++) {
const cached = nextMatch[i];
if (cached !== -1 && (cached === notSearched || cached < fromIndex)) {
nextMatch[i] = value.indexOf(secrets[i], fromIndex);
}
const index = nextMatch[i];
if (index !== -1 && (matchIndex === -1 || index < matchIndex)) {
matchIndex = index;
matchEnd = index + secrets[i].length;
}
}
if (matchIndex === -1) {
break;
}
// extend the replacement over every secret that starts inside the matched
// span, so a secret overlapping the chosen match cannot leak its tail
let extended = true;
while (extended) {
extended = false;
for (let i = 0; i < secrets.length; i++) {
const index = nextMatch[i];
if (index !== -1 && index >= matchIndex && index < matchEnd) {
const end = index + secrets[i].length;
if (end > matchEnd) {
matchEnd = end;
extended = true;
}
}
}
}
redacted += `${value.slice(fromIndex, matchIndex)}[REDACTED]`;
fromIndex = matchEnd;
}
Comment thread
Charan-Ratlan marked this conversation as resolved.
Resolved
return redacted + value.slice(fromIndex);
}
if (!isTraversableObject(value)) {
return value;
Expand All @@ -1910,36 +1983,58 @@ function redactSensitiveValues(
}

function handleRequestHandlerError(res: express.Response, error: unknown, requestBody?: unknown) {
let err;
if (error instanceof Error) {
err = error;
} else if (typeof error === 'string') {
err = new BitGoExpressError('(string_error) ' + error);
} else {
err = new BitGoExpressError('(object_error) ' + JSON.stringify(error));
}

const sensitiveValues = collectSensitiveRequestValues(requestBody);
const message = (redactSensitiveValues(err.message, sensitiveValues) as string) || 'local error';
let result = redactSensitiveValues(err.result || { error: message }, sensitiveValues);
result = _.extend({}, result, {
message,
name: err.name || 'BitGoExpressError',
bitgoJsVersion: version,
bitgoExpressVersion: pjson.version,
...(err.code ? { code: err.code } : {}),
});
const status = err.status || 500;
if (!(status >= 200 && status < 300)) {
console.log('error %s: %s', status, message);
}
if (status >= 500 && status <= 599) {
if (err.response && err.response.request) {
console.log(`failed to make ${err.response.request.method} request to ${err.response.request.url}`);
try {
let err;
if (error instanceof Error) {
err = error;
} else if (typeof error === 'string') {
err = new BitGoExpressError('(string_error) ' + error);
} else {
err = new BitGoExpressError('(object_error) ' + JSON.stringify(error));
}

const { values: sensitiveValues, exceeded: exceededSensitiveValues } = collectSensitiveRequestValues(requestBody);
// Values of any length (including 1-3 characters) are redacted, so
// nothing sensitive is echoed. Only a request carrying more distinct
// sensitive values than can be guaranteed redactable fails closed - with
// a message naming that cause - so callers keep their real, redacted
// error details in every other case.
const message = exceededSensitiveValues
? 'error details withheld: too many sensitive values in request'
: (redactSensitiveValues(err.message, sensitiveValues) as string) || 'local error';
let result = exceededSensitiveValues
? { error: message }
: redactSensitiveValues(err.result || { error: message }, sensitiveValues);

@claude claude Bot Nov 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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.

result = _.extend({}, result, {
message,
name: err.name || 'BitGoExpressError',
bitgoJsVersion: version,
bitgoExpressVersion: pjson.version,
...(err.code ? { code: err.code } : {}),
});
const status = err.status || 500;
if (!(status >= 200 && status < 300)) {
console.log('error %s: %s', status, message);
}
if (status >= 500 && status <= 599 && !exceededSensitiveValues) {
if (err.response && err.response.request) {
console.log(`failed to make ${err.response.request.method} request to ${err.response.request.url}`);
}
console.log(redactSensitiveValues(err.stack, sensitiveValues));
}
res.status(status).send(result);
} catch (e) {
// Building the error response itself failed (for example redaction or
// serialization of a pathological thrown value). Never let this failure
// escape: the wrapper's catch block has already run, so an escaped throw
// becomes an unobserved rejection and the request hangs until the connect
// timeout. Respond generically instead, and log the error name only -
// its message may contain unredacted secret content.
if (!res.headersSent) {
res.status(500).send({ message: 'internal error' });
}
console.log(redactSensitiveValues(err.stack, sensitiveValues));
console.log('error-response construction failed:', e instanceof Error ? e.name : typeof e);
}
res.status(status).send(result);
}

/**
Expand Down
Loading
Loading