From 1e501fa5cbaf28909c0982cf573019251035aa0b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:57:38 -0400 Subject: [PATCH] fix: redact sensitive query values from timeout diagnostics (#2319 round 23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timeout message interpolated the raw request URL, and createFetchTracker records that message verbatim into the Network log and the persisted session. An OAuth endpoint's query string can carry a `code`, an `access_token` or a `client_secret` — exactly the fields fetchTracking already redacts everywhere else it records a URL — so a stalled signed endpoint would have written its secret to both. Redacted with the existing `redactUrlQuery`, in the OAuthRequestTimeoutError constructor rather than at each display site, so both the message and the public `url` field are covered by construction and no future consumer can reintroduce it. The route's own proxied-timeout message and the URL in its 504 marker get the same treatment, since both cross back to the browser and are recorded there. The path and every non-sensitive parameter survive, so the endpoint stays identifiable — which was the point of naming it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../src/test/core/auth/requestTimeout.test.ts | 30 +++++++++++++++++++ core/auth/requestTimeout.ts | 19 ++++++++++-- core/mcp/remote/node/server.ts | 10 +++++-- 3 files changed, 55 insertions(+), 4 deletions(-) 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,