diff --git a/README.md b/README.md index 7411914dc..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. 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 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 new file mode 100644 index 000000000..e2669183e --- /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 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. +- 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..6bbd3d337 100644 --- a/src/mcpl/protocol-family.ts +++ b/src/mcpl/protocol-family.ts @@ -126,8 +126,21 @@ 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}), + * 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 + * 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 +152,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 +175,108 @@ 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])); + // 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; + 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)]; + })); + }; + 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 +475,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 +491,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 +512,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..cfd6893c2 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,33 @@ 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 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 the framework acts on: its startup ends + // on it, and a reconnect reports it. + 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 +444,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..bc8006060 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,556 @@ 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 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; + }); + // 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. + 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.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; + }); + } +}); + +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 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; + 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(