fix: key the OAuth clear's active-session check on the storage key

clients/web/src/App.test.tsx

@@ -151,12 +151,18 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
     clearOAuthTokens = vi
       .fn()
       .mockResolvedValue({ status: "skipped", reason: "no_endpoint" });
+    // #2217: the clear path resolves the session's OAuth key from the config
+    // the client was BUILT with, not the (mutable) catalog entry — so the fake
+    // has to carry the constructor's config the way the real client does.
+    transportConfig: unknown = undefined;
+    getTransportConfig = vi.fn(() => this.transportConfig);
   }
   const instances: FakeInspectorClient[] = [];
   return {
     ...actual,
-    InspectorClient: vi.fn(function () {
+    InspectorClient: vi.fn(function (transportConfig: unknown) {
       const client = new FakeInspectorClient();
+      client.transportConfig = transportConfig;
       instances.push(client);
       return client;
     }),
@@ -244,7 +250,13 @@ vi.mock("@inspector/core/mcp/state/stderrLogState.js", () => ({
   }),
 }));
 
-vi.mock("@inspector/core/mcp/remote/index.js", () => ({
+vi.mock("@inspector/core/mcp/remote/index.js", async (importOriginal) => ({
+  // Partial: `getWebProxiedFetch` (the OAuth clear's revocation fetch, #2144)
+  // reaches for `createRemoteFetch` from this module, and a bare stub would
+  // throw a missing-export error before the clear under test ever ran.
+  ...(await importOriginal<
+    typeof import("@inspector/core/mcp/remote/index.js")
+  >()),
   RemoteInspectorClientStorage: vi.fn(function () {
     return { saveSession: vi.fn() };
   }),
@@ -601,6 +613,11 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
       <button onClick={() => props.servers.onServerSettings("A")}>
         open-settings
       </button>
+      {/* A second settings target, so a test can drive a clear against an
+          entry other than the active one (#2217). */}
+      <button onClick={() => props.servers.onServerSettings("B")}>
+        open-settings-b
+      </button>
       {/* The real server grid (and its Add / Edit controls) lives inside this
           mocked view, so the config modal is only reachable through these
           callbacks — and the highlight batch only observable through this prop. */}
@@ -4214,3 +4231,123 @@ describe("App MCP App listed-resource metadata wiring (#2055)", () => {
     }
   });
 });
+
+// #2217 — persisted OAuth state is keyed by server URL and `serverList`
+// enforces no URL uniqueness, so two catalog entries can share one credential
+// blob, one grant and one revocation. `runClear`'s in-flight guard used to
+// dedupe by catalog id, which cannot see that: clearing both entries while the
+// first revocation was still out ran concurrent store writes, concurrent RFC
+// 7009 requests and two contradictory teardown toasts — the exact race the
+// guard exists to prevent (Copilot, on this PR).
+describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", () => {
+  const SHARED_URL = "https://shared.example/mcp";
+  const sharedEntry = (id: string, name: string): ServerEntry => ({
+    id,
+    name,
+    config: { type: "streamable-http", url: SHARED_URL },
+    connection: { status: "disconnected" },
+  });
+
+  let previousUseServers: typeof useServers | undefined;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    clientInstances.length = 0;
+    previousUseServers = vi.mocked(useServers).getMockImplementation();
+    vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT);
+    vi.mocked(useServers).mockReturnValue({
+      servers: [sharedEntry("A", "PlotRocket"), sharedEntry("B", "Same URL")],
+      loading: false,
+      error: undefined,
+      refresh: vi.fn().mockResolvedValue(undefined),
+      addServer: addServerSpy,
+      updateServer: updateServerSpy,
+      updateServerSettings: updateServerSettingsSpy,
+      removeServer: vi.fn(),
+      reorderServers: vi.fn(),
+      importSource: vi.fn().mockResolvedValue({ servers: {} }),
+    });
+  });
+
+  afterEach(() => {
+    if (previousUseServers) {
+      vi.mocked(useServers).mockImplementation(previousUseServers);
+    }
+  });
+
+  /** Open the settings modal for `which` and press its OAuth-section clear. */
+  async function clearFromSettings(
+    user: ReturnType<typeof userEvent.setup>,
+    which: "open-settings" | "open-settings-b",
+  ): Promise<void> {
+    await user.click(screen.getByText(which));
+    // The control lives in an accordion section. The modal stays mounted
+    // across a target switch, so the section may already be open from a
+    // previous call — toggling it again would close it.
+    const section = await screen.findByRole("button", {
+      name: "OAuth Settings",
+    });
+    if (section.getAttribute("aria-expanded") !== "true") {
+      await user.click(section);
+    }
+    await user.click(
+      await screen.findByRole("button", { name: "Clear stored OAuth state" }),
+    );
+  }
+
+  // Three full modal interaction sequences against the whole App tree, so this
+  // one runs long enough to trip the 5s default when the suite is under load.
+  it(
+    "suppresses a second clear for another entry with the same URL, and allows one after it settles",
+    { timeout: 20000 },
+    async () => {
+      // `delay: null` drops userEvent's inter-event waits, which dominate here.
+      const user = userEvent.setup({ delay: null });
+      renderWithMantine(<App />);
+
+      await user.click(screen.getByText("connect"));
+      await waitFor(() => expect(clientInstances).toHaveLength(1));
+      // The instances are typed `EventTarget`; an intersection names the
+      // test-only spy without erasing that (Copilot).
+      const client = clientInstances[0] as EventTarget & {
+        clearOAuthTokens: ReturnType<typeof vi.fn>;
+      };
+
+      // Hold the first clear open, as a pending RFC 7009 request would.
+      let settle: (v: { status: string; reason: string }) => void = () => {};
+      client.clearOAuthTokens.mockImplementation(
+        () =>
+          new Promise((resolve) => {
+            settle = resolve as typeof settle;
+          }),
+      );
+
+      await clearFromSettings(user, "open-settings");
+      await waitFor(() =>
+        expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1),
+      );
+
+      // Entry B: a different catalog id, the same OAuth storage key. Both clears
+      // route through the live client precisely because they share that key, so
+      // an id-keyed guard would let this second one straight through.
+      await clearFromSettings(user, "open-settings-b");
+      expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1);
+
+      // Suppression is for the duration of the in-flight clear only — the key
+      // must be released when it settles, or the control is dead for the rest of
+      // the session.
+      await act(async () => {
+        settle({ status: "skipped", reason: "no_endpoint" });
+        await Promise.resolve();
+      });
+      client.clearOAuthTokens.mockResolvedValue({
+        status: "skipped",
+        reason: "no_endpoint",
+      });
+      await clearFromSettings(user, "open-settings-b");
+      await waitFor(() =>
+        expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2),
+      );
+    },
+  );
+});

clients/web/src/App.tsx

@@ -115,6 +115,7 @@ import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
 import { getAuthToken } from "./lib/authToken";
 import { messagesToLogEntries } from "./lib/protocolReplay";
 import { EMPTY_SETTINGS } from "./utils/serverSettingsDefaults";
+import { resolveOAuthClearIdentity } from "./utils/oauthClearKey";
 import {
   bodyDroppedToastId,
   CLIENT_CONFIG_LOAD_ERROR_NOTIFICATION_ID,
@@ -1350,10 +1351,10 @@ function App() {
   const settingsModalIsStdio = settingsModalServerType === "stdio";
 
   /**
-   * Servers whose clear is in flight (#2144). Keyed by id, not a single flag:
-   * the callback explicitly supports clearing a server other than the active
-   * one, so a global lock would silently drop B's click while A's revocation
-   * was still out. See `runClear`.
+   * Servers whose clear is in flight (#2144). Keyed per server, not a single
+   * flag: the callback explicitly supports clearing a server other than the
+   * active one, so a global lock would silently drop B's click while A's
+   * revocation was still out. See `runClear`.
    */
   const clearOAuthInFlightRef = useRef<Set<string>>(new Set());
 
@@ -1375,11 +1376,27 @@ function App() {
       // — and with revocation taking up to five seconds, that means concurrent
       // RFC 7009 requests, concurrent store writes, and two contradictory
       // toasts. Keyed by server so a *different* server's clear is unaffected.
-      if (clearOAuthInFlightRef.current.has(server.id)) return;
-      clearOAuthInFlightRef.current.add(server.id);
+      //
+      // "Different server" is the OAuth storage key, not the catalog id
+      // (#2217, Copilot): two entries against one URL share one blob, one
+      // grant and one revocation, so an id-keyed guard lets exactly the race
+      // above through between them. And for anything touching the live
+      // session the key is the *client's*, not the entry's — an entry edited
+      // while connected reads a URL the session never authorized against, so
+      // an entry-keyed lock would name an operation nobody is performing.
+      // `resolveOAuthClearIdentity` is the same call the clear itself makes,
+      // so the two cannot disagree.
+      const { inFlightKey } = resolveOAuthClearIdentity({
+        server,
+        activeServerId,
+        activeClientConfig: inspectorClient?.getTransportConfig(),
+        activeEntryConfig: activeServer?.config,
+      });
+      if (clearOAuthInFlightRef.current.has(inFlightKey)) return;
+      clearOAuthInFlightRef.current.add(inFlightKey);
       clearServerOAuthAndDisconnect(server)
         .finally(() => {
-          clearOAuthInFlightRef.current.delete(server.id);
+          clearOAuthInFlightRef.current.delete(inFlightKey);
         })
         .catch((err: unknown) => {
           notifications.show({
@@ -1392,7 +1409,12 @@ function App() {
           });
         });
     },
-    [clearServerOAuthAndDisconnect],
+    [
+      clearServerOAuthAndDisconnect,
+      activeServerId,
+      inspectorClient,
+      activeServer,
+    ],
   );
 
   const handleClearConnectionOAuth = useCallback(() => {

clients/web/src/hooks/useOAuthRecovery.test.tsx

@@ -112,6 +112,12 @@ const entry = (
   ...over,
 });
 
+/** A catalog entry pointing at a different URL, so a different OAuth blob. */
+const otherUrlEntry = (id: string): ServerEntry =>
+  entry(id, {
+    config: { type: "streamable-http", url: "https://other.example/mcp" },
+  });
+
 const challenge = (
   reason: AuthChallenge["reason"] = "unauthorized",
 ): AuthChallenge => ({ reason });
@@ -151,6 +157,13 @@ function fakeClient(over: Partial<Record<string, unknown>> = {}) {
     handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "failed" }),
     disconnect: vi.fn().mockResolvedValue(undefined),
     resumeAfterOAuth: vi.fn().mockResolvedValue(undefined),
+    // #2217: the clear path resolves the session's OAuth key from the config
+    // the client was built with, since a catalog entry can be edited while
+    // connected without rebuilding it. Defaults to the shared fixture URL.
+    getTransportConfig: vi.fn(() => ({
+      type: "streamable-http" as const,
+      url: "https://mcp.example/mcp",
+    })),
     ...over,
   });
 }
@@ -1801,14 +1814,105 @@ describe("useOAuthRecovery", () => {
       const client = fakeClient();
       const h = harness({ servers: [entry("a")], activeServerId: "a", client });
       await act(async () => {
-        await h.api().clearServerOAuthAndDisconnect(entry("b"));
+        // A genuinely unrelated entry: its own URL, so its own OAuth blob.
+        await h.api().clearServerOAuthAndDisconnect(otherUrlEntry("b"));
       });
       expect(client.disconnect).not.toHaveBeenCalled();
       expect(
         toastWith('Stored OAuth state was removed for "Server b"'),
       ).toBeDefined();
     });
 
+    // #2217 — OAuth state is keyed by URL, so two catalog entries against the
+    // same URL share one blob. Clearing the inactive one destroys the active
+    // session's tokens (and revokes its grant), which an id-only check cannot
+    // see; the session was left connected on dead credentials with no notice.
+    it("disconnects the active session when clearing an entry that shares its URL", async () => {
+      const client = fakeClient();
+      const h = harness({
+        servers: [entry("a"), entry("b")],
+        activeServerId: "a",
+        client,
+      });
+      await act(async () => {
+        await h.api().clearServerOAuthAndDisconnect(entry("b"));
+      });
+      expect(client.disconnect).toHaveBeenCalled();
+      // The live client owns the clear, so in-memory flow state goes too.
+      expect(clearServerOAuthStateMock).toHaveBeenCalledWith(
+        expect.objectContaining({
+          isActiveConnection: true,
+          inspectorClient: client,
+        }),
+      );
+      expect(
+        toastWith(
+          "authorizes against the same URL, so its stored tokens went too",
+        ),
+      ).toBeDefined();
+    });
+
+    // Copilot on this PR: a card can be edited while connected and the catalog
+    // write does not rebuild the client, so the active *entry* can read a URL
+    // the live session never authorized against. Reading the entry would miss
+    // an entry still sitting on the client's real URL.
+    it("compares against the live client's URL, not the edited catalog entry's", async () => {
+      const client = fakeClient({
+        // Still authorized against the shared URL...
+        getTransportConfig: vi.fn(() => ({
+          type: "streamable-http" as const,
+          url: "https://mcp.example/mcp",
+        })),
+      });
+      const h = harness({
+        // ...while A's catalog entry has since been edited elsewhere.
+        servers: [otherUrlEntry("a"), entry("b")],
+        activeServerId: "a",
+        client,
+      });
+      await act(async () => {
+        await h.api().clearServerOAuthAndDisconnect(entry("b"));
+      });
+      expect(client.disconnect).toHaveBeenCalled();
+    });
+
+    // The same-URL branch still snapshots the session it acted on, so a switch
+    // during the in-flight clear must not drag the cleanup onto the new one.
+    it("does not disconnect a session switched to during a shared-URL clear", async () => {
+      let settle: (r: { cleared: boolean }) => void = () => {};
+      clearServerOAuthStateMock.mockImplementation(
+        () =>
+          new Promise((resolve) => {
+            settle = resolve as typeof settle;
+          }),
+      );
+      const client = fakeClient();
+      const h = harness({
+        servers: [entry("a"), entry("b"), otherUrlEntry("c")],
+        activeServerId: "a",
+        client,
+      });
+
+      let done: Promise<void>;
+      await act(async () => {
+        done = h.api().clearServerOAuthAndDisconnect(entry("b"));
+        await Promise.resolve();
+      });
+
+      h.rerender({
+        servers: [entry("a"), entry("b"), otherUrlEntry("c")],
+        activeServerId: "c",
+        client,
+      });
+
+      await act(async () => {
+        settle({ cleared: true });
+        await done;
+      });
+
+      expect(client.disconnect).not.toHaveBeenCalled();
+    });
+
     // #2144 — this is the web client's production wiring for revocation.
     // Without asserting the arguments, removing the per-server opt-out or
     // handing it the page-origin fetch would leave every test green.