fix: resolve the clear's identity from the live client, in one place (#2217)

Copilot: the catalog entry is mutable and the live client is not. A card
can be edited while connected — `onConfigSubmit` writes the catalog and
does not rebuild the client — so after A connects to X and is edited to
Y, the entry reads Y while the session is still authorized against X.
Both consumers read the entry, so both got it wrong in the same way:

- the hook compared a cleared entry against A's *entry* URL, so clearing
  another entry still at X missed the match and deleted and revoked the
  live client's X-keyed credentials without disconnecting it — the exact
  failure this PR exists to prevent;
- `runClear` locked on the entry's key, so a clear of edited-A (`url:Y`)
  and a clear of entry B at X (`url:X`) took different locks while both
  performed the same X-keyed operation through the same live client.

`InspectorClient.getTransportConfig()` returns the config it was
constructed with, which is what its credentials are keyed under, so both
now resolve from that and fall back to the catalog entry only when there
is no client.

The rule moves into one exported function, `resolveOAuthClearIdentity`.
Two copies of it that disagree is precisely the class of bug #2217 is,
and the two consumers each re-deriving it is how this round's finding
came to exist at all.

Both fakes gain a `getTransportConfig`, and three tests cover the split:
the resolver's own two (the client's URL wins; an active clear locks on
the client's key) and a hook case asserting an edited active entry still
disconnects when the entry sharing the client's real URL is cleared.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>

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;
     }),

clients/web/src/App.tsx

@@ -115,7 +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 { oauthClearKey } from "./utils/oauthClearKey";
+import { resolveOAuthClearIdentity } from "./utils/oauthClearKey";
 import {
   bodyDroppedToastId,
   CLIENT_CONFIG_LOAD_ERROR_NOTIFICATION_ID,
@@ -1380,9 +1380,18 @@ function App() {
       // "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. `oauthClearKey` falls back to the id for a
-      // config with no OAuth URL, which has no shared state to collide over.
-      const inFlightKey = oauthClearKey(server.config, server.id);
+      // 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)
@@ -1400,7 +1409,12 @@ function App() {
           });
         });
     },
-    [clearServerOAuthAndDisconnect],
+    [
+      clearServerOAuthAndDisconnect,
+      activeServerId,
+      inspectorClient,
+      activeServer,
+    ],
   );
 
   const handleClearConnectionOAuth = useCallback(() => {

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

@@ -157,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,
   });
 }
@@ -1652,6 +1659,30 @@ describe("useOAuthRecovery", () => {
       ).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 () => {

clients/web/src/hooks/useOAuthRecovery.ts

@@ -16,7 +16,6 @@ import {
   parseOAuthCallbackParams,
   parseOAuthState,
 } from "@inspector/core/auth/index.js";
-import { getOAuthServerUrl } from "@inspector/core/mcp/config.js";
 import { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js";
 import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
 import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js";
@@ -36,6 +35,7 @@ import { oauthDetailsFromConnectionState } from "../components/groups/Connection
 import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage";
 import { getWebProxiedFetch } from "../lib/webProxiedFetch";
 import { clearServerOAuthState } from "../lib/clearServerOAuthState";
+import { resolveOAuthClearIdentity } from "../utils/oauthClearKey";
 import { getAuthToken } from "../lib/authToken";
 import {
   isBrowserTabVisible,
@@ -1380,7 +1380,6 @@ export function useOAuthRecovery({
 
   const clearServerOAuthAndDisconnect = useCallback(
     async (server: ClearableServer) => {
-      const isActive = server.id === activeServerId;
       // OAuth state is keyed by the server URL, but "is this the active
       // connection" was keyed by catalog entry id — and `serverList` enforces
       // no URL uniqueness, so two entries with different ids and the same URL
@@ -1391,20 +1390,20 @@ export function useOAuthRecovery({
       // session connected on credentials that no longer exist, silently
       // (#2217). Comparing the storage keys is what sees it; the id check
       // cannot, by construction.
-      const activeOAuthKey = (): string | undefined => {
-        const active = sessionRef.current.servers.find(
-          (s) => s.id === activeServerId,
-        );
-        return active ? getOAuthServerUrl(active.config) : undefined;
-      };
-      const clearedOAuthKey = getOAuthServerUrl(server.config);
-      const sharesActiveOAuthKey =
-        !isActive &&
-        clearedOAuthKey !== undefined &&
-        clearedOAuthKey === activeOAuthKey();
-      // Either way the active session's credentials are being destroyed, so it
-      // takes the live-client path, disconnects, and runs the session cleanup.
-      const affectsActiveSession = isActive || sharesActiveOAuthKey;
+      //
+      // The live client's own config is what the comparison uses, because a
+      // card can be edited while connected and the catalog write does not
+      // rebuild the client (Copilot). `resolveOAuthClearIdentity` owns the
+      // rule; `App`'s in-flight guard reads the same answer from it.
+      const { isActive, sharesActiveOAuthKey, affectsActiveSession } =
+        resolveOAuthClearIdentity({
+          server,
+          activeServerId,
+          activeClientConfig: inspectorClient?.getTransportConfig(),
+          activeEntryConfig: sessionRef.current.servers.find(
+            (s) => s.id === activeServerId,
+          )?.config,
+        });
       const client = affectsActiveSession ? inspectorClient : null;
       // The RFC 7009 leg is a bounded network request (#2144), so this callback
       // can stay suspended for seconds — long enough for the user to close the

clients/web/src/utils/oauthClearKey.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { oauthClearKey } from "./oauthClearKey";
+import { oauthClearKey, resolveOAuthClearIdentity } from "./oauthClearKey";
 
 describe("oauthClearKey", () => {
   // #2217 — the whole point: two entries against one URL share one OAuth blob,
@@ -36,3 +36,116 @@ describe("oauthClearKey", () => {
     );
   });
 });
+
+describe("resolveOAuthClearIdentity", () => {
+  const http = (url: string) => ({ type: "streamable-http", url }) as const;
+  const X = http("https://x.example/mcp");
+  const Y = http("https://y.example/mcp");
+
+  const resolve = (
+    over: Partial<Parameters<typeof resolveOAuthClearIdentity>[0]> = {},
+  ) =>
+    resolveOAuthClearIdentity({
+      server: { id: "A", config: X },
+      activeServerId: "A",
+      activeClientConfig: X,
+      activeEntryConfig: X,
+      ...over,
+    });
+
+  it("treats the active entry as affecting the session", () => {
+    const id = resolve();
+    expect(id.isActive).toBe(true);
+    expect(id.sharesActiveOAuthKey).toBe(false);
+    expect(id.affectsActiveSession).toBe(true);
+  });
+
+  it("treats an unrelated entry as affecting nothing", () => {
+    const id = resolve({ server: { id: "B", config: Y } });
+    expect(id.affectsActiveSession).toBe(false);
+    expect(id.inFlightKey).toBe("url:https://y.example/mcp");
+  });
+
+  // The #2217 case: distinct ids, one blob.
+  it("treats a different entry with the session's URL as affecting the session", () => {
+    const id = resolve({ server: { id: "B", config: X } });
+    expect(id.isActive).toBe(false);
+    expect(id.sharesActiveOAuthKey).toBe(true);
+    expect(id.affectsActiveSession).toBe(true);
+  });
+
+  // Copilot: a card can be edited while connected, and the catalog write does
+  // not rebuild the client — so the entry reads Y while the live session is
+  // still authorized against X. Reading the entry would miss entry B at X.
+  it("follows the live client's URL, not the edited catalog entry's", () => {
+    const edited = resolve({
+      server: { id: "B", config: X },
+      activeClientConfig: X,
+      activeEntryConfig: Y,
+    });
+    expect(edited.sharesActiveOAuthKey).toBe(true);
+    // And an entry at the entry's *new* URL is not the session's credentials.
+    const notShared = resolve({
+      server: { id: "B", config: Y },
+      activeClientConfig: X,
+      activeEntryConfig: Y,
+    });
+    expect(notShared.sharesActiveOAuthKey).toBe(false);
+  });
+
+  // The lock has to name the storage operation actually performed. Clearing
+  // the edited active entry routes through the live client, which still acts
+  // on X — so an entry-keyed lock (`url:Y`) would not collide with a
+  // concurrent clear of entry B at X, which performs the very same operation.
+  it("locks an active-session clear on the client's key, not the entry's", () => {
+    expect(
+      resolve({
+        server: { id: "A", config: Y },
+        activeClientConfig: X,
+        activeEntryConfig: Y,
+      }).inFlightKey,
+    ).toBe("url:https://x.example/mcp");
+    expect(
+      resolve({
+        server: { id: "B", config: X },
+        activeClientConfig: X,
+        activeEntryConfig: Y,
+      }).inFlightKey,
+    ).toBe("url:https://x.example/mcp");
+  });
+
+  it("falls back to the active entry's config when there is no live client", () => {
+    const id = resolve({
+      server: { id: "B", config: X },
+      activeClientConfig: undefined,
+      activeEntryConfig: X,
+    });
+    expect(id.sharesActiveOAuthKey).toBe(true);
+    expect(id.inFlightKey).toBe("url:https://x.example/mcp");
+  });
+
+  it("handles no active server at all", () => {
+    const id = resolve({
+      server: { id: "B", config: X },
+      activeServerId: undefined,
+      activeClientConfig: undefined,
+      activeEntryConfig: undefined,
+    });
+    expect(id.isActive).toBe(false);
+    expect(id.affectsActiveSession).toBe(false);
+    expect(id.inFlightKey).toBe("url:https://x.example/mcp");
+  });
+
+  // A stdio active session has no OAuth key, so an active clear against it
+  // falls back to the entry id rather than collapsing onto a shared URL key.
+  it("falls back to the entry id when the session has no OAuth URL", () => {
+    const stdio = { type: "stdio", command: "node" } as const;
+    expect(
+      resolve({
+        server: { id: "A", config: stdio },
+        activeClientConfig: stdio,
+        activeEntryConfig: stdio,
+      }).inFlightKey,
+    ).toBe("id:A");
+  });
+});

clients/web/src/utils/oauthClearKey.ts

@@ -2,7 +2,8 @@ import { getOAuthServerUrl } from "@inspector/core/mcp/config.js";
 import type { MCPServerConfig } from "@inspector/core/mcp/types.js";
 
 /**
- * The identity a *clear* of stored OAuth state acts on.
+ * The identity a *clear* of stored OAuth state acts on, for a config on its
+ * own.
  *
  * Persisted OAuth state is keyed by the server URL, and `core/mcp/serverList`
  * enforces no URL uniqueness — so two catalog entries with distinct ids can
@@ -21,3 +22,70 @@ export function oauthClearKey(config: MCPServerConfig, id: string): string {
   // Prefixed so a URL-keyed entry can never collide with an id-keyed one.
   return url !== undefined ? `url:${url}` : `id:${id}`;
 }
+
+/** What a clear is being asked to do, relative to the live session. */
+export interface OAuthClearIdentityInput {
+  /** The catalog entry whose OAuth state the user asked to clear. */
+  server: { id: string; config: MCPServerConfig };
+  activeServerId: string | undefined;
+  /**
+   * The config the **live** `InspectorClient` was built with
+   * (`getTransportConfig()`), when there is one.
+   *
+   * This — not the catalog entry — is what the session's credentials are keyed
+   * under. A card can be edited while connected and the catalog write does not
+   * rebuild the client, so after A connects to X and is edited to Y, the live
+   * client still authorizes against X while the entry reads Y (Copilot).
+   */
+  activeClientConfig: MCPServerConfig | undefined;
+  /** The active entry's catalog config, used only when there is no client. */
+  activeEntryConfig: MCPServerConfig | undefined;
+}
+
+export interface OAuthClearIdentity {
+  /** The cleared entry *is* the active one. */
+  isActive: boolean;
+  /** A different entry, whose OAuth state the live session is using. */
+  sharesActiveOAuthKey: boolean;
+  /** Either of the above: the active session's credentials are being destroyed. */
+  affectsActiveSession: boolean;
+  /**
+   * The identity to lock an in-flight clear on. Names the storage operation
+   * that will actually be performed — for anything touching the live session
+   * that is the client's key, which is not necessarily the entry's own.
+   */
+  inFlightKey: string;
+}
+
+/**
+ * Resolve who a clear affects and what it locks, from one place.
+ *
+ * Both consumers need the same answer and would otherwise each re-derive it:
+ * `useOAuthRecovery` to decide whether to route through the live client and
+ * disconnect, and `App`'s `runClear` to dedupe concurrent clears. Two copies of
+ * this rule that disagree is exactly the class of bug #2217 is.
+ */
+export function resolveOAuthClearIdentity({
+  server,
+  activeServerId,
+  activeClientConfig,
+  activeEntryConfig,
+}: OAuthClearIdentityInput): OAuthClearIdentity {
+  const isActive = activeServerId !== undefined && server.id === activeServerId;
+  // Client first: the entry is mutable, the built client is not.
+  const activeConfig = activeClientConfig ?? activeEntryConfig;
+  const activeKey = activeConfig ? getOAuthServerUrl(activeConfig) : undefined;
+  const clearedKey = getOAuthServerUrl(server.config);
+  const sharesActiveOAuthKey =
+    !isActive && clearedKey !== undefined && clearedKey === activeKey;
+  const affectsActiveSession = isActive || sharesActiveOAuthKey;
+  return {
+    isActive,
+    sharesActiveOAuthKey,
+    affectsActiveSession,
+    inFlightKey:
+      affectsActiveSession && activeKey !== undefined
+        ? `url:${activeKey}`
+        : oauthClearKey(server.config, server.id),
+  };
+}