fix(auth): release discarded probe bodies, report the redirected source, proxy the CLI refresh
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
89d6af4436
commit
039fc6f02e
3 changed files with 88 additions and 5 deletions
|
|
@ -335,15 +335,21 @@ export async function refreshStoredAuthToken(
|
||||||
// wrapper — otherwise a stored refresh token with no persisted
|
// wrapper — otherwise a stored refresh token with no persisted
|
||||||
// `serverMetadata` still cannot refresh against an authorization server that
|
// `serverMetadata` still cannot refresh against an authorization server that
|
||||||
// publishes RFC 8414 metadata at the OIDC well-known path (Copilot).
|
// 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 =
|
const discover: typeof discoverAuthorizationServerMetadata =
|
||||||
deps.discover ??
|
deps.discover ??
|
||||||
((authorizationServerUrl, options) =>
|
((authorizationServerUrl, options) =>
|
||||||
discoverAuthorizationServerMetadata(authorizationServerUrl, {
|
discoverAuthorizationServerMetadata(authorizationServerUrl, {
|
||||||
...options,
|
...options,
|
||||||
// A caller-supplied fetch is left alone — it is theirs to compose. The
|
// 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.
|
// walker below passes no options, so in practice this is ours.
|
||||||
fetchFn: options?.fetchFn ?? compatFetch,
|
fetchFn: options?.fetchFn ?? storedAuthFetch,
|
||||||
}));
|
}));
|
||||||
|
|
||||||
const snapshot = await readOAuthSnapshot(statePath);
|
const snapshot = await readOAuthSnapshot(statePath);
|
||||||
|
|
@ -395,6 +401,7 @@ export async function refreshStoredAuthToken(
|
||||||
clientInformation,
|
clientInformation,
|
||||||
refreshToken,
|
refreshToken,
|
||||||
resource: new URL(serverUrl),
|
resource: new URL(serverUrl),
|
||||||
|
fetchFn: storedAuthFetch,
|
||||||
});
|
});
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
const message = err instanceof Error ? err.message : String(err);
|
const message = err instanceof Error ? err.message : String(err);
|
||||||
|
|
|
||||||
|
|
@ -244,6 +244,59 @@ describe("withRfc8414OidcCompat", () => {
|
||||||
expect(base).toHaveBeenCalledTimes(2);
|
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<typeof fetch>(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<typeof fetch>(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 () => {
|
it("walks past a candidate that 404s, as the SDK does", async () => {
|
||||||
const original = notFound();
|
const original = notFound();
|
||||||
const base = vi.fn<typeof fetch>().mockResolvedValue(original);
|
const base = vi.fn<typeof fetch>().mockResolvedValue(original);
|
||||||
|
|
|
||||||
|
|
@ -160,6 +160,15 @@ function continuesDiscovery(status: number): boolean {
|
||||||
return (status >= 400 && status < 500) || status === 502;
|
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(() => {});
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether a parsed body is RFC 8414 authorization-server metadata that is *not*
|
* Whether a parsed body is RFC 8414 authorization-server metadata that is *not*
|
||||||
* a valid OpenID provider document — the exact shape the upstream schema
|
* 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;
|
return response;
|
||||||
}
|
}
|
||||||
if (!probe.ok) {
|
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;
|
if (continuesDiscovery(probe.status)) continue;
|
||||||
return response;
|
return response;
|
||||||
}
|
}
|
||||||
|
|
@ -282,8 +297,16 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
|
||||||
return response;
|
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(
|
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 ` +
|
`metadata, not an OpenID provider document. The MCP TypeScript SDK ` +
|
||||||
`validates that path as OpenID Connect Discovery and would reject it ` +
|
`validates that path as OpenID Connect Discovery and would reject it ` +
|
||||||
`(modelcontextprotocol/typescript-sdk#2733), so the Inspector is ` +
|
`(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
|
// rather than letting the Network tab imply the RFC 8414 path
|
||||||
// answered. Named on the response, not logged only, so it survives
|
// answered. Named on the response, not logged only, so it survives
|
||||||
// into the captured entry.
|
// into the captured entry.
|
||||||
[COMPAT_SOURCE_HEADER]: candidate,
|
[COMPAT_SOURCE_HEADER]: source,
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue