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..dbb8167 --- /dev/null +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -0,0 +1,68 @@ +--- +"@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? }`). +- `@call-e/core/http` adds `TransportError` (`url`, `method`, `timedOut`, `code`) and + `causeCodeOf`. `requestJson` throws it when `fetch` rejects, times out, or the response body + cannot be read. `HttpStatusError` now records `url`. +- `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"`. + +**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. +- 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 f159391..ad840a8 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -105,9 +105,15 @@ original confirmation context without printing it. If only the initial status query fails, the command still returns the accepted `run_id` and a `call status` `next_command`. -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 466e75b..d69e9e8 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). ## JSON Result Envelopes @@ -45,6 +47,83 @@ 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`. + +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 response was received | The request failed at the network layer: DNS, connection, TLS, or timeout. Absent otherwise — an unrelated local error is never described as a network condition. | +| `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_command` | `call` stage failures | Which stage failed and whether it is safe to retry. When `retry_safe` is `false`, run the returned `next_command` (`calle call recover …`) instead of starting a new call. | +| `help_command` | `invalid_arguments` only | A directly runnable `--help` command. | + +`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_command` 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 | The request never received a usable response: DNS, connection, TLS, a reset while reading the body, or a timeout outside a call stage. `transport: true`. Inside a `call` stage it also carries `stage`, `call_started`, and `retry_safe`. | +| `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_command` 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, 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 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; 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 4bf9c4c..8b099a8 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -23,6 +23,15 @@ import { resolveRuntimeConfig, } from "./config.js"; import { ensurePendingLogin, loginWithBroker } from "./broker-client.js"; +import { HttpStatusError, TransportError } from "./http.js"; +import { + REMOTE_MESSAGE_LIMIT, + publicRemoteError, + safeRemoteCode, + safeRemoteString, + sanitizeRemoteError, + stripTerminalControls, +} from "./sanitize.js"; import { AuthRequiredError, McpHttpError, @@ -57,8 +66,11 @@ class CallStageError extends McpHttpError { recoveryId = null, nextCommand = null, remoteError = null, + transport = false, + timedOut = false, + cause, }) { - super(message, { code, statusCode }); + super(message, { code, statusCode, transport, timedOut, ...(cause !== undefined ? { cause } : {}) }); this.name = "CallStageError"; this.stage = stage; this.callStarted = callStarted; @@ -855,12 +867,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, @@ -872,34 +891,170 @@ function errorPayload(error, config, helpCommand = null) { ...(error.nextCommand ? { next_command: error.nextCommand } : {}), } : {}), error: { - code: error.code || "mcp_error", - message: error.message, + code: classified.code, + message: localMessage(error.message), + status_code: error.statusCode, + ...(classified.transport ? { transport: true } : {}), + ...(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?.error_code !== undefined ? { error_code: remoteError.error_code } : {}), - ...(remoteError?.status !== undefined ? { status: remoteError.status } : {}), + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + + if (error instanceof TransportError) { + const causeCode = safeRemoteCode(error.code); + const summary = error.timedOut + ? `Request timed out for ${error.method ?? "request"} ${describeUrl(error.url)}.` + : `Request failed before a response was received for ${error.method ?? "request"} ${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, + ...(causeCode ? { cause_code: causeCode } : {}), }, }, }; } 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 }, + 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 }; + } + // 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; @@ -914,16 +1069,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) { @@ -1018,13 +1164,8 @@ 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) || {}; @@ -1062,23 +1203,36 @@ 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; + const message = timedOut + ? `${stage} timed out before the CLI received a response.` + : (transport + ? `${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, + ...(error?.cause !== undefined ? { cause: error.cause } : {}), } ); } @@ -1106,7 +1260,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, @@ -1217,7 +1372,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", @@ -1391,16 +1547,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 } } : {}), } ); } @@ -1716,10 +1876,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; @@ -1730,7 +1886,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 20f195f..81a6527 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", + "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", "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 6fcb880..c260fb5 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -370,6 +370,598 @@ 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 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.match( + payload.error.message, + /^Request failed before a response was received for POST 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 for POST 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 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); + assert.match(payload.error.remote_error.message, /^line oneinjected/u); + 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}`); + } +}); + +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.match(abortPayload.error.message, /^plan_call timed out before the CLI received a response\.$/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.match(resetPayload.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("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 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"; @@ -1484,8 +2076,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 () => { @@ -1658,7 +2251,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/); @@ -1719,7 +2314,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/core/README.md b/packages/core/README.md index 7524ef2..dbb91ff 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -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,51 @@ 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`, `code`. | +| `McpHttpError` | MCP client | HTTP failure (`code: "http_error"`), JSON-RPC error (`"mcp_error"`), or transport failure (`"transport_error"`). | + +`@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. + ## Development ```bash diff --git a/packages/core/lib/http.d.ts b/packages/core/lib/http.d.ts index 4a40fca..93ad520 100644 --- a/packages/core/lib/http.d.ts +++ b/packages/core/lib/http.d.ts @@ -4,6 +4,14 @@ export interface HttpStatusErrorOptions { statusCode?: number | null; responseText?: string; headers?: Record; + url?: string | null; +} + +export interface TransportErrorOptions { + url?: string | null; + method?: string | null; + timedOut?: boolean; + cause?: unknown; } export interface RequestJsonOptions { @@ -18,6 +26,20 @@ 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; + +/** The request never received an HTTP response (DNS, connection, TLS, or timeout). */ +export class TransportError extends Error { + constructor(message: string, options?: TransportErrorOptions); + url: string | null; + method: string | null; + timedOut: boolean; + /** "timeout", or the Node.js error code of the cause (e.g. "ENOTFOUND"), or null. */ + code: string | null; } export function requestJson( diff --git a/packages/core/lib/http.js b/packages/core/lib/http.js index bca9492..3936b5e 100644 --- a/packages/core/lib/http.js +++ b/packages/core/lib/http.js @@ -1,10 +1,41 @@ 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; +} + +/** + * The request never received an HTTP response: DNS failure, connection refused, TLS error, + * or the client-side timeout. 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 transport. + */ +export class TransportError extends Error { + constructor(message, { url, method, timedOut = false, cause } = {}) { + super(message, cause !== undefined ? { cause } : undefined); + this.name = "TransportError"; + this.url = url ?? null; + this.method = method ?? null; + this.timedOut = Boolean(timedOut); + this.code = timedOut ? "timeout" : causeCodeOf(cause); } } @@ -20,8 +51,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,12 +63,38 @@ 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 }); + } + throw new TransportError(`Response body could not be read for ${method} ${url}`, { url, method, cause: error }); + } + + try { if (!response.ok) { throw new HttpStatusError(`Client error '${response.status} ${response.statusText}' for url '${url}'`, { statusCode: response.status, responseText: text, headers: Object.fromEntries(response.headers.entries()), + url, }); } if (!text.trim()) { @@ -47,11 +105,6 @@ export async function requestJson(method, url, { headers = {}, json = undefined, throw new Error(`Expected JSON object response for ${method} ${url}`); } return parsed; - } catch (error) { - if (error?.name === "AbortError") { - throw new Error(`Request timed out for ${method} ${url}`); - } - throw error; } 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..84aa4c8 100644 --- a/packages/core/lib/mcp-client.d.ts +++ b/packages/core/lib/mcp-client.d.ts @@ -17,6 +17,9 @@ export interface McpHttpErrorOptions { payload?: unknown; headers?: Record; code?: string; + transport?: boolean; + timedOut?: boolean; + cause?: unknown; } export interface McpToolDefinition extends JsonObject { @@ -52,6 +55,13 @@ export class McpHttpError extends Error { payload: unknown; headers: Record; code: string; + /** True only when no HTTP response was received (timeout, DNS, connection, TLS). */ + transport: boolean; + timedOut: boolean; + /** "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..b9d0450 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,34 @@ 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, + 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); + /** "timeout", the system error code behind a rejected fetch, or null. */ + this.causeCode = timedOut ? "timeout" : causeCodeOf(cause); + this.remoteError = sanitizeRemoteError(payload ?? responseText); } } @@ -57,14 +78,53 @@ 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, + }); + } + // 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, + 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, + }); + } + throw new McpHttpError(`MCP response body could not be read for ${payload.method}`, { + code: "transport_error", + transport: true, + cause: error, + }); + } + + try { let body = null; try { body = parseResponseBody(text); @@ -83,20 +143,16 @@ 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, + // 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..e59e498 --- /dev/null +++ b/packages/core/lib/sanitize.js @@ -0,0 +1,162 @@ +/** + * 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 first (not replaced with a space), + * 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. Secret + * detection runs on that canonical text; only then is the result bounded. + * + * 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); + +// CSI (colours, cursor movement, erase), OSC (titles, hyperlinks, terminated by BEL or ESC \), +// two-character ESC sequences, and the C0 / DEL / C1 control ranges (covers CR, LF, TAB). +// The regex source is assembled from character codes so the file itself contains no control +// bytes and no escape sequence that a tool or editor could rewrite. +const LBRACKET = `${BACKSLASH}[`; +const RBRACKET = `${BACKSLASH}]`; +const TERMINAL_CONTROL_RE = new RegExp( + [ + `${ESC}${LBRACKET}[0-?]*[ -/]*[@-~]`, + `${ESC}${RBRACKET}[^${BEL}${ESC}]*(?:${BEL}|${ESC}${BACKSLASH}${BACKSLASH})`, + `${ESC}[@-_]`, + `[${C0_START}-${C0_END}${DEL}-${C1_END}]`, + ].join("|"), + "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, ""); +} + +/** 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 (canonicalized), secrets redacted + * on the canonical text, 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 canonical = stripTerminalControls(value); + const cleaned = redactSecrets(canonical).trim(); + if (!cleaned) { + return undefined; + } + return cleaned.slice(0, maxLength); +} + +/** + * A remote machine code kept as an opaque token. Strings are control-stripped and matched + * against the safe charset; numbers are accepted only as safe integers (so `-32000` is kept + * and `1e100`, `NaN`, or `1.5` are dropped). Anything else is dropped. + */ +export function safeRemoteCode(value) { + if (typeof value === "number") { + return Number.isSafeInteger(value) ? String(value) : undefined; + } + if (typeof value !== "string") { + return undefined; + } + const cleaned = stripTerminalControls(value).trim(); + return REMOTE_CODE_RE.test(cleaned) ? cleaned : 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..4078301 100644 --- a/packages/core/test/core.test.js +++ b/packages/core/test/core.test.js @@ -547,9 +547,212 @@ 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 jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-9" } }); + } + if (payload.method === "notifications/initialized") { + return jsonResponse({}); + } + return jsonResponse({ 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("sanitize helpers strip terminal controls, redact secrets, and bound length", async () => { + const { safeRemoteString, safeRemoteCode, redactSecrets, sanitizeRemoteError } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(27); + const BEL = String.fromCharCode(7); + const cleaned = safeRemoteString(`a${ESC}[2J${ESC}]8;;http://x${BEL}link${ESC}]8;;${BEL}b\r\nc`); + assert.equal(cleaned.includes(ESC), false); + assert.doesNotMatch(cleaned, /[\r\n]/u); + assert.equal(cleaned, "alinkbc", "controls are removed, not spaced, so nothing can be split"); + + // 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(`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("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("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 jsonResponse({ 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..a4b1c84 100644 --- a/packages/core/test/types.ts +++ b/packages/core/test/types.ts @@ -8,8 +8,17 @@ 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, 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 +70,32 @@ 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 TransportError) { + 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;