From fec8d17b1974aa15d94ec72f25155d5a0a15b416 Mon Sep 17 00:00:00 2001 From: thierryvm Date: Sat, 25 Jul 2026 21:48:04 +0200 Subject: [PATCH] fix(i18n): carry the user's locale through every server redirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit localePrefix: 'as-needed' puts French on unprefixed URLs — the French prefix is /fr-BE, never the empty string — so a path like /login matches no locale prefix at all. Until #258 next-intl covered for that: the cookie branch of localeDetection read NEXT_LOCALE and 307'd the request to /en/login. #258 turned that detection off, because it was the same gate that let a background prefetch silently rewrite the user's language. The safety net went with it, and every bare redirect() started dropping English users on French pages — session expiry, login, logout, end of onboarding, admin refusal. The helper I planned to write already existed. src/i18n/navigation.ts exposes a redirect from createNavigation(routing) that returns never, throws synchronously like next/navigation's, and whose type FORCES the locale to be passed. No new module, and it removes a real hazard: eslint has no type-aware linting here, so no-floating-promises is off, and an async redirect helper would have let a forgotten await fall through — the guard would not throw, and the visitor would proceed unauthenticated with neither TypeScript nor lint complaining. TypeScript does not propagate never-narrowing from a destructured binding, so the code after each call read as reachable and surfaced 8 possibly-null errors. Each call site now returns the redirect explicitly, which makes the termination visible to both the reader and the compiler. Two things are deliberately NOT localised. Google's OAuth URL is absolute and external — prefixing it would turn it into a broken same-origin path — so it keeps next/navigation's redirect under the explicit alias redirectToExternalUrl. And auth/callback keeps resolving the locale from the cookie: the proxy matcher excludes that route, so the middleware never runs there and the header getLocale() depends on is absent; it would fall back to a path costing an extra auth.getUser() plus a users select on the OAuth hot path, for a value the cookie already carries. Both now say so in a comment. requireUser's redirectTo parameter is removed rather than validated: none of its six callers passes an argument, so localising a caller-supplied path would have invented an open-redirect surface. Measured A/B against production: /en/app used to 307 to /login (French) and now 307s to /en/login, while an unprefixed /app still goes to /login unchanged. The redirecting branches had zero coverage — the neighbouring suite only exercises getOptionalUser and never mocks navigation. The new test mocks a redirect that THROWS, because an inert mock would let requireUser fall through and return undefined as User, keeping the suite green over a guard that no longer guards. Worth stating plainly: the authenticated Playwright specs are skipped in CI, so these unit tests are the only automated net for this regression class, not a complement to an E2E one. --- ...-i18n-localised-server-redirects-report.md | 138 +++++++++++++++ .../app/settings/deletion-status/page.tsx | 4 +- src/app/[locale]/onboarding/page.tsx | 6 +- src/app/auth/callback/route.ts | 9 + src/lib/actions/auth.ts | 27 ++- src/lib/actions/onboarding.ts | 6 +- src/lib/auth/__tests__/require-admin.test.ts | 17 +- .../__tests__/require-user-redirects.test.ts | 159 ++++++++++++++++++ src/lib/auth/__tests__/require-user.test.ts | 14 ++ src/lib/auth/require-admin.ts | 7 +- src/lib/auth/require-user.ts | 18 +- src/lib/data/workspace-snapshot.ts | 13 +- 12 files changed, 391 insertions(+), 27 deletions(-) create mode 100644 docs/prs/PR-i18n-localised-server-redirects-report.md create mode 100644 src/lib/auth/__tests__/require-user-redirects.test.ts diff --git a/docs/prs/PR-i18n-localised-server-redirects-report.md b/docs/prs/PR-i18n-localised-server-redirects-report.md new file mode 100644 index 0000000..6698f56 --- /dev/null +++ b/docs/prs/PR-i18n-localised-server-redirects-report.md @@ -0,0 +1,138 @@ +# PR — les redirections serveur portent la langue de l'utilisateur + +**Date** : 25 juillet 2026 +**Auteur** : @cc-ankora +**Branche** : `fix/i18n-localised-server-redirects` +**Revue de plan** : `plan-reviewer` — 🟡 APPROVED WITH CHANGES (4 corrections, toutes intégrées) +**Ferme** : le P0 laissé ouvert par #258, arbitré par @thierry le 25/07 + +--- + +## 1. La régression fermée + +`localePrefix: 'as-needed'` met le français sur les URLs **non préfixées** : le préfixe du +français est `/fr-BE`, jamais la chaîne vide. Un chemin comme `/login` ne matche donc aucun +préfixe de locale. + +Jusqu'à #258, next-intl rattrapait le coup : la branche cookie de `localeDetection` lisait +`NEXT_LOCALE` et émettait un 307 vers `/en/login`. #258 a désactivé cette détection — c'était +la même garde qui laissait un prefetch d'arrière-plan réécrire silencieusement la langue de +l'utilisateur. Le filet a disparu avec elle, et chaque `redirect()` nu renvoyait dès lors un +utilisateur anglophone sur une page française. + +Concerné : session expirée, connexion, déconnexion, fin d'onboarding, refus d'accès admin. + +## 2. Le correctif + +Le helper que j'avais prévu d'écrire **existait déjà** — `plan-reviewer` l'a relevé, et c'est +strictement meilleur que ma proposition. `src/i18n/navigation.ts` expose un `redirect` issu de +`createNavigation(routing)` : + +```ts +redirect({ href: '/login', locale: await getLocale() }); +``` + +Il retourne `never`, lève immédiatement comme celui de `next/navigation`, et **son type impose +la locale**. Aucun module à écrire, et le risque décrit ci-dessous disparaît par construction. + +### Le piège que ce choix évite + +J'avais envisagé un helper `async` du type `await redirectLocalised('/login')`. Vérification +faite : `eslint.config.mjs` n'active pas `@typescript-eslint/no-floating-promises` (pas de lint +typé — ni `project` ni `projectService`). Un `await` oublié sur une garde d'authentification +n'aurait donc levé **ni erreur TypeScript ni erreur de lint** : `redirect` ne lèverait plus, +l'exécution continuerait, et l'utilisateur passerait **non authentifié**. Le `redirect` +synchrone de next-intl rend cette classe d'erreur impossible. + +### Terminaison explicite + +TypeScript ne propage pas le narrowing `never` depuis un identifiant issu d'un destructuring +(`export const { redirect } = createNavigation(...)`), contrairement au `redirect` de +`next/navigation`. Le code après l'appel était donc considéré atteignable, ce qui a fait +remonter 8 erreurs `'x' is possibly null`. Corrigé par un `return redirect(...)` explicite à +chaque site : la terminaison est visible à la lecture, et TypeScript la comprend. + +## 3. Sites modifiés (14 redirections, 8 fichiers) + +| Fichier | Cibles localisées | +| -------------------------------------------------------- | ---------------------------------------------------------------------- | +| `src/lib/auth/require-user.ts` | `/login`, `/onboarding` | +| `src/lib/auth/require-admin.ts` | `/app` | +| `src/lib/data/workspace-snapshot.ts` | `/login`, `/onboarding` (×3) | +| `src/lib/actions/auth.ts` | `/signup/check-email`, `/app`\|`/onboarding`, `/`, `/login?reset=done` | +| `src/lib/actions/onboarding.ts` | `/app` | +| `src/app/[locale]/onboarding/page.tsx` | `/app` | +| `src/app/[locale]/app/settings/deletion-status/page.tsx` | `/app/settings` | + +### Ce qui n'est délibérément PAS localisé + +- **`src/lib/actions/auth.ts` — l'URL OAuth de Google.** Elle est absolue et externe ; la + passer au `redirect` locale-aware la préfixerait et la transformerait en chemin + same-origin cassé. Elle garde le `redirect` de `next/navigation`, importé sous l'alias + explicite `redirectToExternalUrl` — la distinction est porteuse de sens, pas cosmétique. +- **`src/app/auth/callback/route.ts`** garde sa résolution par cookie. Cette route est exclue + du matcher du proxy, donc le middleware n'y tourne pas et l'en-tête `X-NEXT-INTL-LOCALE` + dont dépend `getLocale()` est absent : il retomberait sur `resolveLocaleFromUserOrCookie()`, + qui coûte un `auth.getUser()` **plus** un select `users` sur le chemin chaud de l'OAuth, pour + la valeur que le cookie porte déjà. Commentaire ajouté pour que personne ne « simplifie ». +- **`src/proxy.ts` et `src/i18n/routing.ts`** ne sont pas touchés. Un 307 basé sur la lecture + du cookie dans le proxy serait l'alternative tentante en une ligne ; elle est interdite par + la note de `routing.ts` (« trois correctifs middleware construits et mesurés — do not retry + that layer »). Localiser aux call-sites est la bonne couche. +- **`src/app/not-found.tsx`** lit `NEXT_LOCALE` mais ne contient aucun `redirect()` — hors + périmètre, vérifié. + +### Paramètre mort supprimé + +`requireUser(redirectTo = '/login')` : les 6 appelants du repo ne passent **aucun** argument. +Localiser un chemin fourni par un appelant inexistant reviendrait à inventer une surface +d'open redirect. Le paramètre est supprimé plutôt que validé. + +### Le middleware n'est pas en cause + +`updateSession` (`src/lib/supabase/middleware.ts`) n'émet aucune redirection — il ne fait que +rafraîchir les cookies de session. Le `307 → /login` mesuré provient donc bien d'un +`redirect()` RSC, et le relevé `grep -rn "redirect("` couvre tout le rayon d'explosion. + +## 4. Preuve + +Mesure A/B sur le comportement réel, build de production local contre la production actuelle : + +| Requête | Production (avant) | Cette branche | +| ---------------------------- | -------------------------------- | -------------------------- | +| `/en/app` — visiteur anglais | `307 → /login` ❌ page française | `307 → /en/login` ✅ | +| `/app` — visiteur français | `307 → /login` | `307 → /login` ✅ inchangé | + +### Tests + +`src/lib/auth/__tests__/require-user-redirects.test.ts` — **nouveau**. Ces branches avaient +**zéro couverture** : la suite voisine `require-user.test.ts` n'exerce que `getOptionalUser`, +n'importe jamais `requireUser` et ne mocke pas `next/navigation`. + +Le mock `redirect` **lève**, sur le modèle de `require-admin.test.ts`. Un mock inerte laisserait +`requireUser` continuer et retourner `undefined as User` : le test resterait vert au-dessus +d'une garde qui ne garde plus. Un test dédié verrouille précisément ça. + +Deux suites existantes ont dû être alignées : elles mockaient `next/navigation`, alors que les +modules importent désormais `@/i18n/navigation`. Mocker le barrel garde aussi le build ESM de +next-intl hors du graphe Vitest, où il échoue à résoudre `next/navigation`. + +**Angle mort assumé** : les specs Playwright authentifiées sont skippées en CI (secrets `E2E_*` +absents — un seul projet Supabase, la clé `service_role` ne doit pas atteindre la CI). Ces +tests unitaires sont donc le **seul** filet automatisé pour cette classe de régression, pas un +complément à une couverture E2E. + +### Quality gates + +`npm run typecheck` 0 erreur · `npm run lint` 0 erreur · `npm run lint:use-server` OK · +`npm run test` **1652 / 1652** + +## 5. Definition of DONE + +| # | Critère | Preuve | +| --- | ----------------------------------- | -------------------------------------- | +| 1 | CI verte | cf. checks de la PR | +| 2 | Sourcery muet sur le dernier commit | `gh api …/comments` → sortie vide | +| 3 | Threads de review résolus | GraphQL `reviewThreads` → 0 non résolu | +| 4 | Pas de conflit avec `main` | `mergeStateStatus: CLEAN` | +| 5 | Rapport livré | ce fichier | diff --git a/src/app/[locale]/app/settings/deletion-status/page.tsx b/src/app/[locale]/app/settings/deletion-status/page.tsx index 218c136..61463ec 100644 --- a/src/app/[locale]/app/settings/deletion-status/page.tsx +++ b/src/app/[locale]/app/settings/deletion-status/page.tsx @@ -1,5 +1,5 @@ import type { Metadata } from 'next'; -import { redirect } from 'next/navigation'; +import { redirect } from '@/i18n/navigation'; import { getLocale, getTranslations } from 'next-intl/server'; import { Link } from '@/i18n/navigation'; @@ -31,7 +31,7 @@ export default async function DeletionStatusPage() { .limit(1) .maybeSingle(); - if (!data) redirect('/app/settings'); + if (!data) return redirect({ href: '/app/settings', locale: await getLocale() }); const scheduled = new Date(data.scheduled_for); const now = new Date(); diff --git a/src/app/[locale]/onboarding/page.tsx b/src/app/[locale]/onboarding/page.tsx index 1052a6e..6aaa950 100644 --- a/src/app/[locale]/onboarding/page.tsx +++ b/src/app/[locale]/onboarding/page.tsx @@ -1,6 +1,8 @@ import type { Metadata } from 'next'; -import { redirect } from 'next/navigation'; +import { getLocale } from 'next-intl/server'; + +import { redirect } from '@/i18n/navigation'; import { requireUser } from '@/lib/auth/require-user'; import { createClient } from '@/lib/supabase/server'; @@ -17,7 +19,7 @@ export default async function OnboardingPage() { .eq('id', user.id) .maybeSingle(); - if (profile?.onboarded_at) redirect('/app'); + if (profile?.onboarded_at) return redirect({ href: '/app', locale: await getLocale() }); return (
diff --git a/src/app/auth/callback/route.ts b/src/app/auth/callback/route.ts index 17dd064..3055486 100644 --- a/src/app/auth/callback/route.ts +++ b/src/app/auth/callback/route.ts @@ -19,6 +19,15 @@ import { routing, type Locale } from '@/i18n/routing'; * Falls back to `routing.defaultLocale` (fr-BE) if the cookie is missing or * carries an unknown value. The TS type guard rules out a spoofed cookie. */ +/** + * Reads the cookie DELIBERATELY, and must keep doing so — do not "simplify" + * this to `getLocale()` from `next-intl/server`. This route is excluded from + * the proxy matcher (`src/proxy.ts`), so the middleware never runs here and the + * `X-NEXT-INTL-LOCALE` header `getLocale()` relies on is absent. It would fall + * back to `resolveLocaleFromUserOrCookie()` in `src/i18n/request.ts`, which + * costs an extra `auth.getUser()` plus a `users` select on the OAuth hot path, + * for the same value the cookie already carries. + */ async function resolveLocale(): Promise { const cookieStore = await cookies(); const raw = cookieStore.get('NEXT_LOCALE')?.value; diff --git a/src/lib/actions/auth.ts b/src/lib/actions/auth.ts index 9231f5e..e6cd533 100644 --- a/src/lib/actions/auth.ts +++ b/src/lib/actions/auth.ts @@ -1,7 +1,10 @@ 'use server'; import { headers } from 'next/headers'; -import { redirect } from 'next/navigation'; +import { redirect as redirectToExternalUrl } from 'next/navigation'; +import { getLocale } from 'next-intl/server'; + +import { redirect } from '@/i18n/navigation'; import { env } from '@/lib/env'; import { createClient } from '@/lib/supabase/server'; @@ -73,7 +76,7 @@ export async function signupAction(formData: FormData): Promise { userAgent, }); - redirect('/signup/check-email'); + return redirect({ href: '/signup/check-email', locale: await getLocale() }); } export async function signInWithGoogleAction(): Promise { @@ -103,7 +106,11 @@ export async function signInWithGoogleAction(): Promise { return { ok: false, errorCode: 'errors.auth.googleFailed' }; } - redirect(data.url); + // EXTERNAL URL — Google's consent screen. It must never go through the + // locale-aware `redirect`: that one prefixes its `href`, which would turn an + // absolute third-party URL into a broken same-origin path. Hence the explicit + // alias — the distinction is load-bearing, not stylistic. + redirectToExternalUrl(data.url); } export async function loginAction(formData: FormData): Promise { @@ -151,7 +158,10 @@ export async function loginAction(formData: FormData): Promise { .eq('id', data.user.id) .maybeSingle(); - redirect(profile?.onboarded_at ? '/app' : '/onboarding'); + return redirect({ + href: profile?.onboarded_at ? '/app' : '/onboarding', + locale: await getLocale(), + }); } export async function logoutAction(): Promise { @@ -167,7 +177,12 @@ export async function logoutAction(): Promise { await logAuditEvent(AuditEvent.AUTH_LOGOUT, { userId: user.id, ipAddress: ip, userAgent }); } - redirect('/'); + // Prefixed on purpose. The @thierry arbitration of 2026-07-25 ("`/` always + // renders French") is about Accept-Language detection on an unprefixed URL, + // not about an explicit redirect from a context where the locale is known. + // Dropping an English user on the French landing right after logout is the + // very bug this PR closes. + return redirect({ href: '/', locale: await getLocale() }); } export async function requestPasswordResetAction(formData: FormData): Promise { @@ -240,5 +255,5 @@ export async function confirmPasswordResetAction(formData: FormData): Promise { +const redirectMock = vi.fn((_args?: unknown): never => { throw new Error('NEXT_REDIRECT'); }); const notFoundMock = vi.fn(() => { @@ -17,10 +17,21 @@ vi.mock('next/headers', () => ({ })); vi.mock('next/navigation', () => ({ - redirect: (path: string) => redirectMock(path), notFound: () => notFoundMock(), })); +// `redirect` now comes from the locale-aware barrel, so the guard can send an +// English admin to `/en/app` instead of the French cockpit. Mocking the barrel +// (rather than `next/navigation`) also keeps next-intl's ESM build out of the +// Vitest module graph — it fails to resolve `next/navigation` there. +vi.mock('@/i18n/navigation', () => ({ + redirect: (args: unknown) => redirectMock(args), +})); + +vi.mock('next-intl/server', () => ({ + getLocale: async () => 'fr-BE', +})); + vi.mock('@/lib/auth/require-user', () => ({ requireUser: async () => requireUserMock(), })); @@ -103,7 +114,7 @@ describe('requireAdmin() — rate-limited + audit-logged guard', () => { await expect(requireAdmin()).rejects.toThrow('NEXT_REDIRECT'); - expect(redirectMock).toHaveBeenCalledWith('/app'); + expect(redirectMock).toHaveBeenCalledWith({ href: '/app', locale: 'fr-BE' }); // Per security-auditor P1-B + gdpr P1, attempted_user_id is no longer // duplicated into metadata — canonical `userId` column carries it. expect(logAuditEventMock).toHaveBeenCalledWith( diff --git a/src/lib/auth/__tests__/require-user-redirects.test.ts b/src/lib/auth/__tests__/require-user-redirects.test.ts new file mode 100644 index 0000000..fcf13a1 --- /dev/null +++ b/src/lib/auth/__tests__/require-user-redirects.test.ts @@ -0,0 +1,159 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +/** + * Locale-aware server redirects — `requireUser` / `requireUserWithWorkspace`. + * + * Why this file exists. `localePrefix: 'as-needed'` puts French on unprefixed + * URLs, so `/login` IS the French login page. Until #258, a bare + * `redirect('/login')` was rescued downstream: next-intl's `localeDetection` + * read the NEXT_LOCALE cookie and 307'd the request to `/en/login`. #258 turned + * that detection off (it was the same gate that let a background prefetch + * silently rewrite the user's language), so the rescue is gone and every bare + * redirect now lands an English user on a French page. + * + * These branches had ZERO coverage before: the neighbouring + * `require-user.test.ts` exercises `getOptionalUser` only — it never imports + * `requireUser`, and never mocks `next/navigation`. + * + * The `redirect` mock THROWS on purpose, mirroring `require-admin.test.ts`. A + * no-op mock would let `requireUser` fall through and return `undefined as + * User`, so the test would stay green over a guard that no longer guards — + * the exact failure mode this suite is meant to catch. + * + * Note on coverage honesty: the authenticated Playwright specs are skipped in + * CI (no `E2E_*` secrets — one Supabase project, service_role key must not + * reach CI). These unit tests are therefore the only automated net for this + * regression class, not a complement to an E2E one. + */ + +const redirectMock = vi.hoisted(() => + vi.fn((_args?: unknown): never => { + throw new Error('NEXT_REDIRECT'); + }), +); +const getLocaleMock = vi.hoisted(() => vi.fn(async () => 'fr-BE')); +const getUserMock = vi.hoisted(() => vi.fn()); +const fromMock = vi.hoisted(() => vi.fn()); + +vi.mock('@/i18n/navigation', () => ({ + redirect: (args: unknown) => redirectMock(args), +})); + +vi.mock('next-intl/server', () => ({ + getLocale: () => getLocaleMock(), +})); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: async () => ({ + auth: { getUser: () => getUserMock() }, + from: (table: string) => fromMock(table), + }), +})); + +vi.mock('@/lib/log', () => ({ + log: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, +})); + +import { requireUser, requireUserWithWorkspace } from '../require-user'; + +const fakeUser = { + id: 'user-123', + email: 'thierry@example.test', + app_metadata: {}, + user_metadata: {}, + aud: 'authenticated', + created_at: '2026-01-01T00:00:00.000Z', +}; + +/** Minimal chainable stub for `.from(...).select(...)…maybeSingle()`. */ +function membershipQuery(result: { data: unknown; error: unknown }) { + const chain = { + select: () => chain, + eq: () => chain, + order: () => chain, + limit: () => chain, + maybeSingle: async () => result, + }; + return chain; +} + +beforeEach(() => { + redirectMock.mockClear(); + getLocaleMock.mockClear(); + getLocaleMock.mockResolvedValue('fr-BE'); + getUserMock.mockReset(); + fromMock.mockReset(); +}); + +describe('requireUser — the login redirect carries the locale', () => { + it('sends an English visitor to the English login page', async () => { + getLocaleMock.mockResolvedValue('en'); + getUserMock.mockResolvedValue({ data: { user: null }, error: null }); + + await expect(requireUser()).rejects.toThrow('NEXT_REDIRECT'); + expect(redirectMock).toHaveBeenCalledWith({ href: '/login', locale: 'en' }); + }); + + it('sends a French visitor to the French login page', async () => { + getUserMock.mockResolvedValue({ data: { user: null }, error: null }); + + await expect(requireUser()).rejects.toThrow('NEXT_REDIRECT'); + expect(redirectMock).toHaveBeenCalledWith({ href: '/login', locale: 'fr-BE' }); + }); + + it('does not redirect when a session exists', async () => { + getUserMock.mockResolvedValue({ data: { user: fakeUser }, error: null }); + + await expect(requireUser()).resolves.toMatchObject({ id: 'user-123' }); + expect(redirectMock).not.toHaveBeenCalled(); + }); +}); + +describe('requireUserWithWorkspace — the onboarding redirect carries the locale', () => { + it('sends an English visitor without a workspace to the English onboarding', async () => { + getLocaleMock.mockResolvedValue('en'); + getUserMock.mockResolvedValue({ data: { user: fakeUser }, error: null }); + fromMock.mockReturnValue(membershipQuery({ data: null, error: null })); + + await expect(requireUserWithWorkspace()).rejects.toThrow('NEXT_REDIRECT'); + expect(redirectMock).toHaveBeenCalledWith({ href: '/onboarding', locale: 'en' }); + }); + + it('returns the membership when there is one, without redirecting', async () => { + getUserMock.mockResolvedValue({ data: { user: fakeUser }, error: null }); + fromMock.mockReturnValue( + membershipQuery({ data: { workspace_id: 'ws-1', role: 'owner' }, error: null }), + ); + + await expect(requireUserWithWorkspace()).resolves.toMatchObject({ + workspaceId: 'ws-1', + role: 'owner', + }); + expect(redirectMock).not.toHaveBeenCalled(); + }); +}); + +describe('the guard cannot be silently defeated', () => { + it('never returns a user when the session is missing', async () => { + // Guards against the failure mode a non-throwing redirect mock would hide: + // if `redirect` stopped terminating, `requireUser` would fall through and + // hand back `undefined` typed as `User`, and every caller would treat the + // visitor as authenticated. + getUserMock.mockResolvedValue({ data: { user: null }, error: null }); + + const outcome = await requireUser().then( + (value) => ({ returned: value }), + (error: Error) => ({ threw: error.message }), + ); + + expect(outcome).toEqual({ threw: 'NEXT_REDIRECT' }); + }); +}); diff --git a/src/lib/auth/__tests__/require-user.test.ts b/src/lib/auth/__tests__/require-user.test.ts index 0e12f49..6a977cd 100644 --- a/src/lib/auth/__tests__/require-user.test.ts +++ b/src/lib/auth/__tests__/require-user.test.ts @@ -10,6 +10,20 @@ const createClientMock = vi.fn(async () => ({ const logWarnMock = vi.hoisted(() => vi.fn()); const logErrorMock = vi.hoisted(() => vi.fn()); +// This suite covers `getOptionalUser` only, but the module under test now +// imports the locale-aware `redirect`. Stub the barrel so next-intl's ESM build +// stays out of the Vitest module graph (it fails to resolve `next/navigation` +// there). The redirecting branches are covered in `require-user-redirects.test.ts`. +vi.mock('@/i18n/navigation', () => ({ + redirect: () => { + throw new Error('NEXT_REDIRECT'); + }, +})); + +vi.mock('next-intl/server', () => ({ + getLocale: async () => 'fr-BE', +})); + vi.mock('@/lib/supabase/server', () => ({ createClient: () => createClientMock(), })); diff --git a/src/lib/auth/require-admin.ts b/src/lib/auth/require-admin.ts index 12d7a1a..d2f0f46 100644 --- a/src/lib/auth/require-admin.ts +++ b/src/lib/auth/require-admin.ts @@ -1,5 +1,8 @@ import { headers } from 'next/headers'; -import { notFound, redirect } from 'next/navigation'; +import { notFound } from 'next/navigation'; +import { getLocale } from 'next-intl/server'; + +import { redirect } from '@/i18n/navigation'; import type { User } from '@supabase/supabase-js'; import { env } from '@/lib/env'; @@ -75,7 +78,7 @@ export async function requireAdmin(): Promise { { userId: user.id, ipAddress, userAgent }, { path }, ); - redirect('/app'); + return redirect({ href: '/app', locale: await getLocale() }); } // 4. Granted diff --git a/src/lib/auth/require-user.ts b/src/lib/auth/require-user.ts index 648d20b..491a8b4 100644 --- a/src/lib/auth/require-user.ts +++ b/src/lib/auth/require-user.ts @@ -1,6 +1,7 @@ -import { redirect } from 'next/navigation'; import type { User } from '@supabase/supabase-js'; +import { getLocale } from 'next-intl/server'; +import { redirect } from '@/i18n/navigation'; import { createClient } from '@/lib/supabase/server'; import { log } from '@/lib/log'; @@ -73,12 +74,19 @@ export async function getOptionalUser(): Promise { * Delegates the session lookup to `getOptionalUser` so the Supabase wiring * (createClient + getUser + error handling) lives in a single place. */ -export async function requireUser(redirectTo = '/login'): Promise { +export async function requireUser(): Promise { const user = await getOptionalUser(); if (!user) { - log.warn('[503-diag] require-user requireUser redirect', { redirectTo }); - redirect(redirectTo); + log.warn('[503-diag] require-user requireUser redirect', { redirectTo: '/login' }); + // Locale-aware on purpose: `localePrefix: 'as-needed'` means French lives + // on unprefixed URLs, so a bare `redirect('/login')` sends an English user + // to the French login page. next-intl stopped covering for this when + // `localeDetection` was turned off (#258) — the cookie branch that used to + // 307 `/login` to `/en/login` is gone. This `redirect` returns `never` and + // throws synchronously like the one from `next/navigation`, and its type + // forces the locale to be passed. + return redirect({ href: '/login', locale: await getLocale() }); } return user; @@ -121,7 +129,7 @@ export async function requireUserWithWorkspace(): Promise<{ userId: user.id, hadError: !!error, }); - redirect('/onboarding'); + return redirect({ href: '/onboarding', locale: await getLocale() }); } return { diff --git a/src/lib/data/workspace-snapshot.ts b/src/lib/data/workspace-snapshot.ts index 7c25e27..00551f4 100644 --- a/src/lib/data/workspace-snapshot.ts +++ b/src/lib/data/workspace-snapshot.ts @@ -1,4 +1,6 @@ -import { redirect } from 'next/navigation'; +import { getLocale } from 'next-intl/server'; + +import { redirect } from '@/i18n/navigation'; import { createClient } from '@/lib/supabase/server'; import { log } from '@/lib/log'; @@ -150,14 +152,14 @@ export async function getWorkspaceSnapshot(): Promise { const { data: { user }, } = await supabase.auth.getUser(); - if (!user) redirect('/login'); + if (!user) return redirect({ href: '/login', locale: await getLocale() }); const { data: profile } = await supabase .from('users') .select('onboarded_at') .eq('id', user.id) .maybeSingle(); - if (!profile?.onboarded_at) redirect('/onboarding'); + if (!profile?.onboarded_at) return redirect({ href: '/onboarding', locale: await getLocale() }); const { data: membership } = await supabase .from('workspace_members') @@ -166,7 +168,7 @@ export async function getWorkspaceSnapshot(): Promise { .order('joined_at', { ascending: true }) .limit(1) .maybeSingle(); - if (!membership) redirect('/onboarding'); + if (!membership) return redirect({ href: '/onboarding', locale: await getLocale() }); const workspaceId = membership.workspace_id; @@ -245,7 +247,8 @@ export async function getWorkspaceSnapshot(): Promise { .eq('period_month', previousMonth), ]); - if (wsRes.error || !wsRes.data) redirect('/onboarding'); + if (wsRes.error || !wsRes.data) + return redirect({ href: '/onboarding', locale: await getLocale() }); // Never swallow a charges read failure silently again (incident 2026-07-18: // the `?? []` fallback made a failing SELECT look like an empty workspace).