Web: migrate auth store to shared /store API (RemoteOAuthStorage parity with TUI/CLI)

[cliffhall]

Closes #1548

Wave 1 of #1579. Migrates the web client's OAuth storage from BrowserOAuthStorage (sessionStorage) to RemoteOAuthStorage, so it reads/writes the same ~/.mcp-inspector/storage/oauth.json the TUI/CLI use — giving web ⇄ TUI ⇄ CLI parity. Re-implemented deliberately from the reference in #1510, adapted to current v2/main (which has since gained EMA + connection-state).

What changed

Core (shared with TUI/CLI):

• OAuthStorageBase now drives a single async hydration and exposes ready() / getHydrationError(). Every read on the post-redirect callback path (getClientInformation, getTokens, getCodeVerifier, getServerMetadata, getIdpSession) and every save awaits hydration first, so a late hydration merge can't clobber a value written before it landed. getCodeVerifier and getServerMetadata become async (interface + BaseOAuthClientProvider + EMA idpOidc / connection-state callers updated to await).
• The store is created with skipHydration: true so the constructor's explicit rehydrate() is the sole hydration (no auto-hydration to race it).
• store.ts adds normalizeServerUrl so a token saved under https://Example.com/mcp is found when the CLI later asks for https://example.com/mcp/; getServerState falls back to the raw key for pre-normalization blobs.
• remote-storage adapter POSTs with keepalive: true (survives the OAuth authorize redirect), surfaces Zustand's otherwise-swallowed persist write failures, and emits richer read/write error messages.
• generateOAuthState throws if crypto.getRandomValues is unavailable instead of silently degrading to Math.random.
• NodeOAuthStorage honors MCP_INSPECTOR_OAUTH_STATE_PATH for isolated fixtures.

Web:

• New memoized lib/remoteOAuthStorage accessor (one instance per {baseUrl, authToken}) wires RemoteOAuthStorage into environmentFactory (the connection path), the EMA IdP hook, and the per-server "clear OAuth" action, so all three share one in-memory view of oauth.json. clearServerOAuthState now takes the injected storage instead of reaching for the sessionStorage singleton.

Tests

• New oauth-storage.test.ts (async-hydration gating, hydration-error fallback, normalizeServerUrl), keepalive + persist-failure adapter coverage, a getBrowserOAuthStorage singleton test, the generateOAuthState throw path, and a remoteOAuthStorage memoization test.
• Migrated the existing storage / provider / EMA / node-storage / adapter tests to the async signatures (awaiting saves before sync reads/clears; normalizeServerUrl for raw-key blob assertions; explicit rehydrate() where a raw store is inspected).

Validation

• clients/web: validate (2458 unit), test:coverage (3259 tests, per-file gate green, EXIT 0), test:integration (801) all pass.
• Root validate green for web / cli (110) / tui (237) / launcher (5) — core changes don't break the other clients.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 10s —— View job

---

Review: Web OAuth store migration to shared /store API (#1592)

• Read the diff vs origin/v2/main
• Review core auth changes
• Review remote-storage adapter changes
• Review web changes
• Review test coverage

Nicely done — the async-hydration gating, the skipHydration: true + single explicit rehydrate(), and the keepalive/persist-error surfacing are all the right calls, and the docstrings explaining why are excellent. Tests are thorough (async-gating, throw-path fallback, keepalive assertion, normalization, memoization). Migration paths and the async-signature ripple through providers/connection-state/idpOidc all look correct and the callers consistently await.

A few things worth considering — none are blockers, mostly asymmetries and edge cases:

1. clear* methods don't await hydration, but save* does (asymmetry)

core/auth/oauth-storage.ts — every getter and every save* now await this.hydrated (great, and the docstring explains the clobber risk). But the clear* family stays synchronous: clearTokens (L181), clearCodeVerifier (L199), clearScope (L223), clearServerMetadata (L243), clear (L249), clearIdpSession (L274).

A clear issued before hydration lands has the same race the saves are protected against, in reverse: the setState clears the key, then the pending rehydrate() merges the persisted (un-cleared) blob back on top, silently resurrecting the cleared credential. In the web flow this is usually masked because getRemoteOAuthStorage is memoized and the EMA useMemo kicks off hydration on mount, so by the time a user clicks "clear OAuth" it has settled — but a freshly-constructed instance (e.g. a CLI clear command) could hit it. Since OAuthStorage.clear* are declared sync in the interface, awaiting would be a signature change; at minimum it's worth a comment noting the reasoning, or gating the clears too. Fix this →

2. normalizeServerUrl fallback can orphan fields from a pre-normalization blob

core/auth/store.ts — getServerState reads servers[canonical] ?? servers[raw], but setServerState only ever writes the canonical key (spreading ...state.servers[key] = canonical). So if oauth.json already holds a blob under a non-canonical raw key (e.g. a token the CLI wrote under https://api.partner.dev with no slash), the first partial write from the new code (say saveCodeVerifier) creates a separate canonical entry, and getServerState then returns that canonical entry — shadowing the raw-key blob's other fields (the token) rather than merging them. Result: an existing credential becomes unreachable and the user re-auths. Migration-only and low-probability, but the fallback read doesn't actually protect a subsequent partial write. Consider migrating/merging the raw-key entry into the canonical key on first access.

3. keepalive: true has a 64 KB body cap

core/storage/adapters/remote-storage.ts L79 — keepalive is the right fix for surviving the authorize redirect, but the fetch spec caps total in-flight keepalive request bodies at 64 KB. oauth.json is the whole blob (all servers, tokens, metadata), so a user with many authorized servers could exceed that, and the browser rejects the request — precisely on the redirect-critical write this is meant to protect. Likely fine in practice, but worth a comment acknowledging the ceiling.

4. Memoization key omits fetchFn (benign, worth a note)

clients/web/src/lib/remoteOAuthStorage.ts — the cache key is ${baseUrl} ${authToken}. environmentFactory passes its own fetchFn; the EMA/clear call sites pass none (defaulting to webOAuthFetch). Whichever constructs first wins, so the fetchFn arg is effectively ignored on later calls. Both wrappers are functionally identical (globalThis.fetch), so no behavior difference — but a future caller passing a genuinely different fetchFn would silently get the wrong one. The cache is also never evicted; fine given the token is session-stable. I confirmed the baseUrl strings match between environmentFactory (L45) and getWebOAuthBaseUrl(), so the shared-instance goal does hold. 👍

5. Minor doc nit — getScope sync-safety rationale

core/auth/oauth-storage.ts L205-212 — the comment says scope is "always set via saveScope in the same session before it's read." On the post-redirect callback path the in-memory store is reset, so the value actually comes from hydration; it's safe there only because buildOAuthConnectionState (connection-state.ts L86-89) awaits getClientInformation/getServerMetadata first, which flushes hydration before the sync getScope at L88. Worth tightening the comment to reflect the real invariant (a prior awaited read guarantees hydration), since that's the thing a future refactor could break.

Everything else — the generateOAuthState throw-instead-of-degrade, the richer adapter error messages, MCP_INSPECTOR_OAUTH_STATE_PATH for fixtures, and the test migrations to async signatures — looks correct and well-covered.
· branch v2/1548-web-remote-oauth-storage

[cliffhall]

Thanks for the thorough review. Addressed all five in 8c10b8eb:

1. clear* hydration asymmetry — Fixed for real rather than just commented. Added a private clearAfterHydration(mutate) helper: if hydration has already landed it runs synchronously (common case, immediately observable), otherwise it defers the mutation until after hydration. Deferring is the correct fix because applying a clear to the still-empty pre-hydration store is doubly wrong — the pending rehydrate() merge resurrects it and (worse) the persist write of the near-empty store would clobber the whole on-disk blob (every other server's tokens) before hydration ever reads it. All six clear*/clearTokens/… go through the helper. Tests added for clear-before-hydration (not resurrected, sibling fields preserved) and the already-hydrated synchronous path.

2. normalizeServerUrl orphaned fields — setServerState now migrates a pre-normalization raw-key blob onto the canonical key on first partial write (mirrors getServerState's canonical→raw fallback), so a partial write merges onto the existing credential instead of shadowing it. clearServerState also drops the raw-key orphan now, so a clear fully removes it. Tests added for both.

3. keepalive 64 KB cap — Added a comment acknowledging the combined-body ceiling, noting it's well above realistic usage and that the fix (if ever hit) is per-server stores rather than dropping keepalive; the surrounding catch surfaces such a rejection.

4. Memo key omits fetchFn — Documented on getRemoteOAuthStorage: the key is intentionally {baseUrl, authToken} only, why that's safe today (all call sites pass a functionally-identical globalThis.fetch wrapper), and what a future caller with a genuinely different transport must do.

5. getScope sync-safety doc — Tightened to state the real invariant: safety comes from a preceding awaited read flushing hydration — saveScope pre-redirect, or buildOAuthConnectionState awaiting getClientInformation/getServerMetadata before the sync getScope on the post-redirect path — and flags that a refactor reading getScope without a preceding awaited storage read would break it.

clients/web validate + test:integration + the full coverage gate (per-file ≥90 all four dims) all green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 2s —— View job

---

Re-review: fixes from 8c10b8eb

• Verify Progress notifications #1 — clear* hydration gating (clearAfterHydration)
• Verify Request logging #2 — raw-key blob migration in setServerState/clearServerState
• Verify Completion requests #3 — keepalive 64 KB comment
• Verify SSE transport support #4 — memo-key doc
• Verify OAuth support #5 — getScope doc
• Scan for new issues introduced by the fixes

All five items from the prior review are addressed well. The clearAfterHydration helper (oauth-storage.ts:119) is the right call — deferring rather than commenting, and the docstring correctly captures the two failure modes (resurrection and near-empty-blob clobber). The setServerState migration (store.ts:107-114) mirrors getServerState's fallback so a partial write merges onto the raw-key blob instead of orphaning it, and clearServerState (store.ts:127) now drops the raw orphan too. The keepalive (remote-storage.ts:76-83), memo-key (remoteOAuthStorage.ts:36-43), and getScope (oauth-storage.ts:237-252) docs all state the real invariants precisely. Nice work.

One gap surfaced while verifying #1:

clearClientInformation was left out of the clearAfterHydration migration

core/auth/oauth-storage.ts:174-185 — every other clear* method now routes through clearAfterHydration, but clearClientInformation still calls setServerState synchronously:

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); // ← not gated
}

It carries the exact clobber risk the fix was written for: issued before hydration lands, the pre-hydration set() triggers a persist write of the near-empty store, potentially overwriting the whole on-disk blob (every other server's tokens) before rehydrate() reads it. This was likely just an oversight — the prior review enumerated the six methods it happened to spot (clearTokens/clearCodeVerifier/clearScope/clearServerMetadata/clear/clearIdpSession) and clearClientInformation wasn't on that list, so the changelog's "all six" is literally accurate but the family has a seventh member.

Practical severity is low today: clearClientInformation currently has no production caller — it's reachable only via the SDK's invalidateCredentials("client"), which BaseOAuthClientProvider does not implement (confirmed: no invalidateCredentials anywhere in core/), so it's exercised only by tests. But it's a latent inconsistency that a future caller (or a provider gaining invalidateCredentials) would trip over, and it breaks the "all clears are hydration-safe" invariant the rest of the fix establishes. Worth closing now while the reasoning is fresh.

Suggested fix — wrap the mutation the same way:

clearClientInformation(serverUrl: string, isPreregistered?: boolean): void {
  this.clearAfterHydration(() => {
    const updates: Partial<ServerOAuthState> = {};
    if (isPreregistered) {
      updates.preregisteredClientInformation = undefined;
    } else {
      updates.clientInformation = undefined;
      updates.clientRegistrationKind = undefined;
    }
    this.store.getState().setServerState(serverUrl, updates);
  });
}

Fix this → (add a clear-before-hydration test mirroring the existing clearTokens one).

Everything else checks out — no other unguarded synchronous store writes remain (all save* await hydration; only this one clear slipped through). Not a blocker.
· branch v2/1548-web-remote-oauth-storage

[cliffhall]

Good catch on the seventh clear-family member. Fixed in 4e149719: clearClientInformation now routes through clearAfterHydration like the rest, so a pre-hydration clear can no longer persist a near-empty store and clobber the whole on-disk oauth.json. Added a clear-before-hydration test that asserts a sibling token survives the deferred clear (proving other servers' blobs aren't wiped).

clients/web validate + test:integration green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 16s —— View job

---

Re-review: clearClientInformation fix (4e149719)

• Verify clearClientInformation now routes through clearAfterHydration
• Verify the new clear-before-hydration test
• Scan for any remaining unguarded synchronous store writes
• Post review

The seventh clear-family member is closed correctly. clearClientInformation (oauth-storage.ts:174-185) now wraps its mutation in clearAfterHydration, so a pre-hydration clear can no longer persist a near-empty store and clobber the whole on-disk oauth.json. The new test (oauth-storage.test.ts:175-195) is exactly the right assertion — it clears client info before release(), then proves the sibling token from the persisted blob survives (getTokens still returns {access_token: "t", …}), which is the real-world "other servers' credentials aren't wiped" guarantee, not just "the deferred clear ran."

I re-scanned every store write in oauth-storage.ts to confirm the invariant now holds with no gaps:

Write	Guard
saveClientInformation (L157), savePreregistered… (L168), saveTokens (L203), saveCodeVerifier (L226), saveScope (L260), saveIdpSession (L317)	await this.hydrated ✓
clearClientInformation (L183), clearTokens (L211), clearScope (L265), clear (L295), clearIdpSession (L322), clearEnterpriseManagedResourceServers (L327)	clearAfterHydration ✓

Every mutation is now hydration-safe — no unguarded synchronous writes remain. This resolves the last item; the PR looks good to merge from my side. 👍

One optional doc nit (non-blocking, pre-existing — not from this commit)

getClientRegistrationKind (oauth-storage.ts:144-149) is the other synchronous getter besides getScope, and it relies on the same "a preceding awaited read flushed hydration" invariant: its only caller, buildOAuthConnectionState (connection-state.ts:94), reads it after awaiting getClientInformation/getServerMetadata (L86-89). That's safe today, but unlike getScope (which you tightened with an excellent invariant docstring in 8c10b8eb) it carries no such note — so a future refactor reading it without a preceding awaited storage read would silently get pre-hydration undefined on the callback path. Worth a one-line comment mirroring the getScope one, but purely a nicety. Fix this →
· branch v2/1548-web-remote-oauth-storage

[cliffhall]

Thanks — glad the seventh clear member and the invariant docs all check out. Addressed the optional nit too in 588fc3f8: getClientRegistrationKind now carries a docstring mirroring getScope's, spelling out that it's safe only because buildOAuthConnectionState awaits getClientInformation/getServerMetadata before reading it (flushing hydration on the post-redirect path), and that a future refactor reading it without a preceding awaited storage read would get a pre-hydration undefined. Doc-only; validate + test:integration green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 38s —— View job

---

Re-review (588fc3f8)

• Read the diff vs origin/v2/main (current state, incl. 588fc3f8)
• Verify prior fixes are all in place
• Review core auth changes
• Review remote-storage adapter & web changes
• Scan for any remaining/new issues

All five items from the first round plus the clearClientInformation seventh-member fix and the getClientRegistrationKind doc are in place and correct. I re-scanned the whole surface once more for the invariants this PR establishes and everything holds:

Every store mutation is hydration-safe. Re-confirmed the table from last round against oauth-storage.ts at HEAD — all six save* (saveClientInformation L166, savePreregistered… L177, saveTokens L212, saveCodeVerifier L235, saveScope L269, saveIdpSession L326, saveServerMetadata L289) await this.hydrated, and all seven clear* route through clearAfterHydration (clearClientInformation L185, clearTokens L220, clearCodeVerifier L240, clearScope L274, clearServerMetadata L296, clear L304, clearIdpSession L331, clearEnterpriseManagedResourceServers L337). No unguarded synchronous write remains.

The two sync getters are the only unguarded reads, and both are documented + protected by the "preceding awaited read flushes hydration" invariant. getScope (L263) and getClientRegistrationKind (L154) both now carry the invariant docstring, and buildOAuthConnectionState (connection-state.ts L86–94) awaits getClientInformation/getServerMetadata before reading either. ✅

Async-signature ripple is complete and consistent. getCodeVerifier/getServerMetadata → Promise in the interface (storage.ts), BaseOAuthClientProvider.codeVerifier()/getServerMetadata() now async (providers.ts), and idpOidc.ts awaits both. I traced every .codeVerifier() caller: transportProvider.ts:101 forwards with the correct string | Promise<string> type (matching the SDK's OAuthClientProvider), and remote-tokenAuthProvider (the sync undefined-returning one) is a different provider left untouched. No un-awaited call slipped through.

Web wiring shares one instance. getRemoteOAuthStorage(getWebOAuthBaseUrl(), getAuthToken()) is used identically at all three call sites (environmentFactory.ts:71, App.tsx EMA memo + clear-OAuth), and the baseUrl strings match, so the memo genuinely returns one shared in-memory view. 👍

This all looks good to merge from my side. Nothing blocking. One micro-observation for the record, not worth acting on:

(nit, informational only) setServerState migration leaves a raw orphan when both keys already exist

core/auth/store.ts:107-115 — the migration only fires when servers[key] === undefined. In the (essentially impossible) case where a canonical entry and a stale raw-key entry coexist, a partial write updates canonical and leaves the raw blob as dead data on disk. It's harmless — getServerState prefers canonical (L90) so the orphan is shadowed, and clearServerState deletes both (L127) — and after the first partial write the raw key is already migrated away, so the coexistence window doesn't arise in normal operation. Genuinely not worth code; noting only so the asymmetry with getServerState/clearServerState (both of which handle both keys) is on record.

Everything else — keepalive + its 64 KB caveat, the persist-failure surfacing, generateOAuthState's throw-instead-of-degrade, MCP_INSPECTOR_OAUTH_STATE_PATH, and the async-migrated tests — is correct and well-covered. Nice, thorough iteration.
· branch v2/1548-web-remote-oauth-storage

[cliffhall]

📋 Smoke test & AGENTS.md audit for this PR: #1548 (comment) (posted on the linked issue #1548).

[BobDickinson]

My plan was to remove Zustand at the same time, which should eliminate all of the hydration gymnastics entirely (with just a nice clean async read/write).