From 5d7c9d8b3bbc4d342c40e227f37ab4b7229e5d7e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 00:05:10 -0500 Subject: [PATCH 1/2] mcpdo: keep the sign-in helper's failure classification without a TTY (#2962) The detached sign-in helper now classifies a flow failure with classifyError, where the original error object still exists, and relays the exit code and envelope fields on its error event. The parent rebuilds that CliExitCodeError (type-checked, sanitized) so a non-TTY connect reports e.g. fetch_refused / 12 with its url, as the TTY path does. auth_required stays the fallback for an unclassified failure. The EMA login helper shares the same event and gets the same fix. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- clients/mcpdo/__tests__/auth-helper.test.ts | 173 ++++++++++++++++++ .../mcpdo/__tests__/ema-login-helper.test.ts | 4 + clients/mcpdo/src/connection/auth-helper.ts | 98 +++++++++- .../mcpdo/src/connection/ema-login-helper.ts | 6 +- 4 files changed, 271 insertions(+), 10 deletions(-) diff --git a/clients/mcpdo/__tests__/auth-helper.test.ts b/clients/mcpdo/__tests__/auth-helper.test.ts index 9f362b14f3..f996846ebe 100644 --- a/clients/mcpdo/__tests__/auth-helper.test.ts +++ b/clients/mcpdo/__tests__/auth-helper.test.ts @@ -4,6 +4,8 @@ import * as os from "node:os"; import * as path from "node:path"; import { PassThrough } from "node:stream"; import type { CallbackNavigation } from "@inspector/core/auth/index.js"; +import { FetchTargetError } from "@inspector/core/auth/fetchRefusal.js"; +import { EXIT_CODES } from "@inspector/core/cli/error-handler.js"; const authorizeInFrontend = vi.fn(); @@ -21,9 +23,11 @@ vi.mock("../src/connection/authorize.js", () => ({ import { AUTH_HELPER_COMMAND, + helperErrorEvent, obtainPendingAuthUrl, pendingAuthMarkerPath, readLivePendingAuthMarker, + relayedHelperError, removeOwnPendingAuthMarker, runAuthHelper, type PendingAuthMarker, @@ -195,6 +199,40 @@ describe("auth-helper", () => { }); }); + it("keeps a helper's relayed classification instead of auth_required (#2962)", async () => { + const script = writeHelperScript(` + process.stdin.resume(); + process.stdin.on("end", () => { + process.stdout.write( + JSON.stringify({ + event: "error", + message: "Refusing to fetch from a link-local or metadata address (169.254.169.254)", + exitCode: 12, + envelope: { + code: "fetch_refused", + url: "http://169.254.169.254/.well-known/oauth-authorization-server", + }, + }) + "\\n", + ); + }); + `); + await expect( + obtainPendingAuthUrl( + { type: "streamable-http", url: SERVER_URL }, + undefined, + { helperArgv1: script }, + ), + ).rejects.toMatchObject({ + exitCode: EXIT_CODES.FETCH_REFUSED, + message: + "Sign-in helper failed: Refusing to fetch from a link-local or metadata address (169.254.169.254)", + envelope: { + code: "fetch_refused", + url: "http://169.254.169.254/.well-known/oauth-authorization-server", + }, + }); + }); + it("relays fetch_refused events as sanitized warnings before the URL (#2905)", async () => { const script = writeHelperScript(` process.stdin.resume(); @@ -687,6 +725,42 @@ describe("auth-helper", () => { expect(fs.readdirSync(dir).filter((f) => f.includes("auth"))).toEqual([]); }); + it("relays the failure's classification on the error event (#2962)", async () => { + authorizeInFrontend.mockRejectedValueOnce( + new TypeError("fetch failed", { + cause: new FetchTargetError( + "Refusing to fetch from a link-local or metadata address (169.254.169.254)", + "http://169.254.169.254/.well-known/oauth-authorization-server", + ), + }), + ); + const restoreStdin = stubStdin( + JSON.stringify({ + serverConfig: { type: "streamable-http", url: SERVER_URL }, + }), + ); + const stdout = captureStdout(); + try { + await expect(runAuthHelper()).rejects.toThrow("fetch failed"); + } finally { + stdout.restore(); + restoreStdin(); + } + expect(stdout.lines().map((l) => JSON.parse(l) as unknown)).toEqual([ + { + event: "error", + message: "fetch failed", + exitCode: EXIT_CODES.FETCH_REFUSED, + envelope: { + code: "fetch_refused", + cause: + "Refusing to fetch from a link-local or metadata address (169.254.169.254)", + url: "http://169.254.169.254/.well-known/oauth-authorization-server", + }, + }, + ]); + }); + it("stringifies a non-Error flow failure in the error event", async () => { authorizeInFrontend.mockRejectedValueOnce("string boom"); const restoreStdin = stubStdin( @@ -759,4 +833,103 @@ describe("auth-helper", () => { } }); }); + + describe("helperErrorEvent", () => { + it("omits the classification of an unclassified failure", () => { + expect(helperErrorEvent(new Error("no AS metadata"))).toEqual({ + event: "error", + message: "no AS metadata", + }); + }); + + it("carries a classified failure's exit code and envelope, minus message", () => { + const refusal = new FetchTargetError( + "refused", + "http://169.254.169.254/x", + ); + expect(helperErrorEvent(refusal)).toEqual({ + event: "error", + message: "refused", + exitCode: EXIT_CODES.FETCH_REFUSED, + envelope: { code: "fetch_refused", url: "http://169.254.169.254/x" }, + }); + }); + }); + + describe("relayedHelperError", () => { + const message = "Sign-in helper failed: x"; + type RelayEvent = Parameters[0]; + + it.each<[string, unknown]>([ + ["no exit code", { event: "error", message: "x" }], + [ + "exit code OK", + { event: "error", message: "x", exitCode: 0, envelope: { code: "ok" } }, + ], + [ + "an unknown exit code", + { event: "error", message: "x", exitCode: 99, envelope: { code: "z" } }, + ], + ["no envelope", { event: "error", message: "x", exitCode: 12 }], + [ + "a null envelope", + { event: "error", message: "x", exitCode: 12, envelope: null }, + ], + [ + "a non-string code", + { event: "error", message: "x", exitCode: 12, envelope: { code: 12 } }, + ], + ])("returns undefined for %s", (_label, event) => { + // Decoded wire input: the shape is exactly what is under test. + expect(relayedHelperError(event as RelayEvent, message)).toBeUndefined(); + }); + + it("keeps well-typed fields, sanitizes strings, and drops the rest", () => { + const error = relayedHelperError( + { + event: "error", + message: "x", + exitCode: EXIT_CODES.AUTH_REQUIRED, + envelope: { + code: "oauth_error", + status: 400, + url: "https://as.example/token\u001b[2J", + oauthError: "invalid_grant", + oauthErrorDescription: "expired", + method: "tools/list", + retryAfter: "5", + retryAfterMs: 5000, + // Not a field the relay carries: classifyError reads a cause + // off the error itself, not off its envelope. + cause: "not relayed", + }, + }, + message, + ); + expect(error?.exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(error?.message).toBe(message); + expect(error?.envelope).toEqual({ + code: "oauth_error", + status: 400, + url: "https://as.example/token\u241b[2J", + oauthError: "invalid_grant", + oauthErrorDescription: "expired", + method: "tools/list", + retryAfter: "5", + retryAfterMs: 5000, + }); + }); + + it("drops mistyped optional fields rather than coercing them", () => { + const event: unknown = { + event: "error", + message: "x", + exitCode: EXIT_CODES.FETCH_REFUSED, + envelope: { code: "fetch_refused", status: "500", url: 42 }, + }; + // Decoded wire input: the mistyped fields are what is under test. + const error = relayedHelperError(event as RelayEvent, message); + expect(error?.envelope).toEqual({ code: "fetch_refused" }); + }); + }); }); diff --git a/clients/mcpdo/__tests__/ema-login-helper.test.ts b/clients/mcpdo/__tests__/ema-login-helper.test.ts index c1f358c644..888ec374f4 100644 --- a/clients/mcpdo/__tests__/ema-login-helper.test.ts +++ b/clients/mcpdo/__tests__/ema-login-helper.test.ts @@ -270,10 +270,14 @@ describe("ema-login-helper", () => { it("emits an error event and rethrows when EMA is not configured", async () => { await expect(runEmaLoginHelper()).rejects.toThrow(/not configured/i); + // The failure is pre-classified (usage), and that classification rides + // on the event so the parent reports it instead of auth_required (#2962). expect(events()).toEqual([ { event: "error", message: expect.stringContaining("not configured") as string, + exitCode: 1, + envelope: { code: "usage" }, }, ]); }); diff --git a/clients/mcpdo/src/connection/auth-helper.ts b/clients/mcpdo/src/connection/auth-helper.ts index 098a31fac9..c947d42e79 100644 --- a/clients/mcpdo/src/connection/auth-helper.ts +++ b/clients/mcpdo/src/connection/auth-helper.ts @@ -8,8 +8,10 @@ import type { MCPServerConfig, } from "@inspector/core/mcp/types.js"; import { + classifyError, CliExitCodeError, EXIT_CODES, + type ErrorEnvelope, } from "@inspector/core/cli/error-handler.js"; import { writeFetchRefusalWarning, @@ -40,6 +42,13 @@ import { sanitizeText } from "./sanitize.js"; * must tell the user travels on the stdout protocol too: a destination-guard * refusal (#2846) arrives as a `fetch_refused` event, which the parent prints * as its own `Warning:` line before the URL or the error (#2905). + * + * A flow failure is classified **in the helper**, where the original error + * object still exists, and its exit code and envelope fields ride on the + * `error` event (#2962). The parent cannot classify the relayed text itself — + * a fetch-guard refusal is recognised by its error type, not its wording — so + * without this every pre-URL failure came out as `auth_required` / exit 3 + * while the TTY path reported the same failure as, say, `fetch_refused` / 12. */ /** Hidden subcommand name (see registerAuthCommands in mcp.ts). */ @@ -56,7 +65,82 @@ export type AuthHelperEvent = | { event: "auth_url"; url: string } | { event: "fetch_refused"; url: string; reason: string } | { event: "done" } - | { event: "error"; message: string }; + | { + event: "error"; + message: string; + /** + * The failure's classification, computed by the helper with + * `classifyError` (#2962). Present only when the failure classified as + * something more specific than the catch-all `error`; absent, the + * parent falls back to `auth_required`. + */ + exitCode?: number; + envelope?: Partial; + }; + +/** + * Build the `error` event a helper emits for a failed flow, carrying the + * failure's classification when it has one (#2962). The catch-all `error` + * classification is left off: the parent's `auth_required` fallback is the + * better reading of an unclassified sign-in failure, and it is what the + * event meant before classification was relayed. + */ +export function helperErrorEvent(error: unknown): AuthHelperEvent { + const message = error instanceof Error ? error.message : String(error); + const { exitCode, envelope } = classifyError(error); + if (envelope.code === "error") return { event: "error", message }; + // `message` travels once, at the top level; the envelope carries the rest. + const fields: Partial = { ...envelope }; + delete fields.message; + return { event: "error", message, exitCode, envelope: fields }; +} + +/** Every exit code a relayed classification may name — never `OK`. */ +const RELAYABLE_EXIT_CODES: ReadonlySet = new Set( + Object.values(EXIT_CODES).filter((code) => code !== EXIT_CODES.OK), +); + +/** + * Rebuild the classified error a helper relayed (#2962), or `undefined` when + * the event carries no usable classification. Every field is type-checked + * rather than trusted, and every string is sanitized: it is server-derived + * text on its way to a terminal, like the message. + */ +export function relayedHelperError( + event: Extract, + message: string, +): CliExitCodeError | undefined { + const { exitCode, envelope } = event; + if ( + typeof exitCode !== "number" || + !RELAYABLE_EXIT_CODES.has(exitCode) || + typeof envelope !== "object" || + envelope === null || + typeof envelope.code !== "string" + ) { + return undefined; + } + const text = (value: unknown) => + typeof value === "string" ? sanitizeText(value) : undefined; + const number = (value: unknown) => + typeof value === "number" && Number.isFinite(value) ? value : undefined; + const fields: Partial = { + code: sanitizeText(envelope.code), + status: number(envelope.status), + url: text(envelope.url), + method: text(envelope.method), + retryAfter: text(envelope.retryAfter), + retryAfterMs: number(envelope.retryAfterMs), + oauthError: text(envelope.oauthError), + oauthErrorDescription: text(envelope.oauthErrorDescription), + }; + // Drop absent fields rather than carrying `undefined` keys into the + // envelope, matching every other optional envelope field. + for (const key of Object.keys(fields) as (keyof ErrorEnvelope)[]) { + if (fields[key] === undefined) delete fields[key]; + } + return new CliExitCodeError(exitCode, message, fields); +} /** * Pending sign-in marker, one per flow key (a server URL, or an @@ -240,10 +324,7 @@ export async function runAuthHelper(): Promise { }); emit({ event: "done" }); } catch (error) { - emit({ - event: "error", - message: error instanceof Error ? error.message : String(error), - }); + emit(helperErrorEvent(error)); throw error; } finally { if (markerPath !== undefined) removeOwnPendingAuthMarker(markerPath); @@ -494,7 +575,12 @@ async function spawnAuthHelperForUrl( clearTimeout(timer); // The helper relays server-derived text; strip C0/C1 controls // before it reaches a terminal via the error envelope. - fail(`Sign-in helper failed: ${sanitizeText(event.message)}`); + const message = `Sign-in helper failed: ${sanitizeText(event.message)}`; + // Keep the helper's classification when it relayed one (#2962); + // `auth_required` is only the fallback for an unclassified one. + const relayed = relayedHelperError(event, message); + if (relayed !== undefined) reject(relayed); + else fail(message); return; } } diff --git a/clients/mcpdo/src/connection/ema-login-helper.ts b/clients/mcpdo/src/connection/ema-login-helper.ts index cc63adf901..d80dd7c2c3 100644 --- a/clients/mcpdo/src/connection/ema-login-helper.ts +++ b/clients/mcpdo/src/connection/ema-login-helper.ts @@ -18,6 +18,7 @@ import { import { CallbackNavigation } from "@inspector/core/auth/index.js"; import { NodeOAuthStorage } from "@inspector/core/auth/node/index.js"; import { + helperErrorEvent, obtainPendingUrlForKey, pendingAuthMarkerPath, removeOwnPendingAuthMarker, @@ -135,10 +136,7 @@ export async function runEmaLoginHelper(): Promise { ); emit({ event: "done" }); } catch (error) { - emit({ - event: "error", - message: error instanceof Error ? error.message : String(error), - }); + emit(helperErrorEvent(error)); throw error; } finally { if (markerPath !== undefined) removeOwnPendingAuthMarker(markerPath); From fb5e98d9802295952d279a3fbcc6bea0aa7ba812 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 00:30:30 -0500 Subject: [PATCH 2/2] Address review: carry the relayed cause back onto the rebuilt error (#2962) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- clients/mcpdo/__tests__/auth-helper.test.ts | 26 +++++++++++++++++---- clients/mcpdo/src/connection/auth-helper.ts | 8 ++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/clients/mcpdo/__tests__/auth-helper.test.ts b/clients/mcpdo/__tests__/auth-helper.test.ts index f996846ebe..2c2082d384 100644 --- a/clients/mcpdo/__tests__/auth-helper.test.ts +++ b/clients/mcpdo/__tests__/auth-helper.test.ts @@ -5,7 +5,10 @@ import * as path from "node:path"; import { PassThrough } from "node:stream"; import type { CallbackNavigation } from "@inspector/core/auth/index.js"; import { FetchTargetError } from "@inspector/core/auth/fetchRefusal.js"; -import { EXIT_CODES } from "@inspector/core/cli/error-handler.js"; +import { + classifyError, + EXIT_CODES, +} from "@inspector/core/cli/error-handler.js"; const authorizeInFrontend = vi.fn(); @@ -899,9 +902,7 @@ describe("auth-helper", () => { method: "tools/list", retryAfter: "5", retryAfterMs: 5000, - // Not a field the relay carries: classifyError reads a cause - // off the error itself, not off its envelope. - cause: "not relayed", + cause: "token endpoint said no\u001b[2J", }, }, message, @@ -918,6 +919,23 @@ describe("auth-helper", () => { retryAfter: "5", retryAfterMs: 5000, }); + // The cause goes back onto the error, where classifyError reads it. + expect(error?.cause).toBe("token endpoint said no\u241b[2J"); + }); + + it("round-trips a wrapped refusal to the envelope the TTY path prints", () => { + const original = new TypeError("fetch failed", { + cause: new FetchTargetError( + "Refusing to fetch from a link-local or metadata address (169.254.169.254)", + "http://169.254.169.254/.well-known/oauth-authorization-server", + ), + }); + // Through the wire, exactly as the parent decodes it. + const event = JSON.parse( + JSON.stringify(helperErrorEvent(original)), + ) as RelayEvent; + const relayed = relayedHelperError(event, event.message); + expect(classifyError(relayed)).toEqual(classifyError(original)); }); it("drops mistyped optional fields rather than coercing them", () => { diff --git a/clients/mcpdo/src/connection/auth-helper.ts b/clients/mcpdo/src/connection/auth-helper.ts index c947d42e79..93165e88e6 100644 --- a/clients/mcpdo/src/connection/auth-helper.ts +++ b/clients/mcpdo/src/connection/auth-helper.ts @@ -139,7 +139,13 @@ export function relayedHelperError( for (const key of Object.keys(fields) as (keyof ErrorEnvelope)[]) { if (fields[key] === undefined) delete fields[key]; } - return new CliExitCodeError(exitCode, message, fields); + const relayed = new CliExitCodeError(exitCode, message, fields); + // `classifyError` reads a cause off the error itself, never off its + // envelope, so the relayed one goes back where it came from — otherwise a + // wrapped refusal (`fetch failed` caused by the guard) loses its reason. + const cause = text(envelope.cause); + if (cause !== undefined) relayed.cause = cause; + return relayed; } /**