diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index f9fc7f8a2..ab929146c 100644 --- a/clients/web/src/App.test.tsx +++ b/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", () => ({ + {/* A second settings target, so a test can drive a clear against an + entry other than the active one (#2217). */} + {/* 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, + which: "open-settings" | "open-settings-b", + ): Promise { + 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(); + + 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; + }; + + // 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), + ); + }, + ); +}); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 16f38cd43..03bcda5d4 100644 --- a/clients/web/src/App.tsx +++ b/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>(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(() => { diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 1e3055b4b..dabc41f72 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/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> = {}) { 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,7 +1814,8 @@ 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( @@ -1809,6 +1823,96 @@ describe("useOAuthRecovery", () => { ).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; + 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. diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 44081a2cd..a47b94c20 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -35,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, @@ -1430,8 +1431,31 @@ export function useOAuthRecovery({ const clearServerOAuthAndDisconnect = useCallback( async (server: ClearableServer) => { - const isActive = server.id === activeServerId; - const client = isActive ? inspectorClient : null; + // 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 + // are a supported (and useful) state: separate names, headers or + // settings against one server. Clearing the inactive one deletes the + // *shared* blob and, with #2144 in, revokes the active session's grant — + // while an id-only check takes the inactive branch and leaves that + // session connected on credentials that no longer exist, silently + // (#2217). Comparing the storage keys is what sees it; the id check + // cannot, by construction. + // + // 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 // modal and switch servers. `isActive` and `inspectorClient` were @@ -1446,15 +1470,19 @@ export function useOAuthRecovery({ // disconnect/reconnect to the SAME server builds a replacement // `InspectorClient`, so an id-only check passes again and the old clear // would run its session-wide cleanup against the new session. + // + // The id compared is the *active* one snapshotted alongside `isActive`, + // not the cleared entry's — for a shared-key clear those differ, and the + // session being protected is the active one. const stillTargetsActiveSession = (): boolean => - isActive && - sessionRef.current.activeServerId === server.id && + affectsActiveSession && + sessionRef.current.activeServerId === activeServerId && sessionRef.current.inspectorClient === client; const { cleared, revocation } = await clearServerOAuthState({ config: server.config, inspectorClient: client, - isActiveConnection: isActive, + isActiveConnection: affectsActiveSession, oauthStorage: webOAuthStorage, revoke: server.settings?.oauthRevokeOnClear !== false, fetchFn: getWebProxiedFetch(getAuthToken()), @@ -1481,7 +1509,7 @@ export function useOAuthRecovery({ finalizeExplicitDisconnect(); } } - } else if (!isActive || stillTargetsActiveSession()) { + } else if (!affectsActiveSession || stillTargetsActiveSession()) { // No client to disconnect — either this is a stored-only clear, or the // active session has none yet (it is being built or torn down). Either // way the resume snapshot is stale and must go; skipping it would leave @@ -1495,7 +1523,14 @@ export function useOAuthRecovery({ title: "OAuth state cleared", message: isActive ? `Stored tokens and client registration were removed. Reconnect to run a fresh authorization flow.${revocationSuffix(revocation)}` - : `Stored OAuth state was removed for "${server.name}". Connect to authorize again.${revocationSuffix(revocation)}`, + : sharesActiveOAuthKey + ? // States the credential impact, which is certain, rather than + // the disconnect, which is not: the stale-session guard skips it + // after a switch, and `disconnect()` can reject (that failure + // gets its own toast above). Either way the shared state is gone, + // so the session must reconnect (Copilot). + `Stored OAuth state was removed for "${server.name}". The active session authorizes against the same URL, so its stored tokens went too — reconnect to run a fresh authorization flow.${revocationSuffix(revocation)}` + : `Stored OAuth state was removed for "${server.name}". Connect to authorize again.${revocationSuffix(revocation)}`, color: "blue", }); }, diff --git a/clients/web/src/utils/oauthClearKey.test.ts b/clients/web/src/utils/oauthClearKey.test.ts new file mode 100644 index 000000000..ebce1b24f --- /dev/null +++ b/clients/web/src/utils/oauthClearKey.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it } from "vitest"; +import { oauthClearKey, resolveOAuthClearIdentity } from "./oauthClearKey"; + +describe("oauthClearKey", () => { + // #2217 — the whole point: two entries against one URL share one OAuth blob, + // so they must share one clear identity even though their ids differ. + it("keys two entries with the same URL identically", () => { + const config = { + type: "streamable-http", + url: "https://mcp.example/mcp", + } as const; + expect(oauthClearKey(config, "a")).toBe(oauthClearKey(config, "b")); + }); + + it("separates entries pointing at different URLs", () => { + expect( + oauthClearKey({ type: "sse", url: "https://one.example/mcp" }, "a"), + ).not.toBe( + oauthClearKey({ type: "sse", url: "https://two.example/mcp" }, "a"), + ); + }); + + it("falls back to the entry id when there is no OAuth server URL", () => { + const stdio = { type: "stdio", command: "node" } as const; + expect(oauthClearKey(stdio, "a")).toBe("id:a"); + expect(oauthClearKey(stdio, "a")).not.toBe(oauthClearKey(stdio, "b")); + }); + + // The prefix is what keeps a stdio entry named "url:…" from colliding with a + // URL-keyed one — an id is user-supplied and a URL is not a reserved shape. + it("cannot collide a URL key with an id key", () => { + expect( + oauthClearKey({ type: "stdio", command: "node" }, "url:https://x/mcp"), + ).not.toBe( + oauthClearKey({ type: "sse", url: "https://x/mcp" }, "anything"), + ); + }); +}); + +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[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"); + }); +}); diff --git a/clients/web/src/utils/oauthClearKey.ts b/clients/web/src/utils/oauthClearKey.ts new file mode 100644 index 000000000..5c2b4cf44 --- /dev/null +++ b/clients/web/src/utils/oauthClearKey.ts @@ -0,0 +1,91 @@ +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, 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 + * point at one URL and therefore share one credential blob, one live grant and + * one revocation. Anything that dedupes, locks or compares *clears* has to key + * on that shared identity rather than on the entry id, which cannot see it + * (#2217). + * + * A config with no OAuth server URL (stdio, and anything else + * `getOAuthServerUrl` declines) has no shared blob to collide over, so it falls + * back to the entry id — which keeps every such entry distinct from every + * other, rather than collapsing them all onto one shared key. + */ +export function oauthClearKey(config: MCPServerConfig, id: string): string { + const url = getOAuthServerUrl(config); + // 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), + }; +}