diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 91b3f10d..5cd1da51 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 }); @@ -1608,7 +1614,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( @@ -1616,6 +1623,70 @@ 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 it shared that state"), + ).toBeDefined(); + }); + + // 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 428352b7..0223bdb2 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -16,6 +16,7 @@ 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"; @@ -1380,7 +1381,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. + 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; + 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 @@ -1395,15 +1420,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()), @@ -1430,7 +1459,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 @@ -1444,7 +1473,9 @@ 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 + ? `Stored OAuth state was removed for "${server.name}". The active session authorizes against the same URL, so it shared that state and was disconnected 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", }); },