From 359ee27df11e70ba5cde1d294e68b3765334a183 Mon Sep 17 00:00:00 2001 From: N DIVIJ Date: Fri, 4 Sep 2026 12:47:38 +0530 Subject: [PATCH 1/2] fix(cli): emit the JSON error envelope for transport failures runCli rethrew every error that was not an InvalidArgumentsError, so an HTTP or transport failure escaped to main() and printed a bare message on stderr with nothing on stdout. The skill instructs agent hosts to treat all command output as JSON, so a failed `auth login` gave them an empty stdout and no error.code to branch on. Route every failure through writeCommandError, and give HttpStatusError its own payload branch carrying status_code, the parsed upstream remote_error, and a bounded fallback for non-JSON bodies. A 5xx from the brokered-login registration endpoint now also says the failure is server-side and that the Developer API path does not depend on it, instead of leaving users to guess that reinstalling the CLI might help. Adds coverage for both the JSON and the non-JSON upstream error body. --- ...n-error-envelope-for-transport-failures.md | 38 ++++++++++ packages/cli/lib/cli.js | 76 +++++++++++++++++-- packages/cli/test/cli.test.js | 75 ++++++++++++++++++ 3 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 .changeset/json-error-envelope-for-transport-failures.md 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..0756bf2 --- /dev/null +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -0,0 +1,38 @@ +--- +"@call-e/cli": patch +--- + +Always emit the documented JSON error envelope, and surface the upstream error body. + +`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. + +Failures now leave through `writeCommandError` like any other error, and `HttpStatusError` +gains its own payload branch carrying `status_code`, the parsed upstream `remote_error`, and +a bounded fallback for non-JSON bodies. When the brokered-login registration endpoint returns +5xx, the message also states that the failure is server-side and that the Developer API path +does not depend on it — the previous output invited users to reinstall the CLI instead. + +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": "oauth_register_failed", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client. err_type=HTTPStatusError" + } + } +} +``` diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 4bf9c4c..6c5e06c 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -23,6 +23,7 @@ import { resolveRuntimeConfig, } from "./config.js"; import { ensurePendingLogin, loginWithBroker } from "./broker-client.js"; +import { HttpStatusError } from "./http.js"; import { AuthRequiredError, McpHttpError, @@ -882,6 +883,34 @@ function errorPayload(error, config, helpCommand = null) { }; } + if (error instanceof HttpStatusError) { + const remoteError = parseRemoteErrorBody(error.responseText); + const brokerUnavailable = isBrokerRegistrationFailure(error); + const messageParts = [error.message]; + if (remoteError?.message) { + messageParts.push(remoteError.message); + } + if (brokerUnavailable) { + messageParts.push( + "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." + ); + } + return { + exitCode: 1, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: remoteError?.code + || (brokerUnavailable ? "broker_unavailable" : "http_error"), + message: messageParts.join(" "), + status_code: error.statusCode, + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + return { exitCode: 1, body: { @@ -895,6 +924,41 @@ function errorPayload(error, config, helpCommand = null) { }; } +const REMOTE_ERROR_BODY_LIMIT = 500; + +function parseRemoteErrorBody(responseText) { + const text = String(responseText ?? "").trim(); + if (!text) { + return null; + } + let parsed; + try { + parsed = JSON.parse(text); + } catch { + return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + } + if (!parsed || typeof parsed !== "object") { + return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + } + const source = parsed.error && typeof parsed.error === "object" ? parsed.error : parsed; + const code = typeof source.code === "string" + ? source.code + : (typeof parsed.error === "string" ? parsed.error : undefined); + const message = typeof source.message === "string" ? source.message : undefined; + if (!code && !message) { + return null; + } + return { + ...(code ? { code } : {}), + ...(message ? { message } : {}), + }; +} + +function isBrokerRegistrationFailure(error) { + return Number(error?.statusCode) >= 500 + && /\/api\/v1\/openagent-auth\/sessions/u.test(String(error?.message ?? "")); +} + function writeCommandError(stdout, stderr, error, config, helpCommand = null) { const formatted = errorPayload(error, config, helpCommand); writeJson(stdout, formatted.body); @@ -1716,10 +1780,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 +1790,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/test/cli.test.js b/packages/cli/test/cli.test.js index 6fcb880..4fa90d0 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -370,6 +370,81 @@ 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, "oauth_register_failed"); + assert.deepEqual(payload.error.remote_error, { + code: "oauth_register_failed", + message: "Failed to register an OAuth client. err_type=HTTPStatusError", + }); + assert.match(payload.error.message, /Failed to register an OAuth client/); + assert.match(payload.error.message, /login service is unavailable/); + assert.match(payload.error.message, /dashboard API key/); + assert.match(result.stderr, /Failed to register an OAuth client/); +}); + +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); +}); + 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"; From f57ed621a1ba55761e882bd110f2596f144622d8 Mon Sep 17 00:00:00 2001 From: N DIVIJ Date: Sat, 5 Sep 2026 20:10:24 +0530 Subject: [PATCH 2/2] fix(cli): keep error.code CLI-owned and sanitize upstream error bodies Address review on the JSON error envelope: - error.code is never taken from an upstream response. HTTP failures use broker_unavailable or http_error; a rejected or timed-out fetch uses transport_error. An upstream body can no longer impersonate a stable local code such as auth_required. - Upstream detail lives only under error.remote_error, reduced to code and message (top-level or nested under `error`); every other field is dropped unread so token-like values cannot reach stdout or stderr. - One sanitizer for all remote-derived strings: safeRemoteString now strips ANSI CSI/OSC/ESC sequences and C0/C1 controls before bounding, and safeRemoteCode constrains machine codes to [A-Za-z0-9_.:-]{1,64}. writeCommandError strips controls again before writing stderr. - Add regressions: forged auth_required, 20 KB message, nested error object with internal fields, CR/LF/ANSI content, secret-like fields absent from stdout and stderr, unsafe code dropped, fetch rejecting with TypeError/ENOTFOUND, and a request timeout. - Document the error envelope and its stable fields in cli-reference.md and README.md, replacing the statement that some failures print plain stderr. Correct the changeset to describe the sanitization. --- ...n-error-envelope-for-transport-failures.md | 21 ++- packages/cli/README.md | 9 +- packages/cli/docs/cli-reference.md | 57 +++++++- packages/cli/lib/cli.js | 134 ++++++++++++++---- packages/cli/test/cli.test.js | 127 ++++++++++++++++- 5 files changed, 307 insertions(+), 41 deletions(-) diff --git a/.changeset/json-error-envelope-for-transport-failures.md b/.changeset/json-error-envelope-for-transport-failures.md index 0756bf2..b0420b1 100644 --- a/.changeset/json-error-envelope-for-transport-failures.md +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -2,18 +2,23 @@ "@call-e/cli": patch --- -Always emit the documented JSON error envelope, and surface the upstream error body. +Always emit the documented JSON error envelope, and surface upstream error detail safely. `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. -Failures now leave through `writeCommandError` like any other error, and `HttpStatusError` -gains its own payload branch carrying `status_code`, the parsed upstream `remote_error`, and -a bounded fallback for non-JSON bodies. When the brokered-login registration endpoint returns -5xx, the message also states that the failure is server-side and that the Developer API path -does not depend on it — the previous output invited users to reinstall the CLI instead. +Every failure now leaves through `writeCommandError`. `error.code` is always CLI-owned: +`broker_unavailable` when the brokered-login service returns a 5xx, `http_error` for other +non-success statuses, and `transport_error` when `fetch` rejects or times out before a +response arrives. Upstream detail is exposed only under `error.remote_error` after passing +through the same sanitizer used for MCP call errors: only `code` and `message` are read +(top-level or nested under `error`), all other fields are dropped unread, codes are +constrained to `[A-Za-z0-9_.:-]` and 64 characters, messages are capped at 500 characters, +and ANSI/C0/C1 terminal control sequences are stripped before anything reaches stdout or +stderr. An upstream body cannot set the top-level code, so it cannot impersonate stable +local codes such as `auth_required`. Before, against a broker returning 502: @@ -27,7 +32,7 @@ After: { "ok": false, "error": { - "code": "oauth_register_failed", + "code": "broker_unavailable", "status_code": 502, "remote_error": { "code": "oauth_register_failed", @@ -36,3 +41,5 @@ After: } } ``` + +The CLI reference and README now document the error envelope and its stable fields. diff --git a/packages/cli/README.md b/packages/cli/README.md index f159391..8ee183a 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -105,9 +105,12 @@ 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 one-line summary to stderr, and exits non-zero. Upstream error details +appear only under `error.remote_error`, sanitized and bounded. See +[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..3bdf0e0 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,57 @@ 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": "Client error '502 Bad Gateway' for url '...' Failed to register an OAuth client. The CALL-E login service is unavailable. ...", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client." + } + } +} +``` + +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. Branch on this. | +| `error.message` | yes | Human-readable summary; the same text is written to stderr. | +| `error.status_code` | HTTP errors | Upstream HTTP status. | +| `error.remote_error` | when an upstream body was readable | `{ code?, message? }` extracted from the upstream response, sanitized and bounded. Informational only. | +| `error.cause_code` | transport errors | Node.js error code such as `ENOTFOUND` or `ECONNREFUSED`, when known. | +| `help_command` | argument errors only | A directly runnable `--help` command. | + +`error.code` values: + +| 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 response: DNS, connection, TLS, or timeout. | +| `mcp_error` | 1 | An MCP-level failure, including stage failures from `call` commands, which add `stage`, `call_started`, `retry_safe`, and recovery fields. | + +`error.code` is never taken from an upstream response. Upstream error codes and +messages appear only under `error.remote_error`, after sanitization: unknown +fields are dropped unread, codes are limited to `[A-Za-z0-9_.:-]` and 64 +characters, messages are limited to 500 characters, and terminal control +sequences are removed before anything reaches stdout or stderr. + ## 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 6c5e06c..c58d20f 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -884,16 +884,15 @@ function errorPayload(error, config, helpCommand = null) { } if (error instanceof HttpStatusError) { - const remoteError = parseRemoteErrorBody(error.responseText); + const remoteError = sanitizedRemoteError(error.responseText); const brokerUnavailable = isBrokerRegistrationFailure(error); - const messageParts = [error.message]; + // error.message embeds the upstream status text, so it is remote-influenced too. + const messageParts = [safeRemoteString(error.message, LOCAL_MESSAGE_LIMIT) ?? "HTTP request failed."]; if (remoteError?.message) { messageParts.push(remoteError.message); } if (brokerUnavailable) { - messageParts.push( - "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." - ); + messageParts.push(BROKER_UNAVAILABLE_HINT); } return { exitCode: 1, @@ -901,8 +900,9 @@ function errorPayload(error, config, helpCommand = null) { ok: false, server_url: config?.serverUrl ?? null, error: { - code: remoteError?.code - || (brokerUnavailable ? "broker_unavailable" : "http_error"), + // The top-level code is always locally owned. An upstream body must never be + // able to impersonate a stable local code such as `auth_required`. + code: brokerUnavailable ? "broker_unavailable" : "http_error", message: messageParts.join(" "), status_code: error.statusCode, ...(remoteError ? { remote_error: remoteError } : {}), @@ -911,6 +911,23 @@ function errorPayload(error, config, helpCommand = null) { }; } + if (isTransportFailure(error)) { + const causeCode = safeRemoteCode(error?.cause?.code); + const detail = safeRemoteString(error?.message, LOCAL_MESSAGE_LIMIT) ?? "Request failed before a response was received."; + return { + exitCode: 1, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: "transport_error", + message: causeCode ? `${detail} (${causeCode})` : detail, + ...(causeCode ? { cause_code: causeCode } : {}), + }, + }, + }; + } + return { exitCode: 1, body: { @@ -918,39 +935,59 @@ function errorPayload(error, config, helpCommand = null) { server_url: config?.serverUrl ?? null, error: { code: "mcp_error", - message: error?.message || String(error), + message: safeRemoteString(error?.message ?? String(error), LOCAL_MESSAGE_LIMIT) ?? "Unknown error.", }, }, }; } -const REMOTE_ERROR_BODY_LIMIT = 500; - -function parseRemoteErrorBody(responseText) { - const text = String(responseText ?? "").trim(); - if (!text) { +const LOCAL_MESSAGE_LIMIT = 300; +const REMOTE_MESSAGE_LIMIT = 500; + +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."; + +/** + * Reduce an upstream HTTP error body to at most two sanitized strings. + * + * Allowlist only: `code` (or a string-valued `error`) and `message`, read from the top level + * or from a nested `error` object. Every other field is dropped unread, so token-like or + * internal fields in a response can never reach stdout or stderr. Both strings pass through + * the same sanitizer as MCP call errors; codes are additionally constrained to a machine-safe + * character set and length. + */ +function sanitizedRemoteError(responseText) { + const raw = typeof responseText === "string" ? responseText : ""; + if (!raw.trim()) { return null; } + let parsed; try { - parsed = JSON.parse(text); + parsed = JSON.parse(raw); } catch { - return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + parsed = undefined; } - if (!parsed || typeof parsed !== "object") { - return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + + const record = recordObject(parsed); + if (!record) { + // Non-JSON (typically a gateway's HTML page) or a JSON array/scalar: keep a bounded, + // control-stripped excerpt and nothing else. + const message = safeRemoteString(raw, REMOTE_MESSAGE_LIMIT); + return message ? { message } : null; } - const source = parsed.error && typeof parsed.error === "object" ? parsed.error : parsed; - const code = typeof source.code === "string" - ? source.code - : (typeof parsed.error === "string" ? parsed.error : undefined); - const message = typeof source.message === "string" ? source.message : undefined; - if (!code && !message) { + + const nested = recordObject(record.error) || {}; + const codeValue = nested.code ?? record.code ?? (typeof record.error === "string" ? record.error : undefined); + const code = safeRemoteCode(codeValue); + const message = safeRemoteString(nested.message ?? record.message, REMOTE_MESSAGE_LIMIT); + + if (code === undefined && message === undefined) { return null; } return { - ...(code ? { code } : {}), - ...(message ? { message } : {}), + ...(code !== undefined ? { code } : {}), + ...(message !== undefined ? { message } : {}), }; } @@ -959,11 +996,25 @@ function isBrokerRegistrationFailure(error) { && /\/api\/v1\/openagent-auth\/sessions/u.test(String(error?.message ?? "")); } +// A rejected fetch (DNS failure, connection refused, TLS error) surfaces as a TypeError with +// a `cause`; the core HTTP layer turns an aborted request into a plain timeout Error. +function isTransportFailure(error) { + if (error instanceof HttpStatusError || error instanceof McpHttpError) { + return false; + } + if (error instanceof TypeError) { + return true; + } + return /^Request timed out for /u.test(String(error?.message ?? "")); +} + 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; @@ -1082,11 +1133,38 @@ function structuredPayload(result) { return result?.structuredContent || result?.structured_content || result || {}; } +// ANSI/VT escape sequences (CSI, OSC, and single-character ESC forms) plus C0/C1 control +// characters. Anything remote-supplied that reaches a log line or a terminal goes through +// this, so a hostile or misconfigured upstream cannot inject cursor movement, colour, line +// breaks, or hidden text into agent-visible output. +const TERMINAL_CONTROL_RE = + /\u001b\[[0-?]*[ -\/]*[@-~]|\u001b\][^\u0007\u001b]*(?:\u0007|\u001b\\)|\u001b[@-_]|[\u0000-\u001f\u007f-\u009f]/gu; + +function stripTerminalControls(value) { + return String(value).replace(TERMINAL_CONTROL_RE, " "); +} + function safeRemoteString(value, maxLength = 1000) { - if (typeof value !== "string" || !value.trim()) { + if (typeof value !== "string") { + return undefined; + } + const cleaned = stripTerminalControls(value).trim(); + if (!cleaned) { + return undefined; + } + return cleaned.slice(0, maxLength); +} + +// Machine codes from upstream are kept only as an opaque, normalized token under +// `remote_error`; they never become the CLI's own `error.code`, which agent hosts branch on. +const REMOTE_CODE_RE = /^[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u; + +function safeRemoteCode(value) { + if (typeof value !== "string") { return undefined; } - return value.trim().slice(0, maxLength); + const cleaned = stripTerminalControls(value).trim(); + return REMOTE_CODE_RE.test(cleaned) ? cleaned : undefined; } function safeRemoteCallError(result) { diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index 4fa90d0..b0acc71 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -403,7 +403,7 @@ test("auth login surfaces the upstream error body when brokered login registrati assert.equal(result.code, 1); assert.equal(payload.ok, false); assert.equal(payload.error.status_code, 502); - assert.equal(payload.error.code, "oauth_register_failed"); + 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", @@ -443,6 +443,131 @@ test("auth login keeps a non-JSON upstream error body readable and bounded", asy 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.cause_code, "ENOTFOUND"); + assert.equal(payload.help_command, undefined); + assert.match(payload.error.message, /fetch failed/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 () => { + throw new Error("Request timed out for POST https://mcp.example/api/v1/openagent-auth/sessions"); + }; + 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.cause_code, undefined); + assert.match(payload.error.message, /timed out/u); }); test("auth login start-only replaces locally active pending cache when broker reports it expired", async () => {