From 039fc6f02e0c1fb987e440854fde53ed30ee7ce3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 02:35:27 -0400 Subject: [PATCH] fix(auth): release discarded probe bodies, report the redirected source, proxy the CLI refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings. Probe responses the loop skips — and the original failed response the substitution replaces — were dropped without releasing their bodies. On Node/undici that holds the connection, and this loop can run on every OAuth attempt, so repeated discovery against a 4xx candidate could exhaust the origin's pool. Both paths now cancel, the same discipline `core/mcp/node/authChallengeFetch.ts` uses for a discarded 401. `fetch` follows redirects, so the candidate URL is where the probe was aimed, not necessarily where the document came from. The source header and the console warning now report `probe.url`, falling back to the candidate for a synthesized response that carries none. The CLI's stored-token refresh built its fetch from the global rather than from `createProxyFetch()`, so a server reachable only through `HTTPS_PROXY` was probed directly. It now builds one proxy-aware fetch and hands it to both the discovery probe and `refreshAuthorization`, so neither leg bypasses the proxy — this function runs outside `InspectorClient`, so nothing else puts a proxy under it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/cli/src/cli.ts | 13 +++-- .../core/auth/oidcDiscoveryCompat.test.ts | 53 +++++++++++++++++++ core/auth/oidcDiscoveryCompat.ts | 27 +++++++++- 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 6561e67b..14f51ab9 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -335,15 +335,21 @@ export async function refreshStoredAuthToken( // wrapper — otherwise a stored refresh token with no persisted // `serverMetadata` still cannot refresh against an authorization server that // publishes RFC 8414 metadata at the OIDC well-known path (Copilot). - const compatFetch = withRfc8414OidcCompat(fetch); + // + // Built over `createProxyFetch()` for the same reason `environment.fetch` is + // (#2067): this whole function runs outside `InspectorClient`, so nothing + // else puts a proxy under it, and a server reachable only through + // `HTTPS_PROXY` would otherwise be probed directly. The same fetch is handed + // to the token request below, so neither leg bypasses the proxy (Copilot). + const storedAuthFetch = withRfc8414OidcCompat(createProxyFetch() ?? fetch); const discover: typeof discoverAuthorizationServerMetadata = deps.discover ?? ((authorizationServerUrl, options) => discoverAuthorizationServerMetadata(authorizationServerUrl, { ...options, // A caller-supplied fetch is left alone — it is theirs to compose. The - // walker below passes no options, so in practice this wraps the global. - fetchFn: options?.fetchFn ?? compatFetch, + // walker below passes no options, so in practice this is ours. + fetchFn: options?.fetchFn ?? storedAuthFetch, })); const snapshot = await readOAuthSnapshot(statePath); @@ -395,6 +401,7 @@ export async function refreshStoredAuthToken( clientInformation, refreshToken, resource: new URL(serverUrl), + fetchFn: storedAuthFetch, }); } catch (err) { const message = err instanceof Error ? err.message : String(err); diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 47aaac15..e3bf9d4d 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -244,6 +244,59 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(2); }); + it("releases the bodies it discards", async () => { + // A response nobody reads still holds its connection on Node/undici, and + // this loop runs on every OAuth attempt (Copilot). + vi.spyOn(console, "warn").mockImplementation(() => {}); + const cancelled: string[] = []; + const streamed = (name: string, body: string): Response => + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(body)); + controller.close(); + }, + cancel() { + cancelled.push(name); + }, + }), + { status: name === "appended" ? 200 : 404 }, + ); + + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_APPENDED) + return streamed("appended", JSON.stringify(RFC8414_DOC)); + if (url === OIDC_SUFFIXED) return streamed("suffixed", "nope"); + return streamed("original", "nope"); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect((await wrapped(RFC8414_URL)).json()).resolves.toEqual( + RFC8414_DOC, + ); + // The 404 probe, and the original response the substitution replaces. + expect(cancelled.sort()).toEqual(["original", "suffixed"]); + }); + + it("names the URL the body came from after a redirect", async () => { + // `fetch` follows redirects, so the candidate is where the probe was aimed + // — `probe.url` is where the document actually lives (Copilot). + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const redirected = "https://as.example.com/tenant/oidc-metadata"; + const base = vi.fn(async (input) => { + if (String(input) !== OIDC_SUFFIXED) return notFound(); + const probe = json(RFC8414_DOC); + Object.defineProperty(probe, "url", { value: redirected }); + return probe; + }); + const wrapped = withRfc8414OidcCompat(base); + + const response = await wrapped(RFC8414_URL); + expect(response.headers.get(COMPAT_SOURCE_HEADER)).toBe(redirected); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(redirected)); + }); + it("walks past a candidate that 404s, as the SDK does", async () => { const original = notFound(); const base = vi.fn().mockResolvedValue(original); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 19b80141..bb30e722 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -160,6 +160,15 @@ 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 { + await response.body?.cancel().catch(() => {}); +} + /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -257,6 +266,12 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { 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); if (continuesDiscovery(probe.status)) continue; return response; } @@ -282,8 +297,16 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { 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] ${candidate} returned RFC 8414 OAuth 2.0 authorization server ` + + `[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 ` + @@ -299,7 +322,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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]: candidate, + [COMPAT_SOURCE_HEADER]: source, }, }); }