diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 28b83ce8d8..740f44aea5 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -295,3 +295,115 @@ describe("formatErrorOutput", () => { expect(parsed.error.url).toBe("https://ctx.example/mcp"); }); }); + +/** + * #2423: the envelope is written verbatim to stderr, so every field that can + * carry a URL gets the same query redaction the web client's Network log does. + */ +describe("envelope URL redaction", () => { + const SECRET_URL = + "https://srv.example/mcp?code=abc123&access_token=tok456&tenant=acme"; + const REDACTED_URL = + "https://srv.example/mcp?code=%5BREDACTED%5D&access_token=%5BREDACTED%5D&tenant=acme"; + + it("redacts sensitive query params in the context url", () => { + const { envelope } = classifyError(new Error("nope"), { url: SECRET_URL }); + expect(envelope.url).toBe(REDACTED_URL); + }); + + it("redacts sensitive query params in a CliExitCodeError's own url", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, "login", { + url: SECRET_URL, + }), + ); + expect(envelope.url).toBe(REDACTED_URL); + }); + + it("redacts a URL embedded in the message, keeping trailing punctuation", () => { + const { envelope } = classifyError( + new Error(`Request to ${SECRET_URL}. Retry later`), + ); + expect(envelope.message).toBe(`Request to ${REDACTED_URL}. Retry later`); + expect(envelope.message).not.toContain("abc123"); + expect(envelope.message).not.toContain("tok456"); + }); + + it("redacts a URL whose scheme is upper- or mixed-case", () => { + const { envelope } = classifyError( + new Error( + "a HTTP://srv.example/cb?access_token=tok456 b HttpS://srv.example/cb?code=abc123", + ), + ); + expect(envelope.message).toBe( + "a HTTP://srv.example/cb?access_token=%5BREDACTED%5D b HttpS://srv.example/cb?code=%5BREDACTED%5D", + ); + }); + + it("redacts each of two comma-joined URLs separately", () => { + const { envelope } = classifyError( + new Error( + "https://one.example/cb?state=ok,https://two.example/cb?code=secret", + ), + ); + expect(envelope.message).toBe( + "https://one.example/cb?state=ok,https://two.example/cb?code=%5BREDACTED%5D", + ); + }); + + it("redacts through an apostrophe inside a query value", () => { + const { envelope } = classifyError( + new Error("at https://srv.example/cb?code=abc'def now"), + ); + expect(envelope.message).toBe( + "at https://srv.example/cb?code=%5BREDACTED%5D now", + ); + }); + + it("keeps the closing quote of a single-quoted URL", () => { + const { envelope } = classifyError( + new Error("at 'https://srv.example/cb?code=abc123'."), + ); + expect(envelope.message).toBe( + "at 'https://srv.example/cb?code=%5BREDACTED%5D'.", + ); + }); + + it("redacts a URL embedded in the cause chain", () => { + const { envelope } = classifyError( + new Error("fetch failed", { + cause: new Error(`connect ECONNREFUSED (${SECRET_URL})`), + }), + ); + expect(envelope.cause).toBe(`connect ECONNREFUSED (${REDACTED_URL})`); + }); + + it("classifies on the unredacted text", () => { + // The only auth signal is inside a parameter value that redaction + // replaces; classifying the redacted copy would fall through to USAGE. + const { exitCode, envelope } = classifyError( + new Error("Rejected https://srv.example/cb?token=invalid_token"), + ); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.message).toBe( + "Rejected https://srv.example/cb?token=%5BREDACTED%5D", + ); + }); + + it("leaves text and URLs without sensitive params untouched", () => { + const text = "Failed at https://srv.example/mcp?tenant=acme and nowhere"; + const { envelope } = classifyError(new Error(text), { + url: "https://srv.example/mcp", + }); + expect(envelope.message).toBe(text); + expect(envelope.url).toBe("https://srv.example/mcp"); + }); + + it("keeps the redaction in the serialized stderr line", () => { + const { stderr } = formatErrorOutput(new Error(`boom ${SECRET_URL}`), { + url: SECRET_URL, + }); + expect(stderr).not.toContain("abc123"); + expect(stderr).not.toContain("tok456"); + }); +}); diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 91cf247937..9a1d286208 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -1,3 +1,4 @@ +import { redactUrlQuery } from "@inspector/core/mcp/fetchTracking.js"; import { awaitableError } from "./utils/awaitable-log.js"; /** @@ -138,14 +139,77 @@ function statusOf(error: unknown): number | undefined { const UNREACHABLE_PATTERN = /ENOTFOUND|ECONNREFUSED|ECONNRESET|EAI_AGAIN|ETIMEDOUT|fetch failed|getaddrinfo|connect(?:ion)? timed out|aborted/i; +/** + * An `http(s)://` URL embedded in free text. Stops at whitespace, at the + * double-quote/angle-bracket characters that commonly delimit a URL inside a + * message, and where a second `http(s)://` begins — so two URLs joined by a + * comma are redacted separately rather than the second one's query being read + * as part of the first one's last value (Copilot). An apostrophe is kept in the + * match because it is legal inside a query value; a *trailing* one is peeled + * off as punctuation below, which still handles a `'…'`-quoted URL. + * Case-insensitive because URI schemes are: `HTTPS://…?code=…` is the same + * URL and must not slip past the redaction (Copilot). + */ +const EMBEDDED_URL_PATTERN = /\bhttps?:\/\/(?:(?!https?:\/\/)[^\s"<>])+/gi; + +/** Sentence punctuation (or a closing quote) a message may put right after a URL. */ +const TRAILING_PUNCTUATION = /[.,;:!?)\]']+$/; + +/** + * Apply {@link redactUrlQuery} to every URL embedded in `text`. Trailing + * sentence punctuation is split off first and re-appended, so a URL ending a + * sentence (`…?code=abc.`) keeps its full stop instead of having it folded into + * the redacted parameter value. + */ +function redactUrlsInText(text: string): string { + return text.replace(EMBEDDED_URL_PATTERN, (match) => { + const trailing = TRAILING_PUNCTUATION.exec(match)?.[0] ?? ""; + const url = match.slice(0, match.length - trailing.length); + return redactUrlQuery(url) + trailing; + }); +} + +/** + * Scrub query-string secrets out of every envelope field that can carry a URL + * (#2423). The envelope is written verbatim to stderr — a terminal, a CI log, + * a pipe into another tool — so it gets the same {@link redactUrlQuery} + * guarantee the web client's Network log and `OAuthRequestTimeoutError` + * already have: an OAuth `code`, `access_token` or `client_secret` in a server + * URL is replaced, while the path and non-sensitive parameters stay readable. + * + * Applied to the finished envelope rather than to the inputs, so + * classification still reads the error's own text: redaction rewrites + * parameter values, and a pattern test run on the rewritten copy could land a + * different exit code. + */ +function redactEnvelope(envelope: ErrorEnvelope): ErrorEnvelope { + return { + ...envelope, + message: redactUrlsInText(envelope.message), + ...(envelope.cause !== undefined && { + cause: redactUrlsInText(envelope.cause), + }), + ...(envelope.url !== undefined && { url: redactUrlQuery(envelope.url) }), + }; +} + /** * Classify an arbitrary error into an exit code and envelope. Used both by the * binary's {@link handleError} and by callers that want to throw a - * {@link CliExitCodeError} with the right code up front. + * {@link CliExitCodeError} with the right code up front. Every URL in the + * returned envelope is query-redacted (see {@link redactEnvelope}). */ export function classifyError( error: unknown, context?: { url?: string }, +): { exitCode: number; envelope: ErrorEnvelope } { + const { exitCode, envelope } = classifyUnredacted(error, context); + return { exitCode, envelope: redactEnvelope(envelope) }; +} + +function classifyUnredacted( + error: unknown, + context?: { url?: string }, ): { exitCode: number; envelope: ErrorEnvelope } { const message = error instanceof Error