diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 16f38cd4..e86dd2c6 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 { oauthClearKey } 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,18 @@ 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. `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); + 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({ diff --git a/clients/web/src/utils/oauthClearKey.test.ts b/clients/web/src/utils/oauthClearKey.test.ts new file mode 100644 index 00000000..7132f2d9 --- /dev/null +++ b/clients/web/src/utils/oauthClearKey.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { oauthClearKey } 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"), + ); + }); +}); diff --git a/clients/web/src/utils/oauthClearKey.ts b/clients/web/src/utils/oauthClearKey.ts new file mode 100644 index 00000000..776139b4 --- /dev/null +++ b/clients/web/src/utils/oauthClearKey.ts @@ -0,0 +1,23 @@ +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. + * + * 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}`; +}