From 084a083e2db40ec60c6071e5f4ac042f8ed53fde Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:54:17 -0400 Subject: [PATCH] fix: usable IPv6 spelling, and drop a SEP label that means something else here (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The recovery text told users to move the endpoint to `::1`. A bare IPv6 literal is not a legal URL host — `new URL("http://::1/token")` throws — so anyone copying that into the Token URL override got a parse error from advice meant to unblock them. Now `[::1]`, in both the notice and the manual reproduction guide. The exemption list still reads `::1`, because that is the host the SDK compares; only the remedy is bracketed, because that is what a user types. The comment says so, so neither gets "corrected" into the other. - Dropped SEP-2207 from all nine files it had reached. I took the label from the SDK's own source comments, which attribute this check to it — but this repo already uses SEP-2207 for OIDC refresh / offline_access (specification/v2_auth_hardening.md, plus an e2e test), so grepping it here would have turned up two unrelated things. Replaced with plain description rather than a different SEP number: having guessed wrong once, guessing again is not the fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .claude/skills/test-servers/SKILL.md | 2 +- .../web/src/hooks/useConnectionLifecycle.test.tsx | 4 ++-- clients/web/src/hooks/useConnectionLifecycle.ts | 8 ++++---- clients/web/src/hooks/useOAuthRecovery.test.tsx | 4 ++-- clients/web/src/hooks/useOAuthRecovery.ts | 8 ++++---- clients/web/src/lib/insecureTokenEndpointNotice.ts | 2 +- clients/web/src/test/core/auth/oauthUx.test.ts | 3 +++ core/auth/insecureTokenEndpoint.ts | 2 +- core/auth/oauthUx.ts | 12 +++++++++--- docs/test-servers.md | 6 +++--- 10 files changed, 30 insertions(+), 21 deletions(-) diff --git a/.claude/skills/test-servers/SKILL.md b/.claude/skills/test-servers/SKILL.md index 90051f4a..58da2bd5 100644 --- a/.claude/skills/test-servers/SKILL.md +++ b/.claude/skills/test-servers/SKILL.md @@ -83,7 +83,7 @@ usually looks like a missing capability rather than an error. | A tool result's `structuredContent` section | `structured-output-http.json` (legacy) | | RFC 6570 resource-template expansion | `rfc6570-templates-http.json` | | OAuth token revocation on clear | `oauth-revocation-http.json` (legacy) | -| A token endpoint the SDK refuses (SEP-2207) | `oauth-insecure-token-endpoint-http.json` (legacy) | +| A token endpoint the SDK refuses | `oauth-insecure-token-endpoint-http.json` (legacy) | | Cancelling a call mid-flight | `cancellation-modern-http.json` (modern) | ## Adding a config or preset diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index a016d3a8..1acf79ce 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -262,7 +262,7 @@ const lastClient = (h: Harness): InspectorClient => { /** * The last updater handed to `setReAuthBanner`, applied to a banner. * - * Every terminal SEP-2207 arm clears the banner with a **functional** update + * Every terminal arm clears the banner with a **functional** update * guarded on `serverId`, because these paths are asynchronous and a late * continuation for one server must not erase a banner another raised in the * meantime. The harness's setter is a spy, so the updater is never invoked for @@ -624,7 +624,7 @@ describe("useConnectionLifecycle", () => { }); it("reports an insecure token endpoint as terminal, without flagging the card", async () => { - // SEP-2207 (#2280). Asserted on the hook, not just the notice helper, + // The terminal token-endpoint refusal (#2280). Asserted on the hook, not just the notice helper, // because what makes this arm correct is its *position*: above // `setFailedServerId` and above the generic toast. A helper-only test // cannot see either of those go wrong. diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 2fb94f19..68fe2840 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -640,7 +640,7 @@ export function useConnectionLifecycle({ }); return; } - // SEP-2207 (#2280): a token endpoint the SDK will not post credentials + // A token endpoint the SDK will not post credentials // to. Terminal, so it gets a notice of its own rather than the generic // "Failed to connect" toast, whose detail line would be the raw SDK // text. @@ -698,7 +698,7 @@ export function useConnectionLifecycle({ // held. The fetch log survives a disconnect, so the Network // diagnostics this issue is about are unaffected. await client.disconnect().catch(() => {}); - // SEP-2207 (#2280). The retried `connect()` above can raise the + // The terminal token-endpoint refusal (#2280). The retried `connect()` above can raise the // terminal refusal on its own — a satisfied challenge still ends in // a token exchange — and reporting that as a failed connect attempt // is doubly wrong here: the card goes red and the message is the @@ -765,7 +765,7 @@ export function useConnectionLifecycle({ }); return; } - // See the SEP-2207 note on the handshake arm above (#2280). The + // See the note on the handshake arm above (#2280). The // disconnect already happened at the top of this catch. if (showInsecureTokenEndpointNotice(authErr, target.name)) { setReAuthBanner((prev) => @@ -1014,7 +1014,7 @@ export function useConnectionLifecycle({ authorizationUrl: authUrl, }); } catch (err) { - // SEP-2207 (#2280), and this is the path a user reaches by *acting*: + // The terminal token-endpoint refusal (#2280), on the path a user reaches by *acting*: // a connected session raises an ordinary re-auth banner, they click // Re-authenticate, and the token exchange is refused. Without this // the generic toast below reports it with the raw SDK text — the diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index dde7cd00..c6c186eb 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -981,7 +981,7 @@ describe("useOAuthRecovery", () => { }); it("claims an insecure token endpoint on the command path instead of rethrowing", async () => { - // SEP-2207 (#2280). A mid-session silent refresh rejects here rather than + // The terminal token-endpoint refusal (#2280). A mid-session silent refresh rejects here rather than // as an AuthRecoveryRequiredError, so before this it was rethrown into // the generic reporting below. const client = fakeClient(); @@ -1876,7 +1876,7 @@ describe("useOAuthRecovery", () => { }); it("reports an insecure token endpoint terminally, with no banner and no red card", async () => { - // SEP-2207 (#2280). The three assertions are the whole point of the arm's + // The terminal token-endpoint refusal (#2280). The three assertions are the whole point of the arm's // position: the banner would carry a Re-authenticate button that cannot // work, and flagging the card would present a configuration error as a // failed connect attempt. diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 1207c076..329e6f0f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -392,7 +392,7 @@ export function useOAuthRecovery({ ); /** - * Report a terminal SEP-2207 refusal (#2280) and clear any re-auth banner. + * Report a terminal terminal token-endpoint refusal (#2280) and clear any re-auth banner. * * Every arm goes through this rather than calling the notice helper directly. * The banner clear is not incidental: a banner left from an *earlier* failure @@ -433,7 +433,7 @@ export function useOAuthRecovery({ options?: { reason?: AuthChallengeReason }, ) => { const server = sessionRef.current.servers.find((s) => s.id === serverId); - // SEP-2207 (#2280). The SDK rethrows `InsecureTokenEndpointError` instead + // The terminal token-endpoint refusal (#2280). The SDK rethrows `InsecureTokenEndpointError` instead // of retrying, so the banner's "Re-authenticate" could only fail the same // way. Claimed here, at the single funnel every re-auth banner goes // through, rather than at each of its call sites — a new caller then gets @@ -862,7 +862,7 @@ export function useOAuthRecovery({ } return undefined; } - // SEP-2207 (#2280), on the command path. A mid-session silent refresh + // The terminal token-endpoint refusal (#2280), on the command path. A mid-session silent refresh // against an unusable token endpoint rejects here rather than as an // `AuthRecoveryRequiredError`, so without this it is rethrown and lands // in `runCommandInBackground` — which either shows the raw SDK text @@ -1000,7 +1000,7 @@ export function useOAuthRecovery({ }); } } catch (err) { - // SEP-2207 (#2280) first, and specifically BEFORE the restore below. + // The terminal token-endpoint refusal (#2280) first, and specifically BEFORE the restore below. // `handleAuthChallenge` runs the same SDK auth flow, so it can raise // this terminal error — and the restore's whole premise is that the // recovery is still owed and a later trigger should retry it. For a diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index 932b0945..f5c6c250 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -1,5 +1,5 @@ /** - * Surfaces the SDK's SEP-2207 refusal to post credentials to a non-TLS token + * Surfaces the SDK's terminal token-endpoint refusal to post credentials to a non-TLS token * endpoint as the terminal configuration error it is (#2280). * * Lives in `lib/` rather than `utils/` because showing a notification is a side diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index 439fdeb0..4126264f 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -467,6 +467,9 @@ describe("insecureTokenEndpoint copy", () => { expect(message).toContain(ENDPOINT); expect(message).toContain("HTTPS"); expect(message).toContain("127.0.0.1"); + // Bracketed: a bare IPv6 literal is not a legal URL host, so `::1` copied + // into the Token URL override would not parse. + expect(message).toContain("[::1]"); expect(message).toContain("Token URL override"); // The section name the UI actually renders. Sending someone to a settings // section that does not exist is the worst error this message could make. diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts index 9dfa6717..0374a54a 100644 --- a/core/auth/insecureTokenEndpoint.ts +++ b/core/auth/insecureTokenEndpoint.ts @@ -1,5 +1,5 @@ /** - * SEP-2207: the SDK refuses to send credentials to a non-TLS token endpoint + * The SDK refuses to send credentials to a non-TLS token endpoint * whose host is outside its loopback exemption (`localhost` / `127.0.0.1` / * `::1`), throwing `InsecureTokenEndpointError` from inside * `executeTokenRequest`. diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 7578187d..8b5167ad 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -364,7 +364,7 @@ export function reAuthBannerMessage(options: { } /** - * Heading for the SDK's SEP-2207 refusal to post credentials to a non-TLS token + * Heading for the SDK's terminal token-endpoint refusal to post credentials to a non-TLS token * endpoint (#2280). * * Deliberately not phrased as an authentication failure. Like @@ -389,6 +389,12 @@ export function insecureTokenEndpointTitle(): string { * legitimately sent earlier in the session, and a user who had been connected * for an hour would rightly read it as describing a different failure. * + * The exemption is listed as `::1` (that is the host the SDK compares) but the + * remedy says `[::1]`, because that is what a user must actually type: a bare + * IPv6 literal is not a legal URL host and `new URL("http://::1/token")` + * throws. The two spellings are deliberately different — do not "fix" either + * into the other. + * * The section name is the one the UI actually renders — **OAuth Settings**, * with a **Token URL override** field — not "Authorization". Sending someone to * a settings section that does not exist is the worst possible error in the one @@ -418,8 +424,8 @@ export function insecureTokenEndpointMessage(options: { `its token endpoint ${truncateUrlForDisplay(options.tokenEndpoint)} is ` + "not HTTPS, and its host is outside the MCP SDK's loopback exemption, " + "which covers only localhost, 127.0.0.1 and ::1. Re-authenticating cannot " + - "change this. Serve the token endpoint over HTTPS, or move it to one of " + - "those three spellings — Server Settings → OAuth Settings has a " + + "change this. Serve the token endpoint over HTTPS, or move it to " + + "localhost, 127.0.0.1 or [::1] — Server Settings → OAuth Settings has a " + '"Token URL override" if the authorization server advertises a different ' + "one." ); diff --git a/docs/test-servers.md b/docs/test-servers.md index f8cf1c63..b2300394 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -54,7 +54,7 @@ as a missing capability rather than an error. | `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-revocation-http.json` / `oauth-no-revocation-http.json` **(legacy era)** | RFC 7009 token revocation on clear, with and without a `revocation_endpoint` | [#2144](https://github.com/modelcontextprotocol/inspector/issues/2144) | | `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) | -| `oauth-insecure-token-endpoint-http.json` **(legacy era)** | A token endpoint the SDK refuses to post credentials to (SEP-2207) | [#2280](https://github.com/modelcontextprotocol/inspector/issues/2280) | +| `oauth-insecure-token-endpoint-http.json` **(legacy era)** | A token endpoint the SDK refuses to post credentials to | [#2280](https://github.com/modelcontextprotocol/inspector/issues/2280) | | `logging-{legacy,modern}-http.json` **(era per file)** | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` **(era per file)** | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` **(modern era)** | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | @@ -455,7 +455,7 @@ 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. -## A token endpoint the SDK will not use (SEP-2207) +## A token endpoint the SDK will not use `oauth-insecure-token-endpoint-http.json` is an ordinary combined AS + resource server with one thing changed: `oauth.issuerUrl` is `http://localhost.:8091`, so its advertised `token_endpoint` is `http://localhost.:8091/oauth/token`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. @@ -465,7 +465,7 @@ The trailing dot is the whole trick, and it is doing real work rather than being Add the server, click **Connect**, and complete the authorization. The redirect comes back with a code, the Inspector goes to exchange it, and the SDK refuses. -What you should see is a red, non-expiring **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or move it to one of the three host spellings the SDK exempts (`localhost`, `127.0.0.1`, `::1`). It stays until you close it (`autoClose: false` stops it expiring on a timer; Mantine's own close control still dismisses it, which is what you want for a message you have finished reading). There is deliberately **no** action button. +What you should see is a red, non-expiring **"Token endpoint is not secure"** notification naming the endpoint and the two things that resolve it — serve it over HTTPS, or move it to one of the three hosts the SDK exempts — `localhost`, `127.0.0.1`, or `[::1]` (bracketed, since a bare IPv6 literal is not a legal URL host). It stays until you close it (`autoClose: false` stops it expiring on a timer; Mantine's own close control still dismisses it, which is what you want for a message you have finished reading). There is deliberately **no** action button. Note the second option is phrased as a *spelling* change, not a networking one. `localhost.` already **is** loopback, and so is `tenant.app.localhost`; what they are outside is a three-literal allow-list. Telling a reader to "use a loopback host" when they demonstrably already are is what sends them off to debug their resolver instead of their configuration.