modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: stop leaking metadata values into diagnostics, and salvage per key

Browse source 
Two credential leaks, the same class already fixed for the client logger:
`serverList` printed the malformed entry verbatim — a legacy row missing `key`
still holds whatever the user configured — and the CLI overflow error echoed
the whole `key=value` pair into stderr and CI logs. Both now name the key and
the offending *type*; a test plants a `sk-live-…` and asserts it appears
nowhere while the key still does.

`normalizeStoredMetadata` now also drops a value that parses but cannot be
sent. A hand-edited catalog is the reachable source: it is parsed server-side,
so `1e400` really does become `Infinity` there, and the next write would store
`null`. Filtered per key, matching the per-item salvage this file already does
elsewhere, so one bad value does not cost the user the rest.

Deliberately *not* added to the `/api/servers` guard, which review also
suggested. Writing that test is what showed why: a client's own
`JSON.stringify` turns `Infinity` into `null` before the request is sent, so
the wire cannot carry the bad case at all — and the check made things worse,
rejecting the whole metadata field where the normalizer drops one key. The
guard stays a container check, with a comment recording both points.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 20:03:37 -04:00
commit 56d8644032
5 changed files with 129 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

28
clients/cli/__tests__/metadata.test.ts
View file

@ -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(),

5
clients/cli/src/cli.ts
View file

@ -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).`,
);
}

38
clients/web/src/test/core/mcp/serverList.test.ts
View file

@ -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.

15
core/mcp/remote/node/server.ts
View file

@ -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<string, string> => {

54
core/mcp/serverList.ts
View file

@ -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)