From 8d6803953c161649deb971290cb9e3dc3c6342da Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:11:16 -0400 Subject: [PATCH] perf(auth): gate the override wrapper on the discovery request, and guard disk values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two suppressed review findings. With an override configured, the wrapper cloned and parsed every successful application/json response — every tools/call result and every resource payload on the direct CLI/TUI transports — only to conclude it was not a metadata document. It now tests the request URL for the RFC 8414 / OIDC well-known paths first, so ordinary traffic costs one string test and is returned untouched. The read path also trusted StoredMCPServer's `string` type for the two override fields, which a hand-edited mcp.json does not honor: the CLI and TUI read the file directly and then `.trim()` these, so a number would crash the server load. Guarded with cleanEndpointOverride, mirroring cleanAuthorizationParams. Signed-off-by: cliffhall --- .../test/core/auth/endpointOverrides.test.ts | 64 +++++++++++++++---- .../web/src/test/core/mcp/serverList.test.ts | 23 +++++++ core/auth/endpointOverrides.ts | 48 ++++++++++++++ core/mcp/serverList.ts | 45 +++++++++++-- 4 files changed, 162 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/auth/endpointOverrides.test.ts b/clients/web/src/test/core/auth/endpointOverrides.test.ts index f44a4afc..19fbe266 100644 --- a/clients/web/src/test/core/auth/endpointOverrides.test.ts +++ b/clients/web/src/test/core/auth/endpointOverrides.test.ts @@ -14,6 +14,8 @@ const METADATA = { response_types_supported: ["code"], }; +const AS_METADATA_URL = + "https://as.example.com/.well-known/oauth-authorization-server"; const STAGING_AUTHORIZE = "https://staging.example.com/authorize"; const STAGING_TOKEN = "https://staging.example.com/token"; @@ -190,7 +192,7 @@ describe("withOAuthEndpointOverrides", () => { tokenUrl: STAGING_TOKEN, })); - const response = await wrapped("https://as.example.com/.well-known/x"); + const response = await wrapped(AS_METADATA_URL); await expect(response.json()).resolves.toEqual({ ...METADATA, authorization_endpoint: STAGING_AUTHORIZE, @@ -206,18 +208,52 @@ describe("withOAuthEndpointOverrides", () => { const config: { overrides?: { tokenUrl?: string } } = {}; const wrapped = withOAuthEndpointOverrides(base, () => config.overrides); - const before = await wrapped("https://as.example.com/.well-known/x"); + const before = await wrapped(AS_METADATA_URL); await expect(before.json()).resolves.toMatchObject({ token_endpoint: METADATA.token_endpoint, }); config.overrides = { tokenUrl: STAGING_TOKEN }; - const after = await wrapped("https://as.example.com/.well-known/x"); + const after = await wrapped(AS_METADATA_URL); await expect(after.json()).resolves.toMatchObject({ token_endpoint: STAGING_TOKEN, }); }); + // The gate that keeps this wrapper off the hot path: an ordinary JSON-RPC + // response is not even cloned, let alone parsed. Regression test for PR #2037. + it("ignores a response to a request that is not metadata discovery", async () => { + const original = jsonResponse(METADATA); + const clone = vi.spyOn(original, "clone"); + const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ + tokenUrl: STAGING_TOKEN, + })); + + await expect(wrapped("https://mcp.example.com/mcp")).resolves.toBe( + original, + ); + expect(clone).not.toHaveBeenCalled(); + }); + + it("recognizes discovery through every request-input form", async () => { + const wrapped = withOAuthEndpointOverrides( + // A fresh response per call: each patched one consumes its own clone. + async () => jsonResponse(METADATA), + () => ({ tokenUrl: STAGING_TOKEN }), + ); + const oidcUrl = "https://as.example.com/.well-known/openid-configuration"; + for (const input of [ + AS_METADATA_URL, + new URL(oidcUrl), + new Request(`${AS_METADATA_URL}/tenant-a`), + ]) { + const response = await wrapped(input); + await expect(response.json()).resolves.toMatchObject({ + token_endpoint: STAGING_TOKEN, + }); + } + }); + it("warns once about a malformed override, not on every request", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const base: typeof fetch = async () => jsonResponse(METADATA); @@ -225,9 +261,9 @@ describe("withOAuthEndpointOverrides", () => { tokenUrl: "not a url", })); - await wrapped("https://as.example.com/x"); - await wrapped("https://as.example.com/x"); - await wrapped("https://as.example.com/x"); + await wrapped(AS_METADATA_URL); + await wrapped(AS_METADATA_URL); + await wrapped(AS_METADATA_URL); expect(warn).toHaveBeenCalledTimes(1); }); @@ -238,7 +274,7 @@ describe("withOAuthEndpointOverrides", () => { passThrough(original), () => undefined, ); - await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); + await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original); }); it("leaves an error response alone", async () => { @@ -246,7 +282,7 @@ describe("withOAuthEndpointOverrides", () => { const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ tokenUrl: STAGING_TOKEN, })); - await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); + await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original); }); it("leaves a non-JSON response alone", async () => { @@ -256,7 +292,7 @@ describe("withOAuthEndpointOverrides", () => { const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ tokenUrl: STAGING_TOKEN, })); - await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); + await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original); }); // Everything that is not a metadata document comes back as the caller's own @@ -267,7 +303,7 @@ describe("withOAuthEndpointOverrides", () => { const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ tokenUrl: STAGING_TOKEN, })); - const response = await wrapped("https://as.example.com/token"); + const response = await wrapped(AS_METADATA_URL); expect(response).toBe(original); await expect(response.json()).resolves.toEqual({ access_token: "abc" }); }); @@ -277,7 +313,7 @@ describe("withOAuthEndpointOverrides", () => { const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ tokenUrl: STAGING_TOKEN, })); - const response = await wrapped("https://as.example.com/x"); + const response = await wrapped(AS_METADATA_URL); expect(response).toBe(original); await expect(response.text()).resolves.toBe("not json at all"); }); @@ -316,7 +352,7 @@ describe("withOAuthEndpointOverrides", () => { ), () => ({ tokenUrl: STAGING_TOKEN }), ); - const response = await wrapped("https://as.example.com/x"); + const response = await wrapped(AS_METADATA_URL); await expect(response.json()).resolves.toMatchObject({ token_endpoint: STAGING_TOKEN, }); @@ -333,7 +369,7 @@ describe("withOAuthEndpointOverrides", () => { const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ tokenUrl: STAGING_TOKEN, })); - await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); + await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original); }); it("drops the stale content-length and content-encoding of a rewritten body", async () => { @@ -350,7 +386,7 @@ describe("withOAuthEndpointOverrides", () => { ), () => ({ tokenUrl: STAGING_TOKEN }), ); - const response = await wrapped("https://as.example.com/x"); + const response = await wrapped(AS_METADATA_URL); expect(response.headers.get("content-length")).toBeNull(); expect(response.headers.get("content-encoding")).toBeNull(); expect(response.headers.get("content-type")).toBe("application/json"); diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 09e4119c..0492622d 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1304,6 +1304,29 @@ describe("oauthOnInsufficientScope (SEP-2350)", () => { expect(settings?.oauthTokenUrl).toBe("https://staging.example.com/token"); }); + // Hand-edited `mcp.json` reaches the CLI/TUI unvalidated, and + // `oauthEndpointOverridesFromSettings` calls `.trim()` on these — so a + // non-string would crash the server load rather than being ignored. + it("drops a non-string endpoint override from disk", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const settings = storedFieldsToInspectorSettings({ + oauth: { + clientId: "cid", + // Types the field as a string; disk does not honor that. + authorizationUrl: 42 as unknown as string, + tokenUrl: "https://staging.example.com/token", + }, + }); + expect(settings?.oauthAuthorizationUrl).toBeUndefined(); + expect(settings?.oauthTokenUrl).toBe("https://staging.example.com/token"); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("oauth.authorizationUrl"), + ); + // Restored here rather than left to a global teardown: a later describe + // spies on `console.warn` again and would inherit this spy's recorded call. + warn.mockRestore(); + }); + it("reads a blank endpoint override back as unset", () => { const settings = storedFieldsToInspectorSettings({ oauth: { clientId: "cid", authorizationUrl: "", tokenUrl: "" }, diff --git a/core/auth/endpointOverrides.ts b/core/auth/endpointOverrides.ts index e0e01ae6..c2438b30 100644 --- a/core/auth/endpointOverrides.ts +++ b/core/auth/endpointOverrides.ts @@ -196,6 +196,51 @@ export function applyOAuthEndpointOverrides( return patched; } +/** + * The well-known paths authorization-server metadata is discovered at: RFC 8414 + * (§3.1, including the path-suffixed and path-prefixed variants the SDK's + * `buildDiscoveryUrls` emits) and OpenID Connect Discovery. Matched as a + * substring of the pathname so every variant is covered by two literals. + */ +const AS_METADATA_WELL_KNOWN_PATHS = [ + "/.well-known/oauth-authorization-server", + "/.well-known/openid-configuration", +] as const; + +/** 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; +} + +/** + * Whether a request is an authorization-server metadata discovery request. + * + * This is what keeps the wrapper off the hot path. Without it, every successful + * `application/json` response on the connection — every `tools/call` result, and + * every resource payload on the direct CLI/TUI transports — would be buffered + * and parsed a second time before the SDK could consume it, purely to discover + * that it is not a metadata document. Gating on the discovery URL first means + * ordinary traffic is untouched: one string test and the response is returned. + * + * A URL that will not parse falls back to testing the raw string, which is the + * conservative direction — worst case a request is *considered*, and the + * metadata predicate then rejects it. + */ +function isAuthorizationServerMetadataRequest( + input: RequestInfo | URL, +): boolean { + const raw = requestUrlOf(input); + let pathname: string; + try { + pathname = new URL(raw).pathname; + } catch { + pathname = raw; + } + return AS_METADATA_WELL_KNOWN_PATHS.some((path) => pathname.includes(path)); +} + /** * Whether a `content-type` names a **whole JSON document** — something that can * be read to completion. @@ -268,6 +313,9 @@ export function withOAuthEndpointOverrides( const response = await fetchFn(input, init); const overrides = resolveOverrides(); if (!overrides) return response; + // Cheapest tests first: only a discovery request can carry the document + // this wrapper rewrites, so ordinary traffic never reaches the clone below. + if (!isAuthorizationServerMetadataRequest(input)) return response; if (!response.ok) return response; if (!isJsonDocumentResponse(response.headers.get("content-type"))) return response; diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index c29ba9e6..f4be5f4a 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -248,6 +248,33 @@ export function oauthAuthorizationParamsFromSettings( return Object.keys(record).length > 0 ? record : undefined; } +/** + * Validate one of a server's `oauth` endpoint overrides as it comes off disk, + * or `undefined` when there is nothing usable. (#1906) + * + * Same reasoning as `cleanAuthorizationParams` above: `StoredMCPServer` types + * these as `string`, but that is a compile-time promise a hand-edited file does + * not keep, and the CLI/TUI read `mcp.json` directly. A non-string reaches + * `oauthEndpointOverridesFromSettings`, which calls `.trim()` on it — so without + * this the whole server list fails to load. URL *validity* is not checked here; + * that happens where the override is applied, so a typo drops one field with a + * warning instead of anything louder. + */ +export function cleanEndpointOverride( + value: string | undefined, + field: "authorizationUrl" | "tokenUrl", +): string | undefined { + if (value === undefined) return undefined; + // Keep: unreachable per the parameter type, reachable from disk. + if (typeof value !== "string") { + console.warn( + `Ignoring \`oauth.${field}\`: expected a string, got ${typeof value}.`, + ); + return undefined; + } + return value.trim() || undefined; +} + /** * Collapse a server's endpoint-override fields into the * `InspectorClientOptions.oauth` sub-shape, or `undefined` when neither is set. @@ -381,10 +408,20 @@ export function storedFieldsToInspectorSettings( settings.oauthAuthorizationParams = envRecordToPairs(authorizationParams); } // Truthiness drops empty strings like the credential fields above, so a - // cleared field reads back as unset rather than as an empty override. (#1906) - if (stored.oauth?.authorizationUrl) - settings.oauthAuthorizationUrl = stored.oauth.authorizationUrl; - if (stored.oauth?.tokenUrl) settings.oauthTokenUrl = stored.oauth.tokenUrl; + // cleared field reads back as unset rather than as an empty override. The + // `string` type is a compile-time promise a hand-edited `mcp.json` does not + // keep, and only the web client's `/api/servers` route checks it — the CLI and + // TUI read the file directly and then call `.trim()` on these in + // `oauthEndpointOverridesFromSettings`, so a non-string would crash the server + // load. Sanitized like `authorizationParams` above rather than trusted. + // (#1906) + const authorizationUrl = cleanEndpointOverride( + stored.oauth?.authorizationUrl, + "authorizationUrl", + ); + if (authorizationUrl) settings.oauthAuthorizationUrl = authorizationUrl; + const tokenUrl = cleanEndpointOverride(stored.oauth?.tokenUrl, "tokenUrl"); + if (tokenUrl) settings.oauthTokenUrl = tokenUrl; if (stored.oauth?.onInsufficientScope) { settings.oauthOnInsufficientScope = stored.oauth.onInsufficientScope; }