diff --git a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts index 69191986..d2997c2b 100644 --- a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts +++ b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts @@ -116,6 +116,15 @@ describe("formatJson", () => { expect(formatJson('{"limit":1e400}')).toBe('{"limit":1e400}'); }); + // The starkest instance of the same class: only the *second* `role` survives + // `JSON.parse`, so reindenting would show the reader less than the server + // sent. + it("leaves a payload alone when it names the same member twice", () => { + expect(formatJson('{"role":"user","role":"admin"}')).toBe( + '{"role":"user","role":"admin"}', + ); + }); + // The spelling of a number changes on a round trip and its *value* does not, // so these still get indented: `0.10000000000000001` and `0.1` are the same // double, and `1e2` and `100` are the same number. diff --git a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts index a3531857..92c5a44a 100644 --- a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts +++ b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts @@ -2,12 +2,15 @@ * Pure helpers backing the per-MIME dispatch in {@link ContentViewer}. Kept * free of component dependencies so they can be unit-tested in isolation and * reused by the blob renderers (PDF / CSV / HTML) without dragging in React. - * The two pure predicates imported below are the exception that proves it: - * plain functions over a string, shared so the viewer and the JSON editors - * cannot disagree about which payloads survive a round trip. + * The predicates imported below are the exception that proves it: plain + * functions over a string, shared so the viewer and the JSON editors cannot + * disagree about which payloads survive a round trip. */ import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; -import { hasImpreciseIntegerLiteral } from "../../../utils/jsonObjectDraft"; +import { + findDuplicateObjectKey, + hasImpreciseIntegerLiteral, +} from "../../../utils/jsonObjectDraft"; /** * The renderer family a MIME type maps to. `ContentViewer` switches on this to @@ -160,11 +163,18 @@ export function formatJson(content: string): string { * as it arrived — unindented, but honest. The copy button already copied the * original text either way; this makes what is on screen agree with it. * - * The same two checks the draft parsers use, which is why they live in + * The same checks the draft parsers use, which is why they live in * `utils/jsonObjectDraft` rather than beside either caller. */ function reformatsFaithfully(text: string, parsed: unknown): boolean { - return isSerializableJson(parsed) && !hasImpreciseIntegerLiteral(text); + return ( + isSerializableJson(parsed) && + !hasImpreciseIntegerLiteral(text) && + // A duplicate member name is the same class and the starkest instance: + // `{"role":"user","role":"admin"}` would be *shown* as only the second, + // which is the viewer hiding what the server sent. + findDuplicateObjectKey(text) === null + ); } /** Heuristic: does this plain text (no MIME) look like a JSON document? */ diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index ff8614ca..c041cbae 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -265,6 +265,15 @@ describe("EditReplayModal", () => { expect(screen.getByRole("button", { name: "Send" })).not.toBeDisabled(); }); + // `JSON.parse` keeps only the last occurrence, so the draft would show two + // cursors and Send would dispatch one. + it("disables Send for a document that names the same member twice", async () => { + renderWithMantine(); + await setAceText('{"cursor":"first","cursor":"second"}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect(screen.getByText(/`cursor` appears twice/)).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 17bee0f1..17069c38 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -3,6 +3,8 @@ import type { Tool } from "@modelcontextprotocol/client"; import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { + duplicateKeyError, + findDuplicateObjectKey, hasImpreciseIntegerLiteral, isJsonObject, IMPRECISE_INTEGER_ERROR, @@ -114,6 +116,10 @@ function parseParamsDraft( if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } + const duplicate = findDuplicateObjectKey(text); + if (duplicate !== null) { + return { ok: false, error: duplicateKeyError(duplicate) }; + } // 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 77b6dbb9..a2c5e091 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -792,6 +792,9 @@ describe("SchemaForm", () => { screen.getByText(/^This whole number cannot be represented exactly/), ).toBeInTheDocument(); + await setAceTextByLabel(/Config/, '[{"a":1,"a":2}]'); + expect(screen.getByText(/^`a` appears twice/)).toBeInTheDocument(); + await setAceTextByLabel(/Config/, "[1,2]"); expect(screen.queryByText(/this field will be omitted/)).toBeNull(); }); @@ -1978,6 +1981,22 @@ describe("SchemaForm raw JSON (#2151)", () => { ).toBeInTheDocument(); }); + // `JSON.parse` keeps only the last occurrence, so the editor would show a + // document that submits as less than it says. + it("blocks submission for a document that names the same member twice", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine(); + await enableRawJson(user); + + await setAceTextByLabel( + /Arguments JSON/, + '{"mode":"safe","mode":"unsafe"}', + ); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + expect(screen.getByText(/`mode` appears twice/)).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 73cbcccc..748ce554 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -25,6 +25,8 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { EnlargeButton } from "../../elements/EnlargeButton/EnlargeButton"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { + duplicateKeyError, + findDuplicateObjectKey, hasImpreciseIntegerLiteral, isJsonObject, IMPRECISE_INTEGER_ERROR, @@ -332,6 +334,10 @@ function describeJsonDraftProblem(text: string): string | null { if (hasImpreciseIntegerLiteral(text)) { return `${IMPRECISE_INTEGER_ERROR} — this field will be omitted`; } + const duplicate = findDuplicateObjectKey(text); + if (duplicate !== null) { + return `${duplicateKeyError(duplicate)} — this field will be omitted`; + } return null; } @@ -649,6 +655,10 @@ function parseRawArgumentsDraft( if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } + const duplicate = findDuplicateObjectKey(text); + if (duplicate !== null) { + return { ok: false, error: duplicateKeyError(duplicate) }; + } return { ok: true, value: parsed }; } diff --git a/clients/web/src/utils/jsonObjectDraft.test.ts b/clients/web/src/utils/jsonObjectDraft.test.ts index 5788817d..9c3b8a0c 100644 --- a/clients/web/src/utils/jsonObjectDraft.test.ts +++ b/clients/web/src/utils/jsonObjectDraft.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { + findDuplicateObjectKey, hasImpreciseIntegerLiteral, parseJsonObjectDraft, } from "./jsonObjectDraft"; @@ -70,6 +71,51 @@ describe("hasImpreciseIntegerLiteral", () => { }); }); +// `JSON.parse` accepts duplicate member names and keeps the last silently, so +// the evidence is gone by the time it returns — nothing downstream can see that +// the document said more than the value carries. +describe("findDuplicateObjectKey", () => { + it("names the repeated member", () => { + expect(findDuplicateObjectKey('{"role":"user","role":"admin"}')).toBe( + "role", + ); + }); + + it("accepts a document whose names are all distinct", () => { + expect(findDuplicateObjectKey('{"a":1,"b":2}')).toBeNull(); + }); + + // The repeat has to be within one set of braces. + it("scopes the check per object", () => { + expect(findDuplicateObjectKey('{"a":{"a":1}}')).toBeNull(); + expect(findDuplicateObjectKey('[{"a":1},{"a":2}]')).toBeNull(); + }); + + it("finds a repeat nested inside another object or an array", () => { + expect(findDuplicateObjectKey('{"outer":{"a":1,"a":2}}')).toBe("a"); + expect(findDuplicateObjectKey('[{"a":1,"a":2}]')).toBe("a"); + }); + + // Compared by decoded value: `"a"` and `"\u0061"` name the same member, so + // comparing the raw source would miss a duplicate written the second way. + it("compares names by value, not by spelling", () => { + expect(findDuplicateObjectKey('{"a":1,"\\u0061":2}')).toBe("a"); + }); + + // A string *value* that repeats is not a duplicate name, and a name that + // merely appears inside one is not either. + it("does not confuse values with names", () => { + expect(findDuplicateObjectKey('{"a":"x","b":"x"}')).toBeNull(); + expect(findDuplicateObjectKey('{"a":"b","c":1}')).toBeNull(); + expect(findDuplicateObjectKey('{"a":"}\\"a\\":1{","b":2}')).toBeNull(); + }); + + it("says nothing about a half-typed draft", () => { + expect(findDuplicateObjectKey('{"a":1,"a')).toBeNull(); + expect(findDuplicateObjectKey("")).toBeNull(); + }); +}); + describe("parseJsonObjectDraft", () => { it("reads empty text as the empty object rather than an error", () => { expect(parseJsonObjectDraft("")).toEqual({ ok: true, value: {} }); @@ -112,6 +158,12 @@ describe("parseJsonObjectDraft", () => { }); }); + it("rejects a document that names the same member twice", () => { + const result = parseJsonObjectDraft('{"role":"user","role":"admin"}'); + expect(result.ok).toBe(false); + expect(result.ok === false && result.error).toMatch(/`role` appears twice/); + }); + it("rejects text that is not JSON", () => { expect(parseJsonObjectDraft('{"a":')).toEqual({ ok: false, diff --git a/clients/web/src/utils/jsonObjectDraft.ts b/clients/web/src/utils/jsonObjectDraft.ts index d06ccdc9..98b049ea 100644 --- a/clients/web/src/utils/jsonObjectDraft.ts +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -67,18 +67,10 @@ const JSON_NUMBER = /-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/y; */ function* jsonNumberLiterals(text: string): Generator { let index = 0; - let inString = false; while (index < text.length) { const char = text[index]; - if (inString) { - // A backslash escapes the next character, including a closing quote. - index += char === "\\" ? 2 : 1; - if (char === '"') inString = false; - continue; - } if (char === '"') { - inString = true; - index += 1; + index = readStringToken(text, index).next; continue; } if (char === "-" || (char >= "0" && char <= "9")) { @@ -95,6 +87,97 @@ function* jsonNumberLiterals(text: string): Generator { } } +/** + * Read one JSON string token starting at the opening quote. + * + * Returns where the token ends and its raw source, escapes intact — the caller + * decodes only when it needs the *value*, which for a key it does: `"a"` and + * `"\u0061"` name the same member, so comparing the raw forms would miss a + * duplicate written the second way. + * + * Shared by both scanners here so the escape handling — a backslash consumes + * the next character, including a closing quote — is written once. + */ +function readStringToken( + text: string, + start: number, +): { raw: string; next: number } { + let index = start + 1; + while (index < text.length) { + const char = text[index]; + if (char === "\\") { + index += 2; + continue; + } + index += 1; + if (char === '"') break; + } + return { raw: text.slice(start, index), next: index }; +} + +/** + * The first object member name this document declares twice, or `null`. + * + * `JSON.parse` accepts duplicate names and keeps the last silently, so + * `{"role":"user","role":"admin"}` becomes `{"role":"admin"}` — a document that + * renders, and submits, as *less* than it says. In a read-only view that is the + * Inspector hiding what the server sent; in a draft it is the editor showing + * more than the wire will carry. Neither is acceptable, and nothing else here + * detects it, because by the time `JSON.parse` returns the evidence is gone. + * + * Scoped per object, so `{"a":{"a":1}}` and `[{"a":1},{"a":2}]` are fine — the + * repeat has to be within one set of braces. + */ +export function findDuplicateObjectKey(text: string): string | null { + // One frame per open brace or bracket; `null` marks an array, whose members + // are not named and so cannot collide. + const stack: (Set | null)[] = []; + let expectKey = false; + let index = 0; + while (index < text.length) { + const char = text[index]; + if (char === '"') { + const { raw, next } = readStringToken(text, index); + const frame = stack[stack.length - 1]; + if (expectKey && frame) { + let name: string; + try { + name = JSON.parse(raw) as string; + } catch { + // An unterminated string at the end of a half-typed draft. There is + // no key here to compare, and the parse will report the real problem. + return null; + } + if (frame.has(name)) return name; + frame.add(name); + expectKey = false; + } + index = next; + continue; + } + if (char === "{") { + stack.push(new Set()); + expectKey = true; + } else if (char === "[") { + stack.push(null); + expectKey = false; + } else if (char === "}" || char === "]") { + stack.pop(); + expectKey = false; + } else if (char === ",") { + // Only an object's separator introduces a name; an array's does not. + expectKey = stack[stack.length - 1] != null; + } + index += 1; + } + return null; +} + +/** The message every draft parser uses for {@link findDuplicateObjectKey}. */ +export function duplicateKeyError(key: string): string { + return `\`${key}\` appears twice in the same object — JSON keeps only the last, so part of this would not be sent`; +} + /** * The message every draft parser uses for {@link hasImpreciseIntegerLiteral}. * @@ -148,5 +231,9 @@ export function parseJsonObjectDraft(text: string): JsonObjectDraft { if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } + const duplicate = findDuplicateObjectKey(text); + if (duplicate !== null) { + return { ok: false, error: duplicateKeyError(duplicate) }; + } return { ok: true, value: parsed }; }