From 8f4aa7f8afecf2462043e9205e2fbadff5d3c483 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 04:34:45 -0400 Subject: [PATCH] fix(web): reject drafts whose digits or types would not survive being sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's seventh pass, three comments, all the same class: the editor showing one value while the wire carries another. - `JSON.parse` rounds a whole number past 2^53−1 to the nearest double, so a draft reading `{"id":9007199254740993}` submits `…992`. New `hasRoundedInteger`, applied by all four JSON-draft parsers — the raw arguments editor, `SchemaJsonField`, the Edit-and-replay modal, and `parseJsonObjectDraft`, which Copilot did not flag but shares the defect. It is narrower than "unsafe integer": it also requires the number's shortest form to be written out in full, because JS stringifies from 1e21 upward in exponent form and such a value was typed that way — `{"n":1e308}` is a legitimate `_meta` value that `parseJsonObjectDraft`'s tests already pin as accepted. It still refuses the rare full-form integer that happens to be exactly representable, erring toward refusing a sendable value rather than misreporting an unsendable one — the trade `toNumericValue` already makes. - `getPrompt` runs `convertPromptArguments`, which JSON-stringifies every non-string argument, so `{"count":2}` leaves as `{"count":"2"}` while the editor still shows a number. `reshapedReplayParam` now rejects a non-string prompt argument. Not a workaround: the spec types `GetPromptRequest.params.arguments` as `Record`, so a string is the only thing a prompt argument can be. `tools/call` is untouched — `callTool` sends its arguments as given. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../EditReplayModal/EditReplayModal.test.tsx | 20 +++++++ .../EditReplayModal/EditReplayModal.tsx | 9 +++- .../groups/SchemaForm/SchemaForm.test.tsx | 16 ++++++ .../groups/SchemaForm/SchemaForm.tsx | 23 ++++++-- clients/web/src/lib/protocolReplay.test.ts | 32 ++++++++++++ clients/web/src/lib/protocolReplay.ts | 29 ++++++++--- clients/web/src/utils/jsonObjectDraft.test.ts | 48 ++++++++++++++++- clients/web/src/utils/jsonObjectDraft.ts | 52 +++++++++++++++++++ 8 files changed, 216 insertions(+), 13 deletions(-) diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index b7160a79..e43deaa8 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -197,6 +197,26 @@ describe("EditReplayModal", () => { ).toBeInTheDocument(); }); + // `JSON.parse` rounds a whole number past 2^53−1 to the nearest double, so + // the draft shows digits the wire will not carry. + it("disables Send for a whole number that loses digits when parsed", async () => { + renderWithMantine(); + await setAceText('{"name":"echo","arguments":{"id":9007199254740993}}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect( + screen.getByText(/Whole numbers must be within/), + ).toBeInTheDocument(); + }); + + it("disables Send for a non-string prompt argument", async () => { + renderWithMantine(); + await setAceText('{"name":"greet","arguments":{"count":2}}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect( + screen.getByText(/`count` would be sent as JSON text/), + ).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 eb28d3e1..c70d258f 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -1,7 +1,11 @@ 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 { + hasRoundedInteger, + isJsonObject, + ROUNDED_INTEGER_ERROR, +} from "../../../utils/jsonObjectDraft"; import { replayableParams, reshapedReplayParam, @@ -95,6 +99,9 @@ function parseParamsDraft( "Numbers must be finite — a value like `1e400` overflows and would be sent as null", }; } + if (hasRoundedInteger(parsed)) { + return { ok: false, error: ROUNDED_INTEGER_ERROR }; + } // Seeding from the projection is only half of it: a key the dispatcher does // not read can also be *added* to the draft, and Send would then discard an // edit the user was looking at. Checked with the same function the seed is diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 08eab255..485beb5a 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1913,6 +1913,22 @@ describe("SchemaForm raw JSON (#2151)", () => { expect(screen.getByText(/Numbers must be finite/)).toBeInTheDocument(); }); + // The quieter half of the same defect the `1e400` case covers: a whole number + // past 2^53−1 is rounded by `JSON.parse`, so the editor would show digits the + // wire will not carry. + it("blocks submission for a whole number that loses digits when parsed", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine(); + await enableRawJson(user); + + await setAceTextByLabel(/Arguments JSON/, '{"id":9007199254740993}'); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + expect( + screen.getByText(/Whole numbers must be within/), + ).toBeInTheDocument(); + }); + // Nothing in `values` names a branch, so the picker's index is held by the // form. Editing the discriminator in the raw document is a change to which // branch is in effect, and it carries no other signal — without the re-derive diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index a24649a8..e533c5fc 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -24,7 +24,11 @@ import { import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { EnlargeButton } from "../../elements/EnlargeButton/EnlargeButton"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; -import { isJsonObject } from "../../../utils/jsonObjectDraft"; +import { + hasRoundedInteger, + isJsonObject, + ROUNDED_INTEGER_ERROR, +} from "../../../utils/jsonObjectDraft"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; import type { @@ -280,9 +284,10 @@ function toNumericValue(raw: string | number): number | undefined { * That covers text that is not JSON *yet* — the mid-edit state this field's * whole draft/value split exists for — and text that parses but does not * survive being written back out: `JSON.parse("1e400")` yields `Infinity`, - * which `JSON.stringify` writes as `null`. Both report no value, so the field - * shows its error and `onValidityChange` blocks submission, rather than sending - * a number the user never typed. Same guard, same reason, as + * which `JSON.stringify` writes as `null`, and a whole number past 2^53−1 is + * rounded to the nearest double. All three report no value, so the field shows + * its error and `onValidityChange` blocks submission, rather than sending a + * number the user never typed. Same guards, same reason, as * `parseJsonObjectDraft` and the raw-arguments editor above. */ function parseJsonDraft(text: string): unknown { @@ -295,7 +300,10 @@ function parseJsonDraft(text: string): unknown { } catch { return undefined; } - return isSerializableJson(parsed) ? parsed : undefined; + if (!isSerializableJson(parsed) || hasRoundedInteger(parsed)) { + return undefined; + } + return parsed; } /** Structural equality for the draft/value re-sync, via canonical JSON. */ @@ -609,6 +617,11 @@ function parseRawArgumentsDraft( "Numbers must be finite — a value like `1e400` overflows and would be sent as null", }; } + // The quieter half of the same defect: a whole number past 2^53−1 parses to + // the nearest double, so the draft shows digits the wire will not carry. + if (hasRoundedInteger(parsed)) { + return { ok: false, error: ROUNDED_INTEGER_ERROR }; + } return { ok: true, value: parsed }; } diff --git a/clients/web/src/lib/protocolReplay.test.ts b/clients/web/src/lib/protocolReplay.test.ts index 35c80e3b..6e1d38cb 100644 --- a/clients/web/src/lib/protocolReplay.test.ts +++ b/clients/web/src/lib/protocolReplay.test.ts @@ -167,6 +167,38 @@ describe("reshapedReplayParam", () => { ).toMatch(/must be a JSON object/); }); + // `getPrompt` runs `convertPromptArguments`, which JSON-stringifies anything + // that is not already a string — so `{count: 2}` goes out as `{count: "2"}` + // while the editor still shows the number. The spec types a prompt argument + // as a string, so this is the protocol, not a quirk to route around. + it("rejects a non-string prompt argument, which is stringified on the way out", () => { + expect( + reshapedReplayParam("prompts/get", { + name: "greet", + arguments: { count: 2, who: "ada" }, + }), + ).toMatch(/`count` would be sent as JSON text/); + }); + + it("accepts string prompt arguments", () => { + expect( + reshapedReplayParam("prompts/get", { + name: "greet", + arguments: { count: "2", who: "ada" }, + }), + ).toBeNull(); + }); + + // `callTool` sends its arguments as given, so the same shape is fine there. + it("leaves a tool call's non-string arguments alone", () => { + expect( + reshapedReplayParam("tools/call", { + name: "add", + arguments: { a: 1, b: 2 }, + }), + ).toBeNull(); + }); + 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(); diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 47fcff52..768909d2 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -138,8 +138,8 @@ export function replayableParams( * 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 + * surviving ones survive *intact*. Only `arguments` can fail that, and it fails + * three ways, each of which the Edit-and-replay editor has to catch before * Send: * * - `arguments: null` is nullish, so `?? {}` replaces it — the editor shows @@ -147,6 +147,13 @@ export function replayableParams( * - `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. + * - For **`prompts/get` only**, a non-string *value*. `getPrompt` runs + * `convertPromptArguments`, which `JSON.stringify`s anything that is not + * already a string — so `{"count": 2}` is sent as `{"count": "2"}` while the + * editor still shows the number. This is not a quirk to route around: the + * spec types `GetPromptRequest.params.arguments` as `Record`, + * so a string is the only thing a prompt argument can be. `tools/call` is + * unaffected — `callTool` sends its arguments as given. * * `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 @@ -162,10 +169,20 @@ export function reshapedReplayParam( 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 (`{ … }`)"; + if (!isRecord) { + return args === null + ? "`arguments` is sent as `{}` when null — remove it, or give it an object" + : "`arguments` must be a JSON object (`{ … }`)"; + } + if (method === "prompts/get") { + const coerced = Object.entries(args as Record) + .filter(([, value]) => typeof value !== "string") + .map(([key]) => `\`${key}\``); + if (coerced.length > 0) { + return `A prompt argument is always a string — ${coerced.join(", ")} would be sent as JSON text`; + } + } + return null; } export async function replayProtocolRequest( diff --git a/clients/web/src/utils/jsonObjectDraft.test.ts b/clients/web/src/utils/jsonObjectDraft.test.ts index 51e835a4..5e8be685 100644 --- a/clients/web/src/utils/jsonObjectDraft.test.ts +++ b/clients/web/src/utils/jsonObjectDraft.test.ts @@ -1,5 +1,51 @@ import { describe, it, expect } from "vitest"; -import { parseJsonObjectDraft } from "./jsonObjectDraft"; +import { hasRoundedInteger, parseJsonObjectDraft } from "./jsonObjectDraft"; + +// `isSerializableJson` catches a literal too large to represent (`1e400` → +// `Infinity` → `null` on the wire). This is the quieter one: a whole number +// past 2^53−1 parses to the nearest double, so the draft shows digits the wire +// will not carry. +describe("hasRoundedInteger", () => { + it("accepts whole numbers inside the safe range", () => { + expect(hasRoundedInteger({ id: Number.MAX_SAFE_INTEGER })).toBe(false); + expect(hasRoundedInteger({ id: -Number.MAX_SAFE_INTEGER })).toBe(false); + expect(hasRoundedInteger({ id: 0 })).toBe(false); + }); + + it("rejects a whole number past the safe range", () => { + // Typed as …93, parsed as …92. + expect(hasRoundedInteger(JSON.parse('{"id":9007199254740993}'))).toBe(true); + }); + + // A fractional or exponent-form value is a double by nature and is sent as + // the double it parsed to, so nothing is lost between typing and sending. + it("accepts a fractional value however large", () => { + expect(hasRoundedInteger({ ratio: 1.5 })).toBe(false); + expect(hasRoundedInteger({ ratio: 0.1 + 0.2 })).toBe(false); + }); + + // JS stringifies from 1e21 upward in exponent form, so such a value was typed + // that way and the parsed double is what was asked for. `parseJsonObjectDraft` + // has always accepted these. + it("accepts a value whose shortest form carries an exponent", () => { + expect(hasRoundedInteger({ n: 1e308 })).toBe(false); + expect(hasRoundedInteger({ n: 1e21 })).toBe(false); + }); + + it("looks inside arrays and nested objects", () => { + expect(hasRoundedInteger(JSON.parse('[{"a":[9007199254740993]}]'))).toBe( + true, + ); + expect(hasRoundedInteger({ a: { b: [1, 2, "x", null, true] } })).toBe( + false, + ); + }); + + it("has nothing to say about non-numeric values", () => { + expect(hasRoundedInteger("9007199254740993")).toBe(false); + expect(hasRoundedInteger(null)).toBe(false); + }); +}); describe("parseJsonObjectDraft", () => { it("reads empty text as the empty object rather than an error", () => { diff --git a/clients/web/src/utils/jsonObjectDraft.ts b/clients/web/src/utils/jsonObjectDraft.ts index 731d6b8f..d69ac326 100644 --- a/clients/web/src/utils/jsonObjectDraft.ts +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -20,6 +20,55 @@ export function isJsonObject(value: unknown): value is StrictJsonObject { return value !== null && typeof value === "object" && !Array.isArray(value); } +/** + * Whether `JSON.parse` dropped digits from a whole number written out in full. + * + * `isSerializableJson` catches a literal too large to *represent* — `1e400` + * parses to `Infinity` and writes back as `null`. This catches the quieter one: + * a whole number past 2^53−1 parses to the nearest double, so + * `{"id":9007199254740993}` becomes `…992` with no error anywhere. The draft + * still shows what was typed, which is what makes it a misreport rather than a + * formatting difference — the editor displays one value and the wire carries + * another. Snowflake-style ids are the realistic case. + * + * Two conditions, and the second is what keeps this from over-reaching: + * + * - **An unsafe whole number.** A fractional value is a double by nature and is + * sent as the double it parsed to, so nothing is lost between typing and + * sending. + * - **Whose shortest representation is written out in full.** JS switches to + * exponent form at 1e21, so a value that stringifies as `1e+308` was typed in + * exponent form, where the parsed double *is* the value the user asked for. + * Without this, `{"n":1e308}` — a legitimate `_meta` value — would be + * refused, which `parseJsonObjectDraft`'s own tests pin against. + * + * It still refuses the rare full-form integer past 2^53−1 that happens to be + * exactly representable (2^54, say), because nothing here can tell it apart + * from one that lost digits without the original literal. That errs toward + * refusing a sendable value rather than misreporting an unsendable one, which + * is the same trade `toNumericValue` makes for the schema form's number input. + */ +export function hasRoundedInteger(value: unknown): boolean { + if (typeof value === "number") { + return ( + Number.isInteger(value) && + !Number.isSafeInteger(value) && + !String(value).includes("e") + ); + } + if (Array.isArray(value)) return value.some(hasRoundedInteger); + if (value !== null && typeof value === "object") { + return Object.values(value as Record).some( + hasRoundedInteger, + ); + } + return false; +} + +/** The message every draft parser uses for {@link hasRoundedInteger}. */ +export const ROUNDED_INTEGER_ERROR = + "Whole numbers must be within ±(2^53 − 1) — a longer one is rounded when parsed, so a different value would be sent"; + /** * Read a JSON-object editor's draft text. * @@ -60,5 +109,8 @@ export function parseJsonObjectDraft(text: string): JsonObjectDraft { "Numbers must be finite — a value like `1e400` overflows and would be sent as null", }; } + if (hasRoundedInteger(parsed)) { + return { ok: false, error: ROUNDED_INTEGER_ERROR }; + } return { ok: true, value: parsed }; }