diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 7c80dbab..b2a786c6 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -875,7 +875,7 @@ export function ServerSettingsForm({ { await expect(response.text()).resolves.toBe("not json at all"); }); + // `application/x-ndjson` satisfies a naive `includes("json")` test, and the + // streamable-HTTP transport serves its long-lived server-push channel that way + // (see `isLongLivedStreamResponse`). Awaiting `.json()` on a clone of an + // unbounded stream never resolves, so the MCP connection would hang for as + // long as an override was configured. Regression test for PR #2037 review. + it("does not read an unbounded NDJSON stream", async () => { + // A body that never closes: if the wrapper reads it, this test times out. + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('{"jsonrpc":"2.0"}\n')); + // Deliberately never closed. + }, + }); + const original = new Response(stream, { + headers: { "content-type": "application/x-ndjson" }, + }); + const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ + tokenUrl: STAGING_TOKEN, + })); + await expect(wrapped("https://mcp.example.com/mcp")).resolves.toBe( + original, + ); + await original.body?.cancel(); + }); + + it("ignores content-type parameters when deciding a body is JSON", async () => { + const wrapped = withOAuthEndpointOverrides( + passThrough( + new Response(JSON.stringify(METADATA), { + headers: { "content-type": "application/json; charset=utf-8" }, + }), + ), + () => ({ tokenUrl: STAGING_TOKEN }), + ); + const response = await wrapped("https://as.example.com/x"); + await expect(response.json()).resolves.toMatchObject({ + token_endpoint: STAGING_TOKEN, + }); + }); + // Fetch forbids a body on 204/205, so rebuilding one would throw. A JSON // content-type on an empty response is unusual but legal, and it must not // take down an unrelated transport request. diff --git a/core/auth/endpointOverrides.ts b/core/auth/endpointOverrides.ts index 64534294..e0e01ae6 100644 --- a/core/auth/endpointOverrides.ts +++ b/core/auth/endpointOverrides.ts @@ -22,7 +22,7 @@ * sees both is the `fetchFn` the SDK uses for discovery, so the override is * applied to the metadata document in flight. * - * Three consequences worth knowing: + * Four consequences worth knowing: * * - **A metadata document is required.** When discovery returns nothing, the SDK * falls back to `/authorize` and `/token` on the authorization server's origin @@ -36,6 +36,17 @@ * suppresses the overrides when the server is enterprise-managed. This mirrors * `redirectToExternalAuthorization` skipping the custom authorization * parameters (#2018). + * - **They redirect endpoints, they do not re-point the issuer.** `issuer` is + * left exactly as discovery returned it, deliberately: it is the authorization + * server's *identity*, and it is what RFC 9207 / SEP-2352 check the callback's + * `iss` against to defend against an authorization-server mix-up. So these + * overrides fit alternate endpoints of the same logical issuer (a staging + * deployment fronting the same issuer, a local proxy). Aim one at an + * authorization server that advertises a *different* issuer and the callback + * is rejected as an issuer mismatch before the code is redeemed — which is the + * mix-up defense working. Suppressing that check to make the override "work" + * would trade a real security property for a debugging convenience, so the + * limit is documented rather than removed. * - **The Network tab shows the patched document.** The wrapper is applied to the * base fetch, inside the request tracker, so what the tab renders is the * metadata as the flow consumed it. That is the useful reading: it explains why @@ -185,6 +196,27 @@ export function applyOAuthEndpointOverrides( return patched; } +/** + * Whether a `content-type` names a **whole JSON document** — something that can + * be read to completion. + * + * This must be an exact media-type match, not a substring test for "json". The + * streamable-HTTP transport's long-lived server-push channel can be served as + * `application/x-ndjson` (see `isLongLivedStreamResponse` in + * `core/mcp/fetchTracking.ts`), which is an unbounded stream: awaiting `.json()` + * on a clone of it would never resolve, and since this wrapper awaits before + * returning, the MCP connection would hang for as long as an override is + * configured. Metadata documents are plain `application/json`, so the narrow + * test loses nothing. The `+json` structured suffix is accepted for the same + * reason it exists — it names a concrete document, not a stream. + */ +function isJsonDocumentResponse(contentType: string | null): boolean { + if (!contentType) return false; + // Strip parameters (`; charset=utf-8`) and compare the media type itself. + const mediaType = contentType.split(";")[0].trim().toLowerCase(); + return mediaType === "application/json" || mediaType.endsWith("+json"); +} + /** Rebuild a response around a replacement body, preserving status/headers. */ function responseWithBody(response: Response, body: string): Response { const headers = new Headers(response.headers); @@ -237,8 +269,8 @@ export function withOAuthEndpointOverrides( const overrides = resolveOverrides(); if (!overrides) return response; if (!response.ok) return response; - const contentType = response.headers.get("content-type") ?? ""; - if (!contentType.toLowerCase().includes("json")) return response; + if (!isJsonDocumentResponse(response.headers.get("content-type"))) + return response; // Read through a clone so every path that does NOT patch can return the // caller's own `Response`, untouched. Rebuilding unconditionally would drop diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index c317adfb..ac105f3a 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -181,6 +181,8 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w `oauth.authorizationUrl` and `oauth.tokenUrl` override the `authorization_endpoint` and `token_endpoint` that authorization-server metadata discovery resolved. The Inspector deliberately has no such fields by default — it resolves both from the AS's metadata document, exactly as a real MCP host does — but a server under development often advertises its *production* authorization server while you want to hit staging. Set either (they are independent) to an absolute `http(s)` URL and it replaces what the metadata returned, for both the authorization request and the token request; leave blank to use discovery. A malformed value is flagged inline in the form and dropped with a warning at connect time rather than failing the connection. Edit them in Server Settings → Authorization ("Authorization URL override" / "Token URL override"). +They redirect the **endpoints**, not the authorization server's identity: `issuer` is left exactly as discovery returned it, because that is what RFC 9207 / SEP-2352 validate the callback's `iss` against to defend against an authorization-server mix-up. So they fit alternate endpoints of the *same* logical issuer — a staging deployment fronting the same issuer, a local proxy. Point one at an authorization server advertising a **different** issuer and the callback is rejected as an issuer mismatch before the code is redeemed; that is the mix-up defense working, and it is not disabled to make the override succeed. + They are **not applied under enterprise-managed authorization** (`oauth.enterpriseManaged`), for the same reason `oauth.authorizationParams` isn't: that flow authorizes against the enterprise IdP — a different authorization server, whose OIDC discovery would otherwise be rewritten too, pointing the IdP login (or the IdP code exchange) at the resource server's authorization server. > Because the override is applied to the *discovered metadata document*, a server whose authorization server publishes no metadata at all is unaffected — there the SDK falls back to `/authorize` and `/token` on the AS origin, and there is nothing to override.