diff --git a/AGENTS.md b/AGENTS.md index e54e1346..7674834d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,6 +73,34 @@ v2/main/ │ │ # the advertised document rather than a │ │ # location derived from the MCP server │ │ # URL — #2071; +│ │ # discovery.ts the authorization-server +│ │ # URL POLICY — when protected-resource +│ │ # metadata names no authorization +│ │ # server, the MCP server URL stands in, +│ │ # and its PATH is load-bearing: the +│ │ # SDK's buildDiscoveryUrls derives the +│ │ # path-scoped well-known locations from +│ │ # that pathname, so reducing it to the +│ │ # origin can only ever probe the domain +│ │ # root. getAuthorizationServerUrlCandidates +│ │ # therefore yields the path-scoped URL +│ │ # FIRST and the bare origin second, and +│ │ # discoverAuthorizationServerMetadataForServer +│ │ # walks them — the second candidate is +│ │ # what keeps a server that merely lives +│ │ # under a path while publishing metadata +│ │ # at the root working, so do not drop it +│ │ # in favour of a straight swap. EVERY +│ │ # consumer of the fallback must walk: +│ │ # discoverScopes, the CIMD probe, and +│ │ # the CLI's refreshStoredAuthToken, +│ │ # which is why the walk is also exposed +│ │ # as ...FromCandidates(candidates, +│ │ # discover) — the CLI injects its own +│ │ # discovery function as a test seam and +│ │ # needs to know WHICH candidate +│ │ # answered, since that is the base its +│ │ # token request is made against — #2110; │ │ # scopes.ts SEP-2350 scope union, oauthUx.ts │ │ # shared copy, mcpAuth.ts force-reauthorization, │ │ # issuerBinding.ts SEP-2352 callback-leg failure diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 1b9b5768..3fb5c0b1 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -158,6 +158,97 @@ describe("refreshStoredAuthToken", () => { } }); + // #2110: with no stored metadata the MCP server URL stands in as the + // authorization server. A path-hosted server has two plausible answers, and + // both must keep working — the path-scoped URL for a server that publishes + // its metadata under its own path, the bare origin for one that merely lives + // under a path. The candidate that answers is also the base the token request + // is made against, so the walk has to report *which* one it was. + it("prefers the path-scoped authorization server for a path-hosted MCP server", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const pathMetadata = { + issuer: "https://api.example/mcp", + token_endpoint: "https://api.example/mcp/token", + }; + const discover = vi.fn().mockResolvedValue(pathMetadata); + + await refreshStoredAuthToken(SERVER, path, { refresh, discover }); + + expect(discover).toHaveBeenCalledTimes(1); + expect(discover).toHaveBeenCalledWith(new URL("https://api.example/mcp")); + const [authServerUrl, opts] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/mcp")); + expect(opts.metadata).toEqual(pathMetadata); + } finally { + rmSync(path, { force: true }); + } + }); + + it("falls back to the origin when a path-hosted server publishes metadata at the root", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const rootMetadata = { + issuer: "https://api.example", + token_endpoint: "https://api.example/token", + }; + const discover = vi + .fn() + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(rootMetadata); + + const token = await refreshStoredAuthToken(SERVER, path, { + refresh, + discover, + }); + + expect(token).toBe("refreshed-access-token"); + expect(discover).toHaveBeenNthCalledWith( + 2, + new URL("https://api.example/"), + ); + // The candidate that answered is the one the token request targets. + const [authServerUrl, opts] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/")); + expect(opts.metadata).toEqual(rootMetadata); + } finally { + rmSync(path, { force: true }); + } + }); + + it("keeps the path-scoped candidate as the token-request base when no candidate answers", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const discover = vi.fn().mockResolvedValue(undefined); + + await refreshStoredAuthToken(SERVER, path, { refresh, discover }); + + expect(discover).toHaveBeenCalledTimes(2); + const [authServerUrl] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/mcp")); + } finally { + rmSync(path, { force: true }); + } + }); + it("uses the distinct no_client_information code when client info is missing", async () => { const path = writeOAuthFixture({ [SERVER]: { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index c80cd1ab..af4bed16 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -47,7 +47,11 @@ import { serializeOAuthPersistBlob, type OAuthPersistSnapshot, } from "@inspector/core/auth/oauth-persist.js"; -import { getAuthorizationServerUrl } from "@inspector/core/auth/discovery.js"; +import { + discoverAuthorizationServerMetadataFromCandidates, + getAuthorizationServerUrl, + getAuthorizationServerUrlCandidates, +} from "@inspector/core/auth/discovery.js"; import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, @@ -347,11 +351,27 @@ export async function refreshStoredAuthToken( ); } - const authServerUrl = found.state.serverMetadata?.issuer - ? new URL(found.state.serverMetadata.issuer) - : getAuthorizationServerUrl(serverUrl); - const metadata = - found.state.serverMetadata ?? (await discover(authServerUrl)) ?? undefined; + // With stored metadata the issuer settles it. Without, the MCP server URL + // stands in as the authorization server — and a path-hosted server has two + // plausible answers, so walk them rather than committing to the path-scoped + // one: a server that merely lives under a path while publishing its metadata + // at the domain root must keep working (#2110). The candidate that *answered* + // becomes `authServerUrl`, since it is also the base the token request below + // is made against. + let authServerUrl: URL; + let metadata = found.state.serverMetadata ?? undefined; + if (found.state.serverMetadata?.issuer) { + authServerUrl = new URL(found.state.serverMetadata.issuer); + } else { + const discovered = await discoverAuthorizationServerMetadataFromCandidates( + getAuthorizationServerUrlCandidates(serverUrl), + discover, + ); + authServerUrl = + discovered?.authorizationServerUrl ?? + getAuthorizationServerUrl(serverUrl); + metadata = discovered?.metadata; + } let tokens: OAuthTokens; try { diff --git a/clients/web/src/test/core/mcp/remote/parseSSE.test.ts b/clients/web/src/test/core/mcp/remote/parseSSE.test.ts new file mode 100644 index 00000000..5c91e639 --- /dev/null +++ b/clients/web/src/test/core/mcp/remote/parseSSE.test.ts @@ -0,0 +1,86 @@ +/** + * `parseSSE` cross-chunk frame loss. + * + * The reproduction needs no browser and no network: `parseSSE` takes a reader, + * so a test supplies the chunk boundary directly instead of hoping a real + * transport happens to produce one. That is what makes this defect testable at + * all — on the wire the split is a function of payload size, TCP segmentation + * and any proxy in between, none of which a test can steer. + * + * The bug: `currentEvent` / `currentData` are declared INSIDE the read loop, so + * a frame whose `data:` line arrives in one chunk and whose terminating blank + * line arrives in the next is discarded. `buffer` already carries a partial + * *line* across reads; nothing carries a partial *frame*. + */ + +import { describe, expect, it } from "vitest"; +import { parseSSE } from "@inspector/core/mcp/remote/remoteClientTransport"; + +/** A reader that yields exactly the chunks given, in order. */ +function readerFrom(chunks: string[]): ReadableStreamDefaultReader { + const encoder = new TextEncoder(); + let i = 0; + return { + read: async () => + i < chunks.length + ? { done: false as const, value: encoder.encode(chunks[i++]!) } + : { done: true as const, value: undefined }, + } as ReadableStreamDefaultReader; +} + +async function collect(chunks: string[]) { + const out: { event: string; data: string }[] = []; + for await (const frame of parseSSE(readerFrom(chunks))) out.push(frame); + return out; +} + +const FRAME = 'event: message\ndata: {"id":1,"result":"payload"}\n\n'; + +describe("parseSSE chunk boundaries", () => { + it("delivers a frame that arrives whole (the case that hides the bug)", async () => { + // Chromium and Firefox happen to produce this shape at our payload sizes, + // which is why the defect below has never been observed in practice. + expect(await collect([FRAME])).toEqual([ + { event: "message", data: '{"id":1,"result":"payload"}' }, + ]); + }); + + it("delivers a frame split between its data line and its terminator", async () => { + // THE BUG. Chunk 1 ends after the data line's newline; chunk 2 carries only + // the blank line that closes the frame. Nothing is malformed — this is one + // valid frame, delivered in two reads. + const split = ['event: message\ndata: {"id":1,"result":"payload"}\n', "\n"]; + expect(split.join("")).toBe(FRAME); // same bytes as above, different reads + expect(await collect(split)).toEqual([ + { event: "message", data: '{"id":1,"result":"payload"}' }, + ]); + }); + + it("delivers a frame split mid-payload", async () => { + const split = ['event: message\ndata: {"id":1,"res', 'ult":"payload"}\n\n']; + expect(split.join("")).toBe(FRAME); + expect(await collect(split)).toEqual([ + { event: "message", data: '{"id":1,"result":"payload"}' }, + ]); + }); + + it("delivers every frame when a burst is split at each boundary", async () => { + // Bytes for three frames, re-cut so each split lands between a data line + // and its terminator — the shape a large response most plausibly produces. + const chunks: string[] = []; + for (const id of [1, 2, 3]) { + chunks.push(`event: message\ndata: {"id":${id}}\n`, "\n"); + } + expect(await collect(chunks)).toEqual([ + { event: "message", data: '{"id":1}' }, + { event: "message", data: '{"id":2}' }, + { event: "message", data: '{"id":3}' }, + ]); + }); + + it("flushes a final frame when the stream ends without a blank line", async () => { + expect(await collect(['event: message\ndata: {"id":9}\n'])).toEqual([ + { event: "message", data: '{"id":9}' }, + ]); + }); +}); diff --git a/clients/web/src/test/integration/auth/discovery.test.ts b/clients/web/src/test/integration/auth/discovery.test.ts index 0a7e8464..213e9677 100644 --- a/clients/web/src/test/integration/auth/discovery.test.ts +++ b/clients/web/src/test/integration/auth/discovery.test.ts @@ -1,7 +1,9 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { + discoverAuthorizationServerMetadataForServer, discoverScopes, getAuthorizationServerUrl, + getAuthorizationServerUrlCandidates, } from "@inspector/core/auth/discovery.js"; import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; @@ -335,3 +337,172 @@ describe("getAuthorizationServerUrl", () => { expect(captured?.message).toMatch(/Invalid MCP server URL: "" \(.+\)/); }); }); + +// #2110: an MCP server hosted under a path (Moodle, a WordPress plugin, any +// PHP endpoint) publishes its OAuth metadata under that same path. Reducing the +// fallback authorization-server URL to the origin threw that path away, so +// discovery could only ever probe the domain root and 404'd. +describe("path-hosted MCP servers (#2110)", () => { + const pathServerUrl = "https://misc.example.com/mod/minilesson/mcp.php"; + + describe("getAuthorizationServerUrl", () => { + it("keeps the server's path in the fallback URL", () => { + expect(getAuthorizationServerUrl(pathServerUrl)).toEqual( + new URL(pathServerUrl), + ); + }); + + it("strips query and fragment, which an RFC 8414 issuer never carries", () => { + expect( + getAuthorizationServerUrl(`${pathServerUrl}?session=abc#frag`), + ).toEqual(new URL(pathServerUrl)); + }); + }); + + describe("getAuthorizationServerUrlCandidates", () => { + it("tries the path-scoped URL before the bare origin", () => { + expect(getAuthorizationServerUrlCandidates(pathServerUrl)).toEqual([ + new URL(pathServerUrl), + new URL("https://misc.example.com/"), + ]); + }); + + it("offers a single candidate for a root-hosted server", () => { + expect( + getAuthorizationServerUrlCandidates("https://mcp.example.com"), + ).toEqual([new URL("https://mcp.example.com/")]); + }); + + it("does not probe past an authorization server named by resource metadata", () => { + const resourceMetadata: OAuthProtectedResourceMetadata = { + resource: pathServerUrl, + authorization_servers: ["https://auth.example.com/realms/r"], + }; + expect( + getAuthorizationServerUrlCandidates(pathServerUrl, resourceMetadata), + ).toEqual([new URL("https://auth.example.com/realms/r")]); + }); + }); + + describe("discoverAuthorizationServerMetadataForServer", () => { + const metadata = { + issuer: pathServerUrl, + authorization_endpoint: "https://misc.example.com/mod/authorize.php", + token_endpoint: "https://misc.example.com/mod/token.php", + response_types_supported: ["code"], + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns the path-scoped result without probing the origin", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue( + metadata, + ); + + await expect( + discoverAuthorizationServerMetadataForServer(pathServerUrl), + ).resolves.toEqual(metadata); + expect(discoverAuthorizationServerMetadata).toHaveBeenCalledTimes(1); + expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith( + new URL(pathServerUrl), + { fetchFn: undefined }, + ); + }); + + it("falls back to the origin when the path-scoped candidate has no metadata", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata) + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(metadata); + + await expect( + discoverAuthorizationServerMetadataForServer(pathServerUrl), + ).resolves.toEqual(metadata); + expect(discoverAuthorizationServerMetadata).toHaveBeenNthCalledWith( + 2, + new URL("https://misc.example.com/"), + { fetchFn: undefined }, + ); + }); + + it("survives a throwing candidate while another remains", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata) + .mockRejectedValueOnce(new Error("issuer mismatch")) + .mockResolvedValueOnce(metadata); + + await expect( + discoverAuthorizationServerMetadataForServer(pathServerUrl), + ).resolves.toEqual(metadata); + }); + + it("rethrows the first error when every candidate fails", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata) + .mockRejectedValueOnce(new Error("path-scoped failed")) + .mockRejectedValueOnce(new Error("origin failed")); + + await expect( + discoverAuthorizationServerMetadataForServer(pathServerUrl), + ).rejects.toThrow("path-scoped failed"); + }); + + it("resolves undefined when every candidate answers with no metadata", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue( + undefined, + ); + + await expect( + discoverAuthorizationServerMetadataForServer(pathServerUrl), + ).resolves.toBeUndefined(); + expect(discoverAuthorizationServerMetadata).toHaveBeenCalledTimes(2); + }); + + it("passes the caller's fetch through to every candidate", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue( + undefined, + ); + const fetchFn = vi.fn(); + + await discoverAuthorizationServerMetadataForServer( + pathServerUrl, + undefined, + fetchFn, + ); + + expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith( + new URL(pathServerUrl), + { fetchFn }, + ); + }); + }); + + it("discoverScopes reaches a path-hosted authorization server", async () => { + const { discoverAuthorizationServerMetadata } = + await import("@modelcontextprotocol/client"); + vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue({ + issuer: pathServerUrl, + authorization_endpoint: "https://misc.example.com/mod/authorize.php", + token_endpoint: "https://misc.example.com/mod/token.php", + response_types_supported: ["code"], + scopes_supported: ["read"], + }); + + await expect(discoverScopes(pathServerUrl)).resolves.toBe("read"); + expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith( + new URL(pathServerUrl), + { fetchFn: undefined }, + ); + }); +}); diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index d8ccc8bf..31614d1b 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -1,9 +1,6 @@ -import { - discoverAuthorizationServerMetadata, - discoverOAuthProtectedResourceMetadata, -} from "@modelcontextprotocol/client"; +import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; import type { OAuthClientInformation } from "@modelcontextprotocol/client"; -import { getAuthorizationServerUrl } from "./discovery.js"; +import { discoverAuthorizationServerMetadataForServer } from "./discovery.js"; import type { BaseOAuthClientProvider } from "./providers.js"; /** @@ -49,14 +46,14 @@ export async function ensureCimdClientRegistration(params: { resourceMetadata = undefined; } - const authServerUrl = getAuthorizationServerUrl( + // Walks the path-scoped authorization-server URL before the bare origin, so a + // server hosted under a path is probed where it actually publishes its + // metadata rather than only at the domain root (#2110). + const metadata = await discoverAuthorizationServerMetadataForServer( params.serverUrl, resourceMetadata, + params.fetchFn, ); - - const metadata = await discoverAuthorizationServerMetadata(authServerUrl, { - ...(params.fetchFn && { fetchFn: params.fetchFn }), - }); if (!metadata?.client_id_metadata_document_supported) return; const clientInformation: OAuthClientInformation = { diff --git a/core/auth/discovery.ts b/core/auth/discovery.ts index b64ecab9..789c1303 100644 --- a/core/auth/discovery.ts +++ b/core/auth/discovery.ts @@ -2,6 +2,38 @@ import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/clien import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; import { parseHttpUrl } from "./utils.js"; +type AuthorizationServerMetadata = Awaited< + ReturnType +>; + +/** + * The MCP server's own URL, used as the authorization server when RFC 9728 + * protected-resource metadata named none. + * + * The path is kept. Reducing it to the origin (`new URL("/", serverUrl)`) + * discards exactly the part that tells discovery where to look when the server + * is not hosted at the domain root: the SDK's `buildDiscoveryUrls` derives the + * path-scoped well-known locations — `/.well-known/oauth-authorization-server{path}` + * and `{path}/.well-known/openid-configuration` — from this URL's pathname, so + * an origin-only value can only ever probe the domain root (#2110). + * + * Query and fragment are stripped: an RFC 8414 issuer identifier carries + * neither, and the SDK compares the discovered `issuer` against this URL. + */ +function serverUrlAsAuthorizationServer(serverUrl: string): URL { + try { + const url = new URL(serverUrl); + url.search = ""; + url.hash = ""; + return url; + } catch (err) { + const detail = err instanceof Error ? err.message : String(err); + throw new Error(`Invalid MCP server URL: "${serverUrl}" (${detail})`, { + cause: err, + }); + } +} + /** * Returns the URL to use for OAuth authorization server metadata discovery. * Uses resource metadata's authorization_servers[0] when present, otherwise the MCP server URL. @@ -15,14 +47,87 @@ export function getAuthorizationServerUrl( if (first) { return parseHttpUrl(first, "protected resource authorization_servers[0]"); } - try { - return new URL("/", serverUrl); - } catch (err) { - const detail = err instanceof Error ? err.message : String(err); - throw new Error(`Invalid MCP server URL: "${serverUrl}" (${detail})`, { - cause: err, - }); + return serverUrlAsAuthorizationServer(serverUrl); +} + +/** + * The authorization-server URLs to try, in order. + * + * When protected-resource metadata names an authorization server there is + * exactly one candidate — the server told us where to look, and probing past it + * would be guessing. Otherwise the MCP server URL itself stands in as the + * authorization server, and a path-hosted server has two plausible answers: the + * full URL, whose path-scoped well-known locations are what a server like + * `https://example.com/mod/minilesson/mcp.php` publishes, and the bare origin, + * used by a server that merely *lives* under a path while publishing its + * metadata at the domain root. Trying the path-scoped form first fixes #2110 + * without regressing the root-hosted case, which is all the origin-only value + * ever served. + */ +export function getAuthorizationServerUrlCandidates( + serverUrl: string, + resourceMetadata?: OAuthProtectedResourceMetadata | null, +): URL[] { + const primary = getAuthorizationServerUrl(serverUrl, resourceMetadata); + const namedByMetadata = Boolean(resourceMetadata?.authorization_servers?.[0]); + if (namedByMetadata || primary.pathname === "/") return [primary]; + return [primary, new URL("/", primary)]; +} + +export interface DiscoveredAuthorizationServer { + /** The candidate that answered — not necessarily the first one tried. */ + authorizationServerUrl: URL; + metadata: NonNullable; +} + +/** + * Walks `candidates` until one answers, and reports **which** one did. + * + * A candidate that fails — a 5xx, or an RFC 8414 §3.3 issuer mismatch, both of + * which the SDK raises rather than returning `undefined` — is not fatal while + * another candidate remains. If every candidate fails, the first error is + * rethrown, so a caller still sees the original diagnosis rather than a bare + * `undefined`. + * + * `discover` is injected rather than closed over so a caller that already owns + * a discovery function — the CLI's `refreshStoredAuthToken`, which takes one as + * a test seam — can walk the same candidates without giving that seam up. + */ +export async function discoverAuthorizationServerMetadataFromCandidates( + candidates: URL[], + discover: (url: URL) => Promise, +): Promise { + let firstError: unknown; + let sawError = false; + for (const candidate of candidates) { + try { + const metadata = await discover(candidate); + if (metadata) return { authorizationServerUrl: candidate, metadata }; + } catch (err) { + if (!sawError) { + firstError = err; + sawError = true; + } + } } + if (sawError) throw firstError; + return undefined; +} + +/** + * Discovers authorization server metadata for an MCP server, walking the + * candidates above until one answers. + */ +export async function discoverAuthorizationServerMetadataForServer( + serverUrl: string, + resourceMetadata?: OAuthProtectedResourceMetadata | null, + fetchFn?: typeof fetch, +): Promise { + const found = await discoverAuthorizationServerMetadataFromCandidates( + getAuthorizationServerUrlCandidates(serverUrl, resourceMetadata), + (candidate) => discoverAuthorizationServerMetadata(candidate, { fetchFn }), + ); + return found?.metadata; } /** @@ -38,13 +143,11 @@ export const discoverScopes = async ( fetchFn?: typeof fetch, ): Promise => { try { - const authServerUrl = getAuthorizationServerUrl( + const metadata = await discoverAuthorizationServerMetadataForServer( serverUrl, resourceMetadata, - ); - const metadata = await discoverAuthorizationServerMetadata(authServerUrl, { fetchFn, - }); + ); // Prefer resource metadata scopes, but fall back to OAuth metadata if empty const resourceScopes = resourceMetadata?.scopes_supported; diff --git a/core/mcp/remote/remoteClientTransport.ts b/core/mcp/remote/remoteClientTransport.ts index a9dfed78..5ddbe612 100644 --- a/core/mcp/remote/remoteClientTransport.ts +++ b/core/mcp/remote/remoteClientTransport.ts @@ -165,14 +165,55 @@ function legacySessionId(json: RemoteConnectResponse): string | undefined { } /** - * Parse SSE stream from a ReadableStream. - * Yields { event, data } for each SSE message. + * Parse an SSE stream from a ReadableStream. + * Yields `{ event, data }` for each complete SSE message. + * + * ⚠️ **The in-progress event must live OUTSIDE the read loop.** A `read()` + * returns an arbitrary slice of bytes, not a whole frame, so a frame's `data:` + * line can arrive in one chunk and the blank line terminating it in the next. + * Declaring `currentEvent` / `currentData` inside the loop — which this did + * until #2134 — discards the accumulated payload at that boundary: the frame is + * never yielded, the JSON-RPC response it carried never settles, and the caller + * hangs forever with no error on any channel. `buffer` already carries a partial + * *line* across reads; these carry the partial *frame*. + * + * It survived because Chromium and Firefox happen to deliver whole frames at the + * payload sizes this transport sees. That is luck rather than a guarantee — + * chunk boundaries are a function of payload size, TCP segmentation and any + * intermediary — and the exposure grows with payload size, so a large + * `resources/read` is the most likely first victim. + * + * Exported for `clients/web/src/test/core/mcp/remote/parseSSE.test.ts`, which is + * the only way to reach the defect deterministically: driving a real transport + * cannot steer where the chunk boundary lands, but a test that supplies the + * reader can put it exactly on the terminator. */ -async function* parseSSE( +export async function* parseSSE( reader: ReadableStreamDefaultReader, ): AsyncGenerator<{ event: string; data: string }> { const decoder = new TextDecoder(); let buffer = ""; + // Carried ACROSS reads, not per chunk — see the header. + let currentEvent = "message"; + let currentData: string[] = []; + + const consume = (line: string): { event: string; data: string } | null => { + if (line.startsWith("event:")) { + currentEvent = line.slice(6).trim(); + } else if (line.startsWith("data:")) { + currentData.push(line.slice(5).trimStart()); + } else if (line === "") { + const complete = + currentData.length > 0 + ? { event: currentEvent, data: currentData.join("\n") } + : null; + currentEvent = "message"; + currentData = []; + return complete; + } + // Anything else (a `:` keep-alive comment, `id:`, `retry:`) is ignored. + return null; + }; while (true) { const { done, value } = await reader.read(); @@ -183,36 +224,21 @@ async function* parseSSE( /* v8 ignore next -- String.split always returns a non-empty array, so pop() is never undefined; the ?? "" fallback is unreachable. */ buffer = lines.pop() ?? ""; - let currentEvent = "message"; - let currentData: string[] = []; - for (const line of lines) { - if (line.startsWith("event:")) { - currentEvent = line.slice(6).trim(); - } else if (line.startsWith("data:")) { - currentData.push(line.slice(5).trimStart()); - } else if (line === "") { - if (currentData.length > 0) { - yield { event: currentEvent, data: currentData.join("\n") }; - } - currentEvent = "message"; - currentData = []; - } + const complete = consume(line); + if (complete) yield complete; } } - if (buffer.trim()) { - const lines = buffer.split("\n"); - let currentEvent = "message"; - const currentData: string[] = []; - for (const line of lines) { - if (line.startsWith("event:")) currentEvent = line.slice(6).trim(); - else if (line.startsWith("data:")) - currentData.push(line.slice(5).trimStart()); - } - if (currentData.length > 0) { - yield { event: currentEvent, data: currentData.join("\n") }; - } + // Stream ended: flush the trailing partial line, then anything still held. A + // server that closes without a final blank line has still delivered a complete + // frame, and dropping it would lose the last message of every such stream. + if (buffer) { + const complete = consume(buffer); + if (complete) yield complete; + } + if (currentData.length > 0) { + yield { event: currentEvent, data: currentData.join("\n") }; } }