modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: key the OAuth clear's active-session check on the storage key (#2217)

Browse source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-03 19:42:37 -04:00
commit fcec17f5d1
1 changed files with 109 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

43
clients/web/src/hooks/useOAuthRecovery.ts
View file

@ -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",
});
},