diff --git a/clients/web/src/test/core/mcp/remote/progressToken.test.ts b/clients/web/src/test/core/mcp/remote/progressToken.test.ts index 668a3843..dc00cfee 100644 --- a/clients/web/src/test/core/mcp/remote/progressToken.test.ts +++ b/clients/web/src/test/core/mcp/remote/progressToken.test.ts @@ -1,3 +1,11 @@ +/** + * Unit tests for the shared `progressTokenOf` guard (#2028) — the string/safe + * integer extractor both relay waits (browser `RemoteClientTransport` and Node + * `RemoteSession`) use to correlate a `notifications/progress` with the request + * whose timeout it should re-arm. Covers the protocol's accept set and, since + * the guard delegates to the SDK's `ProgressTokenSchema`, the fractional and + * past-`MAX_SAFE_INTEGER` values a bare `typeof` check would have let through. + */ import { describe, it, expect } from "vitest"; import { progressTokenOf } from "@inspector/core/mcp/remote/progressToken.js"; import type { JSONRPCMessage } from "@modelcontextprotocol/client"; @@ -70,4 +78,38 @@ describe("progressTokenOf (#2028)", () => { ), ).toBeUndefined(); }); + + it("rejects a fractional numeric token (the protocol allows only integers)", () => { + expect( + progressTokenOf( + msg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: 1.5, progress: 1 }, + }), + ), + ).toBeUndefined(); + }); + + it("rejects an integer past MAX_SAFE_INTEGER", () => { + expect( + progressTokenOf( + msg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: Number.MAX_SAFE_INTEGER + 1, progress: 1 }, + }), + ), + ).toBeUndefined(); + }); + + it("accepts a safe integer at the boundary", () => { + expect( + progressTokenOf({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: Number.MAX_SAFE_INTEGER, progress: 1 }, + }), + ).toBe(Number.MAX_SAFE_INTEGER); + }); }); diff --git a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts index 1fc58e92..90de46ef 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { RemoteClientTransport } from "@inspector/core/mcp/remote/remoteClientTransport.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -727,12 +727,22 @@ describe("RemoteClientTransport", () => { return { fetchFn, getSse: () => sse, getSentId: () => sentId }; } - const delay = (ms: number) => new Promise((r) => setTimeout(r, ms)); + // Fake timers so the deadline is driven deterministically rather than + // raced against a real `setTimeout` — a real-sleep test can let the timeout + // fire before a loaded runner processes the enqueued SSE frame (#1596: a + // race is fixed with fake timers, never with headroom). Each push is + // followed by a 0ms async advance to flush the SSE consumer's stream read + // so the frame is parsed before time moves. + const TIMEOUT_MS = 1000; + const flushSse = () => vi.advanceTimersByTimeAsync(0); + + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); it("re-arms the wait on each progress note, so a call outliving the timeout still resolves", async () => { const { fetchFn, getSse, getSentId } = backendHoldingSend(); const transport = new RemoteClientTransport( - { baseUrl, fetchFn, sseResponseTimeoutMs: 150 }, + { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS }, config, ); await transport.start(); @@ -742,22 +752,25 @@ describe("RemoteClientTransport", () => { id: 1, method: "tools/call", }); + await flushSse(); - // Emit progress every 100ms (< the 150ms window) for ~400ms total, then - // the response. Without the re-arm the wait dies at 150ms. - for (let i = 0; i < 4; i++) { - await delay(100); + // Three progress notes, each after 900ms (< the 1000ms window): without + // the re-arm the wait dies at 1000ms, well before the third lands. + for (let i = 0; i < 3; i++) { getSse().pushMessage({ jsonrpc: "2.0", method: "notifications/progress", - params: { progressToken: getSentId(), progress: i, total: 4 }, + params: { progressToken: getSentId(), progress: i, total: 3 }, }); + await flushSse(); + await vi.advanceTimersByTimeAsync(900); } getSse().pushMessage({ jsonrpc: "2.0", id: getSentId(), result: { ok: true }, }); + await flushSse(); await expect(sent).resolves.toBeUndefined(); await transport.close(); @@ -766,7 +779,7 @@ describe("RemoteClientTransport", () => { it("does not re-arm on notifications/message, so a log-only call still times out", async () => { const { fetchFn, getSse, getSentId } = backendHoldingSend(); const transport = new RemoteClientTransport( - { baseUrl, fetchFn, sseResponseTimeoutMs: 150 }, + { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS }, config, ); await transport.start(); @@ -776,24 +789,23 @@ describe("RemoteClientTransport", () => { id: 1, method: "tools/call", }); - void sent.catch(() => {}); - - // Only log messages — these must not extend the deadline. - for (let i = 0; i < 4; i++) { - await delay(50); - getSse().pushMessage({ - jsonrpc: "2.0", - method: "notifications/message", - params: { - level: "info", - data: { msg: `tick ${i}`, token: getSentId() }, - }, - }); - } - - await expect(sent).rejects.toThrow( + // Hold the expected rejection now (as the RemoteSession timeout tests do) + // so it is asserted rather than suppressed by an empty catch. + const rejection = expect(sent).rejects.toThrow( /Timed out waiting for MCP response on SSE/, ); + await flushSse(); + + // A log message before the deadline must not extend it. + getSse().pushMessage({ + jsonrpc: "2.0", + method: "notifications/message", + params: { level: "info", data: { msg: "tick", token: getSentId() } }, + }); + await flushSse(); + await vi.advanceTimersByTimeAsync(TIMEOUT_MS); + + await rejection; await transport.close(); }); }); diff --git a/clients/web/src/test/integration/mcp/remote/remote-session.test.ts b/clients/web/src/test/integration/mcp/remote/remote-session.test.ts index 285efebd..66899eb5 100644 --- a/clients/web/src/test/integration/mcp/remote/remote-session.test.ts +++ b/clients/web/src/test/integration/mcp/remote/remote-session.test.ts @@ -7,11 +7,16 @@ */ import { describe, it, expect, vi } from "vitest"; -import type { Transport } from "@modelcontextprotocol/client"; +import type { Transport, JSONRPCMessage } from "@modelcontextprotocol/client"; import { RemoteSession } from "@inspector/core/mcp/remote/node/remote-session.js"; import type { FetchRequestEntryBase } from "@inspector/core/mcp/types.js"; import { AuthChallengeError } from "@inspector/core/auth/challenge.js"; +// Stage a deliberately off-spec message as `unknown` and narrow with a single +// cast — the guard branches these fixtures exercise are ones the typed +// `JSONRPCMessage` shape forbids, and `unknown` is the safe top type to pass in. +const asMsg = (m: unknown): JSONRPCMessage => m as JSONRPCMessage; + function makeFetchEntry( overrides: Partial = {}, ): FetchRequestEntryBase { @@ -289,11 +294,13 @@ describe("RemoteSession", () => { const wait = session.waitForRequestResponse(8, 1000); const rejection = expect(wait).rejects.toThrow(/timed out/); // A malformed token (object) must be ignored, not coerced to a key. - session.onMessage({ - jsonrpc: "2.0", - method: "notifications/progress", - params: { progressToken: { bad: true }, progress: 1 }, - } as unknown as Parameters[0]); + session.onMessage( + asMsg({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { progressToken: { bad: true }, progress: 1 }, + }), + ); await vi.advanceTimersByTimeAsync(1000); await rejection; vi.useRealTimers(); diff --git a/core/mcp/remote/progressToken.ts b/core/mcp/remote/progressToken.ts index 1759e7fc..2ce37b1a 100644 --- a/core/mcp/remote/progressToken.ts +++ b/core/mcp/remote/progressToken.ts @@ -4,15 +4,20 @@ * and the Node `RemoteSession` import it rather than each carrying a copy. */ +import { ProgressTokenSchema } from "@modelcontextprotocol/core"; import type { JSONRPCMessage } from "@modelcontextprotocol/client"; /** - * The id of the request a `notifications/progress` belongs to, or `undefined` - * for any other message. The SDK stamps `progressToken: messageId` on the - * outbound request, so a progress notification's `progressToken` is the id of - * the request whose wait should be re-armed. A JSON-RPC progress token is a - * string or a number; anything else is ignored so a malformed token can't be - * coerced into a map key. + * The token identifying the request a `notifications/progress` belongs to, or + * `undefined` for any other message. The SDK stamps `progressToken: messageId` + * on the outbound request, so a progress notification's `progressToken` is the + * id of the request whose wait should be re-armed. + * + * Validation is delegated to the SDK's own `ProgressTokenSchema` rather than a + * hand-rolled `typeof` check — the protocol allows only a string or a *safe* + * integer, and zod's `.int()` rejects a fractional or past-`MAX_SAFE_INTEGER` + * value that a bare `typeof token === "number"` would wave through (the same + * drift `isProgressToken` in `core/mcp/inspectorClient.ts` exists to avoid). */ export function progressTokenOf( message: JSONRPCMessage, @@ -24,9 +29,11 @@ export function progressTokenOf( typeof message.params === "object" && message.params !== null ) { - const token = (message.params as { progressToken?: unknown }).progressToken; - if (typeof token === "string" || typeof token === "number") { - return token; + const parsed = ProgressTokenSchema.safeParse( + (message.params as { progressToken?: unknown }).progressToken, + ); + if (parsed.success) { + return parsed.data; } } return undefined;