fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808)

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)

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<string, string>();
+  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<typeof useServers> => ({
+    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(<App />);
+  };
+
+  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<typeof vi.fn>;
+    };
+
+    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;

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<PendingReauth | null>(
     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() {
           <ReAuthBannerBar>
             <ReAuthBanner
               message={reAuthBanner.message}
+              title={reAuthBanner.title}
+              actionLabel={reAuthBanner.actionLabel}
               onReauthenticate={onReauthenticateFromBanner}
               onDismiss={() => setReAuthBanner(null)}
             />