From e035b52833ad82205b722782b89434ffe8cb14fd Mon Sep 17 00:00:00 2001 From: AngelPaella Date: Mon, 10 Aug 2026 11:36:18 -0600 Subject: [PATCH] fix(react-ui): make each OAuth flow own the popup it opened `useOAuthWindowListener` passed event names to `off()`, which never matched a listener id and removed nothing, so every provider click stacked another listener pair on the same `ChildWindow`. One popup callback then redeemed the same one-time secret once per pair, and the second redemption fails server-side, so the user saw a login error after appearing to succeed. Capturing the ids `on()` already returns is enough to fix that. It also exposes the real problem underneath: all providers share one named popup, and the flow resumes at three points after handing that popup over (the URL await, the refresh await, the 2.5s closure poller). The previous `cancelled` flag was consulted at one of them. Each flow now claims `flowIdRef` and rechecks the claim at all three plus the catch path, so a superseded flow can no longer close the popup, publish its error, or clear the loading state belonging to the flow that replaced it. Two teardown-ordering fixes follow from `off()` finally working: - Teardown moved from the top of the call to the moment of takeover. Earlier left a still-live popup with nothing listening across the URL await, dropping a login that had already completed. - The closure poller no longer removes listeners. The callback page posts its secret and then closes itself, so a tick landing in that gap dropped the material. Takeover and unmount both remove them, so at most one pair is ever alive. Unmounting mid-flow now closes the popup it opened rather than leaving a window on screen that nothing will close. Restoring the old behaviour where the login still landed after unmount would mean keeping the listener leak on purpose. Six tests cover supersede in-flight, supersede after setup, auth-material redemption, and both unmount paths. Verified by mutation: removing the takeover cleanup, the catch guard, or the unmount close each fail a test. --- .changeset/oauth-popup-ownership.md | 16 ++ .../hooks/use-oauth-window-listener.test.ts | 181 ++++++++++++++++++ .../src/hooks/useOAuthWindowListener.ts | 69 +++++-- 3 files changed, 254 insertions(+), 12 deletions(-) create mode 100644 .changeset/oauth-popup-ownership.md create mode 100644 packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts diff --git a/.changeset/oauth-popup-ownership.md b/.changeset/oauth-popup-ownership.md new file mode 100644 index 000000000..e72976e7a --- /dev/null +++ b/.changeset/oauth-popup-ownership.md @@ -0,0 +1,16 @@ +--- +"@crossmint/client-sdk-react-ui": patch +--- + +Fixes the OAuth popup flow losing logins when a second provider is clicked. + +`useOAuthWindowListener` passed event names to `off()`, which never matched a listener id and so removed nothing. Every provider click stacked another listener pair on the same `ChildWindow`, and one popup callback then redeemed the same one-time secret once per pair; the second redemption fails server-side, so the user saw a login error after appearing to succeed. + +All providers share one named popup (`PopupWindow.initEmpty` opens `window.open(..., "popupWindow")`), and the flow resumes at three points after handing that popup over: the URL await, the auth-material refresh await, and the 2.5s closure poller. Each flow now claims the popup and re-checks the claim at every one of them, so a superseded flow can no longer close the popup, publish its error, or clear the loading state belonging to the flow that replaced it. + +Two teardown-ordering fixes fall out of `off()` finally working: + +- Listener teardown happens when a new flow takes the popup over, not at the top of the call. Doing it earlier left a still-live popup with nothing listening across the URL await, dropping a login that had already completed. +- The closure poller no longer removes listeners. The callback page posts its one-time secret and then closes itself, so a tick landing in that gap dropped the material. Takeover and unmount both remove them, so at most one pair is ever alive. + +Unmounting mid-flow now closes the popup it opened, instead of leaving a window on screen that nothing will ever close. diff --git a/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts b/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts new file mode 100644 index 000000000..c5a695d9d --- /dev/null +++ b/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts @@ -0,0 +1,181 @@ +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, test, vi } from "vitest"; + +import { useOAuthWindowListener } from "./useOAuthWindowListener"; + +const childWindow = { + on: vi.fn((event: string, _handler: (data: unknown) => unknown) => `listener-id:${event}`), + off: vi.fn(), +}; + +// PopupWindow.initEmpty opens a window by name, so every call hands back the same popup. +const popupWindow = { location: { href: "about:blank" }, closed: false, close: vi.fn() }; + +vi.mock("@crossmint/client-sdk-window", () => ({ + ChildWindow: vi.fn(() => childWindow), + PopupWindow: { initEmpty: vi.fn(() => ({ window: popupWindow })) }, +})); + +const getOAuthUrl = vi.fn(); +const handleRefreshAuthMaterial = vi.fn(); + +vi.mock("@/hooks", () => ({ + useCrossmintAuth: () => ({ + crossmintAuth: { getOAuthUrl, handleRefreshAuthMaterial }, + }), +})); + +// No prefetched URLs, so both clicks await getOAuthUrl. +const NO_PREFETCHED_URLS = { google: "", twitter: "" } as Parameters[0]; +const PREFETCHED_URLS = { + google: "https://oauth.example/google", + twitter: "https://oauth.example/twitter", +} as Parameters[0]; + +// Kept outside the render callback so a superseded flow's writes are observable, and so the +// hook's useCallback sees the stable identity OAuthFlowProvider gives it in production. +const setError = vi.fn(); + +function renderOAuthHook(urls = NO_PREFETCHED_URLS) { + return renderHook(() => useOAuthWindowListener(urls, setError)); +} + +function getListener(event: string) { + const call = childWindow.on.mock.calls.find(([registeredEvent]) => registeredEvent === event); + if (call == null) { + throw new Error(`no listener registered for ${event}`); + } + return call[1] as unknown as (data: unknown) => Promise | void; +} + +describe("useOAuthWindowListener", () => { + afterEach(() => { + // Before clearing the spies, not after: unmounting tears a live flow down, and testing + // library's auto-cleanup runs late enough that those calls would land in the next test. + cleanup(); + popupWindow.location.href = "about:blank"; + popupWindow.closed = false; + vi.clearAllMocks(); + }); + + describe("when a second provider is clicked while the first URL is still resolving", () => { + test("registers one listener pair, so the popup's one-time secret is redeemed once", async () => { + const resolvers: Array<(url: string) => void> = []; + getOAuthUrl.mockImplementation(() => new Promise((resolve) => resolvers.push(resolve))); + + const { result } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google"); + result.current.createPopupAndSetupListeners("twitter"); + await waitFor(() => expect(resolvers).toHaveLength(2)); + resolvers[0]("https://oauth.example/google"); + resolvers[1]("https://oauth.example/twitter"); + }); + + await waitFor(() => expect(childWindow.on).toHaveBeenCalledTimes(2)); + expect(childWindow.on).toHaveBeenCalledWith("authMaterialFromPopupCallback", expect.any(Function)); + expect(childWindow.on).toHaveBeenCalledWith("errorFromPopupCallback", expect.any(Function)); + expect(popupWindow.location.href).toBe("https://oauth.example/twitter"); + }); + + test("leaves the popup, the error and the loading state to the flow that took over", async () => { + const resolvers: Array<(url: string) => void> = []; + const rejecters: Array<(error: Error) => void> = []; + getOAuthUrl.mockImplementation( + () => + new Promise((resolve, reject) => { + resolvers.push(resolve); + rejecters.push(reject); + }) + ); + + const { result } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google"); + result.current.createPopupAndSetupListeners("twitter"); + await waitFor(() => expect(resolvers).toHaveLength(2)); + resolvers[1]("https://oauth.example/twitter"); + }); + await act(async () => { + rejecters[0](new Error("google 500")); + }); + + expect(popupWindow.location.href).toBe("https://oauth.example/twitter"); + expect(popupWindow.close).not.toHaveBeenCalled(); + expect(setError).not.toHaveBeenCalledWith("google 500"); + expect(result.current.activeOAuthProvider).toBe("twitter"); + }); + }); + + describe("when a second provider is clicked after the first flow is fully set up", () => { + test("removes the first flow's listeners by their returned ids", async () => { + const { result } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google"); + }); + expect(childWindow.off).not.toHaveBeenCalled(); + + await act(async () => { + await result.current.createPopupAndSetupListeners("twitter"); + }); + + expect(childWindow.off).toHaveBeenCalledWith("listener-id:authMaterialFromPopupCallback"); + expect(childWindow.off).toHaveBeenCalledWith("listener-id:errorFromPopupCallback"); + expect(childWindow.off).toHaveBeenCalledTimes(2); + expect(childWindow.on).toHaveBeenCalledTimes(4); + }); + }); + + describe("when the popup reports auth material", () => { + test("redeems the secret, then closes the popup and clears the loading state", async () => { + const { result } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google"); + }); + await act(async () => { + await getListener("authMaterialFromPopupCallback")({ oneTimeSecret: "secret" }); + }); + + expect(handleRefreshAuthMaterial).toHaveBeenCalledWith("secret"); + expect(popupWindow.close).toHaveBeenCalled(); + expect(result.current.activeOAuthProvider).toBeNull(); + }); + }); + + describe("when the provider unmounts mid-flow", () => { + test("removes the listeners and closes the popup it opened", async () => { + const { result, unmount } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google"); + }); + unmount(); + + expect(childWindow.off).toHaveBeenCalledWith("listener-id:authMaterialFromPopupCallback"); + expect(childWindow.off).toHaveBeenCalledWith("listener-id:errorFromPopupCallback"); + expect(popupWindow.close).toHaveBeenCalled(); + }); + + test("does not publish an error for the flow it abandoned", async () => { + const rejecters: Array<(error: Error) => void> = []; + getOAuthUrl.mockImplementation(() => new Promise((_, reject) => rejecters.push(reject))); + + const { result, unmount } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google"); + await waitFor(() => expect(rejecters).toHaveLength(1)); + }); + unmount(); + await act(async () => { + rejecters[0](new Error("google 500")); + }); + + expect(setError).not.toHaveBeenCalledWith("google 500"); + }); + }); +}); diff --git a/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts b/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts index 0fc7a6891..94bab0782 100644 --- a/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts +++ b/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts @@ -11,8 +11,15 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro // Track which OAuth provider's window is currently being interacted with const [activeOAuthProvider, setActiveOAuthProvider] = useState(null); const childRef = useRef | null>(null); + const cleanupRef = useRef<(() => void) | null>(null); + const popupRef = useRef | null>(null); + // Every click claims the next id. Because all flows share one named popup, each resumption + // point below has to check it still holds the claim before touching the popup or the state. + const flowIdRef = useRef(0); + const mountedRef = useRef(true); useEffect(() => { + mountedRef.current = true; if (childRef.current == null) { childRef.current = new ChildWindow(window.opener || window.parent, "*", { incomingEvents, @@ -20,9 +27,13 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro } return () => { - if (childRef.current != null) { - childRef.current.off("authMaterialFromPopupCallback"); - } + mountedRef.current = false; + cleanupRef.current?.(); + cleanupRef.current = null; + // Nothing is left to adopt this popup, so abort the flow instead of leaving a window + // on screen that no longer has a listener to close it. + popupRef.current?.window?.close(); + popupRef.current = null; }; }, []); @@ -31,6 +42,10 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro if (childRef.current == null) { throw new Error("Child window not initialized"); } + // Claim the flow before the first await. + const flowId = ++flowIdRef.current; + const ownsPopup = () => flowIdRef.current === flowId && mountedRef.current; + setActiveOAuthProvider(provider); setError(null); @@ -45,6 +60,7 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro height: 700, incomingEvents, }); + popupRef.current = popup; const prefetchedUrl = oauthUrlMap[provider]; const resolvedUrl = prefetchedUrl || (await crossmintAuth?.getOAuthUrl(provider)); @@ -53,9 +69,17 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro } baseUrl = new URL(resolvedUrl); } catch (e) { - popup?.window?.close(); - setActiveOAuthProvider(null); - setError(e instanceof Error ? e.message : "Failed to start OAuth login"); + if (ownsPopup()) { + popup?.window?.close(); + setActiveOAuthProvider(null); + setError(e instanceof Error ? e.message : "Failed to start OAuth login"); + } + return; + } + + // PopupWindow.initEmpty opens a named window, so the later click reused this + // popup. Leave it to that flow rather than closing it or navigating it again. + if (!ownsPopup()) { return; } @@ -77,35 +101,56 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro }); } + // Drop the previous flow's listeners only now that this one is taking the popup over. + // Doing it before the await above would leave its still-live popup with nothing listening. + cleanupRef.current?.(); + if (popup.window != null) { popup.window.location.href = baseUrl.toString(); } const handleAuthMaterial = async (data: { oneTimeSecret: string }) => { await crossmintAuth?.handleRefreshAuthMaterial(data.oneTimeSecret); - childRef.current?.off("authMaterialFromPopupCallback"); + if (!ownsPopup()) { + return; + } + cleanup(); popup.window?.close(); setActiveOAuthProvider(null); }; const handleError = (data: { error: string }) => { + if (!ownsPopup()) { + return; + } setError(data.error); - childRef.current?.off("errorFromPopupCallback"); + cleanup(); popup.window?.close(); setActiveOAuthProvider(null); }; - childRef.current.on("authMaterialFromPopupCallback", handleAuthMaterial); - childRef.current.on("errorFromPopupCallback", handleError); + const authMaterialListener = childRef.current.on("authMaterialFromPopupCallback", handleAuthMaterial); + const errorListener = childRef.current.on("errorFromPopupCallback", handleError); // Add a check for manual window closure // Ideally we should find a more explicit way of doing this, but I think this is fine for now. + // The listeners deliberately stay registered: a callback page posts its secret and then closes + // itself, so tearing them down here would drop material that is already in flight. Takeover + // and unmount both remove them, so at most one pair is ever alive. const checkWindowClosure = setInterval(() => { if (popup.window?.closed) { clearInterval(checkWindowClosure); - setActiveOAuthProvider(null); - childRef.current?.off("authMaterialFromPopupCallback"); + if (ownsPopup()) { + setActiveOAuthProvider(null); + } } }, 2500); // Check every 2.5 seconds + + const cleanup = () => { + clearInterval(checkWindowClosure); + childRef.current?.off(authMaterialListener); + childRef.current?.off(errorListener); + }; + cleanupRef.current = cleanup; }, [oauthUrlMap, crossmintAuth, setError] );