diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 1875d878..c7cb609f 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -88,6 +88,36 @@ describe("withOAuthRequestTimeout", () => { expect(timeout.timeoutMs).toBe(1000); }); + it("redacts sensitive query values from the endpoint it names", async () => { + vi.useFakeTimers(); + // This message is recorded verbatim by `createFetchTracker` into the + // Network log and the persisted session, and an OAuth endpoint's query + // string can carry a `code`, an `access_token` or a `client_secret` — + // which `fetchTracking` already redacts everywhere else it records a URL. + const sensitive = + "https://as.example.com/token?code=abc123&client_secret=shhh&state=keepme"; + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const assertion = wrapped(sensitive).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + const err = (await assertion) as OAuthRequestTimeoutError; + + for (const secret of ["abc123", "shhh"]) { + expect(err.message).not.toContain(secret); + expect(err.url).not.toContain(secret); + } + // Still identifiable, which was the point of naming the endpoint: the path + // and every non-sensitive parameter survive. Read through `URL`, since + // `URLSearchParams.toString()` percent-encodes the redaction marker. + const redacted = new URL(err.url); + expect(redacted.origin + redacted.pathname).toBe( + "https://as.example.com/token", + ); + expect(redacted.searchParams.get("state")).toBe("keepme"); + expect(redacted.searchParams.get("code")).toBe("[REDACTED]"); + expect(redacted.searchParams.get("client_secret")).toBe("[REDACTED]"); + }); + it("takes the URL off a Request object too", async () => { vi.useFakeTimers(); const wrapped = withOAuthRequestTimeout(neverSettles, 1000); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 3ccca28d..ba06e53f 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -63,6 +63,8 @@ * halves the 60s the SDK handshake incidentally provides, and it is the only * bound at all on the legs that sit outside an SDK request. */ +import { redactUrlQuery } from "../mcp/fetchTracking.js"; + export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; /** @@ -73,13 +75,26 @@ export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; * hosts, and which of them stalled is the whole diagnostic. */ export class OAuthRequestTimeoutError extends Error { + /** + * The endpoint that stalled, with sensitive query values redacted. + * + * Redacted in the constructor rather than at each display site, so every + * consumer is covered by construction: this error's message is recorded + * verbatim by `createFetchTracker` into the Network log and the persisted + * session, and an OAuth endpoint can carry a `code`, an `access_token` or a + * `client_secret` in its query string — which `fetchTracking` already + * deliberately redacts everywhere else it records a URL (Copilot). The path + * and every non-sensitive parameter survive, so the endpoint stays + * identifiable, which was the point of naming it. + */ readonly url: string; readonly timeoutMs: number; constructor(url: string, timeoutMs: number) { - super(`OAuth request to ${url} timed out after ${timeoutMs}ms`); + const safeUrl = redactUrlQuery(url); + super(`OAuth request to ${safeUrl} timed out after ${timeoutMs}ms`); this.name = "OAuthRequestTimeoutError"; - this.url = url; + this.url = safeUrl; this.timeoutMs = timeoutMs; } } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 62819249..ccdc7a8b 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -29,6 +29,7 @@ import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; +import { redactUrlQuery } from "../../fetchTracking.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1266,6 +1267,11 @@ export function createRemoteApp( Number.isFinite(requestedTimeoutMs) ? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs))) : undefined; + // Redacted for the same reason `OAuthRequestTimeoutError` redacts: this + // message and this URL are handed back to the browser, recorded in the + // Network log and persisted, and an OAuth endpoint's query string can carry + // a `code`, an `access_token` or a `client_secret` (Copilot). + const safeUrl = redactUrlQuery(url); const timer = deadlineMs === undefined ? undefined @@ -1273,7 +1279,7 @@ export function createRemoteApp( deadlineFired = true; controller.abort( new Error( - `proxied request to ${url} timed out after ${deadlineMs}ms`, + `proxied request to ${safeUrl} timed out after ${deadlineMs}ms`, ), ); }, deadlineMs); @@ -1338,7 +1344,7 @@ export function createRemoteApp( { error: msg, code: OAUTH_TIMEOUT_WIRE_CODE, - url, + url: safeUrl, timeoutMs: deadlineMs, }, 504,