From 0e9069a78dc1609a3ccfff2b4bbd8900e6df9687 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 04:17:19 -0400 Subject: [PATCH] fix(web): reject a reshaped replay param, and forward a declared JSON body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's sixth pass generated no new inline comments; both suppressed ones were right. - `arguments` surviving the key check is not the same as being sent as written. `replayProtocolRequest` applies `arguments ?? {}`, so a `null` shown in the editor reaches the wire as `{}`; an array or scalar is not nullish and rides the cast into a call whose type says it is a named-argument record. `reshapedReplayParam` rejects both before Send. `name` and `uri` are deliberately not checked there — the dispatch already refuses those with a reason the caller shows as a toast, so they fail visibly rather than silently. - `NetworkEntry` never forwarded the body's content type, so the Network tab could not reach the declared-JSON branch at all and a malformed `application/json` response read as plain text with no sign it was meant to be JSON. Forwarded — but only when the declared type *is* JSON: passing it wholesale would also route an HTML body into the sandboxed frame and a CSV one into a table, which is a change to what the Network tab is and not one this makes. The `NetworkEntry > Auth Success` play function moved off `textContent` for body assertions. Ace repaints its rows on an animation frame and virtualizes them, so reading the DOM straight after Reveal returned the previous rows under load — it passed in isolation and failed in the full run. It now reads the editors' documents alongside the DOM text, and waits. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../EditReplayModal/EditReplayModal.test.tsx | 19 +++++++++ .../EditReplayModal/EditReplayModal.tsx | 11 +++++- .../NetworkEntry/NetworkEntry.stories.tsx | 36 ++++++++++++++--- .../groups/NetworkEntry/NetworkEntry.test.tsx | 31 ++++++++++++++- .../groups/NetworkEntry/NetworkEntry.tsx | 22 +++++++++++ clients/web/src/lib/protocolReplay.test.ts | 39 +++++++++++++++++++ clients/web/src/lib/protocolReplay.ts | 35 +++++++++++++++++ 7 files changed, 186 insertions(+), 7 deletions(-) diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index ad49c770..b7160a79 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -178,6 +178,25 @@ describe("EditReplayModal", () => { ).toBeInTheDocument(); }); + // Surviving the key check is not the same as being sent as written: + // `replayProtocolRequest` applies `arguments ?? {}`, so a null shown in the + // editor reaches the wire as `{}`. + it("disables Send for a null arguments, which would be sent as {}", async () => { + renderWithMantine(); + await setAceText('{"name":"echo","arguments":null}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect(screen.getByText(/sent as `\{\}` when null/)).toBeInTheDocument(); + }); + + it("disables Send for a non-object arguments", async () => { + renderWithMantine(); + await setAceText('{"name":"echo","arguments":[1,2]}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect( + screen.getByText(/`arguments` must be a JSON object/), + ).toBeInTheDocument(); + }); + it("closes without sending when cancelled", async () => { const user = userEvent.setup(); const onSend = vi.fn(); diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx index d8fcfb83..eb28d3e1 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -2,7 +2,10 @@ import { useState } from "react"; import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { isJsonObject } from "../../../utils/jsonObjectDraft"; -import { replayableParams } from "../../../lib/protocolReplay"; +import { + replayableParams, + reshapedReplayParam, +} from "../../../lib/protocolReplay"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; export interface EditReplayModalProps { @@ -106,6 +109,12 @@ function parseParamsDraft( } so this matches what is sent`, }; } + // Surviving the key check is not the same as being sent as written: see + // `reshapedReplayParam` for the two ways `arguments` does not. + const reshaped = reshapedReplayParam(method, parsed); + if (reshaped !== null) { + return { ok: false, error: reshaped }; + } return { ok: true, value: parsed }; } diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx index a4f05254..21349a41 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx @@ -1,5 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, userEvent, within } from "storybook/test"; +import { expect, userEvent, waitFor, within } from "storybook/test"; import type { FetchRequestEntry } from "../../../../../../core/mcp/types.js"; import { NetworkEntry } from "./NetworkEntry"; @@ -104,6 +104,26 @@ export const TransportSuccessExpanded: Story = { args: { entry: transportEntry, isListExpanded: true }, }; +/** + * Every rendered body, as text, however each one is rendered. + * + * A JSON body renders in Ace (#2151), which repaints its rows on an animation + * frame — so reading `canvasElement.textContent` straight after a state change + * can still return the previous rows, and does under load. It also virtualizes, + * so only what its viewport covers is in the DOM at all. Reading the editors' + * documents alongside the DOM text is immediate and complete; the DOM half is + * kept because a body that renders as plain text is not in any editor. + */ +function renderedBodyText(canvasElement: HTMLElement): string { + const editors = Array.from(canvasElement.querySelectorAll(".ace_editor")).map( + (node) => + ( + node as HTMLElement & { env?: { editor?: { getValue(): string } } } + ).env?.editor?.getValue() ?? "", + ); + return [canvasElement.textContent ?? "", ...editors].join("\n"); +} + export const AuthSuccess: Story = { args: { entry: authEntry, isListExpanded: true }, play: async ({ canvasElement }) => { @@ -113,10 +133,12 @@ export const AuthSuccess: Story = { // must not be visible until explicitly revealed. const hidden = canvas.getAllByText("Secrets hidden"); await expect(hidden.length).toBeGreaterThanOrEqual(2); - await expect(canvasElement.textContent).not.toContain("eyJhbGciOiJSUzI1"); - await expect(canvasElement.textContent).toContain("••••••••"); + await expect(renderedBodyText(canvasElement)).not.toContain( + "eyJhbGciOiJSUzI1", + ); + await expect(renderedBodyText(canvasElement)).toContain("••••••••"); // Non-secret fields stay visible. - await expect(canvasElement.textContent).toContain("Bearer"); + await expect(renderedBodyText(canvasElement)).toContain("Bearer"); // Reveal every masked body and confirm the raw response token appears. const revealButtons = canvas.getAllByRole("button", { @@ -128,7 +150,11 @@ export const AuthSuccess: Story = { await expect( canvas.getAllByText("Secrets revealed").length, ).toBeGreaterThanOrEqual(revealButtons.length); - await expect(canvasElement.textContent).toContain("eyJhbGciOiJSUzI1"); + await waitFor(async () => { + await expect(renderedBodyText(canvasElement)).toContain( + "eyJhbGciOiJSUzI1", + ); + }); }, }; diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx index 0f8c2250..e42d841c 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx @@ -7,7 +7,7 @@ import { screen, waitFor, } from "../../../test/renderWithMantine"; -import { getAceText } from "../../../test/aceEditor"; +import { getAceText, getAceTextByLabel } from "../../../test/aceEditor"; import { NetworkEntry } from "./NetworkEntry"; const baseEntry: FetchRequestEntry = { @@ -209,6 +209,35 @@ describe("NetworkEntry", () => { expect(screen.queryByText("Response Body")).not.toBeInTheDocument(); }); + // The declared content type reaches `ContentViewer`, so a body the server + // *said* was JSON renders as JSON even when it does not parse. Without it + // the Network tab could only ever reach the parse-it-first heuristic, and a + // malformed JSON response read as plain text with no sign it was meant to be + // JSON. + it("renders a malformed but declared JSON body as JSON", () => { + const malformed: FetchRequestEntry = { + ...baseEntry, + responseHeaders: { "content-type": "application/json" }, + responseBody: '{"ok":', + }; + renderWithMantine(); + expect(getAceTextByLabel(/Response body JSON/)).toBe('{"ok":'); + }); + + // Narrowed to JSON on purpose: forwarding the content type wholesale would + // route an HTML body into the sandboxed frame and a CSV one into a table, + // which is a change to what the Network tab is. + it("does not route a non-JSON declared body to another renderer", () => { + const html: FetchRequestEntry = { + ...baseEntry, + responseHeaders: { "content-type": "text/html" }, + responseBody: "

hi

", + }; + renderWithMantine(); + expect(screen.getByText("

hi

")).toBeInTheDocument(); + expect(document.querySelector("iframe")).toBeNull(); + }); + it("shows a 'too large' notice when a body exceeds the inline preview limit", async () => { const user = userEvent.setup(); const huge = "x".repeat(150_000); diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index 399cdfe4..8aecb0fc 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -17,6 +17,7 @@ import { RiErrorWarningLine } from "react-icons/ri"; import type { FetchRequestEntry } from "@inspector/core/mcp/types.js"; import { isLongLivedStreamResponse } from "@inspector/core/mcp/fetchTracking.js"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { getMimeKind } from "../../elements/ContentViewer/contentViewerUtils"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { ExpandToggle } from "../../elements/ExpandToggle/ExpandToggle"; import { MethodBadge } from "../../elements/MethodBadge/MethodBadge"; @@ -391,6 +392,23 @@ function BodyPreview({ const tooLarge = body.length > MAX_INLINE_BODY_CHARS; + // Forward the declared MIME, but only when it is JSON. + // + // `ContentViewer` renders a *declared* `application/json` as JSON even when + // it does not parse — the server said what it sent — while an undeclared + // body only gets that treatment if it parses. Without this the Network tab + // could never reach the declared branch, so a malformed JSON response read + // as plain text with no indication that it was meant to be JSON. + // + // Narrowed to JSON on purpose: forwarding the content type wholesale would + // also route a `text/html` body into the sandboxed HTML frame and a + // `text/csv` one into a table. Turning wire bodies into rendered documents + // is a real change to what the Network tab is, and not one this makes. + const jsonMimeType = + contentType && getMimeKind(contentType) === "json" + ? contentType + : undefined; + // OAuth responses (token exchange, DCR) and the token request carry // bearer-grade secrets. Mask them by default and gate the raw values behind // an explicit reveal so they aren't exposed at a glance during a @@ -424,6 +442,7 @@ function BodyPreview({ ); @@ -448,6 +467,9 @@ function BodyPreview({ diff --git a/clients/web/src/lib/protocolReplay.test.ts b/clients/web/src/lib/protocolReplay.test.ts index b60b0174..35c80e3b 100644 --- a/clients/web/src/lib/protocolReplay.test.ts +++ b/clients/web/src/lib/protocolReplay.test.ts @@ -5,6 +5,7 @@ import { messagesToLogEntries, replayProtocolRequest, replayableParams, + reshapedReplayParam, type ReplayClient, } from "./protocolReplay"; @@ -135,6 +136,44 @@ describe("replayableParams", () => { }); }); +// `replayableParams` answers which keys survive; this answers whether the +// surviving ones survive intact. +describe("reshapedReplayParam", () => { + it("accepts an object arguments", () => { + expect( + reshapedReplayParam("tools/call", { name: "x", arguments: { a: 1 } }), + ).toBeNull(); + }); + + it("accepts an absent arguments", () => { + expect(reshapedReplayParam("tools/call", { name: "x" })).toBeNull(); + }); + + // The sharp case: `?? {}` replaces a null, so the editor shows `null` and the + // wire carries `{}`. + it("rejects a null arguments, which is sent as an empty object", () => { + expect( + reshapedReplayParam("tools/call", { name: "x", arguments: null }), + ).toMatch(/sent as `\{\}` when null/); + }); + + it.each([ + ["an array", [1, 2]], + ["a number", 4], + ["a string", "a"], + ])("rejects %s arguments", (_label, args) => { + expect( + reshapedReplayParam("prompts/get", { name: "x", arguments: args }), + ).toMatch(/must be a JSON object/); + }); + + it("has nothing to say about methods that take no arguments", () => { + expect(reshapedReplayParam("tools/list", { cursor: "a" })).toBeNull(); + expect(reshapedReplayParam("resources/read", { uri: "x://y" })).toBeNull(); + expect(reshapedReplayParam("ping", undefined)).toBeNull(); + }); +}); + describe("replayProtocolRequest", () => { const tool: Tool = { name: "echo", inputSchema: { type: "object" } }; diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 5bee9026..47fcff52 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -133,6 +133,41 @@ export function replayableParams( }; } +/** + * A param the dispatch below would *reshape* on the way out, or null when every + * kept param is sent as written. + * + * {@link replayableParams} answers which keys survive; this answers whether the + * surviving ones survive *intact*. Only `arguments` can fail that today, and it + * fails two ways, both of which the Edit-and-replay editor has to catch before + * Send: + * + * - `arguments: null` is nullish, so `?? {}` replaces it — the editor shows + * `null` and the wire carries `{}`. + * - `arguments: [1,2]` or `arguments: 4` is not nullish, so the cast carries it + * through unchanged into a call whose type says it is a named-argument + * record. Nothing reshapes it, but nothing can send it either. + * + * `name` and `uri` are deliberately not checked here: the dispatch already + * refuses a missing or non-string one with a reason the caller surfaces as a + * toast, so those fail visibly rather than silently. + */ +export function reshapedReplayParam( + method: string, + params: Record | undefined, +): string | null { + if (!params) return null; + if (method !== "tools/call" && method !== "prompts/get") return null; + if (!Object.hasOwn(params, "arguments")) return null; + const args = params.arguments; + const isRecord = + args !== null && typeof args === "object" && !Array.isArray(args); + if (isRecord) return null; + return args === null + ? "`arguments` is sent as `{}` when null — remove it, or give it an object" + : "`arguments` must be a JSON object (`{ … }`)"; +} + export async function replayProtocolRequest( client: ReplayClient, method: string,