From fcec17f5d1f7085a934c175f72cfa5d2e9b36592 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 19:42:37 -0400 Subject: [PATCH 1/6] 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 91b3f10d0..5cd1da51d 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 428352b77..0223bdb2f 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", }); }, From 79896b63560874c78800a902011e492c06ed9ff7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:28:53 -0400 Subject: [PATCH 2/6] fix: state the credential impact, not the disconnect, in the shared-URL toast Copilot: the stale-session guard deliberately skips `disconnect()` after a switch, and `disconnect()` can also reject (which gets its own toast), so claiming the active session "was disconnected too" is not always true. What is always true is that its stored tokens went with the shared blob and it must reconnect, so the message says that instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- clients/web/src/hooks/useOAuthRecovery.test.tsx | 4 +++- clients/web/src/hooks/useOAuthRecovery.ts | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 5cd1da51d..6ffcc6c17 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1646,7 +1646,9 @@ describe("useOAuthRecovery", () => { }), ); expect( - toastWith("authorizes against the same URL, so it shared that state"), + toastWith( + "authorizes against the same URL, so its stored tokens went too", + ), ).toBeDefined(); }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 0223bdb2f..9998590d2 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1474,7 +1474,12 @@ export function useOAuthRecovery({ message: isActive ? `Stored tokens and client registration were removed. Reconnect to run a fresh authorization flow.${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)}` + ? // 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", }); From b2bbfcb409900fd10c82f57a19f61b71bdf33f29 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:48:45 -0400 Subject: [PATCH 3/6] fix: key the in-flight clear guard on the OAuth storage key too (#2217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot: `runClear`'s double-click guard deduped by `server.id`, which is the same identity mismatch this PR fixes, one layer up. Two catalog entries against one URL share one credential blob, one grant and one revocation, so an id-keyed guard let exactly the race it exists to prevent through between them — concurrent store writes, concurrent RFC 7009 requests, and two contradictory teardown toasts. The guard now keys on `oauthClearKey`, a pure helper that resolves the OAuth storage key and falls back to the entry id for a config that has none (stdio), where there is no shared state to collide over. The id fallback keeps such entries distinct from each other rather than collapsing them onto one key, and both forms are prefixed so a user-supplied id can never impersonate a URL. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- clients/web/src/App.tsx | 22 ++++++++---- clients/web/src/utils/oauthClearKey.test.ts | 38 +++++++++++++++++++++ clients/web/src/utils/oauthClearKey.ts | 23 +++++++++++++ 3 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 clients/web/src/utils/oauthClearKey.test.ts create mode 100644 clients/web/src/utils/oauthClearKey.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 16f38cd43..e86dd2c67 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 000000000..7132f2d9c --- /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 000000000..776139b4c --- /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}`; +} From 0d81cf7e84f713f97312fa6da89956e6ec1f5b96 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 22:03:21 -0400 Subject: [PATCH 4/6] test: cover the App-level shared-key clear suppression (#2217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot: `oauthClearKey`'s unit tests prove the derivation, but a regression that dropped the `has` check, added or deleted the wrong key, or invoked the clear twice would still have passed. Adds an App.test.tsx case that connects entry A, holds its clear open on a deferred `clearOAuthTokens`, then drives a clear for entry B — a different catalog id against the same URL — and asserts the underlying clear ran exactly once, and once more after the first settles, so the key is proven released rather than leaking a permanently dead control. Verified to fail against the id-keyed guard. Two supporting changes it needs: - an `open-settings-b` control on the mocked InspectorView, the only way to reach a settings modal for a server other than the active one; - `@inspector/core/mcp/remote/index.js` becomes a partial mock. `getWebProxiedFetch` reaches for `createRemoteFetch` there, and the bare stub threw a missing-export error before the clear under test ever ran — the failure surfaced as the generic "Could not clear the stored OAuth state" toast. The case carries an explicit 20s timeout and `delay: null`: it drives three full modal interaction sequences against the whole App tree, which runs past the 5s default when the suite is under load. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 129 ++++++++++++++++++++++++++++++++++- 1 file changed, 128 insertions(+), 1 deletion(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index f9fc7f8a2..c6a2dd0d0 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -244,7 +244,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 +607,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 +4225,119 @@ 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(), + } as unknown as ReturnType); + }); + + 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)); + const client = clientInstances[0] as unknown as { + 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), + ); + }, + ); +}); From be867919005fe379e949bc569ac9bd8bb3112990 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 23:31:38 -0400 Subject: [PATCH 5/6] test: drop two double casts from the new App case (#2217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, and the repo's own rule against an unjustified `as unknown as`. The `useServers` cast was the more useful catch: removing it surfaced that the mock omitted `reorderServers` and `importSource`, so App code calling either would have read `undefined` with this test still passing type-check. The full result shape is supplied instead, matching the `mockServersWith` helper above. The client cast becomes an intersection — the instances are already typed `EventTarget`, so naming the test-only spy on top of that expresses what is needed without erasing the rest. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index c6a2dd0d0..659a04339 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -4258,7 +4258,9 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn(), - } as unknown as ReturnType); + reorderServers: vi.fn(), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); }); afterEach(() => { @@ -4299,7 +4301,9 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", await user.click(screen.getByText("connect")); await waitFor(() => expect(clientInstances).toHaveLength(1)); - const client = clientInstances[0] as unknown as { + // The instances are typed `EventTarget`; an intersection names the + // test-only spy without erasing that (Copilot). + const client = clientInstances[0] as EventTarget & { clearOAuthTokens: ReturnType; }; From f78a00ea6cb54c406c718d5381f5ea90bd98ed95 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 00:02:45 -0400 Subject: [PATCH 6/6] fix: resolve the clear's identity from the live client, in one place (#2217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot: the catalog entry is mutable and the live client is not. A card can be edited while connected — `onConfigSubmit` writes the catalog and does not rebuild the client — so after A connects to X and is edited to Y, the entry reads Y while the session is still authorized against X. Both consumers read the entry, so both got it wrong in the same way: - the hook compared a cleared entry against A's *entry* URL, so clearing another entry still at X missed the match and deleted and revoked the live client's X-keyed credentials without disconnecting it — the exact failure this PR exists to prevent; - `runClear` locked on the entry's key, so a clear of edited-A (`url:Y`) and a clear of entry B at X (`url:X`) took different locks while both performed the same X-keyed operation through the same live client. `InspectorClient.getTransportConfig()` returns the config it was constructed with, which is what its credentials are keyed under, so both now resolve from that and fall back to the catalog entry only when there is no client. The rule moves into one exported function, `resolveOAuthClearIdentity`. Two copies of it that disagree is precisely the class of bug #2217 is, and the two consumers each re-deriving it is how this round's finding came to exist at all. Both fakes gain a `getTransportConfig`, and three tests cover the split: the resolver's own two (the client's URL wins; an active clear locks on the client's key) and a hook case asserting an edited active entry still disconnects when the entry sharing the client's real URL is cleared. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 8 +- clients/web/src/App.tsx | 24 +++- .../web/src/hooks/useOAuthRecovery.test.tsx | 31 +++++ clients/web/src/hooks/useOAuthRecovery.ts | 31 +++-- clients/web/src/utils/oauthClearKey.test.ts | 115 +++++++++++++++++- clients/web/src/utils/oauthClearKey.ts | 70 ++++++++++- 6 files changed, 255 insertions(+), 24 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 659a04339..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; }), diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index e86dd2c67..03bcda5d4 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 6ffcc6c17..c5b303527 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 9998590d2..5c3cb9e49 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 7132f2d9c..ebce1b24f 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 776139b4c..5c2b4cf44 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), + }; +}