From fc089d8134bf870b2d3458ed9d0a5ba082c1ec9a Mon Sep 17 00:00:00 2001 From: Fran McDade <18710366+frano-m@users.noreply.github.com> Date: Wed, 10 Jun 2026 17:29:20 +1000 Subject: [PATCH 1/2] feat: loginview should honor nextauth callbackurl from the query string (middleware-friendly) (#955) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../session/useSessionActive.ts | 30 ++++++- src/nextauth/hooks/useNextAuthService.ts | 14 ++- tests/getQueryCallbackUrl.test.ts | 47 ++++++++++ tests/useNextAuthService.test.ts | 88 +++++++++++++++++++ tests/useSessionActive.test.ts | 48 ++++++++++ 5 files changed, 222 insertions(+), 5 deletions(-) create mode 100644 tests/getQueryCallbackUrl.test.ts create mode 100644 tests/useNextAuthService.test.ts diff --git a/src/hooks/authentication/session/useSessionActive.ts b/src/hooks/authentication/session/useSessionActive.ts index d42fb10d..be0e8a04 100644 --- a/src/hooks/authentication/session/useSessionActive.ts +++ b/src/hooks/authentication/session/useSessionActive.ts @@ -1,4 +1,4 @@ -import Router from "next/router"; +import Router, { useRouter } from "next/router"; import { useEffect } from "react"; import { AUTH_STATUS, AuthState } from "../../../auth/types/auth"; import { @@ -16,17 +16,43 @@ export const useSessionActive = ( ): void => { const { status: authStatus } = authState; const { status: authenticationStatus } = authenticationState; + const { query } = useRouter(); const { callbackUrl } = useRouteHistory(2); const isReady = authenticationStatus === AUTHENTICATION_STATUS.SETTLED && authStatus === AUTH_STATUS.SETTLED; + const queryCallbackUrl = getQueryCallbackUrl(query.callbackUrl); useEffect(() => { if (!isReady) return; - Router.push(callbackUrl(transformRoute)); + Router.push(queryCallbackUrl ?? callbackUrl(transformRoute)); + // eslint-disable-next-line react-hooks/exhaustive-deps -- queryCallbackUrl is intentionally omitted: this effect is a one-shot post-settle redirect, capturing the URL at first settle rather than re-firing each time the query changes. }, [callbackUrl, isReady]); }; +/** + * Extracts a same-origin `callbackUrl` path from a Next.js router query value. + * Next middleware (`withAuth`) appends `?callbackUrl=` when it + * redirects an unauthenticated request to the sign-in page; honoring this + * keeps the consumer aligned with the standard NextAuth + middleware contract. + * + * Only accepts same-origin paths (must start with `/` and not `//`). Rejects + * absolute URLs (`https://...`) and protocol-relative URLs (`//evil.com/x`, + * which browsers treat as absolute) so the value can be safely passed to + * `Router.push`, which does not perform its own same-origin filtering. + * + * @param value - Raw `callbackUrl` query value (string, string[], or undefined). + * @returns The same-origin path, or `undefined` when missing or unsafe. + */ +export function getQueryCallbackUrl( + value: string | string[] | undefined, +): string | undefined { + const raw = Array.isArray(value) ? value[0] : value; + if (typeof raw !== "string" || raw.length === 0) return undefined; + if (!raw.startsWith("/") || raw.startsWith("//")) return undefined; + return raw; +} + /** * Finds the most recent route in the history that is not the login route and removes the inactivity timeout query parameter. * The inactivity timeout query parameter is appended to a URL to indicate that a session has expired. This function iterates diff --git a/src/nextauth/hooks/useNextAuthService.ts b/src/nextauth/hooks/useNextAuthService.ts index 8b88f5d4..432ea4b4 100644 --- a/src/nextauth/hooks/useNextAuthService.ts +++ b/src/nextauth/hooks/useNextAuthService.ts @@ -1,7 +1,11 @@ +import { useRouter } from "next/router"; import { useCallback } from "react"; import { Service } from "../../auth/types/auth"; import { ProviderId } from "../../auth/types/common"; -import { transformRoute } from "../../hooks/authentication/session/useSessionActive"; +import { + getQueryCallbackUrl, + transformRoute, +} from "../../hooks/authentication/session/useSessionActive"; import { useRouteHistory } from "../../hooks/useRouteHistory"; import { service } from "../service"; @@ -10,13 +14,17 @@ import { service } from "../service"; * @returns auth service. */ export const useNextAuthService = (): Service => { + const { query } = useRouter(); const { callbackUrl } = useRouteHistory(2); + const queryCallbackUrl = getQueryCallbackUrl(query.callbackUrl); const onLogin = useCallback( (providerId: ProviderId) => { - service.login(providerId, { callbackUrl: callbackUrl(transformRoute) }); + service.login(providerId, { + callbackUrl: queryCallbackUrl ?? callbackUrl(transformRoute), + }); }, - [callbackUrl], + [callbackUrl, queryCallbackUrl], ); const onLogout = useCallback( diff --git a/tests/getQueryCallbackUrl.test.ts b/tests/getQueryCallbackUrl.test.ts new file mode 100644 index 00000000..77db3e9a --- /dev/null +++ b/tests/getQueryCallbackUrl.test.ts @@ -0,0 +1,47 @@ +import { getQueryCallbackUrl } from "../src/hooks/authentication/session/useSessionActive"; + +describe("getQueryCallbackUrl", () => { + it("returns the string when query is a non-empty same-origin path", () => { + expect(getQueryCallbackUrl("/atlases")).toBe("/atlases"); + }); + + it("preserves query string and fragment in the returned path", () => { + expect(getQueryCallbackUrl("/atlases?tab=metadata#row-1")).toBe( + "/atlases?tab=metadata#row-1", + ); + }); + + it("returns the first element when query is a string array", () => { + expect(getQueryCallbackUrl(["/atlases", "/other"])).toBe("/atlases"); + }); + + it("returns undefined when query is undefined", () => { + expect(getQueryCallbackUrl(undefined)).toBeUndefined(); + }); + + it("returns undefined when query is an empty string", () => { + expect(getQueryCallbackUrl("")).toBeUndefined(); + }); + + it("returns undefined when query is an empty array", () => { + expect(getQueryCallbackUrl([])).toBeUndefined(); + }); + + it("returns undefined when the first array element is empty", () => { + expect(getQueryCallbackUrl(["", "/other"])).toBeUndefined(); + }); + + it("rejects absolute http URLs (open-redirect guard)", () => { + expect(getQueryCallbackUrl("https://evil.example/phish")).toBeUndefined(); + expect(getQueryCallbackUrl("http://evil.example/phish")).toBeUndefined(); + }); + + it("rejects protocol-relative URLs (browsers treat as absolute)", () => { + expect(getQueryCallbackUrl("//evil.example/phish")).toBeUndefined(); + }); + + it("rejects values that do not start with '/'", () => { + expect(getQueryCallbackUrl("atlases")).toBeUndefined(); + expect(getQueryCallbackUrl("javascript:alert(1)")).toBeUndefined(); + }); +}); diff --git a/tests/useNextAuthService.test.ts b/tests/useNextAuthService.test.ts new file mode 100644 index 00000000..3e460201 --- /dev/null +++ b/tests/useNextAuthService.test.ts @@ -0,0 +1,88 @@ +import { jest } from "@jest/globals"; +import { act, renderHook } from "@testing-library/react"; +import { TransformRouteFn } from "../src/hooks/useRouteHistory"; + +const ROOT_PATH = "/"; +const ROUTES = ["/login", "/route1", "/route2"]; +const PROVIDER_ID = "google"; + +let mockRouterQuery: Record = {}; + +jest.unstable_mockModule("next/router", () => ({ + ...jest.requireActual("next/router"), + useRouter: jest.fn(() => ({ query: mockRouterQuery })), +})); +jest.unstable_mockModule("../src/hooks/useRouteHistory", () => ({ + useRouteHistory: jest.fn(), +})); +jest.unstable_mockModule("../src/nextauth/service", () => ({ + service: { login: jest.fn(), logout: jest.fn() }, +})); + +const { useRouteHistory } = await import("../src/hooks/useRouteHistory"); +const { service } = await import("../src/nextauth/service"); +const { useNextAuthService } = + await import("../src/nextauth/hooks/useNextAuthService"); + +const MOCK_USE_ROUTE_HISTORY = useRouteHistory as jest.MockedFunction< + typeof useRouteHistory +>; +const MOCK_LOGIN = service.login as jest.MockedFunction; + +describe("useNextAuthService", () => { + beforeEach(() => { + mockRouterQuery = {}; + MOCK_LOGIN.mockReset(); + MOCK_USE_ROUTE_HISTORY.mockReset(); + MOCK_USE_ROUTE_HISTORY.mockReturnValue({ + callbackUrl: jest.fn( + (transformFn?: TransformRouteFn | undefined) => + transformFn?.(ROUTES) ?? ROOT_PATH, + ), + }); + }); + + test("requestLogin uses route-history fallback when query.callbackUrl is absent", () => { + const { result } = renderHook(() => useNextAuthService()); + act(() => result.current.requestLogin(PROVIDER_ID)); + expect(MOCK_LOGIN).toHaveBeenCalledWith(PROVIDER_ID, { + callbackUrl: ROUTES[1], + }); + }); + + test("requestLogin prefers a same-origin query.callbackUrl over route history", () => { + mockRouterQuery = { callbackUrl: "/from-query" }; + const { result } = renderHook(() => useNextAuthService()); + act(() => result.current.requestLogin(PROVIDER_ID)); + expect(MOCK_LOGIN).toHaveBeenCalledWith(PROVIDER_ID, { + callbackUrl: "/from-query", + }); + }); + + test("requestLogin falls back to route history when query.callbackUrl is empty", () => { + mockRouterQuery = { callbackUrl: "" }; + const { result } = renderHook(() => useNextAuthService()); + act(() => result.current.requestLogin(PROVIDER_ID)); + expect(MOCK_LOGIN).toHaveBeenCalledWith(PROVIDER_ID, { + callbackUrl: ROUTES[1], + }); + }); + + test("requestLogin rejects an absolute URL in query.callbackUrl (open-redirect guard)", () => { + mockRouterQuery = { callbackUrl: "https://evil.example/phish" }; + const { result } = renderHook(() => useNextAuthService()); + act(() => result.current.requestLogin(PROVIDER_ID)); + expect(MOCK_LOGIN).toHaveBeenCalledWith(PROVIDER_ID, { + callbackUrl: ROUTES[1], + }); + }); + + test("requestLogin rejects a protocol-relative URL in query.callbackUrl", () => { + mockRouterQuery = { callbackUrl: "//evil.example/phish" }; + const { result } = renderHook(() => useNextAuthService()); + act(() => result.current.requestLogin(PROVIDER_ID)); + expect(MOCK_LOGIN).toHaveBeenCalledWith(PROVIDER_ID, { + callbackUrl: ROUTES[1], + }); + }); +}); diff --git a/tests/useSessionActive.test.ts b/tests/useSessionActive.test.ts index 8c000902..39a2f03b 100644 --- a/tests/useSessionActive.test.ts +++ b/tests/useSessionActive.test.ts @@ -35,12 +35,15 @@ const AUTHENTICATION_STATE_SETTLED: AuthenticationState = { const ROOT_PATH = "/"; const ROUTES = ["/login", "/route1", "/route2"]; +let mockRouterQuery: Record = {}; + jest.unstable_mockModule("next/router", () => { return { ...jest.requireActual("next/router"), default: { push: jest.fn(), }, + useRouter: jest.fn(() => ({ query: mockRouterQuery })), }; }); jest.unstable_mockModule("../src/hooks/useRouteHistory", () => ({ @@ -58,6 +61,7 @@ const MOCK_USE_ROUTE_HISTORY = useRouteHistory as jest.MockedFunction< describe("useSessionActive", () => { beforeEach(() => { + mockRouterQuery = {}; MOCK_USE_ROUTE_HISTORY.mockReset(); MOCK_USE_ROUTE_HISTORY.mockReturnValue({ callbackUrl: jest.fn( @@ -100,4 +104,48 @@ describe("useSessionActive", () => { ); expect(Router.push).toHaveBeenCalledWith(ROUTES[1]); }); + + test("prefers router.query.callbackUrl over route-history fallback", () => { + mockRouterQuery = { callbackUrl: "/from-query" }; + renderHook(() => + useSessionActive( + AUTH_STATE_AUTHENTICATED_SETTLED, + AUTHENTICATION_STATE_SETTLED, + ), + ); + expect(Router.push).toHaveBeenCalledWith("/from-query"); + }); + + test("falls back to route history when router.query.callbackUrl is an empty string", () => { + mockRouterQuery = { callbackUrl: "" }; + renderHook(() => + useSessionActive( + AUTH_STATE_AUTHENTICATED_SETTLED, + AUTHENTICATION_STATE_SETTLED, + ), + ); + expect(Router.push).toHaveBeenCalledWith(ROUTES[1]); + }); + + test("falls back to route history when router.query.callbackUrl is an absolute URL (open-redirect guard)", () => { + mockRouterQuery = { callbackUrl: "https://evil.example/phish" }; + renderHook(() => + useSessionActive( + AUTH_STATE_AUTHENTICATED_SETTLED, + AUTHENTICATION_STATE_SETTLED, + ), + ); + expect(Router.push).toHaveBeenCalledWith(ROUTES[1]); + }); + + test("falls back to route history when router.query.callbackUrl is protocol-relative", () => { + mockRouterQuery = { callbackUrl: "//evil.example/phish" }; + renderHook(() => + useSessionActive( + AUTH_STATE_AUTHENTICATED_SETTLED, + AUTHENTICATION_STATE_SETTLED, + ), + ); + expect(Router.push).toHaveBeenCalledWith(ROUTES[1]); + }); }); From 3ff991047b756f298619c5eca97e3934b15c1608 Mon Sep 17 00:00:00 2001 From: Fran McDade <18710366+frano-m@users.noreply.github.com> Date: Wed, 10 Jun 2026 17:40:12 +1000 Subject: [PATCH 2/2] test: clear all mocks in beforeEach so hook tests are reorder-safe (#955) Addresses copilot review feedback on PR #959: Router.push and other module-level jest.fn() mocks weren't being cleared between tests, which made assertions order-dependent. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/useNextAuthService.test.ts | 1 + tests/useSessionActive.test.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/useNextAuthService.test.ts b/tests/useNextAuthService.test.ts index 3e460201..4e22fe4d 100644 --- a/tests/useNextAuthService.test.ts +++ b/tests/useNextAuthService.test.ts @@ -31,6 +31,7 @@ const MOCK_LOGIN = service.login as jest.MockedFunction; describe("useNextAuthService", () => { beforeEach(() => { + jest.clearAllMocks(); mockRouterQuery = {}; MOCK_LOGIN.mockReset(); MOCK_USE_ROUTE_HISTORY.mockReset(); diff --git a/tests/useSessionActive.test.ts b/tests/useSessionActive.test.ts index 39a2f03b..a338e904 100644 --- a/tests/useSessionActive.test.ts +++ b/tests/useSessionActive.test.ts @@ -61,6 +61,7 @@ const MOCK_USE_ROUTE_HISTORY = useRouteHistory as jest.MockedFunction< describe("useSessionActive", () => { beforeEach(() => { + jest.clearAllMocks(); mockRouterQuery = {}; MOCK_USE_ROUTE_HISTORY.mockReset(); MOCK_USE_ROUTE_HISTORY.mockReturnValue({