You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found in the v2.5.0 milestone-merge review (#2215), against code that shipped on v2/main during the milestone (#2144 / #2186). Filed here rather than fixed in the merge PR, whose tree is byte-identical to origin/v2/main.
The mismatch
OAuth state is keyed by server URL. The "is this the active connection" check is keyed by catalog entry id. Those are not the same identity, and nothing keeps them in sync.
clients/web/src/lib/clearServerOAuthState.ts resolves its target with getOAuthServerUrl(params.config) — for sse and streamable-http that is literally config.url — and clears the OAuthStorage blob under it.
clients/web/src/hooks/useOAuthRecovery.ts (clearServerOAuthAndDisconnect) decides everything else from server.id === activeServerId: whether to route through the live InspectorClient, whether to disconnect, and whether to run the session-wide UI cleanup.
core/mcp/serverList.ts enforces no URL uniqueness, so two catalog entries with distinct ids and the same URL are a supported state — and a natural one, since separate entries are how a user keeps different names, custom headers or per-server settings against one server. The hook's own test fixture already builds them that way: entry("a") and entry("b") both carry https://mcp.example/mcp.
Consequence
With entry a connected and entry b inactive, clearing b:
deletes the shared URL-keyed OAuth blob — a's tokens, DCR client id and PKCE state;
takes the inactive branch because b.id !== activeServerId — so it does not borrow the live client, does not disconnect a, and does not run the session cleanup or show the re-auth banner.
Entry a is left connected on an in-memory access token whose persisted state is gone and whose grant may be dead at the authorization server. Nothing tells the user. The break surfaces later and somewhere else — the next refresh, the next 401, or the next reload — which is exactly the shape that is expensive to attribute.
Note this is not the stale-session race the surrounding comment block addresses (#2144's stillTargetsActiveSession guard, and the client-identity half of it). That guard is correct for what it covers: it stops a suspended callback applying to a session the user switched to. This is a different axis — two distinct entries that legitimately share one credential store — and the id check cannot see it by construction.
Fix
Decide the branch on whether the cleared entry shares the active server's OAuth storage key, not on the entry id:
resolve getOAuthServerUrl for both the cleared entry and the active one and compare, keeping the existing id and client-identity checks on top for the session-cleanup half (they guard a real and separate hazard);
when the keys match but the ids do not, the active session's credentials are being destroyed — so it must take the live-client path, disconnect, and surface the re-auth banner, exactly as clearing the active entry does.
The alternative, prohibiting duplicate URL keys in serverList.ts, is worse: it removes a legitimate workflow to fix a bug in the consumer, and it cannot repair catalogs that already hold duplicates.
Tests should cover the case directly: two entries, same URL, one connected — clear the inactive one and assert the active session is disconnected and re-auth-flagged rather than silently left on dead credentials. The existing entry("a") / entry("b") fixture already supplies the shape.
Found in the v2.5.0 milestone-merge review (#2215), against code that shipped on
v2/mainduring the milestone (#2144 / #2186). Filed here rather than fixed in the merge PR, whose tree is byte-identical toorigin/v2/main.The mismatch
OAuth state is keyed by server URL. The "is this the active connection" check is keyed by catalog entry id. Those are not the same identity, and nothing keeps them in sync.
clients/web/src/lib/clearServerOAuthState.tsresolves its target withgetOAuthServerUrl(params.config)— forsseandstreamable-httpthat is literallyconfig.url— and clears theOAuthStorageblob under it.clients/web/src/hooks/useOAuthRecovery.ts(clearServerOAuthAndDisconnect) decides everything else fromserver.id === activeServerId: whether to route through the liveInspectorClient, whether to disconnect, and whether to run the session-wide UI cleanup.core/mcp/serverList.tsenforces no URL uniqueness, so two catalog entries with distinct ids and the same URL are a supported state — and a natural one, since separate entries are how a user keeps different names, custom headers or per-server settings against one server. The hook's own test fixture already builds them that way:entry("a")andentry("b")both carryhttps://mcp.example/mcp.Consequence
With entry a connected and entry b inactive, clearing b:
b.id !== activeServerId— so it does not borrow the live client, does not disconnect a, and does not run the session cleanup or show the re-auth banner.Entry a is left connected on an in-memory access token whose persisted state is gone and whose grant may be dead at the authorization server. Nothing tells the user. The break surfaces later and somewhere else — the next refresh, the next 401, or the next reload — which is exactly the shape that is expensive to attribute.
Note this is not the stale-session race the surrounding comment block addresses (#2144's
stillTargetsActiveSessionguard, and the client-identity half of it). That guard is correct for what it covers: it stops a suspended callback applying to a session the user switched to. This is a different axis — two distinct entries that legitimately share one credential store — and the id check cannot see it by construction.Fix
Decide the branch on whether the cleared entry shares the active server's OAuth storage key, not on the entry id:
getOAuthServerUrlfor both the cleared entry and the active one and compare, keeping the existing id and client-identity checks on top for the session-cleanup half (they guard a real and separate hazard);The alternative, prohibiting duplicate URL keys in
serverList.ts, is worse: it removes a legitimate workflow to fix a bug in the consumer, and it cannot repair catalogs that already hold duplicates.Tests should cover the case directly: two entries, same URL, one connected — clear the inactive one and assert the active session is disconnected and re-auth-flagged rather than silently left on dead credentials. The existing
entry("a")/entry("b")fixture already supplies the shape.Reported by Copilot on #2215.