From 7ca753bc4c78bc0d740c2fd631f4354e1e1ea0e4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 00:40:11 -0400 Subject: [PATCH 1/5] fix(auth): accept RFC 8414 metadata served at the OIDC well-known path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SDK's `discoverAuthorizationServerMetadata` picks its validation schema from the well-known filename that resolved rather than from the document that came back, so anything found at `/.well-known/openid-configuration` is validated as an OpenID provider document — requiring `jwks_uri`, `subject_types_supported` and `id_token_signing_alg_values_supported`, three fields RFC 8414 does not define. RFC 8414 §5 permits that filename for general OAuth metadata, so a conforming plain OAuth 2.0 authorization server is rejected; and because the parse throws instead of continuing the candidate loop, discovery aborts and the connection fails outright. Filed upstream as modelcontextprotocol/typescript-sdk#2733. `core/auth/oidcDiscoveryCompat.ts` works around it without fabricating a field: on a failed RFC 8414 candidate it probes the OIDC candidates the SDK would try next and, when one returns a document that satisfies `OAuthMetadataSchema` but fails the OIDC schema, serves that body as the RFC 8414 response so the SDK picks the schema that describes it. A genuine OpenID provider document is left to the SDK's own OIDC leg. Issuer validation runs unchanged, since the substituted document is the one the server published. It wraps `effectiveAuthFetch` — above the fetch tracker, the opposite of `withOAuthEndpointOverrides` — so the Network tab still records the real 404 and the real probe rather than the substitution. The candidate derivation mirrors the SDK's `buildDiscoveryUrls`, which a fetch wrapper cannot call, and a test pins it against that export so the two cannot drift. Adds `oauth.asMetadataPath` to the composable test server (mirroring `oauth.resourceMetadataPath`) and the `oauth-rfc8414-at-oidc-path-http.json` showcase config that reproduces the failure. Closes #2172 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- AGENTS.md | 19 + README.md | 17 + .../core/auth/oidcDiscoveryCompat.test.ts | 330 ++++++++++++++++++ .../auth/rfc8414AtOidcPath.test.ts | 100 ++++++ core/auth/index.ts | 5 + core/auth/oidcDiscoveryCompat.ts | 254 ++++++++++++++ core/mcp/inspectorClient.ts | 11 +- .../oauth-rfc8414-at-oidc-path-http.json | 19 + test-servers/src/composable-test-server.ts | 15 + test-servers/src/load-config.ts | 8 + test-servers/src/test-helpers.ts | 10 +- test-servers/src/test-server-fixtures.ts | 12 + test-servers/src/test-server-oauth.ts | 80 +++-- 13 files changed, 844 insertions(+), 36 deletions(-) create mode 100644 clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts create mode 100644 clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts create mode 100644 core/auth/oidcDiscoveryCompat.ts create mode 100644 test-servers/configs/oauth-rfc8414-at-oidc-path-http.json diff --git a/AGENTS.md b/AGENTS.md index 1fc7f2d341..b2ee092107 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,6 +120,25 @@ v2/main/ │ │ # metadata document, the one seam SDK v2 routes │ │ # BOTH endpoints through (neither reaches the │ │ # OAuthClientProvider) — #1906; +│ │ # oidcDiscoveryCompat.ts workaround for +│ │ # typescript-sdk#2733: the SDK picks the +│ │ # metadata schema from the well-known +│ │ # FILENAME, so a plain OAuth 2.0 AS +│ │ # publishing RFC 8414 metadata at +│ │ # /.well-known/openid-configuration (which +│ │ # RFC 8414 §5 permits) is parsed as an +│ │ # OpenID provider document and THROWS, +│ │ # aborting discovery rather than trying the +│ │ # next candidate. The wrapper fabricates +│ │ # NOTHING — on a failed RFC 8414 candidate +│ │ # it probes the OIDC candidates and, when +│ │ # one is RFC 8414 metadata that is not a +│ │ # valid OIDC document, serves that body as +│ │ # the RFC 8414 response so the SDK picks the +│ │ # right schema. Sits ABOVE the fetch tracker +│ │ # (unlike endpointOverrides, which sits +│ │ # below) so the Network tab records the real +│ │ # 404 and the real probe — #2172; │ │ # secret-storage-info.ts browser-safe │ │ # descriptor of WHERE a typed secret lands │ │ # — kind/plaintext/durable plus the label, diff --git a/README.md b/README.md index ccf9d6002b..e56dc04549 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | +| `oauth-rfc8414-at-oidc-path-http.json` **(legacy era)** | Plain OAuth 2.0 AS metadata served at the OIDC well-known path | [#2172](https://github.com/modelcontextprotocol/inspector/issues/2172) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | @@ -438,6 +439,22 @@ The same server is worth running against `--cli` / `--tui`, which reach it by a The value now rides the normalized `AuthChallenge` as a string — it has to be serializable, because the web client's challenge crosses the remote-backend boundary as JSON — and is converted to a `URL` at the OAuth boundary, where it is handed to `auth()` as `resourceMetadataUrl` and to the CIMD pre-registration probe, which runs *before* `auth()` and would otherwise do its own default-location discovery. A malformed value is ignored rather than surfaced, matching the SDK's own `WWW-Authenticate` parser: discovery falls back to the default locations instead of failing the whole authorization on a bad header. The callback leg needs nothing extra — SDK `auth()` persists the URL in its discovery state, so it survives both the web full-page redirect and the CLI/TUI loopback callback. +#### Plain OAuth 2.0 metadata at the OIDC well-known path + +`oauth-rfc8414-at-oidc-path-http.json` is an OAuth-protected server (combined AS + resource, DCR enabled) whose RFC 8414 authorization-server metadata is served **only** from `/.well-known/openid-configuration`. It is a plain OAuth 2.0 authorization server — no ID tokens, no `jwks_uri`, no `sub` claims — and RFC 8414 §5 explicitly permits that filename for general OAuth metadata. `/.well-known/oauth-authorization-server` is deliberately left unserved. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Add the server and click **Connect**: authorization must proceed normally. On the broken build it failed before the browser ever opened, with a `ZodError` naming three fields the server had no reason to publish: + +``` +"path":["jwks_uri"] … "path":["subject_types_supported"] … "path":["id_token_signing_alg_values_supported"] +``` + +The cause is upstream. `discoverAuthorizationServerMetadata` in `@modelcontextprotocol/client@2.0.0` picks its validation schema from the well-known **filename that resolved**, not from the document that came back — anything found at `openid-configuration` is parsed as OpenID Connect Discovery 1.0 provider metadata, which requires those three fields. And because the parse *throws* rather than continuing the candidate loop, discovery aborts outright instead of falling through ([#2172](https://github.com/modelcontextprotocol/inspector/issues/2172), filed upstream as [typescript-sdk#2733](https://github.com/modelcontextprotocol/typescript-sdk/issues/2733)). + +`core/auth/oidcDiscoveryCompat.ts` works around it without fabricating anything. When the RFC 8414 candidate comes back 4xx, it fetches the OIDC candidates the SDK would try next; if one returns a document that satisfies `OAuthMetadataSchema` but *fails* the OIDC schema, that document is returned as the response to the RFC 8414 request — so the SDK validates it under the schema that actually describes it. A genuine OpenID provider document is left alone and takes the SDK's normal OIDC leg. Issuer validation is untouched, since the substituted document is the one the server published, `issuer` included. + +Watch the Network tab to confirm the Inspector is not hiding anything: the real 404 on `/.well-known/oauth-authorization-server` and the real request to `/.well-known/openid-configuration` are both recorded, and the metadata shown in the Auth tab is exactly what the server sent — no invented `jwks_uri`. The wrapper sits above the fetch tracker for that reason. + #### Logging, both eras `logging-legacy-http.json` and `logging-modern-http.json` both serve `logging: true` plus a `send_notification` tool that emits a `notifications/message` at a chosen level. The legacy one is a plain streamable-HTTP server; the modern one sets `transport.modern: true`. diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts new file mode 100644 index 0000000000..88ef8d208f --- /dev/null +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -0,0 +1,330 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { buildDiscoveryUrls } from "@modelcontextprotocol/client"; +import { + oidcDiscoveryCandidates, + isRfc8414OnlyMetadata, + withRfc8414OidcCompat, +} from "@inspector/core/auth/oidcDiscoveryCompat.js"; + +/** Minimal RFC 8414 authorization-server metadata — no OIDC-only fields. */ +const RFC8414_DOC = { + issuer: "https://as.example.com/tenant", + authorization_endpoint: "https://as.example.com/tenant/authorize", + token_endpoint: "https://as.example.com/tenant/token", + response_types_supported: ["code"], +}; + +/** The same document plus the three fields OpenID Connect Discovery requires. */ +const OIDC_DOC = { + ...RFC8414_DOC, + jwks_uri: "https://as.example.com/tenant/jwks", + subject_types_supported: ["public"], + id_token_signing_alg_values_supported: ["RS256"], +}; + +const RFC8414_URL = + "https://as.example.com/.well-known/oauth-authorization-server/tenant"; +const OIDC_SUFFIXED = + "https://as.example.com/.well-known/openid-configuration/tenant"; +const OIDC_APPENDED = + "https://as.example.com/tenant/.well-known/openid-configuration"; + +function json(body: unknown, init?: ResponseInit): Response { + return new Response(JSON.stringify(body), { + status: 200, + headers: { "content-type": "application/json" }, + ...init, + }); +} + +function notFound(): Response { + return new Response("nope", { status: 404 }); +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("oidcDiscoveryCandidates", () => { + // The derivation is a hand-written mirror of the SDK's `buildDiscoveryUrls`, + // which a fetch wrapper cannot call (it sees a request URL, not the + // authorization-server URL). Pinning it against the SDK's own export is what + // keeps the two from drifting when the SDK changes its candidate list. + it.each([ + "https://as.example.com", + "https://as.example.com/", + "https://as.example.com/tenant", + "https://as.example.com/tenant/", + "https://misc.poodll.com/mod/minilesson/mcp.php", + ])("matches the SDK's OIDC candidates for %s", (authServerUrl) => { + const sdkUrls = buildDiscoveryUrls(authServerUrl); + const rfc8414 = sdkUrls.find((entry) => entry.type === "oauth"); + expect(rfc8414).toBeDefined(); + const expected = sdkUrls + .filter((entry) => entry.type === "oidc") + .map((entry) => entry.url.href); + expect(oidcDiscoveryCandidates(rfc8414!.url.href)).toEqual(expected); + }); + + it("returns nothing for a URL that is not an RFC 8414 candidate", () => { + expect(oidcDiscoveryCandidates("https://as.example.com/tokens")).toEqual( + [], + ); + }); + + it("returns nothing for an unparseable URL", () => { + expect(oidcDiscoveryCandidates("not a url")).toEqual([]); + }); +}); + +describe("isRfc8414OnlyMetadata", () => { + it("accepts plain RFC 8414 metadata", () => { + expect(isRfc8414OnlyMetadata(RFC8414_DOC)).toBe(true); + }); + + it("rejects a document that is a valid OpenID provider document", () => { + expect(isRfc8414OnlyMetadata(OIDC_DOC)).toBe(false); + }); + + it("rejects a body that is not authorization-server metadata at all", () => { + expect(isRfc8414OnlyMetadata({ hello: "world" })).toBe(false); + }); +}); + +describe("withRfc8414OidcCompat", () => { + it("passes a successful response through untouched", async () => { + const original = json(RFC8414_DOC); + const base = vi.fn().mockResolvedValue(original); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(1); + }); + + it("passes a status the SDK will not walk past through untouched", async () => { + const original = new Response("boom", { status: 500 }); + const base = vi.fn().mockResolvedValue(original); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(1); + }); + + it("does not probe when the failed request is not a discovery request", async () => { + const original = notFound(); + const base = vi.fn().mockResolvedValue(original); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped("https://as.example.com/tokens")).resolves.toBe( + original, + ); + expect(base).toHaveBeenCalledTimes(1); + }); + + it("serves an RFC 8414 document found on the appended OIDC path", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_APPENDED) return json(RFC8414_DOC); + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + const response = await wrapped(RFC8414_URL); + expect(response.status).toBe(200); + expect(response.headers.get("content-type")).toBe("application/json"); + await expect(response.json()).resolves.toEqual(RFC8414_DOC); + // The failed original, then both OIDC candidates. + expect(base).toHaveBeenCalledTimes(3); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(OIDC_APPENDED)); + }); + + it("serves an RFC 8414 document found on the path-suffixed OIDC path", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_SUFFIXED) return json(RFC8414_DOC); + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect((await wrapped(RFC8414_URL)).json()).resolves.toEqual( + RFC8414_DOC, + ); + // The failed original, then the first OIDC candidate — no need for a second. + expect(base).toHaveBeenCalledTimes(2); + }); + + it("leaves a genuine OpenID provider document to the SDK's own OIDC leg", async () => { + const original = notFound(); + const base = vi.fn(async (input) => { + if (String(input) === OIDC_SUFFIXED) return json(OIDC_DOC); + return original; + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + // Stops at the document discovery would have used; the appended candidate + // is never probed. + expect(base).toHaveBeenCalledTimes(2); + }); + + it("skips a candidate that is not JSON and keeps looking", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_SUFFIXED) { + return new Response("login", { + status: 200, + headers: { "content-type": "text/html" }, + }); + } + if (url === OIDC_APPENDED) return json(RFC8414_DOC); + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect((await wrapped(RFC8414_URL)).json()).resolves.toEqual( + RFC8414_DOC, + ); + }); + + it("skips a candidate whose JSON body will not parse", async () => { + const original = notFound(); + const base = vi.fn(async (input) => { + if (String(input).includes("openid-configuration")) { + return new Response("{ not json", { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + return original; + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(3); + }); + + it("skips a candidate that itself fails", async () => { + const original = notFound(); + const base = vi.fn().mockResolvedValue(original); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(3); + }); + + it("treats a probe that throws as a candidate to skip", async () => { + const original = notFound(); + const base = vi.fn(async (input) => { + if (String(input).includes("openid-configuration")) { + throw new TypeError("Failed to fetch"); + } + return original; + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(3); + }); + + it("probes on a 502, which the SDK also walks past", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const base = vi.fn(async (input) => { + if (String(input) === OIDC_APPENDED) return json(RFC8414_DOC); + return new Response("bad gateway", { status: 502 }); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect((await wrapped(RFC8414_URL)).json()).resolves.toEqual( + RFC8414_DOC, + ); + }); + + it("carries the discovery headers from the failed request onto the probe", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const seen: Headers[] = []; + const base = vi.fn(async (input, init) => { + const url = String(input); + if (url.includes("openid-configuration")) { + seen.push(new Headers(init?.headers)); + return json(RFC8414_DOC); + } + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await wrapped(RFC8414_URL, { + headers: { + "MCP-Protocol-Version": "2025-11-25", + Accept: "application/json", + }, + }); + expect(seen[0]?.get("mcp-protocol-version")).toBe("2025-11-25"); + expect(seen[0]?.get("accept")).toBe("application/json"); + }); + + it("reads the headers off a Request input when there is no init", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const seen: Headers[] = []; + const base = vi.fn(async (input, init) => { + const url = String(input instanceof Request ? input.url : input); + if (url.includes("openid-configuration")) { + seen.push(new Headers(init?.headers)); + return json(RFC8414_DOC); + } + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await wrapped( + new Request(RFC8414_URL, { + headers: { "MCP-Protocol-Version": "2025-11-25" }, + }), + ); + expect(seen[0]?.get("mcp-protocol-version")).toBe("2025-11-25"); + }); + + it("accepts a URL instance as the request input", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const base = vi.fn(async (input) => { + if (String(input) === OIDC_APPENDED) return json(RFC8414_DOC); + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect((await wrapped(new URL(RFC8414_URL))).json()).resolves.toEqual( + RFC8414_DOC, + ); + }); + + it("handles a root authorization server, which has a single OIDC candidate", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const rootDoc = { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + response_types_supported: ["code"], + }; + const base = vi.fn(async (input) => { + if ( + String(input) === + "https://as.example.com/.well-known/openid-configuration" + ) { + return json(rootDoc); + } + return notFound(); + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect( + ( + await wrapped( + "https://as.example.com/.well-known/oauth-authorization-server", + ) + ).json(), + ).resolves.toEqual(rootDoc); + expect(base).toHaveBeenCalledTimes(2); + }); +}); diff --git a/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts new file mode 100644 index 0000000000..d2c0e08443 --- /dev/null +++ b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts @@ -0,0 +1,100 @@ +/** + * RFC 8414 authorization-server metadata served at the OpenID Connect + * well-known path, end to end against a real OAuth test server (#2172). + * + * The unit tests prove the wrapper substitutes the right document. This file + * proves the two halves only a real server can: that the SDK genuinely rejects + * a conforming plain-OAuth document found at `openid-configuration`, and that + * the wrapper recovers discovery against the same server without adding a + * field the server never published. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from "vitest"; +import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/client"; +import { + TestServerHttp, + getDefaultServerConfig, + createOAuthTestServerConfig, + waitForOAuthWellKnown, +} from "@modelcontextprotocol/inspector-test-server"; +import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; + +const OIDC_PATH = "/.well-known/openid-configuration"; + +/** The three fields OpenID Connect Discovery requires and RFC 8414 does not. */ +const OIDC_ONLY_FIELDS = [ + "jwks_uri", + "subject_types_supported", + "id_token_signing_alg_values_supported", +] as const; + +describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => { + let mcpServer: TestServerHttp | null = null; + let serverUrl = ""; + + beforeAll(async () => { + mcpServer = new TestServerHttp({ + ...getDefaultServerConfig(), + serverType: "streamable-http" as const, + ...createOAuthTestServerConfig({ + requireAuth: true, + asMetadataPath: OIDC_PATH, + }), + }); + const port = await mcpServer.start(); + serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl, { metadataPath: OIDC_PATH }); + }, 30_000); + + afterAll(async () => { + await mcpServer?.stop(); + mcpServer = null; + }, 30_000); + + it("serves plain RFC 8414 metadata only from the OIDC path", async () => { + const oidc = await fetch(`${serverUrl}${OIDC_PATH}`); + expect(oidc.status).toBe(200); + const metadata = (await oidc.json()) as Record; + expect(metadata.issuer).toBe(serverUrl); + for (const field of OIDC_ONLY_FIELDS) { + expect(metadata).not.toHaveProperty(field); + } + + const rfc8414 = await fetch( + `${serverUrl}/.well-known/oauth-authorization-server`, + ); + expect(rfc8414.status).toBe(404); + }); + + it("is rejected by unwrapped SDK discovery", async () => { + // The upstream defect this module exists for + // (modelcontextprotocol/typescript-sdk#2733). If this ever starts + // resolving, the SDK has been fixed and `oidcDiscoveryCompat` can go. + await expect( + discoverAuthorizationServerMetadata(serverUrl), + ).rejects.toThrow(/jwks_uri/); + }); + + it("resolves through the compat wrapper without fabricating a field", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const metadata = await discoverAuthorizationServerMetadata(serverUrl, { + fetchFn: withRfc8414OidcCompat(fetch), + }); + + expect(metadata?.issuer).toBe(serverUrl); + expect(metadata?.authorization_endpoint).toBe( + `${serverUrl}/oauth/authorize`, + ); + expect(metadata?.token_endpoint).toBe(`${serverUrl}/oauth/token`); + for (const field of OIDC_ONLY_FIELDS) { + expect(metadata).not.toHaveProperty(field); + } + expect(warn).toHaveBeenCalledWith( + expect.stringContaining(`${serverUrl}${OIDC_PATH}`), + ); + } finally { + warn.mockRestore(); + } + }); +}); diff --git a/core/auth/index.ts b/core/auth/index.ts index f38fcbe037..6118d5be87 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -45,6 +45,11 @@ export { withOAuthEndpointOverrides, } from "./endpointOverrides.js"; export type { OAuthEndpointOverrides } from "./endpointOverrides.js"; +export { + oidcDiscoveryCandidates, + isRfc8414OnlyMetadata, + withRfc8414OidcCompat, +} from "./oidcDiscoveryCompat.js"; // Storage export type { diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts new file mode 100644 index 0000000000..498749ccae --- /dev/null +++ b/core/auth/oidcDiscoveryCompat.ts @@ -0,0 +1,254 @@ +/** + * 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[]`, 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 hide the real traffic.** It is installed *above* the fetch + * tracker (`InspectorClient.effectiveAuthFetch`), so the Network tab records + * the genuine 404 on the RFC 8414 path and the genuine request to the OIDC + * path. The substitution is visible only to the SDK's parse. + * + * ## 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 { + 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"; + +/** + * 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); + // 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; +} + +/** + * Whether a `content-type` names a whole JSON document. + * + * An exact media-type test rather than a substring search for "json", for the + * same reason `endpointOverrides.ts` uses one: a streaming media type would be + * read to completion and never resolve. + */ +function isJsonDocumentResponse(contentType: string | null): boolean { + if (!contentType) return false; + const mediaType = contentType.split(";")[0].trim().toLowerCase(); + return mediaType === "application/json" || mediaType.endsWith("+json"); +} + +/** + * 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(); +} + +/** + * 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; + + const candidates = oidcDiscoveryCandidates(requestUrlOf(input)); + if (candidates.length === 0) return response; + + const headers = discoveryHeaders(input, init); + for (const candidate of candidates) { + let probe: Response; + try { + probe = await fetchFn(candidate, { headers }); + } catch { + // A network or CORS failure on a URL the SDK had not asked for yet is + // not this wrapper's to report — the SDK will make the same request and + // handle it. Try the next candidate. + continue; + } + if (!probe.ok) continue; + if (!isJsonDocumentResponse(probe.headers.get("content-type"))) continue; + + let parsed: unknown; + let body: string; + try { + body = await probe.text(); + parsed = JSON.parse(body); + } catch { + continue; + } + 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; + } + + console.warn( + `[oauth] ${candidate} 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" }, + }); + } + + return response; + }; +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 2f115beb49..28cffb6bc0 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -238,6 +238,7 @@ import { type HandleAuthChallengeOptions, } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; +import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { silentLogger, type InspectorLogger } from "../logging/logger.js"; import { createFetchTracker } from "./fetchTracking.js"; @@ -791,7 +792,15 @@ export class InspectorClient extends InspectorClientEventTarget { this.fetchFn = withOAuthEndpointOverrides(this.fetchFn ?? fetch, () => this.oauthManager?.getEndpointOverrides(), ); - this.effectiveAuthFetch = this.buildEffectiveAuthFetch(); + // #2172: recover discovery when a plain OAuth 2.0 authorization server + // publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which + // the SDK validates as an OpenID provider document and rejects. Wraps the + // tracked fetch rather than the base one — the opposite of the overrides + // above — so the Network tab records the real 404 and the real probe rather + // than the substitution the SDK's parse sees. + this.effectiveAuthFetch = withRfc8414OidcCompat( + this.buildEffectiveAuthFetch(), + ); this.sessionId = options.sessionId; diff --git a/test-servers/configs/oauth-rfc8414-at-oidc-path-http.json b/test-servers/configs/oauth-rfc8414-at-oidc-path-http.json new file mode 100644 index 0000000000..a55edffd9e --- /dev/null +++ b/test-servers/configs/oauth-rfc8414-at-oidc-path-http.json @@ -0,0 +1,19 @@ +{ + "serverInfo": { + "name": "oauth-rfc8414-at-oidc-path", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }], + "oauth": { + "enabled": true, + "mode": "combined", + "requireAuth": true, + "asMetadataPath": "/.well-known/openid-configuration", + "scopesSupported": ["mcp"], + "supportDCR": true + }, + "transport": { + "type": "streamable-http", + "port": 8083 + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 85494d1fb2..6ba49795a0 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -613,6 +613,21 @@ export interface ServerConfig { */ resourceMetadataPath?: string; + /** + * Serve the RFC 8414 authorization-server metadata document from this + * non-default path *instead of* `/.well-known/oauth-authorization-server` + * (combined mode only). + * + * Set it to `/.well-known/openid-configuration` to reproduce #2172: a + * plain OAuth 2.0 authorization server — no `jwks_uri`, no + * `subject_types_supported`, no `id_token_signing_alg_values_supported` — + * publishing RFC 8414 metadata at the OIDC well-known path, which RFC 8414 + * §5 permits. As with `resourceMetadataPath`, the default route is left + * unserved, so a client that cannot read the document where it actually + * lives fails outright rather than quietly succeeding elsewhere. + */ + asMetadataPath?: string; + /** * OAuth authorization server issuer URL (combined mode AS metadata). * If not provided, defaults to the test server's base URL. diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 18e09f5a12..38aa510218 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -22,6 +22,8 @@ export interface ConfigFileOAuth { resource?: string; /** Serve RFC 9728 metadata from this path and advertise it on 401 (#2071). */ resourceMetadataPath?: string; + /** Serve RFC 8414 AS metadata from this path instead of the default (#2172). */ + asMetadataPath?: string; issuerUrl?: string; accessTokenIssuers?: string[]; jwksUri?: string; @@ -222,6 +224,12 @@ function validateConfig( `Invalid config in ${filePath}: oauth.resourceMetadataPath must be an origin-relative path (e.g. "/custom/protected-resource") — a value such as "//host/doc" would advertise a document the server does not serve`, ); } + const asPath = oauth.asMetadataPath; + if (asPath !== undefined && !isOriginRelativePath(asPath)) { + throw new Error( + `Invalid config in ${filePath}: oauth.asMetadataPath must be an origin-relative path (e.g. "/.well-known/openid-configuration") — a value such as "//host/doc" would move the document off this server entirely`, + ); + } if (transportType === "stdio" && oauth.enabled === true) { throw new Error( `Invalid config in ${filePath}: oauth requires streamable-http or sse transport`, diff --git a/test-servers/src/test-helpers.ts b/test-servers/src/test-helpers.ts index da73f6f28f..90347015b1 100644 --- a/test-servers/src/test-helpers.ts +++ b/test-servers/src/test-helpers.ts @@ -79,6 +79,13 @@ export interface WaitForOAuthWellKnownOptions { interval?: number; /** Max time per fetch attempt (so one hung request doesn't burn the whole timeout). Default 1000. */ requestTimeout?: number; + /** + * Origin-relative path the AS metadata document is served from. Defaults to + * the RFC 8414 well-known path; pass the configured `oauth.asMetadataPath` + * when the fixture moved the document (#2172), since polling a route the + * server deliberately does not serve can only ever time out. + */ + metadataPath?: string; } /** @@ -97,8 +104,9 @@ export async function waitForOAuthWellKnown( timeout = 5000, interval = 50, requestTimeout = 1000, + metadataPath = "/.well-known/oauth-authorization-server", } = options ?? {}; - const wellKnownUrl = `${serverBaseUrl.replace(/\/$/, "")}/.well-known/oauth-authorization-server`; + const wellKnownUrl = `${serverBaseUrl.replace(/\/$/, "")}${metadataPath}`; const start = Date.now(); let lastStatus: number | undefined; let lastError: unknown; diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 0eeccffc88..4f4fda6916 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -2915,6 +2915,13 @@ export function createOAuthTestServerConfig(options: { * it via `WWW-Authenticate: Bearer resource_metadata="…"` (#2071). */ resourceMetadataPath?: string; + /** + * Move the RFC 8414 authorization-server metadata document off the + * well-known path — set it to `/.well-known/openid-configuration` to serve + * plain OAuth 2.0 metadata where the SDK expects an OpenID provider + * document (#2172). + */ + asMetadataPath?: string; }): Partial { return { oauth: { @@ -2928,6 +2935,11 @@ export function createOAuthTestServerConfig(options: { ...(options.resourceMetadataPath !== undefined ? { resourceMetadataPath: options.resourceMetadataPath } : {}), + // Same `!== undefined` reasoning as above: an explicit `""` is invalid + // and must reach the server so it reports the bad fixture. + ...(options.asMetadataPath !== undefined + ? { asMetadataPath: options.asMetadataPath } + : {}), staticClients: options.staticClients, supportDCR: options.supportDCR ?? false, supportCIMD: options.supportCIMD ?? false, diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 3b7a28e88b..d042a79342 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -83,6 +83,24 @@ function resourceMetadataPath(config: OAuthConfig): string | undefined { return path; } +/** + * The path the RFC 8414 authorization-server metadata document is served from, + * validated the same way `resourceMetadataPath` is. Defaults to the well-known + * location; `asMetadataPath` moves it (see the field's doc comment). + */ +function asMetadataPath(config: OAuthConfig): string { + const path = config.asMetadataPath; + if (path === undefined) { + return "/.well-known/oauth-authorization-server"; + } + if (!isOriginRelativePath(path)) { + throw new Error( + `oauth.asMetadataPath must be an origin-relative path (got ${JSON.stringify(path)})`, + ); + } + return path; +} + /** * The `WWW-Authenticate` challenge sent with every 401. * @@ -219,42 +237,36 @@ function setupMetadataEndpoints( if (mode === "combined") { // OAuth Authorization Server Metadata (local AS) - app.get( - "/.well-known/oauth-authorization-server", - (req: Request, res: Response) => { - const requestBaseUrl = `${req.protocol}://${req.get("host")}`; - const actualIssuerUrl = config.issuerUrl ?? new URL(requestBaseUrl); - const metadata = { - // RFC 8414 §3.3: the issuer MUST be identical to the base URL the - // well-known path was appended to — i.e. no trailing slash. SDK v2's - // client enforces this exactly (IssuerMismatchError otherwise). - issuer: actualIssuerUrl.href.replace(/\/$/, ""), - authorization_endpoint: new URL("/oauth/authorize", actualIssuerUrl) + app.get(asMetadataPath(config), (req: Request, res: Response) => { + const requestBaseUrl = `${req.protocol}://${req.get("host")}`; + const actualIssuerUrl = config.issuerUrl ?? new URL(requestBaseUrl); + const metadata = { + // RFC 8414 §3.3: the issuer MUST be identical to the base URL the + // well-known path was appended to — i.e. no trailing slash. SDK v2's + // client enforces this exactly (IssuerMismatchError otherwise). + issuer: actualIssuerUrl.href.replace(/\/$/, ""), + authorization_endpoint: new URL("/oauth/authorize", actualIssuerUrl) + .href, + token_endpoint: new URL("/oauth/token", actualIssuerUrl).href, + scopes_supported: scopes, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["client_secret_basic", "none"], + // RFC 9207 / SEP-2468: advertise iss on authorization responses so + // clients must validate (and our e2e can exercise reject paths). + authorization_response_iss_parameter_supported: true, + ...(config.supportDCR && { + registration_endpoint: new URL("/oauth/register", actualIssuerUrl) .href, - token_endpoint: new URL("/oauth/token", actualIssuerUrl).href, - scopes_supported: scopes, - response_types_supported: ["code"], - grant_types_supported: ["authorization_code", "refresh_token"], - code_challenge_methods_supported: ["S256"], - token_endpoint_auth_methods_supported: [ - "client_secret_basic", - "none", - ], - // RFC 9207 / SEP-2468: advertise iss on authorization responses so - // clients must validate (and our e2e can exercise reject paths). - authorization_response_iss_parameter_supported: true, - ...(config.supportDCR && { - registration_endpoint: new URL("/oauth/register", actualIssuerUrl) - .href, - }), - ...(config.supportCIMD && { - client_id_metadata_document_supported: true, - }), - }; + }), + ...(config.supportCIMD && { + client_id_metadata_document_supported: true, + }), + }; - res.json(metadata); - }, - ); + res.json(metadata); + }); } // OAuth Protected Resource Metadata. `resourceMetadataPath` moves the From 1db1b9be1f05b43e45e73ec04e67f99c8efafadd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 01:11:51 -0400 Subject: [PATCH 2/5] fix(auth): cover the CLI refresh path and tighten the well-known match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings. The stored-token refresh path (`refreshStoredAuthToken`) calls SDK discovery directly rather than through `InspectorClient.effectiveAuthFetch`, so a state file holding a refresh token and client information but no `serverMetadata` still hit the upstream OIDC-schema failure and could not refresh against the very servers this change is for. Its default `discover` now wraps the global fetch with the compat shim; a caller-supplied `fetchFn` is left alone. Covered by a test that injects no `discover` — injecting one would bypass the default under test — against a real server that 404s the RFC 8414 path and serves plain RFC 8414 metadata at the appended OIDC path. `oidcDiscoveryCandidates` matched the well-known path by bare prefix, so it also claimed `/.well-known/oauth-authorization-server-backup` and would have replaced that path's failed response with a document fetched from a derived URL. It now requires the exact path or a `/` boundary, which is precisely the set `buildDiscoveryUrls` can emit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/cli/__tests__/stored-auth.test.ts | 83 +++++++++++++++++++ clients/cli/src/cli.ts | 17 +++- .../core/auth/oidcDiscoveryCompat.test.ts | 10 +++ core/auth/oidcDiscoveryCompat.ts | 7 ++ 4 files changed, 116 insertions(+), 1 deletion(-) diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 3fb5c0b188..bea1ad4994 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -873,3 +873,86 @@ describe("--wait-for-auth", () => { expect(result.stderr).toContain("positive number of seconds"); }); }); + +/** + * The stored-token refresh path calls SDK discovery directly rather than + * through `InspectorClient.effectiveAuthFetch`, so it carries its own copy of + * the #2172 compatibility wrapper. This test injects no `discover`, because + * the wrapper lives in the *default* — injecting one would bypass exactly what + * is under test. + */ +describe("refreshStoredAuthToken discovery compatibility (#2172)", () => { + const REFRESHED = { + access_token: "refreshed-access-token", + token_type: "Bearer", + refresh_token: "rotated-refresh-token", + expires_in: 3600, + }; + + it("refreshes against an AS publishing RFC 8414 metadata at the OIDC path", async () => { + let base = ""; + let rfc8414Probes = 0; + const server: Server = createServer((req, res) => { + const path = req.url ?? ""; + if (path.startsWith("/.well-known/oauth-authorization-server")) { + rfc8414Probes += 1; + res.writeHead(404).end(); + return; + } + // Plain OAuth 2.0: no jwks_uri, no subject_types_supported, no + // id_token_signing_alg_values_supported — the shape the SDK rejects when + // it finds it under this filename. + if (path === "/mcp/.well-known/openid-configuration") { + res.writeHead(200, { "content-type": "application/json" }); + res.end( + JSON.stringify({ + issuer: `${base}/mcp`, + authorization_endpoint: `${base}/oauth/authorize`, + token_endpoint: `${base}/oauth/token`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + token_endpoint_auth_methods_supported: ["client_secret_post"], + }), + ); + return; + } + if (path === "/oauth/token") { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify(REFRESHED)); + return; + } + res.writeHead(404).end(); + }); + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + const addr = server.address(); + base = + typeof addr === "object" && addr ? `http://127.0.0.1:${addr.port}` : ""; + const serverUrl = `${base}/mcp`; + const fixture = writeOAuthFixture({ + [serverUrl]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid", client_secret: "sec" }, + }, + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + await expect(refreshStoredAuthToken(serverUrl, fixture)).resolves.toBe( + "refreshed-access-token", + ); + // The RFC 8414 location really was tried and really did 404, so the + // document could only have come from the OIDC path. + expect(rfc8414Probes).toBeGreaterThan(0); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("/mcp/.well-known/openid-configuration"), + ); + } finally { + warn.mockRestore(); + rmSync(dirname(fixture), { recursive: true, force: true }); + await new Promise((resolve) => { + server.close(() => resolve()); + }); + } + }); +}); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index af4bed16bb..6561e67b72 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -52,6 +52,7 @@ import { getAuthorizationServerUrl, getAuthorizationServerUrlCandidates, } from "@inspector/core/auth/discovery.js"; +import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, @@ -329,7 +330,21 @@ export async function refreshStoredAuthToken( deps: RefreshStoredAuthDeps = {}, ): Promise { const refresh = deps.refresh ?? refreshAuthorization; - const discover = deps.discover ?? discoverAuthorizationServerMetadata; + // #2172: this path calls SDK discovery directly rather than through + // `InspectorClient.effectiveAuthFetch`, so it needs the same compatibility + // 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); + 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, + })); const snapshot = await readOAuthSnapshot(statePath); const servers = snapshot.servers as StoredServers; diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 88ef8d208f..87500e0c53 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -75,6 +75,16 @@ describe("oidcDiscoveryCandidates", () => { it("returns nothing for an unparseable URL", () => { expect(oidcDiscoveryCandidates("not a url")).toEqual([]); }); + + it("does not claim a path that merely shares the well-known prefix", () => { + // A bare prefix match would treat this as a discovery request and replace + // its failed response with a document from a derived URL (Copilot). + expect( + oidcDiscoveryCandidates( + "https://as.example.com/.well-known/oauth-authorization-server-backup", + ), + ).toEqual([]); + }); }); describe("isRfc8414OnlyMetadata", () => { diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 498749ccae..b45d8afcb0 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -115,6 +115,13 @@ export function oidcDiscoveryCandidates(rfc8414Url: string): string[] { } 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]; From 28c7a9ff8a402be3249821a37b3499481f939191 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 01:39:28 -0400 Subject: [PATCH 3/5] fix(auth): cover the transport's own discovery, and only rewrite GETs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more review findings, both real. The shim wrapped `effectiveAuthFetch` only, but the transport is constructed with the base `this.fetchFn`, and the SDK runs its own discovery from inside the transport on the 401/refresh leg — so a reconnect with an existing auth provider could still hit the upstream failure. It now wraps the base fetch, the same seam `withOAuthEndpointOverrides` uses and the only one that reaches both paths. That trades away the previous claim that a captured Network entry shows the real 404: below the trackers, what they record is the substitution. Rather than leave that silently misleading, a substituted response now carries `x-inspector-oauth-metadata-source` naming the URL its body was actually fetched from — the transport builds its tracker internally, so there is no seam above it to install into instead. An integration test captures the fetch the transport is handed and drives it against a real server, since nothing else proves that wiring. The wrapper also keyed on URL shape alone, but nothing stops an authorization or token endpoint from living under `/.well-known/oauth-authorization-server/`; a POST there returning an ordinary OAuth 4xx would have had its error replaced by a metadata document. Metadata discovery is a GET, so anything else is now left alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- AGENTS.md | 13 +++-- README.md | 2 +- .../core/auth/oidcDiscoveryCompat.test.ts | 24 ++++++++ .../auth/rfc8414AtOidcPath.test.ts | 50 ++++++++++++++++- core/auth/oidcDiscoveryCompat.ts | 56 +++++++++++++++++-- core/mcp/inspectorClient.ts | 14 +++-- 6 files changed, 141 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b2ee092107..28a290e20a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,10 +135,15 @@ v2/main/ │ │ # one is RFC 8414 metadata that is not a │ │ # valid OIDC document, serves that body as │ │ # the RFC 8414 response so the SDK picks the -│ │ # right schema. Sits ABOVE the fetch tracker -│ │ # (unlike endpointOverrides, which sits -│ │ # below) so the Network tab records the real -│ │ # 404 and the real probe — #2172; +│ │ # right schema. GET-only, and the well-known +│ │ # match requires a `/` boundary, so a token +│ │ # endpoint living under that prefix keeps its +│ │ # own error. Sits on the BASE fetch like +│ │ # endpointOverrides — the one seam that also +│ │ # covers the discovery the SDK runs inside the +│ │ # transport — so the substituted response +│ │ # carries COMPAT_SOURCE_HEADER naming the URL +│ │ # its body came from — #2172; │ │ # secret-storage-info.ts browser-safe │ │ # descriptor of WHERE a typed secret lands │ │ # — kind/plaintext/durable plus the label, diff --git a/README.md b/README.md index e56dc04549..baab107615 100644 --- a/README.md +++ b/README.md @@ -453,7 +453,7 @@ The cause is upstream. `discoverAuthorizationServerMetadata` in `@modelcontextpr `core/auth/oidcDiscoveryCompat.ts` works around it without fabricating anything. When the RFC 8414 candidate comes back 4xx, it fetches the OIDC candidates the SDK would try next; if one returns a document that satisfies `OAuthMetadataSchema` but *fails* the OIDC schema, that document is returned as the response to the RFC 8414 request — so the SDK validates it under the schema that actually describes it. A genuine OpenID provider document is left alone and takes the SDK's normal OIDC leg. Issuer validation is untouched, since the substituted document is the one the server published, `issuer` included. -Watch the Network tab to confirm the Inspector is not hiding anything: the real 404 on `/.well-known/oauth-authorization-server` and the real request to `/.well-known/openid-configuration` are both recorded, and the metadata shown in the Auth tab is exactly what the server sent — no invented `jwks_uri`. The wrapper sits above the fetch tracker for that reason. +The metadata shown in the Auth tab is exactly what the server sent — no invented `jwks_uri`. In the Network tab the substituted response is captured against the RFC 8414 URL (the wrapper sits on the base fetch, below the tracker, because that is the only seam that also covers the discovery the SDK runs from inside the transport), so it carries an `x-inspector-oauth-metadata-source` response header naming the URL its body was actually fetched from. The same URL is printed as a console warning. #### Logging, both eras diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 87500e0c53..6e85562947 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, afterEach } from "vitest"; import { buildDiscoveryUrls } from "@modelcontextprotocol/client"; import { + COMPAT_SOURCE_HEADER, oidcDiscoveryCandidates, isRfc8414OnlyMetadata, withRfc8414OidcCompat, @@ -144,11 +145,34 @@ describe("withRfc8414OidcCompat", () => { expect(response.status).toBe(200); expect(response.headers.get("content-type")).toBe("application/json"); await expect(response.json()).resolves.toEqual(RFC8414_DOC); + // The substitution names where its body came from, so a captured Network + // entry does not read as a 200 from the RFC 8414 path. + expect(response.headers.get(COMPAT_SOURCE_HEADER)).toBe(OIDC_APPENDED); // The failed original, then both OIDC candidates. expect(base).toHaveBeenCalledTimes(3); expect(warn).toHaveBeenCalledWith(expect.stringContaining(OIDC_APPENDED)); }); + it("does not touch a non-GET request to a matching path", async () => { + // An authorization or token endpoint may legally live under the RFC 8414 + // well-known prefix; its ordinary OAuth error must reach the caller intact + // rather than being replaced with a metadata document (Copilot). + const original = new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, + headers: { "content-type": "application/json" }, + }); + const base = vi.fn().mockResolvedValue(original); + const wrapped = withRfc8414OidcCompat(base); + + await expect( + wrapped(`${RFC8414_URL}/token`, { method: "post" }), + ).resolves.toBe(original); + await expect( + wrapped(new Request(`${RFC8414_URL}/token`, { method: "POST" })), + ).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(2); + }); + it("serves an RFC 8414 document found on the path-suffixed OIDC path", async () => { vi.spyOn(console, "warn").mockImplementation(() => {}); const base = vi.fn(async (input) => { diff --git a/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts index d2c0e08443..cbd06e52df 100644 --- a/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts +++ b/clients/web/src/test/integration/auth/rfc8414AtOidcPath.test.ts @@ -17,7 +17,12 @@ import { createOAuthTestServerConfig, waitForOAuthWellKnown, } from "@modelcontextprotocol/inspector-test-server"; -import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; +import { + COMPAT_SOURCE_HEADER, + withRfc8414OidcCompat, +} from "@inspector/core/auth/oidcDiscoveryCompat.js"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; const OIDC_PATH = "/.well-known/openid-configuration"; @@ -37,7 +42,9 @@ describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => { ...getDefaultServerConfig(), serverType: "streamable-http" as const, ...createOAuthTestServerConfig({ - requireAuth: true, + // Metadata routes are served either way; leaving auth unrequired lets + // the transport-wiring test below actually connect. + requireAuth: false, asMetadataPath: OIDC_PATH, }), }); @@ -97,4 +104,43 @@ describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => { warn.mockRestore(); } }); + + it("hands the transport a fetch that carries the shim", async () => { + // The SDK also runs discovery from *inside* the transport, which receives + // `InspectorClient`'s base fetch directly — so a reconnect with an existing + // auth provider must not bypass the shim (Copilot). Capturing the fetch the + // transport was handed and driving it against this server is what proves + // the wiring, without depending on the SDK to trigger that leg. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + let transportFetch: typeof fetch | undefined; + const capturingTransport: typeof createTransportNode = ( + config, + options, + ) => { + transportFetch = options?.fetchFn; + return createTransportNode(config, options); + }; + const client = new InspectorClient( + { type: "streamable-http", url: `${serverUrl}/mcp` }, + { environment: { transport: capturingTransport } }, + ); + try { + await client.connect(); + expect(transportFetch).toBeDefined(); + + const response = await transportFetch!( + `${serverUrl}/.well-known/oauth-authorization-server`, + ); + expect(response.status).toBe(200); + expect(response.headers.get(COMPAT_SOURCE_HEADER)).toBe( + `${serverUrl}${OIDC_PATH}`, + ); + await expect(response.json()).resolves.toMatchObject({ + issuer: serverUrl, + }); + } finally { + await client.disconnect(); + warn.mockRestore(); + } + }); }); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index b45d8afcb0..5f1fa93ff5 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -68,10 +68,18 @@ * 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 hide the real traffic.** It is installed *above* the fetch - * tracker (`InspectorClient.effectiveAuthFetch`), so the Network tab records - * the genuine 404 on the RFC 8414 path and the genuine request to the OIDC - * path. The substitution is visible only to the SDK's parse. + * - **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 * @@ -93,6 +101,14 @@ 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. @@ -196,6 +212,21 @@ function discoveryHeaders( 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. @@ -209,6 +240,13 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { 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; @@ -252,7 +290,15 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { return new Response(body, { status: 200, statusText: "OK", - headers: { "content-type": "application/json" }, + 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]: candidate, + }, }); } diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 28cffb6bc0..1fa1a9ebda 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -795,12 +795,14 @@ export class InspectorClient extends InspectorClientEventTarget { // #2172: recover discovery when a plain OAuth 2.0 authorization server // publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which // the SDK validates as an OpenID provider document and rejects. Wraps the - // tracked fetch rather than the base one — the opposite of the overrides - // above — so the Network tab records the real 404 and the real probe rather - // than the substitution the SDK's parse sees. - this.effectiveAuthFetch = withRfc8414OidcCompat( - this.buildEffectiveAuthFetch(), - ); + // *base* fetch for the same reason the overrides above do: the SDK also + // runs discovery from inside the transport, which is handed `this.fetchFn` + // directly, so a reconnect with an existing auth provider would otherwise + // still hit the upstream failure. The substituted response is stamped with + // `COMPAT_SOURCE_HEADER` so a captured entry names the URL its body came + // from rather than appearing to be a 200 from the RFC 8414 path. + this.fetchFn = withRfc8414OidcCompat(this.fetchFn); + this.effectiveAuthFetch = this.buildEffectiveAuthFetch(); this.sessionId = options.sessionId; From 89d6af4436f168c3bee7c99ef53c1f2a45dc1c12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 01:59:04 -0400 Subject: [PATCH 4/5] fix(auth): mirror the SDK's candidate policy in the compat probe loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe loop advanced to the next OIDC candidate on any failure, which meant it could promote a later candidate over an earlier one the SDK would have stopped at — turning a failure into a success, a worse defect than the one being worked around. Four distinct cases: a 500 (an outage the SDK surfaces), a 2xx body that will not parse (terminal for the SDK), a network error (which the SDK propagates outside the browser's CORS case), and a non-JSON media type. It now continues only where the SDK's own loop does — a 4xx or 502 — and otherwise returns the original response, leaving the SDK to make the same request and reach its own verdict. The `content-type` gate is gone entirely, because the SDK parses a 2xx discovery body whatever media type it carries, so gating could skip a genuine OpenID provider document and substitute an RFC-8414-only one from a lower-priority candidate in its place. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 48 ++++++++++++++----- core/auth/oidcDiscoveryCompat.ts | 42 ++++++++-------- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 6e85562947..47aaac15b0 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -203,17 +203,17 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(2); }); - it("skips a candidate that is not JSON and keeps looking", async () => { + it("accepts a 2xx candidate whatever media type it carries", async () => { + // The SDK parses a 2xx discovery body regardless of `content-type`, so + // gating on it here could skip a document the SDK would have used (Copilot). vi.spyOn(console, "warn").mockImplementation(() => {}); const base = vi.fn(async (input) => { - const url = String(input); - if (url === OIDC_SUFFIXED) { - return new Response("login", { + if (String(input) === OIDC_SUFFIXED) { + return new Response(JSON.stringify(RFC8414_DOC), { status: 200, - headers: { "content-type": "text/html" }, + headers: { "content-type": "text/plain" }, }); } - if (url === OIDC_APPENDED) return json(RFC8414_DOC); return notFound(); }); const wrapped = withRfc8414OidcCompat(base); @@ -223,10 +223,12 @@ describe("withRfc8414OidcCompat", () => { ); }); - it("skips a candidate whose JSON body will not parse", async () => { + it("stops at a candidate whose body will not parse", async () => { + // Terminal for the SDK, so a later candidate must not be promoted over it + // — that would turn a failure into a success (Copilot). const original = notFound(); const base = vi.fn(async (input) => { - if (String(input).includes("openid-configuration")) { + if (String(input) === OIDC_SUFFIXED) { return new Response("{ not json", { status: 200, headers: { "content-type": "application/json" }, @@ -237,10 +239,12 @@ describe("withRfc8414OidcCompat", () => { const wrapped = withRfc8414OidcCompat(base); await expect(wrapped(RFC8414_URL)).resolves.toBe(original); - expect(base).toHaveBeenCalledTimes(3); + // The failed original and the first candidate only — the second is not + // reached. + expect(base).toHaveBeenCalledTimes(2); }); - it("skips a candidate that itself fails", async () => { + it("walks past a candidate that 404s, as the SDK does", async () => { const original = notFound(); const base = vi.fn().mockResolvedValue(original); const wrapped = withRfc8414OidcCompat(base); @@ -249,7 +253,27 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(3); }); - it("treats a probe that throws as a candidate to skip", async () => { + it("stops at a candidate returning a status the SDK would not walk past", async () => { + // A 500 on the first OIDC candidate is an outage the SDK surfaces; hiding + // it behind a second candidate's document would be worse than the bug + // being worked around (Copilot). + const original = notFound(); + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_SUFFIXED) return new Response("boom", { status: 500 }); + if (url === OIDC_APPENDED) return json(RFC8414_DOC); + return original; + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(2); + }); + + it("stops when a probe throws", async () => { + // The SDK swallows only the browser's CORS `TypeError`; elsewhere a network + // error propagates. Leave it to the SDK to make the same request and reach + // its own verdict (Copilot). const original = notFound(); const base = vi.fn(async (input) => { if (String(input).includes("openid-configuration")) { @@ -260,7 +284,7 @@ describe("withRfc8414OidcCompat", () => { const wrapped = withRfc8414OidcCompat(base); await expect(wrapped(RFC8414_URL)).resolves.toBe(original); - expect(base).toHaveBeenCalledTimes(3); + expect(base).toHaveBeenCalledTimes(2); }); it("probes on a 502, which the SDK also walks past", async () => { diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 5f1fa93ff5..19b8014151 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -160,19 +160,6 @@ function continuesDiscovery(status: number): boolean { return (status >= 400 && status < 500) || status === 502; } -/** - * Whether a `content-type` names a whole JSON document. - * - * An exact media-type test rather than a substring search for "json", for the - * same reason `endpointOverrides.ts` uses one: a streaming media type would be - * read to completion and never resolve. - */ -function isJsonDocumentResponse(contentType: string | null): boolean { - if (!contentType) return false; - const mediaType = contentType.split(";")[0].trim().toLowerCase(); - return mediaType === "application/json" || mediaType.endsWith("+json"); -} - /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -252,26 +239,41 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { if (candidates.length === 0) return response; const headers = discoveryHeaders(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) { let probe: Response; try { probe = await fetchFn(candidate, { headers }); } catch { - // A network or CORS failure on a URL the SDK had not asked for yet is - // not this wrapper's to report — the SDK will make the same request and - // handle it. Try the next candidate. - continue; + return response; + } + if (!probe.ok) { + if (continuesDiscovery(probe.status)) continue; + return response; } - if (!probe.ok) continue; - if (!isJsonDocumentResponse(probe.headers.get("content-type"))) continue; + // 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 { - continue; + return response; } if (!isRfc8414OnlyMetadata(parsed)) { // Either not metadata at all, or a genuine OpenID provider document the From 039fc6f02e0c1fb987e440854fde53ed30ee7ce3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 02:35:27 -0400 Subject: [PATCH 5/5] 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 6561e67b72..14f51ab93e 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 47aaac15b0..e3bf9d4de1 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 19b8014151..bb30e72226 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, }, }); }