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;