From f85bee1fc8c6ea7da49ffd952a4b560eb04b24a3 Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Mon, 19 Oct 2026 09:14:36 -0400 Subject: [PATCH 1/3] fix(mcpl): no error a host's call gets carries a configured credential A server's words reached a host in errors the framework built from them, and a library's error reached it as raised: - the legacy initialize error, a refusal of the protocol revision and its data, the revision a legacy server answered with, and both engines' error answers kept the server's words, cut to 200 code points with control characters replaced, and trimmed. The framework redacted its records afterwards, so a credential the cut split kept its start, one holding a control character survived changed, and one ending in a space survived whole, in those records and in an agent's tool result. The caller of a connection got the words, an error answer's data, a refusal's list of revisions and a tools/list cursor's errors unredacted. - a failed legacy handshake's stderrTail was the end of the server's stderr as written, cut before anything redacted it. - a modern connection's failed connect rethrew the SDK's error, which can quote the url in its message, and under Bun carries the url in its path property. - a legacy stdio server that couldn't be started was refused with Node's error as its cause, the child's command line in its spawnargs, and an argument Node refused at once quoted in its words. - McplUnreapedLaunchError printed its connection's configuration, credentials included, wherever the error was inspected. - the record of an answer that came after its deadline was bounded, never redacted. Each error now redacts the server's words with its configuration where it is built, then trims and bounds them, and redacts the strings in an error answer's data (redactedData), to a depth of 64 levels, keeping every entry under a name of its own; with a configuration that can't be read, the data is withheld whole. Each such error takes one build of its redactor (ServerTextRedactor.now), so it reads the configuration at most once. A failed handshake's stderrTail is the stream's last 2,048 characters, redacted as the transport's window of its last 16,384 has them: the redactor can start at a place in a text (Redaction's from) and replaces a credential that reaches across that place whole, so a credential of up to 14,336 characters that reaches into the tail is matched whole. The raw stderrTail getter reads the same window. A library's error is handed on as its name and its redacted words alone (maskedError): a modern connect's failure, and a legacy stdio spawn failure's cause, which keeps its errno code, whether Node refuses the launch at once, when the words stop before Node's quote of the refused value, or reports it on a tick after. An error answer's code is redacted in its log line unless it's a number (loggedCode), and McplUnreapedLaunchError.connection isn't enumerable. The late answer's record is redacted, then bounded. redactServerText's scope sentence, the README and PublishOutcome.detail's doc say so: a JSON-RPC answer's result passes as it is, no error a host's own call gets carries a credential the redactor replaces, a close's or a cleanup's failure carries the transports' own words, and a connection's events are the records' inputs. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- .../host-errors-carry-no-credentials.fixed.md | 45 ++ src/framework.ts | 9 +- src/mcpl/channel-registry.ts | 6 +- src/mcpl/log-value.ts | 9 + src/mcpl/modern-connection.ts | 63 +- src/mcpl/protocol-family.ts | 140 ++++- src/mcpl/server-connection.ts | 106 ++-- src/mcpl/transport.ts | 96 ++- test/mcp-modern-http.test.ts | 34 +- test/mcp-modern-outcomes.test.ts | 6 +- test/mcp-recorded-text.test.ts | 545 +++++++++++++++++- test/mcpl-agent-words.test.ts | 23 +- test/mcpl-stderr-tail.test.ts | 7 +- test/mcpl-transport-close.test.ts | 5 +- 15 files changed, 971 insertions(+), 125 deletions(-) create mode 100644 changelog.d/host-errors-carry-no-credentials.fixed.md diff --git a/README.md b/README.md index 7411914dc..0a584344d 100644 --- a/README.md +++ b/README.md @@ -362,7 +362,7 @@ revisions the server says it supports ("none listed" for an empty list), or its and when the list includes `2026-07-28` it says to configure the server with `protocol: 'modern'` (stdio) or reach it over http(s). -Server and transport text the framework records has the server's configured credentials replaced, in both engines. That covers connection traces, console lines, stderr traces, ops alerts, and a tool call's failure as its caller, the model and the `tool:failed` trace get it. The credentials are those the configuration holds and those its dials sent: from the url as each dial sends it (a WebSocket dial sets the token in the query, which writes the whole query again, form-encoded, and an access provider's token can be in it), credential-like path segments and query names, credential-shaped or credential-named query values, and a WebSocket url's userinfo, in their encoded and decoded forms, replaced wherever they occur; every non-empty query parameter as its whole `name=value`, where no letter or digit adjoins it; the static `token`, and each token an access provider handed a dial, the newest sixteen, raw and url-encoded; and a stdio server's `env` values whose names have a part ending in a credential word, such as `GITHUB_TOKEN`, `PGPASSWORD` or `SESSION_COOKIE`, raw and url-encoded. A stdio server's command-line arguments aren't derived. Standalone strings shorter than eight characters stay unchanged because they may be ordinary words; short query values are still removed with their parameter names. Treat a submitted configuration as immutable while the server uses it: apply changes with `restartMcplServer(id, newConfig)`. Secrets neither the configuration holds nor a dial sent remain outside this filtering. JSON-RPC tool answers, and the error a host's own connect raises, pass through as sent. +Server and transport text the framework records has the server's configured credentials replaced, in both engines. That covers connection traces, console lines, stderr traces, ops alerts, and a tool call's failure as its caller, the model and the `tool:failed` trace get it. The credentials are those the configuration holds and those its dials sent: from the url as each dial sends it (a WebSocket dial sets the token in the query, which writes the whole query again, form-encoded, and an access provider's token can be in it), credential-like path segments and query names, credential-shaped or credential-named query values, and a WebSocket url's userinfo, in their encoded and decoded forms, replaced wherever they occur; every non-empty query parameter as its whole `name=value`, where no letter or digit adjoins it; the static `token`, and each token an access provider handed a dial, the newest sixteen, raw and url-encoded; and a stdio server's `env` values whose names have a part ending in a credential word, such as `GITHUB_TOKEN`, `PGPASSWORD` or `SESSION_COOKIE`, raw and url-encoded. A stdio server's command-line arguments aren't derived. Standalone strings shorter than eight characters stay unchanged because they may be ordinary words; short query values are still removed with their parameter names. Treat a submitted configuration as immutable while the server uses it: apply changes with `restartMcplServer(id, newConfig)`. Secrets neither the configuration holds nor a dial sent remain outside this filtering. A JSON-RPC tool answer's result passes through as sent. No error a host's own call gets, whether thrown, rejected with or in an error answer's data, carries a credential this filtering replaces, whoever wrote its words: a server's words are redacted, then bounded, where an error is built from them, the strings in an error answer's data are redacted, and an error a library raised, such as a failed modern connect's or a stdio child's that couldn't be started, is handed on as its name and its redacted words alone. With a configuration that can't be read, the server's words and data are withheld. A failed handshake's `stderrTail` is the end of the server's stderr with every credential that reaches into it replaced whole, up to 14,336 characters long. A number in an error answer's data is matched as JavaScript reads it, so one with more significant digits than a double holds has lost its last digits before it's matched. A close's or a cleanup's failure carries the transports' own words, a child's pid, its server's id and the wait it was given. A connection's events are inputs to the framework's records, which redact them. #### Feature sets diff --git a/changelog.d/host-errors-carry-no-credentials.fixed.md b/changelog.d/host-errors-carry-no-credentials.fixed.md new file mode 100644 index 000000000..f4c202f2e --- /dev/null +++ b/changelog.d/host-errors-carry-no-credentials.fixed.md @@ -0,0 +1,45 @@ +- An error a host's own call to agent-framework gets, thrown, rejected with + or in an error answer's data, no longer carries a credential the framework + replaces in its records, whoever wrote its words. Before: + - a server's words in an error built from them were cut to 200 code + points, with control characters replaced, before anything redacted + them. That covers an initialize error, a refusal of the protocol + revision, the revision a legacy server answered with, and a tool call's + error answer. A credential the cut split kept its start in the + framework's records and in an agent's tool result, and one holding a + control character survived changed. An error answer's words were also + trimmed first, so a credential ending in a space at their end survived + whole. The caller of a connection got the server's words, an error + answer's data, the revisions a refusal lists and a tools/list cursor in + its errors unredacted; + - a failed legacy handshake's `stderrTail` was the end of the server's + stderr as written, cut before anything redacted it; + - a modern connection's failed connect handed the host the SDK's error as + raised, with the url in its words, and under Bun in its `path`; + - a legacy stdio server that couldn't be started was refused with Node's + error as the `cause`, its command line in `spawnargs`, and an argument + Node refuses quoted in the words; + - `McplUnreapedLaunchError` printed its connection's configuration, + credentials included, wherever the error was inspected. + + Now each error redacts the server's words where it's built, then trims and + bounds them, and redacts the strings in an error answer's data. In that + data, a number whose digits hold a credential, as JavaScript reads the + number, becomes a string, its digits with the credential replaced; a + number with more significant digits than a double holds has lost its last + ones before that. A redacted key that comes out the same as another key is + numbered (`redacted (2)`), and data nested past 64 levels reads + `[withheld: nested too deep to redact]`. A failed handshake's `stderrTail` + is the same end of the server's stderr, with every credential that reaches + into it, up to 14,336 characters long, replaced whole. A library's error + is handed on as its name and its redacted words alone. A legacy stdio + server that couldn't be started is refused with its words redacted and a + `cause` that keeps Node's error's name, its redacted words and its errno + `code`, and nothing else of it; when Node refuses an argument or an env + value at once, the words leave the value out. + `McplUnreapedLaunchError.connection` isn't enumerable. With a + configuration that can't be read, the server's words are withheld, as the + records already withhold them, and an error answer's data is withheld + whole. +- The record of a tools/call or push/render answer that came after its + deadline is redacted as an on-time answer's record is. diff --git a/src/framework.ts b/src/framework.ts index 90b3d1b59..945b03343 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -24781,10 +24781,13 @@ export class AgentFramework { error?: { code: unknown; message: unknown }; }) => { // A late JSON-RPC error is recorded as the connection words a live one, - // `[code] message`, bounded and with its control characters replaced, - // since the server wrote it. + // `[code] message`: redacted as the server's text, as an on-time + // answer's record is, and then bounded with its control characters + // replaced, since the server wrote it. Redacting first keeps a + // credential the cut or a replaced character would split whole for the + // redactor. const error = info.error === undefined ? {} : { - error: boundedForLog(`[${describeCause(info.error.code)}] ${describeCause(info.error.message)}`, 500), + error: boundedForLog(recorded(`[${describeCause(info.error.code)}] ${describeCause(info.error.message)}`), 500), }; // A push/render answered after its timeout, when the batch's fallback // was already materialized: RFC-006's audit records the discarded diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 45a13d162..43711faf6 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -471,7 +471,8 @@ const CHANNEL_TOOL_DEFINITIONS: ToolDefinition[] = [ // ============================================================================ /** Bound connector values for notices, logs and an outcome's reason. A - * PublishOutcome keeps the connector's structured data (`detail`) whole. */ + * PublishOutcome keeps the connector's structured data (`detail`) as the + * connection gives it, its strings redacted (redactedData). */ export function connectorText(value: unknown): string { let text: string; try { @@ -515,7 +516,8 @@ export interface PublishOutcome { /** Why it failed, or why its outcome is unknown — for a connector error, * its message, bounded (which may say what was already posted). */ reason?: string; - /** The connector error's structured `data`, verbatim, when it sent one. */ + /** The connector error's structured `data`, with the strings in it + * redacted (redactedData), when it sent one. */ detail?: unknown; /** When the outcome was established (epoch ms). */ at: number; diff --git a/src/mcpl/log-value.ts b/src/mcpl/log-value.ts index 8ddb7d75a..d7216180f 100644 --- a/src/mcpl/log-value.ts +++ b/src/mcpl/log-value.ts @@ -55,6 +55,15 @@ export function logValueRedacted(value: unknown, redact: (text: string) => strin return printed(redact(textOf(value)), FIELD); } +/** + * A JSON-RPC error answer's code as a log line prints it: a number, as + * JSON-RPC says a code is, as logValue prints it; any other JSON value a + * server sends there redacted (logValueRedacted), since the server wrote it. + */ +export function loggedCode(code: unknown, redact: (text: string) => string): string { + return typeof code === 'number' ? logValue(code) : logValueRedacted(code, redact); +} + /** * The host's own message, as the free text that ends a log line after its * fields: `reason=coalesce-invalid field=payload.content: "content must be an diff --git a/src/mcpl/modern-connection.ts b/src/mcpl/modern-connection.ts index c9f6ccb42..ceaa4c756 100644 --- a/src/mcpl/modern-connection.ts +++ b/src/mcpl/modern-connection.ts @@ -68,10 +68,10 @@ import { import { StdioTransport, type McplTransport } from './transport.js'; import { McplProtocolVersionError, McplRequestError, McplUnreapedLaunchError } from './server-connection.js'; -import { LEGACY_MCP_PROTOCOL_VERSION, MAX_TIMER_MS, MODERN_MCP_PROTOCOL_VERSION, checkServerConfig, describeCredentialError, noteDialSent, redactServerText, serverTextRedactor } from './protocol-family.js'; +import { LEGACY_MCP_PROTOCOL_VERSION, MAX_TIMER_MS, MODERN_MCP_PROTOCOL_VERSION, checkServerConfig, describeCredentialError, maskedError, noteDialSent, redactServerText, redactedData, serverTextRedactor, type ServerTextRedactor } from './protocol-family.js'; import type { McplServerConfig, McpToolDefinition } from './types.js'; import { boundedForLog } from './errors.js'; -import { logDetail, logValue, logValueRedacted } from './log-value.js'; +import { logDetail, logValue, logValueRedacted, loggedCode } from './log-value.js'; const DEFAULT_REQUEST_TIMEOUT_MS = 60_000; /** Bound on establishing the connection (`server/discover`), like the legacy @@ -452,6 +452,13 @@ export class ModernMcpConnection extends EventEmitter { * connect() returns), delivered to the first listener. Bounded: the * newest lines are kept. */ private stderrBacklog: string[] = []; + private redactor?: ServerTextRedactor; + + /** The server's text with its configuration's credentials replaced, for + * the errors this connection hands its caller and its records. */ + private get recorded(): ServerTextRedactor { + return (this.redactor ??= serverTextRedactor(this.config)); + } private constructor(private readonly config: McplServerConfig) { super(); @@ -585,10 +592,16 @@ export class ModernMcpConnection extends EventEmitter { this.reconnectEnabled = false; throw error; } + // Any other failure is the SDK's or the transport's: it goes on as its + // words alone, redacted, since its message can quote the url and its + // other properties, such as Bun's fetch error's `path` or a wrapper's + // `cause`, can carry it whole (maskedError). The protocol refusal + // above was built from redacted words. + const masked = maskedError(error, this.recorded); // A launch that couldn't be reaped halts reconnecting, so no retry // will come: the first connect fails, as one without reconnect does. - if (!this.reconnectEnabled || this.holdsUnreapedChild) throw error; - console.error(`MCP server "${this.id}" initial connect failed, will retry:`, redactServerText((error as Error).message, this.config)); + if (!this.reconnectEnabled || this.holdsUnreapedChild) throw masked; + console.error(`MCP server "${this.id}" initial connect failed, will retry:`, masked.message); this.reconnectAttempts = 1; this.emit('connect-failed', { error: (error as Error).message, attempt: 0 }); this.scheduleReconnect(); @@ -726,7 +739,11 @@ export class ModernMcpConnection extends EventEmitter { */ private versionVerdict(error: unknown): McplProtocolVersionError | null { if (!(error instanceof UnsupportedProtocolVersionError)) return null; - const data = (error as { data?: unknown }).data as { supported?: unknown } | undefined; + // The server's words and data, redacted before they're bounded or kept: + // the error goes to the host. One build for all of them, so a + // configuration that can't be read is read once. + const redact = this.recorded.now(); + const data = redactedData((error as { data?: unknown }).data, redact) as { supported?: unknown } | undefined; const supported = Array.isArray(data?.supported) && data.supported.every((v) => typeof v === 'string') ? (data.supported as string[]) : null; @@ -740,13 +757,13 @@ export class ModernMcpConnection extends EventEmitter { : ` It speaks only legacy MCP, which this client reaches over stdio or WebSocket (ws://, wss://), not http(s).`; return new McplProtocolVersionError( `MCP server "${this.id}" does not support MCP ${MODERN_MCP_PROTOCOL_VERSION}` + - (supported ? ` (it supports: ${supported.join(', ') || 'none listed'}).` : ` (${error.message}).`) + + (supported ? ` (it supports: ${boundedForLog(supported.join(', ')) || 'none listed'}).` : ` (${boundedForLog(redact(error.message))}).`) + legacyHint, 'rejected', null, supported, (error as { code?: unknown }).code === undefined ? undefined : Number((error as { code?: unknown }).code), - (error as { data?: unknown }).data, + data, MODERN_MCP_PROTOCOL_VERSION, ); } @@ -1116,7 +1133,10 @@ export class ModernMcpConnection extends EventEmitter { private requestFailure(what: string, record: WireRecord, error: unknown, timeoutMs?: number, lost = false): Error { const server = `the "${this.id}" server`; const Server = `The "${this.id}" server`; - const message = redactServerText(error instanceof Error ? error.message : String(error), this.config); + // One build of the redactor for all of this failure's texts, so a + // configuration that can't be read is read once. + const redact = this.recorded.now(); + const message = redact(error instanceof Error ? error.message : String(error)); const log = (line: string) => console.error(`MCP server "${this.id}" ${line}`); const listing = record.method === 'tools/list'; // What a failure whose effect is in doubt says: for a tool call, that the @@ -1140,18 +1160,21 @@ export class ModernMcpConnection extends EventEmitter { } if (record.answer === 'error' && record.error) { const { code, message: wireMessage, data } = record.error; - // A JSON-RPC answer reaches the caller unredacted (redactServerText's - // scope). The log is a record of the server's text, so the message is - // redacted there, whatever JSON value it is, as every other line this - // connection writes is. - log(`answered ${what} with error ${logValue(code)}: ${logValueRedacted(wireMessage, (text) => redactServerText(text, this.config))}`); - // The server's own words, bounded as errors.ts says. Only a string is - // words: a message of any other JSON value reads as none. - const said = typeof wireMessage === 'string' ? boundedForLog(wireMessage.trim()) : ''; + // The log is a record of the server's text, so the code (loggedCode) + // and the message are redacted there, whatever JSON values they are, + // as every other line this connection writes is. + log(`answered ${what} with error ${loggedCode(code, redact)}: ${logValueRedacted(wireMessage, redact)}`); + // The server's own words, redacted and then bounded as errors.ts says, + // since they're in an error the caller gets (redactServerText's + // scope), and its data's strings redacted too. It's trimmed after it's + // redacted, so a credential that ends in a space is whole when it's + // matched. Only a string is words: a message of any other JSON value + // reads as none. + const said = typeof wireMessage === 'string' ? boundedForLog(redact(wireMessage).trim()) : ''; return new McplRequestError( said ? `${Server} answered with an error: ${said}` : unknown(`${Server} answered with an error without saying what went wrong`), 'error-response', - data, + redactedData(data, redact), ); } if (record.answer === 'complete') { @@ -1197,10 +1220,14 @@ export class ModernMcpConnection extends EventEmitter { // The response body (the SDK's `text`) is the server's text and can // echo the request's url, so it's left out of the data; the status stays. const { text: _body, ...data } = (error instanceof SdkHttpError && error.data && typeof error.data === 'object' ? error.data : {}) as Record; + const redacted = redactedData(data, redact); return new McplRequestError( `${Server} refused this host access, so the request didn't run. If you need it, ask whoever runs this host.`, 'error-response', - { ...data, httpStatus: refusedWith }, + // The status is this host's own, so it's added after the server's + // data is redacted, and it stays when a configuration that can't be + // read withholds that data. + { ...(redacted !== null && typeof redacted === 'object' ? redacted : {}), httpStatus: refusedWith }, ); } if (deadline) { diff --git a/src/mcpl/protocol-family.ts b/src/mcpl/protocol-family.ts index c2def6aeb..78babeeec 100644 --- a/src/mcpl/protocol-family.ts +++ b/src/mcpl/protocol-family.ts @@ -126,8 +126,20 @@ export function redactUrl(url: string | undefined): string { * strings (serverCredentialPatterns), so strings of different servers that * overlap are both replaced whole. A credential neither the configuration * holds nor a dial of it sent, such as another origin's, is outside it, and - * so are other encodings of the ones it holds. JSON-RPC answers, and the - * error a host's own connect raises, pass as they are. + * so are other encodings of the ones it holds. A JSON-RPC answer's result + * passes as it is. No error a host's own call gets, whether thrown, rejected + * with or in an error answer's data, carries one of the strings above, + * whoever wrote the words: a server's words are redacted, then bounded, + * where an error is built from them, the strings in an error answer's data + * are redacted ({@link redactedData}), and an error a library raised is + * handed on as its name and its redacted words alone ({@link maskedError}). + * A failed handshake's stderr tail is redacted from a longer window of the + * stream, so a credential of up to 14,336 characters that reaches into it + * is replaced whole (McplTransport.stderrTailRedacted). A close's or a + * cleanup's failure carries the transports' own words, a child's pid, its + * server's id and the wait it was given, and passes as built. A + * connection's events are the framework's records' inputs, and the records + * redact them. * * A configuration that can't be read, such as one whose `env` is a getter * that throws, says nothing of which strings to replace, so the whole text @@ -139,6 +151,22 @@ export function redactServerText(text: unknown, config: Pick WITHHELD_TEXT; + /** * {@link redactServerText} for one configuration, its strings found and its * pattern built once, and again once a dial has sent something new @@ -146,29 +174,103 @@ export function redactServerText(text: unknown, config: Pick): (text: unknown) => string { +export function serverTextRedactor(config: Pick): ServerTextRedactor { let built = -1; - let redact!: (text: unknown) => string; - return (text) => { + let redact!: Redaction; + const now = (): Redaction => { const version = dialsSent.get(config)?.version ?? 0; if (version !== built) { let patterns: CredentialPattern[]; try { patterns = serverCredentialPatterns(config); } catch { - return WITHHELD_TEXT; + return withheld; } redact = credentialRedactor(patterns); built = version; } - return redact(text); + return redact; }; + return Object.assign((text: unknown, from?: number) => now()(text, from), { now }); } /** What a redaction gives in place of text when the configuration it * redacts with can't be read ({@link redactServerText}). */ export const WITHHELD_TEXT = "[withheld: the server's configuration couldn't be read to redact this]"; +/** What {@link redactedData} gives in place of data nested deeper than it + * walks. */ +export const TOO_DEEP_TEXT = '[withheld: nested too deep to redact]'; + +/** How deep {@link redactedData} walks. A server's JSON can nest deeper than + * a recursive walk's stack holds (10,000 levels overflow it), and the walk + * runs where a throw would leave the request it answers unsettled. */ +const REDACTED_DATA_DEPTH = 64; + +/** + * The data a server's error answer carries beside its words, with every + * string in it, an object's keys included, redacted (`redact`, a server's + * {@link serverTextRedactor}, built once for the walk), so an error handed + * to a host carries no credential its configuration holds. Arrays and plain + * objects are walked, to a depth of 64; deeper data is + * {@link TOO_DEEP_TEXT}. A number goes as its redacted text if its digits, + * as JavaScript reads the number, hold a credential: a number with more + * significant digits than a double holds (about 15) has lost its last ones + * before anything sees it, so a credential sent as such a number isn't + * matched. A redacted key that comes out the same as another key is + * numbered, so every entry is kept under a name of its own, and a key the + * redaction leaves alone keeps its name. Any other value, null or a + * boolean, is kept as it is. With a configuration that can't be read, the + * data as a whole is {@link WITHHELD_TEXT}. + */ +export function redactedData(value: unknown, redactor: ServerTextRedactor | Redaction): unknown { + const redact = 'now' in redactor ? redactor.now() : redactor; + if (redact === withheld) return value === undefined ? undefined : WITHHELD_TEXT; + const walk = (item: unknown, depth: number): unknown => { + if (typeof item === 'string') return redact(item); + if (typeof item === 'number') { + const digits = String(item); + const redacted = redact(digits); + return redacted === digits ? item : redacted; + } + if (item === null || typeof item !== 'object') return item; + const walked = Array.isArray(item) || [Object.prototype, null].includes(Object.getPrototypeOf(item)); + if (!walked) return item; + if (depth >= REDACTED_DATA_DEPTH) return TOO_DEEP_TEXT; + if (Array.isArray(item)) return item.map((entry) => walk(entry, depth + 1)); + // A key the redaction leaves as it is keeps its name; a redacted one + // takes the first name no other key has, so no entry is lost or moved. + const entries = Object.entries(item); + const names = entries.map(([key]) => redact(key)); + const taken = new Set(names.filter((name, n) => name === entries[n][0])); + return Object.fromEntries(entries.map(([key, entry], n) => { + let name = names[n]; + if (name !== key) { + const base = name; + for (let k = 2; taken.has(name); k++) name = `${base} (${k})`; + taken.add(name); + } + return [name, walk(entry, depth + 1)]; + })); + }; + return walk(value, 0); +} + +/** + * An error a library raised, as the framework hands it to a host: its name, + * which is its class's, as it is, and its words redacted (`redact`, a + * server's {@link serverTextRedactor}), and nothing else. A library's error + * can carry the url it dialed elsewhere than in its message (Bun's fetch + * errors keep it in `path`, its path and query included), or keep the raw + * error it wraps as its `cause`, so none of that is carried over. + */ +export function maskedError(error: unknown, redact: (text: unknown) => string): Error { + const masked = new Error(redact(error instanceof Error ? error.message : error)); + // Not enumerable, as an error's own name is not. + if (error instanceof Error) Object.defineProperty(masked, 'name', { value: error.name, writable: true, configurable: true }); + return masked; +} + /** * What a configuration's dials sent that the configuration doesn't hold: the * urls dialed with an access provider's token, and the provider's tokens, @@ -367,6 +469,15 @@ export function serverCredentialPatterns(config: Pick string; + /** * Every place one of the given patterns matches becomes `redacted`, * overlapping places included: the matches are found in the text as given, @@ -374,13 +485,13 @@ export function serverCredentialPatterns(config: Pick string { - if (patterns.length === 0) return asText; +export function credentialRedactor(patterns: readonly CredentialPattern[]): Redaction { + if (patterns.length === 0) return (text, from = 0) => asText(text).slice(from); // Longest first, so at each place the longest string that matches there is // the match. const sources = [...patterns].sort((a, b) => b.value.length - a.value.length).map(({ source }) => source); const pattern = new RegExp([...new Set(sources)].join('|'), 'g'); - return (text) => { + return (text, from = 0) => { const input = asText(text); const spans: Array<[start: number, end: number]> = []; pattern.lastIndex = 0; @@ -395,12 +506,13 @@ export function credentialRedactor(patterns: readonly CredentialPattern[]): (tex pattern.lastIndex = start + 1; } let redacted = ''; - let from = 0; + let at = from; for (const [start, end] of spans) { - redacted += `${input.slice(from, start)}redacted`; - from = end; + if (end <= from) continue; + redacted += `${input.slice(at, start)}redacted`; + at = end; } - return redacted + input.slice(from); + return redacted + input.slice(at); }; } diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 46843f95a..0e96ac352 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -15,13 +15,15 @@ import { openTransport, STDERR_END_WAIT_MS, type McplTransport, type TransportCl import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; import { CAPABILITY_DISABLED, boundedForLog, describeCause } from './errors.js'; -import { logDetail, logValue, logValueRedacted } from './log-value.js'; +import { logDetail, logValue, logValueRedacted, loggedCode } from './log-value.js'; import { LEGACY_MCP_PROTOCOL_VERSION, MODERN_MCP_PROTOCOL_VERSION, checkServerConfig, redactServerText, + redactedData, serverTextRedactor, + type ServerTextRedactor, } from './protocol-family.js'; import type { @@ -157,11 +159,13 @@ export class McplProtocolVersionError extends Error { constructor( message: string, readonly kind: 'rejected' | 'mismatch', - /** Revision the server answered with (`mismatch`; null when it gave none). */ + /** Revision the server answered with (`mismatch`; null when it gave + * none), redacted as the server's words are. */ readonly returned: string | null, /** Revisions the server says it supports (`rejected`, when stated). */ readonly supported: readonly string[] | null, - /** The JSON-RPC error's code and data, verbatim (`rejected`). */ + /** The JSON-RPC error's code, and its data with each string in it + * redacted (`rejected`). */ readonly code?: number, readonly data?: unknown, /** The revision this client offered: the legacy engine's, unless a @@ -173,8 +177,13 @@ export class McplProtocolVersionError extends Error { } } -function protocolVersionRejection(id: string, error: { code: number; message: string; data?: unknown }): McplProtocolVersionError { - const data = error.data as { supported?: unknown } | undefined; +function protocolVersionRejection(config: McplServerConfig, error: { code: number; message: string; data?: unknown }): McplProtocolVersionError { + const id = config.id; + // The server's words and data, redacted before they're bounded or kept: + // the error goes to the host. One build for all of them, so a + // configuration that can't be read is read once. + const redact = serverTextRedactor(config).now(); + const data = redactedData(error.data, redact) as { supported?: unknown } | undefined; const supported = Array.isArray(data?.supported) && data.supported.every((v) => typeof v === 'string') ? (data.supported as string[]) : null; @@ -184,14 +193,15 @@ function protocolVersionRejection(id: string, error: { code: number; message: st return new McplProtocolVersionError( `MCPL server "${id}" does not support MCP ${MCP_PROTOCOL_VERSION}` + // The server's message can be any JSON value, so it's read without - // throwing (describeCause) and bounded, as the initialize error is. - (supported ? ` (it supports: ${supported.join(', ') || 'none listed'}).` : ` (${boundedForLog(describeCause(error.message))}).`) + + // throwing (describeCause), and redacted, then bounded, as the + // initialize error is. + (supported ? ` (it supports: ${boundedForLog(supported.join(', ')) || 'none listed'}).` : ` (${boundedForLog(redact(describeCause(error.message)))}).`) + modernHint, 'rejected', null, supported, error.code, - error.data, + data, ); } @@ -219,16 +229,21 @@ class HandshakeCleanupError extends Error { * is confirmed, so the caller can still clean up, and should start nothing * else for this server until then. connect() and connectWithReconnect() * throw this, and the modern engine's ModernMcpConnection.connect() too. - * `cause` is the connect's own failure. + * `cause` is the connect's own failure. `connection` isn't enumerable, so + * printing the error doesn't print the connection's configuration, its + * credentials included. */ export class McplUnreapedLaunchError } = McplServerConnection> extends Error { + declare readonly connection: Connection; + constructor( - readonly connection: Connection, + connection: Connection, failure: Error, /** The cleanup's own failure: the child it couldn't confirm exited. */ readonly cleanup: Error, ) { super(`${failure.message}; its cleanup also failed: ${cleanup.message}`, { cause: failure }); + Object.defineProperty(this, 'connection', { value: connection, enumerable: false }); this.name = 'McplUnreapedLaunchError'; } } @@ -251,8 +266,9 @@ export class McplRequestError extends Error { constructor( message: string, readonly outcome: 'not-sent' | 'error-response' | 'no-response', - /** The JSON-RPC error's `data`, verbatim, when the server supplied one - * (e.g. which parts of a multi-part send were posted). */ + /** The JSON-RPC error's `data`, when the server supplied one (e.g. which + * parts of a multi-part send were posted), with each string in it + * redacted (redactedData): a host hands it on. */ readonly data?: unknown, ) { super(message); @@ -572,10 +588,12 @@ export class McplServerConnection extends EventEmitter { /** The redactor of the server's text for `config`, made at its first error * answer, by when `config` is set, and kept: a framework-launched child's * `env` is a getter that reads the awareness ledger, so a redactor built - * per answer would read it per answer (#540's review, R1-2). It builds - * again only after a build that failed, which withholds that line, or once - * a dial notes something new (serverTextRedactor). */ - private redactor: ((text: unknown) => string) | null = null; + * per answer would read it per answer (#540's review, R1-2). An answer + * takes one build of it for its words, data and log line (`now()`), so + * it reads the ledger at most once. It builds again only after a build + * that failed, which withholds that answer's text, or once a dial notes + * something new (serverTextRedactor). */ + private redactor: ServerTextRedactor | null = null; private hostCapabilities: McplHostCapabilities | null = null; private reconnectEnabled = false; private reconnectIntervalMs = 5000; @@ -1185,7 +1203,10 @@ export class McplServerConnection extends EventEmitter { // travels to traces and tool results, and a server's stderr is its // own text, which can hold a credential. const failed = (failure: string): Error & { stderrTail?: string } => { - const tail = transport.stderrTail.trimEnd(); + // It goes to the host too, so it's redacted, from a window of the + // stream longer than the tail, so a credential the tail's cut + // would split is whole when it's matched (stderrTailRedacted). + const tail = transport.stderrTailRedacted(serverTextRedactor(config)).trimEnd(); return Object.assign(new Error(failure), tail ? { stderrTail: tail } : {}); }; const cleanup = () => { @@ -1229,21 +1250,25 @@ export class McplServerConnection extends EventEmitter { // listener after cleanup() has cleared the timeout: an uncaught // exception, which ends a host that doesn't catch them, and a // handshake left to wait forever. It's the server's text in an - // error's message, so it's bounded as errors.ts says. + // error the host gets, so it's redacted, then bounded as + // errors.ts says: a cut or a replaced character first would + // leave part of a credential the redactor can't match. reject(msg.error.code === UNSUPPORTED_PROTOCOL_VERSION_CODE - ? protocolVersionRejection(config.id, msg.error) - : new Error(`MCPL server "${config.id}" initialize error: ${boundedForLog(describeCause(msg.error.message))}`)); + ? protocolVersionRejection(config, msg.error) + : new Error(`MCPL server "${config.id}" initialize error: ${boundedForLog(redactServerText(describeCause(msg.error.message), config))}`)); return; } const result = msg.result as Record | undefined; // The server's answer is the established revision. This engine // implements exactly one, so any other answer (or none) means the // two sides never agreed on what they're speaking. - const returned = typeof result?.protocolVersion === 'string' ? result.protocolVersion : null; - if (returned !== MCP_PROTOCOL_VERSION) { + const answered = typeof result?.protocolVersion === 'string' ? result.protocolVersion : null; + if (answered !== MCP_PROTOCOL_VERSION) { + // The server's words go to the host, so they're redacted. + const returned = answered === null ? null : redactServerText(answered, config); reject(new McplProtocolVersionError( `MCPL server "${config.id}" answered initialize with protocolVersion ` + - `${returned === null ? '(none)' : JSON.stringify(returned)}; this client's legacy engine speaks only ${MCP_PROTOCOL_VERSION}`, + `${returned === null ? '(none)' : boundedForLog(JSON.stringify(returned))}; this client's legacy engine speaks only ${MCP_PROTOCOL_VERSION}`, 'mismatch', returned, null, @@ -1258,7 +1283,7 @@ export class McplServerConnection extends EventEmitter { resolve({ mcpl: (experimental?.mcpl as McplCapabilities) ?? null, mcpToolsAdvertised: caps?.tools !== undefined, - protocolVersion: returned, + protocolVersion: answered, }); }; const onClose = (info: TransportCloseInfo) => { @@ -1592,10 +1617,11 @@ export class McplServerConnection extends EventEmitter { return { ...rest, tools }; } if (typeof result.nextCursor !== 'string') { - throw malformed(`nextCursor must be a string, got ${JSON.stringify(result.nextCursor)}`); + // The cursor is the server's, so it's redacted, then bounded. + throw malformed(`nextCursor must be a string, got ${boundedForLog(redactServerText(JSON.stringify(result.nextCursor), this.config ?? {}))}`); } if (seen.has(result.nextCursor)) { - throw new Error(`MCPL server "${this.id}" repeated tools/list cursor ${JSON.stringify(result.nextCursor)}`); + throw new Error(`MCPL server "${this.id}" repeated tools/list cursor ${boundedForLog(redactServerText(JSON.stringify(result.nextCursor), this.config ?? {}))}`); } seen.add(result.nextCursor); cursor = result.nextCursor; @@ -2427,16 +2453,18 @@ export class McplServerConnection extends EventEmitter { if (pending.timer) clearTimeout(pending.timer); if (response.error) { - // The server's own words, after a word on whose they are, bounded as - // errors.ts says, since they're the server's text. Only a string is - // words: a message of any other JSON value reads as none. The method, - // id and JSON-RPC code are the operator's, so they go to the log, which - // prints any value a server can send without throwing (logValue). The - // caller's text keeps the server's words unredacted, as a JSON-RPC - // answer passes (redactServerText's scope); the log is a record of the - // server's text, so the message is redacted there, whatever JSON value - // it is, as every other line this connection writes is. - const said = typeof response.error.message === 'string' ? boundedForLog(response.error.message.trim()) : ''; + // The server's own words, after a word on whose they are, redacted and + // then bounded as errors.ts says, since they're the server's text in + // an error the caller gets (redactServerText's scope), and its data's + // strings redacted too. It's trimmed after it's redacted, so a + // credential that ends in a space is whole when it's matched. Only a + // string is words: a message of any other JSON value reads as none. + // The method and id are the operator's, so they go to the log, and the + // code (loggedCode) and the message, whatever JSON values they are, go + // there redacted (logValueRedacted), as every other line this + // connection writes is. + const redact = (this.redactor ??= serverTextRedactor(this.config ?? {})).now(); + const said = typeof response.error.message === 'string' ? boundedForLog(redact(response.error.message).trim()) : ''; // The call is settled before its log line: its entry is gone and its // timer cleared, so nothing else would settle it, and a host's logger // can throw. The dispatch's containment reports such a throw. (A @@ -2448,11 +2476,11 @@ export class McplServerConnection extends EventEmitter { : `The "${this.id}" server answered with an error without saying what went wrong, so whether the request took effect is unknown. ` + 'Check its effect before trying again.', 'error-response', - (response.error as { data?: unknown }).data, + redactedData((response.error as { data?: unknown }).data, redact), )); console.error( - `MCPL server "${this.id}" answered ${pending.method} (id=${response.id}) with error ${logValue(response.error.code)}: ` + - logValueRedacted(response.error.message, (this.redactor ??= serverTextRedactor(this.config ?? {}))), + `MCPL server "${this.id}" answered ${pending.method} (id=${response.id}) with error ${loggedCode(response.error.code, redact)}: ` + + logValueRedacted(response.error.message, redact), ); } else { pending.resolve(response.result); diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 981e1f664..0be11b628 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -23,7 +23,7 @@ import { StringDecoder } from 'node:string_decoder'; import WebSocket from 'ws'; import type { McplServerConfig } from './types.js'; -import { WITHHELD_TEXT, describeCredentialError, noteDialSent, redactServerText, redactUrl, resolveServerBinding, urlAsDialed } from './protocol-family.js'; +import { WITHHELD_TEXT, describeCredentialError, maskedError, noteDialSent, redactServerText, redactUrl, resolveServerBinding, serverTextRedactor, urlAsDialed, type Redaction } from './protocol-family.js'; import { assertBooleanSetting } from '../config-checks.js'; import { logDetail } from './log-value.js'; @@ -94,6 +94,21 @@ export abstract class McplTransport extends EventEmitter { return ''; } + /** + * {@link stderrTail} as an error a host gets carries it: the same + * characters, redacted with `redact`. The stream's last + * {@link STDERR_WINDOW_CHARS} are redacted together and the tail is taken + * from where it begins in them, so a credential that begins before the + * tail and reaches into it is matched whole and replaced whole + * ({@link Redaction}'s `from`). That holds for a credential of up to + * 14,336 characters, the window less the tail: a longer one that began + * before the window can't be matched, and the tail keeps what it holds of + * it. '' for a transport that has no diagnostic stream. + */ + stderrTailRedacted(_redact: Redaction): string { + return ''; + } + /** * The error a stdio child's stdin or stdout failed with, once one has. The * transport is ending the child then, and stays open until it exits, but @@ -230,9 +245,19 @@ export function buildChildEnv( return merged; } -/** The most of a stdio child's stderr a transport keeps for {@link McplTransport.stderrTail}. */ +/** The most of a stdio child's stderr that {@link McplTransport.stderrTail} gives. */ export const STDERR_TAIL_CHARS = 2_048; +/** The most of a stdio child's stderr a transport keeps, for + * {@link McplTransport.stderrTail} and + * {@link McplTransport.stderrTailRedacted}: eight tails, so a credential of + * up to 14,336 characters that reaches into the tail, a private key's PEM + * of 8,192 bits (about 6,500 characters) included, is whole in it. */ +export const STDERR_WINDOW_CHARS = 8 * STDERR_TAIL_CHARS; + +/** A redaction that replaces nothing: the raw tail. */ +const unredacted: Redaction = (text, from = 0) => String(text).slice(from); + /** * How long a refusal at a stdio child's exit waits for the child's stderr to * end, so that its `stderrTail` holds what the child wrote. Node can emit a @@ -251,13 +276,28 @@ export class StdioTransport extends McplTransport { readonly kind = 'stdio' as const; /** The first error on the child's stdin or stdout, named in the close reason. */ private pipeError: Error | undefined; - private stderrEnd = ''; - private stderrCut = false; + /** The stream's last STDERR_WINDOW_CHARS, raw. */ + private stderrWindow = ''; /** True once the child's stderr has ended or closed. */ private stderrDone = false; override get stderrTail(): string { - return (this.stderrCut ? '…' : '') + this.stderrEnd; + return this.stderrTailRedacted(unredacted); + } + + override stderrTailRedacted(redact: Redaction): string { + // A child that wrote nothing has no tail, whatever its redaction. + if (this.stderrWindow === '') return ''; + // The tail begins at its raw place in the window, so a redaction that + // shortens the window can't move its edge back toward the window's own + // cut. Something before the tail was dropped whenever the window is + // longer than the tail, which it always is once the window was cut. + const from = Math.max(0, this.stderrWindow.length - STDERR_TAIL_CHARS); + const cut = from > 0; + let kept = redact(this.stderrWindow, from); + // A cut between a surrogate pair's halves drops the low half too. + if (cut && /^[\uDC00-\uDFFF]/.test(kept)) kept = kept.slice(1); + return (cut ? '…' : '') + kept; } override get pipeFailure(): Error | undefined { @@ -300,12 +340,8 @@ export class StdioTransport extends McplTransport { let carry = ''; const decoder = new StringDecoder('utf8'); const received = (text: string) => { - const end = this.stderrEnd + text; - this.stderrCut ||= end.length > STDERR_TAIL_CHARS; - let kept = end.slice(-STDERR_TAIL_CHARS); - // A cut between a surrogate pair's halves drops the low half too. - if (/^[\uDC00-\uDFFF]/.test(kept) && end.length > STDERR_TAIL_CHARS) kept = kept.slice(1); - this.stderrEnd = kept; + const window = this.stderrWindow + text; + this.stderrWindow = window.slice(-STDERR_WINDOW_CHARS); const lines = (carry + text).split('\n'); carry = lines.pop() ?? ''; for (const l of lines) if (l.length > 0) this.pushStderr(l); @@ -374,10 +410,32 @@ export class StdioTransport extends McplTransport { `MCPL server "${config.id}": stdio transport requires "command" (none provided)`, ); } - const child = spawn(config.command, config.args ?? [], { - stdio: ['pipe', 'pipe', 'pipe'], - env: buildChildEnv(config), - }); + // Node's error for a launch that failed reaches the host, and its words + // and its other properties (`spawnargs`, `path`) can repeat a credential + // the configuration holds. So it's handed on as its name and redacted + // words (maskedError), with its errno code, which says what failed. What + // spawn() refuses at once is one of the configuration's own arguments or + // options, which Node quotes after " Received", cut to 128 characters: a + // credential across that cut couldn't be matched, so the words stop + // before the quote, still saying which value Node refused and why. + const failed = (err: NodeJS.ErrnoException, refusedAtOnce: boolean): Error => { + const redact = serverTextRedactor(config).now(); + const message = err instanceof Error ? err.message : String(err); + const words = refusedAtOnce ? message.split(' Received')[0] : message; + const shown = Object.assign(new Error(words), { name: err instanceof Error ? err.name : 'Error' }); + const cause = Object.assign(maskedError(shown, redact), err.code === undefined ? {} : { code: err.code }); + return new Error(`MCPL server "${config.id}" could not be started: ${redact(words)}`, { cause }); + }; + // The child's environment is read first, outside that: a configuration + // whose `env` can't be read throws the framework's own error, such as + // its awareness ledger's, which its startup acts on. + const env = buildChildEnv(config); + let child: ChildProcess; + try { + child = spawn(config.command, config.args ?? [], { stdio: ['pipe', 'pipe', 'pipe'], env }); + } catch (error) { + throw failed(error as NodeJS.ErrnoException, true); + } // Node reports a failed spawn (a command that doesn't exist, say) as an // 'error' on a tick after spawn() returns, and no 'exit' follows. It is // settled here, where it is listened for. A caller that listens only once @@ -385,12 +443,10 @@ export class StdioTransport extends McplTransport { // from a timer callback, as a reconnect is: Node runs that tick before the // caller's promise continuation, and throws an 'error' nothing listens for. await new Promise((resolve, reject) => { - const failed = (err: Error) => { - reject(new Error(`MCPL server "${config.id}" could not be started: ${err.message}`, { cause: err })); - }; - child.once('error', failed); + const fail = (err: NodeJS.ErrnoException) => reject(failed(err, false)); + child.once('error', fail); child.once('spawn', () => { - child.off('error', failed); + child.off('error', fail); resolve(); }); }); diff --git a/test/mcp-modern-http.test.ts b/test/mcp-modern-http.test.ts index debfc427d..416cc6e1a 100644 --- a/test/mcp-modern-http.test.ts +++ b/test/mcp-modern-http.test.ts @@ -198,6 +198,29 @@ test('HTTP: an auth wall on a call is an error-response with the status in data' }); }); +test("HTTP: an auth wall's status stays this host's number when the server's configuration can't be read", async (t) => { + t.mock.method(console, 'error', () => {}); + let allow = true; + let unreadable = false; + const { url, server } = await startServer(() => allow); + cleanups.push(() => new Promise((resolve) => { server.closeAllConnections?.(); server.close(() => resolve()); })); + // An `env` getter that throws, as the framework's own can, makes the + // server's text withheld; the status is this host's, not the server's. + const config = { id: 'remote', url, token: 't' }; + Object.defineProperty(config, 'env', { enumerable: true, get: () => { if (unreadable) throw new Error('unreadable'); return undefined; } }); + const connection = await ModernMcpConnection.connect(config); + cleanups.push(() => { unreadable = false; return connection.close(); }); + allow = false; + unreadable = true; + await assert.rejects(connection.callTool('echo', { text: 'x' }), (err: unknown) => { + assert.ok(err instanceof McplRequestError); + assert.equal(err.outcome, 'error-response'); + // The server's own data is withheld, and the status stays. + assert.deepEqual(err.data, { httpStatus: 401 }); + return true; + }); +}); + test('HTTP: an auth wall on a later round of a call is a no-response, since the first round may have acted', async (t) => { const consoleLines: string[] = []; t.mock.method(console, 'error', (...args: unknown[]) => consoleLines.push(args.map(String).join(' '))); @@ -235,7 +258,7 @@ test('HTTP: an auth wall on a later round of a call is a no-response, since the assert.ok(states.length >= 2 && states.slice(1).every((s) => s === 'state-1'), `each later round carried it: ${JSON.stringify(states)}`); }); -test("HTTP: an echoed path in a tool transport failure is redacted in the log and kept from the agent's text, but a JSON-RPC error is the server answer", async (t) => { +test("HTTP: an echoed path in a tool transport failure or a JSON-RPC error is redacted in the log and kept from the caller's and the agent's text", async (t) => { const consoleLines: string[] = []; t.mock.method(console, 'error', (...args: unknown[]) => consoleLines.push(args.map(String).join(' '))); const secret = 'pathSecret0123456789'; @@ -243,7 +266,7 @@ test("HTTP: an echoed path in a tool transport failure is redacted in the log an const { url, server } = await startServer(() => true, (message, res) => { if (message.method !== 'tools/call') return false; if (rpc) { - res.writeHead(200, { 'content-type': 'application/json' }).end(JSON.stringify({ jsonrpc: '2.0', id: message.id, error: { code: -32603, message: secret } })); + res.writeHead(200, { 'content-type': 'application/json' }).end(JSON.stringify({ jsonrpc: '2.0', id: message.id, error: { code: -32603, message: secret, data: { echoed: secret } } })); } else { res.writeHead(500, { 'content-type': 'text/plain' }).end(`Cannot POST /api/mcp/s/${secret}/mcp`); } @@ -254,7 +277,10 @@ test("HTTP: an echoed path in a tool transport failure is redacted in the log an cleanups.push(() => connection.close()); await assert.rejects(connection.callTool('echo', {}), (error: unknown) => { assert.ok(error instanceof McplRequestError); - assert.match(error.message, new RegExp(secret)); + // The server's words and data reach the caller with the url's + // credential redacted, as an error the framework hands a host does. + assert.equal(error.message, 'The "remote" server answered with an error: redacted'); + assert.deepEqual(error.data, { echoed: 'redacted' }); assert.equal(error.outcome, 'error-response'); return true; }); @@ -301,7 +327,7 @@ test("HTTP: a 403 refusal's response body, which can echo the url, stays out of connection.id = 'remote'; connection.config = { id: 'remote', url: 'https://h/api/mcp/s/pathSecret0123456789/mcp' }; const refusal = new SdkHttpError(SdkErrorCode.ClientHttpForbidden, 'Server returned 403 insufficient_scope', { - status: 403, statusText: 'Forbidden', text: 'denied for /api/mcp/s/pathSecret0123456789/mcp', + status: 403, statusText: 'Forbidden for /api/mcp/s/pathSecret0123456789/mcp', text: 'denied for /api/mcp/s/pathSecret0123456789/mcp', }); const err = connection.requestFailure('tools/call "echo"', { method: 'tools/call', handedOff: true, answer: 'none' }, refusal); assert.ok(err instanceof McplRequestError); diff --git a/test/mcp-modern-outcomes.test.ts b/test/mcp-modern-outcomes.test.ts index f2ab07915..918f2bcc9 100644 --- a/test/mcp-modern-outcomes.test.ts +++ b/test/mcp-modern-outcomes.test.ts @@ -983,10 +983,10 @@ test('failures are classified in order: nothing sent first, then the answers, th 'is unknown. Check its effect before trying again.'); loggedLine(logged, /^MCP server "raw" answered tools\/call op with error -32603: "\\u0020\\u0020"$/); - // The server's words reach the caller unredacted, and the log redacted, - // whatever JSON value they are. + // The server's words reach the caller and the log redacted, whatever + // JSON value they are. const echoed = classify({ answer: 'error', error: { code: -32603, message: `bad token ${token}` } }, new Error('the SDK\'s words')); - assert.equal(echoed.message, `The "raw" server answered with an error: bad token ${token}`); + assert.equal(echoed.message, 'The "raw" server answered with an error: bad token redacted'); loggedLine(logged, /^MCP server "raw" answered tools\/call op with error -32603: "bad\\u0020token\\u0020redacted"$/); classify({ answer: 'error', error: { code: -32603, message: { said: `bad token ${token}` } } }, new Error('the SDK\'s words')); assert.ok(logged.some((line) => line.endsWith('with error -32603: "{\\"said\\":\\"bad\\u0020token\\u0020redacted\\"}"')), logged.join('\n')); diff --git a/test/mcp-recorded-text.test.ts b/test/mcp-recorded-text.test.ts index f2b76c5e1..e0a01b331 100644 --- a/test/mcp-recorded-text.test.ts +++ b/test/mcp-recorded-text.test.ts @@ -5,13 +5,15 @@ import type { AddressInfo } from 'node:net'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { Client, type Transport } from '@modelcontextprotocol/client'; +import { Client, UnsupportedProtocolVersionError, type Transport } from '@modelcontextprotocol/client'; import { EventEmitter } from 'node:events'; +import { inspect } from 'node:util'; import type { ModuleContext } from '../src/index.js'; import { AgentFramework } from '../src/framework.js'; -import { McplProtocolVersionError, McplServerConnection } from '../src/mcpl/server-connection.js'; +import { McplProtocolVersionError, McplRequestError, McplServerConnection, McplUnreapedLaunchError } from '../src/mcpl/server-connection.js'; import { ModernMcpConnection, malformedLineDiagnostic } from '../src/mcpl/modern-connection.js'; -import { noteDialSent, redactServerText, serverTextRedactor } from '../src/mcpl/protocol-family.js'; +import { STDERR_TAIL_CHARS, STDERR_WINDOW_CHARS } from '../src/mcpl/transport.js'; +import { TOO_DEEP_TEXT, WITHHELD_TEXT, maskedError, noteDialSent, redactServerText, redactedData, serverTextRedactor } from '../src/mcpl/protocol-family.js'; import { MockMembrane } from './helpers/mock-membrane.js'; /** Discovery succeeds; the inventory request gets a normal echoed-path 404. */ @@ -122,7 +124,7 @@ test('an SDK HTTP 404 error keeps its path but hides the secret in connection co }); -test('startup records redact an echoed path while the direct host connect error stays as raised', async (t) => { +test('startup records and the direct host connect error both redact an echoed path', async (t) => { failConnectWithHttpBody(t); const dir = mkdtempSync(join(tmpdir(), 'mcp-startup-text-')); const consoleLines: string[] = []; @@ -133,8 +135,10 @@ test('startup records redact an echoed path while the direct host connect error const url = `http://127.0.0.1:${(server.address() as AddressInfo).port}/api/mcp/s/${secret}/mcp`; let framework: AgentFramework | undefined; try { - await assert.rejects(ModernMcpConnection.connect({ id: 'raw', url }), error => { - assert.ok(error instanceof Error && error.message.includes(secret)); + await assert.rejects(ModernMcpConnection.connect({ id: 'raw', url }), (error: unknown) => { + assert.ok(error instanceof Error && error.message.includes('Cannot POST /api/mcp/s/redacted/mcp'), String(error)); + assert.ok(!error.message.includes(secret), error.message); + assert.equal(error.cause, undefined, 'the raw error is not kept as its cause'); return true; }); framework = await AgentFramework.create({ @@ -438,3 +442,532 @@ test("a runtime connect's failed cleanup is traced without the server's credenti } }); + +// A stdio MCPL server whose words quote its own credential, an env value +// whose name says it's one: in its initialize error (MODE init-error), its +// refusal of the protocol revision with that in its data too +// (version-error), the revision it answers with (mismatch), its tools/call +// error with that in its data too (tool-error), or a tools/call error sent +// after the call's deadline (late-error). PAD characters come first, so the +// credential can straddle the 200 code points a server's words are cut to. +const QUOTING_SERVER = String.raw` +let buf = ''; +let held = null; +const mode = process.env.MODE; +const token = process.env.API_TOKEN; +const said = 'x'.repeat(Number(process.env.PAD || 0)) + token + (process.env.TAIL ?? ' and more words'); +process.stdin.on('data', (c) => { + buf += c.toString('utf8'); + let i; + while ((i = buf.indexOf('\n')) >= 0) { + const line = buf.slice(0, i); buf = buf.slice(i + 1); + if (!line.trim()) continue; + let m; try { m = JSON.parse(line); } catch { continue; } + if (m.id === undefined || !m.method) continue; + const send = (o, id = m.id) => process.stdout.write(JSON.stringify(Object.assign({ jsonrpc: '2.0', id }, o)) + '\n'); + if (m.method === 'initialize') { + if (mode === 'init-error') send({ error: { code: -32603, message: said } }); + else if (mode === 'version-error') send({ error: { code: -32022, message: said, data: { echoed: token } } }); + else if (mode === 'version-list') send({ error: { code: -32022, message: said, data: { supported: ['2024-10-07', token, ...Array.from({ length: 30 }, (_, n) => 'v' + String(n).padStart(10, '0'))] } } }); + else if (mode === 'stderr-exit') { + // The token between BEFORE a's and AFTER b's, each side joined by + // SEP, then the child exits before it answers. + const sep = process.env.SEP ?? ''; + process.stderr.write('a'.repeat(Number(process.env.BEFORE)) + sep + token + sep + 'b'.repeat(Number(process.env.AFTER)) + '\n', () => process.exit(1)); + } + else if (mode === 'exit-quiet') process.exit(1); + else if (mode === 'stderr-keys') { + // The token, then COPIES lines of the private key, then a line of + // FILLER f's, then the child exits before it answers. + process.stderr.write(token + ('\n' + process.env.PRIVATE_KEY).repeat(Number(process.env.COPIES)) + '\n' + 'f'.repeat(Number(process.env.FILLER)) + '\n', () => process.exit(1)); + } + else send({ result: { protocolVersion: mode === 'mismatch' ? said : '2024-11-05', capabilities: { experimental: { mcpl: { version: '0.5', pushEvents: true } } } } }); + } else if (m.method === 'tools/list') { + // cursor-repeat answers every page with the same cursor, its words; + // cursor-object with a cursor that isn't a string. + const nextCursor = mode === 'cursor-repeat' ? said : mode === 'cursor-object' ? { quoted: said } : undefined; + send({ result: { tools: [{ name: 'x', description: 'x', inputSchema: { type: 'object' } }], ...(nextCursor === undefined ? {} : { nextCursor }) } }); + } + else if (m.method === 'tools/call') { + // code-quote's error answer has its token for a code, which JSON-RPC + // says is a number. + const answer = { error: { code: mode === 'code-quote' ? token : -32000, message: said, data: { echoed: token } } }; + // late-error holds its first call's answer until the next call comes, + // and sends it first then, after the first call's deadline. + if (mode === 'late-error' && held === null) held = m.id; + else { + if (held !== null) { send(answer, held); held = -1; } + send(answer); + } + } else send({ result: {} }); + } +}); +`; + +/** The quoting server's credential: one cut at the 200th code point, one + * holding a TAB, and one that ends in a space and ends the server's words. */ +const QUOTED: Array<{ kind: string; token: string; pad: number; kept: string; tail?: string }> = [ + { kind: 'across the cut', token: 'Kq7vR2mXp9LwT4sB8nYd', pad: 185, kept: 'Kq7vR2mXp9' }, + { kind: 'holding a TAB', token: 'Kq7vR2mXp9\tLwT4sB8nYd', pad: 10, kept: 'Kq7vR2mXp9' }, + { kind: 'ending in a space at the end of the words', token: 'Kq7vR2mXp9LwT4sB8nYd ', pad: 10, kept: 'Kq7vR2mXp9', tail: '' }, +]; + +/** The quoting server's environment for a mode and a credential. */ +const quotingEnv = (mode: string, quoted: typeof QUOTED[number]) => ({ + API_TOKEN: quoted.token, MODE: mode, PAD: String(quoted.pad), ...(quoted.tail === undefined ? {} : { TAIL: quoted.tail }), +}); + +async function frameworkWithQuotingServer(dir: string, mode: string, quoted: typeof QUOTED[number], traces: Array>) { + return AgentFramework.create({ + storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'agent', model: 'test', systemPrompt: 'test' }], + modules: [{ name: 'watcher', async start(ctx: ModuleContext) { ctx.onTrace(event => traces.push(event as never)); }, async stop() {}, getTools: () => [], async handleToolCall() { return { success: false }; }, async onProcess() { return {}; } }], + mcplServers: [{ id: 'probe', command: process.execPath, args: ['-e', QUOTING_SERVER], env: quotingEnv(mode, quoted) }], + }); +} + +for (const quoted of QUOTED) { + test(`an initialize error that quotes a credential ${quoted.kind} is recorded without any of it`, async (t) => { + const dir = mkdtempSync(join(tmpdir(), 'mcp-quoted-init-')); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => lines.push(args.map(String).join(' '))); + const traces: Array> = []; + let framework: AgentFramework | undefined; + try { + framework = await frameworkWithQuotingServer(dir, 'init-error', quoted, traces); + const failed = traces.find(e => e.type === 'mcpl:server-connect-failed'); + assert.ok(failed, JSON.stringify(traces.map(e => e.type))); + assert.match(String(failed!.error), /initialize error: x/); + assert.ok(!JSON.stringify(traces).includes(quoted.kept), JSON.stringify(failed)); + assert.ok(!lines.join('\n').includes(quoted.kept), lines.join('\n')); + } finally { + await framework?.stop().catch(() => {}); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test(`a tool call's error that quotes a credential ${quoted.kind} reaches its caller without any of it`, async (t) => { + const dir = mkdtempSync(join(tmpdir(), 'mcp-quoted-call-')); + t.mock.method(console, 'error', () => {}); + const traces: Array> = []; + let framework: AgentFramework | undefined; + try { + framework = await frameworkWithQuotingServer(dir, 'tool-error', quoted, traces); + const result = await framework.executeToolCall({ id: 'c1', name: 'mcpl--probe--x', input: {} }); + assert.equal(result.success, false); + assert.match(String(result.error), /answered with an error: x/); + assert.ok(!String(result.error).includes(quoted.kept), String(result.error)); + } finally { + await framework?.stop().catch(() => {}); + rmSync(dir, { recursive: true, force: true }); + } + }); +} + +test("a late tools/call error's record is redacted, then bounded, like an on-time one's", async (t) => { + const dir = mkdtempSync(join(tmpdir(), 'mcp-quoted-late-')); + t.mock.method(console, 'error', () => {}); + const traces: Array> = []; + // The record is `[-32000] ` and the words, cut to 500 code points: the + // padding puts the credential across the cut. + const quoted = { kind: "across the record's cut", token: 'Kq7vR2mXp9LwT4sB8nYd', pad: 481, kept: 'Kq7vR2mXp9' }; + let framework: AgentFramework | undefined; + try { + framework = await frameworkWithQuotingServer(dir, 'late-error', quoted, traces); + const server = (framework as unknown as { mcplServerRegistry: { getServer(id: string): McplServerConnection } }) + .mcplServerRegistry.getServer('probe'); + await assert.rejects(server.sendToolsCallWithDeadline('x', {}, 100), /didn't answer within/); + // The server answers the first call only when a second one comes, so + // its answer comes after the first call's deadline. + await assert.rejects(server.sendToolsCall('x', {}), /answered with an error/); + const deadline = Date.now() + 5_000; + while (!traces.some(e => e.type === 'mcpl:orphaned-response')) { + assert.ok(Date.now() < deadline, 'timed out waiting for the late answer\'s record'); + await new Promise((resolve) => setTimeout(resolve, 20)); + } + const late = traces.find(e => e.type === 'mcpl:orphaned-response')!; + assert.match(String(late.error), /\[-32000\] x/); + assert.ok(!JSON.stringify(late).includes(quoted.kept), JSON.stringify(late)); + } finally { + await framework?.stop().catch(() => {}); + rmSync(dir, { recursive: true, force: true }); + } +}); + +/** The quoting server's configuration, for a connection made without the framework. */ +const quotingServer = (mode: string, quoted: typeof QUOTED[number]) => ({ + id: 'probe', command: process.execPath, args: ['-e', QUOTING_SERVER], env: quotingEnv(mode, quoted), +}); + +for (const quoted of QUOTED) { + test(`a legacy connect's error carries none of a credential ${quoted.kind} its server's initialize error quotes`, async () => { + await assert.rejects(McplServerConnection.connect(quotingServer('init-error', quoted), { version: '0.5' }), (error: Error) => { + assert.match(error.message, /initialize error: x/); + assert.ok(!error.message.includes(quoted.kept), error.message); + return true; + }); + }); + + test(`a legacy tool call's error carries none of a credential ${quoted.kind} its server's words or data quote`, async () => { + const connection = await McplServerConnection.connect(quotingServer('tool-error', quoted), { version: '0.5' }); + try { + connection.ready(); + await assert.rejects(connection.sendToolsCall('x', {}), (error: unknown) => { + assert.ok(error instanceof McplRequestError); + assert.match(error.message, /answered with an error: x/); + assert.ok(!error.message.includes(quoted.kept), error.message); + assert.ok(!JSON.stringify(error.data).includes(quoted.kept), JSON.stringify(error.data)); + return true; + }); + } finally { + await connection.close(); + } + }); +} + +test("a legacy connect's refusal of the protocol revision carries none of a credential its server's words, data or revision quote", async () => { + const quoted = QUOTED[0]; + await assert.rejects(McplServerConnection.connect(quotingServer('version-error', quoted), { version: '0.5' }), (error: unknown) => { + assert.ok(error instanceof McplProtocolVersionError); + assert.equal(error.kind, 'rejected'); + assert.ok(!error.message.includes(quoted.kept), error.message); + assert.ok(!JSON.stringify(error.data).includes(quoted.kept), JSON.stringify(error.data)); + return true; + }); + // A long list of revisions, one quoting the credential: redacted, then bounded. + await assert.rejects(McplServerConnection.connect(quotingServer('version-list', quoted), { version: '0.5' }), (error: unknown) => { + assert.ok(error instanceof McplProtocolVersionError); + assert.match(error.message, /\(it supports: 2024-10-07, redacted, v0000000000, [^)]*…\)/); + assert.equal(error.supported?.length, 32); + assert.equal(error.supported?.[1], 'redacted'); + return true; + }); + // A long revision quoting the credential: redacted, then bounded. + await assert.rejects(McplServerConnection.connect(quotingServer('mismatch', quoted), { version: '0.5' }), (error: unknown) => { + assert.ok(error instanceof McplProtocolVersionError); + assert.equal(error.kind, 'mismatch'); + assert.match(error.message, /protocolVersion "x+redacted and [^;]*…; this client's legacy engine/); + assert.ok(!error.message.includes(quoted.kept), error.message); + assert.ok(!String(error.returned).includes(quoted.kept), String(error.returned)); + return true; + }); +}); + +test("a modern connect's failure carries the library's words redacted, and none of what else its error carries", async (t) => { + const url = `https://127.0.0.1:9/api/mcp/s/${secret}/mcp`; + t.mock.method(console, 'error', () => {}); + // As a fetch fails on Bun: a TypeError with the url in its own `path`. + const bun = t.mock.method(Client.prototype, 'connect', async () => { + throw Object.assign(new TypeError('ECONNREFUSED: Unable to connect. Is the computer able to access the url?'), { path: url, code: 'ECONNREFUSED' }); + }); + await assert.rejects(ModernMcpConnection.connect({ id: 'raw', url }), (error: unknown) => { + assert.ok(error instanceof Error); + assert.equal(error.name, 'TypeError'); + assert.equal(error.message, 'ECONNREFUSED: Unable to connect. Is the computer able to access the url?'); + assert.equal((error as { path?: unknown }).path, undefined); + assert.equal((error as { code?: unknown }).code, undefined); + return true; + }); + bun.mock.restore(); + // Under a wrapper that quotes the url and keeps the raw error as its cause. + t.mock.method(Client.prototype, 'connect', async () => { + const raw = Object.assign(new Error(`Unable to connect: ${url}`), { path: url, code: 'ConnectionRefused' }); + throw new Error(`fetch failed for ${url}`, { cause: raw }); + }); + await assert.rejects(ModernMcpConnection.connect({ id: 'raw', url }), (error: unknown) => { + assert.ok(error instanceof Error); + assert.equal(error.message, 'fetch failed for https://127.0.0.1:9/api/mcp/s/redacted/mcp'); + assert.equal(error.cause, undefined); + return true; + }); +}); + +test("a modern refusal of the protocol revision carries none of a credential the server's words or data quote", () => { + const refusal = new UnsupportedProtocolVersionError({ supported: ['2024-11-05'], requested: '2026-07-28' }); + // A server's own words and data, quoting the url's path credential. + Object.defineProperty(refusal, 'message', { value: `not at /api/mcp/s/${secret}/mcp` }); + Object.defineProperty(refusal, 'data', { value: { supported: `none for ${secret}`, echoed: secret } }); + const connection = ModernMcpConnection.create({ id: 'remote', url: `https://h/api/mcp/s/${secret}/mcp` }) as unknown as { + versionVerdict(error: unknown): McplProtocolVersionError | null; + }; + const verdict = connection.versionVerdict(refusal)!; + assert.ok(verdict instanceof McplProtocolVersionError); + assert.match(verdict.message, /\(not at \/api\/mcp\/s\/redacted\/mcp\)/); + assert.ok(!verdict.message.includes(secret), verdict.message); + assert.ok(!JSON.stringify(verdict.data).includes(secret), JSON.stringify(verdict.data)); + // A long list of the revisions it supports, one of them quoting the + // credential: redacted, then bounded. + const others = Array.from({ length: 30 }, (_, n) => 'v' + String(n).padStart(10, '0')); + const listed = new UnsupportedProtocolVersionError({ supported: ['2024-11-05', secret, ...others], requested: '2026-07-28' }); + const listedVerdict = connection.versionVerdict(listed)!; + assert.match(listedVerdict.message, /\(it supports: 2024-11-05, redacted, v0000000000, [^)]*…\)/); + assert.deepEqual(listedVerdict.supported, ['2024-11-05', 'redacted', ...others]); +}); + +test("an error answer whose code quotes a credential is logged without it, by either engine", async (t) => { + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => lines.push(args.map(String).join(' '))); + const quoted = QUOTED[0]; + const connection = await McplServerConnection.connect(quotingServer('code-quote', quoted), { version: '0.5' }); + try { + connection.ready(); + await assert.rejects(connection.sendToolsCall('x', {}), /answered with an error/); + } finally { + await connection.close(); + } + const modern = ModernMcpConnection.create({ id: 'remote', url: 'https://h/mcp', token: quoted.token }) as unknown as { + requestFailure(what: string, record: unknown, error: unknown): Error; + }; + modern.requestFailure('tools/call', { method: 'tools/call', handedOff: true, answer: 'error', error: { code: quoted.token, message: 'refused' }, continued: false }, new Error('answered')); + const logged = lines.filter((line) => /with error /.test(line)); + assert.equal(logged.length, 2, lines.join('\n')); + assert.ok(!logged.join('\n').includes(quoted.kept), logged.join('\n')); +}); + +for (const quoted of QUOTED) { + test(`a legacy tools/list's cursor errors carry none of a credential ${quoted.kind} the server's cursor quotes`, async () => { + for (const mode of ['cursor-repeat', 'cursor-object']) { + const connection = await McplServerConnection.connect(quotingServer(mode, quoted), { version: '0.5' }); + try { + connection.ready(); + await assert.rejects(connection.sendToolsList(), (error: Error) => { + assert.match(error.message, mode === 'cursor-repeat' ? /repeated tools\/list cursor "x+redacted/ : /nextCursor must be a string, got \{"quoted":"x+red/); + assert.ok(!error.message.includes(quoted.kept), error.message); + // The cursor is cut to 200 code points after it's redacted. + if (quoted.pad > 100) assert.match(error.message, /…$/); + return true; + }); + } finally { + await connection.close(); + } + } + }); +} + +for (const quoted of QUOTED) { + test(`a modern error answer's words, quoting a credential ${quoted.kind}, reach the caller without any of it`, (t) => { + t.mock.method(console, 'error', () => {}); + const connection = ModernMcpConnection.create({ id: 'remote', url: 'https://h/mcp', token: quoted.token }) as unknown as { + requestFailure(what: string, record: unknown, error: unknown): Error; + }; + const words = 'x'.repeat(quoted.pad) + quoted.token + (quoted.tail ?? ' and more words'); + const record = { method: 'tools/call', handedOff: true, answer: 'error', error: { code: -32000, message: words, data: { echoed: quoted.token } }, continued: false }; + const error = connection.requestFailure('tools/call', record, new Error('answered with an error')); + assert.ok(error instanceof McplRequestError); + assert.match(error.message, /answered with an error: x/); + assert.ok(!error.message.includes(quoted.kept), error.message); + assert.ok(!JSON.stringify(error.data).includes(quoted.kept), JSON.stringify(error.data)); + }); +} + +test("a failed legacy handshake's stderr tail goes to the host redacted, the credential whole when it's matched though the tail's cut splits it", async () => { + const failing = (token: string, layout: Record) => + McplServerConnection.connect({ ...quotingServer('stderr-exit', { kind: 'tail', token, pad: 0, kept: '' }), env: { API_TOKEN: token, MODE: 'stderr-exit', ...layout } }, { version: '0.5' }); + // A token on one line with no newline, the tail's cut 9 characters from its end. + const token = 'Kq7vR2mXp9LwT4sB8nYd'; + const after = STDERR_TAIL_CHARS - 9 - 1; + await assert.rejects(failing(token, { BEFORE: '3000', AFTER: String(after) }), (error: Error & { stderrTail?: string }) => { + // The token is whole when it's matched, so the tail holds it redacted. + assert.equal(error.stderrTail, `\u2026redacted${'b'.repeat(after)}`); + return true; + }); + // A credential longer than the tail, PEM-like lines about the size of a + // 4,096-bit private key's, its start well before the tail's cut and its + // end inside the tail. + const pem = Array.from({ length: 80 }, (_, n) => `MIIJQwIBADANBgkqhkiG9w0BAQEFAASCCS0wggkpAgEAAo${String(n).padStart(4, '0')}`).join('\n'); + assert.ok(pem.length > STDERR_TAIL_CHARS); + await assert.rejects(failing(pem, { BEFORE: '100', SEP: '\n', AFTER: '1000' }), (error: Error & { stderrTail?: string }) => { + assert.equal(error.stderrTail, `\u2026redacted\n${'b'.repeat(1000)}`); + return true; + }); + // The tail begins where the raw tail does, however much the redaction + // shortens the window. Here the window's own cut falls 5 characters into + // the token, and the 16 lines of a 1,000-character key after it redact to + // 9 characters each, so the redacted window is shorter than the tail. The + // tail begins inside the 15th key, which it replaces whole, and keeps + // none of the token the window's cut split. + const key = 'k'.repeat(1000); + // The stream is the token, 16 times a newline and the key, a newline, the + // filler and a newline: 5 characters more than the window. + const filler = STDERR_WINDOW_CHARS + 5 - token.length - 16 * (1 + key.length) - 2; + await assert.rejects(McplServerConnection.connect({ ...quotingServer('stderr-keys', { kind: 'keys', token, pad: 0, kept: '' }), env: { API_TOKEN: token, PRIVATE_KEY: key, MODE: 'stderr-keys', COPIES: '16', FILLER: String(filler) } }, { version: '0.5' }), (error: Error & { stderrTail?: string }) => { + assert.equal(error.stderrTail, `\u2026redacted\nredacted\n${'f'.repeat(filler)}`); + return true; + }); +}); + +test("a launch that wrote nothing to stderr has no stderr tail, even while its configuration can't be read", async () => { + let readable = true; + const launched = quotingServer('exit-quiet', QUOTED[0]); + const config = Object.defineProperty({ ...launched }, 'env', { + enumerable: true, + configurable: true, + get() { if (!readable) throw new Error('the ledger cannot be read'); return launched.env; }, + }); + // The launch reads the configuration as connect() starts, and it turns + // unreadable after that, before the handshake fails. + const connecting = McplServerConnection.connect(config, { version: '0.5' }); + readable = false; + await assert.rejects(connecting, (error: Error & { stderrTail?: string }) => { + assert.match(error.message, /transport closed before handshake completed/); + assert.equal(error.stderrTail, undefined); + return true; + }); +}); + +test("a stdio server that can't be started is refused without its command line, which can repeat a credential, and with its errno code", async () => { + const token = 'envSecret0123456789'; + // The command's path and an argument both repeat the credential. + const missing = (at: string) => `/nonexistent/${at}/mcpl-server-that-is-not-here`; + await assert.rejects(McplServerConnection.connect({ id: 'missing', command: missing(token), args: ['--token', token], env: { API_TOKEN: token } }, { version: '0.5' }), (error: Error) => { + assert.equal(error.message, `MCPL server "missing" could not be started: spawn ${missing('redacted')} ENOENT`); + const cause = error.cause as NodeJS.ErrnoException & { spawnargs?: unknown }; + assert.equal(cause.message, `spawn ${missing('redacted')} ENOENT`); + assert.equal(cause.code, 'ENOENT'); + for (const kept of ['spawnargs', 'path', 'syscall', 'errno']) assert.equal((cause as unknown as Record)[kept], undefined, kept); + assert.ok(!inspect(error, { depth: 5, showHidden: true }).includes(token), inspect(error, { depth: 5 })); + return true; + }); + // An argument or an env value Node refuses at spawn() itself, before any + // 'error': Node quotes it after " Received", cut to 128 characters, here + // across the credential, so its words stop before the quote. + const refusals = [ + { id: 'refused-arg', args: [`--t=${'a'.repeat(110)}${token}\u0000`], env: { API_TOKEN: token }, said: "The argument 'args[0]' must be a string without null bytes." }, + { id: 'refused-env', args: [], env: { API_TOKEN: `${token}\u0000` }, said: "The property 'options.env['API_TOKEN']' must be a string without null bytes." }, + ]; + for (const { id, args, env, said } of refusals) { + await assert.rejects(McplServerConnection.connect({ id, command: process.execPath, args, env }, { version: '0.5' }), (error: Error) => { + assert.equal(error.message, `MCPL server "${id}" could not be started: ${said}`); + const cause = error.cause as NodeJS.ErrnoException; + assert.equal(cause.message, said); + assert.equal(cause.code, 'ERR_INVALID_ARG_VALUE'); + assert.ok(!inspect(error, { depth: 5, showHidden: true }).includes(token.slice(0, 8)), inspect(error, { depth: 5 })); + return true; + }); + } +}); + +test("a redaction from a place in the text gives what lies from there on, a credential that reaches across it replaced whole", () => { + const redact = serverTextRedactor(config); + const text = `aaa ${secret} bbb`; + assert.equal(redact(text, 0), 'aaa redacted bbb'); + assert.equal(redact(text, 2), 'a redacted bbb'); + // From inside the credential: matched in the whole text, replaced whole. + assert.equal(redact(text, 4 + 5), 'redacted bbb'); + // From where it ends: none of it. + assert.equal(redact(text, 4 + secret.length), ' bbb'); + // From past it: only what lies there, and from the text's end, nothing. + assert.equal(redact(text, 4 + secret.length + 1), 'bbb'); + assert.equal(redact(text, text.length), ''); + assert.equal(redact(text, text.length + 5), ''); + // Two credentials that overlap are one place, replaced whole from inside either. + const overlapping = serverTextRedactor({ url: 'https://h/mcp', token: 'abcdefgh12', env: { API_KEY: 'gh12ijklmn' } }); + assert.equal(overlapping('xx abcdefgh12ijklmn yy', 0), 'xx redacted yy'); + assert.equal(overlapping('xx abcdefgh12ijklmn yy', 14), 'redacted yy'); + // The same through one build, and with nothing to replace. + assert.equal(redact.now()(text, 4 + 5), 'redacted bbb'); + assert.equal(serverTextRedactor({})(text, 4), `${secret} bbb`); +}); + +test("an error answer's data is redacted through and through: its keys, keeping every entry, its numbers, and plain objects, to a depth past which it's withheld", () => { + const redact = serverTextRedactor(config); + assert.deepEqual( + redactedData({ [secret]: [secret, { deep: `at ${secret}` }], [config.token]: 'kept', count: 2, none: null }, redact), + { redacted: ['redacted', { deep: 'at redacted' }], 'redacted (2)': 'kept', count: 2, none: null }, + ); + // A key of the server's own that reads `redacted` keeps its name. + assert.deepEqual(redactedData({ [secret]: 'diagnostic', redacted: false }, redact), { 'redacted (2)': 'diagnostic', redacted: false }); + // A number whose digits are a credential. + assert.deepEqual(redactedData({ pin: 9876543210123, count: 2 }, serverTextRedactor({ url: 'https://h/mcp', token: '9876543210123' })), { pin: 'redacted', count: 2 }); + // An object with no prototype is walked; an instance of a class is not. + assert.deepEqual(redactedData(Object.assign(Object.create(null), { said: secret }), redact), { said: 'redacted' }); + class Opaque { constructor(readonly said: string) {} } + const opaque = new Opaque(secret); + assert.equal(redactedData(opaque, redact), opaque); + // 64 levels of arrays or objects are walked; the 65th is withheld. + const inArrays = (levels: number, inner: unknown): unknown => (levels === 0 ? inner : [inArrays(levels - 1, inner)]); + const inObjects = (levels: number, inner: unknown): unknown => (levels === 0 ? inner : { at: inObjects(levels - 1, inner) }); + assert.deepEqual(redactedData(inArrays(64, secret), redact), inArrays(64, 'redacted')); + assert.deepEqual(redactedData(inArrays(65, secret), redact), inArrays(64, TOO_DEEP_TEXT)); + assert.deepEqual(redactedData(inObjects(64, secret), redact), inObjects(64, 'redacted')); + assert.deepEqual(redactedData(inObjects(65, secret), redact), inObjects(64, TOO_DEEP_TEXT)); + // Nested deeper than a recursive walk's stack holds, as a server's JSON can + // be, in arrays and in objects. + for (const wrap of [(v: unknown) => [v], (v: unknown) => ({ at: v })]) { + let deep: unknown = secret; + for (let i = 0; i < 10_000; i++) deep = wrap(deep); + const walked = JSON.stringify(redactedData(deep, redact)); + assert.ok(walked.includes(JSON.stringify(TOO_DEEP_TEXT)) && !walked.includes(secret), walked.slice(-200)); + } + // With a configuration that can't be read, the data is withheld as a + // whole, keys and all, and absent data stays absent. + const unreadable = { id: 'unreadable', url: 'https://h/mcp' }; + Object.defineProperty(unreadable, 'env', { enumerable: true, get() { throw new Error('the ledger cannot be read'); } }); + for (const redactor of [serverTextRedactor(unreadable), serverTextRedactor(unreadable).now()]) { + assert.equal(redactedData({ [secret]: [secret], count: 2, ok: true }, redactor), WITHHELD_TEXT); + assert.equal(redactedData(undefined, redactor), undefined); + } +}); + +test("an error built from a server's words and data reads a configuration that can't be read once, in either engine", async (t) => { + t.mock.method(console, 'error', () => {}); + let reads = 0; + let readable = true; + // An `env` getter that counts its reads and throws while `readable` is + // false, as the framework's reads the awareness ledger. + const counted = (base: T, env: Record): T => Object.defineProperty(base, 'env', { + enumerable: true, + configurable: true, + get() { reads++; if (!readable) throw new Error('the ledger cannot be read'); return env; }, + }); + const modern = ModernMcpConnection.create(counted({ id: 'remote', url: 'https://h/mcp' }, { API_TOKEN: 'envSecret0123456789' })) as unknown as { + requestFailure(what: string, record: unknown, error: unknown): Error; + versionVerdict(error: unknown): McplProtocolVersionError | null; + }; + readable = false; + let before = reads; + const failure = modern.requestFailure('tools/call', { method: 'tools/call', handedOff: true, answer: 'error', error: { code: -32000, message: 'refused', data: { a: 'x', b: ['y', 'z'] } }, continued: false }, new Error('answered')); + assert.equal(reads - before, 1, 'the modern error answer'); + assert.equal((failure as McplRequestError).data, WITHHELD_TEXT); + before = reads; + modern.versionVerdict(new UnsupportedProtocolVersionError({ supported: ['2024-11-05', 'x'], requested: '2026-07-28' })); + assert.equal(reads - before, 1, 'the modern version verdict'); + // The legacy refusal of the protocol revision, which a stdio server sends + // once it's launched: the launch reads the configuration as connect() + // starts, and it turns unreadable after that. + readable = true; + const launched = quotingServer('version-error', QUOTED[0]); + const connecting = McplServerConnection.connect(counted({ ...launched }, launched.env as Record), { version: '0.5' }); + readable = false; + before = reads; + await assert.rejects(connecting, McplProtocolVersionError); + assert.equal(reads - before, 1, 'the legacy version refusal'); + // A stdio child that can't be started, refused on a tick after the launch + // has read the configuration. + readable = true; + const starting = McplServerConnection.connect(counted({ id: 'missing', command: '/nonexistent/mcpl-server-that-is-not-here' }, { API_TOKEN: 'envSecret0123456789' }), { version: '0.5' }); + readable = false; + before = reads; + await assert.rejects(starting, /could not be started/); + assert.equal(reads - before, 1, 'the launch that failed'); +}); + +test("a library's error is handed on as its name and redacted words alone, and an unreaped launch's error doesn't print its connection", () => { + const redact = serverTextRedactor(config); + const raw = Object.assign(new TypeError(`fetch failed for /api/mcp/s/${secret}/mcp`), { path: secret }, { cause: new Error(secret) }); + const masked = maskedError(raw, redact); + assert.equal(masked.name, 'TypeError'); + assert.equal(masked.message, 'fetch failed for /api/mcp/s/redacted/mcp'); + assert.deepEqual(Object.keys(masked), []); + assert.equal(masked.cause, undefined); + // What a library threw that isn't an Error: its text, as a plain Error's words. + const fromText = maskedError(`refused at /api/mcp/s/${secret}/mcp`, redact); + assert.equal(fromText.name, 'Error'); + assert.equal(fromText.message, 'refused at /api/mcp/s/redacted/mcp'); + // The connection stays reachable for the reap, and printing the error leaves it out. + const held = { close: async () => {}, config }; + const unreaped = new McplUnreapedLaunchError(held, new Error('connect refused'), new Error('reap failed')); + assert.equal(unreaped.connection, held); + assert.ok(!Object.keys(unreaped).includes('connection')); + assert.ok(!inspect(unreaped).includes(config.token), inspect(unreaped)); +}); diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index cc160ccf9..c67089f69 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -141,15 +141,16 @@ test("a server's error answer is shown as the server's, and its method, id and c assert.ok(logged(errors.mock.calls, /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: 42$/), 'the log has the number'); }); -test("a server's error answer that echoes a credential reaches the caller as sent, and the log redacted", async (t) => { +test("a server's error answer that echoes a credential reaches the caller and the log redacted", async (t) => { const errors = t.mock.method(console, 'error', () => {}); const token = 'tok-0123456789abcdef'; const connection = await McplServerConnection.connect(config({ env: { API_TOKEN: token } }), HOST_CAPS); t.after(() => connection.close()); connection.ready(); - // A JSON-RPC answer passes as it is (redactServerText's scope). + // The server's words are in an error the caller gets, so a credential + // its configuration holds leaves them (redactServerText's scope). await assert.rejects(connection.sendToolsCall('echo-token', {}), - { message: `The "words" server answered with an error: bad token ${token}` }); + { message: 'The "words" server answered with an error: bad token redacted' }); // The log is a record of the server's text, so the credential leaves it, // in a message of any JSON value. await assert.rejects(connection.sendToolsCall('echo-token-object', {}), { message: NO_WORDS }); @@ -159,7 +160,7 @@ test("a server's error answer that echoes a credential reaches the caller as sen assert.ok(!lines.some((line) => line.includes(token)), 'no log line has the token'); }); -test("an error answer whose configuration can't be read settles its call, and its log line withholds the server's text", async (t) => { +test("an error answer whose configuration can't be read settles its call, and its error and its log line withhold the server's text", async (t) => { const errors = t.mock.method(console, 'error', () => {}); // A stdio server's env, as the framework gives it, is a getter, which // throws once the reaction baseline's ledger can't be read. @@ -174,7 +175,7 @@ test("an error answer whose configuration can't be read settles its call, and it connection.ready(); unreadable = true; await within(assert.rejects(connection.sendToolsCall('refuse', {}), - { message: 'The "words" server answered with an error: Missing Access' }), 'settled call'); + { message: `The "words" server answered with an error: ${WITHHELD_TEXT}` }), 'settled call'); const lines = errors.mock.calls.map((call) => String(call.arguments[0])); assert.ok(!lines.some((line) => line.includes('Missing')), lines.join('\n')); assert.ok(lines.some((line) => /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: /.test(line) && line.endsWith(logValue(WITHHELD_TEXT))), lines.join('\n')); @@ -226,7 +227,7 @@ test("a request refused as not sent is refused through its promise when the host await assert.rejects(refused!, { outcome: 'not-sent' }); }); -test("error answers' log lines read their connection's configuration until a read succeeds, and not after (#540's review, R1-2)", async (t) => { +test("error answers read their connection's configuration once each until a read succeeds, and not after (#540's review, R1-2)", async (t) => { const errors = t.mock.method(console, 'error', () => {}); // A stdio server's env, as the framework gives it, is a getter, which reads // the reaction baseline's ledger from disk each time, and throws while the @@ -241,14 +242,14 @@ test("error answers' log lines read their connection's configuration until a rea const connection = await McplServerConnection.connect(counted, HOST_CAPS); t.after(() => { unreadable = false; return connection.close(); }); connection.ready(); - const refused = () => assert.rejects(connection.sendToolsCall('refuse', {}), { message: 'The "words" server answered with an error: Missing Access' }); + const refused = (words = 'Missing Access') => assert.rejects(connection.sendToolsCall('refuse', {}), { message: `The "words" server answered with an error: ${words}` }); const answerLines = () => errors.mock.calls.map((call) => String(call.arguments[0])).filter((line) => / answered tools\/call /.test(line)); - // While the ledger can't be read, each answer's line is withheld, and the - // next answer reads it again. + // While the ledger can't be read, each answer's words and line are + // withheld, each answer reads it once, and the next answer reads it again. unreadable = true; let before = reads; - await refused(); - await refused(); + await refused(WITHHELD_TEXT); + await refused(WITHHELD_TEXT); assert.equal(reads - before, 2); assert.ok(answerLines().every((line) => line.endsWith(logValue(WITHHELD_TEXT))), answerLines().join('\n')); // Once a read succeeds, the redaction it built is kept. diff --git a/test/mcpl-stderr-tail.test.ts b/test/mcpl-stderr-tail.test.ts index 94d6d56b9..bfbb7ecbb 100644 --- a/test/mcpl-stderr-tail.test.ts +++ b/test/mcpl-stderr-tail.test.ts @@ -189,16 +189,17 @@ test("a connect and a reconnect that fail their handshakes report the server's s connection.ready(); const failure = 'MCPL server "needs-token" transport closed before handshake completed (child process exited)'; // The log lines are records, so the server's credentials are redacted - // there; the event carries the tail as the server wrote it. + // there, and the event carries the failed connect's tail, redacted as the + // host gets it on the error. const stderr = '. The last of its stderr:\n stderr: "fatal: DISCORD_TOKEN redacted was refused"'; const first = await within(connectFailed, null); assert.ok(first, 'the first connect failed and was reported within 15 s'); - assert.deepEqual(first, { error: failure, stderrTail: 'fatal: DISCORD_TOKEN tok-0123456789abcdef was refused', attempt: 0 }); + assert.deepEqual(first, { error: failure, stderrTail: 'fatal: DISCORD_TOKEN redacted was refused', attempt: 0 }); assert.ok(logged.includes(`MCPL server "needs-token" initial connect failed, will retry: ${failure}${stderr}`), logged.join('\n')); const event = await within(reconnectFailed, null); assert.ok(event, 'a reconnect attempt failed within 15 s'); assert.equal(event.error, failure); - assert.equal(event.stderrTail, 'fatal: DISCORD_TOKEN tok-0123456789abcdef was refused'); + assert.equal(event.stderrTail, 'fatal: DISCORD_TOKEN redacted was refused'); assert.ok(logged.includes(`MCPL server "needs-token" reconnect failed: ${failure}${stderr}`), logged.join('\n')); }); diff --git a/test/mcpl-transport-close.test.ts b/test/mcpl-transport-close.test.ts index 3d0b8aaa9..e7ec70b9a 100644 --- a/test/mcpl-transport-close.test.ts +++ b/test/mcpl-transport-close.test.ts @@ -215,7 +215,10 @@ for (const unreadable of [false, true]) test(`a failed handshake's transport err // The frame's error text, "invalid opcode 3", quotes this token. const config: McplServerConfig = { id: 'malformed', url: `ws://127.0.0.1:${port}`, token: 'invalid opcode' }; if (unreadable) Object.defineProperty(config, 'env', { enumerable: true, get() { throw new Error('this configuration cannot be read'); } }); - await assert.rejects(McplServerConnection.connect(config, { version: '0.5' }), /initialize error: refused by the test server/); + await assert.rejects(McplServerConnection.connect(config, { version: '0.5' }), (error: Error) => { + assert.equal(error.message, `MCPL server "malformed" initialize error: ${unreadable ? WITHHELD : 'refused by the test server'}`); + return true; + }); await waitFor(() => logged.some((line) => line.includes('after the handshake settled')), 'the frame\'s error to be logged'); const text = unreadable ? WITHHELD : 'Invalid WebSocket frame: redacted 3'; assert.ok( From 0ec0935600c133b5b7aa05bb923a04383b2ef45b Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Mon, 19 Oct 2026 10:18:43 -0400 Subject: [PATCH 2/3] docs(mcpl): say what a launch failure's cause keeps, and pin it README, the scope comment and the changelog fragment said a library's error is handed on as its name and redacted words alone, but a stdio launch failure's cause also keeps Node's code, and its words stop before Node's quote of a value it refused at once. They now say so. The first commit's message calls that code an errno code, which it is for a failure Node reports on a tick after, such as ENOENT; for a value Node refuses at once it's Node's own, ERR_INVALID_ARG_VALUE. The stderr tail's 14,336-character bound read as the tail's length; it bounds the credentials replaced whole, and now sits beside them. README's list of what the redactor doesn't derive names a stdio server's command as well as its arguments. The comment on the launch's env read says both startup and a reconnect act on the framework's own error. The launch test pins the cause's name for a value Node refuses at once (TypeError), and that a failure Node reports on a tick after keeps its words whole, redacted, where they hold " Received". Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- .../host-errors-carry-no-credentials.fixed.md | 16 ++++++++-------- src/mcpl/protocol-family.ts | 9 +++++---- src/mcpl/transport.ts | 5 +++-- test/mcp-recorded-text.test.ts | 11 ++++++++++- 5 files changed, 27 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 0a584344d..4374b569b 100644 --- a/README.md +++ b/README.md @@ -362,7 +362,7 @@ revisions the server says it supports ("none listed" for an empty list), or its and when the list includes `2026-07-28` it says to configure the server with `protocol: 'modern'` (stdio) or reach it over http(s). -Server and transport text the framework records has the server's configured credentials replaced, in both engines. That covers connection traces, console lines, stderr traces, ops alerts, and a tool call's failure as its caller, the model and the `tool:failed` trace get it. The credentials are those the configuration holds and those its dials sent: from the url as each dial sends it (a WebSocket dial sets the token in the query, which writes the whole query again, form-encoded, and an access provider's token can be in it), credential-like path segments and query names, credential-shaped or credential-named query values, and a WebSocket url's userinfo, in their encoded and decoded forms, replaced wherever they occur; every non-empty query parameter as its whole `name=value`, where no letter or digit adjoins it; the static `token`, and each token an access provider handed a dial, the newest sixteen, raw and url-encoded; and a stdio server's `env` values whose names have a part ending in a credential word, such as `GITHUB_TOKEN`, `PGPASSWORD` or `SESSION_COOKIE`, raw and url-encoded. A stdio server's command-line arguments aren't derived. Standalone strings shorter than eight characters stay unchanged because they may be ordinary words; short query values are still removed with their parameter names. Treat a submitted configuration as immutable while the server uses it: apply changes with `restartMcplServer(id, newConfig)`. Secrets neither the configuration holds nor a dial sent remain outside this filtering. A JSON-RPC tool answer's result passes through as sent. No error a host's own call gets, whether thrown, rejected with or in an error answer's data, carries a credential this filtering replaces, whoever wrote its words: a server's words are redacted, then bounded, where an error is built from them, the strings in an error answer's data are redacted, and an error a library raised, such as a failed modern connect's or a stdio child's that couldn't be started, is handed on as its name and its redacted words alone. With a configuration that can't be read, the server's words and data are withheld. A failed handshake's `stderrTail` is the end of the server's stderr with every credential that reaches into it replaced whole, up to 14,336 characters long. A number in an error answer's data is matched as JavaScript reads it, so one with more significant digits than a double holds has lost its last digits before it's matched. A close's or a cleanup's failure carries the transports' own words, a child's pid, its server's id and the wait it was given. A connection's events are inputs to the framework's records, which redact them. +Server and transport text the framework records has the server's configured credentials replaced, in both engines. That covers connection traces, console lines, stderr traces, ops alerts, and a tool call's failure as its caller, the model and the `tool:failed` trace get it. The credentials are those the configuration holds and those its dials sent: from the url as each dial sends it (a WebSocket dial sets the token in the query, which writes the whole query again, form-encoded, and an access provider's token can be in it), credential-like path segments and query names, credential-shaped or credential-named query values, and a WebSocket url's userinfo, in their encoded and decoded forms, replaced wherever they occur; every non-empty query parameter as its whole `name=value`, where no letter or digit adjoins it; the static `token`, and each token an access provider handed a dial, the newest sixteen, raw and url-encoded; and a stdio server's `env` values whose names have a part ending in a credential word, such as `GITHUB_TOKEN`, `PGPASSWORD` or `SESSION_COOKIE`, raw and url-encoded. A stdio server's command and arguments aren't derived. Standalone strings shorter than eight characters stay unchanged because they may be ordinary words; short query values are still removed with their parameter names. Treat a submitted configuration as immutable while the server uses it: apply changes with `restartMcplServer(id, newConfig)`. Secrets neither the configuration holds nor a dial sent remain outside this filtering. A JSON-RPC tool answer's result passes through as sent. No error a host's own call gets, whether thrown, rejected with or in an error answer's data, carries a credential this filtering replaces, whoever wrote its words: a server's words are redacted, then bounded, where an error is built from them, the strings in an error answer's data are redacted, and an error a library raised, such as a failed modern connect's, is handed on as its name and its redacted words alone. A stdio child that couldn't be started is refused with Node's words redacted, stopping before Node's quote of a value it refused at once, and a cause that keeps Node's error's name, those words and its `code`. With a configuration that can't be read, the server's words and data are withheld. A failed handshake's `stderrTail` is the end of the server's stderr, with every credential of up to 14,336 characters that reaches into it replaced whole. A number in an error answer's data is matched as JavaScript reads it, so one with more significant digits than a double holds has lost its last digits before it's matched. A close's or a cleanup's failure carries the transports' own words, a child's pid, its server's id and the wait it was given. A connection's events are inputs to the framework's records, which redact them. #### Feature sets diff --git a/changelog.d/host-errors-carry-no-credentials.fixed.md b/changelog.d/host-errors-carry-no-credentials.fixed.md index f4c202f2e..e2669183e 100644 --- a/changelog.d/host-errors-carry-no-credentials.fixed.md +++ b/changelog.d/host-errors-carry-no-credentials.fixed.md @@ -30,14 +30,14 @@ ones before that. A redacted key that comes out the same as another key is numbered (`redacted (2)`), and data nested past 64 levels reads `[withheld: nested too deep to redact]`. A failed handshake's `stderrTail` - is the same end of the server's stderr, with every credential that reaches - into it, up to 14,336 characters long, replaced whole. A library's error - is handed on as its name and its redacted words alone. A legacy stdio - server that couldn't be started is refused with its words redacted and a - `cause` that keeps Node's error's name, its redacted words and its errno - `code`, and nothing else of it; when Node refuses an argument or an env - value at once, the words leave the value out. - `McplUnreapedLaunchError.connection` isn't enumerable. With a + is the same end of the server's stderr, with every credential of up to + 14,336 characters that reaches into it replaced whole. A library's error + is handed on as its name and its redacted words alone, with a launch + failure's `code`: a legacy stdio server that couldn't be started is + refused with its words redacted and a `cause` that keeps Node's error's + name, those words and its `code`, and nothing else of it; when Node + refuses an argument or an env value at once, the words leave the value + out. `McplUnreapedLaunchError.connection` isn't enumerable. With a configuration that can't be read, the server's words are withheld, as the records already withhold them, and an error answer's data is withheld whole. diff --git a/src/mcpl/protocol-family.ts b/src/mcpl/protocol-family.ts index 78babeeec..066f1248e 100644 --- a/src/mcpl/protocol-family.ts +++ b/src/mcpl/protocol-family.ts @@ -132,10 +132,11 @@ export function redactUrl(url: string | undefined): string { * whoever wrote the words: a server's words are redacted, then bounded, * where an error is built from them, the strings in an error answer's data * are redacted ({@link redactedData}), and an error a library raised is - * handed on as its name and its redacted words alone ({@link maskedError}). - * A failed handshake's stderr tail is redacted from a longer window of the - * stream, so a credential of up to 14,336 characters that reaches into it - * is replaced whole (McplTransport.stderrTailRedacted). A close's or a + * handed on as its name and its redacted words alone ({@link maskedError}), + * with a stdio launch failure's code. A failed handshake's stderr tail + * is redacted from a longer window of the stream, so a credential of up to + * 14,336 characters that reaches into it is replaced whole + * (McplTransport.stderrTailRedacted). A close's or a * cleanup's failure carries the transports' own words, a child's pid, its * server's id and the wait it was given, and passes as built. A * connection's events are the framework's records' inputs, and the records diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 0be11b628..cfd6893c2 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -413,7 +413,7 @@ export class StdioTransport extends McplTransport { // Node's error for a launch that failed reaches the host, and its words // and its other properties (`spawnargs`, `path`) can repeat a credential // the configuration holds. So it's handed on as its name and redacted - // words (maskedError), with its errno code, which says what failed. What + // words (maskedError), with its code, which says what failed. What // spawn() refuses at once is one of the configuration's own arguments or // options, which Node quotes after " Received", cut to 128 characters: a // credential across that cut couldn't be matched, so the words stop @@ -428,7 +428,8 @@ export class StdioTransport extends McplTransport { }; // The child's environment is read first, outside that: a configuration // whose `env` can't be read throws the framework's own error, such as - // its awareness ledger's, which its startup acts on. + // its awareness ledger's, which the framework acts on: its startup ends + // on it, and a reconnect reports it. const env = buildChildEnv(config); let child: ChildProcess; try { diff --git a/test/mcp-recorded-text.test.ts b/test/mcp-recorded-text.test.ts index e0a01b331..88ff9061f 100644 --- a/test/mcp-recorded-text.test.ts +++ b/test/mcp-recorded-text.test.ts @@ -815,7 +815,7 @@ test("a launch that wrote nothing to stderr has no stderr tail, even while its c }); }); -test("a stdio server that can't be started is refused without its command line, which can repeat a credential, and with its errno code", async () => { +test("a stdio server that can't be started is refused without its command line, which can repeat a credential, and with its code", async () => { const token = 'envSecret0123456789'; // The command's path and an argument both repeat the credential. const missing = (at: string) => `/nonexistent/${at}/mcpl-server-that-is-not-here`; @@ -828,6 +828,14 @@ test("a stdio server that can't be started is refused without its command line, assert.ok(!inspect(error, { depth: 5, showHidden: true }).includes(token), inspect(error, { depth: 5 })); return true; }); + // A failure Node reports on a tick after keeps its words whole, redacted, + // even where they hold " Received": they quote the command, not a value + // Node refused. + const received = '/nonexistent/x Received y/mcpl-server-that-is-not-here'; + await assert.rejects(McplServerConnection.connect({ id: 'received', command: received, env: { API_TOKEN: token } }, { version: '0.5' }), (error: Error) => { + assert.equal(error.message, `MCPL server "received" could not be started: spawn ${received} ENOENT`); + return true; + }); // An argument or an env value Node refuses at spawn() itself, before any // 'error': Node quotes it after " Received", cut to 128 characters, here // across the credential, so its words stop before the quote. @@ -840,6 +848,7 @@ test("a stdio server that can't be started is refused without its command line, assert.equal(error.message, `MCPL server "${id}" could not be started: ${said}`); const cause = error.cause as NodeJS.ErrnoException; assert.equal(cause.message, said); + assert.equal(cause.name, 'TypeError'); assert.equal(cause.code, 'ERR_INVALID_ARG_VALUE'); assert.ok(!inspect(error, { depth: 5, showHidden: true }).includes(token.slice(0, 8)), inspect(error, { depth: 5 })); return true; From 9dc00cfb0c6aab0a172f93c48335c9c0b33ec0c4 Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Mon, 19 Oct 2026 11:22:16 -0400 Subject: [PATCH 3/3] fix(mcpl): number keys that redact alike in one pass redactedData gave a redacted key that came out the same as another key the first number its name had free, searching from 2 again for each such key. So a server's error data holding many keys that redact alike took time that grows with the square of their count: 16,384 of them took 11.6 s on Linux with Node 24.22.0, and both engines do this before an error reaches its caller. Each name's next number to try is now kept, since every number below it was taken and stays taken, so the names come out the same and the same keys take 25 ms. A test numbers 16,384 keys that a redaction maps to one name and checks the names and that it takes under two seconds. Co-Authored-By: Claude Opus 5.5 --- src/mcpl/protocol-family.ts | 7 ++++++- test/mcp-recorded-text.test.ts | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/mcpl/protocol-family.ts b/src/mcpl/protocol-family.ts index 066f1248e..6bbd3d337 100644 --- a/src/mcpl/protocol-family.ts +++ b/src/mcpl/protocol-family.ts @@ -244,11 +244,16 @@ export function redactedData(value: unknown, redactor: ServerTextRedactor | Reda const entries = Object.entries(item); const names = entries.map(([key]) => redact(key)); const taken = new Set(names.filter((name, n) => name === entries[n][0])); + // Each name's next number to try, so keys that redact alike don't search + // from 2 again: every number below it was taken, and stays taken. + const next = new Map(); return Object.fromEntries(entries.map(([key, entry], n) => { let name = names[n]; if (name !== key) { const base = name; - for (let k = 2; taken.has(name); k++) name = `${base} (${k})`; + let k = next.get(base) ?? 2; + while (taken.has(name)) name = `${base} (${k++})`; + next.set(base, k); taken.add(name); } return [name, walk(entry, depth + 1)]; diff --git a/test/mcp-recorded-text.test.ts b/test/mcp-recorded-text.test.ts index 88ff9061f..bc8006060 100644 --- a/test/mcp-recorded-text.test.ts +++ b/test/mcp-recorded-text.test.ts @@ -918,6 +918,21 @@ test("an error answer's data is redacted through and through: its keys, keeping } }); +test("an error answer's data with many keys that redact alike is numbered in time that grows with the keys, not with their square", () => { + // Every key that starts with "k" redacts to "x" here, so all of them come + // out as one name, as copies of one credential in a server's keys can. + const alike = (text: unknown) => (typeof text === 'string' && text.startsWith('k') ? 'x' : String(text)); + const keys = 16_384; + const data = Object.fromEntries(Array.from({ length: keys }, (_, i) => [`k${i}`, i])); + const started = performance.now(); + const names = Object.keys(redactedData(data, alike) as object); + const ms = performance.now() - started; + assert.deepEqual(names, ['x', ...Array.from({ length: keys - 1 }, (_, i) => `x (${i + 2})`)]); + // Numbering in one pass takes tens of milliseconds for this many keys; + // searching each name's numbers from 2 again takes seconds. + assert.ok(ms < 2_000, `${ms.toFixed(0)} ms`); +}); + test("an error built from a server's words and data reads a configuration that can't be read once, in either engine", async (t) => { t.mock.method(console, 'error', () => {}); let reads = 0;