diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index dbd04a51..2549a138 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -893,6 +893,38 @@ describe("useOAuthRecovery", () => { expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled(); }); + it("reports a rejected challenge against the server it arrived for", async () => { + // The catch must not re-read the active server: a switch during the + // await would otherwise flag the wrong one, or drop the report (#2165). + let rejectCheck: ((err: Error) => void) | undefined; + const client = fakeClient({ + checkAuthChallengeSatisfied: vi.fn( + () => + new Promise((_resolve, reject) => { + rejectCheck = reject; + }), + ), + }); + const servers = [entry("a"), entry("b")]; + const h = harness({ servers, activeServerId: "a", client }); + await act(async () => { + client.emit("authChallengeInteractive", { + challenge: challenge(), + authorizationUrl: AUTH_URL, + }); + }); + await waitFor(() => expect(rejectCheck).toBeDefined()); + + h.rerender({ servers, activeServerId: "b", client }); + await act(async () => { + rejectCheck!(new Error("remote auth state unreachable")); + }); + + await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a")); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("b"); + }); + it("raises the banner on an oauthError event", async () => { const client = fakeClient(); const h = harness({ servers: [entry("a")], activeServerId: "a", client }); @@ -1065,6 +1097,51 @@ describe("useOAuthRecovery", () => { ); }); + it("does not put a stale challenge back over a newer deferral", async () => { + // The tab can go hidden mid-resume and a newer challenge defer itself + // into the slot; that one describes the session as it is now, so the + // failing attempt must not overwrite it (#2165). + let rejectResume: ((err: Error) => void) | undefined; + const client = fakeClient({ + handleAuthChallenge: vi.fn( + () => + new Promise((_resolve, reject) => { + rejectResume = reject; + }), + ), + }); + const h = harness({ servers: [entry("a")], activeServerId: "a", client }); + await defer(client, h); + + await act(async () => { + becomeVisible(); + }); + await waitFor(() => expect(rejectResume).toBeDefined()); + + // A newer challenge arrives while the resume is still in flight. + visibility.visible = false; + await act(async () => { + client.emit("authChallengeInteractive", { + challenge: challenge("insufficient_scope"), + authorizationUrl: AUTH_URL, + }); + }); + + await act(async () => { + rejectResume!(new Error("token endpoint unreachable")); + }); + await waitFor(() => + expect(toastTitles()).toContain("Could not continue authorization"), + ); + + // The newer (step-up) challenge is what the next trigger acts on. + await act(async () => { + becomeVisible(); + }); + await waitFor(() => expect(h.api().pendingStepUp).not.toBeNull()); + 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 de1b850a..f79fa247 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -539,8 +539,10 @@ export function useOAuthRecovery({ ...(authKind === "step_up" && authChallenge && { authChallenge }), ...(recoverySource && { recoverySource }), }); - // The navigation is what this whole callback exists to reach, so a - // rejection here means the flow failed before it started (#2165). Two + // The navigation is what this whole callback exists to reach, and + // `beginInteractiveAuthorization` resolves it *last* — every await it + // can reject on runs before the browser is handed over — 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 @@ -848,7 +850,15 @@ export function useOAuthRecovery({ // 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); + // + // 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); + } notifications.show({ title: "Could not continue authorization", message: authRecoveryRetryFailedMessage( @@ -879,12 +889,17 @@ export function useOAuthRecovery({ const onAuthChallengeInteractive = ( event: TypedEvent<"authChallengeInteractive">, ): void => { + // Read at event time, not inside the async body: this is the server the + // challenge arrived for, and both the body and the catch below have to + // mean *that* one. Re-reading it after an await would let a server + // switch (or a session that cleared it) either report against a server + // this challenge has nothing to do with, or drop the report entirely. + const serverId = sessionRef.current.activeServerId; + if (!serverId) { + return; + } void (async () => { const { challenge, authorizationUrl } = event.detail; - const serverId = sessionRef.current.activeServerId; - if (!serverId) { - return; - } const server = sessionRef.current.servers.find( (s) => s.id === serverId, ); @@ -922,10 +937,6 @@ export function useOAuthRecovery({ // 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); }); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index f0ee3bc6..96d1df80 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -2847,6 +2847,26 @@ describe("OAuthManager", () => { parseSpy.mockRestore(); }); + it("does not navigate when the fallible setup rejects", async () => { + // #2165: the web client drops its OAuth resume snapshot when this + // rejects, which is only sound because the navigation resolves last. + const params = createMockParams(); + storageOf(params).getClientInformation.mockRejectedValue( + new Error("client information unreadable"), + ); + const manager = new OAuthManager(params); + + await expect( + manager.beginInteractiveAuthorization( + new URL("https://auth.example.com/authorize"), + ), + ).rejects.toThrow("client information unreadable"); + expect( + params.initialConfig.navigation!.navigateToAuthorization, + ).not.toHaveBeenCalled(); + expect(params.dispatchOAuthAuthorizationRequired).not.toHaveBeenCalled(); + }); + it("throws when navigation is not configured", async () => { const params = createMockParams({ initialConfig: { diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 873be9f9..0f8f919a 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -974,12 +974,25 @@ export class OAuthManager { } } - this.requireNavigation().navigateToAuthorization(authorizationUrl); - + // Every fallible step runs BEFORE the navigation, deliberately (#2165). + // `navigateToAuthorization` hands the browser to the authorization server, + // and a caller that treats a rejection as "the flow never started" — the + // web client drops its OAuth resume snapshot on one — has no way to tell a + // pre-navigation failure from a post-navigation one. Resolving the + // navigation last makes every rejection this method can produce a + // pre-navigation rejection, so that reading is always correct. + // + // It is also the better behaviour on its own terms: a provider that cannot + // be built, or client information that cannot be read, means the callback + // could not have been completed anyway, so sending the user to the + // authorization server first only buys them a round trip to a dead end. + const navigation = this.requireNavigation(); const provider = await this.createOAuthProvider(); const clientInfo = await provider.clientInformation(); await this.recordAuthorizationCodeFlowState(authorizationUrl, clientInfo); + navigation.navigateToAuthorization(authorizationUrl); + this.params.dispatchOAuthAuthorizationRequired({ url: authorizationUrl }); }