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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SG6q6YEDJyJB5H2Hr8nBDS
Signed-off-by: cliffhall <cliff@futurescale.com>

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<Uint8Array> {
+    return new ReadableStream<Uint8Array>({
+      start(controller) {
+        controller.enqueue(new TextEncoder().encode(prefix));
+      },
+    });
+  }
+
+  async function withDeadlineElapsed<T>(run: () => Promise<T>): Promise<T> {
+    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<Uint8Array>({
+          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<Uint8Array>({
+      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(

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<string | undefined> {
+): Promise<ChallengeBodyRead | undefined> {
   let reader: ReadableStreamDefaultReader<Uint8Array>;
   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;
     }