From b4b82493b1e92b5c36e8cdfd3bbd10b1c757d1aa Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:09:54 -0400 Subject: [PATCH 1/7] fix: report a terminal OAuth token-endpoint refusal instead of a dead retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2280 InsecureTokenEndpointError appeared nowhere in our source, so it fell through to the generic auth-failure path and rendered a "Re-authentication required" banner with a Re-authenticate button. That button can never work: the SDK's assertSecureTokenEndpoint runs inside executeTokenRequest, the error does not extend OAuthError, and auth() special-cases it to rethrow rather than start a fresh /authorize redirect. Clicking re-ran the same flow to the same refusal, under the raw SDK message, which names the three exempt host literals and nothing actionable. A terminal configuration error was being presented as a retryable auth error. Not specific to *.localhost: it fires for any endpoint outside the SDK's exemption — host.docker.internal (#1911), a LAN hostname, a reverse-proxy name, a mistyped scheme. It is now recognized (core/auth/insecureTokenEndpoint.ts, mirroring issuerBinding.ts's brand-plus-name classifier and walking cause / data.cause, since era negotiation and the transport wrappers bury the rejection) and surfaced as the configuration error it is, naming the endpoint and both ways out, with no action affordance. Six paths reach the refusal and all six classify it — the connect handshake, authenticate() during connect, the satisfied-challenge connect retry, the post-redirect callback, the command/background path, the deferred tab-visible resume (which re-armed on every focus, an unbounded loop on a terminal error), and the banner action. They share one wrapper so a seventh cannot omit the banner clear, and every clear is scoped to its own serverId: these paths are asynchronous, so a late continuation for one server must not erase a banner another raised in the meantime. Adds oauth-insecure-token-endpoint-http.json to reproduce it, whose issuer is http://localhost.:8091 — the root-anchored spelling every resolver sends to loopback but which is none of the SDK's exempt literals. transport.strictPort keeps that fixture honest, since its port is hard-coded inside the issuer. Making such an endpoint work has to land in the SDK (typescript-sdk#2591); this changes the reporting, not the outcome. Split out of #2282, where it was tangled with the *.localhost origin work for #1944 — a separate question the reporter is still clarifying. 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 | 1 + .../src/hooks/useConnectionLifecycle.test.tsx | 177 ++++++++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 64 ++++- .../web/src/hooks/useOAuthRecovery.test.tsx | 228 ++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 109 ++++++++- .../lib/insecureTokenEndpointNotice.test.ts | 57 +++++ .../src/lib/insecureTokenEndpointNotice.ts | 59 +++++ .../core/auth/insecureTokenEndpoint.test.ts | 124 ++++++++++ .../web/src/test/core/auth/oauthUx.test.ts | 56 +++++ .../test/integration/mcp/strict-port.test.ts | 228 ++++++++++++++++++ clients/web/src/utils/oauthUx.ts | 2 + core/auth/insecureTokenEndpoint.ts | 107 ++++++++ core/auth/oauthUx.ts | 61 ++++- docs/test-servers.md | 19 ++ .../oauth-insecure-token-endpoint-http.json | 27 +++ test-servers/src/composable-test-server.ts | 11 + test-servers/src/load-config.ts | 44 ++++ test-servers/src/resolve-config.ts | 1 + test-servers/src/test-server-http.ts | 34 ++- 19 files changed, 1400 insertions(+), 9 deletions(-) create mode 100644 clients/web/src/lib/insecureTokenEndpointNotice.test.ts create mode 100644 clients/web/src/lib/insecureTokenEndpointNotice.ts create mode 100644 clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts create mode 100644 clients/web/src/test/integration/mcp/strict-port.test.ts create mode 100644 core/auth/insecureTokenEndpoint.ts create mode 100644 test-servers/configs/oauth-insecure-token-endpoint-http.json diff --git a/.claude/skills/test-servers/SKILL.md b/.claude/skills/test-servers/SKILL.md index 16556b18b..90051f4a3 100644 --- a/.claude/skills/test-servers/SKILL.md +++ b/.claude/skills/test-servers/SKILL.md @@ -83,6 +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) | | 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 f09c020d2..a016d3a8a 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -11,6 +11,7 @@ import type { ClientConfig } from "@inspector/core/client/types.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults"; import { DEEP_LINK_SERVER_ID } from "../utils/deepLink"; @@ -258,6 +259,27 @@ const lastClient = (h: Harness): InspectorClient => { return client; }; +/** + * The last updater handed to `setReAuthBanner`, applied to a banner. + * + * Every terminal SEP-2207 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 + * us — asserting it directly is what actually exercises the guard rather than + * merely reaching the line. + */ +const applyBannerUpdate = ( + spy: ReturnType, + banner: { serverId: string; message: string } | null, +) => { + const updater = spy.mock.calls.at(-1)?.[0] as unknown; + if (typeof updater !== "function") { + throw new Error("expected a functional setReAuthBanner update"); + } + return (updater as (prev: unknown) => unknown)(banner); +}; + const toastTitles = (): string[] => notificationsMock.show.mock.calls.map((c) => String(c[0]?.title)); @@ -601,6 +623,128 @@ describe("useConnectionLifecycle", () => { expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); }); + 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, + // 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. + connectSpy.mockRejectedValueOnce( + new InsecureTokenEndpointError( + "http://tenant.app.localhost:3300/token", + ), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + // The generic arm must not also fire — two notifications for one failure + // is how the raw SDK text would creep back in beside the good copy. + expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + // The clear is scoped: it drops this server's banner and spares another's. + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "a", + message: "x", + }), + ).toBeNull(); + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "other", + message: "x", + }), + ).toMatchObject({ serverId: "other" }); + // The real `connect()` sets status `"error"` and dispatches + // `statusChange` before rethrowing, which paints the card red and pins + // the monitoring sidebar open — presenting this as the failed connect + // attempt the notice says it is not. `connect` is mocked here, so the + // teardown is what this asserts; without it the client is left in that + // state. + expect(disconnectSpy).toHaveBeenCalled(); + }); + + it("finds an insecure token endpoint wrapped under `cause` on the connect path", async () => { + // Era negotiation and the transport wrappers bury the rejection, so the + // shallow check this replaced would have missed exactly this shape. + connectSpy.mockRejectedValueOnce( + new Error("connect failed", { + cause: new InsecureTokenEndpointError("http://localhost.:8091/token"), + }), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + expect(disconnectSpy).toHaveBeenCalled(); + }); + + it("reports an insecure token endpoint raised by the 401 authorization attempt", async () => { + // The second of the two arms: `authenticate()` rejects rather than the + // opening handshake, which is the path a refresh takes. + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockRejectedValueOnce( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(toastTitles()).not.toContain( + 'OAuth authorization failed for "Server a"', + ); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + // The generic arm also records this as the connect error banner text; + // the terminal arm returns before that, so it must stay unset. + expect(h.api().connectErrorMessage).toBeUndefined(); + }); + + it("reports a terminal refusal raised by the retried connect, without flagging the card", async () => { + // The satisfied-challenge retry still ends in a token exchange, so it can + // raise this on its own. Reporting it as a failed connect is doubly wrong + // here: the Inspector has just told the user the authorization worked + // (#2280). + connectSpy + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "unauthorized" }, + ), + ) + .mockRejectedValueOnce( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + checkSpy.mockResolvedValueOnce(true); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(connectSpy).toHaveBeenCalledTimes(2); + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(toastTitles()).not.toContain('Failed to connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + expect( + applyBannerUpdate(h.spies.setReAuthBanner, { + serverId: "other", + message: "x", + }), + ).toMatchObject({ serverId: "other" }); + expect(h.api().connectErrorMessage).toBeUndefined(); + // The teardown the generic arm does is still required on this one. + expect(disconnectSpy).toHaveBeenCalled(); + }); + it("retries the connect when the auth challenge is already satisfied", async () => { connectSpy.mockRejectedValueOnce( new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { @@ -1211,6 +1355,39 @@ describe("useConnectionLifecycle", () => { ); }); + it("reports a terminal token-endpoint refusal from the banner action", async () => { + // The path a user reaches by *acting*: an ordinary re-auth banner, they + // click Re-authenticate, and the exchange is refused. The worst place to + // fall back to the raw SDK text, since they have just been told that + // retrying is the fix (#2280). + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + authenticateSpy.mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "a", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(toastTitles()).not.toContain( + 'OAuth authorization failed for "Server a"', + ); + }); + it("falls back to an unnamed failure toast for an unknown server", async () => { const h = harness({ servers: [entry("a")] }); await act(async () => { diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index f7064c0fc..2fb94f19e 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -30,8 +30,11 @@ import { getActiveEnterpriseManagedAuthIdp, } from "@inspector/core/client/types.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; +import type { Dispatch, SetStateAction } from "react"; import type { SessionRef } from "./useSessionRef"; import type { FetchLogOptions } from "./useInspectorStores"; import type { LastPersistedSettings } from "./useLastPersistedSettings"; @@ -172,7 +175,7 @@ export interface UseConnectionLifecycleOptions { prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void; finalizeExplicitDisconnect: () => void; reAuthBanner: ReAuthBannerState | null; - setReAuthBanner: (next: ReAuthBannerState | null) => void; + setReAuthBanner: Dispatch>; /** See `SessionResetSurface`. */ sessionReset: SessionResetSurface; @@ -637,6 +640,30 @@ export function useConnectionLifecycle({ }); return; } + // SEP-2207 (#2280): 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. + // + // The teardown is load-bearing, not tidiness. `connect()` sets its + // status to `"error"` and dispatches `statusChange` *before* rethrowing + // (it is not a connect-auth-recovery error), and `InspectorView` pins + // the monitoring sidebar open on that transition and paints the card + // red. Returning without it would present this as the failed connect + // attempt the notice explicitly says it is not. The `authenticate()` + // arm below already disconnects for the same reason. + if (findInsecureTokenEndpoint(err)) { + await client.disconnect().catch(() => {}); + showInsecureTokenEndpointNotice(err, target.name); + // Clear a banner left by an earlier failure: its Re-authenticate + // button is just as dead as the one this arm declines to offer, and + // the user cannot tell which failure it belongs to. Scoped to this + // server, so an async continuation cannot erase another's. + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); + return; + } // A 401 from an OAuth-protected server means we have no (valid) token // yet. Kick off the authorization-code flow: `authenticate()` runs @@ -671,6 +698,22 @@ 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 + // 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 + // raw SDK text, on the one path where the Inspector had just told + // the user the authorization *worked*. Placed after the teardown + // above, which this arm needs for the same reason the generic one + // does, and before the flag it must not set. + if (showInsecureTokenEndpointNotice(recoveryErr, target.name)) { + // Only this server's banner — an async continuation must not + // erase one raised for a server the user has since switched to. + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); + return; + } setFailedServerId(id); const message = recoveryErr instanceof Error @@ -722,6 +765,14 @@ export function useConnectionLifecycle({ }); return; } + // See the SEP-2207 note on the handshake arm above (#2280). The + // disconnect already happened at the top of this catch. + if (showInsecureTokenEndpointNotice(authErr, target.name)) { + setReAuthBanner((prev) => + prev && prev.serverId === id ? null : prev, + ); + return; + } // The connect attempt failed, same as any other handshake error — // flag the card (#1621) and, with it, open the monitoring sidebar // onto the OAuth requests that explain the failure (#2108). This @@ -766,6 +817,7 @@ export function useConnectionLifecycle({ setFailedServerId, prepareOAuthRedirect, finalizeExplicitDisconnect, + setReAuthBanner, ], ); @@ -962,6 +1014,16 @@ export function useConnectionLifecycle({ authorizationUrl: authUrl, }); } catch (err) { + // SEP-2207 (#2280), and this is 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 + // worst place to lose the guidance, since they have just been told + // retrying is the fix. No banner clear is needed: this callback + // already cleared it before starting. + if (showInsecureTokenEndpointNotice(err, server?.name)) { + return; + } const message = err instanceof Error ? err.message : String(err); notifications.show({ title: server diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index dabc41f72..dde7cd00f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -9,6 +9,7 @@ import type { import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; import { useEffect, useLayoutEffect, useRef } from "react"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { @@ -979,6 +980,137 @@ 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 + // as an AuthRecoveryRequiredError, so before this it was rethrown into + // the generic reporting below. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + await expect( + h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError( + "http://localhost.:8091/token", + ), + ), + "tool", + ), + ).resolves.toBeUndefined(); + }); + expect(toastTitles()).toContain("Token endpoint is not secure"); + }); + + it("does not clear a banner belonging to a different server", async () => { + // The paths are asynchronous: server A can reject long after the user + // switched away and server B raised its own banner. An unconditional + // clear would erase B's, which is still valid and still actionable. + const client = fakeClient(); + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "b", + client, + }); + await act(async () => { + client.emit("oauthError", { error: new Error("session expired") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("b")); + + // The user switches to "a"; a stale continuation for it now rejects. + h.rerender({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + client, + }); + await act(async () => { + await h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "tool", + ); + }); + + expect(toastTitles()).toContain("Token endpoint is not secure"); + // B's banner survives — it is still valid and still actionable. + expect(h.api().reAuthBanner?.serverId).toBe("b"); + }); + + it("clears a stale banner when a command-path failure is terminal", async () => { + // Every terminal arm goes through one wrapper for this reason: a banner + // left by an earlier failure carries a Re-authenticate button just as + // dead as the one this arm declines to offer, and the user cannot tell + // which failure it belongs to. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + client.emit("oauthError", { error: new Error("session expired") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + + await act(async () => { + await h + .api() + .runWithCommandAuthRecovery( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "tool", + ); + }); + expect(toastTitles()).toContain("Token endpoint is not secure"); + expect(h.api().reAuthBanner).toBeNull(); + }); + + it("shows the terminal notice instead of the generic title in the background form", async () => { + // The `errorTitle` call sites would otherwise render the raw SDK text + // under a generic heading. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + h.api().runCommandInBackground( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "ambient", + "Refresh failed", + ); + await Promise.resolve(); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(toastTitles()).not.toContain("Refresh failed"); + }); + + it("still reports it at a call site whose panel owns reporting", async () => { + // The worse half of the old behavior: with no `errorTitle` the rejection + // was swallowed outright and the command just appeared to do nothing. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + h.api().runCommandInBackground( + () => + Promise.reject( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + "ambient", + ); + await Promise.resolve(); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + }); + it("toasts a background failure only when given a title", async () => { const client = fakeClient(); const h = harness({ servers: [entry("a")], activeServerId: "a", client }); @@ -1181,6 +1313,28 @@ describe("useOAuthRecovery", () => { await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); }); + it("clears a banner already on screen when a later oauthError is terminal", async () => { + // Otherwise the stale Re-authenticate button sits beside the terminal + // notice — the affordance this change removes, sourced from an earlier + // failure rather than this one. + const client = fakeClient(); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + client.emit("oauthError", { error: new Error("token endpoint 500") }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + + await act(async () => { + client.emit("oauthError", { + error: new InsecureTokenEndpointError("http://localhost.:8091/token"), + }); + }); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + }); + it("ignores an oauthError with no active server", async () => { const client = fakeClient(); const h = harness({ servers: [], activeServerId: undefined, client }); @@ -1344,6 +1498,41 @@ describe("useOAuthRecovery", () => { ); }); + it("does not re-arm a deferred recovery whose failure is terminal", async () => { + // The restore's premise is that the recovery is still owed and a later + // trigger should retry it. For a refusal that can only fail the same way, + // re-arming means every future tab focus replays it under a toast + // promising a retry that cannot succeed — an unbounded loop on a terminal + // error (#2280). + const client = fakeClient({ + handleAuthChallenge: vi + .fn() + .mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + }); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await defer(client, h); + await act(async () => { + becomeVisible(); + }); + + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + // Neither the retry promise nor the slot that would make good on it. + // `pendingReauth` is not on the hook's public surface, so it is read back + // through the commit probe, as the step-up tests above do. + expect(toastTitles()).not.toContain("Could not continue authorization"); + expect(h.commits[h.commits.length - 1]?.reauthServerId).toBeUndefined(); + + // The load-bearing assertion: coming back to the tab does not replay it. + await act(async () => { + becomeVisible(); + }); + expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1); + }); + it("does not put a stale challenge back over a newer deferral", async () => { // The tab can go hidden mid-resume and a newer challenge defer itself // into the slot; that one describes the session as it is now, so the @@ -1686,6 +1875,45 @@ describe("useOAuthRecovery", () => { expect(h.spies.setFailedServerId).not.toHaveBeenCalled(); }); + 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 + // 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. + snapshot(); + const client = fakeClient({ + resumeAfterOAuth: vi + .fn() + .mockRejectedValue( + new InsecureTokenEndpointError("http://localhost.:8091/token"), + ), + }); + const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + expect(h.spies.setFailedServerId).not.toHaveBeenCalled(); + }); + + it("finds an insecure token endpoint wrapped under `cause` on the callback leg", async () => { + snapshot(); + const client = fakeClient({ + resumeAfterOAuth: vi.fn().mockRejectedValue( + new Error("resume failed", { + cause: new InsecureTokenEndpointError( + "http://localhost.:8091/token", + ), + }), + ), + }); + const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client); + await waitFor(() => + expect(toastTitles()).toContain("Token endpoint is not secure"), + ); + expect(h.api().reAuthBanner).toBeNull(); + }); + it("offers one-click recovery when the authorization state was lost", async () => { snapshot(); const client = fakeClient({ diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index a47b94c20..1207c076f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1,3 +1,4 @@ +import type { Dispatch, SetStateAction } from "react"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { RefObject } from "react"; import { notifications } from "@mantine/notifications"; @@ -30,6 +31,7 @@ import { emaStepUpSuccessMessage, } from "@inspector/core/auth/oauthUx.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; import type { OAuthDetails } from "../components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { oauthDetailsFromConnectionState } from "../components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState"; import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; @@ -210,7 +212,13 @@ export interface OAuthRecovery { onBeforeOAuthRedirect: (authorizationUrl: URL) => void; prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void; reAuthBanner: ReAuthBannerState | null; - setReAuthBanner: (next: ReAuthBannerState | null) => void; + /** + * The raw state setter, functional form included. Consumers need the updater + * to clear a banner **only when it belongs to the server they are reporting + * on** — these paths are asynchronous, so a late continuation for server A + * must not erase a banner server B raised in the meantime. + */ + setReAuthBanner: Dispatch>; /** * Drops the banner and both pending-OAuth slots. Called from the session * reset, which runs on every disconnect: an unanswered step-up prompt or a @@ -383,6 +391,41 @@ export function useOAuthRecovery({ [sessionRef], ); + /** + * Report a terminal SEP-2207 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 + * carries a Re-authenticate button that is just as dead as the one this + * change removes, and the user cannot tell which failure it belongs to. Round + * 4 fixed that for one arm by hand; wrapping it is what stops the next arm + * from omitting it. + * + * Returns whether the error was claimed, so callers keep their fall-through. + */ + const reportTerminalInsecureTokenEndpoint = useCallback( + ( + err: unknown, + serverId: string | undefined, + serverName?: string, + ): boolean => { + if (!showInsecureTokenEndpointNotice(err, serverName)) { + return false; + } + // Clear only *this* server's banner. The command and deferred-resume + // paths are asynchronous, so server A can reject long after the user + // switched away and server B raised a banner of its own; an unconditional + // clear would then erase B's, which is still valid and still actionable. + // Functional so it sees the queued state rather than the render-time + // value, matching how `setPendingReauth` guards its own late restore. + setReAuthBanner((prev) => + prev && prev.serverId === serverId ? null : prev, + ); + return true; + }, + [setReAuthBanner], + ); + const showReAuthBanner = useCallback( ( serverId: string, @@ -390,6 +433,14 @@ export function useOAuthRecovery({ options?: { reason?: AuthChallengeReason }, ) => { const server = sessionRef.current.servers.find((s) => s.id === serverId); + // SEP-2207 (#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 + // the right behavior by default instead of by remembering. + if (reportTerminalInsecureTokenEndpoint(detail, serverId, server?.name)) { + return; + } const message = reAuthBannerMessage({ serverName: server?.name, detail: @@ -410,7 +461,7 @@ export function useOAuthRecovery({ message, }); }, - [sessionRef], + [sessionRef, reportTerminalInsecureTokenEndpoint], ); /** Clears pending OAuth resume state — explicit user disconnect only. */ @@ -811,10 +862,35 @@ export function useOAuthRecovery({ } return undefined; } + // SEP-2207 (#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 + // under a generic title or, at a call site whose panel owns reporting, + // swallows it and leaves the command looking like it did nothing. + // + // Claimed rather than rethrown, taking the same `undefined` exit the + // unsatisfied-recovery branch above already uses: the failure is + // terminal and now fully reported, so an awaited caller should stop + // rather than render it a second time. + const server = sessionRef.current.servers.find( + (s) => s.id === activeServerId, + ); + if ( + reportTerminalInsecureTokenEndpoint(err, activeServerId, server?.name) + ) { + return undefined; + } throw err; } }, - [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], + [ + inspectorClient, + activeServerId, + handleCommandScopedAuthRecovery, + sessionRef, + reportTerminalInsecureTokenEndpoint, + ], ); /** @@ -924,6 +1000,25 @@ export function useOAuthRecovery({ }); } } catch (err) { + // SEP-2207 (#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 + // refusal that can only fail the same way, re-arming the slot means + // every future tab focus and reconnect replays it, under a toast + // promising a retry that cannot succeed. Report it and let it go. + const failedServer = sessionRef.current.servers.find( + (s) => s.id === pending.serverId, + ); + if ( + reportTerminalInsecureTokenEndpoint( + err, + pending.serverId, + failedServer?.name, + ) + ) { + return; + } // The slot was cleared above only to keep a tab-visible event and a // reconnect from starting the same authorization twice — not because // the recovery was delivered. It still is owed, so restore it and let @@ -969,6 +1064,7 @@ export function useOAuthRecovery({ } }, [ + reportTerminalInsecureTokenEndpoint, sessionRef, inspectorClient, connectionStatus, @@ -1320,6 +1416,12 @@ export function useOAuthRecovery({ }); return; } + // Above `setFailedServerId` for the same reason the EMA arm is: this is + // a configuration error, not a failed attempt, so it should not flag + // the card red or pull the monitoring sidebar open. + if (reportTerminalInsecureTokenEndpoint(err, server.id, server.name)) { + return; + } // The token exchange (or the re-handshake behind it) failed. Flag the // server (#1621) so the monitoring sidebar opens onto the OAuth // requests that explain it (#2108) — the rebuilt client restored the @@ -1424,6 +1526,7 @@ export function useOAuthRecovery({ initialConfigSettledRef, clearResultPanels, showReAuthBanner, + reportTerminalInsecureTokenEndpoint, webOAuthStorage, setUi, setActiveTab, diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.test.ts b/clients/web/src/lib/insecureTokenEndpointNotice.test.ts new file mode 100644 index 000000000..c4265fe05 --- /dev/null +++ b/clients/web/src/lib/insecureTokenEndpointNotice.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, +} from "../utils/oauthUx"; + +const show = vi.fn(); +vi.mock("@mantine/notifications", () => ({ + notifications: { show: (...args: unknown[]) => show(...args) }, +})); + +const { showInsecureTokenEndpointNotice } = + await import("./insecureTokenEndpointNotice"); + +const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + +beforeEach(() => { + show.mockClear(); +}); + +describe("showInsecureTokenEndpointNotice", () => { + it("claims the SDK error and shows the terminal notice", () => { + const handled = showInsecureTokenEndpointNotice( + new InsecureTokenEndpointError(ENDPOINT), + "Acme", + ); + + expect(handled).toBe(true); + expect(show).toHaveBeenCalledTimes(1); + expect(show).toHaveBeenCalledWith({ + title: insecureTokenEndpointTitle(), + message: insecureTokenEndpointMessage({ + tokenEndpoint: ENDPOINT, + serverName: "Acme", + }), + color: "red", + // Non-recoverable, so the explanation must not vanish on a timer — there + // is no second chance to read it. + autoClose: false, + }); + }); + + it("works without a server name", () => { + expect( + showInsecureTokenEndpointNotice(new InsecureTokenEndpointError(ENDPOINT)), + ).toBe(true); + expect(show.mock.calls[0][0].message).toContain("this server"); + }); + + it("declines any other error, leaving the caller's handling in place", () => { + expect(showInsecureTokenEndpointNotice(new Error("boom"), "Acme")).toBe( + false, + ); + expect(show).not.toHaveBeenCalled(); + }); +}); diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts new file mode 100644 index 000000000..932b09459 --- /dev/null +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -0,0 +1,59 @@ +/** + * Surfaces the SDK's SEP-2207 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 + * effect; the copy it renders is pure and lives in `@inspector/core/auth`. + * + * Shaped as a claim-or-decline predicate rather than a plain `show(...)` so the + * three OAuth failure paths that need it — the connect handshake, the post- + * redirect callback, and the re-auth banner funnel — can each spend one line on + * it and keep their existing fall-through intact: + * + * ```ts + * if (showInsecureTokenEndpointNotice(err, server.name)) return; + * ``` + * + * `autoClose: false` matches the other non-recoverable OAuth notices (issuer + * mismatch, unconfigured enterprise IdP): nothing the user does next will make + * this reappear, so a toast that vanishes takes the only explanation with it. + * It stops the notice **expiring**, not the user dismissing it — Mantine's close + * control still works, which is correct for a message someone has finished + * reading. Don't describe this as non-dismissible. + */ + +import { notifications } from "@mantine/notifications"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; +import { + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, +} from "../utils/oauthUx"; + +/** + * Show the terminal notice when `err` is the SDK's `InsecureTokenEndpointError`. + * + * @returns `true` when it was handled (the caller should stop), `false` when + * `err` is some other failure and the caller's normal handling applies. + */ +export function showInsecureTokenEndpointNotice( + err: unknown, + serverName?: string, +): boolean { + // Searched rather than type-tested: era negotiation and the transport + // wrappers bury the rejection under `cause` / `data.cause`, so the connect and + // refresh paths hand us a wrapper rather than the error itself. + const found = findInsecureTokenEndpoint(err); + if (!found) { + return false; + } + notifications.show({ + title: insecureTokenEndpointTitle(), + message: insecureTokenEndpointMessage({ + tokenEndpoint: found.tokenEndpoint, + serverName, + }), + color: "red", + autoClose: false, + }); + return true; +} diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts new file mode 100644 index 000000000..78ff9ccf3 --- /dev/null +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect } from "vitest"; +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; + +const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + +/** + * Documents the assumption the classifier is built on, the same way + * `issuerBinding.test.ts` does for its sibling: the SDK declares `mcpBrand` in + * a `static {}` block, so it lives on the constructor and instances never carry + * it. A classifier that read `err.mcpBrand` would match no real thrown error. + */ +describe("SDK brand placement", () => { + it("keeps `mcpBrand` on the class, not the instance", () => { + expect("mcpBrand" in new InsecureTokenEndpointError(ENDPOINT)).toBe(false); + }); + + it("is not an OAuthError, which is why the retry path must not claim it", () => { + const err = new InsecureTokenEndpointError(ENDPOINT); + // The SDK deliberately keeps this off the `OAuthError` hierarchy so hosts + // do not treat it as a transient authorization failure. If a future SDK + // changes that, the #2280 handling should be revisited rather than silently + // keeping a now-wrong justification. + expect(err.name).toBe("InsecureTokenEndpointError"); + expect(typeof err.tokenEndpoint).toBe("string"); + }); +}); + +describe("findInsecureTokenEndpoint", () => { + it("recognizes a real SDK error and returns its endpoint", () => { + expect( + findInsecureTokenEndpoint(new InsecureTokenEndpointError(ENDPOINT)), + ).toMatchObject({ tokenEndpoint: ENDPOINT }); + }); + + it("recognizes a serialized copy by `name`, where the prototype is gone", () => { + // The fallback arm: a structured clone or JSON hop drops the prototype and + // the brand set but keeps `name`. + expect( + findInsecureTokenEndpoint({ + name: "InsecureTokenEndpointError", + message: "Refusing to send credentials…", + tokenEndpoint: ENDPOINT, + }), + ).toMatchObject({ tokenEndpoint: ENDPOINT }); + }); + + it("rejects a look-alike carrying the endpoint but not the identity", () => { + // Neither the brand nor the name: some other error that happens to have a + // `tokenEndpoint` field must not be swallowed by the terminal arm. + expect( + findInsecureTokenEndpoint({ tokenEndpoint: ENDPOINT, name: "Error" }), + ).toBeUndefined(); + }); + + it("rejects the right identity with no endpoint to report", () => { + // The copy names the endpoint, so a value that cannot supply one is not + // usable by this path and falls through to the generic handling. + expect( + findInsecureTokenEndpoint({ name: "InsecureTokenEndpointError" }), + ).toBeUndefined(); + expect( + findInsecureTokenEndpoint({ + name: "InsecureTokenEndpointError", + tokenEndpoint: 42, + }), + ).toBeUndefined(); + }); + + it.each([null, undefined, "InsecureTokenEndpointError", 0, new Error("x")])( + "rejects %j", + (value) => { + expect(findInsecureTokenEndpoint(value)).toBeUndefined(); + }, + ); + + describe("cause chains", () => { + // Era negotiation and the transport wrappers bury the rejection, so a + // top-level-only check would miss the connect and refresh paths outright + // and let the retryable UI render anyway. + it("finds it under `cause`", () => { + const wrapped = new Error("connect failed", { + cause: new InsecureTokenEndpointError(ENDPOINT), + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("finds it under `data.cause`", () => { + const wrapped = Object.assign(new Error("negotiation failed"), { + data: { cause: new InsecureTokenEndpointError(ENDPOINT) }, + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("finds it several links down", () => { + const wrapped = new Error("outer", { + cause: new Error("middle", { + cause: new InsecureTokenEndpointError(ENDPOINT), + }), + }); + expect(findInsecureTokenEndpoint(wrapped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("terminates on a self-referential cause instead of looping", () => { + const loop: { cause?: unknown; name: string } = { name: "Loop" }; + loop.cause = loop; + expect(findInsecureTokenEndpoint(loop)).toBeUndefined(); + }); + + it("returns undefined for a chain that never contains one", () => { + expect( + findInsecureTokenEndpoint( + new Error("outer", { cause: new Error("inner") }), + ), + ).toBeUndefined(); + }); + }); +}); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index dabe20727..1ef53e11a 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -6,6 +6,8 @@ import { emaStepUpFailureMessage, emaStepUpInProgressMessage, emaStepUpSuccessMessage, + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, isActionTriggeredOAuthRecovery, isEmaStepUp, isReAuthBannerReason, @@ -437,3 +439,57 @@ describe("oauthUx issuer-binding copy", () => { }); }); }); + +describe("insecureTokenEndpoint copy", () => { + const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; + + it("names the failure as a configuration problem, not an auth failure", () => { + expect(insecureTokenEndpointTitle()).toBe("Token endpoint is not secure"); + }); + + it("describes the scheme as not-HTTPS rather than as plain HTTP", () => { + // The SDK's check is `protocol !== "https:"`, so a mistyped `ftp:` or `ws:` + // endpoint lands here too; naming the wrong scheme would send the reader + // hunting for a problem they do not have. + const message = insecureTokenEndpointMessage({ + tokenEndpoint: "ftp://as.example.com/token", + }); + expect(message).toContain("not HTTPS"); + expect(message).not.toContain("plain HTTP"); + }); + + it("names the endpoint, the server, and both ways out", () => { + const message = insecureTokenEndpointMessage({ + tokenEndpoint: ENDPOINT, + serverName: "Acme", + }); + expect(message).toContain('"Acme"'); + expect(message).toContain(ENDPOINT); + expect(message).toContain("HTTPS"); + expect(message).toContain("127.0.0.1"); + expect(message).toContain("Token URL"); + }); + + it("says a retry cannot help, which is the whole point of the message", () => { + // The bug this copy fixes (#2280) was a Re-authenticate button that could + // never succeed. If this sentence goes, the copy stops doing its job. + expect(insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT })).toContain( + "Re-authenticating cannot change this", + ); + }); + + it("falls back to a generic subject with no server name", () => { + const message = insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT }); + expect(message).toContain("this server"); + expect(message).not.toContain('""'); + }); + + it("bounds a hostile-length endpoint for display", () => { + // The endpoint is remote-supplied (it comes from the server's AS metadata), + // so an overlong value must not be echoed back whole into the layout. + const long = `https://example.com/${"a".repeat(500)}`; + const message = insecureTokenEndpointMessage({ tokenEndpoint: long }); + expect(message).not.toContain(long); + expect(message).toContain("…"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts new file mode 100644 index 000000000..31c5f5368 --- /dev/null +++ b/clients/web/src/test/integration/mcp/strict-port.test.ts @@ -0,0 +1,228 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { createServer, type Server } from "node:http"; +import { writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + loadConfig, + resolveConfig, +} from "@modelcontextprotocol/inspector-test-server"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * Live coverage of `ServerConfig.strictPort` (#2280). + * + * Every other fixture walks to the next free port on `EADDRINUSE`, which is + * right for them and wrong for one: `oauth-insecure-token-endpoint-http.json` + * hard-codes its port inside an OAuth issuer string, so a relocated server would + * announce 8092 while all its metadata still pointed at whatever unrelated + * process holds 8091 — and would silently stop reproducing the refusal it + * exists for. This asserts the walk still happens by default and does not + * happen for that fixture, because "fails loudly" is only a safety property if + * it actually fails. + */ +const configsDir = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs", +); + +describe("strictPort (#2280)", () => { + let squatter: Server | null = null; + let server: TestServerHttp | null = null; + + afterEach(async () => { + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + if (squatter) { + await new Promise((resolve) => squatter!.close(() => resolve())); + squatter = null; + } + }); + + /** Hold a port so the next bind has to decide whether to walk. */ + const squat = async (): Promise => { + squatter = createServer((_req, res) => res.end()); + await new Promise((resolve) => + squatter!.listen(0, "127.0.0.1", () => resolve()), + ); + const address = squatter.address(); + if (typeof address !== "object" || address === null) { + throw new Error("no port"); + } + return address.port; + }; + + it("walks to another port by default", async () => { + const taken = await squat(); + server = createTestServerHttp({ + serverInfo: createTestServerInfo("walks", "1.0.0"), + serverType: "streamable-http", + port: taken, + }); + const bound = await server.start(); + expect(bound).not.toBe(taken); + }); + + it("refuses to relocate when strictPort is set", async () => { + const taken = await squat(); + server = createTestServerHttp({ + serverInfo: createTestServerInfo("strict", "1.0.0"), + serverType: "streamable-http", + port: taken, + strictPort: true, + }); + await expect(server.start()).rejects.toMatchObject({ + code: "EADDRINUSE", + }); + // Deliberately NOT nulled: `start()` installs the process-global test-server + // control before it binds, and only `stop()` clears it. Dropping the + // reference here would skip teardown and leave that global pointing at a + // dead server for the rest of the worker. + }); + + it.each([undefined, 0])( + "refuses to start with strictPort and port %j", + async (port) => { + // Nothing to be strict about. Falling through to an OS-assigned port + // would let a misconfigured fixture look strict while relocating every + // run — the failure the flag exists to prevent, now silent. + server = createTestServerHttp({ + serverInfo: createTestServerInfo("misconfigured", "1.0.0"), + serverType: "streamable-http", + port, + strictPort: true, + }); + await expect(server.start()).rejects.toThrow(/integer in 1-65535/); + }, + ); + + it.each(["false", "true", 1, null])( + "rejects a non-boolean strictPort in a config file: %j", + (value) => { + // Consumed as a plain truthiness check at bind time, so the string + // "false" would read as *enabled* and silently disable the port walk — + // the opposite of what the author wrote. + const file = path.join( + tmpdir(), + `strict-port-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport: { type: "streamable-http", port: 8099, strictPort: value }, + }), + ); + try { + expect(() => loadConfig(file)).toThrow( + /transport.strictPort must be a boolean/, + ); + } finally { + rmSync(file, { force: true }); + } + }, + ); + + it.each([ + // Each of these is truthy or type-valid enough to pass a naive check, and + // each fails SILENTLY: the fixture looks strict and relocates anyway. + [ + { type: "streamable-http", port: "0", strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: "8091", strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 0, strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 8091.5, strictPort: true }, + /integer in 1-65535/, + ], + [ + { type: "streamable-http", port: 70000, strictPort: true }, + /integer in 1-65535/, + ], + [{ type: "streamable-http", strictPort: true }, /integer in 1-65535/], + // No listener at all, and `resolveConfig` drops the flag. + [{ type: "stdio", strictPort: true }, /requires an HTTP transport/], + ])("rejects the unhonorable strictPort config %j", (transport, message) => { + const file = path.join( + tmpdir(), + `strict-port-combo-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport, + }), + ); + try { + expect(() => loadConfig(file)).toThrow(message); + } finally { + rmSync(file, { force: true }); + } + }); + + it("still accepts the honorable combination", () => { + const file = path.join( + tmpdir(), + `strict-port-ok-${Date.now()}-${Math.random()}.json`, + ); + writeFileSync( + file, + JSON.stringify({ + serverInfo: { name: "x", version: "1.0.0" }, + transport: { type: "streamable-http", port: 8091, strictPort: true }, + }), + ); + try { + expect(resolveConfig(loadConfig(file)).strictPort).toBe(true); + } finally { + rmSync(file, { force: true }); + } + }); + + it("rejects a truthy-but-unbindable port at bind time too", async () => { + // Defense in depth for a programmatic caller that bypasses `loadConfig`. + // A string "0" is truthy, so a bare falsiness guard would pass it through + // and Node would coerce it to the dynamic port 0. + server = createTestServerHttp({ + serverInfo: createTestServerInfo("stringy", "1.0.0"), + serverType: "streamable-http", + port: "0" as unknown as number, + strictPort: true, + }); + await expect(server.start()).rejects.toThrow(/integer in 1-65535/); + // Deliberately NOT nulled, for the same reason as the EADDRINUSE case + // above: `start()` installs the process-global test-server control before + // it validates, so dropping the reference would skip teardown and leave + // that global pointing at a dead server. + }); + + it("is carried from the fixture's config file to the resolved server config", async () => { + // The plumbing half: a flag the loader drops would leave the fixture + // relocating again with nothing to show for it. + const resolved = resolveConfig( + loadConfig( + path.join(configsDir, "oauth-insecure-token-endpoint-http.json"), + ), + ); + expect(resolved.strictPort).toBe(true); + expect(resolved.port).toBe(8091); + expect(resolved.oauth?.issuerUrl?.href).toContain("localhost.:8091"); + }); +}); diff --git a/clients/web/src/utils/oauthUx.ts b/clients/web/src/utils/oauthUx.ts index eeaa94361..2c8438c13 100644 --- a/clients/web/src/utils/oauthUx.ts +++ b/clients/web/src/utils/oauthUx.ts @@ -14,6 +14,8 @@ export { lostAuthorizationStateTitle, issuerMismatchMessage, issuerMismatchTitle, + insecureTokenEndpointMessage, + insecureTokenEndpointTitle, type OAuthInteractiveAuthKind, type OAuthPreRedirectContext, type OAuthRecoverySource, diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts new file mode 100644 index 000000000..9dfa6717e --- /dev/null +++ b/core/auth/insecureTokenEndpoint.ts @@ -0,0 +1,107 @@ +/** + * SEP-2207: 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`. + * + * That error is **terminal by design**. It does not extend `OAuthError`, and + * `auth()` special-cases it to rethrow rather than fall through to a fresh + * `/authorize` redirect — so nothing the Inspector does can make a retry + * succeed. Recognizing it is what lets the UI say so, instead of offering a + * "Re-authenticate" affordance that can only fail the same way (#2280). + * + * The check is not a fix for `*.localhost` (#1944): the exemption list lives in + * the SDK and takes no options, so widening it has to happen upstream + * (typescript-sdk#2591). This is about how the refusal is *reported* — which + * matters for every endpoint outside that exemption, `host.docker.internal` and + * LAN hostnames included, not only the `.localhost` case. + */ + +import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; + +/** The fields this module needs off the SDK error, once recognized. */ +export interface InsecureTokenEndpointShape { + /** The token endpoint URL the SDK refused to post credentials to. */ + tokenEndpoint: string; +} + +/** + * Recognize the SDK's `InsecureTokenEndpointError` itself (not a wrapper). + * + * Uses the SDK's own `isInstance` predicate, which is cross-copy safe by + * construction: the SDK stamps each instance with a brand set keyed by + * `Symbol.for("mcp.sdk.errorBrands")` and overrides `Symbol.hasInstance` to + * consult it, so an error thrown by a different bundled copy still matches. + * (The brand constant is `static`, so it is never reachable as `err.mcpBrand` + * on an instance — don't check that property.) + * + * The `name` comparison is the same deliberate serialization fallback + * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`: today the + * web client runs `auth()` in the browser, so no boundary is crossed, but the + * prototype and brand set are the first things a structured clone or a JSON hop + * would drop, and `name` survives both. + */ +function isInsecureTokenEndpointShape( + err: unknown, +): err is InsecureTokenEndpointShape { + if (err === null || typeof err !== "object") { + return false; + } + const candidate = err as { tokenEndpoint?: unknown; name?: unknown }; + if (typeof candidate.tokenEndpoint !== "string") { + return false; + } + return ( + InsecureTokenEndpointError.isInstance(err) || + candidate.name === "InsecureTokenEndpointError" + ); +} + +/** + * Find an insecure-token-endpoint refusal anywhere in an error's `cause` / + * `data.cause` chain, and return the shape that carries the endpoint. + * + * Walking the chain is not defensive padding: era negotiation and the transport + * wrappers bury the original rejection, so a top-level-only check would miss the + * connect and refresh paths and let exactly the retryable UI this exists to + * remove render anyway. `findIssuerBindingFailure` in `issuerBinding.ts` walks + * the same two links for the same reason, and this deliberately mirrors it — + * including the `seen` set, which keeps a self-referential `cause` from looping. + */ +export function findInsecureTokenEndpoint( + err: unknown, +): InsecureTokenEndpointShape | undefined { + return findInsecureTokenEndpointDeep(err, new Set()); +} + +function findInsecureTokenEndpointDeep( + err: unknown, + seen: Set, +): InsecureTokenEndpointShape | undefined { + if (err === null || typeof err !== "object" || seen.has(err)) { + return undefined; + } + seen.add(err); + + if (isInsecureTokenEndpointShape(err)) { + return err; + } + + const nested = findInsecureTokenEndpointDeep( + (err as { cause?: unknown }).cause, + seen, + ); + if (nested) { + return nested; + } + + const data = (err as { data?: unknown }).data; + if (data !== null && typeof data === "object") { + return findInsecureTokenEndpointDeep( + (data as { cause?: unknown }).cause, + seen, + ); + } + + return undefined; +} diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 7c07f2331..1a1dfc4dd 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -299,11 +299,16 @@ export function issuerMismatchTitle(): string { */ const MAX_DISPLAYED_ISSUER_LENGTH = 120; +/** Bound a remote-supplied URL for display, marking any truncation. */ +export function truncateUrlForDisplay(url: string): string { + return url.length > MAX_DISPLAYED_ISSUER_LENGTH + ? `${url.slice(0, MAX_DISPLAYED_ISSUER_LENGTH)}…` + : url; +} + /** Bound an issuer for display, marking any truncation. */ export function truncateIssuerForDisplay(issuer: string): string { - return issuer.length > MAX_DISPLAYED_ISSUER_LENGTH - ? `${issuer.slice(0, MAX_DISPLAYED_ISSUER_LENGTH)}…` - : issuer; + return truncateUrlForDisplay(issuer); } /** @@ -357,3 +362,53 @@ export function reAuthBannerMessage(options: { : "Authentication needs attention."; return options.detail ? `${prefix} ${options.detail}` : prefix; } + +/** + * Heading for the SDK's SEP-2207 refusal to post credentials to a non-TLS token + * endpoint (#2280). + * + * Deliberately not phrased as an authentication failure. Like + * {@link issuerMismatchTitle}, this is offered **no** one-click recovery: the + * SDK rethrows `InsecureTokenEndpointError` rather than retrying, so a + * "Re-authenticate" affordance here could only fail the same way, and a button + * that cannot work is worse than no button. + */ +export function insecureTokenEndpointTitle(): string { + return "Token endpoint is not secure"; +} + +/** + * Plain-language explanation and the two things that actually resolve it. + * + * Does not echo the SDK's own message, which reads as a flat refusal and tells + * the user nothing about which lever to reach for. + * + * The scheme half says "not HTTPS" rather than "plain HTTP": the SDK's check is + * `protocol !== "https:"`, so anything else an authorization server advertises + * — including a mistyped `ftp:` or `ws:` endpoint — lands here too, and naming + * the wrong scheme would send the reader looking for a problem they do not have. + * + * The host half is "outside the SDK's loopback exemption", never "not loopback". + * The motivating hosts — `tenant.app.localhost` (#1944), the `localhost.` + * fixture — *are* loopback by RFC 6761 and by every resolver on the machine; + * what they are outside is a three-literal allow-list. Calling them non-loopback + * would send a reader to debug their networking instead of their configuration. The endpoint is + * remote-supplied (it comes from the server's authorization-server metadata), + * so it is bounded for display by {@link truncateUrlForDisplay}; rendering is + * escaped, so that is a layout bound rather than an injection defence. + */ +export function insecureTokenEndpointMessage(options: { + tokenEndpoint: string; + serverName?: string; +}): string { + const target = options.serverName ? `"${options.serverName}"` : "this server"; + return ( + `Authorization for ${target} was stopped before any credentials were sent: ` + + `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 → Authorization 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 6b0a739cb..f4c1dc04e 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -54,6 +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) | | `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) | @@ -454,6 +455,24 @@ 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) + +`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. + +⚠️ It sets `transport.strictPort`, so it **fails to start** if 8091 is taken rather than relocating. (`strictPort` is only honorable alongside an HTTP transport and an integer `port` in 1–65535, so `loadConfig` rejects every other combination outright — each of them would otherwise leave a fixture looking strict while relocating anyway.) Every other fixture walks to the next free port on `EADDRINUSE`, which is right for them and wrong for this one: the issuer is a *fixed string* in the config, so a relocated server would announce 8092 while all its OAuth metadata still pointed at whatever unrelated process holds 8091 — and the fixture would quietly stop reproducing the refusal it exists for. A loud failure is the only honest option here. + +The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. + +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. + +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. + +On the broken build you got a **"Re-authentication required"** banner with a **Re-authenticate** button ([#2280](https://github.com/modelcontextprotocol/inspector/issues/2280)). That button could never work: `InsecureTokenEndpointError` does not extend `OAuthError`, and `auth()` special-cases it to rethrow rather than start a fresh `/authorize` redirect, so clicking it re-ran the same flow to the same refusal. The only text on screen was the raw SDK message, which names the three exempt literals and says nothing about which lever to reach for. + +Note that the fix here is presentational only. Making a `*.localhost` token endpoint actually **work** has to land in the SDK — the assertion runs inside `executeTokenRequest`, takes no options, and there is no hook the Inspector could reach. + ## Revoking tokens on clear (RFC 7009) `oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. diff --git a/test-servers/configs/oauth-insecure-token-endpoint-http.json b/test-servers/configs/oauth-insecure-token-endpoint-http.json new file mode 100644 index 000000000..c2b26d892 --- /dev/null +++ b/test-servers/configs/oauth-insecure-token-endpoint-http.json @@ -0,0 +1,27 @@ +{ + "serverInfo": { + "name": "oauth-insecure-token-endpoint", + "version": "1.0.0" + }, + "tools": [ + { + "preset": "echo" + } + ], + "oauth": { + "enabled": true, + "mode": "combined", + "requireAuth": true, + "scopesSupported": [ + "mcp" + ], + "supportDCR": true, + "supportRefreshTokens": true, + "issuerUrl": "http://localhost.:8091" + }, + "transport": { + "type": "streamable-http", + "port": 8091, + "strictPort": true + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 7123873fb..229df487c 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -456,6 +456,17 @@ export interface ServerConfig { | undefined; // Optional callback to customize resource handler during registration serverType?: "sse" | "streamable-http"; // Transport type (default: "streamable-http") port?: number; // Port to use (optional, will find available port if not specified) + /** + * Refuse to relocate: bind {@link port} exactly, or fail with EADDRINUSE. + * + * Off by default, because walking to the next free port is what lets several + * fixtures run side by side. It exists for a fixture whose *advertised* + * configuration hard-codes the port — `oauth-insecure-token-endpoint-http.json` + * puts it in an OAuth issuer — where relocating leaves the server announcing + * one port while its metadata still points at another process entirely, and + * the fixture silently stops reproducing what it exists to reproduce. + */ + strictPort?: boolean; /** * Whether to advertise listChanged capability for each list type * If enabled, modification tools will send list_changed notifications diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 4f08e232a..74e9fa5ad 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -119,6 +119,8 @@ export interface ConfigFile { transport: { type: "stdio" | "streamable-http" | "sse"; port?: number; + /** Bind `port` exactly, or fail — see `ServerConfig.strictPort`. */ + strictPort?: boolean; /** * Serve the modern (2026-07-28) protocol era via the SDK's * `createMcpHandler` (only valid with `type: "streamable-http"`). `true` @@ -196,6 +198,48 @@ function validateConfig( `Invalid config in ${filePath}: transport.type must be stdio, streamable-http, or sse`, ); } + // `strictPort` is consumed as a plain truthiness check at bind time, so a + // string `"false"` would read as *enabled* and silently disable the port walk + // — the opposite of what the author wrote. Validate the type here, where the + // file is being trusted, rather than letting it through as a `ConfigFile`. + if ( + transport.strictPort !== undefined && + typeof transport.strictPort !== "boolean" + ) { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort must be a boolean`, + ); + } + + // Beyond the type: reject every combination that cannot honor the flag's + // contract, because each of them fails *silently* — the fixture looks strict + // and relocates anyway, which is the failure the flag exists to prevent. + // + // - a string port (`"0"`, `"8091"`) is truthy, so it slips past the runtime + // "no port to be strict about" guard, and Node then coerces `"0"` to the + // dynamic port 0; + // - a non-integer or out-of-range port cannot be bound as written; + // - on `stdio` there is no listener at all, and `resolveConfig` drops the + // flag, so it silently does nothing. + if (transport.strictPort === true) { + if (transportType === "stdio") { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort requires an HTTP transport (streamable-http or sse)`, + ); + } + const port = transport.port; + if ( + typeof port !== "number" || + !Number.isInteger(port) || + port < 1 || + port > 65535 + ) { + throw new Error( + `Invalid config in ${filePath}: transport.strictPort requires transport.port to be an integer in 1-65535 (got ${JSON.stringify(port)})`, + ); + } + } + // Only reject *enabling* modern on a non-HTTP transport; a falsy `modern` // (e.g. `false`) is a no-op that `resolveConfig` normalizes away. if (transport.modern && transportType !== "streamable-http") { diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index 52eaca8ea..8cd876de0 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -102,6 +102,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig { ? (transport.type as "sse" | "streamable-http") : undefined, port: isHttp ? transport.port : undefined, + strictPort: isHttp ? transport.strictPort : undefined, }; // Normalize the modern flag: `true` is shorthand for the default (dual-era diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index f8a965c3b..73ca7689d 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -400,9 +400,39 @@ export class TestServerHttp { const serverType = this.config.serverType ?? "streamable-http"; const requestedPort = this.config.port; + // `strictPort` means "bind exactly this port, or fail". With no fixed port + // to bind there is nothing to be strict about, and falling through to an + // OS-assigned one would let a misconfigured fixture look strict while + // relocating on every run — the exact failure the flag exists to prevent, + // now silent. Reject the combination instead. + if ( + this.config.strictPort && + (typeof requestedPort !== "number" || + !Number.isInteger(requestedPort) || + requestedPort < 1 || + requestedPort > 65535) + ) { + // `loadConfig` rejects these for a config file; this covers a + // programmatic caller, and specifically a value that is *truthy* but not + // bindable as written — a string `"0"` slips past a bare falsiness check + // and Node then coerces it to the dynamic port 0, so the fixture looks + // strict and relocates anyway. + throw new Error( + `strictPort requires an explicit port as an integer in 1-65535 (got ${JSON.stringify(requestedPort)}): ` + + "there is nothing to bind strictly otherwise.", + ); + } + // If a port is explicitly requested, find an available port starting from that value - // Otherwise, use 0 to let the OS assign an available port - const port = requestedPort ? await findAvailablePort(requestedPort) : 0; + // Otherwise, use 0 to let the OS assign an available port. + // `strictPort` opts out of the walk: bind the requested port or fail loudly + // (see the field's doc comment — a relocated server whose advertised config + // hard-codes the port is worse than one that does not start). + const port = requestedPort + ? this.config.strictPort + ? requestedPort + : await findAvailablePort(requestedPort) + : 0; if (serverType === "streamable-http") { return this.startHttp(port); From 6c1b5325f08b8fe6b9aa6bdf1fc695374bb87895 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:23:02 -0400 Subject: [PATCH 2/7] test: drop strictPort and document the port collision instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strictPort was a new test-server config option — plus its validation and 18 tests — invented solely to keep one fixture's hard-coded issuer port honest. That is infrastructure built for a screenshot, so it goes. test-servers/src is now byte-identical to v2/main again. The fixture keeps its fixed port and docs/test-servers.md carries the caveat instead, written to be actionable rather than merely cautionary: the failure mode is confusing rather than obvious, since a relocated server leaves discovery pointing at whatever unrelated process holds 8091, so the note says to check the port before believing what the flow does. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../test/integration/mcp/strict-port.test.ts | 228 ------------------ docs/test-servers.md | 2 +- .../oauth-insecure-token-endpoint-http.json | 3 +- test-servers/src/composable-test-server.ts | 11 - test-servers/src/load-config.ts | 44 ---- test-servers/src/resolve-config.ts | 1 - test-servers/src/test-server-http.ts | 34 +-- 7 files changed, 4 insertions(+), 319 deletions(-) delete mode 100644 clients/web/src/test/integration/mcp/strict-port.test.ts diff --git a/clients/web/src/test/integration/mcp/strict-port.test.ts b/clients/web/src/test/integration/mcp/strict-port.test.ts deleted file mode 100644 index 31c5f5368..000000000 --- a/clients/web/src/test/integration/mcp/strict-port.test.ts +++ /dev/null @@ -1,228 +0,0 @@ -import { describe, it, expect, afterEach } from "vitest"; -import { createServer, type Server } from "node:http"; -import { writeFileSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { - createTestServerHttp, - type TestServerHttp, - createTestServerInfo, - loadConfig, - resolveConfig, -} from "@modelcontextprotocol/inspector-test-server"; -import path from "node:path"; -import { fileURLToPath } from "node:url"; - -/** - * Live coverage of `ServerConfig.strictPort` (#2280). - * - * Every other fixture walks to the next free port on `EADDRINUSE`, which is - * right for them and wrong for one: `oauth-insecure-token-endpoint-http.json` - * hard-codes its port inside an OAuth issuer string, so a relocated server would - * announce 8092 while all its metadata still pointed at whatever unrelated - * process holds 8091 — and would silently stop reproducing the refusal it - * exists for. This asserts the walk still happens by default and does not - * happen for that fixture, because "fails loudly" is only a safety property if - * it actually fails. - */ -const configsDir = path.resolve( - path.dirname(fileURLToPath(import.meta.url)), - "../../../../../../test-servers/configs", -); - -describe("strictPort (#2280)", () => { - let squatter: Server | null = null; - let server: TestServerHttp | null = null; - - afterEach(async () => { - if (server) { - try { - await server.stop(); - } catch { - // ignore - } - server = null; - } - if (squatter) { - await new Promise((resolve) => squatter!.close(() => resolve())); - squatter = null; - } - }); - - /** Hold a port so the next bind has to decide whether to walk. */ - const squat = async (): Promise => { - squatter = createServer((_req, res) => res.end()); - await new Promise((resolve) => - squatter!.listen(0, "127.0.0.1", () => resolve()), - ); - const address = squatter.address(); - if (typeof address !== "object" || address === null) { - throw new Error("no port"); - } - return address.port; - }; - - it("walks to another port by default", async () => { - const taken = await squat(); - server = createTestServerHttp({ - serverInfo: createTestServerInfo("walks", "1.0.0"), - serverType: "streamable-http", - port: taken, - }); - const bound = await server.start(); - expect(bound).not.toBe(taken); - }); - - it("refuses to relocate when strictPort is set", async () => { - const taken = await squat(); - server = createTestServerHttp({ - serverInfo: createTestServerInfo("strict", "1.0.0"), - serverType: "streamable-http", - port: taken, - strictPort: true, - }); - await expect(server.start()).rejects.toMatchObject({ - code: "EADDRINUSE", - }); - // Deliberately NOT nulled: `start()` installs the process-global test-server - // control before it binds, and only `stop()` clears it. Dropping the - // reference here would skip teardown and leave that global pointing at a - // dead server for the rest of the worker. - }); - - it.each([undefined, 0])( - "refuses to start with strictPort and port %j", - async (port) => { - // Nothing to be strict about. Falling through to an OS-assigned port - // would let a misconfigured fixture look strict while relocating every - // run — the failure the flag exists to prevent, now silent. - server = createTestServerHttp({ - serverInfo: createTestServerInfo("misconfigured", "1.0.0"), - serverType: "streamable-http", - port, - strictPort: true, - }); - await expect(server.start()).rejects.toThrow(/integer in 1-65535/); - }, - ); - - it.each(["false", "true", 1, null])( - "rejects a non-boolean strictPort in a config file: %j", - (value) => { - // Consumed as a plain truthiness check at bind time, so the string - // "false" would read as *enabled* and silently disable the port walk — - // the opposite of what the author wrote. - const file = path.join( - tmpdir(), - `strict-port-${Date.now()}-${Math.random()}.json`, - ); - writeFileSync( - file, - JSON.stringify({ - serverInfo: { name: "x", version: "1.0.0" }, - transport: { type: "streamable-http", port: 8099, strictPort: value }, - }), - ); - try { - expect(() => loadConfig(file)).toThrow( - /transport.strictPort must be a boolean/, - ); - } finally { - rmSync(file, { force: true }); - } - }, - ); - - it.each([ - // Each of these is truthy or type-valid enough to pass a naive check, and - // each fails SILENTLY: the fixture looks strict and relocates anyway. - [ - { type: "streamable-http", port: "0", strictPort: true }, - /integer in 1-65535/, - ], - [ - { type: "streamable-http", port: "8091", strictPort: true }, - /integer in 1-65535/, - ], - [ - { type: "streamable-http", port: 0, strictPort: true }, - /integer in 1-65535/, - ], - [ - { type: "streamable-http", port: 8091.5, strictPort: true }, - /integer in 1-65535/, - ], - [ - { type: "streamable-http", port: 70000, strictPort: true }, - /integer in 1-65535/, - ], - [{ type: "streamable-http", strictPort: true }, /integer in 1-65535/], - // No listener at all, and `resolveConfig` drops the flag. - [{ type: "stdio", strictPort: true }, /requires an HTTP transport/], - ])("rejects the unhonorable strictPort config %j", (transport, message) => { - const file = path.join( - tmpdir(), - `strict-port-combo-${Date.now()}-${Math.random()}.json`, - ); - writeFileSync( - file, - JSON.stringify({ - serverInfo: { name: "x", version: "1.0.0" }, - transport, - }), - ); - try { - expect(() => loadConfig(file)).toThrow(message); - } finally { - rmSync(file, { force: true }); - } - }); - - it("still accepts the honorable combination", () => { - const file = path.join( - tmpdir(), - `strict-port-ok-${Date.now()}-${Math.random()}.json`, - ); - writeFileSync( - file, - JSON.stringify({ - serverInfo: { name: "x", version: "1.0.0" }, - transport: { type: "streamable-http", port: 8091, strictPort: true }, - }), - ); - try { - expect(resolveConfig(loadConfig(file)).strictPort).toBe(true); - } finally { - rmSync(file, { force: true }); - } - }); - - it("rejects a truthy-but-unbindable port at bind time too", async () => { - // Defense in depth for a programmatic caller that bypasses `loadConfig`. - // A string "0" is truthy, so a bare falsiness guard would pass it through - // and Node would coerce it to the dynamic port 0. - server = createTestServerHttp({ - serverInfo: createTestServerInfo("stringy", "1.0.0"), - serverType: "streamable-http", - port: "0" as unknown as number, - strictPort: true, - }); - await expect(server.start()).rejects.toThrow(/integer in 1-65535/); - // Deliberately NOT nulled, for the same reason as the EADDRINUSE case - // above: `start()` installs the process-global test-server control before - // it validates, so dropping the reference would skip teardown and leave - // that global pointing at a dead server. - }); - - it("is carried from the fixture's config file to the resolved server config", async () => { - // The plumbing half: a flag the loader drops would leave the fixture - // relocating again with nothing to show for it. - const resolved = resolveConfig( - loadConfig( - path.join(configsDir, "oauth-insecure-token-endpoint-http.json"), - ), - ); - expect(resolved.strictPort).toBe(true); - expect(resolved.port).toBe(8091); - expect(resolved.oauth?.issuerUrl?.href).toContain("localhost.:8091"); - }); -}); diff --git a/docs/test-servers.md b/docs/test-servers.md index f4c1dc04e..f8cf1c632 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -459,7 +459,7 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be `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. -⚠️ It sets `transport.strictPort`, so it **fails to start** if 8091 is taken rather than relocating. (`strictPort` is only honorable alongside an HTTP transport and an integer `port` in 1–65535, so `loadConfig` rejects every other combination outright — each of them would otherwise leave a fixture looking strict while relocating anyway.) Every other fixture walks to the next free port on `EADDRINUSE`, which is right for them and wrong for this one: the issuer is a *fixed string* in the config, so a relocated server would announce 8092 while all its OAuth metadata still pointed at whatever unrelated process holds 8091 — and the fixture would quietly stop reproducing the refusal it exists for. A loud failure is the only honest option here. +⚠️ **Do not run this fixture while port 8091 is already taken.** Like every fixture here it walks to the next free port on `EADDRINUSE` — but its `issuerUrl` is a *fixed string*, so a relocated server announces 8092 while all its OAuth metadata still points at whatever unrelated process holds 8091. The symptom is confusing rather than obvious: discovery reaches the wrong process, and the refusal this fixture exists to demonstrate either never fires or fires for the wrong reason. If the flow does not end at the notice described below, check that 8091 is actually this server (`lsof -nP -iTCP:8091 -sTCP:LISTEN`) before believing anything you see. The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. diff --git a/test-servers/configs/oauth-insecure-token-endpoint-http.json b/test-servers/configs/oauth-insecure-token-endpoint-http.json index c2b26d892..35053e037 100644 --- a/test-servers/configs/oauth-insecure-token-endpoint-http.json +++ b/test-servers/configs/oauth-insecure-token-endpoint-http.json @@ -21,7 +21,6 @@ }, "transport": { "type": "streamable-http", - "port": 8091, - "strictPort": true + "port": 8091 } } diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 229df487c..7123873fb 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -456,17 +456,6 @@ export interface ServerConfig { | undefined; // Optional callback to customize resource handler during registration serverType?: "sse" | "streamable-http"; // Transport type (default: "streamable-http") port?: number; // Port to use (optional, will find available port if not specified) - /** - * Refuse to relocate: bind {@link port} exactly, or fail with EADDRINUSE. - * - * Off by default, because walking to the next free port is what lets several - * fixtures run side by side. It exists for a fixture whose *advertised* - * configuration hard-codes the port — `oauth-insecure-token-endpoint-http.json` - * puts it in an OAuth issuer — where relocating leaves the server announcing - * one port while its metadata still points at another process entirely, and - * the fixture silently stops reproducing what it exists to reproduce. - */ - strictPort?: boolean; /** * Whether to advertise listChanged capability for each list type * If enabled, modification tools will send list_changed notifications diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 74e9fa5ad..4f08e232a 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -119,8 +119,6 @@ export interface ConfigFile { transport: { type: "stdio" | "streamable-http" | "sse"; port?: number; - /** Bind `port` exactly, or fail — see `ServerConfig.strictPort`. */ - strictPort?: boolean; /** * Serve the modern (2026-07-28) protocol era via the SDK's * `createMcpHandler` (only valid with `type: "streamable-http"`). `true` @@ -198,48 +196,6 @@ function validateConfig( `Invalid config in ${filePath}: transport.type must be stdio, streamable-http, or sse`, ); } - // `strictPort` is consumed as a plain truthiness check at bind time, so a - // string `"false"` would read as *enabled* and silently disable the port walk - // — the opposite of what the author wrote. Validate the type here, where the - // file is being trusted, rather than letting it through as a `ConfigFile`. - if ( - transport.strictPort !== undefined && - typeof transport.strictPort !== "boolean" - ) { - throw new Error( - `Invalid config in ${filePath}: transport.strictPort must be a boolean`, - ); - } - - // Beyond the type: reject every combination that cannot honor the flag's - // contract, because each of them fails *silently* — the fixture looks strict - // and relocates anyway, which is the failure the flag exists to prevent. - // - // - a string port (`"0"`, `"8091"`) is truthy, so it slips past the runtime - // "no port to be strict about" guard, and Node then coerces `"0"` to the - // dynamic port 0; - // - a non-integer or out-of-range port cannot be bound as written; - // - on `stdio` there is no listener at all, and `resolveConfig` drops the - // flag, so it silently does nothing. - if (transport.strictPort === true) { - if (transportType === "stdio") { - throw new Error( - `Invalid config in ${filePath}: transport.strictPort requires an HTTP transport (streamable-http or sse)`, - ); - } - const port = transport.port; - if ( - typeof port !== "number" || - !Number.isInteger(port) || - port < 1 || - port > 65535 - ) { - throw new Error( - `Invalid config in ${filePath}: transport.strictPort requires transport.port to be an integer in 1-65535 (got ${JSON.stringify(port)})`, - ); - } - } - // Only reject *enabling* modern on a non-HTTP transport; a falsy `modern` // (e.g. `false`) is a no-op that `resolveConfig` normalizes away. if (transport.modern && transportType !== "streamable-http") { diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index 8cd876de0..52eaca8ea 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -102,7 +102,6 @@ export function resolveConfig(config: ConfigFile): ServerConfig { ? (transport.type as "sse" | "streamable-http") : undefined, port: isHttp ? transport.port : undefined, - strictPort: isHttp ? transport.strictPort : undefined, }; // Normalize the modern flag: `true` is shorthand for the default (dual-era diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index 73ca7689d..f8a965c3b 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -400,39 +400,9 @@ export class TestServerHttp { const serverType = this.config.serverType ?? "streamable-http"; const requestedPort = this.config.port; - // `strictPort` means "bind exactly this port, or fail". With no fixed port - // to bind there is nothing to be strict about, and falling through to an - // OS-assigned one would let a misconfigured fixture look strict while - // relocating on every run — the exact failure the flag exists to prevent, - // now silent. Reject the combination instead. - if ( - this.config.strictPort && - (typeof requestedPort !== "number" || - !Number.isInteger(requestedPort) || - requestedPort < 1 || - requestedPort > 65535) - ) { - // `loadConfig` rejects these for a config file; this covers a - // programmatic caller, and specifically a value that is *truthy* but not - // bindable as written — a string `"0"` slips past a bare falsiness check - // and Node then coerces it to the dynamic port 0, so the fixture looks - // strict and relocates anyway. - throw new Error( - `strictPort requires an explicit port as an integer in 1-65535 (got ${JSON.stringify(requestedPort)}): ` + - "there is nothing to bind strictly otherwise.", - ); - } - // If a port is explicitly requested, find an available port starting from that value - // Otherwise, use 0 to let the OS assign an available port. - // `strictPort` opts out of the walk: bind the requested port or fail loudly - // (see the field's doc comment — a relocated server whose advertised config - // hard-codes the port is worse than one that does not start). - const port = requestedPort - ? this.config.strictPort - ? requestedPort - : await findAvailablePort(requestedPort) - : 0; + // Otherwise, use 0 to let the OS assign an available port + const port = requestedPort ? await findAvailablePort(requestedPort) : 0; if (serverType === "streamable-http") { return this.startHttp(port); From f12ce5aaf8a8f33d0de467d0fe8b43bb67f62248 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:38:02 -0400 Subject: [PATCH 3/7] fix: correct two false claims in the terminal-notice copy (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are accuracy defects in the one message whose entire job is to be actionable, so both matter more than their size. - "before any credentials were sent" is FALSE on the mid-session refresh and re-authentication paths this same notice serves, where credentials were legitimately sent earlier in the session. A user connected for an hour would read it as describing some other failure. Now scoped to the request actually refused: "without sending this request". - The recovery guidance named "Server Settings → Authorization". The UI renders that accordion as "OAuth Settings" and the field as "Token URL override" (ServerSettingsForm.tsx), so the message directed people to a section that does not exist, precisely when it was asking them to go reconfigure something. Both pinned by tests, including negative assertions on the old wording so it cannot drift back. The round's third finding was against test-server-http.ts's strictPort precondition, which no longer exists — that option was dropped in 6c1b532, after the review ran. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/src/test/core/auth/oauthUx.test.ts | 15 ++++++++++++++- core/auth/oauthUx.ts | 18 +++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index 1ef53e11a..439fdeb00 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -467,7 +467,20 @@ describe("insecureTokenEndpoint copy", () => { expect(message).toContain(ENDPOINT); expect(message).toContain("HTTPS"); expect(message).toContain("127.0.0.1"); - expect(message).toContain("Token URL"); + 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. + expect(message).toContain("OAuth Settings"); + expect(message).not.toContain("Server Settings → Authorization"); + }); + + it("does not claim no credentials were sent, which is false on a refresh", () => { + // The same notice serves mid-session refresh and re-auth, where credentials + // were legitimately sent earlier in the session. Scope the claim to the + // request actually refused. + const message = insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT }); + expect(message).toContain("without sending this request"); + expect(message).not.toContain("before any credentials were sent"); }); it("says a retry cannot help, which is the whole point of the message", () => { diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 1a1dfc4dd..7578187d3 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -383,6 +383,17 @@ export function insecureTokenEndpointTitle(): string { * Does not echo the SDK's own message, which reads as a flat refusal and tells * the user nothing about which lever to reach for. * + * The opening says "without sending **this** request" rather than "before any + * credentials were sent". The absolute form was wrong: this same notice serves + * the mid-session refresh and re-authentication paths, where credentials were + * 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 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 + * message whose entire job is telling them where to go. + * * The scheme half says "not HTTPS" rather than "plain HTTP": the SDK's check is * `protocol !== "https:"`, so anything else an authorization server advertises * — including a mistyped `ftp:` or `ws:` endpoint — lands here too, and naming @@ -403,12 +414,13 @@ export function insecureTokenEndpointMessage(options: { }): string { const target = options.serverName ? `"${options.serverName}"` : "this server"; return ( - `Authorization for ${target} was stopped before any credentials were sent: ` + + `Authorization for ${target} was stopped without sending this request: ` + `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 → Authorization has a Token URL " + - "override if the authorization server advertises a different one." + "those three spellings — Server Settings → OAuth Settings has a " + + '"Token URL override" if the authorization server advertises a different ' + + "one." ); } From 084a083e2db40ec60c6071e5f4ac042f8ed53fde Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:54:17 -0400 Subject: [PATCH 4/7] 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 90051f4a3..58da2bd53 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 a016d3a8a..1acf79ce2 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 2fb94f19e..68fe2840a 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 dde7cd00f..c6c186eb0 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 1207c076f..329e6f0fe 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 932b09459..f5c6c2500 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 439fdeb00..4126264f0 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 9dfa6717e..0374a54a2 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 7578187d3..8b5167ada 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 f8cf1c632..b23003948 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. From 8ca00fa2d6b0353b7c69c88c812ea32e819837f6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 10:45:26 -0400 Subject: [PATCH 5/7] test: make the not-an-OAuthError test assert its own title (review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test was named "is not an OAuthError, which is why the retry path must not claim it" and asserted only `name` and `typeof tokenEndpoint`. Neither touches the OAuth hierarchy, so a future SDK reparenting the class would leave it passing with its title now false — and the comment promising the #2280 handling would be revisited if that changed was hollow. A test that has quietly stopped testing its subject is worse than no test. It now asserts against the hierarchy both ways, `OAuthError.isInstance(err)` and `instanceof`. Verified by mutation rather than assumed: flipping the assertion to simulate the reparenting fails exactly this test and nothing else (1 failed, 15 passed). Also fixes a doubled "terminal terminal", collateral from the round-2 SEP label removal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/src/hooks/useOAuthRecovery.ts | 2 +- .../src/test/core/auth/insecureTokenEndpoint.test.ts | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 329e6f0fe..b1daf7218 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 terminal token-endpoint refusal (#2280) and clear any re-auth banner. + * Report a 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 diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts index 78ff9ccf3..b89aa6804 100644 --- a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from "vitest"; -import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { + InsecureTokenEndpointError, + OAuthError, +} from "@modelcontextprotocol/client"; import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; @@ -21,6 +24,13 @@ describe("SDK brand placement", () => { // do not treat it as a transient authorization failure. If a future SDK // changes that, the #2280 handling should be revisited rather than silently // keeping a now-wrong justification. + // + // Asserted against the hierarchy itself, both ways. Checking only `name` + // and `tokenEndpoint` would leave this passing unchanged if the class were + // reparented — the test would keep its title while having stopped testing + // it, which is worse than not having it. + expect(OAuthError.isInstance(err)).toBe(false); + expect(err instanceof OAuthError).toBe(false); expect(err.name).toBe("InsecureTokenEndpointError"); expect(typeof err.tokenEndpoint).toBe("string"); }); From c2d566ad8d3d5cd084abdee35ba2352da2a103b0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 10:59:56 -0400 Subject: [PATCH 6/7] refactor: one shared reporter for the terminal refusal (review round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In round 2 I claimed a single wrapper "stops a future path omitting the banner clear". That was only true inside useOAuthRecovery. useConnectionLifecycle hand-wrote the notice-plus-scoped-clear pair three times, so the exact failure mode I said was impossible was one careless edit away — in the PR that claimed it. reportTerminalInsecureTokenEndpoint now lives in lib/ and both hooks route through it; zero hand-written pairs remain. It is generic over the banner shape rather than importing ReAuthBannerState, since useOAuthRecovery already imports this module and naming its type would close a cycle. All it needs is a serverId to compare. The helper's header also still said "the three OAuth failure paths". There are seven. Rather than update the count I removed the enumeration and recorded why: a list of callers is a comment that rots on the next round, which is what just happened to it. Also fixes "a /etc/hosts" -> "an /etc/hosts". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../web/src/hooks/useConnectionLifecycle.ts | 51 +++++++++------- clients/web/src/hooks/useOAuthRecovery.ts | 24 +++----- .../src/lib/insecureTokenEndpointNotice.ts | 58 ++++++++++++++++--- docs/test-servers.md | 2 +- 4 files changed, 91 insertions(+), 44 deletions(-) diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 68fe2840a..42447bc6c 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -30,7 +30,7 @@ import { getActiveEnterpriseManagedAuthIdp, } from "@inspector/core/client/types.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; -import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; +import { reportTerminalInsecureTokenEndpoint } from "../lib/insecureTokenEndpointNotice"; import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; @@ -654,14 +654,12 @@ export function useConnectionLifecycle({ // arm below already disconnects for the same reason. if (findInsecureTokenEndpoint(err)) { await client.disconnect().catch(() => {}); - showInsecureTokenEndpointNotice(err, target.name); - // Clear a banner left by an earlier failure: its Re-authenticate - // button is just as dead as the one this arm declines to offer, and - // the user cannot tell which failure it belongs to. Scoped to this - // server, so an async continuation cannot erase another's. - setReAuthBanner((prev) => - prev && prev.serverId === id ? null : prev, - ); + reportTerminalInsecureTokenEndpoint({ + err, + serverId: id, + serverName: target.name, + setReAuthBanner, + }); return; } @@ -706,12 +704,14 @@ export function useConnectionLifecycle({ // the user the authorization *worked*. Placed after the teardown // above, which this arm needs for the same reason the generic one // does, and before the flag it must not set. - if (showInsecureTokenEndpointNotice(recoveryErr, target.name)) { - // Only this server's banner — an async continuation must not - // erase one raised for a server the user has since switched to. - setReAuthBanner((prev) => - prev && prev.serverId === id ? null : prev, - ); + if ( + reportTerminalInsecureTokenEndpoint({ + err: recoveryErr, + serverId: id, + serverName: target.name, + setReAuthBanner, + }) + ) { return; } setFailedServerId(id); @@ -767,10 +767,14 @@ export function useConnectionLifecycle({ } // 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) => - prev && prev.serverId === id ? null : prev, - ); + if ( + reportTerminalInsecureTokenEndpoint({ + err: authErr, + serverId: id, + serverName: target.name, + setReAuthBanner, + }) + ) { return; } // The connect attempt failed, same as any other handshake error — @@ -1021,7 +1025,14 @@ export function useConnectionLifecycle({ // worst place to lose the guidance, since they have just been told // retrying is the fix. No banner clear is needed: this callback // already cleared it before starting. - if (showInsecureTokenEndpointNotice(err, server?.name)) { + if ( + reportTerminalInsecureTokenEndpoint({ + err, + serverId, + serverName: server?.name, + setReAuthBanner, + }) + ) { return; } const message = err instanceof Error ? err.message : String(err); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index b1daf7218..292152f6f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -31,7 +31,7 @@ import { emaStepUpSuccessMessage, } from "@inspector/core/auth/oauthUx.js"; import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; -import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; +import { reportTerminalInsecureTokenEndpoint as reportTerminalInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice"; import type { OAuthDetails } from "../components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { oauthDetailsFromConnectionState } from "../components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState"; import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; @@ -408,21 +408,13 @@ export function useOAuthRecovery({ err: unknown, serverId: string | undefined, serverName?: string, - ): boolean => { - if (!showInsecureTokenEndpointNotice(err, serverName)) { - return false; - } - // Clear only *this* server's banner. The command and deferred-resume - // paths are asynchronous, so server A can reject long after the user - // switched away and server B raised a banner of its own; an unconditional - // clear would then erase B's, which is still valid and still actionable. - // Functional so it sees the queued state rather than the render-time - // value, matching how `setPendingReauth` guards its own late restore. - setReAuthBanner((prev) => - prev && prev.serverId === serverId ? null : prev, - ); - return true; - }, + ): boolean => + reportTerminalInsecureTokenEndpointNotice({ + err, + serverId, + serverName, + setReAuthBanner, + }), [setReAuthBanner], ); diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index f5c6c2500..2049df089 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -1,17 +1,18 @@ /** - * 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). + * Surfaces the SDK's 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 * effect; the copy it renders is pure and lives in `@inspector/core/auth`. * - * Shaped as a claim-or-decline predicate rather than a plain `show(...)` so the - * three OAuth failure paths that need it — the connect handshake, the post- - * redirect callback, and the re-auth banner funnel — can each spend one line on - * it and keep their existing fall-through intact: + * Shaped as a claim-or-decline predicate rather than a plain `show(...)` so + * every OAuth failure path that needs it can spend one line and keep its own + * fall-through intact. Deliberately not enumerating the callers here: they have + * gone from three to seven over this PR's review, and a list is a comment that + * rots on the next one. * * ```ts - * if (showInsecureTokenEndpointNotice(err, server.name)) return; + * if (reportTerminalInsecureTokenEndpoint({ err, serverId: id, serverName, setReAuthBanner })) return; * ``` * * `autoClose: false` matches the other non-recoverable OAuth notices (issuer @@ -22,6 +23,7 @@ * reading. Don't describe this as non-dismissible. */ +import type { Dispatch, SetStateAction } from "react"; import { notifications } from "@mantine/notifications"; import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; import { @@ -35,6 +37,48 @@ import { * @returns `true` when it was handled (the caller should stop), `false` when * `err` is some other failure and the caller's normal handling applies. */ +/** + * Report the refusal **and** clear the re-auth banner for that server. + * + * This is the form every caller should use. The notice and the scoped banner + * clear are one invariant, not two steps: a banner left behind carries a + * Re-authenticate button as dead as the one this declines to offer, and the + * user cannot tell which failure it belongs to. Keeping them together in one + * place is what stops a future path doing half of it — an earlier revision made + * that claim while hand-writing the pair at three call sites in a second hook, + * which is exactly how it goes wrong. + * + * The clear is scoped to `serverId` and applied as a functional update: these + * paths are asynchronous, so a late continuation for server A must not erase a + * banner server B raised in the meantime. + * + * Generic over the banner shape rather than importing `ReAuthBannerState`: + * `useOAuthRecovery` already imports this module, so naming its type here would + * close a cycle. All this needs is a `serverId` to compare. + * + * @returns `true` when it was handled (the caller should stop), `false` when + * `err` is some other failure and the caller's normal handling applies. + */ +export function reportTerminalInsecureTokenEndpoint< + TBanner extends { serverId: string }, +>({ + err, + serverId, + serverName, + setReAuthBanner, +}: { + err: unknown; + serverId: string | undefined; + serverName?: string; + setReAuthBanner: Dispatch>; +}): boolean { + if (!showInsecureTokenEndpointNotice(err, serverName)) { + return false; + } + setReAuthBanner((prev) => (prev && prev.serverId === serverId ? null : prev)); + return true; +} + export function showInsecureTokenEndpointNotice( err: unknown, serverName?: string, diff --git a/docs/test-servers.md b/docs/test-servers.md index b23003948..521435ec5 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -461,7 +461,7 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be ⚠️ **Do not run this fixture while port 8091 is already taken.** Like every fixture here it walks to the next free port on `EADDRINUSE` — but its `issuerUrl` is a *fixed string*, so a relocated server announces 8092 while all its OAuth metadata still points at whatever unrelated process holds 8091. The symptom is confusing rather than obvious: discovery reaches the wrong process, and the refusal this fixture exists to demonstrate either never fires or fires for the wrong reason. If the flow does not end at the notice described below, check that 8091 is actually this server (`lsof -nP -iTCP:8091 -sTCP:LISTEN`) before believing anything you see. -The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq. +The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without an `/etc/hosts` entry or dnsmasq. 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. From a5a9d4fe15e3d5a8a22dade096ed0046d0c7ca12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 11:16:51 -0400 Subject: [PATCH 7/7] fix: the classifier does not survive structuredClone, and the test now proves it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since round 1 the doc claimed the `name` fallback covers "a structured clone or a JSON hop". Verified, and it is false for half of that: structuredClone(new InsecureTokenEndpointError(url)) -> name: "Error", tokenEndpoint: undefined, constructor: Error structuredClone normalizes a custom Error subclass back to Error, so neither arm has anything left to match. A caller routing this error through structuredClone or postMessage would silently get the generic retryable handling back — the exact defect this PR removes. The test could not have caught it: it hand-built a look-alike plain object, so it asserted what I believed the boundary does rather than what it does. It now performs a real JSON round trip (asserting the prototype is gone), and a second case pins that structuredClone is NOT recognized — turning a false claim into a tested limitation. The doc comment now scopes the contract to JSON and states the structuredClone limit with the remedy: serialize the fields explicitly across such a boundary. Also removes an orphaned JSDoc block the round-4 refactor stranded above the report helper, and gives showInsecureTokenEndpointNotice its own doc back. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/lib/insecureTokenEndpointNotice.ts | 16 +++++--- .../core/auth/insecureTokenEndpoint.test.ts | 39 ++++++++++++++----- core/auth/insecureTokenEndpoint.ts | 16 ++++++-- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index 2049df089..2d8acb7f0 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -31,12 +31,6 @@ import { insecureTokenEndpointTitle, } from "../utils/oauthUx"; -/** - * Show the terminal notice when `err` is the SDK's `InsecureTokenEndpointError`. - * - * @returns `true` when it was handled (the caller should stop), `false` when - * `err` is some other failure and the caller's normal handling applies. - */ /** * Report the refusal **and** clear the re-auth banner for that server. * @@ -79,6 +73,16 @@ export function reportTerminalInsecureTokenEndpoint< return true; } +/** + * Show the notice alone, without touching the banner. + * + * Prefer {@link reportTerminalInsecureTokenEndpoint} — the two are one + * invariant. This stays exported for the one arm that has already cleared the + * banner itself before starting. + * + * @returns `true` when it was handled (the caller should stop), `false` when + * `err` is some other failure and the caller's normal handling applies. + */ export function showInsecureTokenEndpointNotice( err: unknown, serverName?: string, diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts index b89aa6804..ed855d61d 100644 --- a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -43,16 +43,37 @@ describe("findInsecureTokenEndpoint", () => { ).toMatchObject({ tokenEndpoint: ENDPOINT }); }); - it("recognizes a serialized copy by `name`, where the prototype is gone", () => { - // The fallback arm: a structured clone or JSON hop drops the prototype and - // the brand set but keeps `name`. - expect( - findInsecureTokenEndpoint({ - name: "InsecureTokenEndpointError", - message: "Refusing to send credentials…", - tokenEndpoint: ENDPOINT, + it("recognizes a JSON round trip, where the prototype is gone", () => { + // An ACTUAL round trip, not a hand-built look-alike: constructing the + // object by hand asserts what I believed the boundary does rather than what + // it does. A JSON hop drops the prototype and the brand set while keeping + // `name` and `tokenEndpoint`, which is the case this fallback exists for. + const err = new InsecureTokenEndpointError(ENDPOINT); + const hopped: unknown = JSON.parse( + JSON.stringify({ + name: err.name, + message: err.message, + tokenEndpoint: err.tokenEndpoint, }), - ).toMatchObject({ tokenEndpoint: ENDPOINT }); + ); + expect(Object.getPrototypeOf(hopped)).toBe(Object.prototype); + expect(findInsecureTokenEndpoint(hopped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("does NOT survive structuredClone, and this pins that limit", () => { + // Verified, not assumed: structuredClone normalizes a custom Error subclass + // back to `Error`, so `name` becomes "Error" and `tokenEndpoint` is dropped + // — nothing is left for either arm to match. An earlier revision of the doc + // comment claimed this boundary worked; it does not, and a caller relying on + // it would silently get the generic retryable handling back. + const cloned = structuredClone(new InsecureTokenEndpointError(ENDPOINT)); + expect(cloned.name).toBe("Error"); + expect( + (cloned as { tokenEndpoint?: unknown }).tokenEndpoint, + ).toBeUndefined(); + expect(findInsecureTokenEndpoint(cloned)).toBeUndefined(); }); it("rejects a look-alike carrying the endpoint but not the identity", () => { diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts index 0374a54a2..b943c6a40 100644 --- a/core/auth/insecureTokenEndpoint.ts +++ b/core/auth/insecureTokenEndpoint.ts @@ -36,10 +36,18 @@ export interface InsecureTokenEndpointShape { * on an instance — don't check that property.) * * The `name` comparison is the same deliberate serialization fallback - * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`: today the - * web client runs `auth()` in the browser, so no boundary is crossed, but the - * prototype and brand set are the first things a structured clone or a JSON hop - * would drop, and `name` survives both. + * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`. Today the + * web client runs `auth()` in the browser so no boundary is crossed, but a JSON + * hop drops the prototype and the brand set while preserving `name` and + * `tokenEndpoint`, and that is the case this arm exists for. + * + * ⚠️ **It does not cover `structuredClone`, and cannot.** That algorithm + * normalizes a custom `Error` subclass back to `Error` — `name` becomes + * `"Error"` and own properties like `tokenEndpoint` are dropped entirely — so + * nothing survives for either arm to match on. A caller who routes this error + * through `structuredClone` (or `postMessage`, which uses it) will silently get + * the generic retryable handling back. Serialize the fields explicitly across + * such a boundary rather than relying on this classifier. */ function isInsecureTokenEndpointShape( err: unknown,