Skip to content

Clearing OAuth for one catalog entry silently kills another entry that shares its URL #2217

Description

@cliffhall

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:

  1. deletes the shared URL-keyed OAuth blob — a's tokens, DCR client id and PKCE state;
  2. with feat(auth): revoke OAuth tokens at the authorization server on clear (RFC 7009) #2186 in, revokes a's grant at the authorization server over RFC 7009;
  3. 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.

Reported by Copilot on #2215.

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingv2Issues and PRs for v2

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions