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

fix(web): per-attempt snapshot ids, honest recovery copy, connect teardown

Browse source 
Copilot round 3, four items:

- the snapshot "attempt token" was the stored serialization, which is not
  unique: the snapshot records neither the authorization URL nor its
  `state`, so two concurrent redirects for one server with unchanged UI
  serialize identically. `OAuthResumeSnapshot` now carries an optional
  `attemptId`, stamped per write and matched on clear. Optional because
  an older build's snapshot, mid-redirect across an upgrade, has none —
  and treating that as a match would strand a live callback.
- the resume-failure toast promised a retry even when the session had
  ended and nothing was restored. Copy is now chosen by whether anything
  is left pending; `authRecoveryAbandonedMessage` covers the other case.
- a connect-scoped redirect that failed left the connection at
  "connecting" with the active-server lock held, since the redirect was
  what would have ended the attempt. It now disconnects first, as the
  sibling arm in `useConnectionLifecycle` does — scoped to that context,
  because an ambient or step-up failure does not mean the session is dead.
- both terminal EMA step-up failure arms now drop the stored retry. A
  later step-up does not overwrite the ref, so a retained operation could
  be re-run by an unrelated authorization. The `interactive` arm keeps
  it: that one is still on its way to a redirect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-29 02:32:05 -04:00
commit a46a8a6cf8
7 changed files with 287 additions and 38 deletions
Download patch file Download diff file Expand all files Collapse all files

100
clients/web/src/hooks/useOAuthRecovery.test.tsx
View file

@ -595,6 +595,47 @@ describe("useOAuthRecovery", () => {
expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull();
});
it("ends a failed connect-scoped attempt instead of leaving it spinning", async () => {
// `connect()` rejecting with an auth-recovery error holds the status at
// "connecting"; the redirect was what would have ended that attempt, so
// if it never happens nothing else does (#2165).
const client = fakeClient({
beginInteractiveAuthorization: vi
.fn()
.mockRejectedValue(new Error("provider state unreadable")),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await act(async () => {
h.api().prepareOAuthRedirect({
serverId: "a",
authKind: "reauth",
authorizationUrl: AUTH_URL,
preRedirectContext: "connect",
});
});
await waitFor(() => expect(client.disconnect).toHaveBeenCalled());
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
});
it("leaves a non-connect attempt's session alone", async () => {
const client = fakeClient({
beginInteractiveAuthorization: vi
.fn()
.mockRejectedValue(new Error("provider state unreadable")),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await act(async () => {
h.api().prepareOAuthRedirect({
serverId: "a",
authKind: "step_up",
authorizationUrl: AUTH_URL,
});
});
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(client.disconnect).not.toHaveBeenCalled();
});
it("tolerates having no client at all", () => {
const h = harness({ servers: [entry("a")], client: null });
act(() =>
@ -1172,6 +1213,10 @@ describe("useOAuthRecovery", () => {
expect(toastTitles()).toContain("Could not continue authorization"),
);
// …and says so, rather than promising a retry it will not make.
expect(toastWith("the session it belonged to has ended")).toBeTruthy();
expect(toastWith("will try again")).toBeUndefined();
// Nothing was restored, so returning to the tab resumes nothing.
await act(async () => {
becomeVisible();
@ -1870,6 +1915,61 @@ describe("useOAuthRecovery", () => {
);
});
it("drops the stored retry when an EMA step-up fails", async () => {
// The prompt is already dismissed and a later step-up does not overwrite
// the ref, so a retained operation would be re-run by an unrelated
// authorization (#2165).
const handleAuthChallenge = vi
.fn()
.mockResolvedValueOnce({ kind: "failed", error: new Error("denied") })
.mockResolvedValue({ kind: "satisfied" });
const client = fakeClient({ handleAuthChallenge });
const h = harness({
servers: [entry("a", {}, true)],
activeServerId: "a",
client,
});
const retry = vi.fn().mockResolvedValue(undefined);
await openStepUp(h, "tool", retry);
await act(async () => {
await h.api().handleStepUpAuthorize();
});
expect(retry).not.toHaveBeenCalled();
// A later, unrelated EMA step-up succeeds — and must not re-run it.
await openStepUp(h, "tool");
await act(async () => {
await h.api().handleStepUpAuthorize();
});
expect(toastTitles()).toContain("Permissions updated");
expect(retry).not.toHaveBeenCalled();
});
it("drops the stored retry when an EMA step-up rejects", async () => {
const handleAuthChallenge = vi
.fn()
.mockRejectedValueOnce(new Error("IdP unreachable"))
.mockResolvedValue({ kind: "satisfied" });
const client = fakeClient({ handleAuthChallenge });
const h = harness({
servers: [entry("a", {}, true)],
activeServerId: "a",
client,
});
const retry = vi.fn().mockResolvedValue(undefined);
await openStepUp(h, "tool", retry);
await act(async () => {
await h.api().handleStepUpAuthorize();
});
await openStepUp(h, "tool");
await act(async () => {
await h.api().handleStepUpAuthorize();
});
expect(toastTitles()).toContain("Permissions updated");
expect(retry).not.toHaveBeenCalled();
});
it("cancels back to the panel that asked", async () => {
const client = fakeClient();
const h = harness({ servers: [entry("a")], activeServerId: "a", client });

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

@ -56,6 +56,7 @@ import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs";
import type { PendingReauth } from "../utils/pendingReauth";
import {
authRecoveryRestoredMessage,
authRecoveryAbandonedMessage,
authRecoveryRetryFailedMessage,
isReAuthBannerReason,
issuerBindingFailureCopy,
@ -556,8 +557,18 @@ export function useOAuthRecovery({
// in flight.
oauthClient
?.beginInteractiveAuthorization(authorizationUrl)
.catch((err: unknown) => {
.catch(async (err: unknown) => {
clearOwnOAuthResumeSnapshot(snapshotToken);
// A connect-scoped redirect is the tail of a `connect()` that
// rejected with an `AuthRecoveryRequiredError`, which deliberately
// holds the status at `"connecting"` rather than moving it to
// `"error"`. The redirect was the thing that would have ended that
// attempt, so with it gone nothing else does: the toggle spins and
// the active-server lock is held. Tear it down first, exactly as the
// sibling arm in `useConnectionLifecycle` does for the same reason.
if (preRedirectContext === "connect") {
await oauthClient?.disconnect().catch(() => {});
}
setFailedServerId(serverId);
showReAuthBanner(serverId, err);
});
@ -872,17 +883,23 @@ export function useOAuthRecovery({
// is now. The restore is a functional update so it sees the queued
// state rather than the render-time value, and yields to whatever is
// already there — latest wins.
if (
const stillThisSession =
sessionRef.current.activeServerId === pending.serverId &&
sessionRef.current.inspectorClient === client
) {
sessionRef.current.inspectorClient === client;
if (stillThisSession) {
setPendingReauth((prev) => prev ?? pending);
}
const detail = err instanceof Error ? err.message : String(err);
notifications.show({
title: "Could not continue authorization",
message: authRecoveryRetryFailedMessage(
err instanceof Error ? err.message : String(err),
),
// The copy has to match what actually happens. When the session is
// gone nothing was restored and no retry is coming, so promising one
// would leave the user waiting on it. When it survives, something is
// pending either way — this attempt's challenge, or the newer one
// the functional update yielded to.
message: stillThisSession
? authRecoveryRetryFailedMessage(detail)
: authRecoveryAbandonedMessage(detail),
color: "yellow",
autoClose: 6000,
});
@ -1493,6 +1510,13 @@ export function useOAuthRecovery({
return;
}
if (outcome.kind === "failed") {
// Terminal: drop the stored retry with it. The prompt is already
// dismissed, so nothing will ever consume this operation — and a
// *later* step-up does not overwrite the ref, so leaving it would
// let that unrelated authorization silently re-run this command
// (#2165). The `interactive` arm above deliberately keeps it: that
// one is still on its way to a redirect.
pendingStepUpRetryRef.current = null;
const failureMessage = emaStepUpFailureMessage(outcome.error.message);
notifications.show({
title: "Organization permissions",
@ -1507,7 +1531,9 @@ export function useOAuthRecovery({
// rejection escaping here reaches nobody: the `finally` resets the
// latch and the prompt is already dismissed, leaving the panel that
// asked for the permissions never told (#2165). Reported exactly as
// the `failed` outcome above is, so the two cannot disagree.
// the `failed` outcome above is, so the two cannot disagree — and it
// drops the stored retry for the same reason that arm does.
pendingStepUpRetryRef.current = null;
const failureMessage = emaStepUpFailureMessage(
err instanceof Error ? err.message : String(err),
);

98
clients/web/src/lib/oauthResume.test.ts
View file

@ -53,8 +53,10 @@ describe("oauthResume", () => {
authKind: "reauth",
tabUi: {},
};
writeOAuthResumeSnapshot(snapshot);
expect(consumeOAuthResumeSnapshot()).toEqual(snapshot);
const attemptId = writeOAuthResumeSnapshot(snapshot);
// The write stamps the per-attempt id `clearOwnOAuthResumeSnapshot`
// matches on (#2165); everything else round-trips unchanged.
expect(consumeOAuthResumeSnapshot()).toEqual({ ...snapshot, attemptId });
expect(readOAuthResumeSnapshot()).toBeUndefined();
expect(consumeOAuthResumeSnapshot()).toBeUndefined();
});
@ -87,8 +89,8 @@ describe("oauthResume", () => {
},
remoteSessionId: "remote-abc",
};
writeOAuthResumeSnapshot(snapshot);
expect(readOAuthResumeSnapshot()).toEqual(snapshot);
const attemptId = writeOAuthResumeSnapshot(snapshot);
expect(readOAuthResumeSnapshot()).toEqual({ ...snapshot, attemptId });
expect(storage.get(OAUTH_PENDING_SERVER_KEY)).toBeUndefined();
clearOAuthResumeSnapshot();
expect(readOAuthResumeSnapshot()).toBeUndefined();
@ -350,31 +352,47 @@ describe("oauthResume", () => {
}
});
it("clearOwnOAuthResumeSnapshot removes only the snapshot it was given", () => {
const first = writeOAuthResumeSnapshot({
it("clearOwnOAuthResumeSnapshot removes only the attempt it was given", () => {
// Deliberately IDENTICAL snapshots: two concurrent redirects for the same
// server with the same shell state serialize the same way while carrying
// different authorization URLs, which the snapshot does not record. A byte
// comparison would call them one attempt and delete the wrong one (#2165).
const snapshot = {
version: 1,
serverId: "a",
activeTab: "tools",
authKind: "reauth",
tabUi: {},
});
// A later attempt replaces it before the first one's failure lands.
writeOAuthResumeSnapshot({
version: 1,
serverId: "b",
activeTab: "prompts",
authKind: "reauth",
tabUi: {},
});
} as const;
const first = writeOAuthResumeSnapshot({ ...snapshot });
const second = writeOAuthResumeSnapshot({ ...snapshot });
expect(first).toBeTruthy();
expect(second).not.toBe(first);
expect(clearOwnOAuthResumeSnapshot(first)).toBe(false);
expect(readOAuthResumeSnapshot()?.serverId).toBe("b");
expect(readOAuthResumeSnapshot()?.attemptId).toBe(second);
const second = window.sessionStorage.getItem(OAUTH_RESUME_KEY)!;
expect(clearOwnOAuthResumeSnapshot(second)).toBe(true);
expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull();
});
it("clearOwnOAuthResumeSnapshot ignores a snapshot with no attemptId", () => {
// An older build's snapshot, mid-redirect across an upgrade — dropping it
// would strand a live callback.
window.sessionStorage.setItem(
OAUTH_RESUME_KEY,
JSON.stringify({
version: 1,
serverId: "a",
activeTab: "tools",
authKind: "reauth",
tabUi: {},
}),
);
expect(clearOwnOAuthResumeSnapshot("some-attempt")).toBe(false);
expect(readOAuthResumeSnapshot()?.serverId).toBe("a");
});
it("clearOwnOAuthResumeSnapshot is a no-op without a token", () => {
writeOAuthResumeSnapshot({
version: 1,
@ -414,7 +432,51 @@ describe("oauthResume", () => {
setItem: () => {},
removeItem: () => {},
});
expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false);
expect(clearOwnOAuthResumeSnapshot("attempt")).toBe(false);
});
it("clearOwnOAuthResumeSnapshot is a no-op with nothing stored", () => {
expect(clearOwnOAuthResumeSnapshot("attempt")).toBe(false);
});
it("writeOAuthResumeSnapshot mints an attemptId per write", () => {
const token = writeOAuthResumeSnapshot({
version: 1,
serverId: "a",
activeTab: "tools",
authKind: "reauth",
tabUi: {},
});
expect(token).toEqual(expect.any(String));
expect(readOAuthResumeSnapshot()?.attemptId).toBe(token);
});
it("writeOAuthResumeSnapshot falls back when randomUUID is unavailable", () => {
// `crypto.randomUUID` needs a secure context, which a plain-HTTP
// non-loopback host is not.
const original = Object.getOwnPropertyDescriptor(
globalThis.crypto,
"randomUUID",
);
Object.defineProperty(globalThis.crypto, "randomUUID", {
configurable: true,
value: undefined,
});
try {
const token = writeOAuthResumeSnapshot({
version: 1,
serverId: "a",
activeTab: "tools",
authKind: "reauth",
tabUi: {},
});
expect(token).toEqual(expect.any(String));
expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
} finally {
if (original) {
Object.defineProperty(globalThis.crypto, "randomUUID", original);
}
}
});
it("clearOAuthResumeSnapshot swallows removeItem failures", () => {

60
clients/web/src/lib/oauthResume.ts
View file

@ -57,6 +57,19 @@ export interface OAuthResumeSnapshot {
authChallenge?: AuthChallenge;
/** Command-scoped recovery source when redirect was triggered by a user action. */
recoverySource?: OAuthRecoverySource;
/**
* Identifies the redirect attempt that wrote this snapshot (#2165).
*
* Stamped by {@link writeOAuthResumeSnapshot} and matched by
* {@link clearOwnOAuthResumeSnapshot}, so an attempt that fails before
* navigating cannot delete a *later* attempt's snapshot. Nothing else reads
* it: the callback identifies its server by `serverId`, as before.
*
* Optional because a snapshot written by an older build (across a redirect
* that spans an upgrade) has none, and dropping such a snapshot would strand
* a live callback.
*/
attemptId?: string;
}
export interface LiftedTabUiState {
@ -181,16 +194,33 @@ export function writeOAuthResumeSnapshot(
if (typeof window === "undefined") {
return undefined;
}
const serialized = JSON.stringify(snapshot);
const attemptId = snapshot.attemptId ?? newAttemptId();
try {
window.sessionStorage.setItem(OAUTH_RESUME_KEY, serialized);
return serialized;
window.sessionStorage.setItem(
OAUTH_RESUME_KEY,
JSON.stringify({ ...snapshot, attemptId }),
);
return attemptId;
} catch {
// Best-effort — privacy mode / quota.
return undefined;
}
}
/**
* A per-attempt identifier. `randomUUID` where it exists (it needs a secure
* context, which a `file://` page or a plain-HTTP non-loopback host is not),
* and otherwise a value that only has to be unique among the handful of
* redirect attempts one page can have in flight — never a security token.
*/
function newAttemptId(): string {
const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto);
if (uuid) {
return uuid();
}
return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`;
}
export function readOAuthResumeSnapshot(): OAuthResumeSnapshot | undefined {
if (typeof window === "undefined") {
return undefined;
@ -249,24 +279,30 @@ function readLegacyPendingServerSnapshot(): OAuthResumeSnapshot | undefined {
* `prepareOAuthRedirect` has no single-flight guard, so a redirect that fails
* before navigating can reject after a *later* attempt has already written its
* own snapshot. An unconditional clear there would delete the newer attempt's
* callback-routing state, which is a worse failure than the one it is trying
* to avoid.
* callback-routing state — a worse failure than the stale snapshot it is
* trying to avoid, since that redirect is actually in flight.
*
* The token is the stored serialization, which makes two byte-identical
* snapshots indistinguishable — deliberately: identical bytes describe the
* same redirect, for the same server, with the same shell state, so clearing
* "the other one" reaches the same end state.
* Matched on the snapshot's `attemptId` rather than on its bytes: two
* concurrent redirects for the same server with the same shell state serialize
* identically while carrying *different* authorization URLs, which the
* snapshot does not record — so a byte comparison would report them as the
* same attempt and delete the wrong one.
*
* Returns whether anything was removed.
*/
export function clearOwnOAuthResumeSnapshot(
token: string | undefined,
attemptId: string | undefined,
): boolean {
if (typeof window === "undefined" || token === undefined) {
if (typeof window === "undefined" || attemptId === undefined) {
return false;
}
try {
if (window.sessionStorage.getItem(OAUTH_RESUME_KEY) !== token) {
const raw = window.sessionStorage.getItem(OAUTH_RESUME_KEY);
if (!raw) {
return false;
}
const stored = JSON.parse(raw) as Partial<OAuthResumeSnapshot>;
if (stored?.attemptId !== attemptId) {
return false;
}
window.sessionStorage.removeItem(OAUTH_RESUME_KEY);

10
clients/web/src/test/core/auth/oauthUx.test.ts
View file

@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
import {
authRecoveryRestoredMessage,
authRecoveryRetryFailedMessage,
authRecoveryAbandonedMessage,
emaStepUpFailureMessage,
emaStepUpInProgressMessage,
emaStepUpSuccessMessage,
@ -219,6 +220,15 @@ describe("oauthUx resume/restore copy", () => {
);
});
it("authRecoveryAbandonedMessage promises no retry", () => {
const base =
"Could not continue the pending authorization, and the session it belonged to has ended. Reconnect to authorize again.";
expect(authRecoveryAbandonedMessage()).toBe(base);
expect(authRecoveryAbandonedMessage(" ")).toBe(base);
expect(authRecoveryAbandonedMessage(" nope ")).toBe(`${base} (nope)`);
expect(authRecoveryAbandonedMessage()).not.toContain("try again");
});
it("oauthResumeAbandonedMessage reauth is retry-agnostic", () => {
expect(
oauthResumeAbandonedMessage("reauth", { recoverySource: "tool" }),

1
clients/web/src/utils/oauthUx.ts
View file

@ -1,6 +1,7 @@
export {
authRecoveryRestoredMessage,
authRecoveryRetryFailedMessage,
authRecoveryAbandonedMessage,
isActionTriggeredOAuthRecovery,
oauthPreRedirectToastCopy,
oauthResumeAbandonedMessage,

14
core/auth/oauthUx.ts
View file

@ -60,6 +60,20 @@ export function authRecoveryRetryFailedMessage(detail?: string): string {
return detail?.trim() ? `${base} (${detail.trim()})` : base;
}
/**
* Toast when a deferred authorization recovery fails *and* the session it
* belonged to has ended, so nothing was left pending to retry (#2165).
*
* Distinct from {@link authRecoveryRetryFailedMessage} because promising a
* retry the code has explicitly declined to make is worse than saying nothing:
* the user waits for something that will never happen.
*/
export function authRecoveryAbandonedMessage(detail?: string): string {
const base =
"Could not continue the pending authorization, and the session it belonged to has ended. Reconnect to authorize again.";
return detail?.trim() ? `${base} (${detail.trim()})` : base;
}
export function oauthResumeAbandonedMessage(
authKind: OAuthInteractiveAuthKind,
options?: { recoverySource?: OAuthRecoverySource },