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

fix(auth): make clear* hydration-safe, migrate raw-key blobs, doc keepalive/memo caveats

Browse source 
Address round-1 @claude review of #1592:

- OAuthStorageBase.clear* no longer mutate the pre-hydration (empty) store —
  a clear issued before hydration is deferred until it lands, so it merges
  onto the real persisted state instead of (a) being resurrected by the late
  rehydrate merge or (b) persisting a near-empty blob that clobbers every
  other server's on-disk credential. Already-hydrated clears stay synchronous.
- store.setServerState migrates a pre-normalization raw-key blob onto the
  canonical key on first partial write (was: shadowed it with a fresh
  canonical entry, orphaning the raw blob's other fields). clearServerState
  now also drops the raw-key orphan.
- Document the keepalive 64KB combined-body ceiling on the remote-storage
  write, the getRemoteOAuthStorage memo key intentionally omitting fetchFn,
  and the real ordering invariant behind the sync getScope.
- Tests for the clear-before-hydration and raw-key-migration paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
This commit is contained in:
cliffhall 2026-07-01 19:20:49 -04:00
commit 8c10b8eb8e
3 changed files with 197 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

80
core/auth/oauth-storage.ts
View file

@ -96,6 +96,34 @@ export class OAuthStorageBase implements OAuthStorage {
return this.hydrated;
}
/**
* Run a `clear*` mutation against a hydrated store: synchronously if
* hydration has already landed, otherwise deferred until it does.
*
* `clear*` are declared synchronous in {@link OAuthStorage} (some feed sync
* SDK getters), so unlike the `save*` family they cannot `await this.hydrated`
* up front. But a clear applied to the still-empty pre-hydration store is
* doubly wrong: (a) the pending `rehydrate()` would merge the persisted
* (un-cleared) blob back over it, resurrecting the credential; and worse,
* (b) applying it triggers a persist write of the near-empty store, which for
* a whole-blob adapter (file / remote HTTP) can clobber the on-disk blob —
* every other server's tokens included — before hydration ever reads it.
* Deferring the mutation until after hydration avoids both: it merges onto
* the real persisted state. The common case (hydration already settled) stays
* synchronous and immediately observable. A failed hydration still resolves
* {@link hydrated} (see constructor), so a deferred clear runs harmlessly
* against the empty store. The effect of a pre-hydration clear is therefore
* asynchronous, which is safe: the only callers are fire-and-forget
* invalidations that never synchronously read the value straight back.
*/
private clearAfterHydration(mutate: () => void): void {
if (this.store.persist.hasHydrated()) {
mutate();
} else {
void this.hydrated.then(mutate);
}
}
async getClientInformation(
serverUrl: string,
isPreregistered?: boolean,
@ -179,7 +207,9 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearTokens(serverUrl: string): void {
this.store.getState().setServerState(serverUrl, { tokens: undefined });
this.clearAfterHydration(() =>
this.store.getState().setServerState(serverUrl, { tokens: undefined }),
);
}
async getCodeVerifier(serverUrl: string): Promise<string | undefined> {
@ -197,18 +227,28 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearCodeVerifier(serverUrl: string): void {
this.store
.getState()
.setServerState(serverUrl, { codeVerifier: undefined });
this.clearAfterHydration(() =>
this.store
.getState()
.setServerState(serverUrl, { codeVerifier: undefined }),
);
}
/**
* Intentionally synchronous. The only caller is
* {@link BaseOAuthClientProvider.scope}, a sync getter the SDK requires
* (it feeds the sync `clientMetadata` getter). Scope is always set via
* {@link saveScope} in the same session before it's read (during the
* pre-redirect half of the flow), so the in-memory store has the value
* regardless of async hydration.
* (it feeds the sync `clientMetadata` getter). It is safe without awaiting
* hydration because of an ordering invariant the callers uphold: some
* awaited read always precedes it, flushing hydration first.
* - Pre-redirect half of the flow: `saveScope` (which awaits hydration)
* writes the value in the same session, so the in-memory store has it.
* - Post-redirect callback path: the in-memory store is reset, so the
* value comes from hydration — but `buildOAuthConnectionState`
* (`connection-state.ts`) awaits `getClientInformation`/
* `getServerMetadata` (both of which await hydration) before it reads
* `getScope`, so hydration has already landed by then.
* A future refactor that reads `getScope` without a preceding awaited
* storage read would break this invariant.
*/
getScope(serverUrl: string): string | undefined {
const state = this.store.getState().getServerState(serverUrl);
@ -221,7 +261,9 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearScope(serverUrl: string): void {
this.store.getState().setServerState(serverUrl, { scope: undefined });
this.clearAfterHydration(() =>
this.store.getState().setServerState(serverUrl, { scope: undefined }),
);
}
async getServerMetadata(serverUrl: string): Promise<OAuthMetadata | null> {
@ -241,13 +283,17 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearServerMetadata(serverUrl: string): void {
this.store
.getState()
.setServerState(serverUrl, { serverMetadata: undefined });
this.clearAfterHydration(() =>
this.store
.getState()
.setServerState(serverUrl, { serverMetadata: undefined }),
);
}
clear(serverUrl: string): void {
this.store.getState().clearServerState(serverUrl);
this.clearAfterHydration(() =>
this.store.getState().clearServerState(serverUrl),
);
}
async getIdpSession(issuer: string): Promise<IdpSessionState | undefined> {
@ -272,10 +318,14 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearIdpSession(issuer: string): void {
this.store.getState().clearIdpSession(issuer);
this.clearAfterHydration(() =>
this.store.getState().clearIdpSession(issuer),
);
}
clearEnterpriseManagedResourceServers(): void {
this.store.getState().clearEnterpriseManagedResourceServers();
this.clearAfterHydration(() =>
this.store.getState().clearEnterpriseManagedResourceServers(),
);
}
}

38
core/auth/store.ts
View file

@ -10,7 +10,10 @@ import type {
OAuthTokens,
OAuthMetadata,
} from "@modelcontextprotocol/sdk/shared/auth.js";
import type { IdpSessionState, OAuthClientRegistrationKind } from "./storage.js";
import type {
IdpSessionState,
OAuthClientRegistrationKind,
} from "./storage.js";
/**
* OAuth state for a single server
@ -92,21 +95,36 @@ export function createOAuthStore(
updates: Partial<ServerOAuthState>,
) => {
const key = normalizeServerUrl(serverUrl);
set((state) => ({
servers: {
...state.servers,
[key]: {
...state.servers[key],
...updates,
},
},
}));
set((state) => {
const servers = { ...state.servers };
// Mirror getServerState's canonical→raw fallback for writes: if no
// canonical entry exists yet but a pre-normalization blob lives
// under the raw key, migrate it onto the canonical key so this
// partial write MERGES onto the existing credential instead of
// shadowing it with a fresh canonical entry (which would orphan the
// raw-key blob's other fields — e.g. a token — behind the new
// canonical one that getServerState now prefers).
let base = servers[key];
if (base === undefined && servers[serverUrl] !== undefined) {
// Reaching here implies `serverUrl !== key`: if they were equal,
// `servers[serverUrl]` would equal the `servers[key]` we just
// found undefined, so the guard above could not have passed.
base = servers[serverUrl];
delete servers[serverUrl];
}
servers[key] = { ...base, ...updates };
return { servers };
});
},
clearServerState: (serverUrl: string) => {
const key = normalizeServerUrl(serverUrl);
set((state) => {
const rest = { ...state.servers };
delete rest[key];
// Also drop a pre-normalization blob under the raw key so a clear
// fully removes the credential rather than leaving an orphan that
// getServerState's raw-key fallback would keep surfacing.
if (serverUrl !== key) delete rest[serverUrl];
return { servers: rest };
});
},

9
core/storage/adapters/remote-storage.ts
View file

@ -72,6 +72,15 @@ export function createRemoteStorageAdapter(
// full-page navigation (the OAuth authorize redirect): without it the
// browser may abort the request mid-flight and the just-saved
// codeVerifier/clientInformation never reaches disk.
//
// Caveat: the fetch spec caps the *combined* body size of all in-flight
// keepalive requests at 64 KB. `value` here is the whole store blob
// (every server's tokens + metadata), so a user with a very large
// number of authorized servers could in theory exceed that and have the
// browser reject this write. That ceiling is well above realistic
// inspector usage; if it ever bites, the fix is per-server stores rather
// than dropping `keepalive` (which would reintroduce the redirect-abort
// bug this guards against). The catch below surfaces such a rejection.
const res = await fetchFn(url, {
method: "POST",
headers,