diff --git a/clients/cli/__tests__/metadata.test.ts b/clients/cli/__tests__/metadata.test.ts index a4cb05fc..ef8e2b37 100644 --- a/clients/cli/__tests__/metadata.test.ts +++ b/clients/cli/__tests__/metadata.test.ts @@ -880,6 +880,37 @@ describe("Metadata Tests", () => { } }); + it.each([ + ["an overflowing literal", "n=1e400"], + ["a negative overflow", "n=-1e400"], + ["one nested in an object", 'o={"a":1e400}'], + ])("rejects %s rather than sending null", async (_label, pair) => { + // `JSON.parse` accepts these and yields ±Infinity, which + // `JSON.stringify` writes as `null` — so accepting the flag would + // transmit a value the user did not ask for. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + try { + await server.start(); + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + "--metadata", + pair, + "--transport", + "http", + ]); + expect(result.exitCode).not.toBe(0); + expect(`${result.stderr}${result.stdout}`).toMatch(/finite/i); + } finally { + await server.stop(); + } + }); + it("should handle metadata parsing validation", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 6f9c8550..c486a725 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -25,6 +25,7 @@ import { } from "@inspector/core/mcp/node/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; import type { StrictJsonValue } from "@inspector/core/json/jsonUtils.js"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { canonicalUrlHost, isAllInterfacesHost, @@ -492,9 +493,22 @@ function parseKeyValuePair( try { parsedValue = JSON.parse(val) as StrictJsonValue; } catch { + // Not JSON at all — a bare word or an unquoted string. Sent as a string, + // which is what the user plainly meant. parsedValue = val; } + // Valid JSON syntax is not the same as sendable JSON: `1e400` parses to + // `Infinity`, which `JSON.stringify` writes as `null`. Rejecting is better + // than accepting the flag and silently transmitting a different value — + // and better than falling back to the literal string, which would also not + // be what was asked for. + if (!isSerializableJson(parsedValue)) { + throw new Error( + `Invalid parameter value: ${value}. Numbers must be finite (a value like 1e400 overflows and cannot be sent).`, + ); + } + return { ...previous, [key as string]: parsedValue }; } diff --git a/clients/web/src/test/core/json/isSerializableJson.test.ts b/clients/web/src/test/core/json/isSerializableJson.test.ts new file mode 100644 index 00000000..ea0d91f4 --- /dev/null +++ b/clients/web/src/test/core/json/isSerializableJson.test.ts @@ -0,0 +1,47 @@ +import { describe, it, expect } from "vitest"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; + +describe("isSerializableJson", () => { + it.each([ + ["a string", "a"], + ["a finite number", 1.5], + ["zero", 0], + ["a negative number", -42], + ["a boolean", true], + ["null", null], + ["a nested object", { a: { b: [1, 2, { c: "d" }] } }], + ["an empty array", []], + ])("accepts %s", (_label, value) => { + expect(isSerializableJson(value)).toBe(true); + }); + + it.each([ + ["NaN", NaN], + ["Infinity", Infinity], + ["-Infinity", -Infinity], + ])("rejects %s, which JSON.stringify rewrites to null", (_label, value) => { + expect(isSerializableJson(value)).toBe(false); + // The reason it matters, asserted rather than asserted-about. + expect(JSON.stringify({ v: value })).toBe('{"v":null}'); + }); + + it.each([ + ["undefined", undefined], + ["a function", () => {}], + ["a symbol", Symbol("s")], + ["a bigint", 10n], + ])("rejects %s, which is not JSON at all", (_label, value) => { + expect(isSerializableJson(value)).toBe(false); + }); + + it("descends into arrays and objects", () => { + expect(isSerializableJson({ a: [1, { b: Infinity }] })).toBe(false); + expect(isSerializableJson([[[NaN]]])).toBe(false); + expect(isSerializableJson({ a: [1, { b: 2 }] })).toBe(true); + }); + + it("rejects what `JSON.parse` produces for an overflowing literal", () => { + // The reachable path: valid JSON text that parses to an unsendable value. + expect(isSerializableJson(JSON.parse('{"n":1e400}'))).toBe(false); + }); +}); diff --git a/clients/web/src/utils/jsonObjectDraft.test.ts b/clients/web/src/utils/jsonObjectDraft.test.ts index 83298caf..51e835a4 100644 --- a/clients/web/src/utils/jsonObjectDraft.test.ts +++ b/clients/web/src/utils/jsonObjectDraft.test.ts @@ -24,6 +24,25 @@ describe("parseJsonObjectDraft", () => { }); }); + it.each([ + ["an overflowing literal", '{"n":1e400}'], + ["a negative overflow", '{"n":-1e400}'], + ["one nested in an array", '{"a":[1,1e400]}'], + ])("rejects %s, which would be sent as null", (_label, text) => { + // `JSON.parse` accepts these; `JSON.stringify` then writes `null`, so the + // editor would show one value and the wire would carry another. + const result = parseJsonObjectDraft(text); + expect(result.ok).toBe(false); + expect(result.ok === false && result.error).toMatch(/finite/); + }); + + it("still accepts large-but-finite numbers", () => { + expect(parseJsonObjectDraft('{"n":1e308}')).toEqual({ + ok: true, + value: { n: 1e308 }, + }); + }); + 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 16bc545e..731d6b8f 100644 --- a/clients/web/src/utils/jsonObjectDraft.ts +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -1,4 +1,5 @@ import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; /** * What a JSON-object editor's draft text currently means: the object to emit, @@ -49,5 +50,15 @@ export function parseJsonObjectDraft(text: string): JsonObjectDraft { error: "Must be a JSON object (`{ … }`) — changes are not applied", }; } + // `JSON.parse` accepts numeric literals it cannot represent: `1e400` parses + // to `Infinity`, which `JSON.stringify` then writes as `null`. Accepting it + // here would show the user one value and send another. + if (!isSerializableJson(parsed)) { + return { + ok: false, + error: + "Numbers must be finite — a value like `1e400` overflows and would be sent as null", + }; + } return { ok: true, value: parsed }; } diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index a84de8fc..ba1533e9 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -37,6 +37,40 @@ export type StrictJsonValue = export type StrictJsonObject = { [key: string]: StrictJsonValue }; +/** + * Whether a value is JSON that survives `JSON.stringify` unchanged. + * + * {@link StrictJsonValue} is the *type*-level half of this and cannot express + * the rest: TypeScript has one `number`, but JSON has no encoding for `NaN` or + * `±Infinity`, so `JSON.stringify` rewrites them to `null`. + * + * That is reachable from ordinary input, not just from a careless caller — + * `JSON.parse("1e400")` returns `Infinity` on overflow, so text a user typed + * parses successfully, satisfies the type, and then reaches the wire as `null` + * while the editor still shows what they wrote. Checking at the boundary is the + * only place the distinction exists. + */ +export function isSerializableJson(value: unknown): value is StrictJsonValue { + if (value === null) return true; + switch (typeof value) { + case "string": + case "boolean": + return true; + case "number": + // Rejects NaN and ±Infinity; every other number round-trips. + return Number.isFinite(value); + case "object": + return Array.isArray(value) + ? value.every(isSerializableJson) + : Object.values(value as Record).every( + isSerializableJson, + ); + default: + // `undefined`, functions, symbols and bigints are not JSON at all. + return false; + } +} + /** * Widen a typed object to a generic string-keyed record so its keys can be * iterated or read/written generically. Many of the project's config/SDK types