diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 2549a138..38a72218 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1142,6 +1142,43 @@ describe("useOAuthRecovery", () => { expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1); }); + it("does not resurrect a challenge whose session ended mid-resume", async () => { + // A server switch clears the slot on purpose. The in-flight attempt then + // rejects, finds it empty, and must NOT put its own challenge back — it + // belongs to a session that has ended (#2165). + let rejectResume: ((err: Error) => void) | undefined; + const client = fakeClient({ + handleAuthChallenge: vi.fn( + () => + new Promise((_resolve, reject) => { + rejectResume = reject; + }), + ), + }); + const servers = [entry("a"), entry("b")]; + const h = harness({ servers, activeServerId: "a", client }); + await defer(client, h); + + await act(async () => { + becomeVisible(); + }); + await waitFor(() => expect(rejectResume).toBeDefined()); + + h.rerender({ servers, activeServerId: "b", client }); + await act(async () => { + rejectResume!(new Error("token endpoint unreachable")); + }); + await waitFor(() => + expect(toastTitles()).toContain("Could not continue authorization"), + ); + + // Nothing was restored, so returning to the tab resumes nothing. + await act(async () => { + becomeVisible(); + }); + expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1); + }); + it("does not resume while disconnected, and resumes on the reconnect", async () => { const client = fakeClient({ handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }), diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index f79fa247..f2343011 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -44,6 +44,7 @@ import { applyOAuthResumeUi, buildTabUiSnapshot, clearOAuthResumeSnapshot, + clearOwnOAuthResumeSnapshot, consumeOAuthResumeSnapshot, oauthResumeInsufficientScopeMessage, oauthResumeToastMessage, @@ -529,7 +530,7 @@ export function useOAuthRecovery({ onBeforeOAuthRedirect(authorizationUrl); // Write immediately before navigation so implicit client teardown (during // connect setup) cannot clear the snapshot after we persist it. - writeOAuthResumeSnapshot({ + const snapshotToken = writeOAuthResumeSnapshot({ version: 1, serverId, activeTab, @@ -548,10 +549,15 @@ export function useOAuthRecovery({ // read it as an *abandoned* redirect and offer that diagnosis, which is // wrong — and the failure has to be reported the way every sibling arm // reports one, with the red border (#1621) and the re-auth banner. + // + // Scoped to the snapshot *this* attempt wrote: nothing single-flights + // `prepareOAuthRedirect`, so a later attempt may already have replaced + // it, and deleting that one would strand the redirect that is actually + // in flight. oauthClient ?.beginInteractiveAuthorization(authorizationUrl) .catch((err: unknown) => { - clearOAuthResumeSnapshot(); + clearOwnOAuthResumeSnapshot(snapshotToken); setFailedServerId(serverId); showReAuthBanner(serverId, err); }); @@ -851,13 +857,26 @@ export function useOAuthRecovery({ // the recovery was delivered. It still is owed, so restore it and let // the next trigger retry rather than losing it silently (#2165). // - // Only into an empty slot, though: the tab can go hidden during the - // awaits above and a *newer* challenge defer itself into it, and that - // one describes the session as it is now. Latest wins — putting this - // attempt's stale challenge back over it would retry authorization - // data the server has already superseded. - if (sessionRef.current.pendingReauth === null) { - setPendingReauth(pending); + // Only into the session this attempt belongs to, and only into an + // empty slot. + // + // The awaits above can outlive the session entirely — a server switch + // or a disconnect clears the slot on purpose, and restoring into that + // would resurrect a challenge for a session that has ended. The client + // identity is part of the check, not just the server id: reconnecting + // to the SAME server builds a replacement `InspectorClient`, which an + // id-only check would wave through. + // + // And the tab can go hidden mid-flight with a *newer* challenge + // deferring itself into the slot; that one describes the session as it + // 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 ( + sessionRef.current.activeServerId === pending.serverId && + sessionRef.current.inspectorClient === client + ) { + setPendingReauth((prev) => prev ?? pending); } notifications.show({ title: "Could not continue authorization", diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index bd73611d..6f80ddd7 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -11,6 +11,7 @@ import { readOAuthResumeSnapshot, restoreTabUiFromSnapshot, writeOAuthResumeSnapshot, + clearOwnOAuthResumeSnapshot, type OAuthResumeSnapshot, } from "./oauthResume.js"; import { @@ -349,6 +350,43 @@ describe("oauthResume", () => { } }); + it("clearOwnOAuthResumeSnapshot removes only the snapshot it was given", () => { + const first = writeOAuthResumeSnapshot({ + 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: {}, + }); + + expect(clearOwnOAuthResumeSnapshot(first)).toBe(false); + expect(readOAuthResumeSnapshot()?.serverId).toBe("b"); + + const second = window.sessionStorage.getItem(OAUTH_RESUME_KEY)!; + expect(clearOwnOAuthResumeSnapshot(second)).toBe(true); + expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull(); + }); + + it("clearOwnOAuthResumeSnapshot is a no-op without a token", () => { + writeOAuthResumeSnapshot({ + version: 1, + serverId: "a", + activeTab: "tools", + authKind: "reauth", + tabUi: {}, + }); + expect(clearOwnOAuthResumeSnapshot(undefined)).toBe(false); + expect(readOAuthResumeSnapshot()?.serverId).toBe("a"); + }); + it("writeOAuthResumeSnapshot swallows setItem failures", () => { vi.stubGlobal("sessionStorage", { getItem: () => null, @@ -368,6 +406,17 @@ describe("oauthResume", () => { ).not.toThrow(); }); + it("clearOwnOAuthResumeSnapshot swallows storage failures", () => { + vi.stubGlobal("sessionStorage", { + getItem: () => { + throw new Error("blocked"); + }, + setItem: () => {}, + removeItem: () => {}, + }); + expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false); + }); + it("clearOAuthResumeSnapshot swallows removeItem failures", () => { vi.stubGlobal("sessionStorage", { getItem: () => null, @@ -422,6 +471,10 @@ describe("oauthResume", () => { it("clearOAuthResumeSnapshot is a no-op", () => { expect(() => clearOAuthResumeSnapshot()).not.toThrow(); }); + + it("clearOwnOAuthResumeSnapshot is a no-op", () => { + expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false); + }); }); it("restoreTabUiFromSnapshot returns early when tabUi is undefined", () => { diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index 4684ed71..6bb9a960 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -168,14 +168,26 @@ export function restoreTabUiFromSnapshot( } } -export function writeOAuthResumeSnapshot(snapshot: OAuthResumeSnapshot): void { +/** + * Persist the snapshot, returning the exact serialization stored — the + * "attempt token" {@link clearOwnOAuthResumeSnapshot} matches against (#2165). + * + * `undefined` when nothing was stored (no `window`, privacy mode, quota), so a + * caller holding one knows a clear is meaningful. + */ +export function writeOAuthResumeSnapshot( + snapshot: OAuthResumeSnapshot, +): string | undefined { if (typeof window === "undefined") { - return; + return undefined; } + const serialized = JSON.stringify(snapshot); try { - window.sessionStorage.setItem(OAUTH_RESUME_KEY, JSON.stringify(snapshot)); + window.sessionStorage.setItem(OAUTH_RESUME_KEY, serialized); + return serialized; } catch { // Best-effort — privacy mode / quota. + return undefined; } } @@ -231,6 +243,39 @@ function readLegacyPendingServerSnapshot(): OAuthResumeSnapshot | undefined { } } +/** + * Clear the snapshot **only if it is still the one this attempt wrote** (#2165). + * + * `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. + * + * 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. + * + * Returns whether anything was removed. + */ +export function clearOwnOAuthResumeSnapshot( + token: string | undefined, +): boolean { + if (typeof window === "undefined" || token === undefined) { + return false; + } + try { + if (window.sessionStorage.getItem(OAUTH_RESUME_KEY) !== token) { + return false; + } + window.sessionStorage.removeItem(OAUTH_RESUME_KEY); + return true; + } catch { + return false; + } +} + export function clearOAuthResumeSnapshot(): void { if (typeof window === "undefined") { return;