From 1fd1b1515258167881f8bc210c3aecaeeff06e5d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 01:34:38 -0400 Subject: [PATCH] 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; }