From a141924f0af9c23da846a71e1fc426f7e2196207 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 00:07:22 -0500 Subject: [PATCH 1/7] Add an opt-in per-server HTTP cookie jar for sticky sessions (#2936) Servers deployed as several replicas behind a load balancer that routes by cookie affinity were unusable past initialize: the transports never stored Set-Cookie, so later requests round-robined onto replicas without the MCP session and failed with 404 Session not found. A new cookieJar server setting (default off) gives each Streamable HTTP or legacy SSE connection an in-memory, same-origin jar that stores Set-Cookie and replays Cookie. It sits above the fetch tracker so the Network log shows (redacted) what was sent, never persists, and never touches requests to other origins. Threaded through settings, disk persistence, the /api/servers validation, and the server settings form. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../ServerSettingsForm.stories.tsx | 5 + .../ServerSettingsForm.test.tsx | 64 +++++ .../ServerSettingsForm/ServerSettingsForm.tsx | 14 ++ .../ServerSettingsModal.test.tsx | 36 +++ .../ServerSettingsModal.tsx | 6 + .../test/core/mcp/node/cookieJarFetch.test.ts | 226 ++++++++++++++++++ .../web/src/test/core/mcp/serverList.test.ts | 33 +++ .../mcp/cookie-jar-affinity.test.ts | 193 +++++++++++++++ .../mcp/remote/server-extra-coverage.test.ts | 28 +++ core/mcp/node/cookieJarFetch.ts | 222 +++++++++++++++++ core/mcp/node/transport.ts | 23 +- core/mcp/remote/node/server.ts | 9 + core/mcp/serverList.ts | 11 + core/mcp/types.ts | 18 ++ docs/mcp-server-configuration.md | 1 + 15 files changed, 883 insertions(+), 6 deletions(-) create mode 100644 clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts create mode 100644 clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts create mode 100644 core/mcp/node/cookieJarFetch.ts diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx index b79cae5754..74caffdbf6 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx @@ -118,6 +118,10 @@ function InteractiveRender(args: ServerSettingsFormProps) { settings: { ...args.settings, suppressNotificationStream: value }, }); }} + onCookieJarChange={(value) => { + args.onCookieJarChange(value); + updateArgs({ settings: { ...args.settings, cookieJar: value } }); + }} onAdvertisedExtensionChange={(key, checked) => { args.onAdvertisedExtensionChange(key, checked); updateArgs({ @@ -189,6 +193,7 @@ const meta: Meta = { onAutoRefreshChange: fn(), onPaginatedListsChange: fn(), onSuppressNotificationStreamChange: fn(), + onCookieJarChange: fn(), onAdvertisedExtensionChange: fn(), onMaxFetchRequestsChange: fn(), onSkillCatalogLimitChange: fn(), diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 53a349d519..166645dc85 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -70,6 +70,7 @@ const baseHandlers = { onAutoRefreshChange: vi.fn(), onPaginatedListsChange: vi.fn(), onSuppressNotificationStreamChange: vi.fn(), + onCookieJarChange: vi.fn(), onAdvertisedExtensionChange: vi.fn(), onMaxFetchRequestsChange: vi.fn(), onSkillCatalogLimitChange: vi.fn(), @@ -534,6 +535,69 @@ describe("ServerSettingsForm", () => { ); }); + describe("HTTP Cookie Jar (#2936)", () => { + const name = /Enable HTTP Cookie Jar/; + + it("is unchecked by default and reflects an explicit true", () => { + const { rerender } = renderWithMantine( + , + ); + expect(screen.getByRole("checkbox", { name })).not.toBeChecked(); + rerender( + , + ); + expect(screen.getByRole("checkbox", { name })).toBeChecked(); + }); + + it("invokes onCookieJarChange when toggled", async () => { + const user = userEvent.setup(); + const onCookieJarChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("checkbox", { name })); + expect(onCookieJarChange).toHaveBeenCalledWith(true); + }); + + it("is shown for an sse server", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("checkbox", { name })).toBeInTheDocument(); + }); + + it("is hidden for a stdio server, which has no HTTP cookies", () => { + renderWithMantine( + , + ); + expect(screen.queryByRole("checkbox", { name })).not.toBeInTheDocument(); + }); + }); + it("renders the Advertised Extensions section with Tasks checked by default", () => { renderWithMantine( void; /** Toggle the standalone `GET` notification stream suppression (#2317). */ onSuppressNotificationStreamChange: (value: boolean) => void; + /** Toggle the connection-scoped HTTP cookie jar (#2936). */ + onCookieJarChange: (value: boolean) => void; /** * Toggle whether the Inspector advertises the extension `key` to this server. * `checked` is the new advertise state; the modal folds it into @@ -506,6 +508,7 @@ export function ServerSettingsForm({ onAutoRefreshChange, onPaginatedListsChange, onSuppressNotificationStreamChange, + onCookieJarChange, onAdvertisedExtensionChange, rendersApps = true, onMaxFetchRequestsChange, @@ -564,6 +567,9 @@ export function ServerSettingsForm({ // no-op there (#2317). `auto` keeps it, since it may resolve to legacy. const showSuppressNotificationStream = serverType === "streamable-http" && configuredEra !== "modern"; + // Cookies only exist on the HTTP transports; stdio has no use for a jar. + const showCookieJar = + serverType === "streamable-http" || serverType === "sse"; const showModernLogLevel = configuredEra === "modern" || (configuredEra === "auto" && negotiatedEra !== "legacy"); @@ -780,6 +786,14 @@ export function ServerSettingsForm({ } /> ) : null} + {showCookieJar ? ( + onCookieJarChange(e.currentTarget.checked)} + /> + ) : null} { ); }); + // #2936 — omit-when-off, like the stream suppression box. + it("maps the cookie jar into settings, and back to unset", async () => { + const user = userEvent.setup(); + const onSettingsChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + const name = /Enable HTTP Cookie Jar/; + await user.click(screen.getByRole("checkbox", { name })); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ cookieJar: true }), + ); + + rerender( + , + ); + await user.click(screen.getByRole("checkbox", { name })); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ cookieJar: undefined }), + ); + }); + // #2144 — same omit-the-default shape as the refresh-token pair above. it("maps the revoke-on-clear opt-out into settings, and back to unset", async () => { const user = userEvent.setup(); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index 490e67d5e5..7fae4c9425 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -228,6 +228,11 @@ export function ServerSettingsModal({ }); } + function handleCookieJarChange(value: boolean) { + // Omit-when-off, like the stream suppression box above (#2936). + onSettingsChange({ ...settings, cookieJar: value ? true : undefined }); + } + function handleAdvertisedExtensionChange(key: string, checked: boolean) { const next = { ...settings.advertisedExtensions }; const ext = ADVERTISABLE_EXTENSIONS.find((e) => e.key === key); @@ -333,6 +338,7 @@ export function ServerSettingsModal({ onSuppressNotificationStreamChange={ handleSuppressNotificationStreamChange } + onCookieJarChange={handleCookieJarChange} onAdvertisedExtensionChange={handleAdvertisedExtensionChange} rendersApps={rendersApps} onMaxFetchRequestsChange={handleMaxFetchRequestsChange} diff --git a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts new file mode 100644 index 0000000000..040bcf6ce3 --- /dev/null +++ b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts @@ -0,0 +1,226 @@ +/** + * Unit coverage for the connection-scoped cookie jar (#2936). + * + * Cookies are credentials, so most of these cases are about where the jar + * must NOT act: another origin (OAuth discovery and token requests share the + * transport's fetch), an expired or deleted cookie, an oversized or malformed + * line, and a second connection's jar. The live-transport half — that the real + * SDK transports carry the replayed cookie through a sticky load balancer — is + * `integration/mcp/cookie-jar-affinity.test.ts`. + */ +import { describe, it, expect, vi } from "vitest"; +import { + ConnectionCookieJar, + createCookieJarFetch, + parseSetCookie, + MAX_COOKIES, + MAX_COOKIE_BYTES, +} from "@inspector/core/mcp/node/cookieJarFetch.js"; + +const SERVER = new URL("https://mcp.example.com/mcp"); + +/** + * A response carrying `Set-Cookie` lines. The unit project runs under + * happy-dom, whose `Headers` drops `Set-Cookie` as a browser's would, so the + * lines are served from a stand-in `headers` exposing the one method the + * wrapper reads. Node's real `Headers` path is exercised end to end by the + * integration test named above. + */ +function respondWith(...setCookie: string[]): Response { + const response = new Response("ok", { status: 200 }); + Object.defineProperty(response, "headers", { + value: { getSetCookie: () => setCookie }, + }); + return response; +} + +/** The `Cookie` header the base fetch saw on call `n` (0-based). */ +function sentCookie( + baseFetch: ReturnType, + n: number, +): string | null { + const [input, init] = baseFetch.mock.calls[n] as Parameters; + const headers = new Headers( + init?.headers ?? (input instanceof Request ? input.headers : undefined), + ); + return headers.get("cookie"); +} + +function setup(responses: Response[], now: () => number = Date.now) { + const baseFetch = vi.fn(async () => responses.shift() ?? respondWith()); + const jar = new ConnectionCookieJar(now); + return { + baseFetch, + jar, + fetchFn: createCookieJarFetch(baseFetch, SERVER, jar), + }; +} + +describe("createCookieJarFetch (#2936)", () => { + it("stores Set-Cookie from the server's origin and replays it on later requests", async () => { + const { baseFetch, fetchFn } = setup([ + respondWith("lb=pod-a; Path=/; HttpOnly", "other=1"), + ]); + await fetchFn(SERVER.href, { method: "POST" }); + expect(sentCookie(baseFetch, 0)).toBeNull(); + await fetchFn(SERVER.href, { method: "POST" }); + expect(sentCookie(baseFetch, 1)).toBe("lb=pod-a; other=1"); + }); + + it("passes the request through untouched while the jar is empty", async () => { + const { baseFetch, fetchFn } = setup([]); + const init = { method: "POST", headers: { "x-a": "1" } }; + await fetchFn(SERVER.href, init); + expect(baseFetch).toHaveBeenCalledWith(SERVER.href, init); + }); + + it("replays onto URL and Request inputs, keeping the Request's headers", async () => { + const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a")]); + await fetchFn(new URL(SERVER.href)); + await fetchFn(new Request(SERVER.href, { headers: { "x-keep": "yes" } })); + expect(sentCookie(baseFetch, 1)).toBe("lb=pod-a"); + const [, init] = baseFetch.mock.calls[1] as Parameters; + expect(new Headers(init?.headers).get("x-keep")).toBe("yes"); + }); + + it("merges onto an explicit Cookie header, which wins on a shared name", async () => { + const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a", "tok=jar")]); + await fetchFn(SERVER.href); + await fetchFn(SERVER.href, { headers: { cookie: "tok=static; =x; flag" } }); + expect(sentCookie(baseFetch, 1)).toBe("tok=static; =x; flag; lb=pod-a"); + }); + + it("leaves an explicit Cookie header alone when the jar has nothing new", async () => { + const { baseFetch, fetchFn } = setup([respondWith("tok=jar")]); + await fetchFn(SERVER.href); + const init = { headers: { cookie: "tok=static" } }; + await fetchFn(SERVER.href, init); + expect(baseFetch.mock.calls[1]).toEqual([SERVER.href, init]); + }); + + it("neither stores from nor replays to another origin", async () => { + const { baseFetch, fetchFn, jar } = setup([ + respondWith("auth=secret"), + respondWith("lb=pod-a"), + ]); + await fetchFn("https://auth.example.com/token", { method: "POST" }); + expect(jar.size).toBe(0); + await fetchFn(SERVER.href); + await fetchFn("https://auth.example.com/token"); + await fetchFn("http://mcp.example.com/mcp"); + await fetchFn("https://mcp.example.com:8443/mcp"); + expect(sentCookie(baseFetch, 2)).toBeNull(); + expect(sentCookie(baseFetch, 3)).toBeNull(); + expect(sentCookie(baseFetch, 4)).toBeNull(); + }); + + it("replays to any path on the server's origin", async () => { + const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a; Path=/mcp")]); + await fetchFn(SERVER.href); + await fetchFn("https://mcp.example.com/messages?sessionId=1"); + expect(sentCookie(baseFetch, 1)).toBe("lb=pod-a"); + }); + + it("passes an unparseable URL through untouched", async () => { + const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a")]); + await fetchFn("not a url"); + expect(baseFetch).toHaveBeenCalledWith("not a url", undefined); + }); + + it("keeps separate jars separate", async () => { + const first = setup([respondWith("lb=pod-a")]); + const second = setup([]); + await first.fetchFn(SERVER.href); + await second.fetchFn(SERVER.href); + expect(sentCookie(second.baseFetch, 0)).toBeNull(); + }); +}); + +describe("ConnectionCookieJar (#2936)", () => { + it("overwrites a same-name cookie", () => { + const jar = new ConnectionCookieJar(); + jar.store(["lb=pod-a"]); + jar.store(["lb=pod-b"]); + expect(jar.cookieHeader()).toBe("lb=pod-b"); + }); + + it("expires by Max-Age, which wins over Expires", () => { + let now = 1_000_000; + const jar = new ConnectionCookieJar(() => now); + const farFuture = new Date(now + 3_600_000).toUTCString(); + jar.store([`lb=pod-a; Expires=${farFuture}; Max-Age=10`]); + expect(jar.cookieHeader()).toBe("lb=pod-a"); + now += 10_000; + expect(jar.cookieHeader()).toBeUndefined(); + }); + + it("expires by Expires when there is no Max-Age", () => { + let now = Date.parse("2026-01-01T00:00:00Z"); + const jar = new ConnectionCookieJar(() => now); + jar.store(["lb=pod-a; Expires=Thu, 01 Jan 2026 00:01:00 GMT"]); + expect(jar.size).toBe(1); + now += 60_000; + expect(jar.size).toBe(0); + }); + + it("deletes a cookie the server expires", () => { + const jar = new ConnectionCookieJar(); + jar.store(["lb=pod-a", "keep=1"]); + jar.store(["lb=; Max-Age=0"]); + expect(jar.cookieHeader()).toBe("keep=1"); + jar.store(["keep=; Expires=Thu, 01 Jan 1970 00:00:00 GMT"]); + expect(jar.cookieHeader()).toBeUndefined(); + }); + + it("evicts the oldest entry past MAX_COOKIES", () => { + const jar = new ConnectionCookieJar(); + jar.store(Array.from({ length: MAX_COOKIES + 1 }, (_, i) => `c${i}=${i}`)); + expect(jar.size).toBe(MAX_COOKIES); + expect(jar.cookieHeader()).not.toMatch(/(^|; )c0=/); + expect(jar.cookieHeader()).toMatch(new RegExp(`c${MAX_COOKIES}=`)); + }); + + it("refreshes an overwritten cookie's age so it is not evicted first", () => { + const jar = new ConnectionCookieJar(); + jar.store(Array.from({ length: MAX_COOKIES }, (_, i) => `c${i}=${i}`)); + jar.store(["c0=again", "extra=1"]); + expect(jar.cookieHeader()).toMatch(/c0=again/); + expect(jar.cookieHeader()).not.toMatch(/(^|; )c1=/); + }); + + it("drops malformed and oversized lines", () => { + const jar = new ConnectionCookieJar(); + jar.store([ + "no-equals-sign", + "=no-name", + `big=${"x".repeat(MAX_COOKIE_BYTES)}`, + "ok=1", + ]); + expect(jar.cookieHeader()).toBe("ok=1"); + }); +}); + +describe("parseSetCookie (#2936)", () => { + const now = 1_000; + + it("reads name, value and session lifetime, ignoring other attributes", () => { + expect( + parseSetCookie(" lb = pod-a ; Path=/; Secure; HttpOnly; Domain=x", now), + ).toEqual({ name: "lb", value: "pod-a", expiresAt: undefined }); + }); + + it("ignores a non-numeric Max-Age and an unparseable Expires", () => { + expect( + parseSetCookie("lb=a; Max-Age=soon; Expires=never", now)?.expiresAt, + ).toBeUndefined(); + }); + + it("treats a negative Max-Age as already expired", () => { + expect(parseSetCookie("lb=a; max-age=-1", now)?.expiresAt).toBe(now); + }); + + it("accepts a pair exactly at the byte limit", () => { + const value = "x".repeat(MAX_COOKIE_BYTES - "lb=".length); + expect(parseSetCookie(`lb=${value}`, now)?.value).toBe(value); + }); +}); diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 09be7bffd5..f9b2a7df8a 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -383,6 +383,39 @@ describe("serverEntriesToMcpConfig", () => { ).toBe(false); }); + it("round-trips cookieJar: lifts true to settings and back to disk (#2936)", () => { + const original: MCPConfig = { + mcpServers: { + sticky: { + type: "sse", + url: "https://x.test/sse", + cookieJar: true, + }, + }, + }; + const [entry] = mcpConfigToServerEntries(original); + expect(entry?.settings?.cookieJar).toBe(true); + const round = serverEntriesToMcpConfig(mcpConfigToServerEntries(original)); + expect(round).toEqual(original); + }); + + it("drops a non-true cookieJar on read and omits it on write (#2936)", () => { + const original = { + mcpServers: { + unsticky: { + type: "streamable-http", + url: "https://x.test/mcp", + cookieJar: false, + }, + }, + } satisfies MCPConfig; + const [entry] = mcpConfigToServerEntries(original); + expect(entry?.settings).toBeDefined(); + expect(entry?.settings?.cookieJar).toBeUndefined(); + const round = serverEntriesToMcpConfig(mcpConfigToServerEntries(original)); + expect("cookieJar" in (round.mcpServers.unsticky ?? {})).toBe(false); + }); + it("round-trips advertisedExtensions: lifts a non-empty map to settings and back to disk", () => { const original: MCPConfig = { mcpServers: { diff --git a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts new file mode 100644 index 0000000000..c27c1e2414 --- /dev/null +++ b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts @@ -0,0 +1,193 @@ +/** + * End-to-end coverage of the `cookieJar` setting (#2936) through the real SDK + * transports and a load balancer that routes by cookie affinity. + * + * Two single-process test servers stand in for two replicas of one MCP + * deployment. In front of them sits an in-process proxy shaped like a sticky + * ingress: a request carrying its `lb` cookie goes to that replica, and any + * other request is round-robined and answered with `Set-Cookie: lb=`. + * A replica that does not hold the MCP session answers `404`, exactly as a + * real deployment does. + * + * The control arm proves the proxy would break a connection without the jar, + * so the jar arm passing is evidence of the jar rather than of a lenient + * proxy. Both arms run on Streamable HTTP and on legacy SSE, since the jar is + * wired into each transport's fetch chain separately. + */ +import { describe, it, expect, afterEach } from "vitest"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { Readable } from "node:stream"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import type { + InspectorServerSettings, + MCPServerConfig, +} from "@inspector/core/mcp/types.js"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + createNumberedTools, +} from "@modelcontextprotocol/inspector-test-server"; + +/** Hop-by-hop or recomputed headers the proxy must not copy verbatim. */ +const SKIPPED_HEADERS = new Set([ + "host", + "connection", + "content-length", + "content-encoding", + "transfer-encoding", + "keep-alive", +]); + +interface AffinityProxy { + url: string; + /** The `cookie` header of every request the proxy received, in order. */ + cookies: (string | undefined)[]; + stop(): Promise; +} + +/** + * Start a round-robin proxy over `replicas` that pins a request to a replica + * by its `lb` cookie. + */ +async function startAffinityProxy( + replicas: readonly string[], +): Promise { + let next = 0; + const cookies: (string | undefined)[] = []; + const server = http.createServer((req, res) => { + cookies.push(req.headers.cookie); + const pinned = /(?:^|;\s*)lb=(\d+)/.exec(req.headers.cookie ?? ""); + const index = pinned ? Number(pinned[1]) : next++ % replicas.length; + const chunks: Buffer[] = []; + req.on("data", (chunk: Buffer) => chunks.push(chunk)); + req.on("end", () => { + const abort = new AbortController(); + res.on("close", () => abort.abort()); + const headers = new Headers(); + for (const [name, value] of Object.entries(req.headers)) { + if (SKIPPED_HEADERS.has(name) || value === undefined) continue; + headers.set(name, Array.isArray(value) ? value.join(", ") : value); + } + const body = chunks.length > 0 ? Buffer.concat(chunks) : undefined; + fetch(new URL(req.url ?? "/", replicas[index]), { + method: req.method, + headers, + body, + signal: abort.signal, + }).then( + (upstream) => { + const out: Record = {}; + upstream.headers.forEach((value, name) => { + if (!SKIPPED_HEADERS.has(name)) out[name] = value; + }); + if (!pinned) out["set-cookie"] = `lb=${index}; Path=/; HttpOnly`; + res.writeHead(upstream.status, out); + if (!upstream.body) { + res.end(); + return; + } + Readable.fromWeb(upstream.body) + .on("error", () => res.end()) + .pipe(res); + }, + () => { + if (!res.headersSent) res.writeHead(502); + res.end(); + }, + ); + }); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { + url: `http://127.0.0.1:${port}`, + cookies, + stop: () => + new Promise((resolve) => { + server.closeAllConnections(); + server.close(() => resolve()); + }), + }; +} + +function settings(cookieJar: boolean): InspectorServerSettings { + return { + headers: [], + metadata: {}, + env: [], + connectionTimeout: 0, + requestTimeout: 5_000, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + ...(cookieJar && { cookieJar: true }), + }; +} + +describe.each(["streamable-http", "sse"] as const)( + "cookieJar behind a cookie-affinity load balancer — %s (#2936)", + (serverType) => { + let client: InspectorClient | null = null; + let replicas: TestServerHttp[] = []; + let proxy: AffinityProxy | null = null; + + afterEach(async () => { + await client?.disconnect().catch(() => {}); + client = null; + await proxy?.stop().catch(() => {}); + proxy = null; + await Promise.all(replicas.map((r) => r.stop().catch(() => {}))); + replicas = []; + }); + + async function connectThroughProxy(cookieJar: boolean) { + replicas = [0, 1].map(() => + createTestServerHttp({ + serverInfo: createTestServerInfo("affinity-test", "1.0.0"), + tools: createNumberedTools(2), + serverType, + }), + ); + await Promise.all(replicas.map((r) => r.start())); + proxy = await startAffinityProxy( + replicas.map((r) => new URL(r.url).origin), + ); + const path = serverType === "sse" ? "/sse" : "/mcp"; + const config: MCPServerConfig = { + type: serverType, + url: `${proxy.url}${path}`, + }; + client = new InspectorClient(config, { + environment: { transport: createTransportNode, fetch }, + serverSettings: settings(cookieJar), + }); + return { client, proxy }; + } + + it("fails after the first exchange without the jar (control)", async () => { + const { client: c, proxy: p } = await connectThroughProxy(false); + await expect( + (async () => { + await c.connect(); + await c.listTools(); + })(), + ).rejects.toThrow(); + expect(p.cookies.length).toBeGreaterThan(1); + expect(p.cookies.every((cookie) => cookie === undefined)).toBe(true); + }); + + it("pins the session to one replica with the jar", async () => { + const { client: c, proxy: p } = await connectThroughProxy(true); + await c.connect(); + expect((await c.listTools()).tools).toHaveLength(2); + expect(p.cookies[0]).toBeUndefined(); + expect(p.cookies.slice(1)).not.toHaveLength(0); + expect(p.cookies.slice(1).every((cookie) => cookie === "lb=0")).toBe( + true, + ); + }); + }, +); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 79eedd75d5..1e5024bdfe 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -1145,6 +1145,34 @@ describe("server.ts supplemental coverage", () => { expect(await readSuppressNotificationStream()).toBeUndefined(); }); + it("rejects a non-boolean cookieJar (#2936)", async () => { + const res = await postSettings({ ...base, cookieJar: "yes" }); + expect(res.status).toBe(400); + expect((await res.json()).error).toMatch(/cookieJar/); + }); + + async function readCookieJar() { + const res = await fetch(`${h.baseUrl}/api/servers`); + const body = (await res.json()) as { + mcpServers: Record>; + }; + return body.mcpServers.srv?.cookieJar; + } + + it("persists cookieJar through a save (#2936)", async () => { + expect((await postSettings({ ...base, cookieJar: true })).status).toBe( + 200, + ); + expect(await readCookieJar()).toBe(true); + }); + + it("writes no cookieJar field when off (#2936)", async () => { + expect((await postSettings({ ...base, cookieJar: false })).status).toBe( + 200, + ); + expect(await readCookieJar()).toBeUndefined(); + }); + it("rejects a negative maxFetchRequests", async () => { const res = await postSettings({ ...base, maxFetchRequests: -2 }); expect((await res.json()).error).toMatch(/maxFetchRequests/); diff --git a/core/mcp/node/cookieJarFetch.ts b/core/mcp/node/cookieJarFetch.ts new file mode 100644 index 0000000000..61424433ee --- /dev/null +++ b/core/mcp/node/cookieJarFetch.ts @@ -0,0 +1,222 @@ +/** + * A connection-scoped HTTP cookie jar for the remote MCP transports (#2936). + * + * The Node `fetch` neither stores `Set-Cookie` nor replays `Cookie`, so a + * server deployed as several replicas behind a load balancer that routes by + * **cookie affinity** is unreachable past `initialize`: the balancer hands out + * its affinity cookie on the first response, every later request leaves + * without it, and the requests that round-robin onto a replica not holding the + * MCP session are answered `404 Session not found`. + * + * The jar is opt-in per server (`InspectorServerSettings.cookieJar`) and + * deliberately small — enough for an affinity cookie, not a browser: + * + * - **Same origin only.** Cookies are stored from, and replayed onto, requests + * whose origin (scheme + host + port) is the MCP server's own. The SDK sends + * OAuth discovery and token requests through the same fetch, usually to + * another origin; those are neither read nor stamped. Because the origin + * includes the scheme, a cookie learned over `https` is only ever replayed + * over `https`. + * - **Host-only, path-less.** `Domain` and `Path` are ignored: a cookie is + * replayed to the server's whole origin and never to another host. That is + * narrower than a browser for `Domain` and wider for `Path`, which is the + * right trade for one MCP endpoint. + * - **Expiry.** `Max-Age` wins over `Expires` (RFC 6265 §5.3); a cookie whose + * expiry has passed — the way a server deletes one — is removed. + * - **Bounded.** At most {@link MAX_COOKIES} entries, each `name=value` pair at + * most {@link MAX_COOKIE_BYTES} bytes; the oldest entry is evicted first. + * - **Lifetime.** One jar per transport, held only in memory. Cookies never + * outlive the connection and are never written to disk, so a reconnect + * re-learns affinity from scratch and nothing leaks between servers. + * + * Cookies are credentials, so nothing here logs them. The Network log records + * the `Cookie` / `Set-Cookie` headers this wrapper sends and reads, and + * redacts both (`core/mcp/fetchTracking.ts`). + */ + +/** Most cookies one connection's jar holds; the oldest is evicted past it. */ +export const MAX_COOKIES = 50; +/** Largest `name=value` pair the jar accepts, in UTF-8 bytes. */ +export const MAX_COOKIE_BYTES = 4096; + +interface StoredCookie { + value: string; + /** Epoch ms after which the cookie is dropped; `undefined` = session. */ + expiresAt: number | undefined; +} + +/** + * The cookies one connection has learned from its server's origin. Shared by + * every fetch the transport makes, so a cookie delivered on the legacy SSE + * `GET` stream is replayed on its `POST`s and vice versa. + */ +export class ConnectionCookieJar { + /** Insertion-ordered, so the first key is the oldest entry. */ + private readonly cookies = new Map(); + + constructor(private readonly now: () => number = Date.now) {} + + /** Number of live (unexpired) cookies. */ + get size(): number { + this.dropExpired(); + return this.cookies.size; + } + + /** Fold one response's `Set-Cookie` lines into the jar. */ + store(setCookieLines: readonly string[]): void { + for (const line of setCookieLines) { + const parsed = parseSetCookie(line, this.now()); + if (!parsed) continue; + // Delete first so an overwrite moves the name to the newest position. + this.cookies.delete(parsed.name); + if (parsed.expiresAt !== undefined && parsed.expiresAt <= this.now()) { + continue; + } + this.cookies.set(parsed.name, { + value: parsed.value, + expiresAt: parsed.expiresAt, + }); + while (this.cookies.size > MAX_COOKIES) { + const oldest = this.cookies.keys().next().value; + /* v8 ignore next -- size > MAX_COOKIES ≥ 1 guarantees a first key */ + if (oldest === undefined) break; + this.cookies.delete(oldest); + } + } + } + + /** + * The `Cookie` header value for the live cookies, skipping any name in + * `exclude` (one the request already carries explicitly), or `undefined` + * when there is nothing to send. + */ + cookieHeader(exclude: ReadonlySet = new Set()): string | undefined { + this.dropExpired(); + const pairs: string[] = []; + for (const [name, cookie] of this.cookies) { + if (!exclude.has(name)) pairs.push(`${name}=${cookie.value}`); + } + return pairs.length > 0 ? pairs.join("; ") : undefined; + } + + private dropExpired(): void { + const now = this.now(); + for (const [name, cookie] of this.cookies) { + if (cookie.expiresAt !== undefined && cookie.expiresAt <= now) { + this.cookies.delete(name); + } + } + } +} + +/** + * Wrap `baseFetch` so requests to `serverUrl`'s origin carry the jar's + * cookies, and responses from that origin feed it. Requests to any other + * origin pass through untouched in both directions. + * + * A `Cookie` header the request already carries (a static custom header from + * the server settings) is kept, and wins: the jar appends only names it does + * not already name. + */ +export function createCookieJarFetch( + baseFetch: typeof fetch, + serverUrl: URL, + jar: ConnectionCookieJar, +): typeof fetch { + const origin = serverUrl.origin; + return async (input, init) => { + const url = requestUrl(input); + if (url?.origin !== origin) return baseFetch(input, init); + + const headers = new Headers( + init?.headers ?? (input instanceof Request ? input.headers : undefined), + ); + const existing = headers.get("cookie"); + const jarCookies = jar.cookieHeader(cookieNames(existing)); + let response: Response; + if (jarCookies === undefined) { + response = await baseFetch(input, init); + } else { + headers.set( + "cookie", + existing ? `${existing}; ${jarCookies}` : jarCookies, + ); + response = await baseFetch(input, { ...init, headers }); + } + jar.store(response.headers.getSetCookie()); + return response; + }; +} + +function requestUrl(input: Parameters[0]): URL | undefined { + const raw = + input instanceof Request + ? input.url + : input instanceof URL + ? input.href + : input; + try { + return new URL(raw); + } catch { + return undefined; + } +} + +/** The cookie names in an existing `Cookie` header value. */ +function cookieNames(header: string | null): Set { + const names = new Set(); + if (!header) return names; + for (const pair of header.split(";")) { + const eq = pair.indexOf("="); + const name = (eq === -1 ? pair : pair.slice(0, eq)).trim(); + if (name) names.add(name); + } + return names; +} + +const UTF8 = new TextEncoder(); + +interface ParsedSetCookie { + name: string; + value: string; + expiresAt: number | undefined; +} + +/** + * Parse one `Set-Cookie` line, or `undefined` for a malformed one (no `=`, an + * empty name, or a pair over {@link MAX_COOKIE_BYTES}). Only `Max-Age` and + * `Expires` are read; every other attribute is ignored (see the module + * comment). + */ +export function parseSetCookie( + line: string, + now: number, +): ParsedSetCookie | undefined { + const [pair = "", ...attributes] = line.split(";"); + const eq = pair.indexOf("="); + if (eq === -1) return undefined; + const name = pair.slice(0, eq).trim(); + const value = pair.slice(eq + 1).trim(); + if (!name) return undefined; + if (UTF8.encode(`${name}=${value}`).length > MAX_COOKIE_BYTES) { + return undefined; + } + + let maxAge: number | undefined; + let expires: number | undefined; + for (const attribute of attributes) { + const attrEq = attribute.indexOf("="); + if (attrEq === -1) continue; + const key = attribute.slice(0, attrEq).trim().toLowerCase(); + const attrValue = attribute.slice(attrEq + 1).trim(); + if (key === "max-age" && /^-?\d+$/.test(attrValue)) { + maxAge = Number(attrValue); + } else if (key === "expires") { + const parsed = Date.parse(attrValue); + if (!Number.isNaN(parsed)) expires = parsed; + } + } + const expiresAt = + maxAge !== undefined ? (maxAge <= 0 ? now : now + maxAge * 1000) : expires; + return { name, value, expiresAt }; +} diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index 349a3574b9..a272347a22 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -22,6 +22,7 @@ import { exemptMcpEndpoint } from "../../auth/requestTimeout.js"; import { routeOAuthRequests } from "../../auth/routeOAuthRequests.js"; import { createNotificationHeadersFetch } from "./notificationHeadersFetch.js"; import { createSuppressNotificationStreamFetch } from "./suppressNotificationStreamFetch.js"; +import { ConnectionCookieJar, createCookieJarFetch } from "./cookieJarFetch.js"; /** * Build the wire `headers` record from `settings.headers`, dropping rows with @@ -124,6 +125,16 @@ export function createTransportNode( ? createAuthChallengeInterceptFetch(trackedBase) : trackedBase; + // Opt-in cookie jar (#2936): one per transport, shared by every fetch the + // transport makes, so an affinity cookie learned on any response pins the + // whole session. It sits above the tracker so the Network log records the + // `Cookie` header actually sent (#2385) and the `Set-Cookie` the jar read — + // both redacted there. Off, the chains below are exactly what they were. + const cookieJar = + settings?.cookieJar === true ? new ConnectionCookieJar() : undefined; + const withCookieJar = (inner: typeof fetch, url: URL): typeof fetch => + cookieJar ? createCookieJarFetch(inner, url, cookieJar) : inner; + if (serverType === "stdio") { const stdioConfig = config as StdioServerConfig; const transport = new StdioClientTransport({ @@ -170,7 +181,7 @@ export function createTransportNode( const eventSourceInit: Record = { ...sseConfig.eventSourceInit, ...(headers && { headers }), - fetch: trackedFetch, + fetch: withCookieJar(trackedFetch, url), }; const requestInit: RequestInit = { @@ -182,7 +193,7 @@ export function createTransportNode( authProvider, eventSourceInit, requestInit, - fetch: fetchWithOptionalAuthIntercept, + fetch: withCookieJar(fetchWithOptionalAuthIntercept, url), }); return { transport }; @@ -198,14 +209,14 @@ export function createTransportNode( ...(headers && { headers }), }; - // Both wrappers sit above the tracker. Header stamping, so the tracker - // records the headers actually sent (#2385); stream suppression outermost - // of all, so a suppressed GET is answered before it reaches the tracker — + // Both wrappers sit above the tracker and the cookie jar. Header + // stamping, so the tracker records the headers actually sent (#2385); + // stream suppression outermost of all, so a suppressed GET is answered before it reaches the tracker — // it is never sent, and the Network log should not show a request the // server never saw (#2317). They touch disjoint requests (notification // POSTs vs. the endpoint's SSE GET), so their relative order is free. const stampedFetch = createNotificationHeadersFetch( - fetchWithOptionalAuthIntercept, + withCookieJar(fetchWithOptionalAuthIntercept, url), ); const httpFetch = settings?.suppressNotificationStream === true diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index fb5c6e0b4b..2477cd6cc4 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -2388,6 +2388,13 @@ export function createRemoteApp( error: "settings.suppressNotificationStream must be a boolean", }; } + // Optional on the wire; boolean when present, else unset (off) (#2936). + if (obj.cookieJar !== undefined && typeof obj.cookieJar !== "boolean") { + return { + ok: false, + error: "settings.cookieJar must be a boolean", + }; + } // maxFetchRequests is optional on the wire (older clients won't send it); // when present it must be a non-negative number (0 = unlimited), otherwise // it defaults below. @@ -2558,6 +2565,8 @@ export function createRemoteApp( ...(obj.suppressNotificationStream === true && { suppressNotificationStream: true, }), + // Absent → unset (off); only an explicit true is carried (#2936). + ...(obj.cookieJar === true && { cookieJar: true }), // Absent → product default, matching the read side. The default is the // omit-sentinel in inspectorSettingsToStoredFields, so a client that // didn't send one writes no spurious maxFetchRequests to disk. diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index c6e8729226..6e935ea1b0 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -189,6 +189,7 @@ type StoredInspectorFields = Pick< | "autoRefreshOnListChanged" | "paginatedLists" | "suppressNotificationStream" + | "cookieJar" | "advertisedExtensions" | "maxFetchRequests" | "skillCatalogMaxSkills" @@ -553,6 +554,7 @@ export function storedFieldsToInspectorSettings( stored.autoRefreshOnListChanged !== undefined || stored.paginatedLists !== undefined || stored.suppressNotificationStream !== undefined || + stored.cookieJar !== undefined || stored.advertisedExtensions !== undefined || stored.maxFetchRequests !== undefined || stored.skillCatalogMaxSkills !== undefined || @@ -609,6 +611,10 @@ export function storedFieldsToInspectorSettings( if (stored.suppressNotificationStream === true) { settings.suppressNotificationStream = true; } + // Same shape: only an explicit `true` enables the cookie jar (#2936). + if (stored.cookieJar === true) { + settings.cookieJar = true; + } // Absent on disk reads back as the default era; the write side then omits the // default so a byte-stable round-trip never injects `protocolEra` into files // that never set it. An unknown literal from a hand-edited file is dropped @@ -765,6 +771,10 @@ export function inspectorSettingsToStoredFields( if (settings.suppressNotificationStream) { out.suppressNotificationStream = true; } + // Persist only when enabled — absent reads back as unset (off) (#2936). + if (settings.cookieJar) { + out.cookieJar = true; + } // Persist only when the user has toggled at least one extension override; // an empty map reads back as unset (above), keeping the diff minimal for the @@ -914,6 +924,7 @@ const INSPECTOR_FIELD_KEY_MAP = { autoRefreshOnListChanged: true, paginatedLists: true, suppressNotificationStream: true, + cookieJar: true, advertisedExtensions: true, maxFetchRequests: true, skillCatalogMaxSkills: true, diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 9e9eca02fc..a062bc4fb0 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -192,6 +192,12 @@ export type StoredMCPServer = MCPServerConfig & { * Inspector-specific. Omitted on disk when false (the default). (#2317) */ suppressNotificationStream?: boolean; + /** + * When true, an HTTP (Streamable HTTP or legacy SSE) connection keeps a + * connection-scoped cookie jar. See {@link InspectorServerSettings.cookieJar}. + * Inspector-specific. Omitted on disk when false (the default). (#2936) + */ + cookieJar?: boolean; /** * Per-extension overrides for which extensions the Inspector advertises to * this server (keyed by extension id; a present key wins over the registry @@ -1039,6 +1045,18 @@ export interface InspectorServerSettings { * transports. Default false. */ suppressNotificationStream?: boolean; + /** + * When true, a Streamable HTTP or legacy SSE connection keeps an in-memory + * cookie jar for the server's origin: `Set-Cookie` from its responses is + * stored and replayed as `Cookie` on later requests to the same origin + * (#2936). For servers deployed as several replicas behind a load balancer + * that routes by cookie affinity — without it, every request after + * `initialize` may land on a replica that does not hold the MCP session and + * fail with `404 Session not found`. The jar lives for one connection (a + * reconnect starts empty), is never persisted, and never touches requests to + * another origin. Read at connect time; no effect on stdio. Default false. + */ + cookieJar?: boolean; /** * Maximum number of HTTP fetch requests retained in the Network log for this * server. When exceeded, the oldest entries rotate out (and any deferred diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index b2b15794b4..1598eefd36 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -204,6 +204,7 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `autoRefreshOnListChanged` | `false` | Refresh lists automatically on `*/list_changed` instead of only flagging the indicator | | `paginatedLists` | `false` | Fetch tools/resources/prompts one page at a time instead of auto-aggregating | | `suppressNotificationStream` | `false` | Streamable HTTP, legacy era only: don't open the standalone `GET` notification stream. Server→client messages not carried on a request's own response stream won't arrive. `Last-Event-ID` resumption `GET`s still go out, and modern-era connections are unaffected (they never open this stream). A diagnostic and escape hatch for a server that times out every request after `initialize` because it cannot serve a second concurrent request ([#2317](https://github.com/modelcontextprotocol/inspector/issues/2317)) | +| `cookieJar` | `false` | Streamable HTTP and legacy SSE: keep an in-memory cookie jar for the connection — store `Set-Cookie` from the server's origin and send it back as `Cookie` on later requests to that origin. For a server running as several replicas behind a load balancer that routes by cookie affinity, where requests after `initialize` otherwise land on a replica without the session and fail with `404 Session not found`. Cookies last one connection, are never written to disk, and are never sent to another origin (OAuth discovery and token requests are untouched) ([#2936](https://github.com/modelcontextprotocol/inspector/issues/2936)) | | `advertisedExtensions` | — | Per-extension overrides for what the Inspector declares in `capabilities.extensions` | | `maxFetchRequests` | `1000` | Network-log retention for this server (`DEFAULT_MAX_FETCH_REQUESTS`); `0` means unlimited | | `skillCatalogMaxSkills` | `256` | The maximum number of skills whose files are read in one verification run (`SKILL_MAX_CATALOG_SKILLS`) — the CLI's `--verify` and the TUI Skills pane's `v` (verify all); verifying a single skill always reads it in full. Positive integer; there is no unlimited value | From 5c4853288b3ad38758ed4264a7071904b29be8d3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 00:28:44 -0500 Subject: [PATCH 2/7] Fix typecheck: no parameter property, typed fetch mock, manual body pump (#2936) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../test/core/mcp/node/cookieJarFetch.test.ts | 17 ++++++----- .../mcp/cookie-jar-affinity.test.ts | 29 ++++++++++++++++--- core/mcp/node/cookieJarFetch.ts | 6 +++- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts index 040bcf6ce3..30dfd44c45 100644 --- a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts +++ b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts @@ -8,7 +8,7 @@ * SDK transports carry the replayed cookie through a sticky load balancer — is * `integration/mcp/cookie-jar-affinity.test.ts`. */ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi, type Mock } from "vitest"; import { ConnectionCookieJar, createCookieJarFetch, @@ -35,11 +35,10 @@ function respondWith(...setCookie: string[]): Response { } /** The `Cookie` header the base fetch saw on call `n` (0-based). */ -function sentCookie( - baseFetch: ReturnType, - n: number, -): string | null { - const [input, init] = baseFetch.mock.calls[n] as Parameters; +function sentCookie(baseFetch: Mock, n: number): string | null { + const call = baseFetch.mock.calls[n]; + if (!call) throw new Error(`no fetch call ${n}`); + const [input, init] = call; const headers = new Headers( init?.headers ?? (input instanceof Request ? input.headers : undefined), ); @@ -47,7 +46,9 @@ function sentCookie( } function setup(responses: Response[], now: () => number = Date.now) { - const baseFetch = vi.fn(async () => responses.shift() ?? respondWith()); + const baseFetch = vi.fn( + async () => responses.shift() ?? respondWith(), + ); const jar = new ConnectionCookieJar(now); return { baseFetch, @@ -79,7 +80,7 @@ describe("createCookieJarFetch (#2936)", () => { await fetchFn(new URL(SERVER.href)); await fetchFn(new Request(SERVER.href, { headers: { "x-keep": "yes" } })); expect(sentCookie(baseFetch, 1)).toBe("lb=pod-a"); - const [, init] = baseFetch.mock.calls[1] as Parameters; + const init = baseFetch.mock.calls[1]?.[1]; expect(new Headers(init?.headers).get("x-keep")).toBe("yes"); }); diff --git a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts index c27c1e2414..a6e1bce2d2 100644 --- a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts +++ b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts @@ -17,7 +17,6 @@ import { describe, it, expect, afterEach } from "vitest"; import http from "node:http"; import type { AddressInfo } from "node:net"; -import { Readable } from "node:stream"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; import type { @@ -41,6 +40,29 @@ const SKIPPED_HEADERS = new Set([ "keep-alive", ]); +/** + * Stream an upstream body to the client as it arrives, so an SSE response + * reaches the transport event by event. Owns its failures (an aborted + * upstream ends the response), so the caller may discard the promise. + */ +async function pipeBody( + body: ReadableStream, + res: http.ServerResponse, +): Promise { + const reader = body.getReader(); + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + res.write(value); + } + } catch { + // Upstream aborted (the client went away); just end the response. + } finally { + res.end(); + } +} + interface AffinityProxy { url: string; /** The `cookie` header of every request the proxy received, in order. */ @@ -89,9 +111,8 @@ async function startAffinityProxy( res.end(); return; } - Readable.fromWeb(upstream.body) - .on("error", () => res.end()) - .pipe(res); + // pipeBody owns its failures: it ends the response on any error. + void pipeBody(upstream.body, res); }, () => { if (!res.headersSent) res.writeHead(502); diff --git a/core/mcp/node/cookieJarFetch.ts b/core/mcp/node/cookieJarFetch.ts index 61424433ee..520b5cc8f8 100644 --- a/core/mcp/node/cookieJarFetch.ts +++ b/core/mcp/node/cookieJarFetch.ts @@ -54,7 +54,11 @@ export class ConnectionCookieJar { /** Insertion-ordered, so the first key is the oldest entry. */ private readonly cookies = new Map(); - constructor(private readonly now: () => number = Date.now) {} + private readonly now: () => number; + + constructor(now: () => number = Date.now) { + this.now = now; + } /** Number of live (unexpired) cookies. */ get size(): number { From c346b171e5a36be485bb63a7cdba097bdf0501ec Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 01:01:11 -0500 Subject: [PATCH 3/7] Store cookies only from responses whose final URL is the server's origin (#2936) fetch follows redirects, so a cross-origin redirect could otherwise plant another origin's cookies in the jar and replay them to the MCP server. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../test/core/mcp/node/cookieJarFetch.test.ts | 19 +++++++++++++++++++ core/mcp/node/cookieJarFetch.ts | 9 ++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts index 30dfd44c45..c023e0d03f 100644 --- a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts +++ b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts @@ -34,6 +34,13 @@ function respondWith(...setCookie: string[]): Response { return response; } +/** As {@link respondWith}, but as if `fetch` followed a redirect to `url`. */ +function redirectedTo(url: string, ...setCookie: string[]): Response { + const response = respondWith(...setCookie); + Object.defineProperty(response, "url", { value: url }); + return response; +} + /** The `Cookie` header the base fetch saw on call `n` (0-based). */ function sentCookie(baseFetch: Mock, n: number): string | null { const call = baseFetch.mock.calls[n]; @@ -115,6 +122,18 @@ describe("createCookieJarFetch (#2936)", () => { expect(sentCookie(baseFetch, 4)).toBeNull(); }); + it("ignores cookies from a response redirected to another origin", async () => { + const { baseFetch, fetchFn, jar } = setup([ + redirectedTo("https://elsewhere.example.com/landing", "foreign=1"), + redirectedTo("https://mcp.example.com/mcp/next", "lb=pod-a"), + ]); + await fetchFn(SERVER.href); + expect(jar.size).toBe(0); + await fetchFn(SERVER.href); + await fetchFn(SERVER.href); + expect(sentCookie(baseFetch, 2)).toBe("lb=pod-a"); + }); + it("replays to any path on the server's origin", async () => { const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a; Path=/mcp")]); await fetchFn(SERVER.href); diff --git a/core/mcp/node/cookieJarFetch.ts b/core/mcp/node/cookieJarFetch.ts index 520b5cc8f8..3f12e6b1e3 100644 --- a/core/mcp/node/cookieJarFetch.ts +++ b/core/mcp/node/cookieJarFetch.ts @@ -147,7 +147,14 @@ export function createCookieJarFetch( ); response = await baseFetch(input, { ...init, headers }); } - jar.store(response.headers.getSetCookie()); + // `fetch` follows redirects, so the response may come from another + // origin than the request; only cookies the server's own origin set are + // kept. An empty `url` is a synthetic response, which never left the + // request's origin. + const finalUrl = response.url ? requestUrl(response.url) : url; + if (finalUrl?.origin === origin) { + jar.store(response.headers.getSetCookie()); + } return response; }; } From 5248e083d5dbf30a17070656758d21eda983187b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 01:26:48 -0500 Subject: [PATCH 4/7] Refuse cookies from URL-less responses; prune expired before evicting (#2936) A response rebuilt by a wrapper below the jar (OAuth endpoint overrides, discovery compat) reports an empty url and has lost its provenance, so it can no longer be trusted as same-origin. And expired entries no longer hold capacity that would evict a live affinity cookie. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../test/core/mcp/node/cookieJarFetch.test.ts | 32 ++++++++++++++++--- core/mcp/node/cookieJarFetch.ts | 10 ++++-- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts index c023e0d03f..536a65e358 100644 --- a/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts +++ b/clients/web/src/test/core/mcp/node/cookieJarFetch.test.ts @@ -26,19 +26,25 @@ const SERVER = new URL("https://mcp.example.com/mcp"); * wrapper reads. Node's real `Headers` path is exercised end to end by the * integration test named above. */ -function respondWith(...setCookie: string[]): Response { +function respondFrom(url: string, ...setCookie: string[]): Response { const response = new Response("ok", { status: 200 }); Object.defineProperty(response, "headers", { value: { getSetCookie: () => setCookie }, }); + // A network response reports the URL it finally came from; a rebuilt one + // (`new Response`) reports "". + Object.defineProperty(response, "url", { value: url }); return response; } +/** A network response from the MCP endpoint itself. */ +function respondWith(...setCookie: string[]): Response { + return respondFrom(SERVER.href, ...setCookie); +} + /** As {@link respondWith}, but as if `fetch` followed a redirect to `url`. */ function redirectedTo(url: string, ...setCookie: string[]): Response { - const response = respondWith(...setCookie); - Object.defineProperty(response, "url", { value: url }); - return response; + return respondFrom(url, ...setCookie); } /** The `Cookie` header the base fetch saw on call `n` (0-based). */ @@ -134,6 +140,12 @@ describe("createCookieJarFetch (#2936)", () => { expect(sentCookie(baseFetch, 2)).toBe("lb=pod-a"); }); + it("refuses cookies from a rebuilt response that lost its URL", async () => { + const { fetchFn, jar } = setup([respondFrom("", "lb=pod-a")]); + await fetchFn(SERVER.href); + expect(jar.size).toBe(0); + }); + it("replays to any path on the server's origin", async () => { const { baseFetch, fetchFn } = setup([respondWith("lb=pod-a; Path=/mcp")]); await fetchFn(SERVER.href); @@ -208,6 +220,18 @@ describe("ConnectionCookieJar (#2936)", () => { expect(jar.cookieHeader()).not.toMatch(/(^|; )c1=/); }); + it("prunes expired cookies before evicting a live one for capacity", () => { + let now = 1_000_000; + const jar = new ConnectionCookieJar(() => now); + jar.store(["lb=pod-a"]); + jar.store( + Array.from({ length: MAX_COOKIES - 1 }, (_, i) => `t${i}=1; Max-Age=1`), + ); + now += 2_000; + jar.store(["fresh=1"]); + expect(jar.cookieHeader()).toBe("lb=pod-a; fresh=1"); + }); + it("drops malformed and oversized lines", () => { const jar = new ConnectionCookieJar(); jar.store([ diff --git a/core/mcp/node/cookieJarFetch.ts b/core/mcp/node/cookieJarFetch.ts index 3f12e6b1e3..6276703e54 100644 --- a/core/mcp/node/cookieJarFetch.ts +++ b/core/mcp/node/cookieJarFetch.ts @@ -80,6 +80,8 @@ export class ConnectionCookieJar { value: parsed.value, expiresAt: parsed.expiresAt, }); + // Expired entries must not hold capacity a live cookie needs. + if (this.cookies.size > MAX_COOKIES) this.dropExpired(); while (this.cookies.size > MAX_COOKIES) { const oldest = this.cookies.keys().next().value; /* v8 ignore next -- size > MAX_COOKIES ≥ 1 guarantees a first key */ @@ -149,9 +151,11 @@ export function createCookieJarFetch( } // `fetch` follows redirects, so the response may come from another // origin than the request; only cookies the server's own origin set are - // kept. An empty `url` is a synthetic response, which never left the - // request's origin. - const finalUrl = response.url ? requestUrl(response.url) : url; + // kept. A response with no `url` has lost its provenance — a wrapper + // below rebuilt it (OAuth endpoint overrides, discovery compatibility) + // — so its cookies are refused rather than attributed to the request's + // origin. Network responses always carry one (`proxyFetch` restores it). + const finalUrl = response.url ? requestUrl(response.url) : undefined; if (finalUrl?.origin === origin) { jar.store(response.headers.getSetCookie()); } From 8a79da8bd3f227ef0b99063ba6ab00ee921245cd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 01:45:34 -0500 Subject: [PATCH 5/7] Place the cookie jar below the auth-challenge interceptor (#2936) The interceptor throws on a 401/403, so a jar above it never saw that response's Set-Cookie and the OAuth retry carried stale cookies. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../core/mcp/node/transportCookieJar.test.ts | 86 +++++++++++++++++++ core/mcp/node/cookieJarFetch.ts | 11 +++ core/mcp/node/transport.ts | 40 +++++---- 3 files changed, 119 insertions(+), 18 deletions(-) create mode 100644 clients/web/src/test/core/mcp/node/transportCookieJar.test.ts diff --git a/clients/web/src/test/core/mcp/node/transportCookieJar.test.ts b/clients/web/src/test/core/mcp/node/transportCookieJar.test.ts new file mode 100644 index 0000000000..a892a3a110 --- /dev/null +++ b/clients/web/src/test/core/mcp/node/transportCookieJar.test.ts @@ -0,0 +1,86 @@ +/** + * Where the cookie jar sits in `createTransportNode`'s fetch chain (#2936). + * + * The jar must sit *below* the auth-challenge interceptor: the interceptor + * turns a 401/403 into a throw, so a jar above it would never see that + * response's `Set-Cookie`, and the OAuth retry that follows would carry stale + * cookies. The two-replica behavior itself is + * `integration/mcp/cookie-jar-affinity.test.ts`. + */ +import { describe, it, expect, vi } from "vitest"; +import type { JSONRPCRequest } from "@modelcontextprotocol/client"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; + +const URL_ = "https://mcp.example.com/mcp"; + +const SETTINGS: InspectorServerSettings = { + headers: [], + metadata: {}, + env: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + cookieJar: true, +}; + +/** + * A network response from the MCP endpoint. happy-dom's `Headers` drops + * `Set-Cookie` as a browser's would, so the lines are served through an own + * `getSetCookie` on an otherwise real `Headers`. + */ +function networkResponse( + status: number, + body: string | null, + headers: Record, + setCookie: string[] = [], +): Response { + const response = new Response(body, { status, headers }); + Object.defineProperty(response.headers, "getSetCookie", { + value: () => setCookie, + }); + Object.defineProperty(response, "url", { value: URL_ }); + return response; +} + +const REQUEST: JSONRPCRequest = { + jsonrpc: "2.0", + id: 1, + method: "tools/list", + params: {}, +}; + +describe("createTransportNode cookie jar placement (#2936)", () => { + it("keeps a cookie set on a 401 the interceptor throws on", async () => { + const fetchFn = vi.fn(async () => + networkResponse( + 200, + JSON.stringify({ jsonrpc: "2.0", id: 1, result: { tools: [] } }), + { "content-type": "application/json" }, + ), + ); + fetchFn.mockResolvedValueOnce( + networkResponse(401, null, { "www-authenticate": 'Bearer realm="mcp"' }, [ + "lb=pod-b", + ]), + ); + const { transport } = createTransportNode( + { type: "streamable-http", url: URL_ }, + { fetchFn, settings: SETTINGS, interceptAuthChallenges: true }, + ); + await transport.start(); + await expect(transport.send(REQUEST)).rejects.toThrow(); + await transport.send(REQUEST); + await transport.close(); + + const cookies = fetchFn.mock.calls.map(([input, init]) => + new Headers( + init?.headers ?? (input instanceof Request ? input.headers : undefined), + ).get("cookie"), + ); + expect(cookies[0]).toBeNull(); + expect(cookies[1]).toBe("lb=pod-b"); + }); +}); diff --git a/core/mcp/node/cookieJarFetch.ts b/core/mcp/node/cookieJarFetch.ts index 6276703e54..e19714e116 100644 --- a/core/mcp/node/cookieJarFetch.ts +++ b/core/mcp/node/cookieJarFetch.ts @@ -163,6 +163,17 @@ export function createCookieJarFetch( }; } +/** + * One fresh jar for a transport, as a wrapper to apply to each of the + * transport's fetches — they all share it. + */ +export function createCookieJarWrapper( + serverUrl: URL, +): (inner: typeof fetch) => typeof fetch { + const jar = new ConnectionCookieJar(); + return (inner) => createCookieJarFetch(inner, serverUrl, jar); +} + function requestUrl(input: Parameters[0]): URL | undefined { const raw = input instanceof Request diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index a272347a22..c40feedc20 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -22,7 +22,7 @@ import { exemptMcpEndpoint } from "../../auth/requestTimeout.js"; import { routeOAuthRequests } from "../../auth/routeOAuthRequests.js"; import { createNotificationHeadersFetch } from "./notificationHeadersFetch.js"; import { createSuppressNotificationStreamFetch } from "./suppressNotificationStreamFetch.js"; -import { ConnectionCookieJar, createCookieJarFetch } from "./cookieJarFetch.js"; +import { createCookieJarWrapper } from "./cookieJarFetch.js"; /** * Build the wire `headers` record from `settings.headers`, dropping rows with @@ -121,19 +121,22 @@ export function createTransportNode( // the real response and reads its body (bounded) before the interceptor // cancels it. const trackedBase = withTracking(withChallengeObserver(baseFetch)); - const fetchWithOptionalAuthIntercept = interceptAuthChallenges - ? createAuthChallengeInterceptFetch(trackedBase) - : trackedBase; // Opt-in cookie jar (#2936): one per transport, shared by every fetch the // transport makes, so an affinity cookie learned on any response pins the - // whole session. It sits above the tracker so the Network log records the + // whole session. It sits above the tracker, so the Network log records the // `Cookie` header actually sent (#2385) and the `Set-Cookie` the jar read — - // both redacted there. Off, the chains below are exactly what they were. - const cookieJar = - settings?.cookieJar === true ? new ConnectionCookieJar() : undefined; - const withCookieJar = (inner: typeof fetch, url: URL): typeof fetch => - cookieJar ? createCookieJarFetch(inner, url, cookieJar) : inner; + // both redacted there — and below the auth-challenge interceptor, so a + // 401/403 that the interceptor turns into a throw still updates the jar + // before the OAuth retry. Off, the chains below are exactly what they were. + const withCookieJar = + settings?.cookieJar === true && "url" in config + ? createCookieJarWrapper(new URL(config.url)) + : (inner: typeof fetch): typeof fetch => inner; + const jarredBase = withCookieJar(trackedBase); + const fetchWithOptionalAuthIntercept = interceptAuthChallenges + ? createAuthChallengeInterceptFetch(jarredBase) + : jarredBase; if (serverType === "stdio") { const stdioConfig = config as StdioServerConfig; @@ -173,7 +176,7 @@ export function createTransportNode( | typeof fetch | undefined; const trackedFetch = configuredSseFetch - ? withTracking(withChallengeObserver(configuredSseFetch)) + ? withCookieJar(withTracking(withChallengeObserver(configuredSseFetch))) : fetchWithOptionalAuthIntercept; const headers = headersFromSettings(settings); @@ -181,7 +184,7 @@ export function createTransportNode( const eventSourceInit: Record = { ...sseConfig.eventSourceInit, ...(headers && { headers }), - fetch: withCookieJar(trackedFetch, url), + fetch: trackedFetch, }; const requestInit: RequestInit = { @@ -193,7 +196,7 @@ export function createTransportNode( authProvider, eventSourceInit, requestInit, - fetch: withCookieJar(fetchWithOptionalAuthIntercept, url), + fetch: fetchWithOptionalAuthIntercept, }); return { transport }; @@ -211,12 +214,13 @@ export function createTransportNode( // Both wrappers sit above the tracker and the cookie jar. Header // stamping, so the tracker records the headers actually sent (#2385); - // stream suppression outermost of all, so a suppressed GET is answered before it reaches the tracker — - // it is never sent, and the Network log should not show a request the - // server never saw (#2317). They touch disjoint requests (notification - // POSTs vs. the endpoint's SSE GET), so their relative order is free. + // stream suppression outermost of all, so a suppressed GET is answered + // before it reaches the tracker — it is never sent, and the Network log + // should not show a request the server never saw (#2317). They touch + // disjoint requests (notification POSTs vs. the endpoint's SSE GET), so + // their relative order is free. const stampedFetch = createNotificationHeadersFetch( - withCookieJar(fetchWithOptionalAuthIntercept, url), + fetchWithOptionalAuthIntercept, ); const httpFetch = settings?.suppressNotificationStream === true From afc6371c8a74a3bb373b7544b9532fef95ead09e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 02:07:25 -0500 Subject: [PATCH 6/7] Forward only path and query in the affinity test proxy (#2936) CodeQL js/request-forgery: the proxy resolved the incoming request URL against the replica origin, which an absolute URL could override. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../web/src/test/integration/mcp/cookie-jar-affinity.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts index a6e1bce2d2..a0e4b29e21 100644 --- a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts +++ b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts @@ -94,7 +94,10 @@ async function startAffinityProxy( headers.set(name, Array.isArray(value) ? value.join(", ") : value); } const body = chunks.length > 0 ? Buffer.concat(chunks) : undefined; - fetch(new URL(req.url ?? "/", replicas[index]), { + // Only the path and query are forwarded, appended after the replica's + // fixed origin, so a request cannot steer the proxy to another host. + const pathAndQuery = (req.url ?? "/").replace(/^\/+/, ""); + fetch(`${replicas[index]}/${pathAndQuery}`, { method: req.method, headers, body, From c211313b4361f50c4ebecde5c8766a61a1408c5d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 12 Nov 2026 02:29:51 -0500 Subject: [PATCH 7/7] Build the affinity test proxy's upstream URL from an allowlist (#2936) CodeQL still traced the request path into the forwarded URL; the path is now picked from the fixture's two endpoints and only a validated sessionId is carried over. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../mcp/cookie-jar-affinity.test.ts | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts index a0e4b29e21..060cfed7d6 100644 --- a/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts +++ b/clients/web/src/test/integration/mcp/cookie-jar-affinity.test.ts @@ -63,6 +63,27 @@ async function pipeBody( } } +/** The only endpoints the fixture serves (`/mcp`, legacy `/sse`). */ +const ENDPOINT_PATHS = ["/mcp", "/sse"] as const; + +/** + * The replica URL for an incoming request. Built from the replica's fixed + * origin, a path picked from {@link ENDPOINT_PATHS}, and a validated legacy-SSE + * `sessionId`, so a request cannot steer the proxy anywhere else. + */ +function upstreamUrl(origin: string, requestUrl: string | undefined): URL { + const incoming = new URL(requestUrl ?? "/", "http://proxy.invalid"); + const path = + ENDPOINT_PATHS.find((candidate) => candidate === incoming.pathname) ?? + "/mcp"; + const target = new URL(path, origin); + const sessionId = incoming.searchParams.get("sessionId"); + if (sessionId && /^[\w-]+$/.test(sessionId)) { + target.searchParams.set("sessionId", sessionId); + } + return target; +} + interface AffinityProxy { url: string; /** The `cookie` header of every request the proxy received, in order. */ @@ -94,10 +115,7 @@ async function startAffinityProxy( headers.set(name, Array.isArray(value) ? value.join(", ") : value); } const body = chunks.length > 0 ? Buffer.concat(chunks) : undefined; - // Only the path and query are forwarded, appended after the replica's - // fixed origin, so a request cannot steer the proxy to another host. - const pathAndQuery = (req.url ?? "/").replace(/^\/+/, ""); - fetch(`${replicas[index]}/${pathAndQuery}`, { + fetch(upstreamUrl(replicas[index], req.url), { method: req.method, headers, body,