From af4b574234894a7ea0f9264bb7243102edfb6122 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 00:25:19 -0400 Subject: [PATCH 1/3] fix: record an intercepted 401's status, headers and body (#2297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The network tracker wrapped the auth-challenge interceptor, so a 401/403 never reached it as a response: the interceptor cancelled the body and threw, and the tracker logged only the thrown error. NetworkEntry renders its response section only when a status is present, so the body — and the WWW-Authenticate resource_metadata and scope — were never shown. Move the tracker below the interceptor so it records the real response and clones its body before the interceptor cancels it, and stop awaiting that cancel: with the tracker's clone as the other tee branch, the cancel settles only once the clone reads to the end, which would hang on an unending body. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SG6q6YEDJyJB5H2Hr8nBDS Signed-off-by: cliffhall --- .../integration/mcp/node/transport.test.ts | 79 +++++++++++++++++++ core/mcp/node/authChallengeFetch.ts | 7 +- core/mcp/node/transport.ts | 47 +++++------ 3 files changed, 105 insertions(+), 28 deletions(-) diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index c813fdf0..95481a30 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -242,6 +242,85 @@ describe("Transport", () => { } }); + it("records an intercepted 401's status, WWW-Authenticate and body (#2297)", async () => { + const challengeBody = JSON.stringify({ + error: "invalid_token", + error_description: "token expired", + }); + const wwwAuthenticate = + 'Bearer resource_metadata="https://mcp.example/.well-known/oauth-protected-resource", scope="files:read"'; + const fetchFn: typeof fetch = async () => + new Response(challengeBody, { + status: 401, + statusText: "Unauthorized", + headers: { + "content-type": "application/json", + "www-authenticate": wwwAuthenticate, + }, + }); + + const fetchRequests: FetchRequestEntryBase[] = []; + const bodies = new Map(); + const result = createTransportNode( + { type: "streamable-http", url: "https://mcp.example/mcp" }, + { + fetchFn, + interceptAuthChallenges: true, + onFetchRequest: (entry) => fetchRequests.push(entry), + onFetchResponseBody: (id, body) => bodies.set(id, body), + }, + ); + + const client = new Client( + { name: "test-client", version: "1.0.0" }, + { capabilities: {} }, + ); + await expect(client.connect(result.transport)).rejects.toThrow( + "MCP auth challenge (401)", + ); + + expect(fetchRequests).toHaveLength(1); + const [entry] = fetchRequests; + expect(entry.error).toBeUndefined(); + expect(entry.responseStatus).toBe(401); + expect(entry.responseHeaders?.["www-authenticate"]).toBe(wwwAuthenticate); + await vi.waitFor(() => expect(bodies.get(entry.id)).toBe(challengeBody)); + }); + + it("still throws an intercepted 401 whose body never ends (#2297)", async () => { + const fetchFn: typeof fetch = async () => + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("partial")); + }, + }), + { + status: 401, + headers: { "www-authenticate": 'Bearer error="invalid_token"' }, + }, + ); + + const fetchRequests: FetchRequestEntryBase[] = []; + const result = createTransportNode( + { type: "streamable-http", url: "https://mcp.example/mcp" }, + { + fetchFn, + interceptAuthChallenges: true, + onFetchRequest: (entry) => fetchRequests.push(entry), + }, + ); + + const client = new Client( + { name: "test-client", version: "1.0.0" }, + { capabilities: {} }, + ); + await expect(client.connect(result.transport)).rejects.toThrow( + "MCP auth challenge (401)", + ); + expect(fetchRequests[0]?.responseStatus).toBe(401); + }); + it("applies settings.headers to the outgoing streamable-http request", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), diff --git a/core/mcp/node/authChallengeFetch.ts b/core/mcp/node/authChallengeFetch.ts index 0ce757ba..6bfbf31f 100644 --- a/core/mcp/node/authChallengeFetch.ts +++ b/core/mcp/node/authChallengeFetch.ts @@ -24,8 +24,11 @@ export function createAuthChallengeInterceptFetch( } // Release the connection before throwing so the SDK transport is not left - // with a half-read 401/403 body on streamable HTTP. - await response.body?.cancel().catch(() => {}); + // with a half-read 401/403 body on streamable HTTP. Not awaited: when the + // network tracker sits below this wrapper (#2297) the body is one branch of + // a tee, and cancelling a branch settles only once the tracker's clone has + // read to the end — so a 401 whose body never ends would hang the throw. + void response.body?.cancel().catch(() => {}); throw new AuthChallengeError( challenge, diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index dd3c8358..8ed872f4 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -67,12 +67,27 @@ export function createTransportNode( onAuthChallengeObserved ? createAuthChallengeObserverFetch(inner, onAuthChallengeObserved) : inner; + const withTracking = (inner: typeof fetch): typeof fetch => + onFetchRequest + ? createFetchTracker(inner, { + trackRequest: onFetchRequest, + updateResponseBody: onFetchResponseBody, + }) + : inner; // The observer sits *under* the interceptor so it still reports the // challenge on the path where interception throws — and, more to the point, // on the legacy first-auth path where interception is off entirely. + // + // The network tracker sits under the interceptor too (#2297). Above it, the + // tracker never saw the 401/403 at all: the interceptor cancels the body and + // throws, so the entry was recorded from the thrown error alone — no status, + // no `WWW-Authenticate`, and no response body, which is exactly the detail + // someone debugging an OAuth challenge needs. Below it, the tracker records + // the real response and clones its body before the interceptor cancels it. + const trackedBase = withTracking(withChallengeObserver(baseFetch)); const fetchWithOptionalAuthIntercept = interceptAuthChallenges - ? createAuthChallengeInterceptFetch(withChallengeObserver(baseFetch)) - : withChallengeObserver(baseFetch); + ? createAuthChallengeInterceptFetch(trackedBase) + : trackedBase; if (serverType === "stdio") { const stdioConfig = config as StdioServerConfig; @@ -111,15 +126,9 @@ export function createTransportNode( const configuredSseFetch = sseConfig.eventSourceInit?.fetch as | typeof fetch | undefined; - const sseFetch = configuredSseFetch - ? withChallengeObserver(configuredSseFetch) + const trackedFetch = configuredSseFetch + ? withTracking(withChallengeObserver(configuredSseFetch)) : fetchWithOptionalAuthIntercept; - const trackedFetch = onFetchRequest - ? createFetchTracker(sseFetch, { - trackRequest: onFetchRequest, - updateResponseBody: onFetchResponseBody, - }) - : sseFetch; const headers = headersFromSettings(settings); @@ -134,18 +143,11 @@ export function createTransportNode( ...(headers && { headers }), }; - const postFetch = onFetchRequest - ? createFetchTracker(fetchWithOptionalAuthIntercept, { - trackRequest: onFetchRequest, - updateResponseBody: onFetchResponseBody, - }) - : fetchWithOptionalAuthIntercept; - const transport = new SSEClientTransport(url, { authProvider, eventSourceInit, requestInit, - fetch: postFetch, + fetch: fetchWithOptionalAuthIntercept, }); return { transport }; @@ -161,17 +163,10 @@ export function createTransportNode( ...(headers && { headers }), }; - const transportFetch = onFetchRequest - ? createFetchTracker(fetchWithOptionalAuthIntercept, { - trackRequest: onFetchRequest, - updateResponseBody: onFetchResponseBody, - }) - : fetchWithOptionalAuthIntercept; - const transport = new StreamableHTTPClientTransport(url, { authProvider, requestInit, - fetch: transportFetch, + fetch: fetchWithOptionalAuthIntercept, // SEP-2350: how the transport reacts to a `403 insufficient_scope` // challenge. Defaults to the SDK's `reauthorize` when unset. ...(settings?.oauthOnInsufficientScope && { From a7c4c5260e56d273282ddb492ff144dee0d0829a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 01:01:15 -0400 Subject: [PATCH 2/3] fix: bound the 401/403 body read so an unending body is released (#2297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1. With the tracker below the auth-challenge interceptor, its background clone kept draining a 401 body after the interceptor cancelled its own branch — so a body that never ended left the connection live. Read a 401/403 body up front instead, capped at 64 KiB and 2s, and cancel the clone's reader when either bound is hit so the interceptor's cancel releases the source. The body now lands on the entry itself before the challenge is thrown. Also cover the web path end to end: a streamable-http 401 on /api/mcp/send reaches the event stream with its status and body. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SG6q6YEDJyJB5H2Hr8nBDS Signed-off-by: cliffhall --- .../src/test/core/mcp/fetchTracking.test.ts | 120 ++++++++++++++++++ .../integration/mcp/node/transport.test.ts | 11 +- .../mcp/remote/remote-auth-branches.test.ts | 63 +++++++++ core/mcp/fetchTracking.ts | 74 +++++++++++ core/mcp/node/authChallengeFetch.ts | 4 +- core/mcp/node/transport.ts | 3 +- 6 files changed, 269 insertions(+), 6 deletions(-) diff --git a/clients/web/src/test/core/mcp/fetchTracking.test.ts b/clients/web/src/test/core/mcp/fetchTracking.test.ts index 76abcb0f..4640b694 100644 --- a/clients/web/src/test/core/mcp/fetchTracking.test.ts +++ b/clients/web/src/test/core/mcp/fetchTracking.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, vi } from "vitest"; import { + CHALLENGE_BODY_MAX_BYTES, + CHALLENGE_BODY_READ_MS, + TRUNCATED_BODY_SUFFIX, createFetchTracker, redactSensitiveHeaders, redactBody, @@ -539,6 +542,123 @@ describe("createFetchTracker", () => { }); }); +describe("createFetchTracker — 401/403 challenge bodies (#2297)", () => { + function track(response: Response) { + const tracked: FetchRequestEntryBase[] = []; + const updateResponseBody = vi.fn(); + const fetcher = createFetchTracker((async () => response) as typeof fetch, { + trackRequest: (entry) => tracked.push(entry), + updateResponseBody, + }); + return { fetcher, tracked, updateResponseBody }; + } + + it("records the body on the entry itself, before returning", async () => { + const body = JSON.stringify({ error: "invalid_token" }); + const { fetcher, tracked, updateResponseBody } = track( + new Response(body, { + status: 401, + headers: { "content-type": "application/json" }, + }), + ); + + const res = await fetcher("https://mcp.example/mcp", { method: "POST" }); + + expect(tracked).toHaveLength(1); + expect(tracked[0]?.responseStatus).toBe(401); + expect(tracked[0]?.responseBody).toBe(body); + expect(updateResponseBody).not.toHaveBeenCalled(); + // The caller still gets an unread body. + expect(await res.text()).toBe(body); + }); + + it("does the same for a 403", async () => { + const { fetcher, tracked } = track( + new Response("insufficient_scope", { status: 403 }), + ); + await fetcher("https://mcp.example/mcp", { method: "POST" }); + expect(tracked[0]?.responseBody).toBe("insufficient_scope"); + }); + + it("stops at the byte cap and marks the body truncated", async () => { + const chunk = new Uint8Array(CHALLENGE_BODY_MAX_BYTES / 2).fill(97); + let pulls = 0; + const cancel = vi.fn(); + const stream = new ReadableStream({ + pull(controller) { + pulls += 1; + controller.enqueue(chunk); + }, + cancel, + }); + const { fetcher, tracked } = track(new Response(stream, { status: 401 })); + + const res = await fetcher("https://mcp.example/mcp", { method: "POST" }); + + const recorded = tracked[0]?.responseBody ?? ""; + expect(recorded.endsWith(TRUNCATED_BODY_SUFFIX)).toBe(true); + expect(recorded.length - TRUNCATED_BODY_SUFFIX.length).toBe( + CHALLENGE_BODY_MAX_BYTES, + ); + // With the clone released, cancelling the caller's branch reaches the source. + await res.body?.cancel(); + expect(cancel).toHaveBeenCalled(); + expect(pulls).toBeLessThan(10); + }); + + it("gives up on a stalled body at the deadline and releases the source", async () => { + vi.useFakeTimers(); + try { + const cancel = vi.fn(); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("partial")); + }, + cancel, + }); + const { fetcher, tracked } = track(new Response(stream, { status: 401 })); + + const pending = fetcher("https://mcp.example/mcp", { method: "POST" }); + await vi.advanceTimersByTimeAsync(CHALLENGE_BODY_READ_MS); + const res = await pending; + + expect(tracked[0]?.responseBody).toBe(`partial${TRUNCATED_BODY_SUFFIX}`); + await res.body?.cancel(); + expect(cancel).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it("records the entry with no body when the clone cannot be made", async () => { + const response = new Response("nope", { status: 401 }); + vi.spyOn(response, "clone").mockImplementation(() => { + throw new Error("clone failed"); + }); + const { fetcher, tracked } = track(response); + + await fetcher("https://mcp.example/mcp", { method: "POST" }); + + expect(tracked).toHaveLength(1); + expect(tracked[0]?.responseStatus).toBe(401); + expect(tracked[0]?.responseBody).toBeUndefined(); + }); + + it("records the entry with no body when the read errors", async () => { + const stream = new ReadableStream({ + pull(controller) { + controller.error(new Error("socket reset")); + }, + }); + const { fetcher, tracked } = track(new Response(stream, { status: 401 })); + + await fetcher("https://mcp.example/mcp", { method: "POST" }); + + expect(tracked[0]?.responseStatus).toBe(401); + expect(tracked[0]?.responseBody).toBeUndefined(); + }); +}); + describe("redactUrlQuery", () => { it("redacts sensitive params and keeps the path + other params", () => { expect( diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index 95481a30..eba85969 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -260,14 +260,14 @@ describe("Transport", () => { }); const fetchRequests: FetchRequestEntryBase[] = []; - const bodies = new Map(); + const onFetchResponseBody = vi.fn(); const result = createTransportNode( { type: "streamable-http", url: "https://mcp.example/mcp" }, { fetchFn, interceptAuthChallenges: true, onFetchRequest: (entry) => fetchRequests.push(entry), - onFetchResponseBody: (id, body) => bodies.set(id, body), + onFetchResponseBody, }, ); @@ -279,12 +279,15 @@ describe("Transport", () => { "MCP auth challenge (401)", ); + // The body is on the entry by the time the challenge is thrown — not a + // later update — so a caller that stops listening at the throw has it. expect(fetchRequests).toHaveLength(1); const [entry] = fetchRequests; expect(entry.error).toBeUndefined(); expect(entry.responseStatus).toBe(401); expect(entry.responseHeaders?.["www-authenticate"]).toBe(wwwAuthenticate); - await vi.waitFor(() => expect(bodies.get(entry.id)).toBe(challengeBody)); + expect(entry.responseBody).toBe(challengeBody); + expect(onFetchResponseBody).not.toHaveBeenCalled(); }); it("still throws an intercepted 401 whose body never ends (#2297)", async () => { @@ -319,6 +322,8 @@ describe("Transport", () => { "MCP auth challenge (401)", ); expect(fetchRequests[0]?.responseStatus).toBe(401); + // Cut off at the tracker's read deadline rather than buffered forever. + expect(fetchRequests[0]?.responseBody).toBe("partial\n[truncated]"); }); it("applies settings.headers to the outgoing streamable-http request", async () => { diff --git a/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts b/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts index 355f5bb8..ef1a7967 100644 --- a/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts +++ b/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts @@ -193,6 +193,69 @@ describe("server.ts /api/mcp/* branch coverage", () => { await new Promise((r) => upstream.server.close(() => r())); } }); + + it("relays a streamable-http 401's status and body over the event stream (#2297)", async () => { + const upstream = await startUnauthorizedUpstream(); + const events = new AbortController(); + try { + const connected = (await ( + await connect(h, { type: "streamable-http", url: upstream.url }) + ).json()) as { sessionId?: string }; + expect(connected.sessionId).toBeDefined(); + + const stream = await fetch( + `${h.baseUrl}/api/mcp/events?sessionId=${connected.sessionId}`, + { signal: events.signal }, + ); + const sent = await fetch(`${h.baseUrl}/api/mcp/send`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + sessionId: connected.sessionId, + message: { + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2025-06-18", + capabilities: {}, + clientInfo: { name: "test", version: "1.0.0" }, + }, + }, + }), + }); + expect(await sent.json()).toMatchObject({ + ok: false, + kind: "auth_challenge", + }); + + const reader = stream.body!.getReader(); + const decoder = new TextDecoder(); + const pattern = /data: (\{"type":"fetch_request",.*?\})\n/; + let text = ""; + let match: RegExpMatchArray | null = null; + while (!(match = text.match(pattern))) { + const { done, value } = await reader.read(); + if (done) break; + text += decoder.decode(value, { stream: true }); + } + const event = JSON.parse(match![1]!) as { + data: { + responseStatus?: number; + responseBody?: string; + error?: string; + }; + }; + expect(event.data.error).toBeUndefined(); + expect(event.data.responseStatus).toBe(401); + expect(event.data.responseBody).toBe( + JSON.stringify({ error: "unauthorized" }), + ); + } finally { + events.abort(); + await new Promise((r) => upstream.server.close(() => r())); + } + }); }); describe("POST /api/mcp/send", () => { diff --git a/core/mcp/fetchTracking.ts b/core/mcp/fetchTracking.ts index 7e1e5864..f756b1d4 100644 --- a/core/mcp/fetchTracking.ts +++ b/core/mcp/fetchTracking.ts @@ -223,6 +223,65 @@ export function isLongLivedStreamResponse( ); } +/** Most of a 401/403 body the tracker will buffer before it stops reading. */ +export const CHALLENGE_BODY_MAX_BYTES = 64 * 1024; +/** How long the tracker waits for a 401/403 body before giving up on the rest. */ +export const CHALLENGE_BODY_READ_MS = 2_000; +/** Appended to a 401/403 body the tracker stopped reading early. */ +export const TRUNCATED_BODY_SUFFIX = "\n[truncated]"; + +/** + * Read a 401/403 response body from a clone, bounded by + * {@link CHALLENGE_BODY_MAX_BYTES} and {@link CHALLENGE_BODY_READ_MS}. + * + * Unlike every other status, a challenge body is read *before* the entry is + * recorded (#2297). An auth-challenge interceptor above the tracker cancels the + * response and throws as soon as the tracker returns, and on the web backend + * that ends the connect request — so a body delivered later, as a separate + * update, arrives after anything could still carry it to the log. The bound is + * what makes waiting safe: a body that never ends is cut off, and cancelling the + * clone's reader lets the interceptor's cancel of the original release the + * connection instead of leaving the clone draining it. + */ +async function readChallengeBody( + response: Response, +): Promise { + let reader: ReadableStreamDefaultReader; + try { + const body = response.clone().body; + /* v8 ignore next -- the caller checked response.body, and a clone of a response with a body has one */ + if (!body) return undefined; + reader = body.getReader(); + } catch { + return undefined; + } + + const decoder = new TextDecoder(); + let text = ""; + let bytes = 0; + let timer: ReturnType | undefined; + const deadline = new Promise<"timeout">((resolve) => { + timer = setTimeout(() => resolve("timeout"), CHALLENGE_BODY_READ_MS); + }); + try { + while (bytes < CHALLENGE_BODY_MAX_BYTES) { + const next = await Promise.race([reader.read(), deadline]); + if (next === "timeout") break; + if (next.done) return text + decoder.decode(); + bytes += next.value.byteLength; + text += decoder.decode(next.value, { stream: true }); + } + // Stopped early. Not awaited: cancel() adopts the source's promise, which a + // stalled body may never settle. + reader.cancel().catch(() => {}); + return text + decoder.decode() + TRUNCATED_BODY_SUFFIX; + } catch { + return undefined; + } finally { + clearTimeout(timer); + } +} + export interface FetchTrackingCallbacks { trackRequest?: (entry: FetchRequestEntryBase) => void; /** @@ -376,6 +435,21 @@ export function createFetchTracker( duration, }; + if ( + (responseStatus === 401 || responseStatus === 403) && + !isLongLivedStream && + response.body && + !response.bodyUsed + ) { + const body = await readChallengeBody(response); + entry.responseBody = + body === undefined + ? undefined + : (redactBody(body, response.headers.get("content-type")) ?? body); + callbacks.trackRequest?.(entry); + return response; + } + callbacks.trackRequest?.(entry); // Kick off a fire-and-forget read of the cloned body. The clone is an diff --git a/core/mcp/node/authChallengeFetch.ts b/core/mcp/node/authChallengeFetch.ts index 6bfbf31f..1ed8e34a 100644 --- a/core/mcp/node/authChallengeFetch.ts +++ b/core/mcp/node/authChallengeFetch.ts @@ -26,8 +26,8 @@ export function createAuthChallengeInterceptFetch( // Release the connection before throwing so the SDK transport is not left // with a half-read 401/403 body on streamable HTTP. Not awaited: when the // network tracker sits below this wrapper (#2297) the body is one branch of - // a tee, and cancelling a branch settles only once the tracker's clone has - // read to the end — so a 401 whose body never ends would hang the throw. + // a tee, and cancel() adopts the source's promise, which a stalled body may + // never settle — so awaiting it could hang the throw. void response.body?.cancel().catch(() => {}); throw new AuthChallengeError( diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index 8ed872f4..45fc2c89 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -83,7 +83,8 @@ export function createTransportNode( // throws, so the entry was recorded from the thrown error alone — no status, // no `WWW-Authenticate`, and no response body, which is exactly the detail // someone debugging an OAuth challenge needs. Below it, the tracker records - // the real response and clones its body before the interceptor cancels it. + // the real response and reads its body (bounded) before the interceptor + // cancels it. const trackedBase = withTracking(withChallengeObserver(baseFetch)); const fetchWithOptionalAuthIntercept = interceptAuthChallenges ? createAuthChallengeInterceptFetch(trackedBase) From 1fd1b1515258167881f8bc210c3aecaeeff06e5d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 01:34:38 -0400 Subject: [PATCH 3/3] fix: enforce the challenge body cap per byte and never log cut-off JSON (#2297) Copilot review round 2: - Slice each chunk to the remaining budget, so one oversized chunk can no longer carry the recorded body past CHALLENGE_BODY_MAX_BYTES. - Drop the long-lived-stream exclusion from the challenge branch. It guards an unbounded read and this one is bounded, so a 401 to an SSE GET now records its body too. - Keep truncation out of the string that gets redacted. A cut-off body that looks like JSON cannot be redacted (redactBody only masks JSON that parses), so it is replaced by WITHHELD_TRUNCATED_JSON_BODY. Cut-off form and plain-text bodies are redacted, then marked [truncated]. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SG6q6YEDJyJB5H2Hr8nBDS Signed-off-by: cliffhall --- .../src/test/core/mcp/fetchTracking.test.ts | 159 ++++++++++++++++++ core/mcp/fetchTracking.ts | 60 ++++++- 2 files changed, 210 insertions(+), 9 deletions(-) diff --git a/clients/web/src/test/core/mcp/fetchTracking.test.ts b/clients/web/src/test/core/mcp/fetchTracking.test.ts index 4640b694..08c8566d 100644 --- a/clients/web/src/test/core/mcp/fetchTracking.test.ts +++ b/clients/web/src/test/core/mcp/fetchTracking.test.ts @@ -3,6 +3,7 @@ import { CHALLENGE_BODY_MAX_BYTES, CHALLENGE_BODY_READ_MS, TRUNCATED_BODY_SUFFIX, + WITHHELD_TRUNCATED_JSON_BODY, createFetchTracker, redactSensitiveHeaders, redactBody, @@ -659,6 +660,164 @@ describe("createFetchTracker — 401/403 challenge bodies (#2297)", () => { }); }); +describe("createFetchTracker — challenge body bounds and redaction (#2297)", () => { + function trackOne(response: Response) { + const tracked: FetchRequestEntryBase[] = []; + const fetcher = createFetchTracker((async () => response) as typeof fetch, { + trackRequest: (entry) => tracked.push(entry), + }); + return { fetcher, tracked }; + } + + /** A body that delivers `prefix` and then never ends. */ + function stalledBody(prefix: string): ReadableStream { + return new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(prefix)); + }, + }); + } + + async function withDeadlineElapsed(run: () => Promise): Promise { + vi.useFakeTimers(); + try { + const pending = run(); + await vi.advanceTimersByTimeAsync(CHALLENGE_BODY_READ_MS); + return await pending; + } finally { + vi.useRealTimers(); + } + } + + it("caps a single chunk larger than the budget at the byte limit", async () => { + const oversized = new Uint8Array(1024 * 1024).fill(97); + const { fetcher, tracked } = trackOne( + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(oversized); + controller.close(); + }, + }), + { status: 401 }, + ), + ); + + await fetcher("https://mcp.example/mcp", { method: "POST" }); + + const recorded = tracked[0]?.responseBody ?? ""; + expect(recorded.endsWith(TRUNCATED_BODY_SUFFIX)).toBe(true); + expect(recorded.length - TRUNCATED_BODY_SUFFIX.length).toBe( + CHALLENGE_BODY_MAX_BYTES, + ); + }); + + it("records the body of a 401 to a long-lived-stream GET", async () => { + const { fetcher, tracked } = trackOne( + new Response("unauthorized", { + status: 401, + headers: { "content-type": "text/event-stream" }, + }), + ); + + await fetcher("https://mcp.example/mcp", { method: "GET" }); + + expect(tracked[0]?.responseBody).toBe("unauthorized"); + }); + + it("withholds a cut-off JSON body rather than logging an unredacted prefix", async () => { + const { fetcher, tracked } = trackOne( + new Response(stalledBody('{"access_token":"live-access","token_type'), { + status: 401, + headers: { "content-type": "application/json" }, + }), + ); + + await withDeadlineElapsed(() => + fetcher("https://mcp.example/token", { method: "POST" }), + ); + + expect(tracked[0]?.responseBody).toBe(WITHHELD_TRUNCATED_JSON_BODY); + expect(serializedWithoutId(tracked[0]!)).not.toContain("live-access"); + }); + + it("withholds a cut-off body that sniffs as JSON with no content-type", async () => { + const { fetcher, tracked } = trackOne( + new Response(stalledBody(' [{"refresh_token":"live-refresh"'), { + status: 403, + }), + ); + + await withDeadlineElapsed(() => + fetcher("https://mcp.example/token", { method: "POST" }), + ); + + expect(tracked[0]?.responseBody).toBe(WITHHELD_TRUNCATED_JSON_BODY); + }); + + it("still redacts a complete JSON challenge body", async () => { + const { fetcher, tracked } = trackOne( + new Response(JSON.stringify({ error: "x", access_token: "live" }), { + status: 401, + headers: { "content-type": "application/json" }, + }), + ); + + await fetcher("https://mcp.example/token", { method: "POST" }); + + expect(tracked[0]?.responseBody).toBe( + JSON.stringify({ error: "x", access_token: REDACTED_VALUE }), + ); + }); + + it("redacts a cut-off form body field by field", async () => { + const { fetcher, tracked } = trackOne( + new Response(stalledBody("error=invalid&access_token=live&scope=re"), { + status: 401, + headers: { "content-type": "application/x-www-form-urlencoded" }, + }), + ); + + await withDeadlineElapsed(() => + fetcher("https://mcp.example/token", { method: "POST" }), + ); + + const recorded = tracked[0]?.responseBody ?? ""; + expect(recorded).not.toContain("live"); + expect(recorded.endsWith(TRUNCATED_BODY_SUFFIX)).toBe(true); + }); +}); + +describe("createFetchTracker — challenge body cancel rejection (#2297)", () => { + it("swallows the source's cancel() rejection after stopping early", async () => { + const chunk = new Uint8Array(CHALLENGE_BODY_MAX_BYTES).fill(97); + const stream = new ReadableStream({ + pull(controller) { + controller.enqueue(chunk); + }, + cancel() { + throw new Error("cancel failed"); + }, + }); + const tracked: FetchRequestEntryBase[] = []; + const fetcher = createFetchTracker( + (async () => new Response(stream, { status: 401 })) as typeof fetch, + { trackRequest: (entry) => tracked.push(entry) }, + ); + + const res = await fetcher("https://mcp.example/mcp", { method: "POST" }); + // Cancelling the caller's branch cancels the source, whose rejection then + // reaches the tracker's own cancel of its clone — which must swallow it. + await res.body?.cancel().catch(() => undefined); + await flush(); + + expect(tracked[0]?.responseStatus).toBe(401); + expect(tracked[0]?.responseBody?.endsWith(TRUNCATED_BODY_SUFFIX)).toBe( + true, + ); + }); +}); + describe("redactUrlQuery", () => { it("redacts sensitive params and keeps the path + other params", () => { expect( diff --git a/core/mcp/fetchTracking.ts b/core/mcp/fetchTracking.ts index f756b1d4..0a57fd03 100644 --- a/core/mcp/fetchTracking.ts +++ b/core/mcp/fetchTracking.ts @@ -150,6 +150,14 @@ function redactJsonValue(value: unknown): unknown { * (and other binary/opaque bodies) are passed through verbatim — OAuth never uses * multipart, so the risk is low; revisit if a multipart secret path appears. */ +export function redactBody( + body: string, + contentType: string | null | undefined, +): string; +export function redactBody( + body: string | undefined, + contentType: string | null | undefined, +): string | undefined; export function redactBody( body: string | undefined, contentType: string | null | undefined, @@ -229,6 +237,36 @@ export const CHALLENGE_BODY_MAX_BYTES = 64 * 1024; export const CHALLENGE_BODY_READ_MS = 2_000; /** Appended to a 401/403 body the tracker stopped reading early. */ export const TRUNCATED_BODY_SUFFIX = "\n[truncated]"; +/** + * Recorded in place of a 401/403 body that was cut off and looks like JSON. + * {@link redactBody} can only mask fields in JSON that parses, so a cut-off + * prefix such as `{"access_token":"…",` would otherwise reach the log verbatim. + */ +export const WITHHELD_TRUNCATED_JSON_BODY = + "[truncated JSON body withheld: an incomplete body cannot be redacted]"; + +interface ChallengeBodyRead { + text: string; + truncated: boolean; +} + +/** + * The recorded form of a 401/403 body: redacted, marked when cut off, and never + * a cut-off JSON prefix. Form-encoded bodies still redact when cut, since each + * surviving `key=value` pair is parsed on its own; plain text has nothing to + * redact either way. + */ +function recordableChallengeBody( + read: ChallengeBodyRead, + contentType: string | null, +): string { + if (!read.truncated) return redactBody(read.text, contentType); + const looksLikeJson = + (contentType ?? "").toLowerCase().includes("json") || + /^\s*[{[]/.test(read.text); + if (looksLikeJson) return WITHHELD_TRUNCATED_JSON_BODY; + return redactBody(read.text, contentType) + TRUNCATED_BODY_SUFFIX; +} /** * Read a 401/403 response body from a clone, bounded by @@ -245,7 +283,7 @@ export const TRUNCATED_BODY_SUFFIX = "\n[truncated]"; */ async function readChallengeBody( response: Response, -): Promise { +): Promise { let reader: ReadableStreamDefaultReader; try { const body = response.clone().body; @@ -267,14 +305,17 @@ async function readChallengeBody( while (bytes < CHALLENGE_BODY_MAX_BYTES) { const next = await Promise.race([reader.read(), deadline]); if (next === "timeout") break; - if (next.done) return text + decoder.decode(); - bytes += next.value.byteLength; - text += decoder.decode(next.value, { stream: true }); + if (next.done) return { text: text + decoder.decode(), truncated: false }; + // A stream may hand back one chunk far larger than the budget, so the cap + // is applied to the bytes, not to the chunk count. + const chunk = next.value.subarray(0, CHALLENGE_BODY_MAX_BYTES - bytes); + bytes += chunk.byteLength; + text += decoder.decode(chunk, { stream: true }); } // Stopped early. Not awaited: cancel() adopts the source's promise, which a // stalled body may never settle. reader.cancel().catch(() => {}); - return text + decoder.decode() + TRUNCATED_BODY_SUFFIX; + return { text: text + decoder.decode(), truncated: true }; } catch { return undefined; } finally { @@ -435,17 +476,18 @@ export function createFetchTracker( duration, }; + // Not gated on isLongLivedStream: that exclusion guards an unbounded read, + // and this one is bounded — so a challenge to an SSE GET is recorded too. if ( (responseStatus === 401 || responseStatus === 403) && - !isLongLivedStream && response.body && !response.bodyUsed ) { - const body = await readChallengeBody(response); + const read = await readChallengeBody(response); entry.responseBody = - body === undefined + read === undefined ? undefined - : (redactBody(body, response.headers.get("content-type")) ?? body); + : recordableChallengeBody(read, response.headers.get("content-type")); callbacks.trackRequest?.(entry); return response; }