fix(web): reject a reshaped replay param, and forward a declared JSON body
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
4e7537ea76
commit
0e9069a78d
7 changed files with 186 additions and 7 deletions
|
|
@ -178,6 +178,25 @@ describe("EditReplayModal", () => {
|
||||||
).toBeInTheDocument();
|
).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(<EditReplayModal {...baseProps} method="tools/call" />);
|
||||||
|
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(<EditReplayModal {...baseProps} method="tools/call" />);
|
||||||
|
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 () => {
|
it("closes without sending when cancelled", async () => {
|
||||||
const user = userEvent.setup();
|
const user = userEvent.setup();
|
||||||
const onSend = vi.fn();
|
const onSend = vi.fn();
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,10 @@ import { useState } from "react";
|
||||||
import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core";
|
import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core";
|
||||||
import { JsonEditor } from "../../elements/JsonEditor/JsonEditor";
|
import { JsonEditor } from "../../elements/JsonEditor/JsonEditor";
|
||||||
import { isJsonObject } from "../../../utils/jsonObjectDraft";
|
import { isJsonObject } from "../../../utils/jsonObjectDraft";
|
||||||
import { replayableParams } from "../../../lib/protocolReplay";
|
import {
|
||||||
|
replayableParams,
|
||||||
|
reshapedReplayParam,
|
||||||
|
} from "../../../lib/protocolReplay";
|
||||||
import { isSerializableJson } from "@inspector/core/json/jsonUtils.js";
|
import { isSerializableJson } from "@inspector/core/json/jsonUtils.js";
|
||||||
|
|
||||||
export interface EditReplayModalProps {
|
export interface EditReplayModalProps {
|
||||||
|
|
@ -106,6 +109,12 @@ function parseParamsDraft(
|
||||||
} so this matches what is sent`,
|
} 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 };
|
return { ok: true, value: parsed };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
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 type { FetchRequestEntry } from "../../../../../../core/mcp/types.js";
|
||||||
import { NetworkEntry } from "./NetworkEntry";
|
import { NetworkEntry } from "./NetworkEntry";
|
||||||
|
|
||||||
|
|
@ -104,6 +104,26 @@ export const TransportSuccessExpanded: Story = {
|
||||||
args: { entry: transportEntry, isListExpanded: true },
|
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 = {
|
export const AuthSuccess: Story = {
|
||||||
args: { entry: authEntry, isListExpanded: true },
|
args: { entry: authEntry, isListExpanded: true },
|
||||||
play: async ({ canvasElement }) => {
|
play: async ({ canvasElement }) => {
|
||||||
|
|
@ -113,10 +133,12 @@ export const AuthSuccess: Story = {
|
||||||
// must not be visible until explicitly revealed.
|
// must not be visible until explicitly revealed.
|
||||||
const hidden = canvas.getAllByText("Secrets hidden");
|
const hidden = canvas.getAllByText("Secrets hidden");
|
||||||
await expect(hidden.length).toBeGreaterThanOrEqual(2);
|
await expect(hidden.length).toBeGreaterThanOrEqual(2);
|
||||||
await expect(canvasElement.textContent).not.toContain("eyJhbGciOiJSUzI1");
|
await expect(renderedBodyText(canvasElement)).not.toContain(
|
||||||
await expect(canvasElement.textContent).toContain("••••••••");
|
"eyJhbGciOiJSUzI1",
|
||||||
|
);
|
||||||
|
await expect(renderedBodyText(canvasElement)).toContain("••••••••");
|
||||||
// Non-secret fields stay visible.
|
// 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.
|
// Reveal every masked body and confirm the raw response token appears.
|
||||||
const revealButtons = canvas.getAllByRole("button", {
|
const revealButtons = canvas.getAllByRole("button", {
|
||||||
|
|
@ -128,7 +150,11 @@ export const AuthSuccess: Story = {
|
||||||
await expect(
|
await expect(
|
||||||
canvas.getAllByText("Secrets revealed").length,
|
canvas.getAllByText("Secrets revealed").length,
|
||||||
).toBeGreaterThanOrEqual(revealButtons.length);
|
).toBeGreaterThanOrEqual(revealButtons.length);
|
||||||
await expect(canvasElement.textContent).toContain("eyJhbGciOiJSUzI1");
|
await waitFor(async () => {
|
||||||
|
await expect(renderedBodyText(canvasElement)).toContain(
|
||||||
|
"eyJhbGciOiJSUzI1",
|
||||||
|
);
|
||||||
|
});
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -7,7 +7,7 @@ import {
|
||||||
screen,
|
screen,
|
||||||
waitFor,
|
waitFor,
|
||||||
} from "../../../test/renderWithMantine";
|
} from "../../../test/renderWithMantine";
|
||||||
import { getAceText } from "../../../test/aceEditor";
|
import { getAceText, getAceTextByLabel } from "../../../test/aceEditor";
|
||||||
import { NetworkEntry } from "./NetworkEntry";
|
import { NetworkEntry } from "./NetworkEntry";
|
||||||
|
|
||||||
const baseEntry: FetchRequestEntry = {
|
const baseEntry: FetchRequestEntry = {
|
||||||
|
|
@ -209,6 +209,35 @@ describe("NetworkEntry", () => {
|
||||||
expect(screen.queryByText("Response Body")).not.toBeInTheDocument();
|
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(<NetworkEntry entry={malformed} isListExpanded={true} />);
|
||||||
|
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: "<p>hi</p>",
|
||||||
|
};
|
||||||
|
renderWithMantine(<NetworkEntry entry={html} isListExpanded={true} />);
|
||||||
|
expect(screen.getByText("<p>hi</p>")).toBeInTheDocument();
|
||||||
|
expect(document.querySelector("iframe")).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
it("shows a 'too large' notice when a body exceeds the inline preview limit", async () => {
|
it("shows a 'too large' notice when a body exceeds the inline preview limit", async () => {
|
||||||
const user = userEvent.setup();
|
const user = userEvent.setup();
|
||||||
const huge = "x".repeat(150_000);
|
const huge = "x".repeat(150_000);
|
||||||
|
|
|
||||||
|
|
@ -17,6 +17,7 @@ import { RiErrorWarningLine } from "react-icons/ri";
|
||||||
import type { FetchRequestEntry } from "@inspector/core/mcp/types.js";
|
import type { FetchRequestEntry } from "@inspector/core/mcp/types.js";
|
||||||
import { isLongLivedStreamResponse } from "@inspector/core/mcp/fetchTracking.js";
|
import { isLongLivedStreamResponse } from "@inspector/core/mcp/fetchTracking.js";
|
||||||
import { ContentViewer } from "../../elements/ContentViewer/ContentViewer";
|
import { ContentViewer } from "../../elements/ContentViewer/ContentViewer";
|
||||||
|
import { getMimeKind } from "../../elements/ContentViewer/contentViewerUtils";
|
||||||
import { CopyButton } from "../../elements/CopyButton/CopyButton";
|
import { CopyButton } from "../../elements/CopyButton/CopyButton";
|
||||||
import { ExpandToggle } from "../../elements/ExpandToggle/ExpandToggle";
|
import { ExpandToggle } from "../../elements/ExpandToggle/ExpandToggle";
|
||||||
import { MethodBadge } from "../../elements/MethodBadge/MethodBadge";
|
import { MethodBadge } from "../../elements/MethodBadge/MethodBadge";
|
||||||
|
|
@ -391,6 +392,23 @@ function BodyPreview({
|
||||||
|
|
||||||
const tooLarge = body.length > MAX_INLINE_BODY_CHARS;
|
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
|
// OAuth responses (token exchange, DCR) and the token request carry
|
||||||
// bearer-grade secrets. Mask them by default and gate the raw values behind
|
// 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
|
// an explicit reveal so they aren't exposed at a glance during a
|
||||||
|
|
@ -424,6 +442,7 @@ function BodyPreview({
|
||||||
<ContentViewer
|
<ContentViewer
|
||||||
block={{ type: "text", text: body }}
|
block={{ type: "text", text: body }}
|
||||||
copyable
|
copyable
|
||||||
|
mimeType={jsonMimeType}
|
||||||
jsonLabel={`${label} JSON`}
|
jsonLabel={`${label} JSON`}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
|
|
@ -448,6 +467,9 @@ function BodyPreview({
|
||||||
<ContentViewer
|
<ContentViewer
|
||||||
block={{ type: "text", text: shown }}
|
block={{ type: "text", text: shown }}
|
||||||
copyable
|
copyable
|
||||||
|
// The masked form is what is rendered, and masking can leave a
|
||||||
|
// still-valid JSON document — so the declared type still applies.
|
||||||
|
mimeType={jsonMimeType}
|
||||||
jsonLabel={`${label} JSON`}
|
jsonLabel={`${label} JSON`}
|
||||||
/>
|
/>
|
||||||
</Stack>
|
</Stack>
|
||||||
|
|
|
||||||
|
|
@ -5,6 +5,7 @@ import {
|
||||||
messagesToLogEntries,
|
messagesToLogEntries,
|
||||||
replayProtocolRequest,
|
replayProtocolRequest,
|
||||||
replayableParams,
|
replayableParams,
|
||||||
|
reshapedReplayParam,
|
||||||
type ReplayClient,
|
type ReplayClient,
|
||||||
} from "./protocolReplay";
|
} 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", () => {
|
describe("replayProtocolRequest", () => {
|
||||||
const tool: Tool = { name: "echo", inputSchema: { type: "object" } };
|
const tool: Tool = { name: "echo", inputSchema: { type: "object" } };
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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<string, unknown> | 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(
|
export async function replayProtocolRequest(
|
||||||
client: ReplayClient,
|
client: ReplayClient,
|
||||||
method: string,
|
method: string,
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue