From f74aa757d29ab03da624243fecd1de50cba4e641 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 01:34:40 -0400 Subject: [PATCH] fix(web): terminate the five void-discarded OAuth recovery promises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of the five sites in `useOAuthRecovery` discarded a promise with `void`, satisfying `@typescript-eslint/no-floating-promises` but not the narrower rule AGENTS.md states — `void` only where the callee owns its failures or the caller cannot await, with a comment saying which. Each now picks a user-visible outcome: - `prepareOAuthRedirect` clears the resume snapshot (so the next load cannot read a redirect that never started as an *abandoned* one), flags the server and raises the re-auth banner. - the `authChallengeInteractive` handler flags and banners, where before a rejection drew no UI response at all. - `resumePendingReauth` restores the pending slot and toasts, so the next trigger retries the deferred recovery instead of losing it. - the `/oauth/callback` effect flags and banners what falls between its existing arms — the client rebuild and the post-resume scope check. - `handleStepUpAuthorize`'s EMA branch reports exactly as its `failed` outcome arm does; the stored retry gets its own catch, since by then the permissions leg succeeded and the failure is the command's. The two surviving `void`s carry the justification AGENTS.md asks for. Closes #2165 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 163 ++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 106 +++++++++++- .../web/src/test/core/auth/oauthUx.test.ts | 11 ++ clients/web/src/utils/oauthUx.ts | 1 + core/auth/oauthUx.ts | 14 ++ 5 files changed, 290 insertions(+), 5 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 9eeed3c3..dbd04a51 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -570,6 +570,31 @@ describe("useOAuthRecovery", () => { expect(active.beginInteractiveAuthorization).not.toHaveBeenCalled(); }); + it("clears the snapshot and reports a redirect that never started", async () => { + // The navigation is what the snapshot describes, so a rejection here + // must not leave one behind: the next load would read it as an + // *abandoned* redirect and offer that (wrong) diagnosis (#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, + }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + expect(h.api().reAuthBanner?.message).toContain( + "provider state unreadable", + ); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull(); + }); + it("tolerates having no client at all", () => { const h = harness({ servers: [entry("a")], client: null }); act(() => @@ -845,6 +870,29 @@ describe("useOAuthRecovery", () => { expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled(); }); + it("reports a challenge whose recovery check rejects", async () => { + // A listener cannot be awaited, so without the handler's own catch this + // challenge draws no UI response whatsoever (#2165). + const client = fakeClient({ + checkAuthChallengeSatisfied: vi + .fn() + .mockRejectedValue(new Error("remote auth state unreachable")), + }); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await act(async () => { + client.emit("authChallengeInteractive", { + challenge: challenge(), + authorizationUrl: AUTH_URL, + }); + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + expect(h.api().reAuthBanner?.message).toContain( + "remote auth state unreachable", + ); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled(); + }); + it("raises the banner on an oauthError event", async () => { const client = fakeClient(); const h = harness({ servers: [entry("a")], activeServerId: "a", client }); @@ -987,6 +1035,36 @@ describe("useOAuthRecovery", () => { expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1); }); + it("restores the deferred slot when the resume itself rejects", async () => { + // The slot is cleared to keep two triggers from racing, not because the + // recovery was delivered — so a rejection has to put it back rather than + // dropping the deferred recovery silently (#2165). + const client = fakeClient({ + handleAuthChallenge: vi + .fn() + .mockRejectedValue(new Error("token endpoint unreachable")), + }); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await defer(client, h); + + await act(async () => { + becomeVisible(); + }); + await waitFor(() => + expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1), + ); + expect(toastTitles()).toContain("Could not continue authorization"); + expect(toastWith("token endpoint unreachable")).toBeTruthy(); + + // Restored, so the next trigger retries it. + await act(async () => { + becomeVisible(); + }); + await waitFor(() => + expect(client.handleAuthChallenge).toHaveBeenCalledTimes(2), + ); + }); + it("does not resume while disconnected, and resumes on the reconnect", async () => { const client = fakeClient({ handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }), @@ -1167,6 +1245,43 @@ describe("useOAuthRecovery", () => { ); }); + it("flags and banners a client rebuild that throws", async () => { + // Between the arms that have their own handling (#2165): by the time + // `setupClientForServer` runs, the callback URL and the one-shot + // snapshot are both spent, so there is nothing left to retry with. + snapshot(); + onCallbackUrl(`?code=abc&state=${AUTH_ID}`); + const h = harness({ + servers: [entry("a")], + setupClient: () => { + throw new Error("client construction failed"); + }, + }); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + expect(h.api().reAuthBanner?.message).toContain( + "client construction failed", + ); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + }); + + it("flags and banners a post-resume scope check that rejects", async () => { + snapshot({ + authKind: "step_up", + authChallenge: { reason: "insufficient_scope" }, + }); + const client = fakeClient({ + checkAuthChallengeSatisfied: vi + .fn() + .mockRejectedValue(new Error("scope check unreachable")), + }); + const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client); + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + expect(h.api().reAuthBanner?.message).toContain( + "scope check unreachable", + ); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + }); + it("restores the shell, rebuilds the client and completes", async () => { snapshot({ remoteSessionId: "remote-9" }); const client = fakeClient(); @@ -1593,6 +1708,54 @@ describe("useOAuthRecovery", () => { ); }); + it("routes an EMA step-up that rejects back to the panel that asked", async () => { + // `StepUpAuthModal` calls this handler as `void onAuthorize()`, so + // without the catch the rejection reaches nobody (#2165). + const client = fakeClient({ + handleAuthChallenge: vi + .fn() + .mockRejectedValue(new Error("IdP unreachable")), + }); + const h = harness({ + servers: [entry("a", {}, true)], + activeServerId: "a", + client, + }); + await openStepUp(h, "prompt"); + await act(async () => { + await h.api().handleStepUpAuthorize(); + }); + expect(h.spies.setSourceScopedError).toHaveBeenCalledWith( + "prompt", + expect.stringContaining("IdP unreachable"), + ); + expect(toastTitles()).toContain("Organization permissions"); + }); + + it("reports a failed retry as the command's failure, not the step-up's", async () => { + const client = fakeClient({ + handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }), + }); + const h = harness({ + servers: [entry("a", {}, true)], + activeServerId: "a", + client, + }); + const retry = vi.fn().mockRejectedValue(new Error("tools/call failed")); + await openStepUp(h, "tool", retry); + await act(async () => { + await h.api().handleStepUpAuthorize(); + }); + // The permissions leg succeeded, so this is the retried command's + // failure and is reported as such. + expect(toastTitles()).toContain("Permissions updated"); + expect(toastTitles()).toContain("Retry failed"); + expect(h.spies.setSourceScopedError).toHaveBeenCalledWith( + "tool", + "tools/call failed", + ); + }); + 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 df532f0e..de1b850a 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -55,6 +55,7 @@ import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs"; import type { PendingReauth } from "../utils/pendingReauth"; import { authRecoveryRestoredMessage, + authRecoveryRetryFailedMessage, isReAuthBannerReason, issuerBindingFailureCopy, lostAuthorizationStateActionLabel, @@ -538,9 +539,30 @@ export function useOAuthRecovery({ ...(authKind === "step_up" && authChallenge && { authChallenge }), ...(recoverySource && { recoverySource }), }); - void oauthClient?.beginInteractiveAuthorization(authorizationUrl); + // The navigation is what this whole callback exists to reach, so a + // rejection here means the flow failed before it started (#2165). Two + // things then have to happen, and neither is optional: the snapshot + // written a moment ago has to go — the next page load would otherwise + // 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. + oauthClient + ?.beginInteractiveAuthorization(authorizationUrl) + .catch((err: unknown) => { + clearOAuthResumeSnapshot(); + setFailedServerId(serverId); + showReAuthBanner(serverId, err); + }); }, - [sessionRef, inspectorClient, activeTab, ui, onBeforeOAuthRedirect], + [ + sessionRef, + inspectorClient, + activeTab, + ui, + onBeforeOAuthRedirect, + setFailedServerId, + showReAuthBanner, + ], ); const tryApplyStoredAuthRecovery = useCallback( @@ -821,6 +843,20 @@ export function useOAuthRecovery({ reason: pending.challenge.reason, }); } + } catch (err) { + // The slot was cleared above only to keep a tab-visible event and a + // reconnect from starting the same authorization twice — not because + // the recovery was delivered. It still is owed, so restore it and let + // the next trigger retry rather than losing it silently (#2165). + setPendingReauth(pending); + notifications.show({ + title: "Could not continue authorization", + message: authRecoveryRetryFailedMessage( + err instanceof Error ? err.message : String(err), + ), + color: "yellow", + autoClose: 6000, + }); } finally { reauthResumeInProgressRef.current = false; } @@ -880,7 +916,19 @@ export function useOAuthRecovery({ authorizationUrl, source: "ambient", }); - })(); + })().catch((err: unknown) => { + // An event listener cannot be awaited, so this is the only place the + // failure can be reported (#2165). Without it the challenge draws no + // UI response at all — the stored-recovery check or the remote-state + // push rejected, and the user is left on a session whose + // authorization has lapsed with nothing on screen saying so. + const serverId = sessionRef.current.activeServerId; + if (!serverId) { + return; + } + setFailedServerId(serverId); + showReAuthBanner(serverId, err); + }); }; inspectorClient.addEventListener( @@ -900,12 +948,16 @@ export function useOAuthRecovery({ tryApplyStoredAuthRecovery, runVisibleInteractiveAuth, deferAmbientReauth, + setFailedServerId, + showReAuthBanner, ]); useEffect(() => { return onBrowserTabVisible(() => { const pending = sessionRef.current.pendingReauth; if (pending) { + // `void`: a visibility listener cannot be awaited, and + // `resumePendingReauth` now terminates its own failures (#2165). void resumePendingReauth(pending); } }); @@ -921,6 +973,8 @@ export function useOAuthRecovery({ } const pending = sessionRef.current.pendingReauth; if (pending) { + // `void`: an effect body cannot be awaited, and `resumePendingReauth` + // now terminates its own failures (#2165). void resumePendingReauth(pending); } }, [sessionRef, connectionStatus, inspectorClient, resumePendingReauth]); @@ -1236,7 +1290,17 @@ export function useOAuthRecovery({ autoClose: 6000, }); } - })(); + })().catch((err: unknown) => { + // Everything inside has its own arm; this catches what sits *between* + // them (#2165) — `setupClientForServer` throwing while it builds the + // client, and the post-resume `checkAuthChallengeSatisfied`. Both land + // after the callback URL and the one-shot resume snapshot have been + // consumed, so there is nothing left to retry with and the failure has + // to be reported here or nowhere. + connectStartRef.current = undefined; + setFailedServerId(server.id); + showReAuthBanner(server.id, err instanceof Error ? err : String(err)); + }); }, [ servers, setupClientForServerRef, @@ -1368,7 +1432,23 @@ export function useOAuthRecovery({ const retry = pendingStepUpRetryRef.current; pendingStepUpRetryRef.current = null; if (retry) { - await retry(); + try { + await retry(); + } catch (err) { + // Its own catch, not the outer one: the permissions leg + // succeeded, so a failure from here belongs to the retried + // command and is reported as that command's — routed to the + // panel that issued it rather than dressed up as a step-up + // failure (#2165). + const message = err instanceof Error ? err.message : String(err); + notifications.show({ + title: "Retry failed", + message, + color: "red", + autoClose: 6000, + }); + setSourceScopedError(stepUp.source, message); + } } return; } @@ -1392,6 +1472,22 @@ export function useOAuthRecovery({ }); setSourceScopedError(stepUp.source, failureMessage); } + } catch (err) { + // `StepUpAuthModal` calls this handler as `void onAuthorize()`, so a + // 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. + const failureMessage = emaStepUpFailureMessage( + err instanceof Error ? err.message : String(err), + ); + notifications.show({ + title: "Organization permissions", + message: failureMessage, + color: "red", + autoClose: 6000, + }); + setSourceScopedError(stepUp.source, failureMessage); } finally { stepUpAuthorizeInProgressRef.current = false; } diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index e11bb990..49959de3 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; import { authRecoveryRestoredMessage, + authRecoveryRetryFailedMessage, emaStepUpFailureMessage, emaStepUpInProgressMessage, emaStepUpSuccessMessage, @@ -208,6 +209,16 @@ describe("oauthUx resume/restore copy", () => { ); }); + it("authRecoveryRetryFailedMessage appends only a non-blank detail", () => { + const base = + "Could not continue the pending authorization. The Inspector will try again the next time this tab becomes active or the session reconnects."; + expect(authRecoveryRetryFailedMessage()).toBe(base); + expect(authRecoveryRetryFailedMessage(" ")).toBe(base); + expect(authRecoveryRetryFailedMessage(" token endpoint 500 ")).toBe( + `${base} (token endpoint 500)`, + ); + }); + 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 ba86edf7..4858734d 100644 --- a/clients/web/src/utils/oauthUx.ts +++ b/clients/web/src/utils/oauthUx.ts @@ -1,5 +1,6 @@ export { authRecoveryRestoredMessage, + authRecoveryRetryFailedMessage, isActionTriggeredOAuthRecovery, oauthPreRedirectToastCopy, oauthResumeAbandonedMessage, diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 184e5c5a..cdf8cb7f 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -46,6 +46,20 @@ export function authRecoveryRestoredMessage(options?: { : "Session credentials were updated."; } +/** + * Toast when a *deferred* authorization recovery (#2165) fails partway + * through — the tab-visible / reconnect resume, not a user-initiated one. + * + * Phrased as "will try again" rather than as a final failure because the + * pending recovery is restored when this fires: the slot was cleared only to + * keep two triggers from racing, and it is still owed. + */ +export function authRecoveryRetryFailedMessage(detail?: string): string { + const base = + "Could not continue the pending authorization. The Inspector will try again the next time this tab becomes active or the session reconnects."; + return detail?.trim() ? `${base} (${detail.trim()})` : base; +} + export function oauthResumeAbandonedMessage( authKind: OAuthInteractiveAuthKind, options?: { recoverySource?: OAuthRecoverySource },