diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 659a0433..ab929146 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; }), diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index e86dd2c6..03bcda5d 100644 --- a/clients/web/src/App.tsx +++ b/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(() => { diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 6ffcc6c1..c5b30352 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -157,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, }); } @@ -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 () => { diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 9998590d..5c3cb9e4 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/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 diff --git a/clients/web/src/utils/oauthClearKey.test.ts b/clients/web/src/utils/oauthClearKey.test.ts index 7132f2d9..ebce1b24 100644 --- a/clients/web/src/utils/oauthClearKey.test.ts +++ b/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[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 index 776139b4..5c2b4cf4 100644 --- a/clients/web/src/utils/oauthClearKey.ts +++ b/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), + }; +}