From fcec17f5d1f7085a934c175f72cfa5d2e9b36592 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 19:42:37 -0400 Subject: [PATCH] fix: key the OAuth clear's active-session check on the storage key (#2217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OAuth state is keyed by server URL, but `clearServerOAuthAndDisconnect` decided "is this the active connection" from the catalog entry id. Those are different identities and nothing keeps them in sync: `serverList` enforces no URL uniqueness, so two entries with distinct ids against one URL are a supported state — and a natural one, since separate entries are how a user keeps different names, headers or per-server settings against the same server. Clearing the inactive entry therefore deleted the shared URL-keyed blob — the active session's tokens, DCR client id and PKCE state — and, with #2144 in, revoked its grant at the authorization server, while the id check took the inactive branch: no live-client clear, no disconnect, no session cleanup. The active session was left connected on credentials that no longer existed, with nothing told to the user; the break then surfaced somewhere else entirely, on the next refresh, 401 or reload. The branch now compares the OAuth storage key of the cleared entry against the active entry's, and treats a match as affecting the active session — live-client clear, disconnect, session cleanup — exactly as clearing the active entry does. The existing id and client-identity checks stay on top: they guard the separate stale-session race from #2144, and the snapshotted id they revalidate is now the active one rather than the cleared entry's, which for a shared-key clear are not the same. The toast says why the session went away. The alternative, prohibiting duplicate URLs in `serverList`, would remove a legitimate workflow to fix a bug in the consumer and could not repair catalogs that already hold duplicates. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 73 ++++++++++++++++++- clients/web/src/hooks/useOAuthRecovery.ts | 43 +++++++++-- 2 files changed, 109 insertions(+), 7 deletions(-) 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", }); },