From 0568f1c1ebc54d8901ad3c83c7740aae19049f69 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 26 Feb 2026 16:10:14 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=83=20fix:=20Prevent=20Recursive=20Log?= =?UTF-8?q?in=20Redirect=20Loop=20(#11964)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Prevent recursive login redirect loop buildLoginRedirectUrl() would blindly encode the current URL into a redirect_to param even when already on /login, causing infinite nesting (/login?redirect_to=%2Flogin%3Fredirect_to%3D...). Guard at source so it returns plain /login when pathname starts with /login. Also validates redirect_to in the login error handler with isSafeRedirect to close an open-redirect vector, and removes a redundant /login guard from useAuthRedirect now handled by the centralized check. * 🔀 fix: Handle basename-prefixed login paths and remove double URL decoding buildLoginRedirectUrl now uses isLoginPath() which matches /login, /librechat/login, and /login/2fa — covering subdirectory deployments where window.location.pathname includes the basename prefix. Remove redundant decodeURIComponent calls on URLSearchParams.get() results (which already returns decoded values) in getPostLoginRedirect, Login.tsx, and AuthContext login error handler. The extra decode could throw URIError on inputs containing literal percent signs. * 🔀 fix: Tighten login path matching and add onError redirect tests Replace overbroad `endsWith('/login')` with a single regex `/(^|\/)login(\/|$)/` that matches `/login` only as a full path segment. Unifies `isSafeRedirect` and `buildLoginRedirectUrl` to use the same `LOGIN_PATH_RE` constant — no more divergent definitions. Add tests for the AuthContext onError redirect_to preservation logic (valid path preserved, open-redirect blocked, /login loop blocked), and a false-positive guard proving `/c/loginhistory` is not matched. Update JSDoc on `buildLoginRedirectUrl` to document the plain `/login` early-return, and add explanatory comment in AuthContext `onError` for why `buildLoginRedirectUrl()` cannot be used there. * test: Add unit tests for AuthContextProvider login error handling Introduced a new test suite for AuthContextProvider to validate the handling of login errors and the preservation of redirect parameters. The tests cover various scenarios including valid redirect preservation, open-redirect prevention, and recursive redirect prevention. This enhances the robustness of the authentication flow and ensures proper navigation behavior during login failures. --- client/src/components/Auth/Login.tsx | 2 +- client/src/hooks/AuthContext.tsx | 8 +- .../src/hooks/__tests__/AuthContext.spec.tsx | 174 ++++++++++++++++++ client/src/routes/useAuthRedirect.ts | 5 - client/src/utils/__tests__/redirect.test.ts | 69 +++++++ client/src/utils/redirect.ts | 16 +- 6 files changed, 263 insertions(+), 11 deletions(-) create mode 100644 client/src/hooks/__tests__/AuthContext.spec.tsx diff --git a/client/src/components/Auth/Login.tsx b/client/src/components/Auth/Login.tsx index e0bf89bacd..7c3adf51bd 100644 --- a/client/src/components/Auth/Login.tsx +++ b/client/src/components/Auth/Login.tsx @@ -29,7 +29,7 @@ function Login() { useEffect(() => { const redirectTo = searchParams.get('redirect_to'); if (redirectTo) { - persistRedirectToSession(decodeURIComponent(redirectTo)); + persistRedirectToSession(redirectTo); } else { const state = location.state as LoginLocationState | null; if (state?.redirect_to) { diff --git a/client/src/hooks/AuthContext.tsx b/client/src/hooks/AuthContext.tsx index 04bc3445c9..ca69a68f8b 100644 --- a/client/src/hooks/AuthContext.tsx +++ b/client/src/hooks/AuthContext.tsx @@ -93,8 +93,14 @@ const AuthContextProvider = ({ onError: (error: TResError | unknown) => { const resError = error as TResError; doSetError(resError.message); + // Preserve a valid redirect_to across login failures so the deep link survives retries. + // Cannot use buildLoginRedirectUrl() here — it reads the current pathname (already /login) + // and would return plain /login, dropping the redirect_to destination. const redirectTo = new URLSearchParams(window.location.search).get('redirect_to'); - const loginPath = redirectTo ? `/login?redirect_to=${redirectTo}` : '/login'; + const loginPath = + redirectTo && isSafeRedirect(redirectTo) + ? `/login?redirect_to=${encodeURIComponent(redirectTo)}` + : '/login'; navigate(loginPath, { replace: true }); }, }); diff --git a/client/src/hooks/__tests__/AuthContext.spec.tsx b/client/src/hooks/__tests__/AuthContext.spec.tsx new file mode 100644 index 0000000000..5a24a31ec4 --- /dev/null +++ b/client/src/hooks/__tests__/AuthContext.spec.tsx @@ -0,0 +1,174 @@ +import React from 'react'; +import { render, act } from '@testing-library/react'; +import { RecoilRoot } from 'recoil'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { MemoryRouter } from 'react-router-dom'; + +import type { TAuthConfig } from '~/common'; + +import { AuthContextProvider, useAuthContext } from '../AuthContext'; + +const mockNavigate = jest.fn(); +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useNavigate: () => mockNavigate, +})); + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), + setTokenHeader: jest.fn(), +})); + +let mockCapturedLoginOptions: { + onSuccess: (...args: unknown[]) => void; + onError: (...args: unknown[]) => void; +}; + +jest.mock('~/data-provider', () => ({ + useLoginUserMutation: jest.fn( + (options: { + onSuccess: (...args: unknown[]) => void; + onError: (...args: unknown[]) => void; + }) => { + mockCapturedLoginOptions = options; + return { mutate: jest.fn() }; + }, + ), + useLogoutUserMutation: jest.fn(() => ({ mutate: jest.fn() })), + useRefreshTokenMutation: jest.fn(() => ({ mutate: jest.fn() })), + useGetUserQuery: jest.fn(() => ({ + data: undefined, + isError: false, + error: null, + })), + useGetRole: jest.fn(() => ({ data: null })), +})); + +const authConfig: TAuthConfig = { loginRedirect: '/login', test: true }; + +function TestConsumer() { + const ctx = useAuthContext(); + return
; +} + +function renderProvider() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + + return render( + + + + + + + + + , + ); +} + +describe('AuthContextProvider — login onError redirect handling', () => { + const originalLocation = window.location; + + beforeEach(() => { + jest.clearAllMocks(); + Object.defineProperty(window, 'location', { + value: { ...originalLocation, pathname: '/login', search: '', hash: '' }, + writable: true, + configurable: true, + }); + }); + + afterEach(() => { + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + configurable: true, + }); + }); + + it('preserves a valid redirect_to param across login failure', () => { + Object.defineProperty(window, 'location', { + value: { pathname: '/login', search: '?redirect_to=%2Fc%2Fabc123', hash: '' }, + writable: true, + configurable: true, + }); + + renderProvider(); + + act(() => { + mockCapturedLoginOptions.onError({ message: 'Invalid credentials' }); + }); + + expect(mockNavigate).toHaveBeenCalledWith('/login?redirect_to=%2Fc%2Fabc123', { + replace: true, + }); + }); + + it('drops redirect_to when it contains an absolute URL (open-redirect prevention)', () => { + Object.defineProperty(window, 'location', { + value: { pathname: '/login', search: '?redirect_to=https%3A%2F%2Fevil.com', hash: '' }, + writable: true, + configurable: true, + }); + + renderProvider(); + + act(() => { + mockCapturedLoginOptions.onError({ message: 'Invalid credentials' }); + }); + + expect(mockNavigate).toHaveBeenCalledWith('/login', { replace: true }); + }); + + it('drops redirect_to when it points to /login (recursive redirect prevention)', () => { + Object.defineProperty(window, 'location', { + value: { pathname: '/login', search: '?redirect_to=%2Flogin', hash: '' }, + writable: true, + configurable: true, + }); + + renderProvider(); + + act(() => { + mockCapturedLoginOptions.onError({ message: 'Invalid credentials' }); + }); + + expect(mockNavigate).toHaveBeenCalledWith('/login', { replace: true }); + }); + + it('navigates to plain /login when no redirect_to param exists', () => { + renderProvider(); + + act(() => { + mockCapturedLoginOptions.onError({ message: 'Server error' }); + }); + + expect(mockNavigate).toHaveBeenCalledWith('/login', { replace: true }); + }); + + it('preserves redirect_to with query params and hash', () => { + const target = '/c/abc123?model=gpt-4#section'; + Object.defineProperty(window, 'location', { + value: { + pathname: '/login', + search: `?redirect_to=${encodeURIComponent(target)}`, + hash: '', + }, + writable: true, + configurable: true, + }); + + renderProvider(); + + act(() => { + mockCapturedLoginOptions.onError({ message: 'Invalid credentials' }); + }); + + const navigatedUrl = mockNavigate.mock.calls[0][0] as string; + const params = new URLSearchParams(navigatedUrl.split('?')[1]); + expect(decodeURIComponent(params.get('redirect_to')!)).toBe(target); + }); +}); diff --git a/client/src/routes/useAuthRedirect.ts b/client/src/routes/useAuthRedirect.ts index 7303952155..5508162543 100644 --- a/client/src/routes/useAuthRedirect.ts +++ b/client/src/routes/useAuthRedirect.ts @@ -14,11 +14,6 @@ export default function useAuthRedirect() { return; } - if (location.pathname.startsWith('/login')) { - navigate('/login', { replace: true }); - return; - } - navigate(buildLoginRedirectUrl(location.pathname, location.search, location.hash), { replace: true, }); diff --git a/client/src/utils/__tests__/redirect.test.ts b/client/src/utils/__tests__/redirect.test.ts index 36336b0d94..1d402d2025 100644 --- a/client/src/utils/__tests__/redirect.test.ts +++ b/client/src/utils/__tests__/redirect.test.ts @@ -100,6 +100,45 @@ describe('buildLoginRedirectUrl', () => { const result = buildLoginRedirectUrl(); expect(result).toBe('/login?redirect_to=%2F'); }); + + it('returns plain /login when pathname is /login (prevents recursive redirect)', () => { + const result = buildLoginRedirectUrl('/login', '?redirect_to=%2Fc%2Fnew', ''); + expect(result).toBe('/login'); + }); + + it('returns plain /login when window.location is already /login', () => { + Object.defineProperty(window, 'location', { + value: { pathname: '/login', search: '?redirect_to=%2Fc%2Fabc', hash: '' }, + writable: true, + }); + const result = buildLoginRedirectUrl(); + expect(result).toBe('/login'); + }); + + it('returns plain /login for /login sub-paths', () => { + const result = buildLoginRedirectUrl('/login/2fa', '', ''); + expect(result).toBe('/login'); + }); + + it('returns plain /login for basename-prefixed /login (e.g. /librechat/login)', () => { + Object.defineProperty(window, 'location', { + value: { pathname: '/librechat/login', search: '?redirect_to=%2Fc%2Fabc', hash: '' }, + writable: true, + }); + const result = buildLoginRedirectUrl(); + expect(result).toBe('/login'); + }); + + it('returns plain /login for basename-prefixed /login sub-paths', () => { + const result = buildLoginRedirectUrl('/librechat/login/2fa', '', ''); + expect(result).toBe('/login'); + }); + + it('does NOT match paths where "login" is a substring of a segment', () => { + const result = buildLoginRedirectUrl('/c/loginhistory', '', ''); + expect(result).toContain('redirect_to='); + expect(decodeURIComponent(result.split('redirect_to=')[1])).toBe('/c/loginhistory'); + }); }); describe('getPostLoginRedirect', () => { @@ -170,6 +209,36 @@ describe('getPostLoginRedirect', () => { }); }); +describe('login error redirect_to preservation (AuthContext onError pattern)', () => { + /** Mirrors the logic in AuthContext.tsx loginUser.onError */ + function buildLoginErrorPath(search: string): string { + const redirectTo = new URLSearchParams(search).get('redirect_to'); + return redirectTo && isSafeRedirect(redirectTo) + ? `/login?redirect_to=${encodeURIComponent(redirectTo)}` + : '/login'; + } + + it('preserves a valid redirect_to across login failure', () => { + const result = buildLoginErrorPath('?redirect_to=%2Fc%2Fnew'); + expect(result).toBe('/login?redirect_to=%2Fc%2Fnew'); + }); + + it('drops an open-redirect attempt (absolute URL)', () => { + const result = buildLoginErrorPath('?redirect_to=https%3A%2F%2Fevil.com'); + expect(result).toBe('/login'); + }); + + it('drops a /login redirect_to to prevent loops', () => { + const result = buildLoginErrorPath('?redirect_to=%2Flogin'); + expect(result).toBe('/login'); + }); + + it('returns plain /login when no redirect_to param exists', () => { + const result = buildLoginErrorPath(''); + expect(result).toBe('/login'); + }); +}); + describe('persistRedirectToSession', () => { beforeEach(() => { sessionStorage.clear(); diff --git a/client/src/utils/redirect.ts b/client/src/utils/redirect.ts index d2b7588151..1fb4e66d41 100644 --- a/client/src/utils/redirect.ts +++ b/client/src/utils/redirect.ts @@ -1,18 +1,27 @@ const REDIRECT_PARAM = 'redirect_to'; const SESSION_KEY = 'post_login_redirect_to'; +/** Matches `/login` as a full path segment, with optional basename prefix (e.g. `/librechat/login/2fa`) */ +const LOGIN_PATH_RE = /(?:^|\/)login(?:\/|$)/; + /** Validates that a redirect target is a safe relative path (not an absolute or protocol-relative URL) */ function isSafeRedirect(url: string): boolean { if (!url.startsWith('/') || url.startsWith('//')) { return false; } const path = url.split('?')[0].split('#')[0]; - return !path.startsWith('/login'); + return !LOGIN_PATH_RE.test(path); } -/** Builds a `/login?redirect_to=...` URL, reading from window.location when no args are provided */ +/** + * Builds a `/login?redirect_to=...` URL from the given or current location. + * Returns plain `/login` (no param) when already on a login route to prevent recursive nesting. + */ function buildLoginRedirectUrl(pathname?: string, search?: string, hash?: string): string { const p = pathname ?? window.location.pathname; + if (LOGIN_PATH_RE.test(p)) { + return '/login'; + } const s = search ?? window.location.search; const h = hash ?? window.location.hash; const currentPath = `${p}${s}${h}`; @@ -25,8 +34,7 @@ function buildLoginRedirectUrl(pathname?: string, search?: string, hash?: string * cleans up both sources, and returns the validated target (or null). */ function getPostLoginRedirect(searchParams: URLSearchParams): string | null { - const encoded = searchParams.get(REDIRECT_PARAM); - const urlRedirect = encoded ? decodeURIComponent(encoded) : null; + const urlRedirect = searchParams.get(REDIRECT_PARAM); const storedRedirect = sessionStorage.getItem(SESSION_KEY); const target = urlRedirect ?? storedRedirect;