diff --git a/clients/cli/__tests__/metadata.test.ts b/clients/cli/__tests__/metadata.test.ts index ef8e2b37..55074dd4 100644 --- a/clients/cli/__tests__/metadata.test.ts +++ b/clients/cli/__tests__/metadata.test.ts @@ -911,6 +911,34 @@ describe("Metadata Tests", () => { } }); + it("names the key but never the value when rejecting", async () => { + // The pair can carry a credential, and this message reaches stderr and + // CI logs. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + try { + await server.start(); + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + "--metadata", + 'credentials={"accessToken":"sk-live-nope","n":1e400}', + "--transport", + "http", + ]); + const output = `${result.stderr}${result.stdout}`; + expect(result.exitCode).not.toBe(0); + expect(output).toContain("credentials"); + expect(output).not.toContain("sk-live"); + } 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 c486a725..412fc5dc 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -504,8 +504,11 @@ function parseKeyValuePair( // and better than falling back to the literal string, which would also not // be what was asked for. if (!isSerializableJson(parsedValue)) { + // Names the key, never the value: the pair can carry a credential + // (`credentials={"accessToken":"…","n":1e400}`) and this message lands in + // stderr and CI logs. throw new Error( - `Invalid parameter value: ${value}. Numbers must be finite (a value like 1e400 overflows and cannot be sent).`, + `Invalid value for "${key}": numbers must be finite (a literal like 1e400 overflows to Infinity and cannot be sent).`, ); } diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 2ad70e4c..acca524f 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1477,6 +1477,44 @@ describe("normalizeStoredMetadata (#1910)", () => { expect(warn).toHaveBeenCalledTimes(1); }); + it("drops a value that parses but cannot be sent, keeping its siblings", () => { + // A hand-edited catalog reaches this untouched by any editor, so it is a + // real source of `1e400` — which parses to Infinity and serializes to null. + const out = normalizeStoredMetadata( + JSON.parse('{"good":1,"bad":1e400,"alsoGood":{"n":2}}'), + ); + expect(out).toEqual({ good: 1, alsoGood: { n: 2 } }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("drops a non-serializable value nested inside an object", () => { + const out = normalizeStoredMetadata(JSON.parse('{"a":{"deep":[1e400]}}')); + expect(out).toEqual({}); + }); + + it("applies the same check to a legacy pair value", () => { + const out = normalizeStoredMetadata([ + { key: "ok", value: 1 }, + { key: "bad", value: JSON.parse("1e400") }, + ]); + expect(out).toEqual({ ok: 1 }); + }); + + it("never prints the offending value in a warning", () => { + // `_meta` can hold credentials, and these warnings go to ordinary console + // output — so a diagnostic may name the key and the type, never the value. + normalizeStoredMetadata([ + { value: { accessToken: "sk-live-nope" } }, + { key: "leaky", value: JSON.parse('{"token":1e400}') }, + ]); + normalizeStoredMetadata("sk-live-also-nope"); + const printed = JSON.stringify(warn.mock.calls); + expect(printed).not.toContain("sk-live"); + expect(printed).not.toContain("accessToken"); + // The key is still named, so the warning stays actionable. + expect(printed).toContain("leaky"); + }); + it("migrates a legacy file to the object shape on the next write", () => { // Read side accepts the pair array; write side emits only the object, so // one round-trip is the migration. diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 0b3bffe5..8ceb79ae 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1213,9 +1213,18 @@ export function createRemoteApp( // credentials are intentionally lost on first read (hard cutover per // #1358 decision 4). Users re-enter via the form or hand-edit into the // flat shape. - // A plain JSON object — the container `_meta` requires. Values are - // deliberately unchecked: `_meta` takes arbitrary JSON (#1910), and anything - // that survived `JSON.parse` already is. + // The *container* `_meta` requires — a plain JSON object. + // + // Value-level validity is deliberately not checked here. Values are + // arbitrary JSON (#1910), and the one class that survives `JSON.parse` yet + // cannot be sent — a non-finite number, from a literal like `1e400` in a + // hand-edited catalog — is filtered **per key** by + // `normalizeStoredMetadata`, which every read routes through. Rejecting the + // whole field here instead would cost a user every other key they had + // configured, for one bad value. + // + // Note the wire cannot carry the bad case at all: a client's own + // `JSON.stringify` turns `Infinity` into `null` before the request is sent. const isJsonObject = (v: unknown): v is RequestMetadata => v !== null && typeof v === "object" && !Array.isArray(v); const isStringRecord = (v: unknown): v is Record => { diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index 405bbca2..b21d736b 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -30,6 +30,7 @@ import { } from "../auth/secret-fields.js"; import { toRecord } from "../json/jsonUtils.js"; import type { JsonValue } from "../json/jsonUtils.js"; +import { isSerializableJson } from "../json/jsonUtils.js"; // The full set of valid `type` discriminator values, used to reject anything // else read off disk so unknown strings can't propagate to narrowing sites. @@ -226,9 +227,12 @@ export function normalizeStoredMetadata( entry === null || typeof (entry as { key?: unknown }).key !== "string" ) { + // Structural message only — never the entry. A malformed legacy row + // still holds whatever the user put in it, which for `_meta` can be a + // credential, and this goes to the console of every client that reads + // the catalog. console.warn( - "Ignoring malformed legacy metadata entry (expected `{ key, value }`):", - entry, + "Ignoring malformed legacy metadata entry (expected `{ key, value }`).", ); continue; } @@ -236,13 +240,14 @@ export function normalizeStoredMetadata( if (key.trim() === "") continue; // `??`, not `||`: `null` is a legal `_meta` value and must survive the // migration as `null`. Only a genuinely absent `value` becomes `""`. - // + const resolved = value === undefined ? "" : value; + if (!keepSerializable(key, resolved)) continue; // `defineProperty` rather than `out[key] = …` because a legacy key of // `__proto__` would otherwise hit the prototype setter — silently // dropping the entry (or, for an object value, mutating the prototype) // instead of storing an own property under that name. Object.defineProperty(out, key, { - value: value === undefined ? "" : value, + value: resolved, writable: true, enumerable: true, configurable: true, @@ -252,16 +257,51 @@ export function normalizeStoredMetadata( } if (typeof metadata === "object") { - return { ...(metadata as RequestMetadata) }; + // A hand-edited catalog reaches this untouched by any editor, so it is a + // real source of values that parse but cannot be sent — `1e400` becomes + // `Infinity`, which the next serialization writes as `null`. Same + // invariant the web editor and the CLI enforce, applied per key so one bad + // value does not cost the rest. + const out: RequestMetadata = {}; + for (const [key, value] of Object.entries(metadata as RequestMetadata)) { + if (keepSerializable(key, value)) out[key] = value; + } + return out; } console.warn( - "Ignoring malformed `metadata` (expected a JSON object):", - metadata, + `Ignoring malformed \`metadata\` (expected a JSON object, got ${describeJsonType(metadata)}).`, ); return {}; } +/** + * Whether a metadata value can be sent as written, warning by key when it + * cannot. + * + * Names the key and the offending type, never the value: `_meta` carries + * whatever the user configured, credentials included, and these warnings reach + * ordinary console output. + */ +function keepSerializable(key: string, value: unknown): boolean { + if (isSerializableJson(value)) return true; + console.warn( + `Ignoring metadata key "${key}" — its value is not JSON that can be sent ` + + `(got ${describeJsonType(value)}; note \`1e400\` parses to Infinity).`, + ); + return false; +} + +/** A one-word type name for a diagnostic that must not disclose the value. */ +function describeJsonType(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + if (typeof value === "number" && !Number.isFinite(value)) { + return Number.isNaN(value) ? "NaN" : "a non-finite number"; + } + return typeof value; +} + /** * Validate a server's `oauth.authorizationParams` as it comes off disk, or * `undefined` when there is nothing usable. (#2018)