From e9dc0e5e5fce9ccacb04c85db55a6ed631975cc7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 13:36:45 -0400 Subject: [PATCH] fix: drop a schema-invalid `_meta.progressToken` instead of sending it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `progressToken` is a reserved `_meta` member the spec constrains to `string | integer` (`ProgressTokenSchema = z.union([z.string(), z.number().int()])`). Every other value may be any JSON after #1910, which is what makes this one key need its own check: before the widening a value was always a string and therefore always valid, but now a float, an object or an array can land there, and a conforming server would reject the whole request. `progressTokenOf` only governed whether a progress callback was wired up, so a bad value still rode out inside `_meta`. Sanitize in `mergeMeta` instead — the single point every `_meta` flows through — dropping just the offending member with a warning. The key is optional, so the request stays well-formed and simply asks for no progress. Note the exposure only exists with `progress: false`. With progress on (the default) the SDK sets `onprogress` and stamps its own message id over `_meta.progressToken`, so a caller's value never reaches the wire either way. The new tests therefore run with progress disabled, which is the configuration that can actually ship a bad token: five invalid shapes dropped (float, object, array, null, boolean) with the sibling keys left intact, and string/integer preserved. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- .../integration/mcp/inspectorClient.test.ts | 84 +++++++++++++++++++ core/mcp/inspectorClient.ts | 35 +++++++- 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 9fbb0608..78e25418 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -1412,6 +1412,90 @@ describe("InspectorClient", () => { messageLogState.destroy(); }); + it.each([ + ["a float", 3.5], + ["an object", { id: 1 }], + ["an array", [1]], + ["null", null], + ["a boolean", true], + ])( + "drops a reserved progressToken that is %s rather than sending it", + async (_label, badToken) => { + // Every other `_meta` value may be any JSON (#1910), but the spec + // constrains `progressToken` to `string | integer` + // (`ProgressTokenSchema`). Sending a bad one would make a conforming + // server reject the whole request, so the member is dropped — the key + // is optional, so what goes out is still well-formed. + // + // `progress: false` is what makes this observable. With progress on + // (the default) the SDK sets `onprogress` and stamps its own message + // id over `_meta.progressToken`, so a caller's value never reaches the + // wire either way; with it off nothing overwrites the member and a bad + // one would ship. + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + progress: false, + defaultMetadata: { progressToken: badToken, keep: "me" }, + }, + ); + const messageLogState = new MessageLogState(client); + await client.connect(); + await client.listTools(); + + const listToolsReq = messageLogState + .getMessages() + .find( + (m) => + m.direction === "request" && + (m.message as { method?: string }).method === "tools/list", + ); + expect(listToolsReq).toBeDefined(); + const meta = metaOf(listToolsReq!); + expect(meta).not.toHaveProperty("progressToken"); + // Only the offending member is dropped, not the whole payload. + expect(meta.keep).toBe("me"); + messageLogState.destroy(); + }, + ); + + it.each([ + ["a string", "tok-1"], + ["an integer", 7], + ])("keeps a valid progressToken that is %s", async (_label, token) => { + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + // See the note above: with progress on, the SDK overwrites this. + progress: false, + defaultMetadata: { progressToken: token }, + }, + ); + const messageLogState = new MessageLogState(client); + await client.connect(); + await client.listTools(); + + const listToolsReq = messageLogState + .getMessages() + .find( + (m) => + m.direction === "request" && + (m.message as { method?: string }).method === "tools/list", + ); + expect(metaOf(listToolsReq!).progressToken).toBe(token); + messageLogState.destroy(); + }); + it("call-time metadata overrides defaultMetadata on key collision", async () => { client = new InspectorClient( { diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index f3febd33..a51b0e2e 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -369,6 +369,22 @@ interface ToolCallRequest { // server can't spin a tight zero-delay loop; a successful acknowledgement resets // the count, and past the cap we give up (mark the stream ended) rather than // retry a persistently-failing re-list forever. +/** + * Whether a `_meta.progressToken` value is one the protocol allows. + * + * The spec (and the SDK's `ProgressTokenSchema`, + * `z.union([z.string(), z.number().int()])`) constrains this reserved member to + * a string or an **integer** — a float is as invalid as an object. Everything + * else in `_meta` may be any JSON (#1910), which is exactly why this one key + * needs its own check. + */ +function isProgressToken(value: unknown): value is ProgressToken { + return ( + typeof value === "string" || + (typeof value === "number" && Number.isInteger(value)) + ); +} + const MODERN_RECONNECT_BASE_MS = 500; const MODERN_RECONNECT_MAX_MS = 15_000; const MODERN_RECONNECT_MAX_ATTEMPTS = 8; @@ -1053,6 +1069,21 @@ export class InspectorClient extends InspectorClientEventTarget { ...(logMeta ?? {}), ...(callMetadata ?? {}), }; + // `_meta` values are arbitrary JSON (#1910), but `progressToken` is a + // reserved member the spec constrains to `string | integer` + // (`ProgressTokenSchema = z.union([z.string(), z.number().int()])`). Now + // that a value can be an object, an array or a float, a bad one would ride + // out on the wire and a conforming server would reject the whole request — + // so drop the member rather than send something invalid. It is optional, so + // the request stays well-formed and simply asks for no progress. Warn, + // because a silently dropped key the user typed is worse than a noisy one. + if ("progressToken" in merged && !isProgressToken(merged.progressToken)) { + this.logger.warn( + { progressToken: merged.progressToken }, + "Dropping `_meta.progressToken` — expected a string or an integer.", + ); + delete merged.progressToken; + } return Object.keys(merged).length > 0 ? merged : undefined; } @@ -1071,9 +1102,7 @@ export class InspectorClient extends InspectorClientEventTarget { metadata: RequestMetadata | undefined, ): ProgressToken | undefined { const token = metadata?.progressToken; - return typeof token === "string" || typeof token === "number" - ? token - : undefined; + return isProgressToken(token) ? token : undefined; } private getRequestOptions(