From 6cacfa3ec5caf99802c51b3c7df6efd16537c182 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 23:59:37 -0400 Subject: [PATCH 1/4] fix(core): carry the in-progress SSE frame across chunk boundaries (#2134) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseSSE declared currentEvent/currentData inside the read loop, so an SSE frame whose data: line arrived in one chunk and whose terminating blank line arrived in the next was discarded. buffer already carried a partial LINE across reads; nothing carried a partial FRAME. Because this is the web client's transport, the dropped frame is a JSON-RPC response that never settles: the request hangs forever with no rejection, no console message, and nothing in the Network tab. The payload does not arrive corrupted, it does not arrive at all, which is why it presents as a hang rather than an error. It has not bitten us because Chromium and Firefox happen to deliver whole frames at the payload sizes this transport currently sees. That is luck, not a guarantee: chunk boundaries depend on payload size, TCP segmentation, and any intermediary. The exposure grows with payload size, so a large resources/read is the most likely first victim. The fix hoists both out of the loop and adds an end-of-stream flush — 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. parseSSE is now exported, solely so the test can reach it. That is the only way to hit this deterministically: driving a real transport cannot steer where a chunk boundary lands, but a test that supplies the reader can put it exactly on the terminator. The test keeps the two PASSING cases (whole frame, split mid-payload) alongside the three that failed, because they are what make the diagnosis precise. This is not "SSE parsing is broken" — mid-payload splits are handled correctly today, and only the frame-terminator boundary loses data. Verified against the unfixed code: 3 of 5 fail, all 5 pass after. Signed-off-by: cliffhall --- .../src/test/core/mcp/remote/parseSSE.test.ts | 89 +++++++++++++++++++ core/mcp/remote/remoteClientTransport.ts | 84 +++++++++++------ 2 files changed, 144 insertions(+), 29 deletions(-) create mode 100644 clients/web/src/test/core/mcp/remote/parseSSE.test.ts 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..b0d95f42 --- /dev/null +++ b/clients/web/src/test/core/mcp/remote/parseSSE.test.ts @@ -0,0 +1,89 @@ +/** + * `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/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") }; } } From afa0ba6eeba070309188cf958abefa51fa243f17 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 00:03:40 -0400 Subject: [PATCH 2/4] fix(oauth): keep the server's path in the fallback authorization-server URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When RFC 9728 protected-resource metadata names no authorization server, the MCP server URL stands in as one. `getAuthorizationServerUrl` built that with `new URL("/", serverUrl)`, which discards the path — exactly the part discovery needs when the server is not hosted at the domain root. The SDK's `buildDiscoveryUrls` derives the path-scoped well-known locations from that pathname, so an origin-only value could only ever probe the domain root, which 404s for a server like `https://example.com/mod/minilesson/mcp.php`. Keep the path (and strip query/fragment, which an RFC 8414 issuer never carries). A straight swap would regress the case the origin-only value did serve — a server that merely lives under a path while publishing its metadata at the root — so the URL becomes the first of two candidates: `getAuthorizationServerUrlCandidates` yields the path-scoped form then the bare origin, and `discoverAuthorizationServerMetadataForServer` walks them, treating a candidate's failure as non-fatal while another remains and rethrowing the first error only when all of them fail. `discoverScopes` and the CIMD pre-registration probe both go through the walk. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- AGENTS.md | 18 ++ .../test/integration/auth/discovery.test.ts | 171 ++++++++++++++++++ core/auth/cimd.ts | 17 +- core/auth/discovery.ts | 107 +++++++++-- 4 files changed, 292 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 936af5d8..46fb431b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,6 +73,24 @@ 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 — #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/web/src/test/integration/auth/discovery.test.ts b/clients/web/src/test/integration/auth/discovery.test.ts index 0a7e8464..cfe58bab 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 as unknown as typeof fetch, + ); + + 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..23f5fed4 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,69 @@ 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)]; +} + +/** + * Discovers authorization server metadata for an MCP server, walking the + * candidates above until one answers. + * + * 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`. + */ +export async function discoverAuthorizationServerMetadataForServer( + serverUrl: string, + resourceMetadata?: OAuthProtectedResourceMetadata | null, + fetchFn?: typeof fetch, +): Promise { + const candidates = getAuthorizationServerUrlCandidates( + serverUrl, + resourceMetadata, + ); + let firstError: unknown; + let sawError = false; + for (const candidate of candidates) { + try { + const metadata = await discoverAuthorizationServerMetadata(candidate, { + fetchFn, + }); + if (metadata) return metadata; + } catch (err) { + if (!sawError) { + firstError = err; + sawError = true; + } + } } + if (sawError) throw firstError; + return undefined; } /** @@ -38,13 +125,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; From 99f2a20eb0396cc70c8fddfbbe713c8c316b35a1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 00:23:55 -0400 Subject: [PATCH 3/4] style: prettier-format the parseSSE test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by CI's clients/web format:check, not by my local run — I had formatted core/ and scripts/ but not the web client's src, which is where the test lives. Signed-off-by: cliffhall --- clients/web/src/test/core/mcp/remote/parseSSE.test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/clients/web/src/test/core/mcp/remote/parseSSE.test.ts b/clients/web/src/test/core/mcp/remote/parseSSE.test.ts index b0d95f42..5c91e639 100644 --- a/clients/web/src/test/core/mcp/remote/parseSSE.test.ts +++ b/clients/web/src/test/core/mcp/remote/parseSSE.test.ts @@ -49,10 +49,7 @@ describe("parseSSE chunk boundaries", () => { // 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", - ]; + 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"}' }, From 2bb0b6245c52f22ce32ac52df1eaa87acde89ebc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 00:34:27 -0400 Subject: [PATCH 4/4] fix(oauth): walk the candidates in the CLI refresh path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1. `refreshStoredAuthToken` consumes the same fallback and made a single SDK discovery call, so preserving the path there — without the walk the other two consumers got — would have regressed exactly the case the walk exists to keep: a path-hosted MCP server whose authorization metadata is published at the domain root. It injects its own discovery function as a test seam, so the walk is now also exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates, discover)`, which takes that function and reports *which* candidate answered — the CLI needs the winning URL, since it is the base the token request is made against, not just the metadata. Also drops the `as unknown as typeof fetch` from the new test in favour of `vi.fn()`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- AGENTS.md | 12 ++- clients/cli/__tests__/stored-auth.test.ts | 91 +++++++++++++++++++ clients/cli/src/cli.ts | 32 +++++-- .../test/integration/auth/discovery.test.ts | 4 +- core/auth/discovery.ts | 48 +++++++--- 5 files changed, 163 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 46fb431b..e854e9de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,17 @@ v2/main/ │ │ # 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 — #2110; +│ │ # 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/integration/auth/discovery.test.ts b/clients/web/src/test/integration/auth/discovery.test.ts index cfe58bab..213e9677 100644 --- a/clients/web/src/test/integration/auth/discovery.test.ts +++ b/clients/web/src/test/integration/auth/discovery.test.ts @@ -473,12 +473,12 @@ describe("path-hosted MCP servers (#2110)", () => { vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue( undefined, ); - const fetchFn = vi.fn(); + const fetchFn = vi.fn(); await discoverAuthorizationServerMetadataForServer( pathServerUrl, undefined, - fetchFn as unknown as typeof fetch, + fetchFn, ); expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith( diff --git a/core/auth/discovery.ts b/core/auth/discovery.ts index 23f5fed4..789c1303 100644 --- a/core/auth/discovery.ts +++ b/core/auth/discovery.ts @@ -74,33 +74,35 @@ export function getAuthorizationServerUrlCandidates( return [primary, new URL("/", primary)]; } +export interface DiscoveredAuthorizationServer { + /** The candidate that answered — not necessarily the first one tried. */ + authorizationServerUrl: URL; + metadata: NonNullable; +} + /** - * Discovers authorization server metadata for an MCP server, walking the - * candidates above until one answers. + * 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 discoverAuthorizationServerMetadataForServer( - serverUrl: string, - resourceMetadata?: OAuthProtectedResourceMetadata | null, - fetchFn?: typeof fetch, -): Promise { - const candidates = getAuthorizationServerUrlCandidates( - serverUrl, - resourceMetadata, - ); +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 discoverAuthorizationServerMetadata(candidate, { - fetchFn, - }); - if (metadata) return metadata; + const metadata = await discover(candidate); + if (metadata) return { authorizationServerUrl: candidate, metadata }; } catch (err) { if (!sawError) { firstError = err; @@ -112,6 +114,22 @@ export async function discoverAuthorizationServerMetadataForServer( 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; +} + /** * Discovers OAuth scopes from server metadata, with preference for resource metadata scopes * @param serverUrl - The MCP server URL