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..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 { @@ -47,6 +51,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 +100,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 +125,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 +582,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 +611,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 +2181,159 @@ 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: {} }), + }); + + // 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, + "https://as.example.com", + ); + + 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..6d162c7bd 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -197,12 +197,15 @@ import { clearServerOAuthState } from "./lib/clearServerOAuthState"; import { authRecoveryRestoredMessage, isReAuthBannerReason, + issuerBindingFailureCopy, + lostAuthorizationStateActionLabel, oauthPreRedirectToastCopy, oauthResumeAbandonedMessage, reAuthBannerMessage, type OAuthPreRedirectContext, type OAuthRecoverySource, } from "./utils/oauthUx"; +import { findIssuerBindingFailure } from "@inspector/core/auth/issuerBinding.js"; import { isBrowserTabVisible, onBrowserTabVisible, @@ -879,6 +882,20 @@ 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"; + /** + * 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, @@ -2559,6 +2576,37 @@ 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", + title: copy.title, + actionLabel: lostAuthorizationStateActionLabel(), + }); + } 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 +2887,45 @@ 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", + // 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; + } + } + await onToggleConnection(serverId); + })(); + return; + } + if ( serverId === activeServerId && connectionStatus === "connected" && @@ -2888,6 +2973,7 @@ function App() { servers, prepareOAuthRedirect, onToggleConnection, + webOAuthStorage, ]); // --- Action handlers that route directly to the InspectorClient. --- @@ -4259,6 +4345,8 @@ 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..817e19c30 100644 --- a/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx @@ -1,16 +1,27 @@ 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; } +// `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: "Re-authentication required", 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 +43,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..5ebbe8552 --- /dev/null +++ b/clients/web/src/test/core/auth/issuerBinding.test.ts @@ -0,0 +1,191 @@ +import { describe, it, expect } from "vitest"; +import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client"; +import { findIssuerBindingFailure } from "@inspector/core/auth/issuerBinding.js"; + +/** + * 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, +): 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", + ); + err.name = "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 matching error missing the issuer fields", () => { + const err = new Error("half-shaped"); + err.name = "AuthorizationServerMismatchError"; + expect(findIssuerBindingFailure(err)).toBeUndefined(); + + const halfShaped = new Error("only recorded"); + halfShaped.name = "AuthorizationServerMismatchError"; + Object.assign(halfShaped, { recordedIssuer: "https://as.example.com" }); + expect(findIssuerBindingFailure(halfShaped)).toBeUndefined(); + }); + + 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"), { + recordedIssuer: "https://a.example.com", + currentIssuer: "https://b.example.com", + }); + err.name = "IssuerMismatchError"; + expect(findIssuerBindingFailure(err)).toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index f209c9ab2..e11bb990f 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,79 @@ 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("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( + { + 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..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, @@ -249,6 +250,76 @@ describe("runRunnerInteractiveOAuth", () => { ).rejects.toThrow("token exchange failed"); }); + it("rewrites a lost-authorization-state callback failure into actionable copy (#1808)", async () => { + const redirectUrlProvider = { redirectUrl: "" }; + // 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); + 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 = new AuthorizationServerMismatchError( + "https://old.example", + "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..6086d7ac9 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -98,9 +98,20 @@ export { oauthPreRedirectToastCopy, isReAuthBannerReason, reAuthBannerMessage, + lostAuthorizationStateTitle, + lostAuthorizationStateMessage, + lostAuthorizationStateActionLabel, + issuerMismatchTitle, + issuerMismatchMessage, + issuerBindingFailureCopy, type OAuthInteractiveAuthKind, } from "./oauthUx.js"; +export { + findIssuerBindingFailure, + 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..9314f17fa --- /dev/null +++ b/core/auth/issuerBinding.ts @@ -0,0 +1,167 @@ +/** + * 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. + */ + +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"; + +/** 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; +} + +/** + * Recognize the SDK's `AuthorizationServerMismatchError`. + * + * 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, +): err is AuthorizationServerMismatchShape { + if (err === null || typeof err !== "object") { + return false; + } + const candidate = err as { + recordedIssuer?: unknown; + currentIssuer?: unknown; + name?: unknown; + }; + if ( + typeof candidate.recordedIssuer !== "string" || + typeof candidate.currentIssuer !== "string" + ) { + return false; + } + return ( + AuthorizationServerMismatchError.isInstance(err) || + candidate.name === "AuthorizationServerMismatchError" + ); +} + +/** 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, + }; +} 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..184e5c5a6 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,101 @@ 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"; +} + +/** + * 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 + * 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 ` + + `${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." + ); +} + +/** 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;