inspector/core/auth/oidcDiscoveryCompat.ts
cliffhall b4c02865e4 fix: do not await the release on a cancellation path (#2319 round 25)
`ReadableStream.cancel()` adopts the underlying source's cancel promise,
which is permitted never to settle — so awaiting `releaseBody` while
propagating a cancellation can hang the very thing it was meant to end.
The five exceptional exits in the compat probe loop now start the
release without waiting on it: the release is a courtesy to the
connection pool, and the caller's abort or timeout must reach it
regardless. The two non-exceptional awaits are unchanged.

Same class as the tee teardown in round 24, one file over.

Also moves the `redactUrlQuery` import above
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS's JSDoc, which it had split from the
constant — so hover and API tooling see the doc again.

Verified by mutation: awaiting the release makes the new test time out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-09-11 08:33:18 -04:00

415 lines
19 KiB
TypeScript

/**
* Compatibility shim for a plain OAuth 2.0 authorization server that publishes
* its RFC 8414 metadata at the OpenID Connect well-known path (#2172).
*
* ## The upstream defect
*
* Filed as modelcontextprotocol/typescript-sdk#2733.
*
* `discoverAuthorizationServerMetadata` in `@modelcontextprotocol/client@2.0.0`
* tries a fixed list of well-known URLs (`buildDiscoveryUrls`) and picks the
* validation schema from the **filename that resolved**, not from the document
* that came back:
*
* ```js
* const parsed = type === "oauth"
* ? OAuthMetadataSchema.parse(body)
* : OpenIdProviderDiscoveryMetadataSchema.parse(body);
* ```
*
* `type` is `"oidc"` for every `openid-configuration` candidate, so a document
* served there is required to carry `jwks_uri`, `subject_types_supported` and
* `id_token_signing_alg_values_supported` — three fields OpenID Connect
* Discovery 1.0 requires and RFC 8414 does not. A plain OAuth 2.0 authorization
* server — no ID tokens, no JWKS, no `sub` — that publishes at that path is
* therefore rejected with a `ZodError`, and because the parse **throws** rather
* than continuing the loop, discovery aborts outright: the connection fails and
* no later candidate is tried.
*
* RFC 8414 §5 explicitly anticipates this deployment. `openid-configuration` is
* a permitted location for general OAuth authorization server metadata, and the
* OIDC-only fields are not part of the RFC 8414 document. So the server in
* #2172 is conforming and the client is wrong.
*
* ## What this wrapper does
*
* It never fabricates a field. It changes *which candidate URL* the document is
* handed back on, so the SDK validates it under the schema that actually
* describes it:
*
* 1. It watches only for the RFC 8414 candidate,
* `/.well-known/oauth-authorization-server[<path>]`, and only when that
* request came back with a status the SDK treats as "try the next
* candidate" (4xx, or 502).
* 2. It then fetches the OIDC candidates the SDK would try next — derived from
* the request URL by the same rule `buildDiscoveryUrls` uses, which
* `oidcDiscoveryCandidates` mirrors and a test pins against the SDK's own
* exported function so the two cannot drift.
* 3. If one of them returns a document that satisfies `OAuthMetadataSchema` but
* **fails** `OpenIdProviderDiscoveryMetadataSchema` — i.e. it is RFC 8414
* metadata and is not an OpenID provider document — that body is returned as
* the response to the RFC 8414 request. The SDK parses it with
* `OAuthMetadataSchema`, which is the correct schema for it, and discovery
* succeeds.
*
* A document that *does* satisfy the OIDC schema is left alone: the original
* response is returned unchanged and the SDK proceeds to its own OIDC candidate
* as usual. That costs one duplicate request on a genuine OIDC server, which is
* the price of not changing behavior for the case that already works.
*
* ## What it deliberately does not do
*
* - **It does not weaken issuer validation.** The substituted document is the
* one the server actually published, `issuer` included, so the SDK's RFC 8414
* §3.3 issuer-echo check runs on it exactly as before. A document whose
* `issuer` does not match is still rejected.
* - **It does not invent `jwks_uri` (or any other field).** Back-filling the
* three OIDC-required fields would also make the parse succeed, but the
* Inspector would then be displaying, in the Auth and Network tabs, a
* metadata document the server never published — which is the one thing a
* debugging tool must not do.
* - **It does not present the substitution as the server's own answer.** It is
* installed on `InspectorClient`'s *base* fetch — below both fetch trackers,
* the same seam `withOAuthEndpointOverrides` uses — because that is the only
* place that also covers the discovery the SDK runs from **inside the
* transport**, which is handed the base fetch directly and whose tracker is
* built inside the transport where nothing here can reach above it
* (Copilot). One seam therefore covers every path, at the cost that a
* captured entry shows the substituted document rather than the real 404 —
* so the substituted response carries {@link COMPAT_SOURCE_HEADER} naming
* the URL the body actually came from, and the warning below says the same
* thing on the console. The probe itself is issued through the wrapped fetch
* and so is not separately tracked.
*
* ## Removing this
*
* This is a workaround for an upstream bug, not a feature. When
* modelcontextprotocol/typescript-sdk#2733 lands — the SDK selecting the schema
* from the document rather than from the filename, or treating a schema failure
* as a candidate to skip rather than as fatal — delete this module and its
* wiring in `core/mcp/inspectorClient.ts`.
*/
import { callerSignalOf, OAuthRequestTimeoutError } from "./requestTimeout.js";
import {
OAuthMetadataSchema,
OpenIdProviderDiscoveryMetadataSchema,
} from "@modelcontextprotocol/core";
/** The RFC 8414 well-known path, which every OAuth-typed candidate starts with. */
const RFC8414_WELL_KNOWN = "/.well-known/oauth-authorization-server";
/** The OpenID Connect Discovery well-known path. */
const OIDC_WELL_KNOWN = "/.well-known/openid-configuration";
/**
* Response header stamped on a substituted document, naming the URL the body
* was actually fetched from. Inspector-private (`x-inspector-`), never sent on
* a request — it exists so a captured Network entry is self-describing rather
* than appearing to be a 200 from the RFC 8414 path.
*/
export const COMPAT_SOURCE_HEADER = "x-inspector-oauth-metadata-source";
/**
* The OIDC discovery candidates the SDK would try after the RFC 8414 candidate
* `rfc8414Url` failed, in the SDK's own order.
*
* Derived from the RFC 8414 URL rather than from the authorization-server URL
* because that is all a `fetch` wrapper sees. The two are equivalent: the SDK
* builds the RFC 8414 candidate as
* `${origin}/.well-known/oauth-authorization-server${path}` with the
* authorization server's (trailing-slash-stripped) pathname as `path`, so
* recovering `path` recovers everything `buildDiscoveryUrls` keyed off.
*
* Returns an empty array when the URL is not an RFC 8414 candidate at all.
*/
export function oidcDiscoveryCandidates(rfc8414Url: string): string[] {
let url: URL;
try {
url = new URL(rfc8414Url);
} catch {
return [];
}
if (!url.pathname.startsWith(RFC8414_WELL_KNOWN)) return [];
const path = url.pathname.slice(RFC8414_WELL_KNOWN.length);
// A bare prefix match would also claim `/.well-known/oauth-authorization-server-backup`
// — a path this wrapper has no business rewriting, and one whose failed
// response it would replace with a document fetched from somewhere else
// entirely (Copilot). The SDK appends the authorization server's pathname,
// which always begins with `/`, so requiring the exact path or a `/`
// boundary is precisely the set it can emit.
if (path !== "" && !path.startsWith("/")) return [];
// The authorization server had no path, so the SDK emitted a single OIDC
// candidate at the origin.
if (path === "") return [new URL(OIDC_WELL_KNOWN, url.origin).href];
// A path-suffixed RFC 8414 candidate always has two OIDC siblings: the
// path-suffixed form and the path-prefixed ("appended") form.
return [
new URL(`${OIDC_WELL_KNOWN}${path}`, url.origin).href,
new URL(`${path}${OIDC_WELL_KNOWN}`, url.origin).href,
];
}
/**
* Whether a status makes the SDK move on to the next discovery candidate.
*
* Mirrors `discoverAuthorizationServerMetadata`: a 4xx or a 502 continues the
* loop, anything else throws. Probing on a status the SDK will not walk past
* would be wasted work — the flow is already over.
*/
function continuesDiscovery(status: number): boolean {
return (status >= 400 && status < 500) || status === 502;
}
/**
* Discard a response body nobody is going to read, so the connection under it
* is returned to the pool rather than held open. Best-effort: a body already
* consumed, locked, or absent is not an error here.
*/
async function releaseBody(response: Response): Promise<void> {
await response.body?.cancel().catch(() => {});
}
/**
* The same release, started but not waited on.
*
* ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel promise,
* which is permitted never to settle — so awaiting it on a path that is
* *propagating a cancellation* can hang the very thing that was meant to end
* (Copilot). Every exceptional exit below uses this: the release is a courtesy
* to the connection pool, and the caller's abort or timeout must reach it
* regardless. The `void` is the documented case where the callee owns its
* failures — `releaseBody` swallows its own — and the caller genuinely cannot
* await.
*/
function releaseBodyDetached(response: Response): void {
void releaseBody(response);
}
/**
* Whether a parsed body is RFC 8414 authorization-server metadata that is *not*
* a valid OpenID provider document — the exact shape the upstream schema
* selection rejects.
*
* Both halves matter. Without the first, any JSON body would be substituted;
* without the second, a genuine OIDC document would be diverted onto the RFC
* 8414 path and silently change behavior for servers that work today.
*/
export function isRfc8414OnlyMetadata(value: unknown): boolean {
if (!OAuthMetadataSchema.safeParse(value).success) return false;
return !OpenIdProviderDiscoveryMetadataSchema.safeParse(value).success;
}
/** The request URL a `fetch` call was made with, in any of its three forms. */
function requestUrlOf(input: RequestInfo | URL): string {
if (typeof input === "string") return input;
if (input instanceof URL) return input.href;
return input.url;
}
/**
* The headers the failed discovery request carried, so the probe is answered
* the same way the SDK's own request would have been (it sends
* `MCP-Protocol-Version` and `Accept`). Copied from the request rather than
* reconstructed, so a header the SDK adds later is carried without this module
* needing to know about it.
*/
function discoveryHeaders(
input: RequestInfo | URL,
init: RequestInit | undefined,
): Headers {
if (init?.headers) return new Headers(init.headers);
if (typeof input !== "string" && !(input instanceof URL)) {
return new Headers(input.headers);
}
return new Headers();
}
/**
* The effective HTTP method of a `fetch` call, normalized. `fetch` defaults to
* `GET` and treats the method case-insensitively.
*/
function requestMethodOf(
input: RequestInfo | URL,
init: RequestInit | undefined,
): string {
if (init?.method) return init.method.toUpperCase();
if (typeof input !== "string" && !(input instanceof URL)) {
return input.method.toUpperCase();
}
return "GET";
}
/**
* Wrap a `fetch` so a plain OAuth 2.0 authorization server publishing RFC 8414
* metadata at `/.well-known/openid-configuration` is discoverable.
*
* Inert for every request that is not a *failed* RFC 8414 discovery request, so
* the overwhelmingly common case costs one status check and one string
* comparison.
*/
export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
return async (input, init) => {
const response = await fetchFn(input, init);
if (response.ok) return response;
if (!continuesDiscovery(response.status)) return response;
// The URL shape alone does not prove this was a discovery request: nothing
// stops an authorization or token endpoint from living under
// `/.well-known/oauth-authorization-server/…`, and a `POST` there returning
// an ordinary OAuth `400 invalid_grant` must reach its caller intact rather
// than being replaced with a metadata document (Copilot). Metadata
// discovery is a `GET`, so anything else is not ours.
if (requestMethodOf(input, init) !== "GET") return response;
const candidates = oidcDiscoveryCandidates(requestUrlOf(input));
if (candidates.length === 0) return response;
const headers = discoveryHeaders(input, init);
// The probe is a request the caller never made, issued on its behalf — so
// it has to be cancellable by the caller too. Without the signal, aborting
// after the initial 404 left the probe running to its own deadline and the
// caller was handed a timeout in place of its own abort (Copilot). Same
// precedence rule as everywhere else, hence the shared helper rather than a
// second copy of it.
const callerSignal = callerSignalOf(input, init);
// The loop advances to the next candidate only where the SDK's own loop
// would. Anywhere else it hands the original response back and lets
// discovery run its normal course, because *promoting a later candidate
// over an earlier one the SDK would have stopped at* would turn a failure
// into a success — a much worse defect than the one being worked around
// (Copilot). Concretely: a 500 on the first OIDC candidate is an outage the
// SDK surfaces, a malformed JSON body there is terminal for it, and a
// network error propagates outside the browser's CORS case. In each of
// those, returning `response` leaves the SDK to make the same request and
// reach the same verdict it always would.
for (const candidate of candidates) {
// Don't issue a probe the caller has already given up on. A real `fetch`
// rejects immediately on a pre-aborted signal, so this is mostly about
// not sending the request at all — the same reasoning as the
// short-circuit in `withOAuthRequestTimeout`.
//
// Every exceptional exit below releases `response` first. On a normal
// return it is the value the caller gets and reads; on a throw nobody
// will ever read it, and an unread body holds its connection open on
// Node/undici — the same reason the successful-substitution path
// releases it (Copilot). Matters most for a standalone user of this
// exported wrapper, where no timeout wrapper is underneath to have
// already drained it.
if (callerSignal?.aborted) {
releaseBodyDetached(response);
throw callerSignal.reason;
}
let probe: Response;
try {
probe = await fetchFn(candidate, { headers, signal: callerSignal });
} catch (err) {
// A deadline the Inspector imposed is *our* error, not the server's
// answer, so it escapes rather than being folded back into the original
// response (#2319, Copilot). Swallowing it would hand the caller the
// preceding 404 and discard the one thing the deadline adds — the name
// of the endpoint that stalled — leaving discovery to fail later under
// a message that points at the wrong URL. There is no flow this can
// break that was not already broken: every candidate this loop probes
// is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a
// stall the SDK would have hit itself, unbounded. Everything else — a
// CORS rejection, DNS, a reset — still falls back as documented above.
if (err instanceof OAuthRequestTimeoutError) {
releaseBodyDetached(response);
throw err;
}
// Likewise a caller that gave up: substituting the preceding 404 for
// its own abort would report a discovery failure for a request it
// deliberately cancelled. The caller's own `reason` is what surfaces,
// here and at the two sites below, rather than whatever shape the
// underlying layer happened to reject with.
if (callerSignal?.aborted) {
releaseBodyDetached(response);
throw callerSignal.reason;
}
return response;
}
if (!probe.ok) {
// A probe response nobody will read still holds its connection open on
// Node/undici, and this loop can run on every OAuth attempt — so
// release it rather than letting repeated discovery against a 404
// candidate exhaust the origin's pool (Copilot). Same discipline as
// `core/mcp/node/authChallengeFetch.ts`.
await releaseBody(probe);
// Rechecked after the release, which awaits: an abort that lands while
// the body is being discarded must not be answered with the preceding
// response either, and `continue` would otherwise carry on probing for
// a caller that has stopped waiting.
if (callerSignal?.aborted) {
releaseBodyDetached(response);
throw callerSignal.reason;
}
if (continuesDiscovery(probe.status)) continue;
return response;
}
// Deliberately not gated on `content-type`: the SDK parses a 2xx
// discovery body whatever media type it carries, so gating here could
// skip a genuine OIDC document served with an odd one and substitute a
// later RFC-8414-only document in its place. A body that will not parse
// falls through to `return response` below, where the SDK re-fetches it
// and raises its own parse error.
let parsed: unknown;
let body: string;
try {
body = await probe.text();
parsed = JSON.parse(body);
} catch {
// `fetch` resolves on headers, so the abort can land here rather than
// in the catch above: the probe's headers arrived, the caller gave up,
// and `probe.text()` rejects on the partial body. Substituting the
// preceding 404 would hand a standalone user of this wrapper a
// discovery failure in place of its own abort reason (Copilot). A body
// that merely will not parse still falls through, which is the case
// this catch exists for.
if (callerSignal?.aborted) {
releaseBodyDetached(response);
throw callerSignal.reason;
}
return response;
}
if (!isRfc8414OnlyMetadata(parsed)) {
// Either not metadata at all, or a genuine OpenID provider document the
// SDK can already read. Stop looking: this is the document discovery
// would have used, and it needs no help.
return response;
}
// `fetch` follows redirects, so the candidate is where the probe was
// *aimed*; `probe.url` is where the body actually came from. Report the
// latter, falling back for a synthesized response that carries no url
// (Copilot).
const source = probe.url || candidate;
// The original failed response is about to be dropped in favour of the
// substitution, so release its connection too.
await releaseBody(response);
console.warn(
`[oauth] ${source} returned RFC 8414 OAuth 2.0 authorization server ` +
`metadata, not an OpenID provider document. The MCP TypeScript SDK ` +
`validates that path as OpenID Connect Discovery and would reject it ` +
`(modelcontextprotocol/typescript-sdk#2733), so the Inspector is ` +
`handing it to discovery as the RFC 8414 document it is.`,
);
return new Response(body, {
status: 200,
statusText: "OK",
headers: {
"content-type": "application/json",
// The substituted response is what the fetch trackers above this
// wrapper record, so it says where its body actually came from
// rather than letting the Network tab imply the RFC 8414 path
// answered. Named on the response, not logged only, so it survives
// into the captured entry.
[COMPAT_SOURCE_HEADER]: source,
},
});
}
return response;
};
}