diff --git a/.changeset/json-error-envelope-for-transport-failures.md b/.changeset/json-error-envelope-for-transport-failures.md new file mode 100644 index 0000000..911128f --- /dev/null +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -0,0 +1,100 @@ +--- +"@call-e/core": minor +"@call-e/cli": patch +--- + +Always emit the documented JSON error envelope, with one sanitization boundary for remote text. + +`runCli` previously rethrew every error that was not an `InvalidArgumentsError`, so any +transport or upstream HTTP failure escaped to `main()` and printed a bare message to stderr +with nothing on stdout. Agent hosts are instructed to treat all command output as JSON, so a +failed `auth login` left them with an empty stdout and no `error.code` to branch on. + +**core** (minor: new public subpath and additive error API) + +- New public subpath `@call-e/core/sanitize`: `stripTerminalControls`, `redactSecrets`, + `safeRemoteString`, `safeRemoteCode`, `publicRemoteError`, `sanitizeRemoteError`. One + implementation for every remote-supplied string. Control sequences are *removed* before + credential detection so a control code cannot split a secret; credential-shaped substrings + are redacted; codes must match `-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}` (numbers only as safe + integers); messages are bounded to 500 characters; `publicRemoteError` is the only shape + remote detail may take (`{ code?, message? }`). +- Control removal is by *sequence*, covering the 7-bit `ESC [` / `ESC ]` forms, the 8-bit + `U+009B` / `U+009D` introducers, and invisible format characters (zero widths, joiners, bidi + controls, soft hyphen, BOM). Safety is checked over two canonicalizations, because a sequence + swallows its final byte and that byte can be chosen from the word being searched for + (`Bearer secret` strips to `Beaer`). Whenever the readings differ at all, the whole + string is redacted. Mixed sequences can require a different interpretation per sequence, + leaving neither global reading with a recognisable sensitive key, so conditioning the + fail-closed path on either reading finding a credential is insufficient. +- Every terminal string control is consumed with its payload — OSC, DCS, SOS, PM and APC, in + both 7-bit and 8-bit forms. OSC ends at BEL or ST; the other four end only at ST. All consume + through end of input when unterminated. Embedded non-terminating ESC sequences remain part + of that payload rather than defeating the outer match. Unicode line/paragraph separators + are removed with other line + controls so they cannot split a credential value. Private, standardized, and + intermediate-byte ECMA-35 escape functions are removed whole, and C0/C1 bytes embedded in a + CSI cannot strand its parameters. Stripping a lone introducer left the payload as text and + split key names apart. +- `@call-e/core/http` adds `TransportError` (`url`, `method`, `timedOut`, `phase`, `code`), + `InvalidResponseError`, and `causeCodeOf`. `McpHttpError` carries `phase` too, so every + transport failure names where it failed. `requestJson` throws `TransportError` when `fetch` + rejects, times out, or the body cannot be read, and `InvalidResponseError` when a 2xx body is + not a JSON object — `JSON.parse` quotes its input in its own message, so letting a native + `SyntaxError` escape would have published remote text as a locally-authored summary. Arrays + are no longer accepted as JSON objects. `HttpStatusError` now records `url` and keeps the + server-controlled HTTP reason phrase out of its locally authored `message`. +- `BrokerLoginError` keeps a terminal broker status/error message out of `Error.message` and + distinguishes a terminal authorization outcome from the overall authorization wait timeout. +- `McpHttpError.message` is always locally authored. The server's JSON-RPC error text is kept + raw in `payload` and, sanitized, in the new `remoteError` field. New fields `transport`, + `timedOut`, `causeCode`. Timeouts, rejected fetches, and body-read failures are + `code: "transport_error"`. Successful statuses must carry a JSON-RPC 2.0 response for the + exact request ID with exactly one well-formed result/error; malformed, stale, wrong-version, + or ambiguous outcomes are typed `invalid_response` errors instead of becoming successes. + +**cli** + +- Every failure leaves through `writeCommandError`. `error.code` comes from a single + exported `ERROR_CODES` table via `classifyError`, which the JSON envelope, stderr, and + telemetry all share; a test asserts the table matches `docs/cli-reference.md` exactly. +- `error.message` and stderr are authored by the CLI and never contain remote text. Remote + detail — HTTP bodies, JSON-RPC errors, `plan_not_ready` clarifying questions — appears only + under `error.remote_error` after sanitization. +- `transport_error` (and `error.transport: true`) is set only from the typed transport + boundary. An unrelated local `TypeError` is `internal_error`, never a network condition. +- Terminal broker outcomes and the overall authorization wait use the CLI-owned + `broker_login_failed` / `broker_login_timeout` codes; service wording is sanitized under + `error.remote_error`, never copied into the trusted summary. +- `error.phase` survives the call-stage wrapper, and a body-phase failure says the request + had already been accepted rather than claiming nothing was received — the difference + decides whether retrying would place a second real call. +- Hostile-input regressions: forged `auth_required`, 20 KB flat and nested bodies, + CR/LF/ANSI content, secret-like fields and secret-like substrings inside messages absent + from stdout and stderr, hostile MCP `tools/list` and `tools/call` errors, a hostile + clarifying question, rejected fetch, timeout, and an unrelated `TypeError`. + +Before, against a broker returning 502: + +```text +Client error '502 Bad Gateway' for url '.../api/v1/openagent-auth/sessions' +``` + +After: + +```json +{ + "ok": false, + "error": { + "code": "broker_unavailable", + "message": "HTTP 502 from https://.../api/v1/openagent-auth/sessions. The CALL-E login service is unavailable. ...", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client. err_type=HTTPStatusError" + } + } +} +``` + +The CLI reference documents every stable envelope field and the complete `error.code` list. diff --git a/packages/cli/README.md b/packages/cli/README.md index d741587..3a82fa3 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -162,9 +162,15 @@ If only the initial status query fails, the command still returns the accepted `run_id` and a `call status` `next_argv` array. Use the same launcher for that status request. -Successful command stdout is JSON except help and version output. Some -top-level or local failures may print plain stderr. Access tokens are read from -the local cache and are never printed. + +Command stdout is JSON except help and version output, for failures as well as +successes: every error writes a JSON envelope with a CLI-owned `error.code` to +stdout, a CLI-authored one-line summary to stderr, and exits non-zero. +`error.transport: true` marks the only case that is a network condition. Remote +text appears only under `error.remote_error`, sanitized, redacted, and bounded. +The complete code list is in +[Error Envelopes](./docs/cli-reference.md#error-envelopes). Access tokens are +read from the local cache and are never printed. ## Options diff --git a/packages/cli/docs/cli-reference.md b/packages/cli/docs/cli-reference.md index 07b28e6..b99f55d 100644 --- a/packages/cli/docs/cli-reference.md +++ b/packages/cli/docs/cli-reference.md @@ -4,8 +4,10 @@ This is the canonical reference for `calle` commands, options, defaults, and parameter examples. When changing CLI commands or options, update this document and any synchronized command guidance in the same change. -Successful command stdout is JSON except `--help`, `-h`, `--version`, and `-V`. -Some top-level or local failures may print plain stderr. +Command stdout is JSON except `--help`, `-h`, `--version`, and `-V`. This holds +for failures too: every error leaves through the same JSON envelope on stdout, +with a one-line summary on stderr and a non-zero exit code. See +[Error Envelopes](#error-envelopes). ## Selecting the CLI Entry Point @@ -121,6 +123,95 @@ than the latest call state. See the [MCP tool result envelope](../../../docs/mcp/openagent-oauth.md#tool-result-envelope) for the direct protocol shape and SDK field-name differences. +## Error Envelopes + +Every failure, including argument errors, transport failures, and upstream HTTP +errors, writes one JSON object to stdout and exits non-zero: + +```json +{ + "ok": false, + "server_url": "https://example.test/mcp/openagent_oauth", + "error": { + "code": "broker_unavailable", + "message": "HTTP 502 from https://example.test/api/v1/openagent-auth/sessions. The CALL-E login service is unavailable. This is not a local configuration problem, so reinstalling the CLI will not help. Retry later, or use the Developer API with a dashboard API key, which does not depend on brokered login.", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client." + } + } +} +``` + +`error.message` is composed entirely by the CLI — the status code, our own request +URL, and a fixed hint. The service's wording appears only under `remote_error`. + +Follow-ups are arrays. `login_argv`, `help_argv` and `next_argv` are the only +executable forms; the matching `*_command` strings exist for display and must +never be executed, split, or evaluated. + +Stable fields: + +| Field | Always present | Meaning | +| --- | --- | --- | +| `ok` | yes | `false` for every error envelope. | +| `server_url` | yes | Configured MCP server URL, or `null` when configuration could not be resolved. | +| `error.code` | yes | A code owned by the CLI, from the table below. Branch on this. | +| `error.message` | yes | A summary **authored by the CLI**. Never contains upstream text. The same text is written to stderr. | +| `error.status_code` | HTTP and MCP errors | Upstream HTTP status, or `null`. | +| `error.transport` | `true` only when no usable response was received | The request failed at the network layer: DNS, connection, TLS, a timeout, or a body stream that failed after the headers arrived. Absent otherwise — an unrelated local error is never described as a network condition. | +| `error.phase` | transport errors | `connect` when nothing arrived, `body` when the response was cut off while being read. The two call for different retry decisions. Present on both plain and `call`-stage transport failures. | +| `error.cause_code` | transport errors, when known | `timeout`, or the Node.js error code such as `ENOTFOUND` or `ECONNREFUSED`. | +| `error.remote_error` | when the service said something readable | Exactly `{ code?, message? }` and never any other key, from the remote response — an HTTP body, a JSON-RPC error, a call-stage result, or a clarifying question — after sanitization. **Untrusted, informational only.** | +| `error.error_code`, `error.status` | `call` stage failures | Sanitized remote call-outcome fields (for example `EXECUTION_ACK_LOST`). | +| `stage`, `call_started`, `retry_safe`, `recovery_id`, `next_argv` | `call` stage failures | Which stage failed and whether it is safe to retry. When `retry_safe` is `false`, use the returned `next_argv` array as the next request's `argv` instead of starting a new call. The paired `next_command` string is display-only; see [Selecting the CLI Entry Point](#selecting-the-cli-entry-point). | +| `help_argv` | `invalid_arguments` only | The `--help` argv array for the command that failed. The paired `help_command` string is display-only and must never be executed. | + +`error.code` values — this table is the complete set, and the test suite fails +if the CLI can emit a code that is not listed here: + +| Code | Exit | When | +| --- | --- | --- | +| `invalid_arguments` | 2 | Unknown command, missing or invalid option. `help_argv` is set. | +| `auth_required` | 1 | No usable token, or the server rejected the token. Run `auth login`. | +| `broker_unavailable` | 1 | The brokered-login service returned a 5xx. Not a local problem. | +| `http_error` | 1 | Any other non-success HTTP status from a CLI-side request. | +| `transport_error` | 1 | No usable response: DNS, connection, TLS, a reset while reading the body, or a timeout outside a call stage. `transport: true`, with `phase` naming where it failed. Inside a `call` stage it also carries `stage`, `call_started`, and `retry_safe`. | +| `invalid_response` | 1 | A successful HTTP or MCP status whose body was not the expected JSON object or a JSON-RPC 2.0 response correlated to the exact request with exactly one valid outcome. The body is remote text, so it appears only under `remote_error`. | +| `broker_login_failed` | 1 | Brokered authorization reached a terminal failed/expired/exchanged state. Sanitized service detail is under `remote_error`. | +| `broker_login_timeout` | 1 | The overall brokered-authorization wait expired while the broker was still pending. This is not a network transport error. | +| `mcp_error` | 1 | The MCP server returned a JSON-RPC error. Its message is under `remote_error`. | +| `plan_not_ready` | 1 | `call start`: the plan needs more information. The clarifying question is under `remote_error.message`. | +| `plan_call_invalid_response` | 1 | `call start`: `plan_call` succeeded but returned no usable `plan_id` / `confirm_token`. | +| `run_call_missing_run_id` | 1 | `call start` / `call run`: execution may have been accepted without a stable `run_id`; a `recovery_id` and `next_argv` are returned. | +| `recovery_not_found` | 1 | `call recover`: no local recovery record for that id. | +| `recovery_storage_error` | 1 | `call recover`: the local recovery record could not be read or written. | +| `plan_call_error` | 1 | The `plan_call` stage failed with a non-transport error. | +| `plan_call_timeout` | 1 | The `plan_call` stage received no response in time. `transport: true`. | +| `run_call_error` | 1 | The `run_call` stage failed with a non-transport error. | +| `run_call_timeout` | 1 | The `run_call` stage received no response in time. `transport: true`. | +| `get_call_run_error` | 1 | The `get_call_run` stage failed with a non-transport error. | +| `get_call_run_timeout` | 1 | The `get_call_run` stage received no response in time. `transport: true`. | +| `internal_error` | 1 | An unexpected local exception inside the CLI. Not a network condition. | + +`error.code` is never taken from a remote response, and `error.message` never +contains remote text. Remote text — HTTP bodies, broker terminal messages, +JSON-RPC error messages, clarifying questions, call-outcome fields — appears only under +`error.remote_error` (and the sanitized `error_code` / `status` stage fields), +after one shared sanitizer: only `code` and `message` are read, every other +field is dropped unread; terminal control sequences, embedded string-control +payloads, and Unicode line/paragraph separators are removed *before* +credential detection so a control code cannot split a secret into two +innocent-looking halves; credential-shaped substrings are redacted; codes must +match `-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}` (numeric codes only as safe integers) +or are dropped. If terminal-accurate sequence removal and control-byte removal +produce different text, the ambiguous remote message is withheld as `[redacted]`; +mixed sequences therefore cannot evade both global readings. Messages are limited +to 500 characters. Telemetry reports the +same `error.code` as the envelope, and `transport` is a property of the code, so +the two cannot disagree. + ## Finding Command Help Help is available at the root, command-group, and subcommand levels: diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 10bfce7..73fdb96 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -22,7 +22,16 @@ import { CLI_VERSION, resolveRuntimeConfig, } from "./config.js"; -import { ensurePendingLogin, loginWithBroker } from "./broker-client.js"; +import { BrokerLoginError, ensurePendingLogin, loginWithBroker } from "./broker-client.js"; +import { HttpStatusError, InvalidResponseError, TransportError } from "./http.js"; +import { + REMOTE_MESSAGE_LIMIT, + publicRemoteError, + safeRemoteCode, + safeRemoteString, + sanitizeRemoteError, + stripTerminalControls, +} from "./sanitize.js"; import { AuthRequiredError, McpHttpError, @@ -57,8 +66,12 @@ class CallStageError extends McpHttpError { recoveryId = null, nextCommand = null, remoteError = null, + transport = false, + timedOut = false, + phase = null, + cause, }) { - super(message, { code, statusCode }); + super(message, { code, statusCode, transport, timedOut, phase, ...(cause !== undefined ? { cause } : {}) }); this.name = "CallStageError"; this.stage = stage; this.callStarted = callStarted; @@ -860,12 +873,19 @@ function errorPayload(error, config, helpCommand = null) { }; } + const classified = classifyError(error); + if (error instanceof McpHttpError) { - const remoteError = error instanceof CallStageError && error.remoteError - ? error.remoteError - : null; + const stageRemote = error instanceof CallStageError && error.remoteError ? error.remoteError : null; + // Every remote_error goes through publicRemoteError: at most { code, message }, each + // validated. Never `error.payload`, never stage fields. A stage error carries the call + // result's `error_code`/`message`; a plain MCP error carries the core client's copy. + const remoteError = error instanceof CallStageError + ? publicRemoteError(stageRemote ? { code: stageRemote.error_code ?? stageRemote.code, message: stageRemote.message } : null) + : publicRemoteError(error.remoteError); + const causeCode = safeRemoteCode(error.causeCode); return { - exitCode: 1, + exitCode: classified.exitCode, body: { ok: false, server_url: config?.serverUrl ?? null, @@ -877,34 +897,219 @@ function errorPayload(error, config, helpCommand = null) { ...(error.nextCommand ?? {}), } : {}), error: { - code: error.code || "mcp_error", - message: error.message, + code: classified.code, + message: localMessage(error.message), status_code: error.statusCode, - ...(remoteError?.error_code !== undefined ? { error_code: remoteError.error_code } : {}), - ...(remoteError?.status !== undefined ? { status: remoteError.status } : {}), + ...(classified.transport ? { transport: true } : {}), + ...(classified.transport && error.phase ? { phase: error.phase } : {}), + ...(causeCode ? { cause_code: causeCode } : {}), + ...(stageRemote?.error_code !== undefined ? { error_code: stageRemote.error_code } : {}), + ...(stageRemote?.status !== undefined ? { status: stageRemote.status } : {}), + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + + if (error instanceof HttpStatusError) { + const remoteError = publicRemoteError(sanitizeRemoteError(error.responseText)); + const brokerUnavailable = classified.code === "broker_unavailable"; + // The summary is authored here, from the status code and our own URL — never from the + // response, whose status text and body are both remote-controlled. + const summary = `HTTP ${error.statusCode ?? "error"} from ${describeUrl(error.url)}.`; + return { + exitCode: classified.exitCode, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: classified.code, + message: brokerUnavailable ? `${summary} ${BROKER_UNAVAILABLE_HINT}` : summary, + status_code: error.statusCode, + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + + if (error instanceof InvalidResponseError) { + const remoteError = publicRemoteError(sanitizeRemoteError(error.responseText)); + return { + exitCode: classified.exitCode, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: "invalid_response", + // Composed here. The body is remote, and JSON.parse quotes it in its own message. + message: `${describeUrl(error.url)} returned a ${error.statusCode ?? "successful"} response whose body was not the expected JSON.`, + ...(error.statusCode !== null ? { status_code: error.statusCode } : {}), + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + + if (error instanceof TransportError) { + const causeCode = safeRemoteCode(error.code); + // The phase matters to whoever has to decide about retrying: nothing sent is a different + // situation from a request that was accepted and then cut off mid-body. + const where = error.phase === "body" ? "while reading the response body from" : "before a response was received from"; + const summary = error.timedOut + ? `Request timed out ${error.phase === "body" ? "while reading the response body from" : "waiting for"} ${describeUrl(error.url)}.` + : `Request failed ${where} ${describeUrl(error.url)}.`; + return { + exitCode: classified.exitCode, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: "transport_error", + message: causeCode && !error.timedOut ? `${summary} (${causeCode})` : summary, + transport: true, + phase: error.phase, + ...(causeCode ? { cause_code: causeCode } : {}), + }, + }, + }; + } + + if (error instanceof BrokerLoginError) { + const remoteError = publicRemoteError(error.remoteError); + return { + exitCode: classified.exitCode, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: classified.code, + message: localMessage(error.message) ?? "Brokered login failed.", + ...(remoteError ? { remote_error: remoteError } : {}), }, }, }; } return { - exitCode: 1, + exitCode: classified.exitCode, body: { ok: false, server_url: config?.serverUrl ?? null, error: { - code: "mcp_error", - message: error?.message || String(error), + code: classified.code, + message: localMessage(error?.message ?? String(error)) ?? "Unexpected error.", }, }, }; } +const LOCAL_MESSAGE_LIMIT = 300; + +const BROKER_UNAVAILABLE_HINT = + "The CALL-E login service is unavailable. This is not a local configuration problem, so reinstalling the CLI will not help. Retry later, or use the Developer API with a dashboard API key, which does not depend on brokered login."; + +/** + * The complete set of `error.code` values the CLI can emit, with their exit codes. + * + * This object IS the contract. `classifyError` never returns a code outside it, + * `errorTelemetryCode` reports the same value, and the test suite asserts that + * docs/cli-reference.md documents exactly this set — so the three cannot drift apart. + */ +export const ERROR_CODES = Object.freeze({ + invalid_arguments: { exitCode: 2, transport: false }, + auth_required: { exitCode: 1, transport: false }, + broker_unavailable: { exitCode: 1, transport: false }, + http_error: { exitCode: 1, transport: false }, + transport_error: { exitCode: 1, transport: true }, + invalid_response: { exitCode: 1, transport: false }, + broker_login_failed: { exitCode: 1, transport: false }, + broker_login_timeout: { exitCode: 1, transport: false }, + mcp_error: { exitCode: 1, transport: false }, + plan_not_ready: { exitCode: 1, transport: false }, + plan_call_invalid_response: { exitCode: 1, transport: false }, + run_call_missing_run_id: { exitCode: 1, transport: false }, + recovery_not_found: { exitCode: 1, transport: false }, + recovery_storage_error: { exitCode: 1, transport: false }, + plan_call_error: { exitCode: 1, transport: false }, + plan_call_timeout: { exitCode: 1, transport: true }, + run_call_error: { exitCode: 1, transport: false }, + run_call_timeout: { exitCode: 1, transport: true }, + get_call_run_error: { exitCode: 1, transport: false }, + get_call_run_timeout: { exitCode: 1, transport: true }, + internal_error: { exitCode: 1, transport: false }, +}); + +/** + * Single place that maps a thrown error to a CLI-owned code. Used by the JSON envelope, by + * stderr, and by telemetry, so all three always agree. + */ +export function classifyError(error) { + if (error instanceof InvalidArgumentsError) { + return { code: "invalid_arguments", exitCode: 2, transport: false }; + } + if (error instanceof AuthRequiredError || isUnauthorizedMcpError(error)) { + return { code: "auth_required", exitCode: 1, transport: false }; + } + if (error instanceof McpHttpError) { + const candidate = typeof error.code === "string" && Object.hasOwn(ERROR_CODES, error.code) + ? error.code + : "mcp_error"; + // `transport` is a property of the code, read from the table — never inferred from the + // error object — so the envelope can never claim a network condition for a code the + // contract defines as non-transport. + return { code: candidate, exitCode: ERROR_CODES[candidate].exitCode, transport: ERROR_CODES[candidate].transport }; + } + if (error instanceof HttpStatusError) { + const code = isBrokerRegistrationFailure(error) ? "broker_unavailable" : "http_error"; + return { code, exitCode: 1, transport: false }; + } + if (error instanceof TransportError) { + return { code: "transport_error", exitCode: 1, transport: true }; + } + if (error instanceof InvalidResponseError) { + return { code: "invalid_response", exitCode: 1, transport: false }; + } + if (error instanceof BrokerLoginError) { + const code = error.code === "broker_login_timeout" ? error.code : "broker_login_failed"; + return { code, exitCode: 1, transport: false }; + } + // Anything else is a local defect. It is never described as a network condition. + return { code: "internal_error", exitCode: 1, transport: false }; +} + +/** Bound and control-strip a message the CLI authored itself before it reaches an envelope. */ +function localMessage(value) { + return safeRemoteString(value, LOCAL_MESSAGE_LIMIT); +} + +/** Origin and path of a URL we requested — ours to print, but never the query string. */ +function describeUrl(url) { + if (typeof url !== "string" || !url) { + return "the CALL-E service"; + } + try { + const parsed = new URL(url); + return `${parsed.origin}${parsed.pathname}`; + } catch { + return "the CALL-E service"; + } +} + +function isBrokerRegistrationFailure(error) { + if (Number(error?.statusCode) < 500) { + return false; + } + const url = typeof error?.url === "string" ? error.url : String(error?.message ?? ""); + return /\/api\/v1\/openagent-auth\/sessions/u.test(url); +} + function writeCommandError(stdout, stderr, error, config, helpCommand = null) { const formatted = errorPayload(error, config, helpCommand); writeJson(stdout, formatted.body); + // Remote-derived strings are sanitized at the source; this is the last line of defence + // for the one channel that goes straight to a terminal. stderr([ - formatted.body.error.message, + stripTerminalControls(formatted.body.error.message), ...(formatted.body.help_command ? [`Run '${formatted.body.help_command}' for usage.`] : []), ].join("\n")); return formatted.exitCode; @@ -919,16 +1124,7 @@ function prePlanInvokedCommand(group, command) { } function errorTelemetryCode(error) { - if (error instanceof InvalidArgumentsError) { - return "invalid_arguments"; - } - if (error instanceof AuthRequiredError || isUnauthorizedMcpError(error)) { - return "auth_required"; - } - if (error instanceof McpHttpError) { - return error.code || "mcp_error"; - } - return "local_error"; + return classifyError(error).code; } function errorTelemetryProperties(error) { @@ -1023,25 +1219,19 @@ function structuredPayload(result) { return result?.structuredContent || result?.structured_content || result || {}; } -function safeRemoteString(value, maxLength = 1000) { - if (typeof value !== "string" || !value.trim()) { - return undefined; - } - return value.trim().slice(0, maxLength); -} - +// Remote strings are sanitized by @call-e/core/sanitize - one implementation shared with the +// MCP client, so a message is made safe where the error is created, not where it is printed. function safeRemoteCallError(result) { const structured = recordObject(structuredPayload(result)) || {}; const nestedError = recordObject(structured.error) || {}; const field = (name) => nestedError[name] ?? structured[name]; - const errorCodeValue = field("error_code") ?? field("code"); - const errorCode = typeof errorCodeValue === "number" - ? errorCodeValue - : safeRemoteString(errorCodeValue, 200); + // Same validation as any other remote code: safe charset, bounded, safe integers only. + // A number is not automatically trustworthy — 1e100 and NaN are numbers too. + const errorCode = safeRemoteCode(field("error_code") ?? field("code")); const statusValue = field("status"); const status = typeof statusValue === "number" - ? statusValue - : safeRemoteString(statusValue, 200); + ? (Number.isSafeInteger(statusValue) ? statusValue : undefined) + : safeRemoteCode(statusValue); const message = safeRemoteString(field("message")); const retrySafe = typeof field("retry_safe") === "boolean" ? field("retry_safe") : undefined; const callStartedValue = field("call_started"); @@ -1067,23 +1257,45 @@ function callStageErrorFrom(error, { if (error instanceof CallStageError) { return error; } - const timedOut = error instanceof McpHttpError && /timed out/iu.test(error.message); + // Only a genuine transport timeout (typed by the core client) becomes `_timeout`; + // matching on message text would let a remote string choose our error code. + const timedOut = error instanceof McpHttpError && error.timedOut === true; const remoteError = error instanceof McpHttpError && error.payload ? safeRemoteCallError(error.payload) : null; + // The summary is ours. The server's wording, if any, rides along under remote_error. + const transport = error instanceof McpHttpError && error.transport === true; + // A body-phase failure means the request was accepted and the response was cut off while + // being read. Reporting that as "before a response was received" would invite a retry of a + // call that may already have been placed. + const phase = transport ? (error.phase ?? "connect") : null; + const message = timedOut + ? (phase === "body" + ? `${stage} timed out while the response was being read; the request had already been accepted.` + : `${stage} timed out before the CLI received a response.`) + : (transport + ? (phase === "body" + ? `${stage} failed while the response was being read; the request had already been accepted.` + : `${stage} failed before a response was received.`) + : `${stage} failed.`); + // A rejected/reset transport at a stage is `transport_error` (with the stage fields kept), + // so the code and the `transport` flag can never disagree with the documented table. + const code = timedOut ? `${stage}_timeout` : (transport ? "transport_error" : `${stage}_error`); return new CallStageError( - timedOut - ? `${stage} timed out before the CLI received a response.` - : remoteError?.message || `${stage} failed: ${error?.message || String(error)}`, + message, { stage, - code: timedOut ? `${stage}_timeout` : `${stage}_error`, + code, statusCode: error instanceof McpHttpError ? error.statusCode : null, callStarted: remoteError?.call_started ?? callStarted, retrySafe: remoteError?.retry_safe ?? retrySafe, recoveryId, nextCommand, remoteError, + transport, + timedOut, + phase, + ...(error?.cause !== undefined ? { cause: error.cause } : {}), } ); } @@ -1111,7 +1323,8 @@ async function callCallStage({ }); if (result?.isError === true) { const remoteError = safeRemoteCallError(result); - throw new CallStageError(remoteError.message || `${stage} returned an error.`, { + // Fixed local summary. The server's wording is available under error.remote_error. + throw new CallStageError(`${stage} returned an error.`, { stage, code: `${stage}_error`, callStarted: remoteError.call_started ?? callStarted, @@ -1222,7 +1435,8 @@ async function runPlannedCall({ config, deps, planId, confirmToken, timezone = n const runId = extractRunId(runResult); if (!runId) { const remoteError = safeRemoteCallError(runResult); - throw new CallStageError(remoteError.message || "run_call did not return a run_id.", { + // Fixed local summary; the server's wording is under error.remote_error. + throw new CallStageError("run_call did not return a run_id.", { stage: "run_call", code: "run_call_missing_run_id", callStarted: remoteError.call_started ?? "unknown", @@ -1396,16 +1610,20 @@ async function handleCallCommand({ command, positional, options, config, deps, s }); const structuredPlan = structuredPayload(planResult); if (structuredPlan.ready_to_run === false) { - const question = Array.isArray(structuredPlan.clarifying_questions) - ? structuredPlan.clarifying_questions.find((item) => typeof item === "string" && item.trim())?.trim() + // The clarifying question is server text. It is shown under remote_error, sanitized + // and bounded, never interpolated into the CLI's own summary. + const rawQuestion = Array.isArray(structuredPlan.clarifying_questions) + ? structuredPlan.clarifying_questions.find((item) => typeof item === "string" && item.trim()) : null; + const question = safeRemoteString(rawQuestion, REMOTE_MESSAGE_LIMIT); throw new CallStageError( - `Call plan needs more information before it can run${question ? `: ${question}` : "."}`, + "Call plan needs more information before it can run. See error.remote_error.message for the question the service asked.", { stage: "plan_call", code: "plan_not_ready", callStarted: false, retrySafe: true, + ...(question ? { remoteError: { message: question } } : {}), } ); } @@ -1721,10 +1939,6 @@ export async function runCli(argv, deps = {}) { try { return await runCliCommand(argv, deps); } catch (error) { - if (!(error instanceof InvalidArgumentsError)) { - throw error; - } - const stdout = deps.stdout || ((text) => process.stdout.write(text)); const stderr = deps.stderr || ((text) => process.stderr.write(`${text}\n`)); const [group, command, ...rest] = argv; @@ -1735,7 +1949,13 @@ export async function runCli(argv, deps = {}) { } catch { // Invalid option syntax may prevent runtime configuration from being resolved. } - return writeCommandError(stdout, stderr, error, config, helpCommandFor(group, command)); + // Every failure leaves through the documented JSON envelope, not just argument errors. + // Agent hosts are instructed to treat all command output as JSON, so a transport or + // upstream failure that printed a bare string left them with nothing to parse. + const helpCommand = error instanceof InvalidArgumentsError + ? helpCommandFor(group, command) + : null; + return writeCommandError(stdout, stderr, error, config, helpCommand); } } diff --git a/packages/cli/lib/sanitize.js b/packages/cli/lib/sanitize.js new file mode 100644 index 0000000..65e0d9d --- /dev/null +++ b/packages/cli/lib/sanitize.js @@ -0,0 +1 @@ +export * from "@call-e/core/sanitize"; diff --git a/packages/cli/package.json b/packages/cli/package.json index 0e6d5b5..c453bf9 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -45,7 +45,7 @@ "test:e2e": "node --test ./test/e2e/*.test.js", "verify:live": "node ./scripts/live-e2e.mjs", "verify:live:call": "node ./scripts/live-e2e.mjs --call", - "check": "node ../../scripts/check-runtime-syntax.mjs bin/calle.js lib/broker-client.js lib/cache.js lib/cli.js lib/config.js lib/http.js lib/mcp-client.js lib/telemetry.js scripts/live-e2e.mjs scripts/run-agent-command.mjs", + "check": "node ../../scripts/check-runtime-syntax.mjs bin/calle.js lib/broker-client.js lib/cache.js lib/cli.js lib/config.js lib/http.js lib/mcp-client.js lib/sanitize.js lib/telemetry.js scripts/live-e2e.mjs scripts/run-agent-command.mjs", "pack:dry-run": "tmpdir=$(mktemp -d) && trap 'rm -rf \"$tmpdir\"' EXIT && pnpm pack --pack-destination \"$tmpdir\"" } } diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index 06f8401..d46482b 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -370,6 +370,913 @@ test("auth login start-only returns authorization hint without polling", async ( assert.doesNotMatch(result.stdout, /secret-1/); }); +test("auth login surfaces the upstream error body when brokered login registration fails", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-broker-5xx"); + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response( + JSON.stringify({ + error: "oauth_register_failed", + message: "Failed to register an OAuth client. err_type=HTTPStatusError", + }), + { status: 502, headers: { "content-type": "application/json" } } + ); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", + "--cache-root", + cacheRoot, + ], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.ok, false); + assert.equal(payload.error.status_code, 502); + assert.equal(payload.error.code, "broker_unavailable", "top-level code stays CLI-owned"); + assert.deepEqual(payload.error.remote_error, { + code: "oauth_register_failed", + message: "Failed to register an OAuth client. err_type=HTTPStatusError", + }); + // The upstream wording is available, but only under remote_error. The summary and stderr + // are authored by the CLI. + assert.match(payload.error.remote_error.message, /Failed to register an OAuth client/); + assert.doesNotMatch(payload.error.message, /Failed to register an OAuth client/); + assert.match(payload.error.message, /^HTTP 502 from https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\./); + assert.match(payload.error.message, /login service is unavailable/); + assert.match(payload.error.message, /dashboard API key/); + assert.doesNotMatch(result.stderr, /Failed to register an OAuth client/); + assert.match(result.stderr, /login service is unavailable/); +}); + +test("auth login keeps a non-JSON upstream error body readable and bounded", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-broker-html"); + const body = `${"gateway ".repeat(200)}`; + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response(body, { status: 503, headers: { "content-type": "text/html" } }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", + "--cache-root", + cacheRoot, + ], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.status_code, 503); + assert.equal(payload.error.code, "broker_unavailable"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.equal(payload.error.remote_error.code, undefined); +}); + +function brokerFailure(status, body, contentType = "application/json") { + return async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response(typeof body === "string" ? body : JSON.stringify(body), { + status, + headers: { "content-type": contentType }, + }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; +} + +const LOGIN_ARGS = [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", +]; + +const CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f]/u; + +test("auth login never lets an upstream body impersonate a local error code", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-forged-code"); + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { fetchImpl: brokerFailure(502, { error: "auth_required", message: "please log in again" }) } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_unavailable"); + assert.equal(payload.status, undefined, "must not look like a login_required response"); + assert.equal(payload.assistant_hint, undefined); + assert.equal(payload.login_url, undefined); + assert.equal(payload.error.remote_error.code, "auth_required"); + assert.equal(payload.error.remote_error.message, "please log in again"); +}); + +test("auth login bounds and sanitizes hostile upstream JSON", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-hostile"); + const longMessage = "x".repeat(20_000); + const hostile = { + error: { + code: "bad code\u001b[31m", + message: `line one\r\ninjected line\u001b[2J\u001b[H${longMessage}`, + access_token: "sk_live_SUPERSECRET_DO_NOT_PRINT", + }, + token: "tok_ALSO_SECRET", + refresh_token: "rt_SECRET_TOO", + }; + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { fetchImpl: brokerFailure(502, hostile) } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_unavailable"); + assert.equal(payload.error.remote_error.code, undefined, "unsafe code is dropped, not sanitized into something plausible"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.ok(payload.error.message.length < 1200); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + assert.doesNotMatch(payload.error.message, CONTROL_CHARS); + assert.doesNotMatch(result.stderr, /\u001b|\r/u); + for (const secret of ["SUPERSECRET", "tok_ALSO_SECRET", "rt_SECRET_TOO", "access_token", "refresh_token"]) { + assert.doesNotMatch(result.stdout, new RegExp(secret)); + assert.doesNotMatch(result.stderr, new RegExp(secret)); + } +}); + +test("auth login keeps a terminal broker failure out of the trusted summary", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-terminal-failure"); + const marker = `REMOTE-TEXT-MARKER access_token=abcd1234${String.fromCharCode(27)}[31mefgh5678`; + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return jsonResponse({ + session_id: "session-failed", + session_secret: "secret-failed", + login_url: "https://mcp.example/start", + status: "PENDING", + poll_after_ms: 1, + }); + } + if (String(url).endsWith("/api/v1/openagent-auth/sessions/session-failed") && init?.method === "GET") { + return jsonResponse({ status: "FAILED", error_message: marker }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + ["auth", "login", "--no-browser-open", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl }, + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_login_failed"); + assert.equal(payload.error.message, "Brokered login failed."); + assert.equal(payload.error.remote_error.code, "FAILED"); + assert.equal(payload.error.remote_error.message, "[redacted]"); + assert.doesNotMatch(payload.error.message, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + assert.doesNotMatch(result.stderr, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); +}); + +test("auth login reports the overall authorization wait as a non-transport timeout", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-wait-timeout"); + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return jsonResponse({ + session_id: "session-pending", + session_secret: "secret-pending", + login_url: "https://mcp.example/start", + status: "PENDING", + poll_after_ms: 1, + }); + } + if (String(url).endsWith("/api/v1/openagent-auth/sessions/session-pending") && init?.method === "GET") { + return jsonResponse({ status: "PENDING", poll_after_ms: 1 }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + [ + "auth", "login", "--no-browser-open", "--poll-timeout-seconds", "0.0001", + "--base-url", "https://mcp.example", "--cache-root", cacheRoot, + ], + { fetchImpl, sleepImpl: async () => {} }, + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_login_timeout"); + assert.equal(payload.error.message, "Timed out waiting for brokered login authorization."); + assert.equal(payload.error.transport, undefined, "the broker kept answering; this is not a network failure"); + assert.equal(payload.error.remote_error, undefined); +}); + +test("auth login reads a nested upstream error object and drops everything else", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-nested"); + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { + fetchImpl: brokerFailure(500, { + error: { code: "nested.code-1", message: "nested message", details: { internal: "trace-abc" } }, + request_id: "req_123", + }), + } + ); + const payload = JSON.parse(result.stdout); + + assert.deepEqual(payload.error.remote_error, { code: "nested.code-1", message: "nested message" }); + assert.doesNotMatch(result.stdout, /trace-abc|req_123|details|request_id/u); +}); + +test("auth login returns a transport_error envelope when fetch rejects", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-fetch-rejected"); + const fetchImpl = async () => { + const error = new TypeError("fetch failed"); + error.cause = { code: "ENOTFOUND", syscall: "getaddrinfo", hostname: "mcp.example" }; + throw error; + }; + const result = await run([...LOGIN_ARGS, "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.ok, false); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.transport, true); + assert.equal(payload.error.cause_code, "ENOTFOUND"); + assert.equal(payload.help_command, undefined); + // Locally authored: names our request and the system error code, never the runtime's text. + assert.equal(payload.error.phase, "connect", "nothing arrived at all"); + assert.match( + payload.error.message, + /^Request failed before a response was received from https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\. \(ENOTFOUND\)$/u + ); + assert.ok(result.stderr.length < 500); + // The test harness terminates each stderr write with a newline; everything else must be clean. + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); +}); + +test("auth login classifies a request timeout as a transport_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-timeout"); + const fetchImpl = async () => { + const aborted = new Error("aborted"); + aborted.name = "AbortError"; + throw aborted; + }; + const result = await run([...LOGIN_ARGS, "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.transport, true); + assert.equal(payload.error.cause_code, "timeout"); + assert.match(payload.error.message, /^Request timed out waiting for https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\./u); +}); + +test("an unrelated local TypeError is internal_error, never transport_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-local-typeerror"); + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return jsonResponse( + { + session_id: "session-1", + session_secret: "secret-1", + login_url: "https://mcp.example/openagent-auth/sessions/session-1/start", + status: "PENDING", + poll_after_ms: 1, + expires_at: "2030-01-01T00:00:00Z", + }, + { status: 201 } + ); + } + if (String(url).endsWith("/api/v1/openagent-auth/sessions/session-1") && init?.method === "GET") { + return jsonResponse({ status: "PENDING", expires_at: "2030-01-01T00:00:00Z" }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + // A bug inside the CLI's own polling loop, not a network condition. + const sleepImpl = async () => { + throw new TypeError("Cannot read properties of undefined (reading 'x')"); + }; + + const result = await run( + ["auth", "login", "--no-browser-open", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl, sleepImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "internal_error"); + assert.equal(payload.error.transport, undefined); + assert.equal(payload.error.cause_code, undefined); + assert.match(payload.error.message, /Cannot read properties/u); +}); + +function mcpFixture({ serverUrl, onToolsList, onToolsCall }) { + return async (url, init) => { + assert.equal(String(url), serverUrl); + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonRpcResponse({ jsonrpc: "2.0", id: payload.id, result: {} }, { headers: { "mcp-session-id": "sess-h" } }); + } + if (payload.method === "notifications/initialized") { + return jsonRpcResponse({}); + } + if (payload.method === "tools/list" && onToolsList) { + return onToolsList(payload); + } + if (payload.method === "tools/call" && onToolsCall) { + return onToolsCall(payload); + } + throw new Error(`unexpected method: ${payload.method}`); + }; +} + +const ESC_CHAR = String.fromCharCode(27); +const HOSTILE_REMOTE_TEXT = + `line one\r\ninjected${ESC_CHAR}[2J${ESC_CHAR}[H bearer abcdefghijklmnopqrstuvwxyz0123 ` + + `access_token=sk_live_ABCDEFGHIJKLMNOPQRST ${"z".repeat(20_000)}`; + +test("mcp tools keeps a hostile JSON-RPC error out of the summary and bounds it under remote_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-tools-hostile"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsList: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + error: { code: -32000, message: HOSTILE_REMOTE_TEXT, data: { refresh_token: "rt_SECRET_ABCDEFGH" } }, + }), + }); + + const result = await run(["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "mcp_error"); + assert.equal(payload.error.message, "Remote MCP error for tools/list"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.equal(payload.error.remote_error.code, "-32000"); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + for (const secret of ["sk_live_", "abcdefghijklmnopqrstuvwxyz0123", "rt_SECRET", "refresh_token"]) { + assert.doesNotMatch(result.stdout, new RegExp(secret)); + assert.doesNotMatch(result.stderr, new RegExp(secret)); + } + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); + assert.ok(result.stderr.length < 300); +}); + +test("mcp tools withholds mixed terminal-sequence text from every public error surface", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-tools-mixed-controls"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const CSI8 = String.fromCharCode(0x9b); + const ST8 = String.fromCharCode(0x9c); + const OSC8 = String.fromCharCode(0x9d); + const DCS8 = String.fromCharCode(0x90); + const mixedMessage = + `access_to${OSC8}junk${ST8}k${CSI8}en=abcd1234efgh5678 ` + + `access_to${DCS8}junk${ST8}k${CSI8}en=ijkl9012mnop3456`; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsList: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + error: { code: -32000, message: mixedMessage }, + }), + }); + + const result = await run( + ["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl }, + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "mcp_error"); + assert.equal(payload.error.remote_error.message, "[redacted]"); + for (const fragment of ["abcd1234", "efgh5678", "ijkl9012", "mnop3456"]) { + assert.equal(result.stdout.includes(fragment), false); + assert.equal(result.stderr.includes(fragment), false); + } + for (const sequenceByte of [CSI8, ST8, OSC8, DCS8]) { + assert.equal(result.stdout.includes(sequenceByte), false); + assert.equal(result.stderr.includes(sequenceByte), false); + } + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); +}); + +test("mcp tools rejects a successful status with a malformed JSON-RPC body", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-invalid-response"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const marker = "REMOTE-TEXT-MARKER access_token=abcd1234efgh5678"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsList: () => new Response(marker, { status: 200, headers: { "content-type": "text/plain" } }), + }); + + const result = await run( + ["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl }, + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "invalid_response"); + assert.equal(payload.error.message, "MCP response was invalid for tools/list"); + assert.equal(payload.error.status_code, 200); + assert.doesNotMatch(payload.error.message, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + assert.doesNotMatch(result.stderr, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + assert.match(payload.error.remote_error.message, /\[redacted\]/u); +}); + +test("mcp call applies the same boundary to a tool-call error", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-call-hostile"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsCall: (payload) => jsonRpcResponse({ jsonrpc: "2.0", id: payload.id, error: { code: -32601, message: HOSTILE_REMOTE_TEXT } }), + }); + + const result = await run( + ["mcp", "call", "get_call_run", "--args-json", '{"run_id":"run_1"}', "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "mcp_error"); + assert.equal(payload.error.message, "Remote MCP error for tools/call"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.doesNotMatch(result.stdout, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); +}); + +test("call start keeps a hostile clarifying question out of the plan_not_ready summary", async () => { + const cacheRoot = makeTempRoot("calle-cli-call-start-hostile-question"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsCall: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + result: { structuredContent: { ready_to_run: false, clarifying_questions: [HOSTILE_REMOTE_TEXT] } }, + }), + }); + + const result = await run( + ["call", "start", "--to-phone", "+15551234567", "--goal", "Confirm appointment", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.stage, "plan_call"); + assert.equal(payload.error.code, "plan_not_ready"); + assert.match(payload.error.message, /^Call plan needs more information before it can run\./u); + assert.doesNotMatch(payload.error.message, /injected|zzzz/u); + assert.ok(payload.error.remote_error.message.length <= 500); + // The question carries both control sequences and credentials, so the readings disagree and + // the whole string is withheld rather than partially shown. + assert.equal(payload.error.remote_error.message, "[redacted]"); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + assert.doesNotMatch(result.stdout, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr, /injected|sk_live_/u); +}); + +test("telemetry reports the same error code as the envelope for broker and transport failures", async () => { + const brokerRoot = makeTempRoot("calle-cli-telemetry-broker"); + const brokerEvents = []; + await run( + [...LOGIN_ARGS, "--cache-root", brokerRoot], + { + fetchImpl: brokerFailure(502, { error: "oauth_register_failed", message: "x" }), + env: { CALLE_TELEMETRY: "1" }, + telemetryFetchImpl: captureTelemetry(brokerEvents), + } + ); + const brokerFailed = brokerEvents.find((event) => event.payload.event === "auth_login_local_failed"); + assert.ok(brokerFailed, "auth_login_local_failed telemetry was emitted"); + assert.equal(brokerFailed.payload.properties.error_code, "broker_unavailable"); + + const mcpRoot = makeTempRoot("calle-cli-telemetry-transport"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(mcpRoot, serverUrl, "tool-token"); + const mcpEvents = []; + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ECONNREFUSED" }; + const result = await run( + ["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", mcpRoot], + { fetchImpl: async () => { throw dns; }, env: { CALLE_TELEMETRY: "1" }, telemetryFetchImpl: captureTelemetry(mcpEvents) } + ); + const payload = JSON.parse(result.stdout); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.cause_code, "ECONNREFUSED"); + const checked = mcpEvents.find((event) => event.payload.event === "mcp_tools_checked" && event.payload.properties.outcome === "failure"); + assert.ok(checked, "mcp_tools_checked failure telemetry was emitted"); + assert.equal(checked.payload.properties.error_code, "transport_error"); +}); + +test("every error code the CLI can emit is documented, and nothing undocumented is emitted", async () => { + const { ERROR_CODES } = await import("../lib/cli.js"); + const reference = fs.readFileSync(new URL("../docs/cli-reference.md", import.meta.url), "utf8"); + const section = reference.split("## Error Envelopes")[1]?.split(/\n## /u)[0] ?? ""; + const documented = new Set([...section.matchAll(/^\| `([a-z_]+)` \| \d /gmu)].map((m) => m[1])); + const emitted = new Set(Object.keys(ERROR_CODES)); + + assert.deepEqual([...documented].sort(), [...emitted].sort()); + for (const [code, meta] of Object.entries(ERROR_CODES)) { + assert.match(section, new RegExp(`^\\| \`${code}\` \\| ${meta.exitCode} `, "mu"), `exit code documented for ${code}`); + } +}); + +test("the error-envelope docs never tell an agent to execute a command string", () => { + const reference = fs.readFileSync(new URL("../docs/cli-reference.md", import.meta.url), "utf8"); + const section = reference.split("## Error Envelopes")[1]?.split(/\n## /u)[0] ?? ""; + assert.ok(section.length > 0, "the Error Envelopes section exists"); + + // The canonical page states that only the *_argv arrays are executable and the paired + // *_command strings are display-only. This section must not contradict it. + assert.match(section, /`login_argv`, `help_argv` and `next_argv` are the only\nexecutable forms/u); + assert.match(section, /`next_argv` array as the next request's `argv`/u); + assert.match(section, /`help_argv` \| `invalid_arguments` only/u); + + for (const line of section.split("\n")) { + if (!/`(next|help|login)_command`/u.test(line)) continue; + assert.match( + line, + /display-only|never be executed/u, + `a *_command mention must mark it display-only: ${line.trim().slice(0, 120)}`, + ); + assert.doesNotMatch( + line, + /\b(run|execute|invoke) the returned `\w+_command`|directly runnable/u, + `the docs must not instruct executing a command string: ${line.trim().slice(0, 120)}`, + ); + } +}); + +function bodyFailingResponse(error) { + return { + ok: true, + status: 200, + statusText: "OK", + headers: new Headers({ "mcp-session-id": "sess-body" }), + async text() { + throw error; + }, + }; +} + +test("a body read that fails during a call stage is a typed transport outcome with stage context", async () => { + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const startArgs = ["call", "start", "--to-phone", "+15551234567", "--goal", "Confirm appointment", "--base-url", "https://mcp.example"]; + + const abortRoot = makeTempRoot("calle-cli-stage-body-abort"); + writeToken(abortRoot, serverUrl, "tool-token"); + const aborted = new Error("aborted"); + aborted.name = "AbortError"; + const abortResult = await run([...startArgs, "--cache-root", abortRoot], { + fetchImpl: mcpFixture({ serverUrl, onToolsCall: () => bodyFailingResponse(aborted) }), + }); + const abortPayload = JSON.parse(abortResult.stdout); + assert.equal(abortResult.code, 1); + assert.equal(abortPayload.stage, "plan_call"); + assert.equal(abortPayload.retry_safe, true); + assert.equal(abortPayload.error.code, "plan_call_timeout"); + assert.equal(abortPayload.error.transport, true); + assert.equal(abortPayload.error.cause_code, "timeout"); + assert.equal(abortPayload.error.phase, "body"); + assert.match( + abortPayload.error.message, + /^plan_call timed out while the response was being read; the request had already been accepted\.$/u + ); + + const resetRoot = makeTempRoot("calle-cli-stage-body-reset"); + writeToken(resetRoot, serverUrl, "tool-token"); + const reset = new Error("socket hang up"); + reset.code = "ECONNRESET"; + const resetResult = await run([...startArgs, "--cache-root", resetRoot], { + fetchImpl: mcpFixture({ serverUrl, onToolsCall: () => bodyFailingResponse(reset) }), + }); + const resetPayload = JSON.parse(resetResult.stdout); + assert.equal(resetResult.code, 1); + assert.equal(resetPayload.stage, "plan_call"); + assert.equal(resetPayload.call_started, false); + assert.equal(resetPayload.retry_safe, true); + assert.equal(resetPayload.error.code, "transport_error", "a rejected transport at a stage is transport_error, not _error"); + assert.equal(resetPayload.error.transport, true); + assert.equal(resetPayload.error.cause_code, "ECONNRESET"); + assert.equal(resetPayload.error.phase, "body"); + assert.match( + resetPayload.error.message, + /^plan_call failed while the response was being read; the request had already been accepted\.$/u + ); +}); + +test("a call-stage body failure keeps its phase and does not claim nothing was received", async () => { + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const startArgs = ["call", "start", "--to-phone", "+15551234567", "--goal", "g", "--base-url", "https://mcp.example"]; + + const bodyFailures = [ + { + name: "timeout while reading the body", + error: Object.assign(new Error("aborted"), { name: "AbortError" }), + code: "plan_call_timeout", + causeCode: "timeout", + summary: /^plan_call timed out while the response was being read; the request had already been accepted\.$/u, + }, + { + name: "reset while reading the body", + error: Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }), + code: "transport_error", + causeCode: "ECONNRESET", + summary: /^plan_call failed while the response was being read; the request had already been accepted\.$/u, + }, + ]; + + for (const failure of bodyFailures) { + const cacheRoot = makeTempRoot("calle-cli-stage-phase"); + writeToken(cacheRoot, serverUrl, "tool-token"); + const result = await run([...startArgs, "--cache-root", cacheRoot], { + fetchImpl: mcpFixture({ serverUrl, onToolsCall: () => bodyFailingResponse(failure.error) }), + }); + const payload = JSON.parse(result.stdout); + + assert.equal(payload.error.code, failure.code, failure.name); + assert.equal(payload.error.transport, true, failure.name); + assert.equal(payload.error.phase, "body", `${failure.name}: the phase must survive the stage wrapper`); + assert.equal(payload.error.cause_code, failure.causeCode, failure.name); + assert.match(payload.error.message, failure.summary, failure.name); + assert.doesNotMatch(payload.error.message, /before a response was received/u, failure.name); + assert.equal(payload.stage, "plan_call"); + } + + // A connect-phase failure at the same stage must still say the request never landed. + const cacheRoot = makeTempRoot("calle-cli-stage-phase-connect"); + writeToken(cacheRoot, serverUrl, "tool-token"); + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + const connect = await run([...startArgs, "--cache-root", cacheRoot], { fetchImpl: async () => { throw dns; } }); + const connectPayload = JSON.parse(connect.stdout); + assert.equal(connectPayload.error.phase, "connect"); + assert.match(connectPayload.error.message, /^plan_call failed before a response was received\.$/u); +}); + +test("a credential split by a control sequence inside a remote body is still fully redacted", async () => { + const cacheRoot = makeTempRoot("calle-cli-split-credential"); + const ESC = String.fromCharCode(27); + const body = { + error: "oauth_register_failed", + message: + `access_token=abcd${ESC}[31m1234efgh5678 and sk_live_ABCDEFGHIJ${ESC}[0mKLMNOPQRSTUV plus ` + + `Bearer abcdefghijkl${ESC}]8;;x${String.fromCharCode(7)}mnopqrstuvwxyz012345`, + }; + const result = await run([...LOGIN_ARGS, "--cache-root", cacheRoot], { fetchImpl: brokerFailure(502, body) }); + const payload = JSON.parse(result.stdout); + + assert.equal(payload.error.code, "broker_unavailable"); + for (const fragment of ["abcd1234", "1234efgh", "efgh5678", "ABCDEFGHIJ", "KLMNOPQRSTUV", "abcdefghijkl", "mnopqrstuvwxyz012345"]) { + assert.doesNotMatch(result.stdout, new RegExp(fragment), `fragment ${fragment} leaked to stdout`); + assert.doesNotMatch(result.stderr, new RegExp(fragment), `fragment ${fragment} leaked to stderr`); + } + assert.match(payload.error.remote_error.message, /\[redacted\]/u); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); +}); + +test("nested terminal payloads and Unicode line separators cannot leak through the CLI envelope", async () => { + const ESC = String.fromCharCode(0x1b); + const BEL = String.fromCharCode(0x07); + const cases = [ + `access_to${ESC}]title${ESC}[31mmore${BEL}ken=abcd1234efgh5678`, + `access_to${String.fromCharCode(0x90)}junk${ESC}[31mmore${String.fromCharCode(0x9c)}ken=abcd1234efgh5678`, + `access_to${String.fromCharCode(0x90)}junk${BEL}more${String.fromCharCode(0x9c)}ken=abcd1234efgh5678`, + `access_to${ESC}cken=abcd1234efgh5678`, + `access_to${String.fromCharCode(0x9b)}1${String.fromCharCode(0)}2mken=abcd1234efgh5678`, + "access_token=abcd1234\u2028efgh5678", + "Bearer abcdefghijkl\u2029mnopqrstuvwxyz012345", + ]; + + for (const message of cases) { + const cacheRoot = makeTempRoot("calle-cli-nested-control-credential"); + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { fetchImpl: brokerFailure(502, { error: "oauth_register_failed", message }) }, + ); + const payload = JSON.parse(result.stdout); + + assert.equal(payload.error.code, "broker_unavailable"); + assert.match(payload.error.remote_error.message, /\[redacted\]/u); + for (const fragment of ["abcd1234", "efgh5678", "abcdefghijkl", "mnopqrstuvwxyz012345", "junk", "more"]) { + assert.doesNotMatch(result.stdout, new RegExp(fragment), `${fragment} leaked to stdout`); + assert.doesNotMatch(result.stderr, new RegExp(fragment), `${fragment} leaked to stderr`); + } + } +}); + +test("every envelope agrees with the contract: transport flag, remote_error shape, local summary", async () => { + const { ERROR_CODES } = await import("../lib/cli.js"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const REMOTE_MARK = "REMOTE-TEXT-MARKER"; + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + const aborted = new Error("aborted"); + aborted.name = "AbortError"; + + const scenarios = [ + { name: "invalid arguments", args: ["call", "plan", "--to"], deps: {} }, + { name: "broker 5xx", args: [...LOGIN_ARGS], deps: { fetchImpl: brokerFailure(502, { error: "x", message: REMOTE_MARK }) } }, + { name: "broker 4xx", args: [...LOGIN_ARGS], deps: { fetchImpl: brokerFailure(400, { error: "x", message: REMOTE_MARK }) } }, + { name: "broker fetch rejected", args: [...LOGIN_ARGS], deps: { fetchImpl: async () => { throw dns; } } }, + { name: "broker timeout", args: [...LOGIN_ARGS], deps: { fetchImpl: async () => { throw aborted; } } }, + { + name: "mcp json-rpc error", args: ["mcp", "tools", "--base-url", "https://mcp.example"], token: true, + deps: { fetchImpl: mcpFixture({ serverUrl, onToolsList: (p) => jsonRpcResponse({ jsonrpc: "2.0", id: p.id, error: { code: -32000, message: REMOTE_MARK } }) }) }, + }, + { name: "mcp fetch rejected", args: ["mcp", "tools", "--base-url", "https://mcp.example"], token: true, deps: { fetchImpl: async () => { throw dns; } } }, + { + name: "plan not ready", args: ["call", "start", "--to-phone", "+15551234567", "--goal", "g", "--base-url", "https://mcp.example"], token: true, + deps: { fetchImpl: mcpFixture({ serverUrl, onToolsCall: (p) => jsonRpcResponse({ jsonrpc: "2.0", id: p.id, result: { structuredContent: { ready_to_run: false, clarifying_questions: [REMOTE_MARK] } } }) }) }, + }, + { + name: "stage isError", args: ["call", "start", "--to-phone", "+15551234567", "--goal", "g", "--base-url", "https://mcp.example"], token: true, + deps: { fetchImpl: mcpFixture({ serverUrl, onToolsCall: (p) => jsonRpcResponse({ jsonrpc: "2.0", id: p.id, result: { isError: true, structuredContent: { error_code: "REMOTE_CODE", message: REMOTE_MARK } } }) }) }, + }, + { + name: "stage body reset", args: ["call", "start", "--to-phone", "+15551234567", "--goal", "g", "--base-url", "https://mcp.example"], token: true, + deps: { fetchImpl: mcpFixture({ serverUrl, onToolsCall: () => bodyFailingResponse(Object.assign(new Error("reset"), { code: "ECONNRESET" })) }) }, + }, + ]; + + for (const scenario of scenarios) { + const cacheRoot = makeTempRoot("calle-cli-parity"); + if (scenario.token) writeToken(cacheRoot, serverUrl, "tool-token"); + const result = await run([...scenario.args, "--cache-root", cacheRoot], scenario.deps); + const payload = JSON.parse(result.stdout); + const label = `[${scenario.name}] code=${payload.error?.code}`; + + assert.notEqual(result.code, 0, label); + assert.equal(payload.ok, false, label); + assert.ok(Object.hasOwn(ERROR_CODES, payload.error.code), `${label}: code is in the contract`); + assert.equal(result.code, ERROR_CODES[payload.error.code].exitCode, `${label}: exit code matches the contract`); + assert.equal(Boolean(payload.error.transport), ERROR_CODES[payload.error.code].transport, `${label}: transport flag matches the contract`); + assert.doesNotMatch(payload.error.message, new RegExp(REMOTE_MARK), `${label}: summary is locally authored`); + assert.doesNotMatch(result.stderr, new RegExp(REMOTE_MARK), `${label}: stderr is locally authored`); + if (payload.error.remote_error !== undefined) { + const keys = Object.keys(payload.error.remote_error); + assert.ok(keys.length > 0 && keys.every((k) => k === "code" || k === "message"), `${label}: remote_error is only {code, message}`); + if (payload.error.remote_error.code !== undefined) { + assert.match(payload.error.remote_error.code, /^-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u, `${label}: remote code charset`); + } + } + } +}); + +test("a successful response with a non-JSON body does not publish the body as the summary", async () => { + const cacheRoot = makeTempRoot("calle-cli-invalid-json"); + const marker = "REMOTE-TEXT-MARKER access_token=sk_live_ABCDEFGHIJKLMNOP"; + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { + fetchImpl: async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response(marker, { status: 200, headers: { "content-type": "text/plain" } }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }, + } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "invalid_response"); + assert.equal(payload.error.status_code, 200); + assert.equal(payload.error.transport, undefined, "a bad body is not a network condition"); + assert.match(payload.error.message, /whose body was not the expected JSON/u); + // Node's own SyntaxError would have quoted the body here. + assert.doesNotMatch(payload.error.message, /REMOTE-TEXT-MARKER/u); + assert.doesNotMatch(result.stderr, /REMOTE-TEXT-MARKER/u); + assert.doesNotMatch(result.stdout, /sk_live_/u); + assert.doesNotMatch(result.stderr, /sk_live_/u); +}); + +test("a remote error_code is validated exactly like any other machine code", async () => { + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + const cases = [ + { code: "EXECUTION_ACK_LOST", kept: "EXECUTION_ACK_LOST" }, + { code: -32000, kept: "-32000" }, + { code: 1e100, kept: undefined }, + { code: 1.5, kept: undefined }, + { code: "x".repeat(80), kept: undefined }, + { code: " spaced code ", kept: undefined }, + { code: " SAFE_CODE ", kept: undefined }, + { code: `bad${String.fromCharCode(27)}[31m`, kept: undefined }, + { code: "has spaces", kept: undefined }, + ]; + + for (const { code, kept } of cases) { + const cacheRoot = makeTempRoot("calle-cli-errorcode"); + writeToken(cacheRoot, serverUrl, "tool-token"); + const result = await run( + ["call", "start", "--to-phone", "+15551234567", "--goal", "g", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { + fetchImpl: mcpFixture({ + serverUrl, + onToolsCall: (p) => jsonRpcResponse({ + jsonrpc: "2.0", + id: p.id, + result: { isError: true, structuredContent: { error_code: code, message: "stage failed" } }, + }), + }), + } + ); + const payload = JSON.parse(result.stdout); + assert.equal(payload.error.error_code, kept, `for input ${JSON.stringify(code)}`); + if (payload.error.error_code !== undefined) { + assert.match(payload.error.error_code, /^-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u); + } + } +}); + +test("a hostile plan_call result that omits plan_id cannot leak through the invalid-response path", async () => { + // extractRequiredStructuredString throws with the entire tool result as `payload`. That + // result is server-controlled: its text content, its structuredContent, and any extra + // fields. None of it may reach the summary, stderr, or an unvalidated remote_error. + const cacheRoot = makeTempRoot("calle-cli-plan-invalid-hostile"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const ESC = String.fromCharCode(27); + const hostileText = + `${ESC}[2J${ESC}[H` + + `{"confirm_token":"confirm-secret-DO-NOT-PRINT","access_token":"sk_live_ABCDEFGHIJKLMNOPQRST"}` + + `\r\nplan-secret ${"y".repeat(20_000)}`; + const fetchImpl = mcpFixture({ + serverUrl, + onToolsCall: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + result: { + content: [{ type: "text", text: hostileText }], + // ready_to_run is true but plan_id is absent, so the CLI must reject the plan. + structuredContent: { + ready_to_run: true, + confirm_token: "confirm-secret-DO-NOT-PRINT", + message: `remote message ${ESC}[31m tok_SECRET_VALUE_1234567890`, + refresh_token: "rt_SECRET_ABCDEFGHIJ", + }, + }, + }), + }); + + const result = await run( + ["call", "start", "--to-phone", "+15551234567", "--goal", "Confirm appointment", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.stage, "plan_call"); + assert.equal(payload.call_started, false); + assert.equal(payload.retry_safe, true); + assert.equal(payload.error.code, "plan_call_invalid_response"); + assert.equal(payload.error.message, "plan_call did not return plan_id"); + assert.equal(payload.error.transport, undefined); + assert.ok(result.stdout.length < 2000, "no amplification of the 20 KB body"); + for (const secret of ["confirm-secret", "DO-NOT-PRINT", "sk_live_", "plan-secret", "tok_SECRET", "rt_SECRET", "refresh_token", "yyyyyyyy"]) { + assert.doesNotMatch(result.stdout, new RegExp(secret), `${secret} leaked to stdout`); + assert.doesNotMatch(result.stderr, new RegExp(secret), `${secret} leaked to stderr`); + } + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); + if (payload.error.remote_error !== undefined) { + assert.ok(Object.keys(payload.error.remote_error).every((k) => k === "code" || k === "message")); + assert.doesNotMatch(JSON.stringify(payload.error.remote_error), CONTROL_CHARS); + } +}); + test("auth login start-only replaces locally active pending cache when broker reports it expired", async () => { const cacheRoot = makeTempRoot("calle-cli-login-start-only-expired-broker"); const serverUrl = "https://mcp.example/mcp/openagent_oauth"; @@ -1519,8 +2426,9 @@ test("call start reports plan clarification and skips run_call when planning is assert.equal(payload.error.code, "plan_not_ready"); assert.equal( payload.error.message, - "Call plan needs more information before it can run: What should the agent ask or say on the call?" + "Call plan needs more information before it can run. See error.remote_error.message for the question the service asked." ); + assert.equal(payload.error.remote_error.message, "What should the agent ask or say on the call?"); }); test("call start rejects a null structured confirm token without calling run_call", async () => { @@ -1693,7 +2601,9 @@ test("call start preserves safe run_call error fields and an opaque recovery id" assert.equal(payload.error.code, "run_call_error"); assert.equal(payload.error.error_code, "EXECUTION_ACK_LOST"); assert.equal(payload.error.status, "UNKNOWN"); - assert.equal(payload.error.message, "Execution acknowledgement was lost."); + assert.equal(payload.error.message, "run_call returned an error."); + assert.equal(payload.error.remote_error.message, "Execution acknowledgement was lost."); + assert.deepEqual(Object.keys(payload.error.remote_error).sort(), ["code", "message"]); assert.match(payload.recovery_id, /^[A-Za-z0-9_-]{20,}$/u); assert.match(payload.next_command, new RegExp(`calle call recover --recovery-id ${payload.recovery_id}`)); assert.doesNotMatch(result.stdout, /plan-secret|confirm-secret|service-secret|do-not-print/); @@ -1754,7 +2664,8 @@ test("call run preserves safe error fields when run_call omits run_id", async () assert.equal(payload.error.code, "run_call_missing_run_id"); assert.equal(payload.error.error_code, "DESTINATION_REJECTED"); assert.equal(payload.error.status, "FAILED"); - assert.equal(payload.error.message, "The destination was rejected."); + assert.equal(payload.error.message, "run_call did not return a run_id."); + assert.equal(payload.error.remote_error.message, "The destination was rejected."); assert.doesNotMatch(result.stdout, /plan-secret|confirm-secret|do-not-print/); }); diff --git a/packages/cli/test/e2e/cli-e2e.test.js b/packages/cli/test/e2e/cli-e2e.test.js index 494cfa8..9d6a8d7 100644 --- a/packages/cli/test/e2e/cli-e2e.test.js +++ b/packages/cli/test/e2e/cli-e2e.test.js @@ -1004,6 +1004,15 @@ test("agent requests preserve opaque values and recover once with the old SDK an assert.match(oldHelp.stderr, /MCP command help check failed/); // ponytail: dependency fixtures are CLI 0.5.0; install a candidate tarball when runtime dependencies change. for (const dir of ["bin", "lib", "scripts"]) fs.cpSync(path.join(packageRoot, dir), path.join(mcp, dir), { recursive: true }); + // The registry copy of @call-e/core pinned by CLI 0.5.0 predates the subpaths this working + // tree imports, so the candidate CLI would fail to resolve them and exit before writing any + // envelope. Overlay the local core for the same reason the lines above overlay the CLI: the + // fixture exists to exercise this tree inside a shadowed environment, not the published core. + const localCore = path.join(packageRoot, "..", "core"); + const installedCore = path.join(mcp, "node_modules/@call-e/core"); + fs.mkdirSync(installedCore, { recursive: true }); + fs.cpSync(path.join(localCore, "lib"), path.join(installedCore, "lib"), { recursive: true }); + fs.cpSync(path.join(localCore, "package.json"), path.join(installedCore, "package.json")); const fakeBin = path.join(root, "fake bin"); const fakeLog = path.join(root, "shadow-arguments.jsonl"); diff --git a/packages/core/README.md b/packages/core/README.md index 7524ef2..f1a4119 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -8,7 +8,7 @@ This package is used by CALL-E integrations such as `@call-e/cli`. It is not a s ```js import { tokenCachePath } from "@call-e/core/cache"; -import { createBrokerSession } from "@call-e/core/broker-client"; +import { BrokerLoginError, createBrokerSession } from "@call-e/core/broker-client"; import { callMcpTool } from "@call-e/core/mcp-client"; ``` @@ -20,6 +20,7 @@ Public subpaths: - `@call-e/core/http` - `@call-e/core/broker-client` - `@call-e/core/mcp-client` +- `@call-e/core/sanitize` TypeScript declarations are included for the root export and every public subpath. @@ -109,6 +110,74 @@ for the tool inputs, result handoffs, polling guidance, and complete safety contract. At runtime, `listMcpTools` remains authoritative for the server's current MCP schemas. +## Errors and Remote Text + +Every string that arrives from the network is untrusted. The library keeps it out of +`Error.message` and offers one sanitizer for displaying it. + +```js +import { requestJson, HttpStatusError, TransportError, causeCodeOf } from "@call-e/core/http"; +import { callMcpTool, McpHttpError } from "@call-e/core/mcp-client"; +import { publicRemoteError, safeRemoteString } from "@call-e/core/sanitize"; + +try { + await callMcpTool({ config, toolName: "plan_call" }); +} catch (error) { + if (error instanceof McpHttpError) { + error.message; // locally authored, safe to print: "Remote MCP error for tools/call" + error.payload; // raw server error, for programmatic use only + error.remoteError; // { code?, message? } sanitized, safe to display + error.transport; // true only when no usable response was received + error.timedOut; // true for the client-side timeout + error.causeCode; // "timeout", a Node.js system code such as "ENOTFOUND", or null + } +} +``` + +| Type | Thrown by | Meaning | +| --- | --- | --- | +| `HttpStatusError` | `requestJson` | A non-success HTTP status. `statusCode`, `responseText`, `headers`, `url`. | +| `TransportError` | `requestJson` | No usable response: `fetch` rejected, the body could not be read, or the timeout fired. `url`, `method`, `timedOut`, `phase` (`connect` or `body`), `code` (`timeout`, or the system code such as `ECONNRESET` for a body-phase failure). | +| `InvalidResponseError` | `requestJson` | A 2xx whose body was not the expected JSON object. `responseText` holds the raw body; `message` never quotes it, because `JSON.parse` puts its input into its own message. | +| `McpHttpError` | MCP client | HTTP failure (`code: "http_error"`), JSON-RPC error (`"mcp_error"`), malformed or mismatched successful response (`"invalid_response"`), or transport failure (`"transport_error"`). | +| `BrokerLoginError` | `loginWithBroker` | A terminal broker outcome (`code: "broker_login_failed"`) or overall authorization wait timeout (`"broker_login_timeout"`). `message` is locally authored; sanitized service detail is in `remoteError`. | + +`@call-e/core/sanitize`: + +| Function | Purpose | +| --- | --- | +| `stripTerminalControls(value)` | Remove ANSI CSI/OSC/ESC sequences and C0/C1 control characters. | +| `redactSecrets(value)` | Replace credential-shaped substrings (bearer tokens, `token=`-style pairs, known prefixes, long opaque runs) with `[redacted]`. | +| `safeRemoteString(value, maxLength = 500)` | Controls removed first, then secrets redacted, then bounded. `undefined` for non-strings and empty results. | +| `safeRemoteCode(value)` | A machine code matching `-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}`; numbers only as safe integers; otherwise `undefined`. | +| `publicRemoteError(value)` | The only shape remote detail should take in a public payload: `{ code?, message? }` or `null`. | +| `sanitizeRemoteError(body)` | Reduce a JSON-RPC error, HTTP body, or tool result to `publicRemoteError` shape, reading only `code` / `message`. | + +Controls are removed rather than replaced before secret detection, so +`access_token=abcd[31m1234` is redacted as one credential instead of surviving as two +halves. Removal is by *sequence*, covering both the 7-bit `ESC [` / `ESC ]` forms and the +8-bit `U+009B` / `U+009D` introducers, plus invisible format characters such as zero-width +spaces and bidi controls and Unicode line/paragraph separators. + +Removal covers every terminal string control — OSC, DCS, SOS, PM and APC, in their 7-bit +(`ESC ]`, `ESC P`, `ESC X`, `ESC ^`, `ESC _`) and 8-bit forms — through its terminator, and +through end of input when a sequence is left unterminated. Stripping only the introducer would +leave the payload behind as ordinary text, which is what splits a key name apart. An embedded +ESC sequence is consumed as payload unless it is the string terminator; it cannot make the +outer sequence fall back to character-at-a-time stripping. Other ECMA-35 escape functions, +including private, standardized, and intermediate-byte forms, are removed as complete +sequences; embedded C0/C1 controls do not make CSI parameter text survive. BEL is accepted as +a legacy OSC terminator only; inside DCS, SOS, PM, and APC it remains payload until ST. + +Safety is checked over two canonicalizations, because they disagree and both matter. Consuming +a whole sequence is what a terminal does, but a sequence swallows its final byte, and that +byte can be chosen from the word being searched for: `Bearer secret` is a valid CSI +sequence ending in `r`, so correct stripping yields `Beaer` and the credential stops looking +like one. Whenever the two readings differ at all, the whole string is replaced with +`[redacted]`. Requiring a recognised credential in either reading is not sufficient: mixed +sequences can require a different interpretation per sequence, so neither global reading +reconstructs the sensitive key even though the displayed value still contains its credential. + ## Development ```bash diff --git a/packages/core/lib/broker-client.d.ts b/packages/core/lib/broker-client.d.ts index d175da0..95ba238 100644 --- a/packages/core/lib/broker-client.d.ts +++ b/packages/core/lib/broker-client.d.ts @@ -68,6 +68,18 @@ export interface BrokerLoginResult { tokenDocument: TokenDocument; } +export interface BrokerLoginErrorOptions { + code?: "broker_login_failed" | "broker_login_timeout"; + remoteError?: unknown; +} + +/** A broker workflow outcome whose message is locally authored and safe to print. */ +export class BrokerLoginError extends Error { + constructor(message: string, options?: BrokerLoginErrorOptions); + code: "broker_login_failed" | "broker_login_timeout"; + remoteError: { code?: string; message?: string } | null; +} + export function createBrokerSession( config: CreateBrokerSessionConfig, options?: BrokerRequestOptions, diff --git a/packages/core/lib/broker-client.js b/packages/core/lib/broker-client.js index 11c1323..63873dc 100644 --- a/packages/core/lib/broker-client.js +++ b/packages/core/lib/broker-client.js @@ -1,6 +1,17 @@ import { pendingCachePath, pendingIsExpired, readPendingLogin, removeFile, tokenCachePath, tokenIsUsable, writePrivateJson, readJson } from "./cache.js"; import { INTEGRATION_HEADER, SESSION_SECRET_HEADER } from "./constants.js"; import { HttpStatusError, requestJson } from "./http.js"; +import { publicRemoteError } from "./sanitize.js"; + +/** A broker workflow outcome whose message is always locally authored. */ +export class BrokerLoginError extends Error { + constructor(message, { code = "broker_login_failed", remoteError = null } = {}) { + super(message); + this.name = "BrokerLoginError"; + this.code = code === "broker_login_timeout" ? code : "broker_login_failed"; + this.remoteError = publicRemoteError(remoteError); + } +} function integrationHeaders(config) { return config?.integrationHeader ? { [INTEGRATION_HEADER]: config.integrationHeader } : {}; @@ -176,12 +187,16 @@ export async function loginWithBroker(config, { } if (status === "FAILED" || status === "EXPIRED" || status === "EXCHANGED") { removeFile(pendingPath); - throw new Error(`Brokered login failed: ${current.error_message || status}`); + throw new BrokerLoginError("Brokered login failed.", { + remoteError: { code: status, message: current.error_message }, + }); } const delayMs = Math.max(500, Math.min(Number(current.poll_after_ms || 2000), 10000)); await sleepImpl(delayMs); } - throw new Error("Timed out waiting for brokered login authorization."); + throw new BrokerLoginError("Timed out waiting for brokered login authorization.", { + code: "broker_login_timeout", + }); } diff --git a/packages/core/lib/http.d.ts b/packages/core/lib/http.d.ts index 4a40fca..a8c83d2 100644 --- a/packages/core/lib/http.d.ts +++ b/packages/core/lib/http.d.ts @@ -4,6 +4,23 @@ export interface HttpStatusErrorOptions { statusCode?: number | null; responseText?: string; headers?: Record; + url?: string | null; +} + +export interface TransportErrorOptions { + url?: string | null; + method?: string | null; + timedOut?: boolean; + /** "connect" when nothing arrived; "body" when the stream failed after headers. */ + phase?: "connect" | "body"; + cause?: unknown; +} + +export interface InvalidResponseErrorOptions { + url?: string | null; + method?: string | null; + statusCode?: number | null; + responseText?: string; } export interface RequestJsonOptions { @@ -18,6 +35,36 @@ export class HttpStatusError extends Error { statusCode: number | null; responseText: string; headers: Record; + url: string | null; +} + +/** The Node.js system error code behind a failed fetch (`ENOTFOUND`, `ECONNREFUSED`, ...), or null. */ +export function causeCodeOf(error: unknown): string | null; + +/** + * No usable response: DNS, connection, TLS, a timeout, or a body stream that failed after the + * headers arrived. `phase` distinguishes the last case from the rest. + */ +export class TransportError extends Error { + constructor(message: string, options?: TransportErrorOptions); + url: string | null; + method: string | null; + timedOut: boolean; + phase: "connect" | "body"; + /** "timeout", or the Node.js error code of the cause (e.g. "ENOTFOUND"), or null. */ + code: string | null; +} + +/** + * A 2xx response whose body was not the expected JSON. The raw body lives in `responseText` + * for sanitizing; `message` never quotes it. + */ +export class InvalidResponseError extends Error { + constructor(message: string, options?: InvalidResponseErrorOptions); + url: string | null; + method: string | null; + statusCode: number | null; + responseText: string; } export function requestJson( diff --git a/packages/core/lib/http.js b/packages/core/lib/http.js index bca9492..eaa3db9 100644 --- a/packages/core/lib/http.js +++ b/packages/core/lib/http.js @@ -1,10 +1,63 @@ export class HttpStatusError extends Error { - constructor(message, { statusCode, responseText, headers } = {}) { + constructor(message, { statusCode, responseText, headers, url } = {}) { super(message); this.name = "HttpStatusError"; this.statusCode = statusCode ?? null; this.responseText = responseText ?? ""; this.headers = headers ?? {}; + this.url = url ?? null; + } +} + +/** + * The Node.js system error code behind a failed fetch, if any. `fetch` rejects with + * `TypeError: fetch failed` whose `cause` is the system error (`ENOTFOUND`, `ECONNREFUSED`, + * `CERT_HAS_EXPIRED`, ...), so the code may sit one or two levels down. + */ +export function causeCodeOf(error) { + for (let cursor = error, depth = 0; cursor && depth < 4; cursor = cursor.cause, depth++) { + if (typeof cursor.code === "string" && cursor.code) { + return cursor.code; + } + } + return null; +} + +/** + * No usable response: DNS failure, connection refused, TLS error, the client-side timeout, or + * a body stream that failed after the headers arrived. `phase` distinguishes the last case + * (`body`) from the rest (`connect`). This is the only condition that may be described to a + * caller as a network problem; an unrelated local exception must not be classified as one. + */ +export class TransportError extends Error { + constructor(message, { url, method, timedOut = false, phase = "connect", cause } = {}) { + super(message, cause !== undefined ? { cause } : undefined); + this.name = "TransportError"; + this.url = url ?? null; + this.method = method ?? null; + this.timedOut = Boolean(timedOut); + /** "connect" when nothing arrived, "body" when the stream failed after headers. */ + this.phase = phase; + this.code = timedOut ? "timeout" : causeCodeOf(cause); + } +} + +/** + * A 2xx response whose body is not the JSON the caller needs. + * + * This exists because `JSON.parse` puts the offending input into its own message — Node emits + * `Unexpected token R, "REMOTE-TEXT-MARKER" is not valid JSON` — so letting a native + * SyntaxError escape would publish remote text as the CLI's locally-authored summary. The raw + * body is kept here for sanitizing, and never in `message`. + */ +export class InvalidResponseError extends Error { + constructor(message, { url, method, statusCode = null, responseText = "" } = {}) { + super(message); + this.name = "InvalidResponseError"; + this.url = url ?? null; + this.method = method ?? null; + this.statusCode = statusCode; + this.responseText = responseText; } } @@ -20,8 +73,9 @@ export async function requestJson(method, url, { headers = {}, json = undefined, timeout.unref(); } + let response; try { - const response = await fetchImpl(url, { + response = await fetchImpl(url, { method, headers: { Accept: "application/json", @@ -31,27 +85,77 @@ export async function requestJson(method, url, { headers = {}, json = undefined, body: json !== undefined ? JSON.stringify(json) : undefined, signal: controller.signal, }); - const text = await response.text(); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new TransportError(`Request timed out for ${method} ${url}`, { url, method, timedOut: true }); + } + throw new TransportError(`Request failed before a response was received for ${method} ${url}`, { + url, + method, + cause: error, + }); + } + + // Headers arrived; the body can still fail (timeout mid-stream, socket reset). That is a + // transport failure too, and must not escape as a raw AbortError. + let text; + try { + text = await response.text(); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new TransportError(`Request timed out for ${method} ${url}`, { + url, + method, + timedOut: true, + phase: "body", + }); + } + throw new TransportError(`Response body could not be read for ${method} ${url}`, { + url, + method, + phase: "body", + cause: error, + }); + } + + try { if (!response.ok) { - throw new HttpStatusError(`Client error '${response.status} ${response.statusText}' for url '${url}'`, { + // `statusText` is supplied by the server. Keep Error.message locally authored so core + // consumers can print it without repeating the CLI's remote-text boundary themselves. + throw new HttpStatusError(`HTTP ${response.status} for ${method} ${url}`, { statusCode: response.status, responseText: text, headers: Object.fromEntries(response.headers.entries()), + url, }); } if (!text.trim()) { return {}; } - const parsed = JSON.parse(text); - if (!parsed || typeof parsed !== "object") { - throw new Error(`Expected JSON object response for ${method} ${url}`); + let parsed; + try { + parsed = JSON.parse(text); + } catch { + // Deliberately not rethrowing the SyntaxError: its message quotes the response body. + throw new InvalidResponseError(`Response body was not valid JSON for ${method} ${url}`, { + url, + method, + statusCode: response.status, + responseText: text, + }); } - return parsed; - } catch (error) { - if (error?.name === "AbortError") { - throw new Error(`Request timed out for ${method} ${url}`); + // Arrays are objects to `typeof`, but not what any caller of this helper wants. + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new InvalidResponseError(`Response body was not a JSON object for ${method} ${url}`, { + url, + method, + statusCode: response.status, + responseText: text, + }); } - throw error; + return parsed; } finally { clearTimeout(timeout); } diff --git a/packages/core/lib/index.d.ts b/packages/core/lib/index.d.ts index dd6b575..a2fbe84 100644 --- a/packages/core/lib/index.d.ts +++ b/packages/core/lib/index.d.ts @@ -4,3 +4,4 @@ export * from "./config.js"; export * from "./constants.js"; export * from "./http.js"; export * from "./mcp-client.js"; +export * from "./sanitize.js"; diff --git a/packages/core/lib/index.js b/packages/core/lib/index.js index dd6b575..a2fbe84 100644 --- a/packages/core/lib/index.js +++ b/packages/core/lib/index.js @@ -4,3 +4,4 @@ export * from "./config.js"; export * from "./constants.js"; export * from "./http.js"; export * from "./mcp-client.js"; +export * from "./sanitize.js"; diff --git a/packages/core/lib/mcp-client.d.ts b/packages/core/lib/mcp-client.d.ts index e910935..62d1f5e 100644 --- a/packages/core/lib/mcp-client.d.ts +++ b/packages/core/lib/mcp-client.d.ts @@ -17,6 +17,10 @@ export interface McpHttpErrorOptions { payload?: unknown; headers?: Record; code?: string; + transport?: boolean; + timedOut?: boolean; + phase?: "connect" | "body" | null; + cause?: unknown; } export interface McpToolDefinition extends JsonObject { @@ -52,6 +56,16 @@ export class McpHttpError extends Error { payload: unknown; headers: Record; code: string; + /** True only when no usable response arrived: timeout, DNS, connection, TLS, or a body + * stream that failed after the headers. `phase` says which. */ + transport: boolean; + timedOut: boolean; + /** "connect" or "body" on a transport failure; null otherwise. */ + phase: "connect" | "body" | null; + /** "timeout", the system error code behind a rejected fetch (e.g. "ENOTFOUND"), or null. */ + causeCode: string | null; + /** Sanitized, bounded `{ code?, message? }` from the remote body, or null. Safe to display. */ + remoteError: { code?: string; message?: string } | null; } export function isUnauthorizedMcpError(error: unknown): error is McpHttpError; diff --git a/packages/core/lib/mcp-client.js b/packages/core/lib/mcp-client.js index 2c412a7..7e3681b 100644 --- a/packages/core/lib/mcp-client.js +++ b/packages/core/lib/mcp-client.js @@ -5,6 +5,8 @@ import { INTEGRATION_HEADER, MCP_PROTOCOL_VERSION, } from "./constants.js"; +import { causeCodeOf } from "./http.js"; +import { sanitizeRemoteError } from "./sanitize.js"; export class AuthRequiredError extends Error { constructor(message = "A usable CALL-E auth token is required.") { @@ -13,15 +15,37 @@ export class AuthRequiredError extends Error { } } +/** + * `message` is always authored locally and safe to print. Whatever the server said is kept + * raw in `payload` / `responseText` for programmatic use, and in sanitized, bounded form in + * `remoteError` for display. Nothing remote reaches `message`. + */ export class McpHttpError extends Error { - constructor(message, { statusCode = null, responseText = "", payload = null, headers = {}, code = "http_error" } = {}) { - super(message); + constructor(message, { + statusCode = null, + responseText = "", + payload = null, + headers = {}, + code = "http_error", + transport = false, + timedOut = false, + phase = null, + cause, + } = {}) { + super(message, cause !== undefined ? { cause } : undefined); this.name = "McpHttpError"; this.statusCode = statusCode; this.responseText = responseText; this.payload = payload; this.headers = headers; this.code = code; + this.transport = Boolean(transport); + this.timedOut = Boolean(timedOut); + /** "connect" or "body" on a transport failure; null otherwise. */ + this.phase = transport ? (phase ?? "connect") : null; + /** "timeout", the system error code behind a rejected fetch, or null. */ + this.causeCode = timedOut ? "timeout" : causeCodeOf(cause); + this.remoteError = sanitizeRemoteError(payload ?? responseText); } } @@ -57,19 +81,63 @@ async function requestJsonRpc(fetchImpl, url, { headers, payload, timeoutMs }) { timeout.unref(); } + let response; try { - const response = await fetchImpl(url, { + response = await fetchImpl(url, { method: "POST", headers, body: JSON.stringify(payload), signal: controller.signal, }); - const text = await response.text(); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new McpHttpError(`MCP request timed out for ${payload.method}`, { + code: "transport_error", + transport: true, + timedOut: true, + phase: "connect", + }); + } + // fetch rejected before any response: DNS, connection, TLS. Only this path is transport. + throw new McpHttpError(`MCP request failed before a response was received for ${payload.method}`, { + code: "transport_error", + transport: true, + phase: "connect", + cause: error, + }); + } + + // Headers arrived; the body can still fail (timeout mid-stream, socket reset). Map that to + // the same typed transport error as a rejected fetch. + let text; + try { + text = await response.text(); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new McpHttpError(`MCP request timed out for ${payload.method}`, { + code: "transport_error", + transport: true, + timedOut: true, + phase: "body", + }); + } + throw new McpHttpError(`MCP response body could not be read for ${payload.method}`, { + code: "transport_error", + transport: true, + phase: "body", + cause: error, + }); + } + + try { let body = null; + let parseFailed = false; try { body = parseResponseBody(text); } catch { - body = null; + parseFailed = true; } const responseHeaders = Object.fromEntries(response.headers.entries()); @@ -82,21 +150,62 @@ async function requestJsonRpc(fetchImpl, url, { headers, payload, timeoutMs }) { }); } - if (body?.error) { - const error = body.error; - throw new McpHttpError(error.message || `Remote MCP error for ${payload.method}`, { - payload: error, + // A successful status is not a successful RPC when the response cannot be interpreted. + // Keep the raw body available for the shared sanitizer, but never let JSON.parse's + // remote-quoting SyntaxError escape or silently turn malformed input into an empty result. + const expectsResponse = payload.id !== undefined; + const bodyIsObject = Boolean(body && typeof body === "object" && !Array.isArray(body)); + const hasRpcResult = bodyIsObject && Object.hasOwn(body, "result"); + const hasRpcErrorField = bodyIsObject && Object.hasOwn(body, "error"); + const hasRpcError = hasRpcErrorField && Boolean( + body.error + && typeof body.error === "object" + && !Array.isArray(body.error) + && Number.isInteger(body.error.code) + && typeof body.error.message === "string", + ); + const bodyIsEmptyObject = bodyIsObject && Object.keys(body).length === 0; + + // HTTP acknowledgements for notifications have no JSON-RPC response to correlate. Keep + // accepting an empty body (and the existing empty-object acknowledgement), while still + // rejecting malformed non-empty JSON. + const validNotificationAck = !expectsResponse + && !parseFailed + && (!text.trim() || bodyIsEmptyObject); + + // A request response is valid only when it belongs to this exact request and carries one + // outcome. Checking merely for `result` let stale/wrong-version responses and envelopes + // containing both `result` and `error` pass as successes. + const validRpcResponse = expectsResponse + && !parseFailed + && bodyIsObject + && body.jsonrpc === "2.0" + && Object.hasOwn(body, "id") + && body.id === payload.id + && hasRpcResult !== hasRpcErrorField + && (hasRpcResult || hasRpcError); + + if (!validNotificationAck && !validRpcResponse) { + throw new McpHttpError(`MCP response was invalid for ${payload.method}`, { + statusCode: response.status, + responseText: text, + payload: bodyIsObject ? body : null, + headers: responseHeaders, + code: "invalid_response", + }); + } + + if (hasRpcError) { + // The server's message is untrusted: it is kept in `payload` and, sanitized, in + // `remoteError`. The Error message itself stays locally authored. + throw new McpHttpError(`Remote MCP error for ${payload.method}`, { + payload: body.error, headers: responseHeaders, code: "mcp_error", }); } return { body, headers: responseHeaders }; - } catch (error) { - if (error?.name === "AbortError") { - throw new McpHttpError(`MCP request timed out for ${payload.method}`, { code: "http_error" }); - } - throw error; } finally { clearTimeout(timeout); } diff --git a/packages/core/lib/sanitize.d.ts b/packages/core/lib/sanitize.d.ts new file mode 100644 index 0000000..b839e21 --- /dev/null +++ b/packages/core/lib/sanitize.d.ts @@ -0,0 +1,15 @@ +export const REMOTE_MESSAGE_LIMIT: number; +export const REMOTE_CODE_LIMIT: number; + +export interface SanitizedRemoteError { + code?: string; + message?: string; +} + +export function stripTerminalControls(value: unknown): string; +export function redactSecrets(value: unknown): string; +export function safeRemoteString(value: unknown, maxLength?: number): string | undefined; +export function safeRemoteCode(value: unknown): string | undefined; +/** The only shape remote detail may take in a public envelope. */ +export function publicRemoteError(value: unknown): SanitizedRemoteError | null; +export function sanitizeRemoteError(body: unknown): SanitizedRemoteError | null; diff --git a/packages/core/lib/sanitize.js b/packages/core/lib/sanitize.js new file mode 100644 index 0000000..b4accc3 --- /dev/null +++ b/packages/core/lib/sanitize.js @@ -0,0 +1,263 @@ +/** + * One boundary for every remote-supplied string. + * + * Anything that arrives from the network — an MCP JSON-RPC error, an upstream HTTP body, a + * clarifying question inside a tool result — is untrusted. Before it can appear in a JSON + * envelope, a log line, or a terminal, it passes through here. + * + * Order matters. Terminal control *sequences* are removed whole, before any credential + * detection, so that `access_token=abcd[31m1234` canonicalizes to + * `access_token=abcd1234` and is redacted as one credential rather than surviving as two + * innocent-looking halves. + * + * Both the core library and the CLI import these helpers so there is exactly one + * implementation to review. + */ + +const ESC = String.fromCharCode(0x1b); +const BEL = String.fromCharCode(0x07); +const BACKSLASH = String.fromCharCode(0x5c); +const C0_START = String.fromCharCode(0x00); +const C0_END = String.fromCharCode(0x1f); +const DEL = String.fromCharCode(0x7f); +const C1_END = String.fromCharCode(0x9f); + +// 8-bit C1 forms of the introducers. A terminal accepts these as readily as their ESC-prefixed +// twins, so anything that understands only the 7-bit form can be walked straight past. +const CSI_8BIT = String.fromCharCode(0x9b); +const ST_8BIT = String.fromCharCode(0x9c); +// The five "string" controls, whose payload runs until a terminator: OSC, DCS, SOS, PM, APC. +const OSC_8BIT = String.fromCharCode(0x9d); +const DCS_8BIT = String.fromCharCode(0x90); +const SOS_8BIT = String.fromCharCode(0x98); +const PM_8BIT = String.fromCharCode(0x9e); +const APC_8BIT = String.fromCharCode(0x9f); + +/* + * Removal is by sequence, and every family has to be covered. + * + * Deleting a lone introducer leaves its payload behind as ordinary text, which is a bypass + * rather than a fix: `access_tojunkken=secret` becomes `access_tojunkken=secret`, + * the key name no longer matches, and the credential survives untouched. CSI was handled + * first; DCS, SOS, PM and APC behave identically and need the same treatment, in both their + * 7-bit (`ESC P`, `ESC X`, `ESC ^`, `ESC _`) and 8-bit forms. + * + * An unterminated sequence is consumed through end of input. Leaving the tail visible would + * let a payload that simply omits its terminator carry a credential into the output. + * + * Invisible format characters are removed for the same reason: a zero-width space or word + * joiner splits a token in two without changing a single visible glyph, and half a secret in + * a log is still a secret. + */ +const LBRACKET = `${BACKSLASH}[`; +const RBRACKET = `${BACKSLASH}]`; + +// OSC ] , DCS P, SOS X, PM ^, APC _ — introduced either as ESC + letter or as one C1 byte. +// OSC has a legacy BEL terminator. DCS, SOS, PM and APC do not: a BEL inside those payloads +// must stay consumed until ST, or the text after it can survive and split a credential. +const OSC_INTRO = `(?:${ESC}${RBRACKET}|${OSC_8BIT})`; +const ST_STRING_INTRO = + `(?:${ESC}[PX^_]|[${DCS_8BIT}${SOS_8BIT}${PM_8BIT}${APC_8BIT}])`; +const ST_END = `(?:${ESC}${BACKSLASH}${BACKSLASH}|${ST_8BIT}|$)`; +const OSC_END = `(?:${BEL}|${ST_END})`; +// ESC is legal inside a string-control payload unless it introduces ST (`ESC \\`). Match it +// explicitly rather than excluding every ESC: otherwise an embedded CSI makes the whole +// string-control match fail, and the payload is left behind as ordinary text. +const ST_STRING_PAYLOAD = + `(?:[^${ESC}${ST_8BIT}]|${ESC}(?!${BACKSLASH}${BACKSLASH}))*`; +const OSC_PAYLOAD = + `(?:[^${BEL}${ESC}${ST_8BIT}]|${ESC}(?!${BACKSLASH}${BACKSLASH}))*`; + +const TERMINAL_CONTROL_RE = new RegExp( + [ + // String controls first: they own their payload, and their introducers also match the + // general ESC rule below. + `${OSC_INTRO}${OSC_PAYLOAD}${OSC_END}`, + `${ST_STRING_INTRO}${ST_STRING_PAYLOAD}${ST_END}`, + // CSI, terminated by its final byte or by end of input. C0/C1 controls may occur while a + // terminal is parsing CSI; consume them with the sequence instead of leaving parameter + // text behind when the strict parameter/intermediate grammar is interrupted. + `(?:${ESC}${LBRACKET}|${CSI_8BIT})[${C0_START}-${C0_END}${DEL}-${C1_END} -?]*(?:[@-~]|$)`, + // Remaining ECMA-35 escape sequences: zero or more intermediate bytes, then a final byte. + // This includes private and standardized forms such as ESC 7, ESC =, ESC c and ESC ( B, + // not only the Fe (`ESC @` through `ESC _`) family. + `${ESC}[${C0_START}-${C0_END}${DEL}-${C1_END} -/]*(?:[0-~]|$)`, + // Whatever control characters are left, including CR, LF and TAB. + `[${C0_START}-${C0_END}${DEL}-${C1_END}]`, + // Invisible format and default-ignorable characters: zero widths, joiners, bidi controls, + // soft hyphen, BOM. Unicode line/paragraph separators are terminal/log line controls too; + // removing them also prevents a token value being split across two visual lines. + `[${BACKSLASH}p{Cf}${BACKSLASH}p{Zl}${BACKSLASH}p{Zp}${BACKSLASH}p{Default_Ignorable_Code_Point}]`, + ].join("|"), + "gu", +); + +// Every control and invisible character, removed individually without consuming a sequence's +// payload. This is the *wrong* thing to display, and a necessary second reading for detection +// — see canonicalForms. +const LONE_CONTROL_RE = new RegExp( + `[${C0_START}-${C0_END}${DEL}-${C1_END}]|[${BACKSLASH}p{Cf}${BACKSLASH}p{Zl}${BACKSLASH}p{Zp}${BACKSLASH}p{Default_Ignorable_Code_Point}]`, + "gu", +); + +export const REMOTE_MESSAGE_LIMIT = 500; +export const REMOTE_CODE_LIMIT = 64; + +// A machine code: optional leading minus (JSON-RPC codes are negative integers), then a safe +// token. Anything else is dropped, never "cleaned" into something plausible. +const REMOTE_CODE_RE = /^-?[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u; + +const REDACTION = "[redacted]"; + +// Token-like material that may appear inside an otherwise-allowlisted message string. +// Each pattern is deliberately broad: a false redaction costs a little readability, a missed +// secret ends up in an agent transcript. +const SECRET_PATTERNS = [ + // "Bearer abc..." / "Basic abc..." + /\b(bearer|basic)\s+[A-Za-z0-9._~+/=-]{8,}/giu, + // key=value / key: value / "key": "value" for sensitive key names + /\b(access[_-]?token|refresh[_-]?token|id[_-]?token|token|secret|password|passwd|api[_-]?key|apikey|authorization|cookie|session[_-]?secret|private[_-]?key|client[_-]?secret)\b(\s*["']?\s*[:=]\s*["']?)[^\s"',;)}\]]{4,}/giu, + // Well-known prefixed credentials + /\b(?:sk|pk|rk|tok|rt|xox[abpr]|ghp|gho|ghu|ghs|AKIA|iams|calle)[_-][A-Za-z0-9_-]{12,}/gu, + // Long opaque runs: hex, base64url, uuid-ish + /\b[A-Fa-f0-9]{32,}\b/gu, + /\b[A-Za-z0-9_-]{40,}\b/gu, +]; + +/** Remove terminal control sequences and control characters entirely. Never throws. */ +export function stripTerminalControls(value) { + return String(value ?? "").replace(TERMINAL_CONTROL_RE, ""); +} + +/** + * The two readings of a hostile string, because they disagree and both matter. + * + * Consuming a whole sequence is what a terminal does, and it is what makes output safe to + * print. But a sequence swallows its final byte, and an attacker can choose a final byte that + * belongs to the word we are looking for: `Bearer secret` is a valid CSI sequence + * ending in `r`, so correct stripping yields `Beaer` and the credential no longer looks like + * one. Removing controls individually keeps `Bearer` intact but leaves sequence payloads + * embedded, which is the bypass the other reading catches. + * + * Neither reading is sufficient alone. + */ +function canonicalForms(value) { + const text = String(value ?? ""); + return [text.replace(TERMINAL_CONTROL_RE, ""), text.replace(LONE_CONTROL_RE, "")]; +} + +/** Redact credential-shaped substrings. Never throws. */ +export function redactSecrets(value) { + let out = String(value ?? ""); + out = out.replace(SECRET_PATTERNS[0], (_m, scheme) => `${scheme} ${REDACTION}`); + out = out.replace(SECRET_PATTERNS[1], (_m, key, sep) => `${key}${sep}${REDACTION}`); + for (const pattern of SECRET_PATTERNS.slice(2)) { + out = out.replace(pattern, REDACTION); + } + return out; +} + +/** + * A remote string made safe for display: controls removed, secrets redacted, whitespace + * trimmed, length bounded. Returns undefined for non-strings and empty results so callers can + * omit the field rather than emit an empty one. + */ +export function safeRemoteString(value, maxLength = REMOTE_MESSAGE_LIMIT) { + if (typeof value !== "string") { + return undefined; + } + + const [display, alternate] = canonicalForms(value); + + // Fail closed whenever the readings disagree at all. + // + // Comparing what each reading *found* is not enough, however carefully. Two different + // sequences can require opposite interpretations to reconstruct a sensitive key, leaving + // neither global reading with a recognisable credential. Disagreement means controlled text + // changes what the string says; there is no unambiguous safe display form, so withhold it. + if (display !== alternate) { + return REDACTION; + } + + const cleaned = redactSecrets(display).trim(); + if (!cleaned) { + return undefined; + } + return cleaned.slice(0, maxLength); +} + +/** + * A remote machine code kept as an opaque token. Anything outside the safe character set is + * dropped rather than "cleaned" into something that merely looks valid. + */ +export function safeRemoteCode(value) { + if (typeof value === "number") { + return Number.isSafeInteger(value) ? String(value) : undefined; + } + if (typeof value !== "string") { + return undefined; + } + const trimmed = value.trim(); + // Codes are opaque machine values, not prose. Do not normalize surrounding whitespace + // into a different, plausible code any more than we normalize embedded controls. + if (trimmed !== value) { + return undefined; + } + // A code carrying control characters is dropped, never repaired. Accepting the remainder + // would turn `bad[31m` into the entirely plausible `bad`, which is precisely the + // "clean it into something that looks valid" behaviour this function refuses to do. + if (stripTerminalControls(trimmed) !== trimmed) { + return undefined; + } + return REMOTE_CODE_RE.test(trimmed) ? trimmed : undefined; +} + +/** + * The only shape remote detail may take in a public envelope: at most `{ code, message }`, + * each individually validated. Every field in the input other than those two is ignored. + * Returns null when nothing survives, so callers omit the field. + */ +export function publicRemoteError(value) { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return null; + } + const code = safeRemoteCode(value.code); + const message = safeRemoteString(value.message); + if (code === undefined && message === undefined) { + return null; + } + return { + ...(code !== undefined ? { code } : {}), + ...(message !== undefined ? { message } : {}), + }; +} + +/** + * Reduce an arbitrary remote error body — a JSON-RPC error object, an HTTP body, a tool + * result — to `publicRemoteError` shape. Reads only `code` (or a string `error`) and + * `message`, at the top level or nested under `error`; everything else is dropped unread. + */ +export function sanitizeRemoteError(body) { + let value = body; + if (typeof value === "string") { + const text = value.trim(); + if (!text) { + return null; + } + try { + value = JSON.parse(text); + } catch { + return publicRemoteError({ message: text }); + } + } + + if (!value || typeof value !== "object" || Array.isArray(value)) { + // JSON scalar or array: keep a bounded excerpt of its serialisation, nothing else. + return publicRemoteError({ message: typeof value === "string" ? value : JSON.stringify(value ?? "") }); + } + + const nested = value.error && typeof value.error === "object" && !Array.isArray(value.error) ? value.error : {}; + const code = nested.code ?? value.code ?? (typeof value.error === "string" ? value.error : undefined); + const message = nested.message ?? value.message; + return publicRemoteError({ code, message }); +} diff --git a/packages/core/package.json b/packages/core/package.json index cb79d04..e6ede49 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -24,7 +24,8 @@ "./config": "./lib/config.js", "./constants": "./lib/constants.js", "./http": "./lib/http.js", - "./mcp-client": "./lib/mcp-client.js" + "./mcp-client": "./lib/mcp-client.js", + "./sanitize": "./lib/sanitize.js" }, "files": [ "README.md", @@ -42,7 +43,7 @@ }, "scripts": { "test": "node --test ./test/*.test.js", - "check": "node ../../scripts/check-runtime-syntax.mjs lib/broker-client.js lib/cache.js lib/config.js lib/constants.js lib/http.js lib/index.js lib/mcp-client.js && pnpm run check:types", + "check": "node ../../scripts/check-runtime-syntax.mjs lib/broker-client.js lib/cache.js lib/config.js lib/constants.js lib/http.js lib/index.js lib/mcp-client.js lib/sanitize.js && pnpm run check:types", "check:types": "tsc --noEmit --strict --module NodeNext --moduleResolution NodeNext --target ES2022 --skipLibCheck test/types.ts", "pack:dry-run": "npm pack --dry-run" } diff --git a/packages/core/test/core.test.js b/packages/core/test/core.test.js index 6fccdb9..6d5ff2f 100644 --- a/packages/core/test/core.test.js +++ b/packages/core/test/core.test.js @@ -25,6 +25,7 @@ import { writePrivateJson, } from "@call-e/core/cache"; import { + BrokerLoginError, createBrokerSession, ensurePendingLogin, loginWithBroker, @@ -54,6 +55,10 @@ function jsonResponse(body, { status = 200, statusText = "OK", headers = {} } = }; } +function jsonRpcResponse(request, outcome, options = {}) { + return jsonResponse({ jsonrpc: "2.0", id: request.id, ...outcome }, options); +} + function mcpConfig(cacheRoot) { const serverUrl = "https://example.test/mcp/openagent_oauth"; writePrivateJson(tokenCachePath(cacheRoot, serverUrl), { @@ -329,6 +334,47 @@ test("broker login exchanges active pending before reusing cached token", async ]); }); +test("broker terminal detail is typed and sanitized instead of entering Error.message", async () => { + const cacheRoot = makeTempRoot("calle-core-broker-terminal-error"); + const config = { + cacheRoot, + brokerBaseUrl: "https://broker.test", + serverUrl: "https://broker.test/mcp/openagent_oauth", + authBaseUrl: "https://broker.test", + channel: "openagent_oauth", + scope: "openid email profile", + clientName: "calle Login", + minTtlSeconds: 300, + timeoutSeconds: 15, + pollTimeoutSeconds: 1, + }; + const marker = "REMOTE-TEXT-MARKER access_token=abcd1234efgh5678"; + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init.method === "POST") { + return jsonResponse({ + session_id: "session-failed", + session_secret: "secret-failed", + login_url: "https://broker.test/start", + status: "PENDING", + poll_after_ms: 1, + }); + } + return jsonResponse({ status: "FAILED", error_message: marker }); + }; + + await assert.rejects( + () => loginWithBroker(config, { fetchImpl, noBrowserOpen: true, sleepImpl: async () => {} }), + (error) => { + assert.ok(error instanceof BrokerLoginError); + assert.equal(error.code, "broker_login_failed"); + assert.equal(error.message, "Brokered login failed."); + assert.deepEqual(error.remoteError, { code: "FAILED", message: "REMOTE-TEXT-MARKER access_token=[redacted]" }); + assert.doesNotMatch(error.message, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + return true; + }, + ); +}); + test("MCP client initializes a session and lists tools", async () => { const config = mcpConfig(makeTempRoot("calle-core-mcp-tools")); const calls = []; @@ -343,15 +389,15 @@ test("MCP client initializes a session and lists tools", async () => { if (payload.method === "initialize") { assert.deepEqual(payload.params.clientInfo, { name: "calle", version: "9.9.9" }); - return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-1" } }); + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-1" } }); } if (payload.method === "notifications/initialized") { assert.equal(init.headers["mcp-session-id"], "mcp-session-1"); - return jsonResponse({}); + return jsonResponse(undefined); } if (payload.method === "tools/list") { assert.equal(init.headers["mcp-session-id"], "mcp-session-1"); - return jsonResponse({ result: { tools: [{ name: "plan_call" }] } }); + return jsonRpcResponse(payload, { result: { tools: [{ name: "plan_call" }] } }); } throw new Error(`Unexpected MCP method ${payload.method}`); }; @@ -370,7 +416,7 @@ test("MCP client calls tools through an initialized session", async () => { const fetchImpl = async (_url, init) => { const payload = JSON.parse(init.body); if (payload.method === "initialize") { - return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-2" } }); + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-2" } }); } if (payload.method === "notifications/initialized") { return jsonResponse({}); @@ -380,7 +426,7 @@ test("MCP client calls tools through an initialized session", async () => { name: "plan_call", arguments: { goal: "Confirm the appointment" }, }); - return jsonResponse({ result: { content: [{ type: "text", text: "ok" }] } }); + return jsonRpcResponse(payload, { result: { content: [{ type: "text", text: "ok" }] } }); } throw new Error(`Unexpected MCP method ${payload.method}`); }; @@ -430,7 +476,7 @@ test("MCP client normalizes tool payloads without discarding the raw envelope", const fetchImpl = async (_url, init) => { const payload = JSON.parse(init.body); if (payload.method === "initialize") { - return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-payload" } }); + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-payload" } }); } if (payload.method === "notifications/initialized") { return jsonResponse({}); @@ -438,7 +484,7 @@ test("MCP client normalizes tool payloads without discarding the raw envelope", if (payload.method === "tools/call") { const result = toolResults[toolCallIndex]; toolCallIndex += 1; - return jsonResponse({ result }); + return jsonRpcResponse(payload, { result }); } throw new Error(`Unexpected MCP method ${payload.method}`); }; @@ -464,7 +510,7 @@ test("MCP client forwards request meta on tool calls", async () => { const fetchImpl = async (_url, init) => { const payload = JSON.parse(init.body); if (payload.method === "initialize") { - return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-2" } }); + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-2" } }); } if (payload.method === "notifications/initialized") { return jsonResponse({}); @@ -478,7 +524,7 @@ test("MCP client forwards request meta on tool calls", async () => { timezone_offset_minutes: -480, }, }); - return jsonResponse({ result: { content: [{ type: "text", text: "ok" }] } }); + return jsonRpcResponse(payload, { result: { content: [{ type: "text", text: "ok" }] } }); } throw new Error(`Unexpected MCP method ${payload.method}`); }; @@ -516,12 +562,12 @@ test("MCP client exposes MCP error payloads", async () => { const fetchImpl = async (_url, init) => { const payload = JSON.parse(init.body); if (payload.method === "initialize") { - return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-3" } }); + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-3" } }); } if (payload.method === "notifications/initialized") { return jsonResponse({}); } - return jsonResponse({ error: { code: -32000, message: "remote failure" } }); + return jsonRpcResponse(payload, { error: { code: -32000, message: "remote failure" } }); }; await assert.rejects( @@ -547,9 +593,704 @@ test("MCP client reports request timeouts", async () => { () => listMcpTools({ config, fetchImpl }), (error) => { assert.ok(error instanceof McpHttpError); - assert.equal(error.code, "http_error"); + assert.equal(error.code, "transport_error"); + assert.equal(error.transport, true); + assert.equal(error.timedOut, true); assert.match(error.message, /timed out/i); return true; }, ); }); + +test("MCP client classifies a rejected fetch as transport, and keeps the server message out of Error.message", async () => { + const config = mcpConfig(makeTempRoot("calle-core-mcp-rejected")); + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + await assert.rejects( + () => listMcpTools({ config, fetchImpl: async () => { throw dns; } }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "transport_error"); + assert.equal(error.transport, true); + assert.equal(error.timedOut, false); + assert.equal(error.causeCode, "ENOTFOUND"); + return true; + }, + ); + + const hostileConfig = mcpConfig(makeTempRoot("calle-core-mcp-hostile")); + const ESC = String.fromCharCode(27); + const hostileMessage = `${"x".repeat(2000)}${ESC}[31m secret=sk_live_ABCDEFGHIJKLMNOPQRSTUV`; + const fetchImpl = async (_url, init) => { + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-9" } }); + } + if (payload.method === "notifications/initialized") { + return jsonResponse({}); + } + return jsonRpcResponse(payload, { error: { code: -32000, message: hostileMessage, access_token: "tok_SECRET_VALUE_123456" } }); + }; + await assert.rejects( + () => callMcpTool({ config: hostileConfig, toolName: "plan_call", fetchImpl }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "mcp_error"); + assert.equal(error.message, "Remote MCP error for tools/call", "Error.message is authored locally"); + assert.ok(error.remoteError.message.length <= 500); + assert.doesNotMatch(error.remoteError.message, /sk_live_|tok_SECRET/u); + assert.equal(error.remoteError.message.includes(ESC), false); + assert.equal(error.remoteError.code, "-32000"); + return true; + }, + ); +}); + +test("successful MCP statuses with invalid JSON-RPC bodies are typed invalid responses", async () => { + const marker = "REMOTE-TEXT-MARKER access_token=abcd1234efgh5678"; + const bodies = [ + marker, + JSON.stringify([marker]), + JSON.stringify(null), + JSON.stringify({}), + JSON.stringify({ error: null }), + JSON.stringify({ error: "not-an-error-object" }), + "", + ]; + + for (const responseText of bodies) { + const config = mcpConfig(makeTempRoot("calle-core-mcp-invalid-response")); + const fetchImpl = async () => ({ + ok: true, + status: 200, + statusText: "OK", + headers: new Headers({ "content-type": "application/json" }), + async text() { return responseText; }, + }); + + await assert.rejects( + () => listMcpTools({ config, fetchImpl }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "invalid_response"); + assert.equal(error.statusCode, 200); + assert.equal(error.message, "MCP response was invalid for initialize"); + assert.doesNotMatch(error.message, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + if (error.remoteError?.message) { + assert.doesNotMatch(error.remoteError.message, /abcd1234|efgh5678/u); + } + return true; + }, + ); + } +}); + +test("MCP responses require version 2.0, the request id, and exactly one valid outcome", async () => { + const invalidResponses = [ + { name: "wrong protocol version", body: { jsonrpc: "1.0", id: "calle-initialize", result: {} } }, + { name: "stale request id", body: { jsonrpc: "2.0", id: "stale-request", result: {} } }, + { name: "missing request id", body: { jsonrpc: "2.0", result: {} } }, + { + name: "both result and error", + body: { + jsonrpc: "2.0", + id: "calle-initialize", + result: {}, + error: { code: -32000, message: "must not coexist" }, + }, + }, + { + name: "error without an integer code", + body: { jsonrpc: "2.0", id: "calle-initialize", error: { code: "-32000", message: "bad" } }, + }, + { + name: "error without a message", + body: { jsonrpc: "2.0", id: "calle-initialize", error: { code: -32000 } }, + }, + ]; + + for (const fixture of invalidResponses) { + const config = mcpConfig(makeTempRoot("calle-core-mcp-invalid-envelope")); + await assert.rejects( + () => listMcpTools({ config, fetchImpl: async () => jsonResponse(fixture.body) }), + (error) => { + assert.ok(error instanceof McpHttpError, fixture.name); + assert.equal(error.code, "invalid_response", fixture.name); + assert.equal(error.message, "MCP response was invalid for initialize", fixture.name); + return true; + }, + ); + } +}); + +test("MCP notifications accept empty acknowledgements but reject response envelopes", async () => { + const config = mcpConfig(makeTempRoot("calle-core-mcp-notification-ack")); + const fetchImpl = async (_url, init) => { + const request = JSON.parse(init.body); + if (request.method === "initialize") { + return jsonRpcResponse(request, { result: {} }, { headers: { "mcp-session-id": "s" } }); + } + return jsonResponse({ jsonrpc: "2.0", id: "stale-request", result: {} }); + }; + + await assert.rejects( + () => listMcpTools({ config, fetchImpl }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "invalid_response"); + assert.equal(error.message, "MCP response was invalid for notifications/initialized"); + return true; + }, + ); +}); + +test("HTTP status reason text and body never reach the core error message", async () => { + const { requestJson, HttpStatusError } = await import("@call-e/core/http"); + const marker = "REMOTE-TEXT-MARKER access_token=abcd1234efgh5678"; + + await assert.rejects( + () => requestJson("POST", "https://example.test/thing", { + fetchImpl: async () => ({ + ok: false, + status: 502, + statusText: marker, + headers: new Headers(), + async text() { return marker; }, + }), + }), + (error) => { + assert.ok(error instanceof HttpStatusError); + assert.equal(error.message, "HTTP 502 for POST https://example.test/thing"); + assert.doesNotMatch(error.message, /REMOTE-TEXT-MARKER|abcd1234|efgh5678/u); + assert.equal(error.responseText, marker, "raw detail remains available for sanitizing"); + return true; + }, + ); +}); + +test("sanitize helpers strip terminal controls, redact secrets, and bound length", async () => { + const { safeRemoteString, safeRemoteCode, stripTerminalControls, redactSecrets, sanitizeRemoteError } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(27); + const BEL = String.fromCharCode(7); + const controlled = `a${ESC}[2J${ESC}]8;;http://x${BEL}link${ESC}]8;;${BEL}b\r\nc`; + const cleaned = safeRemoteString(controlled); + assert.equal(cleaned.includes(ESC), false); + assert.doesNotMatch(cleaned, /[\r\n]/u); + assert.equal(stripTerminalControls(controlled), "alinkbc", "controls are removed, not spaced"); + assert.equal(cleaned, "[redacted]", "ambiguous controlled text is withheld from display"); + + // A bound on ordinary prose. (An unbroken 10,000-character run is redacted as an opaque + // token instead, which is the intended behaviour and is asserted below.) + assert.equal(safeRemoteString("word ".repeat(3000)).length, 500); + assert.equal(safeRemoteString("x".repeat(10_000)), "[redacted]"); + assert.equal(safeRemoteString(" "), undefined); + assert.equal(safeRemoteString(42), undefined); + + const bearer = redactSecrets("Authorization: Bearer abcdefghijklmnopqrstuvwxyz"); + assert.doesNotMatch(bearer, /abcdefghijklmnopqrstuvwxyz/u); + assert.match(bearer, /^Authorization: .*\[redacted\]/u); + assert.match(redactSecrets("Bearer abcdefghijklmnopqrstuvwxyz"), /^Bearer \[redacted\]$/u); + assert.match(redactSecrets("access_token=abcd1234efgh"), /access_token=\[redacted\]/u); + assert.match(redactSecrets("key sk_live_ABCDEFGHIJKLMNOP1234 here"), /key \[redacted\] here/u); + assert.match(redactSecrets("hash 0123456789abcdef0123456789abcdef0123"), /hash \[redacted\]/u); + assert.equal(redactSecrets("Failed to register an OAuth client. err_type=HTTPStatusError"), "Failed to register an OAuth client. err_type=HTTPStatusError"); + + assert.equal(safeRemoteCode("oauth_register_failed"), "oauth_register_failed"); + assert.equal(safeRemoteCode("nested.code-1"), "nested.code-1"); + assert.equal(safeRemoteCode(" oauth_register_failed "), undefined); + assert.equal(safeRemoteCode(`bad code${ESC}[31m`), undefined); + assert.equal(safeRemoteCode("x".repeat(65)), undefined); + + assert.deepEqual(sanitizeRemoteError({ error: "auth_required", message: "please" }), { code: "auth_required", message: "please" }); + assert.deepEqual(sanitizeRemoteError({ error: { code: "n.1", message: "m", details: { internal: "t" } }, request_id: "r" }), { code: "n.1", message: "m" }); + assert.deepEqual(sanitizeRemoteError("gateway"), { message: "gateway" }); + assert.equal(sanitizeRemoteError(""), null); + assert.equal(sanitizeRemoteError({ unrelated: true }), null); +}); + +test("a control sequence inserted inside a credential cannot split it past the redactor", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(27); + const BEL = String.fromCharCode(7); + const NUL = String.fromCharCode(0); + const inserts = [ + `${ESC}[31m`, // CSI colour + `${ESC}[2J${ESC}[H`, // CSI erase + home + `${ESC}]8;;http://x${BEL}`, // OSC hyperlink + `${ESC}M`, // two-character ESC sequence + NUL, // C0 + "\r\n", // CR LF + String.fromCharCode(0x9b), // C1 + ]; + const secrets = [ + { text: "Bearer abcdefghijklmnopqrstuvwxyz012345", halves: ["abcdefghijkl", "mnopqrstuvwxyz012345"] }, + { text: "Basic YWxhZGRpbjpvcGVuc2VzYW1l", halves: ["YWxhZGRp", "bjpvcGVuc2VzYW1l"] }, + { text: "access_token=abcd1234efgh5678", halves: ["abcd1234", "efgh5678"] }, + { text: 'api_key: "QWERTYUIOP12345678"', halves: ["QWERTYUI", "OP12345678"] }, + { text: "sk_live_ABCDEFGHIJKLMNOPQRSTUV", halves: ["ABCDEFGHIJ", "KLMNOPQRSTUV"] }, + { text: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", halves: ["abcdefghijklmnop", "qrstuvwxyz0123456789"] }, + { text: "0123456789abcdef0123456789abcdef01234567", halves: ["0123456789abcdef", "0123456789abcdef01234567"] }, + ]; + for (const secret of secrets) { + for (const insert of inserts) { + // Insert the control sequence at several points, including inside the key name and + // right after the separator, not only in the middle of the value. + const points = [Math.floor(secret.text.length / 2), secret.text.indexOf("=") + 1, secret.text.indexOf(" ") + 1, 3]; + for (const at of points) { + if (at <= 0) continue; + const hostile = `${secret.text.slice(0, at)}${insert}${secret.text.slice(at)}`; + const out = safeRemoteString(`context ${hostile} more`); + assert.equal(out.includes(ESC), false); + for (const half of secret.halves) { + assert.equal(out.includes(half), false, `fragment ${JSON.stringify(half)} survived in ${JSON.stringify(out)} for ${JSON.stringify(hostile)}`); + } + } + } + } +}); + +test("8-bit C1 introducers and invisible format characters cannot smuggle a credential", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const BEL = String.fromCharCode(0x07); + const CSI8 = String.fromCharCode(0x9b); + const OSC8 = String.fromCharCode(0x9d); + const ST8 = String.fromCharCode(0x9c); + + // Every one of these is invisible or terminal-consumed, and each splits a token in a way a + // character-at-a-time strip would preserve. + const inserts = [ + CSI8 + "31m", // 8-bit CSI + OSC8 + "8;;http://x" + BEL, // 8-bit OSC, BEL-terminated + OSC8 + "0;title" + ST8, // 8-bit OSC, ST-terminated + ESC + "[2J", // 7-bit CSI + "​", // zero width space + "⁠", // word joiner + "‍", // zero width joiner + "‮", // right-to-left override + "⁦", // left-to-right isolate + "­", // soft hyphen + "", // BOM + "\r\n", + ]; + + const secrets = [ + { text: "Bearer abcdefghijklmnopqrstuvwxyz012345", fragments: ["abcdefghijkl", "qrstuvwxyz012345"] }, + { text: "access_token=abcd1234efgh5678", fragments: ["abcd1234", "efgh5678"] }, + { text: "sk_live_ABCDEFGHIJKLMNOPQRSTUV", fragments: ["ABCDEFGHIJ", "KLMNOPQRSTUV"] }, + { text: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", fragments: ["abcdefghijklmnop", "qrstuvwxyz0123456789"] }, + { text: "A".repeat(20) + "B".repeat(20), fragments: ["A".repeat(20), "B".repeat(20)] }, + ]; + + for (const secret of secrets) { + for (const insert of inserts) { + for (let at = 1; at < secret.text.length; at += 3) { + const hostile = `context ${secret.text.slice(0, at)}${insert}${secret.text.slice(at)} more`; + const out = safeRemoteString(hostile); + for (const fragment of secret.fragments) { + assert.equal( + out.includes(fragment), + false, + `fragment ${JSON.stringify(fragment)} survived as ${JSON.stringify(out)} for insert ${JSON.stringify(insert)} at ${at}`, + ); + } + } + } + } +}); + +test("every terminal string-control family is consumed with its payload", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const BEL = String.fromCharCode(0x07); + const BACKSLASH = String.fromCharCode(0x5c); + const CSI8 = String.fromCharCode(0x9b); + const ST8 = String.fromCharCode(0x9c); + const OSC8 = String.fromCharCode(0x9d); + const DCS8 = String.fromCharCode(0x90); + const SOS8 = String.fromCharCode(0x98); + const PM8 = String.fromCharCode(0x9e); + const APC8 = String.fromCharCode(0x9f); + + // Introducers in both forms. Stripping only the introducer leaves the payload as ordinary + // text, which is what breaks a key name apart and lets the credential through. + const introducers = [ + ["OSC 7-bit", `${ESC}]`], ["OSC 8-bit", OSC8], + ["DCS 7-bit", `${ESC}P`], ["DCS 8-bit", DCS8], + ["SOS 7-bit", `${ESC}X`], ["SOS 8-bit", SOS8], + ["PM 7-bit", `${ESC}^`], ["PM 8-bit", PM8], + ["APC 7-bit", `${ESC}_`], ["APC 8-bit", APC8], + ]; + const terminators = [["BEL", BEL], ["ESC backslash", `${ESC}${BACKSLASH}`], ["ST 8-bit", ST8], ["unterminated", ""]]; + + const secrets = [ + { text: "access_token=abcd1234efgh5678", fragments: ["abcd1234", "efgh5678"] }, + { text: "Bearer abcdefghijklmnopqrstuvwxyz012345", fragments: ["abcdefghijkl", "qrstuvwxyz012345"] }, + { text: "sk_live_ABCDEFGHIJKLMNOPQRSTUV", fragments: ["ABCDEFGHIJ", "KLMNOPQRSTUV"] }, + ]; + + for (const [introName, intro] of introducers) { + for (const [termName, term] of terminators) { + for (const secret of secrets) { + for (let at = 1; at < secret.text.length; at += 4) { + const hostile = `${secret.text.slice(0, at)}${intro}junk${term}${secret.text.slice(at)}`; + const out = safeRemoteString(`context ${hostile} more`); + const visible = out.replace(/\[redacted\]/gu, ""); + for (const fragment of secret.fragments) { + assert.equal( + visible.includes(fragment), + false, + `${introName} + ${termName} at ${at}: ${JSON.stringify(fragment)} survived as ${JSON.stringify(out)}`, + ); + } + } + } + } + } + + // An unterminated CSI must not leave its parameters behind either. + const csi = safeRemoteString(`access_to${CSI8}12345 token=abcd1234efgh5678`); + assert.doesNotMatch(csi.replace(/\[redacted\]/gu, ""), /abcd1234|efgh5678/u); +}); + +test("embedded ESC payloads and Unicode line separators cannot split credentials", async () => { + const { safeRemoteString, stripTerminalControls } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const BEL = String.fromCharCode(0x07); + const ST8 = String.fromCharCode(0x9c); + const introducers = [ + `${ESC}]`, String.fromCharCode(0x9d), + `${ESC}P`, String.fromCharCode(0x90), + `${ESC}X`, String.fromCharCode(0x98), + `${ESC}^`, String.fromCharCode(0x9e), + `${ESC}_`, String.fromCharCode(0x9f), + ]; + + // An ESC sequence inside the payload used to make the outer string-control regex give up. + // Its `junk...more` payload then survived and split the sensitive key name in both readings. + for (const [index, intro] of introducers.entries()) { + const terminators = index < 2 ? [BEL, `${ESC}\\`, ST8] : [`${ESC}\\`, ST8]; + for (const terminator of terminators) { + const hostile = `access_to${intro}junk${ESC}[31mmore${terminator}ken=abcd1234efgh5678`; + const stripped = stripTerminalControls(hostile); + const out = safeRemoteString(hostile); + assert.equal(stripped, "access_token=abcd1234efgh5678"); + assert.doesNotMatch(out.replace(/\[redacted\]/gu, ""), /abcd1234|efgh5678/u); + } + } + + // BEL terminates OSC only. For the other four string families it is payload, so stripping + // must continue through the later ST instead of stranding the text between BEL and ST. + for (const intro of introducers.slice(2)) { + for (const terminator of [`${ESC}\\`, ST8]) { + const hostile = `access_to${intro}junk${BEL}more${terminator}ken=abcd1234efgh5678`; + assert.equal(stripTerminalControls(hostile), "access_token=abcd1234efgh5678"); + assert.doesNotMatch(safeRemoteString(hostile).replace(/\[redacted\]/gu, ""), /abcd1234|efgh5678/u); + } + } + + // These are line controls even though they sit outside the C0/C1 ranges. Keeping either + // one lets the key/value pattern redact only the first half and publish the tail on a new + // visual line. + for (const separator of ["\u2028", "\u2029"]) { + const hostile = `access_token=abcd1234${separator}efgh5678`; + assert.equal(stripTerminalControls(hostile), "access_token=abcd1234efgh5678"); + assert.equal(safeRemoteString(hostile), "access_token=[redacted]"); + } + + // ECMA-35 has private/standardized and multi-byte escape forms outside ESC @ through + // ESC _. A CSI parser also remains active across embedded C0 controls. + const NUL = String.fromCharCode(0x00); + const CSI8 = String.fromCharCode(0x9b); + for (const sequence of [`${ESC}7`, `${ESC}=`, `${ESC}c`, `${ESC}(B`, `${ESC}[1${NUL}2m`, `${CSI8}1${NUL}2m`]) { + const hostile = `access_to${sequence}ken=abcd1234efgh5678`; + assert.equal(stripTerminalControls(hostile), "access_token=abcd1234efgh5678"); + assert.doesNotMatch(safeRemoteString(hostile).replace(/\[redacted\]/gu, ""), /abcd1234|efgh5678/u); + } +}); + +test("identical credentials crossed between the readings cannot compare equal", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const CSI8 = String.fromCharCode(0x9b); + const OSC8 = String.fromCharCode(0x9d); + const ST8 = String.fromCharCode(0x9c); + const bearer = "Bearer abcdefghijklmnopqrstuvwxyz012345"; + + // Two *identical* credentials. The OSC copy is only recognisable once the sequence has been + // consumed; the CSI copy only survives the character-only reading, because consuming the + // sequence eats the "r" of "Bearer". Findings therefore compare equal string-for-string. + const seenByDisplay = `Bea${OSC8}x${ST8}rer abcdefghijklmnopqrstuvwxyz012345`; + const seenByAlternate = `Bea${CSI8}rer abcdefghijklmnopqrstuvwxyz012345`; + + for (const text of [ + `${seenByDisplay} and ${seenByAlternate}`, + `${seenByAlternate} and ${seenByDisplay}`, + `${seenByDisplay} and ${seenByAlternate} and ${bearer}`, + ]) { + const out = safeRemoteString(text); + assert.equal(out, "[redacted]", "disagreeing readings cost the whole string"); + assert.doesNotMatch(out.replace(/\[redacted\]/gu, ""), /abcdefghijkl/u); + } +}); + +test("mixed terminal sequences cannot evade both global canonical readings", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const CSI8 = String.fromCharCode(0x9b); + const ST8 = String.fromCharCode(0x9c); + const OSC8 = String.fromCharCode(0x9d); + const DCS8 = String.fromCharCode(0x90); + const secret = "abcd1234efgh5678"; + const mixed = [ + `access_to${OSC8}junk${ST8}k${CSI8}en=${secret}`, + `access_to${DCS8}junk${ST8}k${CSI8}en=${secret}`, + `access_to${ESC}]junk${ESC}\\k${ESC}[en=${secret}`, + `access_to${ESC}Pjunk${ESC}\\k${ESC}[en=${secret}`, + ]; + + for (const hostile of mixed) { + const out = safeRemoteString(hostile); + assert.equal(out, "[redacted]", `ambiguous controlled text was shown: ${JSON.stringify(out)}`); + assert.doesNotMatch(out, /abcd1234|efgh5678/u); + } +}); + +test("credentials crossed between the two readings cannot ride out on an equal count", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const BEL = String.fromCharCode(0x07); + const CSI8 = String.fromCharCode(0x9b); + const OSC8 = String.fromCharCode(0x9d); + + // One credential each reading can see, so both find exactly one and the counts tie. + // Keeping the displayed form on that tie publishes the one only the other reading saw. + const seenInDisplay = `access_to${OSC8}8;;x${BEL}ken=abcd1234efgh5678`; + const seenInAlternate = `Bea${CSI8}rer abcdefghijklmnopqrstuvwxyz012345`; + + for (const [first, second] of [[seenInDisplay, seenInAlternate], [seenInAlternate, seenInDisplay]]) { + const out = safeRemoteString(`${first} and ${second}`); + assert.equal(out, "[redacted]", "disagreeing readings cost the whole string"); + for (const fragment of ["abcdefghijkl", "qrstuvwxyz012345", "abcd1234", "efgh5678"]) { + assert.equal(out.includes(fragment), false, `fragment ${fragment} survived`); + } + } + + // Three secrets, two readings, still no partial publication. + const triple = `${seenInDisplay} then ${seenInAlternate} then sk_live_ABCDEFGHIJ${ESC}[0mKLMNOPQRSTUV`; + const out = safeRemoteString(triple); + assert.doesNotMatch(out, /abcdefghijkl|abcd1234|KLMNOPQRSTUV/u); +}); + +test("a second credential visible only in the alternate reading is not published", async () => { + const { safeRemoteString } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(0x1b); + const CSI8 = String.fromCharCode(0x9b); + + // The first credential is caught in the displayed reading, so a test of "did we redact + // anything" passes on its strength alone. The second is only recognisable in the + // character-only reading, and would ride out on the back of the first. + const caughtInDisplay = `access_token=abcd1234efgh${ESC}[31m5678`; + const caughtOnlyInAlternate = `Bea${CSI8}rer abcdefghijklmnopqrstuvwxyz012345`; + const out = safeRemoteString(`${caughtInDisplay} and ${caughtOnlyInAlternate}`); + + assert.doesNotMatch(out, /abcdefghijkl/u, "the bearer token must not survive"); + assert.doesNotMatch(out, /abcd1234|efgh5678/u); + assert.equal(out, "[redacted]", "when the readings disagree in count, the whole string goes"); + + // The ordinary case must not be over-redacted into uselessness. + assert.equal(safeRemoteString("Claim 4471 was paid on August 12."), "Claim 4471 was paid on August 12."); + assert.equal( + safeRemoteString("Failed to register an OAuth client. err_type=HTTPStatusError"), + "Failed to register an OAuth client. err_type=HTTPStatusError", + ); +}); + +test("an MCP transport failure names its phase, as the published contract promises", async () => { + const config = mcpConfig(makeTempRoot("calle-core-mcp-phase")); + + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + await assert.rejects( + () => listMcpTools({ config, fetchImpl: async () => { throw dns; } }), + (error) => { + assert.equal(error.transport, true); + assert.equal(error.phase, "connect", "nothing arrived"); + return true; + }, + ); + + const reset = Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }); + const afterHeaders = async (_url, init) => { + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "s" } }); + } + if (payload.method === "notifications/initialized") return jsonResponse({}); + return { + ok: true, status: 200, statusText: "OK", headers: new Headers(), + async text() { throw reset; }, + }; + }; + await assert.rejects( + () => callMcpTool({ config: mcpConfig(makeTempRoot("calle-core-mcp-phase-body")), toolName: "plan_call", fetchImpl: afterHeaders }), + (error) => { + assert.equal(error.transport, true); + assert.equal(error.phase, "body", "headers arrived, the stream did not finish"); + return true; + }, + ); + + // A non-transport error must not claim a phase at all. + const rpcError = async (_url, init) => { + const payload = JSON.parse(init.body); + if (payload.method === "initialize") return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "s" } }); + if (payload.method === "notifications/initialized") return jsonResponse({}); + return jsonRpcResponse(payload, { error: { code: -32000, message: "nope" } }); + }; + await assert.rejects( + () => callMcpTool({ config: mcpConfig(makeTempRoot("calle-core-mcp-phase-none")), toolName: "plan_call", fetchImpl: rpcError }), + (error) => { + assert.equal(error.transport, false); + assert.equal(error.phase, null); + return true; + }, + ); +}); + +test("a body that is not JSON never reaches Error.message", async () => { + const { requestJson, InvalidResponseError } = await import("@call-e/core/http"); + const marker = "REMOTE-TEXT-MARKER sk_live_ABCDEFGHIJKLMNOP"; + + for (const body of [marker, `"${marker}"`, "[1,2,3]", "null"]) { + await assert.rejects( + () => requestJson("GET", "https://example.test/thing", { + fetchImpl: async () => ({ + ok: true, + status: 200, + statusText: "OK", + headers: new Headers(), + async text() { return body; }, + }), + }), + (error) => { + assert.ok(error instanceof InvalidResponseError, `body ${JSON.stringify(body)}`); + // JSON.parse quotes its input; this message must not. + assert.doesNotMatch(error.message, /REMOTE-TEXT-MARKER|sk_live_/u); + assert.match(error.message, /^Response body was not (valid JSON|a JSON object) for GET https:\/\/example\.test\/thing$/u); + assert.equal(error.statusCode, 200); + assert.equal(error.responseText, body, "the raw body is retained for sanitizing"); + return true; + }, + ); + } +}); + +test("a body stream that fails after headers is transport, and says which phase", async () => { + const { requestJson, TransportError } = await import("@call-e/core/http"); + const reset = Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }); + + await assert.rejects( + () => requestJson("GET", "https://example.test/x", { + fetchImpl: async () => ({ + ok: true, status: 200, statusText: "OK", headers: new Headers(), + async text() { throw reset; }, + }), + }), + (error) => { + assert.ok(error instanceof TransportError); + assert.equal(error.phase, "body"); + assert.equal(error.code, "ECONNRESET"); + return true; + }, + ); + + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + await assert.rejects( + () => requestJson("GET", "https://example.test/y", { fetchImpl: async () => { throw dns; } }), + (error) => { + assert.equal(error.phase, "connect", "nothing arrived, so the phase is connect"); + return true; + }, + ); +}); + +test("numeric remote codes are accepted only as safe integers", async () => { + const { safeRemoteCode, sanitizeRemoteError, publicRemoteError } = await import("@call-e/core/sanitize"); + assert.equal(safeRemoteCode(-32000), "-32000"); + assert.equal(safeRemoteCode(0), "0"); + assert.equal(safeRemoteCode(1e100), undefined); + assert.equal(safeRemoteCode(1.5), undefined); + assert.equal(safeRemoteCode(Number.NaN), undefined); + assert.equal(safeRemoteCode(Number.MAX_SAFE_INTEGER + 2), undefined); + assert.equal(safeRemoteCode("-abc"), "-abc"); + assert.equal(safeRemoteCode(" -abc "), undefined); + assert.equal(safeRemoteCode("1e+100"), undefined); + assert.deepEqual(sanitizeRemoteError({ error: { code: 1e100, message: "m" } }), { message: "m" }); + assert.deepEqual(publicRemoteError({ code: -32601, message: "x", extra: "dropped" }), { code: "-32601", message: "x" }); + assert.equal(publicRemoteError({ extra: "only" }), null); +}); + +function bodyFailingResponse(error, { status = 200, headers = {} } = {}) { + return { + ok: status >= 200 && status < 300, + status, + statusText: "OK", + headers: new Headers(headers), + async text() { + throw error; + }, + }; +} + +test("a body read that aborts or resets after headers is a typed transport failure", async () => { + const { requestJson, TransportError } = await import("@call-e/core/http"); + const aborted = new Error("aborted"); + aborted.name = "AbortError"; + await assert.rejects( + () => requestJson("GET", "https://example.test/slow", { fetchImpl: async () => bodyFailingResponse(aborted) }), + (error) => { + assert.ok(error instanceof TransportError); + assert.equal(error.timedOut, true); + assert.equal(error.code, "timeout"); + return true; + }, + ); + + const reset = new Error("socket hang up"); + reset.code = "ECONNRESET"; + await assert.rejects( + () => requestJson("GET", "https://example.test/reset", { fetchImpl: async () => bodyFailingResponse(reset) }), + (error) => { + assert.ok(error instanceof TransportError); + assert.equal(error.timedOut, false); + assert.equal(error.code, "ECONNRESET"); + assert.match(error.message, /Response body could not be read for GET https:\/\/example\.test\/reset/u); + return true; + }, + ); + + // Same through the MCP client, on the tools/call leg after a healthy initialize. + const config = mcpConfig(makeTempRoot("calle-core-mcp-body-reset")); + const fetchImpl = async (_url, init) => { + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonRpcResponse(payload, { result: {} }, { headers: { "mcp-session-id": "mcp-session-b" } }); + } + if (payload.method === "notifications/initialized") { + return jsonResponse({}); + } + return bodyFailingResponse(reset); + }; + await assert.rejects( + () => callMcpTool({ config, toolName: "plan_call", fetchImpl }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "transport_error"); + assert.equal(error.transport, true); + assert.equal(error.timedOut, false); + assert.equal(error.causeCode, "ECONNRESET"); + return true; + }, + ); +}); diff --git a/packages/core/test/types.ts b/packages/core/test/types.ts index 58e8356..7bda4b9 100644 --- a/packages/core/test/types.ts +++ b/packages/core/test/types.ts @@ -1,4 +1,5 @@ import { + BrokerLoginError, currentTokenDocument, loginWithBroker, tokenIsUsable, @@ -8,8 +9,23 @@ import { ensurePendingLogin } from "@call-e/core/broker-client"; import { readJson } from "@call-e/core/cache"; import { resolveServerUrl } from "@call-e/core/config"; import { DEFAULT_CHANNEL } from "@call-e/core/constants"; -import { requestJson } from "@call-e/core/http"; -import { callMcpTool, listMcpTools } from "@call-e/core/mcp-client"; +import { + HttpStatusError, + InvalidResponseError, + TransportError, + causeCodeOf, + requestJson, +} from "@call-e/core/http"; +import { McpHttpError, callMcpTool, listMcpTools } from "@call-e/core/mcp-client"; +import { + publicRemoteError, + redactSecrets, + safeRemoteCode, + safeRemoteString, + sanitizeRemoteError, + stripTerminalControls, + type SanitizedRemoteError, +} from "@call-e/core/sanitize"; const config: BrokerLoginConfig = { brokerBaseUrl: "https://example.test", @@ -61,6 +77,41 @@ async function consumePublicTypes() { const status = await requestJson<{ ok: boolean }>("GET", "https://example.test/status"); status.ok.valueOf(); + + try { + await requestJson("GET", "https://example.test/status"); + } catch (error) { + if (error instanceof BrokerLoginError) { + error.code.toUpperCase(); + error.remoteError?.message?.toUpperCase(); + } + if (error instanceof InvalidResponseError) { + error.responseText.toUpperCase(); + error.statusCode?.toFixed(); + } + if (error instanceof TransportError) { + error.phase.toUpperCase(); + error.timedOut.valueOf(); + error.code?.toUpperCase(); + error.url?.toUpperCase(); + } + if (error instanceof HttpStatusError) { + error.statusCode?.toFixed(); + error.url?.toUpperCase(); + } + if (error instanceof McpHttpError) { + error.transport.valueOf(); + error.causeCode?.toUpperCase(); + error.remoteError?.message?.toUpperCase(); + } + causeCodeOf(error)?.toUpperCase(); + } + + const shown: SanitizedRemoteError | null = publicRemoteError({ code: -32000, message: "x" }); + shown?.code?.toUpperCase(); + sanitizeRemoteError('{"error":"x"}')?.message?.toUpperCase(); + safeRemoteString(stripTerminalControls(redactSecrets("y")), 100)?.toUpperCase(); + safeRemoteCode(12)?.toUpperCase(); } void consumePublicTypes;