From 716ce0c4cd535bc65a86eb179cc852c674edb8d8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 16:05:26 -0400 Subject: [PATCH 1/2] fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --- AGENTS.md | 6 +- clients/web/src/App.test.tsx | 182 +++++++++++++++++- clients/web/src/App.tsx | 85 ++++++++ .../ReAuthBanner/ReAuthBanner.stories.tsx | 44 +++++ .../groups/ReAuthBanner/ReAuthBanner.test.tsx | 23 +++ .../groups/ReAuthBanner/ReAuthBanner.tsx | 18 +- .../src/test/core/auth/issuerBinding.test.ts | 160 +++++++++++++++ .../web/src/test/core/auth/oauthUx.test.ts | 70 +++++++ .../auth/runner-interactive-oauth.test.ts | 74 +++++++ clients/web/src/utils/oauthUx.ts | 6 + core/auth/index.ts | 12 ++ core/auth/issuerBinding.ts | 161 ++++++++++++++++ core/auth/node/runner-interactive-oauth.ts | 22 ++- core/auth/oauthUx.ts | 78 ++++++++ 14 files changed, 935 insertions(+), 6 deletions(-) create mode 100644 clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.stories.tsx create mode 100644 clients/web/src/test/core/auth/issuerBinding.test.ts create mode 100644 core/auth/issuerBinding.ts diff --git a/AGENTS.md b/AGENTS.md index 08944062d..beb29caf5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,7 +31,11 @@ inspector/ │ ├── auth/ # OAuth: providers, discovery, OAuthStorage + persist backends; │ │ # mid-session recovery (challenge.ts WWW-Authenticate │ │ # parsing, scopes.ts SEP-2350 scope union, oauthUx.ts -│ │ # shared copy, mcpAuth.ts force-reauthorization) +│ │ # shared copy, mcpAuth.ts force-reauthorization, +│ │ # issuerBinding.ts SEP-2352 callback-leg failure +│ │ # classification — separates a recoverable +│ │ # "lost authorization state" from a genuine +│ │ # cross-AS issuer mismatch — #1808) │ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation) │ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer, │ │ │ # runner-interactive-oauth loopback callback flow) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index e8534fe36..5d89736f6 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -47,6 +47,9 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { // When set, the next `connect()` rejects with this value (one-shot), so a test // can exercise the handshake-failure path. Cleared after it fires. let nextConnectRejection: unknown = null; + // Same one-shot arming for the `/oauth/callback` token exchange, so a test can + // exercise the callback-leg failure paths (#1808). + let nextResumeRejection: unknown = null; class FakeInspectorClient extends EventTarget { connect = vi.fn(() => { if (nextConnectRejection !== null) { @@ -93,8 +96,16 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { getRoots = vi.fn().mockReturnValue([]); setRoots = vi.fn().mockResolvedValue(undefined); setServerSettings = vi.fn(); - resumeAfterOAuth = vi.fn().mockResolvedValue(undefined); + resumeAfterOAuth = vi.fn(() => { + if (nextResumeRejection !== null) { + const err = nextResumeRejection; + nextResumeRejection = null; + return Promise.reject(err); + } + return Promise.resolve(undefined); + }); checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(true); + clearOAuthTokens = vi.fn().mockResolvedValue(undefined); } const instances: FakeInspectorClient[] = []; return { @@ -110,6 +121,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { __rejectNextConnect: (err: unknown) => { nextConnectRejection = err; }, + // Test-only: arm the next resumeAfterOAuth() to reject (callback-leg failure). + __rejectNextResumeAfterOAuth: (err: unknown) => { + nextResumeRejection = err; + }, }; }); @@ -563,9 +578,11 @@ import { useManagedRequestorTasks } from "@inspector/core/react/useManagedReques import { useMessageLog } from "@inspector/core/react/useMessageLog.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; +import { useServers } from "@inspector/core/react/useServers.js"; import type { InspectorServerSettings, MessageEntry, + ServerEntry, } from "@inspector/core/mcp/types.js"; // Default useInspectorClient return — capabilities empty (no task tool calls). @@ -590,6 +607,12 @@ const rejectNextConnect = ( McpIndex as unknown as { __rejectNextConnect: (err: unknown) => void } ).__rejectNextConnect; +const rejectNextResumeAfterOAuth = ( + McpIndex as unknown as { + __rejectNextResumeAfterOAuth: (err: unknown) => void; + } +).__rejectNextResumeAfterOAuth; + const fetchLogInstances = ( FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] } ).__fetchLogInstances; @@ -2154,6 +2177,163 @@ describe("App OAuth resume lifecycle", () => { }); }); +// A callback that lands without recorded SEP-2352 discovery state used to +// dead-end with the SDK's raw `AuthorizationServerMismatchError` text. It now +// raises an explicit recovery affordance — but only for that case; a genuine +// cross-authorization-server mismatch stays a security error. See #1808. +describe("App OAuth callback issuer-binding failures (#1808)", () => { + const storage = new Map(); + const HTTP_SERVER: ServerEntry = { + id: "A", + name: "PlotRocket", + config: { type: "streamable-http", url: "https://mcp.example.com/mcp" }, + connection: { status: "disconnected" }, + }; + const STDIO_SERVER: ServerEntry = { + id: "A", + name: "PlotRocket", + config: { type: "stdio", command: "node" }, + connection: { status: "disconnected" }, + }; + + /** Full `useServers` shape so the override typechecks like the real hook. */ + const useServersResult = ( + servers: ServerEntry[], + ): ReturnType => ({ + servers, + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: vi.fn().mockResolvedValue(undefined), + updateServerSettings: updateServerSettingsSpy, + removeServer: vi.fn().mockResolvedValue(undefined), + reorderServers: vi.fn().mockResolvedValue(undefined), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + + const mismatchError = (recordedIssuer: string): Error => { + const err = new Error("Authorization server changed"); + Object.defineProperty(err, "mcpBrand", { + value: "mcp.AuthorizationServerMismatchError", + }); + Object.assign(err, { + recordedIssuer, + currentIssuer: "https://as.example.com", + }); + return err; + }; + + const renderCallbackWithFailure = (err: Error) => { + writeOAuthResumeSnapshot({ + version: 1, + serverId: "A", + activeTab: "Tools", + authKind: "reauth", + tabUi: {}, + }); + window.history.replaceState({}, "", `${OAUTH_CALLBACK_PATH}?code=test`); + rejectNextResumeAfterOAuth(err); + renderWithMantine(); + }; + + beforeEach(() => { + clientInstances.length = 0; + storage.clear(); + notificationsMock.show.mockClear(); + vi.mocked(useInspectorClient).mockReturnValue({ + ...DEFAULT_USE_INSPECTOR_CLIENT, + status: "disconnected", + }); + vi.mocked(useServers).mockReturnValue(useServersResult([HTTP_SERVER])); + vi.stubGlobal("sessionStorage", { + getItem: (key: string) => storage.get(key) ?? null, + setItem: (key: string, value: string) => { + storage.set(key, value); + }, + removeItem: (key: string) => { + storage.delete(key); + }, + }); + window.history.replaceState({}, "", "/"); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT); + vi.mocked(useServers).mockReturnValue(useServersResult([STDIO_SERVER])); + window.history.replaceState({}, "", "/"); + }); + + it("offers an explicit re-authorize affordance when the recorded state was lost", async () => { + renderCallbackWithFailure( + mismatchError( + "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier", + ), + ); + + expect( + await screen.findByText("Authorization state was lost"), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Authorize again" }), + ).toBeInTheDocument(); + // The raw SDK wording never reaches the user. + expect(screen.queryByText(/discoveryState/)).not.toBeInTheDocument(); + expect(screen.queryByText("Re-authentication required")).toBeNull(); + }); + + it("clears the stale OAuth state and reconnects when the affordance is used", async () => { + const user = userEvent.setup(); + renderCallbackWithFailure( + mismatchError("discoveryState was not available on the callback leg"), + ); + + const button = await screen.findByRole("button", { + name: "Authorize again", + }); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + const callbackClient = clientInstances[0] as unknown as { + clearOAuthTokens: ReturnType; + }; + + await user.click(button); + + await waitFor(() => + expect(callbackClient.clearOAuthTokens).toHaveBeenCalled(), + ); + // A fresh client is built for the retry connect. + await waitFor(() => expect(clientInstances.length).toBeGreaterThan(1)); + expect(screen.queryByText("Authorization state was lost")).toBeNull(); + }); + + it("treats a genuine issuer mismatch as a security error with no recovery button", async () => { + renderCallbackWithFailure(mismatchError("https://old.example.com")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Authorization server mismatch", + color: "red", + }), + ), + ); + expect(screen.queryByText("Authorization state was lost")).toBeNull(); + expect( + screen.queryByRole("button", { name: "Authorize again" }), + ).toBeNull(); + }); + + it("falls back to the generic re-auth banner for an unrelated callback failure", async () => { + renderCallbackWithFailure(new Error("token endpoint exploded")); + + expect( + await screen.findByText("Re-authentication required"), + ).toBeInTheDocument(); + expect(screen.queryByText("Authorization state was lost")).toBeNull(); + }); +}); + describe("App paginated list pagination toggle (#1721)", () => { beforeEach(() => { clientInstances.length = 0; diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index aa46f6518..ee099028f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -197,12 +197,16 @@ import { clearServerOAuthState } from "./lib/clearServerOAuthState"; import { authRecoveryRestoredMessage, isReAuthBannerReason, + issuerBindingFailureCopy, + lostAuthorizationStateActionLabel, + lostAuthorizationStateTitle, oauthPreRedirectToastCopy, oauthResumeAbandonedMessage, reAuthBannerMessage, type OAuthPreRedirectContext, type OAuthRecoverySource, } from "./utils/oauthUx"; +import { findIssuerBindingFailure } from "@inspector/core/auth/issuerBinding.js"; import { isBrowserTabVisible, onBrowserTabVisible, @@ -879,6 +883,13 @@ function App() { const [reAuthBanner, setReAuthBanner] = useState<{ serverId: string; message: string; + /** + * `lost_authorization_state` marks the SEP-2352 recovery case (#1808): the + * callback arrived with no recorded discovery state, so the banner offers + * "Authorize again", which drops the stale/partial OAuth state before + * starting a fresh authorization. + */ + kind?: "lost_authorization_state"; } | null>(null); const [pendingReauth, setPendingReauth] = useState( null, @@ -2559,6 +2570,35 @@ function App() { }); return; } + // SEP-2352 issuer binding (#1808). Two very different failures share + // one SDK error class, so classify before falling through to the + // generic re-auth banner (whose detail line would otherwise be the raw + // "AuthorizationServerMismatchError" text): + // - no discovery state was recorded → recoverable bookkeeping loss, + // surface the "Authorize again" affordance; + // - a *different* issuer answered the callback → security signal, so + // no one-click recovery is offered. + const issuerBindingFailure = findIssuerBindingFailure(err); + if (issuerBindingFailure) { + const copy = issuerBindingFailureCopy(issuerBindingFailure, { + serverName: server.name, + }); + if (issuerBindingFailure.kind === "lost_authorization_state") { + setReAuthBanner({ + serverId: server.id, + message: copy.message, + kind: "lost_authorization_state", + }); + } else { + notifications.show({ + title: copy.title, + message: copy.message, + color: "red", + autoClose: false, + }); + } + return; + } queueMicrotask(() => { showReAuthBanner(server.id, err instanceof Error ? err : String(err)); }); @@ -2839,8 +2879,42 @@ function App() { const onReauthenticateFromBanner = useCallback(() => { if (!reAuthBanner) return; const serverId = reAuthBanner.serverId; + const bannerKind = reAuthBanner.kind; setReAuthBanner(null); + // Lost-authorization-state recovery (#1808). The stale half of the flow + // (code verifier without discovery state, possibly a stale registration) + // would make a plain retry fail the same way, so drop the persisted OAuth + // state for this server first, then start a fresh authorization. This + // banner is only raised from the `/oauth/callback` failure path, where the + // session never reached "connected", so connecting is always the right + // toggle direction here. + if (bannerKind === "lost_authorization_state") { + void (async () => { + const server = serversRef.current.find((s) => s.id === serverId); + if (server) { + try { + await clearServerOAuthState({ + config: server.config, + inspectorClient: + serverId === activeServerId ? inspectorClient : null, + isActiveConnection: serverId === activeServerId, + oauthStorage: webOAuthStorage, + }); + } catch (err) { + notifications.show({ + title: "Could not clear the stored authorization state", + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + return; + } + } + await onToggleConnection(serverId); + })(); + return; + } + if ( serverId === activeServerId && connectionStatus === "connected" && @@ -2888,6 +2962,7 @@ function App() { servers, prepareOAuthRedirect, onToggleConnection, + webOAuthStorage, ]); // --- Action handlers that route directly to the InspectorClient. --- @@ -4259,6 +4334,16 @@ function App() { setReAuthBanner(null)} /> diff --git a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.stories.tsx b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.stories.tsx new file mode 100644 index 000000000..f5cbbd1c2 --- /dev/null +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.stories.tsx @@ -0,0 +1,44 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { fn } from "storybook/test"; +import { ReAuthBanner } from "./ReAuthBanner"; +import { + lostAuthorizationStateActionLabel, + lostAuthorizationStateMessage, + lostAuthorizationStateTitle, + reAuthBannerMessage, +} from "../../../utils/oauthUx"; + +const meta: Meta = { + title: "Groups/ReAuthBanner", + component: ReAuthBanner, + args: { + onReauthenticate: fn(), + onDismiss: fn(), + }, +}; + +export default meta; +type Story = StoryObj; + +/** Degraded session: the stored token no longer works. */ +export const SessionNeedsAttention: Story = { + args: { + message: reAuthBannerMessage({ + serverName: "PlotRocket", + detail: "Token expired.", + }), + }, +}; + +/** + * SEP-2352 callback leg arrived with no recorded discovery state (#1808). + * Explains the loss in plain language and offers a one-click fresh + * authorization (which clears the stale state first). + */ +export const AuthorizationStateLost: Story = { + args: { + title: lostAuthorizationStateTitle(), + actionLabel: lostAuthorizationStateActionLabel(), + message: lostAuthorizationStateMessage({ serverName: "PlotRocket" }), + }, +}; diff --git a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.test.tsx b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.test.tsx index 974dffb11..7f34e5fbe 100644 --- a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.test.tsx +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.test.tsx @@ -27,4 +27,27 @@ describe("ReAuthBanner", () => { await user.click(closeButton); expect(onDismiss).toHaveBeenCalledOnce(); }); + + it("renders a custom title and action label (lost-state recovery, #1808)", async () => { + const user = userEvent.setup(); + const onReauthenticate = vi.fn(); + + renderWithMantine( + , + ); + + expect( + screen.getByText("Authorization state was lost"), + ).toBeInTheDocument(); + expect(screen.queryByText("Re-authentication required")).toBeNull(); + + await user.click(screen.getByRole("button", { name: "Authorize again" })); + expect(onReauthenticate).toHaveBeenCalledOnce(); + }); }); diff --git a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx index 38bd9bb44..7bf095907 100644 --- a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx @@ -1,7 +1,14 @@ import { Alert, Button, Group, Text } from "@mantine/core"; +export const DEFAULT_REAUTH_BANNER_TITLE = "Re-authentication required"; +export const DEFAULT_REAUTH_BANNER_ACTION_LABEL = "Re-authenticate"; + export interface ReAuthBannerProps { message: string; + /** Heading; defaults to {@link DEFAULT_REAUTH_BANNER_TITLE}. */ + title?: string; + /** Action button label; defaults to {@link DEFAULT_REAUTH_BANNER_ACTION_LABEL}. */ + actionLabel?: string; onReauthenticate: () => void; onDismiss: () => void; } @@ -9,8 +16,11 @@ export interface ReAuthBannerProps { const ReAuthAlert = Alert.withProps({ color: "red", variant: "reauth", - title: "Re-authentication required", + title: DEFAULT_REAUTH_BANNER_TITLE, withCloseButton: true, + // Mantine's close button renders icon-only; without a label it has no + // accessible name (axe `button-name`). + closeButtonLabel: "Dismiss", }); const BannerRow = Group.withProps({ @@ -32,14 +42,16 @@ const ReAuthButton = Button.withProps({ export function ReAuthBanner({ message, + title = DEFAULT_REAUTH_BANNER_TITLE, + actionLabel = DEFAULT_REAUTH_BANNER_ACTION_LABEL, onReauthenticate, onDismiss, }: ReAuthBannerProps) { return ( - + {message} - Re-authenticate + {actionLabel} ); diff --git a/clients/web/src/test/core/auth/issuerBinding.test.ts b/clients/web/src/test/core/auth/issuerBinding.test.ts new file mode 100644 index 000000000..34a3e94a6 --- /dev/null +++ b/clients/web/src/test/core/auth/issuerBinding.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect } from "vitest"; +import { + findIssuerBindingFailure, + isLostAuthorizationStateError, +} from "@inspector/core/auth/issuerBinding.js"; + +/** + * Builds an object shaped like the SDK's `AuthorizationServerMismatchError` + * (brand + the two issuer fields). The classifier is intentionally structural, + * not `instanceof`, so a plain object is the honest fixture here. + */ +function mismatchError(recordedIssuer: string, currentIssuer: string): Error { + const err = new Error( + "Authorization server changed between redirect and callback", + ); + Object.defineProperty(err, "mcpBrand", { + value: "mcp.AuthorizationServerMismatchError", + }); + Object.assign(err, { recordedIssuer, currentIssuer }); + return err; +} + +const MISSING_STATE_SENTINEL = + "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier"; + +describe("findIssuerBindingFailure", () => { + it("classifies the missing-discovery-state sentinel as recoverable", () => { + const failure = findIssuerBindingFailure( + mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"), + ); + expect(failure).toEqual({ + kind: "lost_authorization_state", + currentIssuer: "https://as.example.com", + }); + }); + + it("classifies non-URL prose in the recorded slot as recoverable", () => { + const failure = findIssuerBindingFailure( + mismatchError("nothing was recorded, sorry", "https://as.example.com"), + ); + expect(failure?.kind).toBe("lost_authorization_state"); + }); + + it("classifies a non-http(s) URL in the recorded slot as recoverable", () => { + const failure = findIssuerBindingFailure( + mismatchError("urn:example:not-an-issuer", "https://as.example.com"), + ); + expect(failure?.kind).toBe("lost_authorization_state"); + }); + + it("classifies two real issuers as a genuine mismatch", () => { + const failure = findIssuerBindingFailure( + mismatchError("https://old.example.com", "https://evil.example.com"), + ); + expect(failure).toEqual({ + kind: "issuer_mismatch", + recordedIssuer: "https://old.example.com", + currentIssuer: "https://evil.example.com", + }); + }); + + it("accepts a plain http issuer as a genuine mismatch", () => { + const failure = findIssuerBindingFailure( + mismatchError("http://localhost:9000", "http://localhost:9001"), + ); + expect(failure?.kind).toBe("issuer_mismatch"); + }); + + it("walks the `cause` chain", () => { + const wrapped = new Error("negotiation failed", { + cause: mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"), + }); + expect(findIssuerBindingFailure(wrapped)?.kind).toBe( + "lost_authorization_state", + ); + }); + + it("walks `data.cause` (SdkError wrapper shape)", () => { + const wrapped = Object.assign(new Error("ERA_NEGOTIATION_FAILED"), { + data: { + cause: mismatchError("https://a.example.com", "https://b.example.com"), + }, + }); + expect(findIssuerBindingFailure(wrapped)?.kind).toBe("issuer_mismatch"); + }); + + it("walks multiple levels", () => { + const wrapped = new Error("outer", { + cause: new Error("inner", { + cause: mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"), + }), + }); + expect(findIssuerBindingFailure(wrapped)?.kind).toBe( + "lost_authorization_state", + ); + }); + + it("ignores a non-object `data`", () => { + const wrapped = Object.assign(new Error("outer"), { data: "nope" }); + expect(findIssuerBindingFailure(wrapped)).toBeUndefined(); + }); + + it("ignores a null `data`", () => { + const wrapped = Object.assign(new Error("outer"), { data: null }); + expect(findIssuerBindingFailure(wrapped)).toBeUndefined(); + }); + + it("terminates on a cyclic cause chain", () => { + const outer: { cause?: unknown } = new Error("outer"); + outer.cause = outer; + expect(findIssuerBindingFailure(outer)).toBeUndefined(); + }); + + it("returns undefined for unrelated errors and non-objects", () => { + expect(findIssuerBindingFailure(new Error("boom"))).toBeUndefined(); + expect(findIssuerBindingFailure(undefined)).toBeUndefined(); + expect(findIssuerBindingFailure(null)).toBeUndefined(); + expect(findIssuerBindingFailure("string")).toBeUndefined(); + }); + + it("ignores a branded error missing the issuer fields", () => { + const err = new Error("half-shaped"); + Object.defineProperty(err, "mcpBrand", { + value: "mcp.AuthorizationServerMismatchError", + }); + expect(findIssuerBindingFailure(err)).toBeUndefined(); + + const halfShaped = new Error("only recorded"); + Object.defineProperty(halfShaped, "mcpBrand", { + value: "mcp.AuthorizationServerMismatchError", + }); + Object.assign(halfShaped, { recordedIssuer: "https://as.example.com" }); + expect(findIssuerBindingFailure(halfShaped)).toBeUndefined(); + }); + + it("ignores a different SDK brand", () => { + const err = Object.assign(new Error("other"), { + mcpBrand: "mcp.IssuerMismatchError", + recordedIssuer: "https://a.example.com", + currentIssuer: "https://b.example.com", + }); + expect(findIssuerBindingFailure(err)).toBeUndefined(); + }); +}); + +describe("isLostAuthorizationStateError", () => { + it("is true only for the missing-discovery-state case", () => { + expect( + isLostAuthorizationStateError( + mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"), + ), + ).toBe(true); + expect( + isLostAuthorizationStateError( + mismatchError("https://a.example.com", "https://b.example.com"), + ), + ).toBe(false); + expect(isLostAuthorizationStateError(new Error("boom"))).toBe(false); + }); +}); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index f209c9ab2..bc899717c 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -19,6 +19,12 @@ import { stepUpInsufficientScopeMessage, stepUpModalTitle, stepUpAuthorizeActionLabel, + issuerBindingFailureCopy, + issuerMismatchMessage, + issuerMismatchTitle, + lostAuthorizationStateActionLabel, + lostAuthorizationStateMessage, + lostAuthorizationStateTitle, } from "@inspector/core/auth/oauthUx.js"; import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; @@ -334,3 +340,67 @@ describe("oauthUx re-auth banner", () => { expect(reAuthBannerMessage({})).toBe("Authentication needs attention."); }); }); + +describe("oauthUx issuer-binding copy", () => { + it("explains lost authorization state in plain language, with the server name", () => { + const withName = lostAuthorizationStateMessage({ serverName: "svc" }); + expect(withName).toContain('"svc"'); + expect(withName).toContain("was lost"); + expect(withName).toContain("Authorize again"); + // Never leaks the SDK's security-flavoured wording. + expect(withName).not.toContain("discoveryState"); + expect(withName).not.toContain("AuthorizationServerMismatchError"); + expect(lostAuthorizationStateMessage()).toContain("this server"); + expect(lostAuthorizationStateTitle()).toBe("Authorization state was lost"); + expect(lostAuthorizationStateActionLabel()).toBe("Authorize again"); + }); + + it("names both issuers for a genuine mismatch and offers no retry nudge", () => { + const message = issuerMismatchMessage({ + recordedIssuer: "https://old.example.com", + currentIssuer: "https://evil.example.com", + serverName: "svc", + }); + expect(message).toContain('"svc"'); + expect(message).toContain("https://old.example.com"); + expect(message).toContain("https://evil.example.com"); + expect(message).toContain("was not exchanged"); + expect(message).not.toContain("Authorize again"); + expect( + issuerMismatchMessage({ + recordedIssuer: "https://old.example.com", + currentIssuer: "https://evil.example.com", + }), + ).toContain("this server"); + expect(issuerMismatchTitle()).toBe("Authorization server mismatch"); + }); + + it("issuerBindingFailureCopy dispatches on the failure kind", () => { + expect( + issuerBindingFailureCopy( + { + kind: "lost_authorization_state", + currentIssuer: "https://as.example.com", + }, + { serverName: "svc" }, + ), + ).toEqual({ + title: lostAuthorizationStateTitle(), + message: lostAuthorizationStateMessage({ serverName: "svc" }), + }); + + expect( + issuerBindingFailureCopy({ + kind: "issuer_mismatch", + recordedIssuer: "https://old.example.com", + currentIssuer: "https://evil.example.com", + }), + ).toEqual({ + title: issuerMismatchTitle(), + message: issuerMismatchMessage({ + recordedIssuer: "https://old.example.com", + currentIssuer: "https://evil.example.com", + }), + }); + }); +}); diff --git a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts index d572e0bb7..033487c2a 100644 --- a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts +++ b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts @@ -249,6 +249,80 @@ describe("runRunnerInteractiveOAuth", () => { ).rejects.toThrow("token exchange failed"); }); + it("rewrites a lost-authorization-state callback failure into actionable copy (#1808)", async () => { + const redirectUrlProvider = { redirectUrl: "" }; + const sdkError = new Error("Authorization server changed"); + Object.defineProperty(sdkError, "mcpBrand", { + value: "mcp.AuthorizationServerMismatchError", + }); + Object.assign(sdkError, { + recordedIssuer: + "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier", + currentIssuer: "https://as.example", + }); + const client = mockClient({ + authenticate: vi.fn(async () => { + await simulateCallback(handlers.current); + return new URL("https://as.example/authorize"); + }), + completeOAuthFlow: vi.fn(async () => { + throw sdkError; + }), + }); + + const failure = await runRunnerInteractiveOAuth({ + client, + redirectUrlProvider, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }).catch((err: unknown) => err); + + expect(failure).toBeInstanceOf(Error); + const message = (failure as Error).message; + expect(message).toContain("Authorization state was lost"); + expect(message).toContain("Authorize again"); + expect(message).not.toContain("discoveryState"); + expect((failure as Error).cause).toBe(sdkError); + }); + + it("keeps the security wording for a genuine issuer mismatch (#1808)", async () => { + const redirectUrlProvider = { redirectUrl: "" }; + const sdkError = Object.assign(new Error("Authorization server changed"), { + mcpBrand: "mcp.AuthorizationServerMismatchError", + recordedIssuer: "https://old.example", + currentIssuer: "https://evil.example", + }); + const client = mockClient({ + authenticate: vi.fn(async () => { + await simulateCallback(handlers.current); + return new URL("https://as.example/authorize"); + }), + completeOAuthFlow: vi.fn(async () => { + throw sdkError; + }), + }); + + const failure = await runRunnerInteractiveOAuth({ + client, + redirectUrlProvider, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }).catch((err: unknown) => err); + + const message = (failure as Error).message; + expect(message).toContain("Authorization server mismatch"); + expect(message).toContain("https://evil.example"); + expect(message).not.toContain("Authorize again"); + }); + it("propagates OAuth callback errors from the authorization server", async () => { const redirectUrlProvider = { redirectUrl: "" }; const client = mockClient({ diff --git a/clients/web/src/utils/oauthUx.ts b/clients/web/src/utils/oauthUx.ts index 4485c48a4..ba86edf78 100644 --- a/clients/web/src/utils/oauthUx.ts +++ b/clients/web/src/utils/oauthUx.ts @@ -6,6 +6,12 @@ export { oauthResumeSuccessMessage, isReAuthBannerReason, reAuthBannerMessage, + issuerBindingFailureCopy, + lostAuthorizationStateActionLabel, + lostAuthorizationStateMessage, + lostAuthorizationStateTitle, + issuerMismatchMessage, + issuerMismatchTitle, type OAuthInteractiveAuthKind, type OAuthPreRedirectContext, type OAuthRecoverySource, diff --git a/core/auth/index.ts b/core/auth/index.ts index bf4a098f8..66a597d09 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -98,9 +98,21 @@ export { oauthPreRedirectToastCopy, isReAuthBannerReason, reAuthBannerMessage, + lostAuthorizationStateTitle, + lostAuthorizationStateMessage, + lostAuthorizationStateActionLabel, + issuerMismatchTitle, + issuerMismatchMessage, + issuerBindingFailureCopy, type OAuthInteractiveAuthKind, } from "./oauthUx.js"; +export { + findIssuerBindingFailure, + isLostAuthorizationStateError, + type IssuerBindingFailure, +} from "./issuerBinding.js"; + // Discovery export { discoverScopes } from "./discovery.js"; diff --git a/core/auth/issuerBinding.ts b/core/auth/issuerBinding.ts new file mode 100644 index 000000000..535d94ce3 --- /dev/null +++ b/core/auth/issuerBinding.ts @@ -0,0 +1,161 @@ +/** + * SEP-2352 issuer-binding failure classification. + * + * On the authorization-code callback leg the SDK compares the authorization + * server resolved by discovery against the one recorded in `discoveryState()` + * at redirect time, and throws `AuthorizationServerMismatchError` when the two + * cannot be reconciled. That single error class covers two very different + * situations: + * + * 1. **Lost authorization state** — no discovery state was recorded at all + * (`recordedIssuer === undefined` inside the SDK). This is a *recoverable* + * bookkeeping failure: the stored authorization state for the server was + * dropped between the redirect and the callback (a new browser session, a + * partially cleared store, a callback resumed in another tab). Nothing + * suspicious happened — the user just has to authorize again. + * 2. **Genuine issuer mismatch** — a real issuer *was* recorded and the callback + * resolved a different one. This is a security signal (RFC 7636: the + * `authorization_code` and `code_verifier` are bound to the AS that minted + * them; replaying them at another AS's token endpoint is a credential + * exfiltration vector). It must never be papered over with a friendly + * "just re-authorize" affordance. + * + * The SDK does not expose a discriminator, so we key off the shape of + * `recordedIssuer`: in the "lost state" case the SDK passes a human-readable + * sentence in the `recordedIssuer` slot ("discoveryState was not available on + * the callback leg; …"); a genuine mismatch always carries an absolute + * `http(s)` issuer URL there. We accept either signal — the sentinel phrase or + * "not a URL" — so a reworded SDK sentence still classifies correctly. + */ + +/** Brand the SDK stamps on `AuthorizationServerMismatchError` instances. */ +const AS_MISMATCH_BRAND = "mcp.AuthorizationServerMismatchError"; + +/** Phrase the SDK puts in the `recordedIssuer` slot when nothing was recorded. */ +const MISSING_DISCOVERY_STATE_PHRASE = "discoveryState was not available"; + +/** Outcome of classifying a callback-leg issuer-binding failure. */ +export type IssuerBindingFailure = + | { + /** No discovery state was recorded — recoverable by authorizing again. */ + kind: "lost_authorization_state"; + /** Issuer resolved by discovery on the callback leg. */ + currentIssuer: string; + } + | { + /** A different authorization server answered the callback — security signal. */ + kind: "issuer_mismatch"; + /** Issuer recorded at redirect time. */ + recordedIssuer: string; + /** Issuer resolved by discovery on the callback leg. */ + currentIssuer: string; + }; + +interface AuthorizationServerMismatchShape { + recordedIssuer: string; + currentIssuer: string; +} + +/** + * Structural check for the SDK's `AuthorizationServerMismatchError`. + * + * Deliberately brand/shape based rather than `instanceof`: the error can be + * constructed by a different copy of the SDK than the one this module imports + * (bundled client vs. the backend's), which makes `instanceof` unreliable. + */ +function isAuthorizationServerMismatchShape( + err: unknown, +): err is AuthorizationServerMismatchShape { + if (err === null || typeof err !== "object") { + return false; + } + const candidate = err as { + mcpBrand?: unknown; + recordedIssuer?: unknown; + currentIssuer?: unknown; + }; + return ( + candidate.mcpBrand === AS_MISMATCH_BRAND && + typeof candidate.recordedIssuer === "string" && + typeof candidate.currentIssuer === "string" + ); +} + +/** True when `recordedIssuer` is the SDK's "nothing was recorded" placeholder. */ +function isMissingDiscoveryStatePlaceholder(recordedIssuer: string): boolean { + if (recordedIssuer.includes(MISSING_DISCOVERY_STATE_PHRASE)) { + return true; + } + // A genuine recorded issuer is always an absolute http(s) URL, so anything + // else in that slot is prose the SDK put there in place of an issuer. + try { + const url = new URL(recordedIssuer); + return url.protocol !== "http:" && url.protocol !== "https:"; + } catch { + return true; + } +} + +/** + * Find and classify a SEP-2352 issuer-binding failure anywhere in an error's + * `cause` / `data.cause` chain (era-negotiation and transport wrappers bury the + * original rejection, see `findNestedAuthError`). + */ +export function findIssuerBindingFailure( + err: unknown, +): IssuerBindingFailure | undefined { + return findIssuerBindingFailureDeep(err, new Set()); +} + +function findIssuerBindingFailureDeep( + err: unknown, + seen: Set, +): IssuerBindingFailure | undefined { + if (err === null || typeof err !== "object" || seen.has(err)) { + return undefined; + } + seen.add(err); + + if (isAuthorizationServerMismatchShape(err)) { + return classifyAuthorizationServerMismatch(err); + } + + const nested = findIssuerBindingFailureDeep( + (err as { cause?: unknown }).cause, + seen, + ); + if (nested) { + return nested; + } + + const data = (err as { data?: unknown }).data; + if (data !== null && typeof data === "object") { + return findIssuerBindingFailureDeep( + (data as { cause?: unknown }).cause, + seen, + ); + } + + return undefined; +} + +function classifyAuthorizationServerMismatch( + err: AuthorizationServerMismatchShape, +): IssuerBindingFailure { + if (isMissingDiscoveryStatePlaceholder(err.recordedIssuer)) { + return { + kind: "lost_authorization_state", + currentIssuer: err.currentIssuer, + }; + } + return { + kind: "issuer_mismatch", + recordedIssuer: err.recordedIssuer, + currentIssuer: err.currentIssuer, + }; +} + +/** True when the callback failed only because the recorded state was lost. */ +export function isLostAuthorizationStateError(err: unknown): boolean { + return findIssuerBindingFailure(err)?.kind === "lost_authorization_state"; +} diff --git a/core/auth/node/runner-interactive-oauth.ts b/core/auth/node/runner-interactive-oauth.ts index edbb394d0..ce0dd199e 100644 --- a/core/auth/node/runner-interactive-oauth.ts +++ b/core/auth/node/runner-interactive-oauth.ts @@ -1,4 +1,6 @@ import type { AuthChallenge } from "../challenge.js"; +import { findIssuerBindingFailure } from "../issuerBinding.js"; +import { issuerBindingFailureCopy } from "../oauthUx.js"; import { createOAuthCallbackServer, type OAuthCallbackServer, @@ -40,6 +42,24 @@ export interface RunRunnerInteractiveOAuthOptions { callbackTimeoutMs?: number; } +/** + * Replace an opaque SEP-2352 callback-leg failure with actionable copy (#1808). + * + * The runners have no banner to render, so their affordance is the message + * itself: it explains that the recorded authorization state was lost and that + * re-running authorization starts a fresh flow (the next `authenticate()` + * re-runs discovery and records new state). A *genuine* issuer mismatch gets + * the security-flavoured copy instead — never a "just try again" nudge. + */ +function toRunnerOAuthError(err: unknown): Error { + const failure = findIssuerBindingFailure(err); + if (failure) { + const copy = issuerBindingFailureCopy(failure); + return new Error(`${copy.title}. ${copy.message}`, { cause: err }); + } + return err instanceof Error ? err : new Error(String(err)); +} + /** * Run interactive OAuth for Node runners (TUI / CLI): loopback callback server, * browser redirect, authorization-code exchange via {@link completeOAuthFlow}. @@ -70,7 +90,7 @@ export async function runRunnerInteractiveOAuth( await options.client.completeOAuthFlow(params.code, params.iss); flowResolve(); } catch (err) { - flowReject(err instanceof Error ? err : new Error(String(err))); + flowReject(toRunnerOAuthError(err)); } }, onError: (params) => { diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index da2737223..5a9dd0856 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -1,4 +1,5 @@ import type { AuthChallenge } from "./challenge.js"; +import type { IssuerBindingFailure } from "./issuerBinding.js"; export type OAuthInteractiveAuthKind = "step_up" | "reauth"; @@ -224,6 +225,83 @@ export function isReAuthBannerReason( ); } +/** Banner/alert heading when the recorded authorization state was lost. */ +export function lostAuthorizationStateTitle(): string { + return "Authorization state was lost"; +} + +/** + * Plain-language explanation for a callback that arrived with no recorded + * discovery state (SEP-2352). Deliberately does not surface the SDK's + * `AuthorizationServerMismatchError` text — that wording reads like a security + * failure, and this case is ordinary bookkeeping loss. + */ +export function lostAuthorizationStateMessage(options?: { + serverName?: string; +}): string { + const target = options?.serverName + ? `"${options.serverName}"` + : "this server"; + return ( + `The stored authorization state for ${target} was lost before the ` + + "authorization server sent you back, so the sign-in could not be " + + "completed. This usually means a new browser session, a different tab, or " + + "authorization state that was cleared while the flow was in progress. " + + "Authorize again to reconnect." + ); +} + +/** Action label for the lost-authorization-state recovery affordance. */ +export function lostAuthorizationStateActionLabel(): string { + return "Authorize again"; +} + +/** Heading for a genuine cross-authorization-server mismatch (a security signal). */ +export function issuerMismatchTitle(): string { + return "Authorization server mismatch"; +} + +/** + * Copy for a genuine SEP-2352 issuer mismatch. This is *not* offered a + * one-click recovery: the authorization code and PKCE verifier are bound to the + * server that minted them, and a different server answering the callback is a + * credential-exfiltration signal the user must investigate. + */ +export function issuerMismatchMessage(options: { + recordedIssuer: string; + currentIssuer: string; + serverName?: string; +}): string { + const target = options.serverName ? `"${options.serverName}"` : "this server"; + return ( + `Authorization for ${target} was stopped: the flow started at ` + + `${options.recordedIssuer} but the callback resolved ` + + `${options.currentIssuer}. The authorization code was not exchanged. ` + + "Verify the server's authorization configuration before trying again." + ); +} + +/** Title + message for a callback-leg issuer-binding failure. */ +export function issuerBindingFailureCopy( + failure: IssuerBindingFailure, + options?: { serverName?: string }, +): { title: string; message: string } { + if (failure.kind === "lost_authorization_state") { + return { + title: lostAuthorizationStateTitle(), + message: lostAuthorizationStateMessage(options), + }; + } + return { + title: issuerMismatchTitle(), + message: issuerMismatchMessage({ + recordedIssuer: failure.recordedIssuer, + currentIssuer: failure.currentIssuer, + serverName: options?.serverName, + }), + }; +} + export function reAuthBannerMessage(options: { serverName?: string; detail?: string; From edddcdad4edb1209e0153b724602efc4dcd071cc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 16:28:48 -0400 Subject: [PATCH 2/2] fix(auth): detect the SDK mismatch error via isInstance (#1808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --- clients/web/src/App.test.tsx | 22 ++-- clients/web/src/App.tsx | 25 +++-- .../groups/ReAuthBanner/ReAuthBanner.tsx | 3 +- .../src/test/core/auth/issuerBinding.test.ts | 103 ++++++++++++------ .../web/src/test/core/auth/oauthUx.test.ts | 12 ++ .../auth/runner-interactive-oauth.test.ts | 25 ++--- core/auth/index.ts | 1 - core/auth/issuerBinding.ts | 36 +++--- core/auth/oauthUx.ts | 22 +++- 9 files changed, 158 insertions(+), 91 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 5d89736f6..0869d0d5d 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1,5 +1,9 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { ProtocolErrorCode, ProtocolError } from "@modelcontextprotocol/client"; +import { + ProtocolErrorCode, + ProtocolError, + AuthorizationServerMismatchError, +} from "@modelcontextprotocol/client"; import { UrlElicitationLoopError } from "@inspector/core/mcp/urlElicitation.js"; import { ToolCallCancelledError } from "@inspector/core/mcp/toolCallCancelledError.js"; import { @@ -2212,17 +2216,13 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => { importSource: vi.fn().mockResolvedValue({ servers: {} }), }); - const mismatchError = (recordedIssuer: string): Error => { - const err = new Error("Authorization server changed"); - Object.defineProperty(err, "mcpBrand", { - value: "mcp.AuthorizationServerMismatchError", - }); - Object.assign(err, { + // A real SDK error, not a look-alike: the SDK's `mcpBrand` is static, so a + // fabricated instance-branded object would exercise a shape that cannot occur. + const mismatchError = (recordedIssuer: string): Error => + new AuthorizationServerMismatchError( recordedIssuer, - currentIssuer: "https://as.example.com", - }); - return err; - }; + "https://as.example.com", + ); const renderCallbackWithFailure = (err: Error) => { writeOAuthResumeSnapshot({ diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index ee099028f..6d162c7bd 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -199,7 +199,6 @@ import { isReAuthBannerReason, issuerBindingFailureCopy, lostAuthorizationStateActionLabel, - lostAuthorizationStateTitle, oauthPreRedirectToastCopy, oauthResumeAbandonedMessage, reAuthBannerMessage, @@ -890,6 +889,13 @@ function App() { * starting a fresh authorization. */ kind?: "lost_authorization_state"; + /** + * Resolved at the point the banner is raised so `issuerBindingFailureCopy` + * stays the single source of the `kind → copy` mapping; both fall back to + * `ReAuthBanner`'s own defaults when absent. + */ + title?: string; + actionLabel?: string; } | null>(null); const [pendingReauth, setPendingReauth] = useState( null, @@ -2588,6 +2594,8 @@ function App() { serverId: server.id, message: copy.message, kind: "lost_authorization_state", + title: copy.title, + actionLabel: lostAuthorizationStateActionLabel(), }); } else { notifications.show({ @@ -2906,6 +2914,9 @@ function App() { title: "Could not clear the stored authorization state", message: err instanceof Error ? err.message : String(err), color: "red", + // The banner is already dismissed and the flow is dead, so this + // is the only remaining explanation — don't time it out. + autoClose: false, }); return; } @@ -4334,16 +4345,8 @@ function App() { setReAuthBanner(null)} /> diff --git a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx index 7bf095907..817e19c30 100644 --- a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx @@ -13,10 +13,11 @@ export interface ReAuthBannerProps { onDismiss: () => void; } +// `title` is intentionally not baked in here: the component always passes a +// resolved `title`, whose parameter default is the single source of truth. const ReAuthAlert = Alert.withProps({ color: "red", variant: "reauth", - title: DEFAULT_REAUTH_BANNER_TITLE, withCloseButton: true, // Mantine's close button renders icon-only; without a label it has no // accessible name (axe `button-name`). diff --git a/clients/web/src/test/core/auth/issuerBinding.test.ts b/clients/web/src/test/core/auth/issuerBinding.test.ts index 34a3e94a6..5ebbe8552 100644 --- a/clients/web/src/test/core/auth/issuerBinding.test.ts +++ b/clients/web/src/test/core/auth/issuerBinding.test.ts @@ -1,21 +1,61 @@ import { describe, it, expect } from "vitest"; -import { - findIssuerBindingFailure, - isLostAuthorizationStateError, -} from "@inspector/core/auth/issuerBinding.js"; +import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client"; +import { findIssuerBindingFailure } from "@inspector/core/auth/issuerBinding.js"; /** - * Builds an object shaped like the SDK's `AuthorizationServerMismatchError` - * (brand + the two issuer fields). The classifier is intentionally structural, - * not `instanceof`, so a plain object is the honest fixture here. + * Builds a **real** SDK `AuthorizationServerMismatchError`. + * + * This must stay the real class rather than a look-alike plain object. The SDK + * declares `mcpBrand` in a `static {}` block, so it sits on the constructor and + * is invisible from the instance; a fabricated fixture carrying an own + * `mcpBrand` property would pass against a classifier that reads + * `err.mcpBrand` — which no real thrown error would ever satisfy. */ -function mismatchError(recordedIssuer: string, currentIssuer: string): Error { +function mismatchError( + recordedIssuer: string, + currentIssuer: string, +): AuthorizationServerMismatchError { + return new AuthorizationServerMismatchError(recordedIssuer, currentIssuer); +} + +/** + * Documents why the classifier must not test `err.mcpBrand`: the SDK declares + * that brand `static`, so it lives on the class and instances never carry it. + * If a future SDK moves it onto the instance, this fails and the note in + * `issuerBinding.ts` can be revisited. + */ +describe("SDK brand placement", () => { + it("keeps `mcpBrand` on the class, not the instance", () => { + const err = mismatchError("https://a.example.com", "https://b.example.com"); + expect("mcpBrand" in err).toBe(false); + // The brand is defined at runtime via `Object.defineProperty` in a static + // block, so it is absent from the SDK's published type declarations — hence + // the cast to read it. + const brandOnClass = ( + AuthorizationServerMismatchError as { mcpBrand?: unknown } + ).mcpBrand; + expect(brandOnClass).toBe("mcp.AuthorizationServerMismatchError"); + }); + + it("matches via `isInstance`, which survives a foreign SDK copy", () => { + const err = mismatchError("https://a.example.com", "https://b.example.com"); + expect(AuthorizationServerMismatchError.isInstance(err)).toBe(true); + }); +}); + +/** + * An error that crossed a serialization boundary: neither the SDK's + * `Symbol.for()`-keyed brand set nor the prototype survives, but `name` does. + * This is the fallback arm of the classifier's predicate. + */ +function serializedMismatchError( + recordedIssuer: string, + currentIssuer: string, +): Error { const err = new Error( "Authorization server changed between redirect and callback", ); - Object.defineProperty(err, "mcpBrand", { - value: "mcp.AuthorizationServerMismatchError", - }); + err.name = "AuthorizationServerMismatchError"; Object.assign(err, { recordedIssuer, currentIssuer }); return err; } @@ -118,43 +158,34 @@ describe("findIssuerBindingFailure", () => { expect(findIssuerBindingFailure("string")).toBeUndefined(); }); - it("ignores a branded error missing the issuer fields", () => { + it("ignores a matching error missing the issuer fields", () => { const err = new Error("half-shaped"); - Object.defineProperty(err, "mcpBrand", { - value: "mcp.AuthorizationServerMismatchError", - }); + err.name = "AuthorizationServerMismatchError"; expect(findIssuerBindingFailure(err)).toBeUndefined(); const halfShaped = new Error("only recorded"); - Object.defineProperty(halfShaped, "mcpBrand", { - value: "mcp.AuthorizationServerMismatchError", - }); + halfShaped.name = "AuthorizationServerMismatchError"; Object.assign(halfShaped, { recordedIssuer: "https://as.example.com" }); expect(findIssuerBindingFailure(halfShaped)).toBeUndefined(); }); - it("ignores a different SDK brand", () => { + it("classifies a serialized error via the `name` fallback", () => { + expect( + findIssuerBindingFailure( + serializedMismatchError( + MISSING_STATE_SENTINEL, + "https://as.example.com", + ), + )?.kind, + ).toBe("lost_authorization_state"); + }); + + it("ignores a different SDK error carrying the same fields", () => { const err = Object.assign(new Error("other"), { - mcpBrand: "mcp.IssuerMismatchError", recordedIssuer: "https://a.example.com", currentIssuer: "https://b.example.com", }); + err.name = "IssuerMismatchError"; expect(findIssuerBindingFailure(err)).toBeUndefined(); }); }); - -describe("isLostAuthorizationStateError", () => { - it("is true only for the missing-discovery-state case", () => { - expect( - isLostAuthorizationStateError( - mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"), - ), - ).toBe(true); - expect( - isLostAuthorizationStateError( - mismatchError("https://a.example.com", "https://b.example.com"), - ), - ).toBe(false); - expect(isLostAuthorizationStateError(new Error("boom"))).toBe(false); - }); -}); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index bc899717c..e11bb990f 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -375,6 +375,18 @@ describe("oauthUx issuer-binding copy", () => { expect(issuerMismatchTitle()).toBe("Authorization server mismatch"); }); + it("bounds an overlong remote-supplied issuer in the mismatch copy", () => { + const overlong = `https://evil.example.com/${"a".repeat(400)}`; + const message = issuerMismatchMessage({ + recordedIssuer: "https://old.example.com", + currentIssuer: overlong, + }); + expect(message).not.toContain(overlong); + expect(message).toContain("…"); + // The short issuer on the same call is passed through untouched. + expect(message).toContain("https://old.example.com"); + }); + it("issuerBindingFailureCopy dispatches on the failure kind", () => { expect( issuerBindingFailureCopy( diff --git a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts index 033487c2a..b141c1cba 100644 --- a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts +++ b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, afterEach } from "vitest"; +import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client"; import { runRunnerInteractiveOAuth, type RunnerInteractiveOAuthClient, @@ -251,15 +252,12 @@ describe("runRunnerInteractiveOAuth", () => { it("rewrites a lost-authorization-state callback failure into actionable copy (#1808)", async () => { const redirectUrlProvider = { redirectUrl: "" }; - const sdkError = new Error("Authorization server changed"); - Object.defineProperty(sdkError, "mcpBrand", { - value: "mcp.AuthorizationServerMismatchError", - }); - Object.assign(sdkError, { - recordedIssuer: - "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier", - currentIssuer: "https://as.example", - }); + // Real SDK error — its `mcpBrand` is static, so a fabricated + // instance-branded look-alike would not exercise the real shape. + const sdkError = new AuthorizationServerMismatchError( + "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier", + "https://as.example", + ); const client = mockClient({ authenticate: vi.fn(async () => { await simulateCallback(handlers.current); @@ -291,11 +289,10 @@ describe("runRunnerInteractiveOAuth", () => { it("keeps the security wording for a genuine issuer mismatch (#1808)", async () => { const redirectUrlProvider = { redirectUrl: "" }; - const sdkError = Object.assign(new Error("Authorization server changed"), { - mcpBrand: "mcp.AuthorizationServerMismatchError", - recordedIssuer: "https://old.example", - currentIssuer: "https://evil.example", - }); + const sdkError = new AuthorizationServerMismatchError( + "https://old.example", + "https://evil.example", + ); const client = mockClient({ authenticate: vi.fn(async () => { await simulateCallback(handlers.current); diff --git a/core/auth/index.ts b/core/auth/index.ts index 66a597d09..6086d7ac9 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -109,7 +109,6 @@ export { export { findIssuerBindingFailure, - isLostAuthorizationStateError, type IssuerBindingFailure, } from "./issuerBinding.js"; diff --git a/core/auth/issuerBinding.ts b/core/auth/issuerBinding.ts index 535d94ce3..9314f17fa 100644 --- a/core/auth/issuerBinding.ts +++ b/core/auth/issuerBinding.ts @@ -28,8 +28,7 @@ * "not a URL" — so a reworded SDK sentence still classifies correctly. */ -/** Brand the SDK stamps on `AuthorizationServerMismatchError` instances. */ -const AS_MISMATCH_BRAND = "mcp.AuthorizationServerMismatchError"; +import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client"; /** Phrase the SDK puts in the `recordedIssuer` slot when nothing was recorded. */ const MISSING_DISCOVERY_STATE_PHRASE = "discoveryState was not available"; @@ -57,11 +56,18 @@ interface AuthorizationServerMismatchShape { } /** - * Structural check for the SDK's `AuthorizationServerMismatchError`. + * Recognize the SDK's `AuthorizationServerMismatchError`. * - * Deliberately brand/shape based rather than `instanceof`: the error can be - * constructed by a different copy of the SDK than the one this module imports - * (bundled client vs. the backend's), which makes `instanceof` unreliable. + * 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 of the SDK still + * matches. (Note the brand constant itself is declared `static`, so it lives on + * the class and is never reachable as `err.mcpBrand` — checking that property on + * an instance always fails.) + * + * The `name` comparison is a deliberate fallback for a serialization boundary, + * where neither the brand set nor the prototype survives but `name` does. */ function isAuthorizationServerMismatchShape( err: unknown, @@ -70,14 +76,19 @@ function isAuthorizationServerMismatchShape( return false; } const candidate = err as { - mcpBrand?: unknown; recordedIssuer?: unknown; currentIssuer?: unknown; + name?: unknown; }; + if ( + typeof candidate.recordedIssuer !== "string" || + typeof candidate.currentIssuer !== "string" + ) { + return false; + } return ( - candidate.mcpBrand === AS_MISMATCH_BRAND && - typeof candidate.recordedIssuer === "string" && - typeof candidate.currentIssuer === "string" + AuthorizationServerMismatchError.isInstance(err) || + candidate.name === "AuthorizationServerMismatchError" ); } @@ -154,8 +165,3 @@ function classifyAuthorizationServerMismatch( currentIssuer: err.currentIssuer, }; } - -/** True when the callback failed only because the recorded state was lost. */ -export function isLostAuthorizationStateError(err: unknown): boolean { - return findIssuerBindingFailure(err)?.kind === "lost_authorization_state"; -} diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 5a9dd0856..184e5c5a6 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -261,6 +261,23 @@ export function issuerMismatchTitle(): string { return "Authorization server mismatch"; } +/** + * Longest issuer we will echo back into user-facing copy. + * + * `currentIssuer` is remote-supplied (it comes from the configured server's AS + * metadata), so an overlong value could be used to abuse the notification + * layout. Rendering is escaped, so this is a presentation bound, not an + * injection defence. + */ +const MAX_DISPLAYED_ISSUER_LENGTH = 120; + +/** 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; +} + /** * Copy for a genuine SEP-2352 issuer mismatch. This is *not* offered a * one-click recovery: the authorization code and PKCE verifier are bound to the @@ -275,8 +292,9 @@ export function issuerMismatchMessage(options: { const target = options.serverName ? `"${options.serverName}"` : "this server"; return ( `Authorization for ${target} was stopped: the flow started at ` + - `${options.recordedIssuer} but the callback resolved ` + - `${options.currentIssuer}. The authorization code was not exchanged. ` + + `${truncateIssuerForDisplay(options.recordedIssuer)} but the callback ` + + `resolved ${truncateIssuerForDisplay(options.currentIssuer)}. ` + + "The authorization code was not exchanged. " + "Verify the server's authorization configuration before trying again." ); }