From 1097e50dd5601ec3dfb5c1bb41eae48f6603ce23 Mon Sep 17 00:00:00 2001 From: mindsers Date: Thu, 16 Jul 2026 08:27:05 +0200 Subject: [PATCH 1/2] feat(authentication): preserve intended URL after re-login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a session expires while a user has a deep-linked page open (a territory, a publisher card, etc.), send them back to that URL after logging in instead of always landing on /. Only the pure not-authenticated branches carry the URL through — wall pages (suspended, trial expired, unverified email, tenant mismatch) stay bare so users are not looped back into a page they cannot access. Sanitization flows through the existing safeRedirectUrl helper to block open-redirect targets in both the URL query and the form field. --- app/features/authentication/routes/login.tsx | 29 +++-- .../server/post-login-redirect.server.test.ts | 103 ++++++++++++++++++ .../server/post-login-redirect.server.ts | 23 ++++ .../server/session.server.test.ts | 43 +++++--- .../authentication/server/session.server.ts | 29 +++-- app/shared/utils/safe-redirect.server.ts | 5 + app/tests/e2e/login.spec.ts | 30 +++++ 7 files changed, 229 insertions(+), 33 deletions(-) create mode 100644 app/features/authentication/server/post-login-redirect.server.test.ts create mode 100644 app/features/authentication/server/post-login-redirect.server.ts 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..181f2fea 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() From 66528f86ca8ae717894cceacf6713c652f241a04 Mon Sep 17 00:00:00 2001 From: mindsers Date: Thu, 16 Jul 2026 08:41:23 +0200 Subject: [PATCH 2/2] docs(authentication): document login redirect semantics Add a WHY comment on the tenant-mismatch bare-redirect so a future contributor doesn't "fix the inconsistency" and leak a cross-tenant path back into the URL. Extend the safeRedirectUrl JSDoc to note the rejection warn log, and expand the auth flow diagram to cover session expiry, the re-auth destination, and the wall-page exceptions. --- app/features/authentication/server/session.server.ts | 1 + app/shared/utils/safe-redirect.server.ts | 1 + docs/development/architecture.md | 9 ++++++++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/features/authentication/server/session.server.ts b/app/features/authentication/server/session.server.ts index 181f2fea..2927e64a 100644 --- a/app/features/authentication/server/session.server.ts +++ b/app/features/authentication/server/session.server.ts @@ -85,6 +85,7 @@ export async function verifySession(request: Request) { const urlCongregation = await resolveCongregationFromRequest(request) if (urlCongregation && urlCongregation.id !== user.congregationId) { + // Don't preserve the URL: it belongs to a different congregation and must not survive re-login. return redirectToLogin(session) } diff --git a/app/shared/utils/safe-redirect.server.ts b/app/shared/utils/safe-redirect.server.ts index 07903b58..b89c7793 100644 --- a/app/shared/utils/safe-redirect.server.ts +++ b/app/shared/utils/safe-redirect.server.ts @@ -3,6 +3,7 @@ import logger from '~/shared/infra/logger.server' /** * Validates that a redirect URL is a safe relative path. * Prevents open redirect attacks via attacker-controlled URLs (e.g., Referer header). + * Logs a warning when a non-empty url is rejected — useful for spotting phishing probes. */ export function safeRedirectUrl(url: string | null, fallback: string): string { if (url?.startsWith('/') && !url.startsWith('//')) return url 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