diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index ca25de3a..1cc704ab 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -2004,6 +2004,61 @@ describe("useOAuthRecovery", () => { expect(retry).not.toHaveBeenCalled(); }); + it("never touches a retry installed by a newer step-up", async () => { + // Dismissing the prompt frees the slot, so a newer command can open its + // own step-up while this authorization is still in flight. Reading the + // shared ref afterwards would run that newer command under this + // authorization; clearing it would delete it (#2165). + let settleFirst: ((outcome: unknown) => void) | undefined; + const handleAuthChallenge = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + settleFirst = resolve; + }), + ) + .mockResolvedValue({ kind: "satisfied" }); + const client = fakeClient({ handleAuthChallenge }); + const h = harness({ + servers: [entry("a", {}, true)], + activeServerId: "a", + client, + }); + + const firstRetry = vi.fn().mockResolvedValue(undefined); + await openStepUp(h, "tool", firstRetry); + let authorize: Promise | undefined; + await act(async () => { + authorize = h.api().handleStepUpAuthorize(); + }); + await waitFor(() => expect(settleFirst).toBeDefined()); + + // A newer command opens its own step-up mid-flight. + const secondRetry = vi.fn().mockResolvedValue(undefined); + await openStepUp(h, "prompt", secondRetry); + + await act(async () => { + settleFirst!({ kind: "failed", error: new Error("denied") }); + await authorize; + }); + + // The old attempt reported against its own source and ran nothing. + expect(h.spies.setSourceScopedError).toHaveBeenCalledWith( + "tool", + expect.stringContaining("denied"), + ); + expect(firstRetry).not.toHaveBeenCalled(); + expect(secondRetry).not.toHaveBeenCalled(); + + // …and the newer prompt's retry survived to run on its own authorization. + await act(async () => { + await h.api().handleStepUpAuthorize(); + }); + expect(secondRetry).toHaveBeenCalledTimes(1); + expect(firstRetry).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 05d4e2fd..f64cd6db 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1462,6 +1462,17 @@ export function useOAuthRecovery({ if (stepUp.enterpriseManaged) { stepUpAuthorizeInProgressRef.current = true; setPendingStepUp(null); + // Take this attempt's retry out of the shared ref BEFORE any await + // (#2165). Dismissing the prompt above frees the step-up slot, so a + // newer command can open its own prompt and install its own retry while + // the authorization below is in flight — and every arm that reads or + // clears the ref afterwards would then be acting on that newer + // operation: running it under this authorization and reporting its + // failure to this attempt's source, or deleting it outright. Owning the + // value locally is what makes each arm's decision apply to the attempt + // that made it, and leaves a later prompt's retry untouched. + const retry = pendingStepUpRetryRef.current; + pendingStepUpRetryRef.current = null; notifications.show({ title: "Organization permissions", message: emaStepUpInProgressMessage(), @@ -1482,8 +1493,6 @@ export function useOAuthRecovery({ color: "green", autoClose: 5000, }); - const retry = pendingStepUpRetryRef.current; - pendingStepUpRetryRef.current = null; if (retry) { try { await retry(); @@ -1513,23 +1522,15 @@ export function useOAuthRecovery({ authChallenge: outcome.challenge, recoverySource: stepUp.source, }); - // Handing off, so the retry goes too — as the standard (non-EMA) - // step-up below already does. A full-page redirect cannot carry a - // closure across the navigation, so keeping it buys nothing on the - // success path; and `prepareOAuthRedirect` returns before the - // navigation is resolved, so on the failure path it would survive - // as a stale operation a later, unrelated step-up could re-run - // (#2165). - pendingStepUpRetryRef.current = null; + // Nothing to drop: this attempt's retry left the shared ref + // before the awaits above, and a full-page redirect could not + // carry the closure across the navigation anyway. 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). - pendingStepUpRetryRef.current = null; + // Terminal, and this attempt's retry is already out of the shared + // ref — so it dies with the attempt, and whatever a later prompt + // has installed is left alone. const failureMessage = emaStepUpFailureMessage(outcome.error.message); notifications.show({ title: "Organization permissions", @@ -1544,9 +1545,7 @@ 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 — and it - // drops the stored retry for the same reason that arm does. - pendingStepUpRetryRef.current = null; + // the `failed` outcome above is, so the two cannot disagree. const failureMessage = emaStepUpFailureMessage( err instanceof Error ? err.message : String(err), ); diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index a3f0ee9f..ee2bebf3 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -182,8 +182,13 @@ export function restoreTabUiFromSnapshot( } /** - * Persist the snapshot, returning the exact serialization stored — the - * "attempt token" {@link clearOwnOAuthResumeSnapshot} matches against (#2165). + * Persist the snapshot, returning the **`attemptId`** it was stored under — + * the identifier {@link clearOwnOAuthResumeSnapshot} matches on (#2165), not + * the serialized snapshot and not anything to compare bytes against. + * + * A fresh id is minted per write unless the caller supplied one, so two + * redirect attempts are always distinguishable even when their snapshots are + * byte-identical. * * `undefined` when nothing was stored (no `window`, privacy mode, quota), so a * caller holding one knows a clear is meaningful.