diff --git a/app/features/authentication/routes/login.tsx b/app/features/authentication/routes/login.tsx index e33521b8..22fc5ea7 100644 --- a/app/features/authentication/routes/login.tsx +++ b/app/features/authentication/routes/login.tsx @@ -3,6 +3,10 @@ import { parseWithZod } from '@conform-to/zod' import { data, Form, Link, redirect } from 'react-router' import { loginSchema } from '~/features/authentication/schemas/login.schema' import { needSetupProcess } from '~/features/authentication/server/need-setup-process.server' +import { + buildLoginRedirectUrl, + resolvePostLoginRedirect, +} from '~/features/authentication/server/post-login-redirect.server' import { checkLoginRateLimit, clearLoginAttempts, @@ -19,6 +23,7 @@ import { Button } from '~/shared/ui/button' import { Card, CardContent, CardFooter, CardHeader } from '~/shared/ui/card' import { Input } from '~/shared/ui/input' import { Label } from '~/shared/ui/label' +import { safeRedirectUrl } from '~/shared/utils/safe-redirect.server' import type { Route } from './+types/login' export const meta: Route.MetaFunction = () => { @@ -29,6 +34,8 @@ export async function loader({ request }: Route.LoaderArgs) { // Verify that the subdomain matches an existing congregation await resolveCongregationFromRequest(request) + const redirectTo = safeRedirectUrl(new URL(request.url).searchParams.get('redirectTo'), '/') + const shouldStartSetup = await needSetupProcess() if (shouldStartSetup) { return redirect(process.env.UNITAE_MULTI_TENANT === 'true' ? '/register' : '/setup') @@ -39,7 +46,7 @@ export async function loader({ request }: Route.LoaderArgs) { const uid = Number(session.get('userId')) if (Number.isNaN(uid) || uid <= 0) { return data( - { error: undefined, brandingName: await getBrandingName(request) }, + { error: undefined, brandingName: await getBrandingName(request), redirectTo }, { headers: { 'Set-Cookie': await destroySession(session) } }, ) } @@ -53,18 +60,18 @@ export async function loader({ request }: Route.LoaderArgs) { }) if (!user || user.congregationId !== urlCongregation.id) { return data( - { error: undefined, brandingName: await getBrandingName(request) }, + { error: undefined, brandingName: await getBrandingName(request), redirectTo }, { headers: { 'Set-Cookie': await destroySession(session) } }, ) } } - throw redirect('/') + throw redirect(redirectTo) } const brandingName = await getBrandingName(request) return data( - { error: session.get('error'), brandingName }, + { error: session.get('error'), brandingName, redirectTo }, { headers: { 'Set-Cookie': await commitSession(session), @@ -74,7 +81,7 @@ export async function loader({ request }: Route.LoaderArgs) { } export default function LoginPage({ loaderData, actionData }: Route.ComponentProps) { - const { error, brandingName } = loaderData + const { error, brandingName, redirectTo } = loaderData const [form, fields] = useForm({ lastResult: actionData, @@ -97,6 +104,7 @@ export default function LoginPage({ loaderData, actionData }: Route.ComponentPro )}
+
@@ -127,7 +135,10 @@ export default function LoginPage({ loaderData, actionData }: Route.ComponentPro export async function action({ request }: Route.ActionArgs) { const session = await getSession(request.headers.get('Cookie')) - const submission = parseWithZod(await request.formData(), { schema: loginSchema }) + const formData = await request.formData() + const postLoginRedirect = resolvePostLoginRedirect(request, formData) + const loginUrl = buildLoginRedirectUrl(postLoginRedirect) + const submission = parseWithZod(formData, { schema: loginSchema }) if (submission.status !== 'success') { return data(submission.reply(), { status: 400 }) @@ -139,7 +150,7 @@ export async function action({ request }: Route.ActionArgs) { if (!allowed) { session.flash('error', m.auth_login_rate_limit_error()) - return redirect('/login', { + return redirect(loginUrl, { headers: { 'Set-Cookie': await commitSession(session) }, }) } @@ -158,7 +169,7 @@ export async function action({ request }: Route.ActionArgs) { } session.flash('error', m.auth_login_invalid_credentials_error()) - return redirect('/login', { + return redirect(loginUrl, { headers: { 'Set-Cookie': await commitSession(session) }, }) } @@ -177,7 +188,7 @@ export async function action({ request }: Route.ActionArgs) { }) } - return redirect('/', { + return redirect(postLoginRedirect, { headers: { 'Set-Cookie': await commitSession(session) }, }) } diff --git a/app/features/authentication/server/post-login-redirect.server.test.ts b/app/features/authentication/server/post-login-redirect.server.test.ts new file mode 100644 index 00000000..3fcdc77d --- /dev/null +++ b/app/features/authentication/server/post-login-redirect.server.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest' +import { buildLoginRedirectUrl, resolvePostLoginRedirect } from './post-login-redirect.server' + +function makeFormData(entries: Record): FormData { + const formData = new FormData() + for (const [key, value] of Object.entries(entries)) { + formData.set(key, value) + } + return formData +} + +describe('resolvePostLoginRedirect', () => { + it("préfère la valeur du form à celle de l'URL", () => { + const request = new Request('http://localhost/login?redirectTo=%2Ffrom-url') + const formData = makeFormData({ redirectTo: '/from-form' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/from-form') + }) + + it("utilise le paramètre d'URL quand le champ de formulaire est absent", () => { + const request = new Request('http://localhost/login?redirectTo=%2Fterritories%2F1') + const formData = makeFormData({}) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/territories/1') + }) + + it("utilise le paramètre d'URL quand le champ de formulaire est vide", () => { + const request = new Request('http://localhost/login?redirectTo=%2Fterritories%2F1') + const formData = makeFormData({ redirectTo: '' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/territories/1') + }) + + it("retourne '/' quand ni le form ni l'URL ne fournissent de redirectTo", () => { + const request = new Request('http://localhost/login') + const formData = makeFormData({}) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("retourne '/' quand le paramètre d'URL redirectTo est présent mais vide", () => { + const request = new Request('http://localhost/login?redirectTo=') + const formData = makeFormData({}) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("retourne '/' quand le champ de formulaire et le paramètre d'URL sont tous les deux vides", () => { + const request = new Request('http://localhost/login?redirectTo=') + const formData = makeFormData({ redirectTo: '' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("rejette une URL protocol-relative (//evil.com) et retombe sur '/'", () => { + const request = new Request('http://localhost/login') + const formData = makeFormData({ redirectTo: '//evil.com/path' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("rejette une URL absolue (https://evil.com) et retombe sur '/'", () => { + const request = new Request('http://localhost/login') + const formData = makeFormData({ redirectTo: 'https://evil.com/path' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("rejette un chemin ne commençant pas par '/' et retombe sur '/'", () => { + const request = new Request('http://localhost/login') + const formData = makeFormData({ redirectTo: 'javascript:alert(1)' }) + + expect(resolvePostLoginRedirect(request, formData)).toBe('/') + }) + + it("retombe sur '/' quand request.url est mal formée (ne parvient pas à parser)", () => { + const badRequest = { url: 'not-a-valid-url' } as unknown as Request + const formData = makeFormData({}) + + expect(resolvePostLoginRedirect(badRequest, formData)).toBe('/') + }) + + it('privilégie la valeur du form même si request.url est mal formée', () => { + const badRequest = { url: 'not-a-valid-url' } as unknown as Request + const formData = makeFormData({ redirectTo: '/from-form' }) + + expect(resolvePostLoginRedirect(badRequest, formData)).toBe('/from-form') + }) +}) + +describe('buildLoginRedirectUrl', () => { + it("retourne '/login' sans query quand la cible est '/'", () => { + expect(buildLoginRedirectUrl('/')).toBe('/login') + }) + + it('encode la cible dans le paramètre redirectTo', () => { + expect(buildLoginRedirectUrl('/territories/1?x=2')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') + }) + + it('encode les caractères spéciaux (unicode, espaces)', () => { + expect(buildLoginRedirectUrl('/territoires/été 42')).toBe('/login?redirectTo=%2Fterritoires%2F%C3%A9t%C3%A9%2042') + }) +}) diff --git a/app/features/authentication/server/post-login-redirect.server.ts b/app/features/authentication/server/post-login-redirect.server.ts new file mode 100644 index 00000000..5a253ab4 --- /dev/null +++ b/app/features/authentication/server/post-login-redirect.server.ts @@ -0,0 +1,23 @@ +import logger from '~/shared/infra/logger.server' +import { safeRedirectUrl } from '~/shared/utils/safe-redirect.server' + +export function resolvePostLoginRedirect(request: Request, formData: FormData): string { + const formValue = formData.get('redirectTo') + if (formValue != null && typeof formValue !== 'string') { + logger.debug(`resolvePostLoginRedirect: ignoring non-string redirectTo form value (${typeof formValue})`) + } + const fromForm = typeof formValue === 'string' && formValue.length > 0 ? formValue : null + + let fromUrl: string | null = null + try { + fromUrl = new URL(request.url).searchParams.get('redirectTo') + } catch { + logger.warn(`resolvePostLoginRedirect: unable to parse request.url (${request.url})`) + } + + return safeRedirectUrl(fromForm ?? fromUrl, '/') +} + +export function buildLoginRedirectUrl(target: string): string { + return target === '/' ? '/login' : `/login?redirectTo=${encodeURIComponent(target)}` +} diff --git a/app/features/authentication/server/session.server.test.ts b/app/features/authentication/server/session.server.test.ts index 5478f103..7fe434c4 100644 --- a/app/features/authentication/server/session.server.test.ts +++ b/app/features/authentication/server/session.server.test.ts @@ -97,43 +97,44 @@ describe('verifySession', () => { expect(result.session).toBe(mockSession) }) - it('redirige vers /login si le userId est absent de la session', async () => { + it('redirige vers /login avec redirectTo si le userId est absent de la session', async () => { mockSession.get.mockReturnValue(undefined) - const response = await getRedirectResponse(() => verifySession(makeRequest())) - expect(response.headers.get('Location')).toBe('/login') + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) + expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) - it('redirige vers /login si le userId est NaN', async () => { + it('redirige vers /login avec redirectTo si le userId est NaN', async () => { mockSession.get.mockReturnValue('invalid') - const response = await getRedirectResponse(() => verifySession(makeRequest())) - expect(response.headers.get('Location')).toBe('/login') + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) + expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) - it("redirige vers /login si l'utilisateur n'existe pas", async () => { + it("redirige vers /login avec redirectTo si l'utilisateur n'existe pas", async () => { mockSession.get.mockReturnValue('42') vi.mocked(db.userAccount.findUnique).mockResolvedValue(null as never) - const response = await getRedirectResponse(() => verifySession(makeRequest())) - expect(response.headers.get('Location')).toBe('/login') + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) + expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) - it("redirige vers /login si l'utilisateur est inactif", async () => { + it("redirige vers /login avec redirectTo si l'utilisateur est inactif", async () => { mockSession.get.mockReturnValue('42') vi.mocked(db.userAccount.findUnique).mockResolvedValue({ ...fakeUser, active: false } as never) - const response = await getRedirectResponse(() => verifySession(makeRequest())) - expect(response.headers.get('Location')).toBe('/login') + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) + expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) - it("redirige vers /login si le subdomain ne correspond pas à l'assemblée de l'utilisateur", async () => { + it("redirige vers /login sans redirectTo si le subdomain ne correspond pas à l'assemblée de l'utilisateur", async () => { mockSession.get.mockReturnValue('42') vi.mocked(db.userAccount.findUnique).mockResolvedValue(fakeUser as never) vi.mocked(resolveCongregationFromRequest).mockResolvedValue({ id: 999 } as never) - const response = await getRedirectResponse(() => verifySession(makeRequest())) + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) expect(response.headers.get('Location')).toBe('/login') + expect(response.headers.get('Location')).not.toContain('redirectTo') }) it("redirige vers /suspended si l'assemblée est suspendue", async () => { @@ -165,13 +166,13 @@ describe('verifySession', () => { expect(response.headers.get('Location')).toBe('/trial-expired') }) - it('redirige vers /login si findUnique échoue avec P2007', async () => { + it('redirige vers /login avec redirectTo si findUnique échoue avec P2007', async () => { mockSession.get.mockReturnValue('42') const p2007Error = Object.assign(new Error('Type mismatch'), { code: 'P2007' }) vi.mocked(db.userAccount.findUnique).mockRejectedValue(p2007Error) - const response = await getRedirectResponse(() => verifySession(makeRequest())) - expect(response.headers.get('Location')).toBe('/login') + const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/territories/1?x=2'))) + expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) it("redirige vers /verify-email si l'email n'est pas vérifié", async () => { @@ -183,6 +184,14 @@ describe('verifySession', () => { const response = await getRedirectResponse(() => verifySession(makeRequest())) expect(response.headers.get('Location')).toBe('/verify-email') }) + + it('redirige vers /login sans redirectTo quand request.url est mal formée', async () => { + mockSession.get.mockReturnValue(undefined) + const badRequest = { url: 'not-a-valid-url', headers: { get: () => null } } as unknown as Request + + const response = await getRedirectResponse(() => verifySession(badRequest)) + expect(response.headers.get('Location')).toBe('/login') + }) }) // verifySession throws redirect responses — this helper catches them diff --git a/app/features/authentication/server/session.server.ts b/app/features/authentication/server/session.server.ts index d4baeeb3..2927e64a 100644 --- a/app/features/authentication/server/session.server.ts +++ b/app/features/authentication/server/session.server.ts @@ -1,4 +1,5 @@ import { createCookieSessionStorage, redirect, type Session } from 'react-router' +import { buildLoginRedirectUrl } from '~/features/authentication/server/post-login-redirect.server' import { sanitizeAccount } from '~/shared/auth/sanitize-account.server' import { SESSION_MAX_AGE_SECONDS_DEV, SESSION_MAX_AGE_SECONDS_PROD } from '~/shared/constants/limits' import { resolveCongregation, resolveCongregationFromRequest } from '~/shared/domain/congregation.server' @@ -32,19 +33,32 @@ const { getSession, commitSession, destroySession } = createCookieSessionStorage export { commitSession, destroySession, getSession } -async function redirectToLogin(session: Session): Promise { - throw redirect('/login', { +async function redirectToLogin( + session: Session, + options: { redirectTo?: string } = {}, +): Promise { + throw redirect(buildLoginRedirectUrl(options.redirectTo ?? '/'), { headers: { 'Set-Cookie': await destroySession(session), }, }) } -async function findUserFromSession(session: Session) { +function extractRedirectTo(request: Request): string { + try { + const url = new URL(request.url) + return `${url.pathname}${url.search}` + } catch { + logger.warn(`verifySession: unable to parse request.url (${request.url})`) + return '/' + } +} + +async function findUserFromSession(session: Session, redirectTo: string) { const rawUserId = session.get('userId') const userId = Number(rawUserId) if (!rawUserId || Number.isNaN(userId) || userId <= 0) { - return redirectToLogin(session) + return redirectToLogin(session, { redirectTo }) } try { @@ -53,12 +67,12 @@ async function findUserFromSession(session: Session 0) { + logger.warn(`safeRedirectUrl: rejected potentially unsafe redirect target (${url})`) + } return fallback } diff --git a/app/tests/e2e/login.spec.ts b/app/tests/e2e/login.spec.ts index 9343af2c..4d1c5850 100644 --- a/app/tests/e2e/login.spec.ts +++ b/app/tests/e2e/login.spec.ts @@ -5,6 +5,7 @@ const TEST_EMAIL = process.env.E2E_USER_EMAIL ?? 'admin@unitae.test' const TEST_PASSWORD = process.env.E2E_USER_PASSWORD ?? 'password' const LOGIN_URL_RE = /\/login/ +const LOGIN_WITH_PUBLISHERS_REDIRECT_RE = /\/login\?redirectTo=%2Fpublishers/ const PUBLISHERS_URL_RE = /\/publishers/ const EMAIL_FIELD_RE = /email/i const PASSWORD_FIELD_RE = /mot de passe/i @@ -21,6 +22,35 @@ test.describe('Authentication', () => { await expect(page).toHaveURL(LOGIN_URL_RE) }) + test('unauthenticated access preserves the intended URL as redirectTo', async ({ page }) => { + await page.goto('/publishers') + await expect(page).toHaveURL(LOGIN_WITH_PUBLISHERS_REDIRECT_RE) + }) + + test('logging in from a preserved redirectTo returns to the intended URL', async ({ page }) => { + await page.goto('/publishers') + + // If no users exist yet, the app redirects to /setup or /register — skip. + if (page.url().includes('/setup') || page.url().includes('/register')) { + test.skip() + return + } + + const emailField = page.getByLabel(EMAIL_FIELD_RE) + if (!(await emailField.isVisible({ timeout: 3000 }).catch(() => false))) { + test.skip() + return + } + + await emailField.fill(TEST_EMAIL) + await page.getByLabel(PASSWORD_FIELD_RE).fill(TEST_PASSWORD) + await page.getByRole('button', { name: SUBMIT_BUTTON_RE }).click() + + await page.waitForLoadState('networkidle') + await expect(page).toHaveURL(PUBLISHERS_URL_RE) + await expect(page).not.toHaveURL(LOGIN_URL_RE) + }) + test('login with valid credentials navigates away from login page', async ({ page }) => { const loggedIn = await login(page, TEST_EMAIL, TEST_PASSWORD) if (!loggedIn) test.skip() diff --git a/docs/development/architecture.md b/docs/development/architecture.md index 9d892487..121b1af6 100644 --- a/docs/development/architecture.md +++ b/docs/development/architecture.md @@ -146,7 +146,14 @@ The authoritative list lives in `app/database/schema.prisma`. ``` Login → validateCredentials(email, password) → set session cookie (userId) - → redirect to / + → redirect to `?redirectTo=` (sanitized via safeRedirectUrl) + or `/` when no redirectTo is present + +Session expiry → verifySession bounces to /login?redirectTo= + → after re-auth, user lands back on the original path + → tenant mismatch, suspended congregation, expired trial, + unverified email, and missing GDPR consent bounce bare + (no redirectTo — those wall pages are the correct terminal state) Protected Route → requireAuth() middleware on layout route → verifySession: fetch user (unscopedDb), check suspension/trial/email