merge: v2/1548-web-remote-oauth-storage (#1592)

# Conflicts:
#	clients/web/src/test/core/auth/utils.test.ts
This commit is contained in:
cliffhall 2026-07-01 22:05:52 -04:00
commit e425602295
8 changed files with 988 additions and 151 deletions

View file

@ -86,7 +86,7 @@ export async function buildOAuthConnectionState(
const preregistered = await storage.getClientInformation(serverUrl, true);
const dynamic = await storage.getClientInformation(serverUrl);
const storageScope = storage.getScope(serverUrl);
const serverMetadata = storage.getServerMetadata(serverUrl);
const serverMetadata = await storage.getServerMetadata(serverUrl);
const authorized = isAccessTokenUsable(tokens);
const grantedScope = resolveGrantedScope(tokens, storageScope);
@ -120,7 +120,9 @@ export async function buildOAuthConnectionState(
const idp = params.enterpriseManagedAuth.idp;
const issuer = normalizeIdpIssuer(idp.issuer);
const idpSession = await getEmaIdpLoginState(storage, issuer);
const idpMetadata = storage.getServerMetadata(idpOAuthStorageKey(issuer));
const idpMetadata = await storage.getServerMetadata(
idpOAuthStorageKey(issuer),
);
state.ema = {
idpIssuer: issuer,
idpClientId: idp.clientId,

View file

@ -35,7 +35,7 @@ async function resolveIdpMetadata(
fetchFn?: typeof fetch,
): Promise<OAuthMetadata> {
const storageKey = idpOAuthStorageKey(issuer);
const cached = storage.getServerMetadata(storageKey);
const cached = await storage.getServerMetadata(storageKey);
if (cached?.token_endpoint) {
return cached;
}
@ -101,7 +101,7 @@ export async function completeIdpOidcAuthorization(params: {
}> {
const issuer = normalizeIdpIssuer(params.idp.issuer);
const storageKey = idpOAuthStorageKey(issuer);
const metadata = params.storage.getServerMetadata(storageKey);
const metadata = await params.storage.getServerMetadata(storageKey);
if (!metadata) {
throw new Error("IdP OAuth metadata not found — restart EMA IdP login");
}
@ -111,7 +111,7 @@ export async function completeIdpOidcAuthorization(params: {
if (!clientInformation) {
throw new Error("IdP client information not found — restart EMA IdP login");
}
const codeVerifier = params.storage.getCodeVerifier(storageKey);
const codeVerifier = await params.storage.getCodeVerifier(storageKey);
if (!codeVerifier) {
throw new Error("IdP PKCE verifier not found — restart EMA IdP login");
}

View file

@ -10,11 +10,17 @@ import {
const DEFAULT_STATE_PATH = getStoreFilePath(getDefaultStorageDir(), "oauth");
/**
* Get path to OAuth state file.
* @param customPath - Optional custom path (full path to state file). Default: ~/.mcp-inspector/storage/oauth.json
* Get path to OAuth state file. Resolution order: explicit `customPath`, then
* the `MCP_INSPECTOR_OAUTH_STATE_PATH` environment variable (so tests and
* scripted runs can point at an isolated fixture without touching
* `~/.mcp-inspector`), then the default `~/.mcp-inspector/storage/oauth.json`.
*/
export function getStateFilePath(customPath?: string): string {
return customPath ?? DEFAULT_STATE_PATH;
return (
customPath ??
process.env.MCP_INSPECTOR_OAUTH_STATE_PATH ??
DEFAULT_STATE_PATH
);
}
const storeCache = new Map<string, ReturnType<typeof createOAuthStore>>();

View file

@ -20,18 +20,115 @@ import type {
* Concrete OAuthStorage implementation parameterized on a Zustand store.
* The store carries the storage adapter (sessionStorage, file, remote HTTP, …),
* so the same body works for browser, Node, and remote environments.
*
* With an async storage adapter (file, remote HTTP) the store is empty until
* hydration completes. Every read used on the post-OAuth-redirect callback path
* (`getCodeVerifier`, `getServerMetadata`, `getClientInformation`, `getTokens`)
* — and every save — therefore awaits {@link ready} first; saves wait so a
* late hydration merge cannot clobber a value written before it landed. The
* underlying store is created with `skipHydration: true`
* ({@link createOAuthStore}), so the constructor's `rehydrate()` is the single
* hydration and there is no auto-hydration to race it. With a synchronous
* adapter (sessionStorage) hydration resolves on the next microtask and the
* behaviour is effectively unchanged.
*/
export class OAuthStorageBase implements OAuthStorage {
private readonly store: ReturnType<typeof createOAuthStore>;
private readonly hydrated: Promise<void>;
private hydrationError?: Error;
constructor(store: ReturnType<typeof createOAuthStore>) {
this.store = store;
// The store is created with `skipHydration: true` so this is the SOLE
// hydration. Driving it explicitly (rather than relying on persist's
// auto-hydration + onFinishHydration) lets us catch a rejecting adapter —
// onFinishHydration only fires on success, so a throwing getItem would
// otherwise leave `hydrated` pending forever and hang every getter.
// A failed hydration resolves to "empty store", which is the correct
// fallback (reads return undefined; writes proceed) — but the failure is
// recorded so callers can distinguish "no token stored" from "could not
// read the store" (e.g. corrupt file, EACCES, backend 500).
const recordFailure = (err: unknown) => {
this.hydrationError =
err instanceof Error
? err
: new Error(
typeof err === "string"
? err
: "OAuth storage hydration failed (adapter getItem threw or returned invalid data)",
);
console.warn(
"[OAuthStorage] hydration failed; continuing with empty state:",
this.hydrationError,
);
};
this.hydrated = this.store.persist.hasHydrated()
? Promise.resolve()
: Promise.resolve(this.store.persist.rehydrate()).then(
() => {
// Zustand swallows getItem errors internally (routing them to
// onRehydrateStorage) and resolves rehydrate() regardless;
// hasHydrated() only flips true on success, so use it to detect a
// silently-failed hydration. The underlying error is lost in this
// path — record a generic one so getHydrationError() is non-null.
if (!this.store.persist.hasHydrated()) recordFailure(undefined);
},
(err: unknown) => recordFailure(err),
);
}
/**
* If hydration failed (adapter threw, file unreadable/corrupt, backend
* non-2xx), the captured error. Undefined when hydration succeeded or has
* not yet completed — call after `await ready()`. Lets callers report
* "oauth.json is unreadable" instead of a misleading "no token stored".
*/
getHydrationError(): Error | undefined {
return this.hydrationError;
}
/**
* Resolves once the underlying persist adapter has hydrated the store.
* Callers that need to read state outside the typed getters can await this
* directly (e.g. before reading the store via `getState()` for diagnostics).
*/
ready(): Promise<void> {
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,
): Promise<OAuthClientInformation | undefined> {
await this.hydrated;
const state = this.store.getState().getServerState(serverUrl);
const clientInfo = isPreregistered
? state.preregisteredClientInformation
@ -44,6 +141,16 @@ export class OAuthStorageBase implements OAuthStorage {
return await OAuthClientInformationSchema.parseAsync(clientInfo);
}
/**
* Intentionally synchronous — same invariant as {@link getScope}: safe
* without awaiting hydration only because a preceding awaited read has
* already flushed it. Its sole caller, `buildOAuthConnectionState`
* (`connection-state.ts`), reads this right after awaiting
* `getClientInformation`/`getServerMetadata`, so on the post-redirect
* callback path (where the in-memory store is reset) hydration has landed
* by the time this runs. A future refactor reading it without a preceding
* awaited storage read would get a pre-hydration `undefined`.
*/
getClientRegistrationKind(
serverUrl: string,
): OAuthClientRegistrationKind | undefined {
@ -56,6 +163,7 @@ export class OAuthStorageBase implements OAuthStorage {
clientInformation: OAuthClientInformation,
options: SaveClientInformationOptions,
): Promise<void> {
await this.hydrated;
this.store.getState().setServerState(serverUrl, {
clientInformation,
clientRegistrationKind: options.registrationKind,
@ -66,6 +174,7 @@ export class OAuthStorageBase implements OAuthStorage {
serverUrl: string,
clientInformation: OAuthClientInformation,
): Promise<void> {
await this.hydrated;
this.store.getState().setServerState(serverUrl, {
preregisteredClientInformation: clientInformation,
clientRegistrationKind: "static",
@ -73,19 +182,20 @@ export class OAuthStorageBase implements OAuthStorage {
}
clearClientInformation(serverUrl: string, isPreregistered?: boolean): void {
const updates: Partial<ServerOAuthState> = {};
if (isPreregistered) {
updates.preregisteredClientInformation = undefined;
} else {
updates.clientInformation = undefined;
updates.clientRegistrationKind = undefined;
}
this.store.getState().setServerState(serverUrl, updates);
this.clearAfterHydration(() => {
const updates: Partial<ServerOAuthState> = {};
if (isPreregistered) {
updates.preregisteredClientInformation = undefined;
} else {
updates.clientInformation = undefined;
updates.clientRegistrationKind = undefined;
}
this.store.getState().setServerState(serverUrl, updates);
});
}
async getTokens(serverUrl: string): Promise<OAuthTokens | undefined> {
await this.hydrated;
const state = this.store.getState().getServerState(serverUrl);
if (!state.tokens) {
return undefined;
@ -99,6 +209,7 @@ export class OAuthStorageBase implements OAuthStorage {
tokens: OAuthTokens,
options?: SaveTokensOptions,
): Promise<void> {
await this.hydrated;
this.store.getState().setServerState(serverUrl, {
tokens,
...(options?.enterpriseManaged === true && { enterpriseManaged: true }),
@ -106,10 +217,13 @@ 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 }),
);
}
getCodeVerifier(serverUrl: string): string | undefined {
async getCodeVerifier(serverUrl: string): Promise<string | undefined> {
await this.hydrated;
const state = this.store.getState().getServerState(serverUrl);
return state.codeVerifier;
}
@ -118,29 +232,52 @@ export class OAuthStorageBase implements OAuthStorage {
serverUrl: string,
codeVerifier: string,
): Promise<void> {
await this.hydrated;
this.store.getState().setServerState(serverUrl, { codeVerifier });
}
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). 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);
return state.scope;
}
async saveScope(serverUrl: string, scope: string | undefined): Promise<void> {
await this.hydrated;
this.store.getState().setServerState(serverUrl, { scope });
}
clearScope(serverUrl: string): void {
this.store.getState().setServerState(serverUrl, { scope: undefined });
this.clearAfterHydration(() =>
this.store.getState().setServerState(serverUrl, { scope: undefined }),
);
}
getServerMetadata(serverUrl: string): OAuthMetadata | null {
async getServerMetadata(serverUrl: string): Promise<OAuthMetadata | null> {
await this.hydrated;
const state = this.store.getState().getServerState(serverUrl);
return state.serverMetadata || null;
}
@ -149,22 +286,28 @@ export class OAuthStorageBase implements OAuthStorage {
serverUrl: string,
metadata: OAuthMetadata,
): Promise<void> {
await this.hydrated;
this.store
.getState()
.setServerState(serverUrl, { serverMetadata: metadata });
}
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> {
await this.hydrated;
const session = this.store.getState().getIdpSession(issuer);
if (
!session.idToken &&
@ -180,14 +323,19 @@ export class OAuthStorageBase implements OAuthStorage {
issuer: string,
session: Partial<IdpSessionState>,
): Promise<void> {
await this.hydrated;
this.store.getState().setIdpSession(issuer, session);
}
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(),
);
}
}

View file

@ -234,8 +234,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
await this.storage.saveCodeVerifier(this.serverUrl, codeVerifier);
}
codeVerifier(): string {
const verifier = this.storage.getCodeVerifier(this.serverUrl);
async codeVerifier(): Promise<string> {
const verifier = await this.storage.getCodeVerifier(this.serverUrl);
if (!verifier) {
throw new Error("No code verifier saved for session");
}
@ -246,8 +246,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
this.storage.clear(this.serverUrl);
}
getServerMetadata(): OAuthMetadata | null {
return this.storage.getServerMetadata(this.serverUrl);
async getServerMetadata(): Promise<OAuthMetadata | null> {
return await this.storage.getServerMetadata(this.serverUrl);
}
async saveServerMetadata(metadata: OAuthMetadata): Promise<void> {

View file

@ -21,6 +21,12 @@ export interface SaveClientInformationOptions {
}
export interface OAuthStorage {
/**
* Resolves once any async hydration of the underlying store has completed.
* With a synchronous storage adapter this resolves immediately.
*/
ready(): Promise<void>;
/**
* Get client information (preregistered or dynamically registered)
*/
@ -78,9 +84,13 @@ export interface OAuthStorage {
clearTokens(serverUrl: string): void;
/**
* Get code verifier (for PKCE)
* Get code verifier (for PKCE).
*
* Async because the backend-backed store may still be hydrating when this is
* read on the post-redirect OAuth callback path; the implementation awaits
* hydration before reading.
*/
getCodeVerifier(serverUrl: string): string | undefined;
getCodeVerifier(serverUrl: string): Promise<string | undefined>;
/**
* Save code verifier (for PKCE)
@ -108,9 +118,13 @@ export interface OAuthStorage {
clearScope(serverUrl: string): void;
/**
* Get server metadata discovered during OAuth
* Get server metadata discovered during OAuth.
*
* Async because the backend-backed store may still be hydrating when this is
* read on the post-redirect OAuth callback path; the implementation awaits
* hydration before reading.
*/
getServerMetadata(serverUrl: string): OAuthMetadata | null;
getServerMetadata(serverUrl: string): Promise<OAuthMetadata | null>;
/**
* Save server metadata discovered during OAuth

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
@ -28,6 +31,26 @@ export interface ServerOAuthState {
enterpriseManaged?: boolean;
}
/**
* Normalize a server URL to its canonical form for use as a store key.
*
* Different callers (web deep-link parser, CLI `--server-url`, the SDK's own
* client constructor) can present the same endpoint with cosmetic differences
* — host case, a trailing slash on a bare-origin URL, default-port omission —
* that `new URL().href` collapses. Keying by the canonical form means a token
* the web inspector saved under `https://Example.com/mcp` is found when the
* CLI later asks for `https://example.com/mcp/`. Non-URL strings (e.g. a
* stdio server name) are returned trimmed so the store still works for them.
*/
export function normalizeServerUrl(serverUrl: string): string {
const trimmed = serverUrl.trim();
try {
return new URL(trimmed).href;
} catch {
return trimmed;
}
}
/**
* Zustand store state (all servers)
*/
@ -59,26 +82,49 @@ export function createOAuthStore(
servers: {},
idpSessions: {},
getServerState: (serverUrl: string) => {
return get().servers[serverUrl] || {};
const servers = get().servers;
// Look up by canonical key, falling back to the raw key so an
// already-persisted blob written before normalization existed (or by
// another writer) is still found.
return (
servers[normalizeServerUrl(serverUrl)] ?? servers[serverUrl] ?? {}
);
},
setServerState: (
serverUrl: string,
updates: Partial<ServerOAuthState>,
) => {
set((state) => ({
servers: {
...state.servers,
[serverUrl]: {
...state.servers[serverUrl],
...updates,
},
},
}));
const key = normalizeServerUrl(serverUrl);
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[serverUrl];
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 };
});
},
@ -118,6 +164,11 @@ export function createOAuthStore(
{
name: "mcp-inspector-oauth",
storage,
// OAuthStorageBase drives `persist.rehydrate()` itself so it can await
// (and catch errors from) the single hydration. Auto-hydration here
// would race the explicit one and could clobber a save that landed
// between them.
skipHydration: true,
},
),
);

View file

@ -42,7 +42,9 @@ export function createRemoteStorageAdapter(
if (res.status === 404) {
return null;
}
throw new Error(`Failed to read store: ${res.status}`);
throw new Error(
`Failed to read store '${options.storeId}' from ${baseUrl}/api/storage/${options.storeId}: ${res.status}`,
);
}
const store = await res.json();
@ -62,16 +64,41 @@ export function createRemoteStorageAdapter(
headers["x-mcp-remote-auth"] = `Bearer ${options.authToken}`;
}
// Zustand gives us the full persisted format as a string
// Store it as-is (the API treats it as an opaque blob)
const res = await fetchFn(`${baseUrl}/api/storage/${options.storeId}`, {
method: "POST",
headers,
body: value, // Already a JSON string from Zustand
});
const url = `${baseUrl}/api/storage/${options.storeId}`;
try {
// Zustand gives us the full persisted format as a string
// Store it as-is (the API treats it as an opaque blob).
// `keepalive` lets this POST outlive an immediately-following
// 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,
body: value, // Already a JSON string from Zustand
keepalive: true,
});
if (!res.ok) {
throw new Error(`Failed to write store: ${res.status}`);
if (!res.ok) {
throw new Error(
`Failed to write store '${options.storeId}' to ${url}: ${res.status}`,
);
}
} catch (err) {
// Zustand's persist middleware swallows setItem rejections — without
// this the user sees a green "Connected" while the token never landed
// on disk. Surface the failure so it's at least observable.
console.error("[remote-storage] persist write failed:", err);
throw err;
}
},
removeItem: async (_name: string) => {