From ddfce0f7ac30cab4316809ae3d6d90f97a238d3f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 20:55:51 -0400 Subject: [PATCH] fix: reject a non-string protocolVersion, and drop the test double casts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review follow-ups: - `applyProtocolVersion` took the value straight from an unvalidated JSON body and went to `RegExp.test`, which coerces — `123` and `true` both matched the token pattern and reached `Transport.setProtocolVersion` with the wrong type. It now takes `unknown` and checks `typeof === "string"` first, with cases for the coercible values. - Replace `as unknown as Transport` in the new session fixtures with a `makeTransport()` helper returning a structurally valid `Transport`, so an interface change stays visible to the tests. - Drop `as unknown as typeof fetch` in the new transport test — `vi.fn()` is already the right type. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 --- .../mcp/remote/remoteClientTransport.test.ts | 5 +--- .../mcp/remote/remote-session.test.ts | 23 +++++++++++++++---- core/mcp/remote/node/remote-session.ts | 9 +++++--- 3 files changed, 26 insertions(+), 11 deletions(-) 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 3c51abcb..c0478851 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -542,10 +542,7 @@ describe("RemoteClientTransport", () => { return new Response("not found", { status: 404 }); }); - const transport = new RemoteClientTransport( - { baseUrl, fetchFn: fetchFn as unknown as typeof fetch }, - config, - ); + const transport = new RemoteClientTransport({ baseUrl, fetchFn }, config); await transport.start(); // Pre-initialize sends carry no version (none negotiated yet). 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 a74e980c..c152b960 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 @@ -25,6 +25,16 @@ function makeFetchEntry( }; } +/** A structurally valid `Transport`, so a fixture needs no cast. */ +function makeTransport(overrides: Partial = {}): Transport { + return { + start: async () => {}, + send: async () => {}, + close: async () => {}, + ...overrides, + }; +} + describe("RemoteSession", () => { it("queues events before a consumer attaches and drains them on attach", () => { const session = new RemoteSession("s1"); @@ -125,7 +135,7 @@ describe("RemoteSession", () => { it("applyProtocolVersion forwards a new version to the transport once", () => { const session = new RemoteSession("s8a"); const setProtocolVersion = vi.fn(); - session.setTransport({ setProtocolVersion } as unknown as Transport); + session.setTransport(makeTransport({ setProtocolVersion })); session.applyProtocolVersion("2025-11-25"); session.applyProtocolVersion("2025-11-25"); @@ -136,21 +146,26 @@ describe("RemoteSession", () => { expect(setProtocolVersion).toHaveBeenLastCalledWith("2026-07-28"); }); - it("applyProtocolVersion ignores an absent or non-token version", () => { + it("applyProtocolVersion ignores an absent, non-token, or non-string version", () => { const session = new RemoteSession("s8b"); const setProtocolVersion = vi.fn(); - session.setTransport({ setProtocolVersion } as unknown as Transport); + session.setTransport(makeTransport({ setProtocolVersion })); session.applyProtocolVersion(undefined); // Header injection attempt — must never reach the upstream transport. session.applyProtocolVersion("2025-11-25\r\nX-Evil: 1"); session.applyProtocolVersion(""); + // The value arrives from an unvalidated JSON body, so a non-string must be + // rejected on its type — `RegExp.test` would coerce these into a match. + session.applyProtocolVersion(123); + session.applyProtocolVersion(true); + session.applyProtocolVersion(null); expect(setProtocolVersion).not.toHaveBeenCalled(); }); it("applyProtocolVersion is a no-op on a transport without setProtocolVersion (stdio)", () => { const session = new RemoteSession("s8c"); - session.setTransport({} as unknown as Transport); + session.setTransport(makeTransport()); expect(() => session.applyProtocolVersion("2025-11-25")).not.toThrow(); }); diff --git a/core/mcp/remote/node/remote-session.ts b/core/mcp/remote/node/remote-session.ts index 60ee176f..fc7c7654 100644 --- a/core/mcp/remote/node/remote-session.ts +++ b/core/mcp/remote/node/remote-session.ts @@ -69,11 +69,14 @@ export class RemoteSession { * * Ignores anything that isn't a plausible version token so a client can't * push arbitrary bytes into an upstream header, and re-applies only on - * change (the value rides every `/api/mcp/send`). + * change (the value rides every `/api/mcp/send`). The value arrives from an + * unvalidated JSON body, so the type is checked rather than assumed — + * `RegExp.test` would otherwise coerce a `123` or `true` into a "matching" + * string and hand the wrong type to the transport. */ - applyProtocolVersion(version: string | undefined): void { + applyProtocolVersion(version: unknown): void { if ( - version === undefined || + typeof version !== "string" || version === this.appliedProtocolVersion || !RemoteSession.PROTOCOL_VERSION_PATTERN.test(version) ) {