From 12d97de6ae14cec5b4c982c28684069e55dadbe9 Mon Sep 17 00:00:00 2001 From: Mike Fortman Date: Fri, 23 Oct 2026 11:01:58 -0500 Subject: [PATCH 1/3] fix: stop Google sign-in hanging on /auth/callback in dev React StrictMode's dev remount unsubscribed the useMutation observer, which detaches it from the in-flight mutation. The remounted effect then hit the sessionStorage dedupe guard and returned, so the exchange succeeded on the backend but the page never saw the result: it stayed on "Signing you in..." with no redirect. Dedupe on the exchange promise (keyed by code) instead, so a re-run of the effect waits on the request already in flight, and keep status in component state. A reload of an already-used callback URL now shows an error instead of spinning with no request in flight. --- frontend/app/auth/callback/page.test.tsx | 84 ++++++++++ frontend/app/auth/callback/page.tsx | 187 +++++++++++++---------- 2 files changed, 194 insertions(+), 77 deletions(-) create mode 100644 frontend/app/auth/callback/page.test.tsx diff --git a/frontend/app/auth/callback/page.test.tsx b/frontend/app/auth/callback/page.test.tsx new file mode 100644 index 000000000..c35e14fb4 --- /dev/null +++ b/frontend/app/auth/callback/page.test.tsx @@ -0,0 +1,84 @@ +import { screen, waitFor } from "@testing-library/react"; +import { HttpResponse, http } from "msw"; +import { beforeEach, describe, expect, it } from "vitest"; +import { renderWithProviders } from "@/test-utils/render"; +import { mockRouter, setMockLocation } from "@/test-utils/router"; +import AuthCallbackPage from "./page"; + +let callbackRequests = 0; +const callbackHandler = http.post("/api/auth/callback", () => { + callbackRequests += 1; + return HttpResponse.json({ purpose: "app_auth" }); +}); + +function setCallbackLocation(code: string) { + setMockLocation({ + pathname: "/auth/callback", + searchParams: { code, state: "test-state" }, + }); +} + +describe("AuthCallbackPage", () => { + beforeEach(() => { + callbackRequests = 0; + localStorage.setItem("auth_purpose", "app_auth"); + }); + + // `reactStrictMode` wraps the whole tree, providers included. A + // nested inside the provider wrapper does not double-invoke effects, so it + // would not reproduce the dev-mode mount/unmount/remount. + it("finishes sign-in under StrictMode's effect double-invoke", async () => { + // A fresh code per test: the page dedupes exchanges by code for the + // lifetime of the module, as the browser would within one page load. + setCallbackLocation(crypto.randomUUID()); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + reactStrictMode: true, + }); + + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + await waitFor(() => expect(mockRouter.push).toHaveBeenCalledWith("/chat"), { + timeout: 3000, + }); + expect(callbackRequests).toBe(1); + }); + + it("shows the backend's error and allows a retry when the exchange fails", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + + renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", () => + HttpResponse.json({ error: "invalid_grant" }, { status: 400 }), + ), + ], + reactStrictMode: true, + }); + + expect(await screen.findByText("Sign In Failed")).toBeInTheDocument(); + expect(screen.getByText("invalid_grant")).toBeInTheDocument(); + expect(sessionStorage.getItem(`callback_processed_${code}`)).toBeNull(); + }); + + it("shows an error instead of spinning when the code was already used", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + // A reload of the callback URL: the earlier load marked the code used. + sessionStorage.setItem(`callback_processed_${code}`, "true"); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + + expect(await screen.findByText("Sign In Failed")).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /back to login/i }), + ).toBeInTheDocument(); + expect(callbackRequests).toBe(0); + }); +}); diff --git a/frontend/app/auth/callback/page.tsx b/frontend/app/auth/callback/page.tsx index 15209186d..4a3e5bce6 100644 --- a/frontend/app/auth/callback/page.tsx +++ b/frontend/app/auth/callback/page.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { useQueryClient } from "@tanstack/react-query"; import { ArrowLeft, CheckCircle, Loader2, XCircle } from "lucide-react"; import { useRouter, useSearchParams } from "next/navigation"; import { @@ -31,6 +31,16 @@ function cleanupOAuthStorage() { localStorage.removeItem("auth_redirect_to"); } +type CallbackResult = { purpose?: string }; + +// OAuth codes are single-use, so each code is exchanged at most once per page +// load. Keeping the promise (not just a "seen" flag) lets a re-run of the +// effect, such as StrictMode's dev remount, wait on the exchange already in +// flight instead of abandoning it. +const exchanges = new Map>(); + +const callbackKeyFor = (code: string) => `callback_processed_${code}`; + function AuthCallbackContent() { const router = useRouter(); const searchParams = useSearchParams(); @@ -70,33 +80,27 @@ function AuthCallbackContent() { : null, ); + // Marked used by an earlier load of this URL (a reload or the back button) + // with no exchange in flight here. The code cannot be exchanged again. + const [codeAlreadyUsed] = useState( + () => + typeof window !== "undefined" && + !!code && + !exchanges.has(code) && + !!sessionStorage.getItem(callbackKeyFor(code)), + ); + const validationError = errorParam ? `OAuth error: ${errorParam}` : !code || !state || !finalConnectorId ? "Missing required parameters for OAuth callback" - : null; + : codeAlreadyUsed + ? "This sign-in link has already been used. Please try again." + : null; - const { mutate: exchangeCode, ...callbackMutation } = useMutation({ - mutationFn: async (params: { - connectionId: string; - code: string; - state: string; - }) => { - const response = await apiClient.post("/auth/callback", { - connection_id: params.connectionId, - authorization_code: params.code, - state: params.state, - }); - const result = response.data; - if (response.status < 200 || response.status >= 300) { - throw new Error(result.error || "Authentication failed"); - } - return result as { purpose?: string }; - }, - onSettled: () => { - queryClient.invalidateQueries({ queryKey: ["connectors"], exact: false }); - }, - }); + const [exchangeResult, setExchangeResult] = useState< + { status: "success" } | { status: "error"; error: string } | null + >(null); useEffect(() => { if (validationError) { @@ -104,10 +108,8 @@ function AuthCallbackContent() { return; } - const callbackKey = `callback_processed_${code}`; - - if (sessionStorage.getItem(callbackKey)) return; - sessionStorage.setItem(callbackKey, "true"); + const callbackKey = callbackKeyFor(code!); + let active = true; let parsedConnectionId = finalConnectorId!; @@ -135,53 +137,89 @@ function AuthCallbackContent() { } } - exchangeCode( - { connectionId: parsedConnectionId, code: code!, state: state! }, - { - onSuccess: async (result) => { - if (result.purpose === "app_auth" || purpose === "app_auth") { - await refreshAuth(); + let exchange = exchanges.get(code!); + if (!exchange) { + sessionStorage.setItem(callbackKey, "true"); + exchange = apiClient + .post("/auth/callback", { + connection_id: parsedConnectionId, + authorization_code: code, + state, + }) + .then((response) => { + const result = response.data; + if (response.status < 200 || response.status >= 300) { + throw new Error(result.error || "Authentication failed"); + } + return result as CallbackResult; + }) + .finally(() => { + queryClient.invalidateQueries({ + queryKey: ["connectors"], + exact: false, + }); + }); + exchanges.set(code!, exchange); + } - const redirectTo = - localStorage.getItem("auth_redirect_to") || - searchParams.get("redirect") || - "/chat"; + // Only the latest run of this effect reacts to the result; an earlier, + // cleaned-up run must not redirect or set state. + exchange.then( + async (result) => { + if (!active) return; + setExchangeResult({ status: "success" }); + if (result.purpose === "app_auth" || purpose === "app_auth") { + await refreshAuth(); + if (!active) return; + + const redirectTo = + localStorage.getItem("auth_redirect_to") || + searchParams.get("redirect") || + "/chat"; - cleanupOAuthStorage(); - redirectTimeoutRef.current = setTimeout( - () => router.push(redirectTo), - 2000, - ); - } else if (result.purpose === "test" || purpose === "test") { - // Test Connection — token exchange succeeded, credentials work. No - // connection was persisted (see _handle_test_auth); send the admin - // back to Connector Settings with a success indicator to toast. - cleanupOAuthStorage(); - localStorage.removeItem("test_connection_return_tab"); - redirectTimeoutRef.current = setTimeout( - () => - router.push( - `/settings/connector-access?oauth_test=success&credential=${encodeURIComponent(storedConnectorType || "")}`, - ), - 1000, - ); - } else { - cleanupOAuthStorage(); - redirectTimeoutRef.current = setTimeout( - () => router.push("/settings?oauth_success=true"), - 2000, - ); - } - }, - onError: () => { + cleanupOAuthStorage(); + redirectTimeoutRef.current = setTimeout( + () => router.push(redirectTo), + 2000, + ); + } else if (result.purpose === "test" || purpose === "test") { + // Test Connection — token exchange succeeded, credentials work. No + // connection was persisted (see _handle_test_auth); send the admin + // back to Connector Settings with a success indicator to toast. cleanupOAuthStorage(); localStorage.removeItem("test_connection_return_tab"); - sessionStorage.removeItem(callbackKey); - }, + redirectTimeoutRef.current = setTimeout( + () => + router.push( + `/settings/connector-access?oauth_test=success&credential=${encodeURIComponent(storedConnectorType || "")}`, + ), + 1000, + ); + } else { + cleanupOAuthStorage(); + redirectTimeoutRef.current = setTimeout( + () => router.push("/settings?oauth_success=true"), + 2000, + ); + } + }, + (error: unknown) => { + if (!active) return; + setExchangeResult({ + status: "error", + error: + error instanceof Error ? error.message : "Authentication failed", + }); + cleanupOAuthStorage(); + localStorage.removeItem("test_connection_return_tab"); + sessionStorage.removeItem(callbackKey); }, ); - return () => clearTimeout(redirectTimeoutRef.current); + return () => { + active = false; + clearTimeout(redirectTimeoutRef.current); + }; }, [ code, state, @@ -190,24 +228,19 @@ function AuthCallbackContent() { validationError, searchParams, isIbmAuthMode, - exchangeCode, + queryClient, refreshAuth, purpose, router, ]); - const status: "processing" | "success" | "error" = - validationError || callbackMutation.isError - ? "error" - : callbackMutation.isSuccess - ? "success" - : "processing"; + const status: "processing" | "success" | "error" = validationError + ? "error" + : (exchangeResult?.status ?? "processing"); const error = validationError || - (callbackMutation.error instanceof Error - ? callbackMutation.error.message - : null); + (exchangeResult?.status === "error" ? exchangeResult.error : null); const isAppAuth = mounted && purpose === "app_auth"; const isTest = mounted && purpose === "test"; From 18b518a4571e3e92b3bf7392d2eb53c320478a09 Mon Sep 17 00:00:00 2001 From: Mike Fortman Date: Fri, 23 Oct 2026 15:29:00 -0500 Subject: [PATCH 2/3] fix: keep the auth callback redirect target across effect re-runs Read auth_redirect_to once on mount: cleanupOAuthStorage() clears it, so a re-run of the effect after cleanup fell back to /chat. Add tests for the stored redirect target and for a remount reusing a settled exchange, and trim comments. --- frontend/app/auth/callback/page.test.tsx | 42 ++++++++++++++++++++---- frontend/app/auth/callback/page.tsx | 28 ++++++++-------- 2 files changed, 49 insertions(+), 21 deletions(-) diff --git a/frontend/app/auth/callback/page.test.tsx b/frontend/app/auth/callback/page.test.tsx index c35e14fb4..f731b302d 100644 --- a/frontend/app/auth/callback/page.test.tsx +++ b/frontend/app/auth/callback/page.test.tsx @@ -24,12 +24,8 @@ describe("AuthCallbackPage", () => { localStorage.setItem("auth_purpose", "app_auth"); }); - // `reactStrictMode` wraps the whole tree, providers included. A - // nested inside the provider wrapper does not double-invoke effects, so it - // would not reproduce the dev-mode mount/unmount/remount. + // A inside the provider wrapper doesn't double-invoke effects. it("finishes sign-in under StrictMode's effect double-invoke", async () => { - // A fresh code per test: the page dedupes exchanges by code for the - // lifetime of the module, as the browser would within one page load. setCallbackLocation(crypto.randomUUID()); renderWithProviders(, { @@ -45,6 +41,41 @@ describe("AuthCallbackPage", () => { expect(callbackRequests).toBe(1); }); + it("reuses a settled exchange when the page remounts with the same code", async () => { + setCallbackLocation(crypto.randomUUID()); + + const first = renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + first.unmount(); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + expect(callbackRequests).toBe(1); + }); + + it("redirects to auth_redirect_to after sign-in", async () => { + setCallbackLocation(crypto.randomUUID()); + localStorage.setItem("auth_redirect_to", "/knowledge"); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + reactStrictMode: true, + }); + + await waitFor( + () => expect(mockRouter.push).toHaveBeenCalledWith("/knowledge"), + { timeout: 3000 }, + ); + expect(localStorage.getItem("auth_redirect_to")).toBeNull(); + }); + it("shows the backend's error and allows a retry when the exchange fails", async () => { const code = crypto.randomUUID(); setCallbackLocation(code); @@ -67,7 +98,6 @@ describe("AuthCallbackPage", () => { it("shows an error instead of spinning when the code was already used", async () => { const code = crypto.randomUUID(); setCallbackLocation(code); - // A reload of the callback URL: the earlier load marked the code used. sessionStorage.setItem(`callback_processed_${code}`, "true"); renderWithProviders(, { diff --git a/frontend/app/auth/callback/page.tsx b/frontend/app/auth/callback/page.tsx index 4a3e5bce6..a11e709cd 100644 --- a/frontend/app/auth/callback/page.tsx +++ b/frontend/app/auth/callback/page.tsx @@ -33,10 +33,8 @@ function cleanupOAuthStorage() { type CallbackResult = { purpose?: string }; -// OAuth codes are single-use, so each code is exchanged at most once per page -// load. Keeping the promise (not just a "seen" flag) lets a re-run of the -// effect, such as StrictMode's dev remount, wait on the exchange already in -// flight instead of abandoning it. +// One exchange per single-use code, kept after settling so re-runs and remounts +// reuse it instead of POSTing the spent code again. const exchanges = new Map>(); const callbackKeyFor = (code: string) => `callback_processed_${code}`; @@ -79,9 +77,16 @@ function AuthCallbackContent() { ? localStorage.getItem("connecting_connector_type") : null, ); + // Read once: cleanupOAuthStorage() clears auth_redirect_to. + const [appAuthRedirectTo] = useState( + () => + (typeof window !== "undefined" && + localStorage.getItem("auth_redirect_to")) || + searchParams.get("redirect") || + "/chat", + ); - // Marked used by an earlier load of this URL (a reload or the back button) - // with no exchange in flight here. The code cannot be exchanged again. + // Reload / back button: an earlier load spent the code and nothing is in flight. const [codeAlreadyUsed] = useState( () => typeof window !== "undefined" && @@ -162,8 +167,6 @@ function AuthCallbackContent() { exchanges.set(code!, exchange); } - // Only the latest run of this effect reacts to the result; an earlier, - // cleaned-up run must not redirect or set state. exchange.then( async (result) => { if (!active) return; @@ -172,14 +175,9 @@ function AuthCallbackContent() { await refreshAuth(); if (!active) return; - const redirectTo = - localStorage.getItem("auth_redirect_to") || - searchParams.get("redirect") || - "/chat"; - cleanupOAuthStorage(); redirectTimeoutRef.current = setTimeout( - () => router.push(redirectTo), + () => router.push(appAuthRedirectTo), 2000, ); } else if (result.purpose === "test" || purpose === "test") { @@ -226,7 +224,7 @@ function AuthCallbackContent() { finalConnectorId, storedConnectorType, validationError, - searchParams, + appAuthRedirectTo, isIbmAuthMode, queryClient, refreshAuth, From 596341c2b1ed6242503f518aad3c0c24db33c7b9 Mon Sep 17 00:00:00 2001 From: Mike Fortman Date: Tue, 27 Oct 2026 08:37:47 -0500 Subject: [PATCH 3/3] fix: clear auth callback dedupe state when the exchange fails Clean up the exchanges entry and sessionStorage key in the request's own rejection path, so a failure that lands after the page unmounts isn't later reported as an already-used code. --- frontend/app/auth/callback/page.test.tsx | 39 ++++++++++++++++++++++++ frontend/app/auth/callback/page.tsx | 8 ++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/frontend/app/auth/callback/page.test.tsx b/frontend/app/auth/callback/page.test.tsx index f731b302d..516371468 100644 --- a/frontend/app/auth/callback/page.test.tsx +++ b/frontend/app/auth/callback/page.test.tsx @@ -95,6 +95,45 @@ describe("AuthCallbackPage", () => { expect(sessionStorage.getItem(`callback_processed_${code}`)).toBeNull(); }); + it("clears the dedupe state when the exchange fails after unmount", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + let failRequest!: () => void; + const requestFailed = new Promise((resolve) => { + failRequest = resolve; + }); + + const view = renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", async () => { + callbackRequests += 1; + await requestFailed; + return HttpResponse.json({ error: "invalid_grant" }, { status: 400 }); + }), + ], + }); + await waitFor(() => expect(callbackRequests).toBe(1)); + view.unmount(); + failRequest(); + + await waitFor(() => + expect(sessionStorage.getItem(`callback_processed_${code}`)).toBeNull(), + ); + + // The rejected exchange isn't reused: a remount reports the backend's error, + // not "already used". + renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", () => + HttpResponse.json({ error: "invalid_grant" }, { status: 400 }), + ), + ], + }); + expect(await screen.findByText("invalid_grant")).toBeInTheDocument(); + }); + it("shows an error instead of spinning when the code was already used", async () => { const code = crypto.randomUUID(); setCallbackLocation(code); diff --git a/frontend/app/auth/callback/page.tsx b/frontend/app/auth/callback/page.tsx index a11e709cd..dee80b598 100644 --- a/frontend/app/auth/callback/page.tsx +++ b/frontend/app/auth/callback/page.tsx @@ -158,6 +158,13 @@ function AuthCallbackContent() { } return result as CallbackResult; }) + .catch((error: unknown) => { + // Runs even if every page instance has unmounted, so a failed code + // isn't later reported as "already used". + exchanges.delete(code!); + sessionStorage.removeItem(callbackKey); + throw error; + }) .finally(() => { queryClient.invalidateQueries({ queryKey: ["connectors"], @@ -210,7 +217,6 @@ function AuthCallbackContent() { }); cleanupOAuthStorage(); localStorage.removeItem("test_connection_return_tab"); - sessionStorage.removeItem(callbackKey); }, );