diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 38a72218..23a72829 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -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 }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index f2343011..2030b54f 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -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), ); diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index 6f80ddd7..ee3a31c0 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -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", () => { diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index 6bb9a960..a3f0ee9f 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -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; + if (stored?.attemptId !== attemptId) { return false; } window.sessionStorage.removeItem(OAUTH_RESUME_KEY); diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index 49959de3..dabe2072 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -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" }), diff --git a/clients/web/src/utils/oauthUx.ts b/clients/web/src/utils/oauthUx.ts index 4858734d..eeaa9436 100644 --- a/clients/web/src/utils/oauthUx.ts +++ b/clients/web/src/utils/oauthUx.ts @@ -1,6 +1,7 @@ export { authRecoveryRestoredMessage, authRecoveryRetryFailedMessage, + authRecoveryAbandonedMessage, isActionTriggeredOAuthRecovery, oauthPreRedirectToastCopy, oauthResumeAbandonedMessage, diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index cdf8cb7f..7c07f233 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -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 },