fix(auth): post-login redirect, plus deterministic branding and a test-database guard

CLAUDE.md

@@ -414,6 +414,7 @@ LOGIN_RATE_LIMIT_GLOBAL_MAX="100"      # Max failed login attempts instance-wide
 | `Cannot find module '~/database/generated/...'` | Run `pnpm prisma generate` |
 | Lint errors after schema changes | Run `pnpm build:format` to auto-fix most; check for new `biome-ignore` needs |
 | Integration tests fail with RLS errors | Ensure `DB_RUNTIME_URL` points to the `unitae_app` role (not superuser) |
+| `Refusing to run integration tests: …must carry a "test" token` | The suite truncates data, so it only runs against a disposable database. Point **both** `DB_URL` and `DB_RUNTIME_URL` at e.g. `unitae_test` — `DB_RUNTIME_URL` is often exported from a shell profile and left on `unitae_dev`. See [Testing](docs/development/testing.md#the-database-these-run-against) |
 | `prisma migrate dev` hangs in terminal | Use `prisma migrate diff --script` + manual migration + `prisma migrate deploy` |
 | Session not persisting in dev | Check `UNITAE_SESSION_SECRET` is set; cookie lifetime is 8h in dev, 1h in prod |
 | Port already in use | `lsof -i :5173` (dev server) or `lsof -i :9090` (worker health) |

app/features/authentication/server/session.server.test.ts

@@ -176,6 +176,26 @@ describe('verifySession', () => {
     expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2')
   })
 
+  // Regression: the request that trips the guard is usually a single-fetch one, so the captured
+  // path used to be `/territories/1.data` and signing back in landed the user on the loader
+  // endpoint instead of the page.
+  it('nettoie le suffixe single-fetch du redirectTo', async () => {
+    setSession({})
+
+    const response = await getRedirectResponse(() =>
+      verifySession(makeRequest('http://localhost/territories/1.data?_routes=routes%2Fterritories')),
+    )
+    expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1')
+  })
+
+  it('ramène la racine décorée à /', async () => {
+    setSession({})
+
+    const response = await getRedirectResponse(() => verifySession(makeRequest('http://localhost/_.data')))
+    // buildLoginRedirectUrl drops a redirectTo of '/' entirely — nothing to preserve.
+    expect(response.headers.get('Location')).toBe('/login')
+  })
+
   it("redirige vers /login sans redirectTo si le subdomain ne correspond pas à l'assemblée de l'utilisateur", async () => {
     setSession({ userId: '42', sessionEpoch: '0' })
     vi.mocked(db.userAccount.findUnique).mockResolvedValue(fakeUser as never)

app/features/authentication/server/session.server.ts

@@ -6,6 +6,7 @@ import { resolveCongregation, resolveCongregationFromRequest } from '~/shared/do
 import { unscopedDb } from '~/shared/infra/db.server'
 import logger from '~/shared/infra/logger.server'
 import { getSessionSecrets } from '~/shared/utils/env.server'
+import { normalizeRedirectPath } from '~/shared/utils/safe-redirect.server'
 
 type SessionData = {
   userId: string
@@ -78,10 +79,16 @@ async function redirectToLogin(
   })
 }
 
+// Where to send the user back to once they have signed in again.
+//
+// Normalised because the request that trips the guard is very often NOT a document request: a
+// client-side navigation fetches `/programs.data`, and capturing that verbatim sends the user
+// to the loader endpoint after login instead of the page. Cleaned here as well as inside
+// safeRedirectUrl so the `?redirectTo=` the user actually sees in the address bar is a page.
 function extractRedirectTo(request: Request): string {
   try {
     const url = new URL(request.url)
-    return `${url.pathname}${url.search}`
+    return normalizeRedirectPath(`${url.pathname}${url.search}`)
   } catch {
     logger.warn(`verifySession: unable to parse request.url (${request.url})`)
     return '/'

app/shared/domain/congregation.server.test.ts

@@ -12,7 +12,9 @@ vi.mock('react-router', () => ({
   }),
 }))
 
-const { resolveCongregation, resolveCongregationFromRequest, getPlatformName } = await import('./congregation.server')
+const { getBrandingName, resolveCongregation, resolveCongregationFromRequest, getPlatformName } = await import(
+  './congregation.server'
+)
 const { unscopedDb: db } = await import('~/shared/infra/db.server')
 
 beforeEach(() => {
@@ -235,3 +237,26 @@ describe('resolveCongregationFromRequest', () => {
     expect(result).toBeNull()
   })
 })
+
+// Single-tenant mode assumes one congregation, and `findFirst` with no ordering asks Postgres
+// for "any row" — with a second one present (a stale import, a leftover test fixture) it can
+// return either, so the login page ends up branded with whichever one the planner reached
+// first, and the answer can change between requests.
+describe('getBrandingName — deterministic single-tenant pick', () => {
+  const singleTenant = { name: 'La Bonne Assemblée', displayName: null }
+
+  it('asks for a deterministic congregation rather than any row', async () => {
+    vi.mocked(db.congregation.findFirst).mockResolvedValue(singleTenant as never)
+
+    await getBrandingName()
+
+    const call = vi.mocked(db.congregation.findFirst).mock.calls[0][0]
+    expect(call?.orderBy).toEqual({ id: 'asc' })
+  })
+
+  it('still returns the congregation name it found', async () => {
+    vi.mocked(db.congregation.findFirst).mockResolvedValue(singleTenant as never)
+
+    await expect(getBrandingName()).resolves.toBe('La Bonne Assemblée')
+  })
+})

app/shared/domain/congregation.server.ts

@@ -134,8 +134,13 @@ export async function getBrandingName(request?: Request): Promise<string> {
       })
     }
   } else {
+    // Ordered because "the congregation" has to be the same one every time. Single-tenant mode
+    // assumes exactly one row, but a stale import or a leftover test fixture makes that false,
+    // and an unordered findFirst then brands the app with whichever row the planner reached
+    // first — differently from one request to the next.
     congregation = await unscopedDb.congregation.findFirst({
       select: { name: true, displayName: true },
+      orderBy: { id: 'asc' },
     })
   }
 

app/shared/utils/locale.server.test.ts

@@ -128,3 +128,18 @@ describe('resolveLocaleFromRequest', () => {
     expect(result).toBe('fr')
   })
 })
+
+// Same defect as getBrandingName: the single-tenant fallback means "the congregation", and an
+// unordered findFirst can hand back a different one from one request to the next.
+describe('resolveLocaleFromRequest — deterministic single-tenant fallback', () => {
+  it('asks for a deterministic congregation rather than any row', async () => {
+    vi.mocked(getSession).mockResolvedValue({ get: () => undefined } as never)
+    vi.mocked(resolveCongregationFromRequest).mockResolvedValue(null as never)
+    vi.mocked(db.congregation.findFirst).mockResolvedValue({ locale: 'en' } as never)
+
+    await resolveLocaleFromRequest(new Request('http://localhost/'))
+
+    const call = vi.mocked(db.congregation.findFirst).mock.calls[0][0]
+    expect(call?.orderBy).toEqual({ id: 'asc' })
+  })
+})

app/shared/utils/locale.server.ts

@@ -34,10 +34,13 @@ export async function resolveLocaleFromRequest(request: Request): Promise<string
     if (congregation) return congregation.locale ?? DEFAULT_LOCALE
   }
 
-  // 3. Single-tenant fallback: first congregation's locale
+  // 3. Single-tenant fallback: first congregation's locale.
+  // Ordered for the same reason as getBrandingName: with more than one row present this has to
+  // keep resolving to the same congregation instead of alternating between them.
   if (process.env.UNITAE_MULTI_TENANT !== 'true') {
     const first = await unscopedDb.congregation.findFirst({
       select: { locale: true },
+      orderBy: { id: 'asc' },
     })
 
     if (first) return first.locale ?? DEFAULT_LOCALE

app/shared/utils/safe-redirect.server.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest'
-import { safeRedirectUrl } from './safe-redirect.server'
+import { normalizeRedirectPath, safeRedirectUrl } from './safe-redirect.server'
 
 describe('safeRedirectUrl', () => {
   it('returns a valid relative path unchanged', () => {
@@ -46,3 +46,85 @@ describe('safeRedirectUrl', () => {
     expect(safeRedirectUrl(null, '/login')).toBe('/login')
   })
 })
+
+// Regression: a client-side navigation fetches `/programs.data`, not `/programs`. The session
+// guard captured that pathname verbatim, so signing in again dropped the user on the loader
+// endpoint — a raw payload or a 404 — instead of the page they were on.
+describe('normalizeRedirectPath', () => {
+  it('strips the single-fetch suffix from a page path', () => {
+    expect(normalizeRedirectPath('/programs.data')).toBe('/programs')
+  })
+
+  it('strips it from a nested path', () => {
+    expect(normalizeRedirectPath('/settings/congregation/templates.data')).toBe('/settings/congregation/templates')
+  })
+
+  // The URL this was actually reported from.
+  it('strips it from the organigram path', () => {
+    expect(normalizeRedirectPath('/congregation/roles/organigram.data')).toBe('/congregation/roles/organigram')
+  })
+
+  // The root route decorates as `/_.data`, not `/.data` — stripping only `.data` would leave
+  // `/_`, which is not a route either.
+  it('maps the decorated root back to the root path', () => {
+    expect(normalizeRedirectPath('/_.data')).toBe('/')
+  })
+
+  it('does not leave a trailing slash on a nested decorated path', () => {
+    expect(normalizeRedirectPath('/settings/_.data')).toBe('/settings')
+  })
+
+  it('drops the _routes single-fetch parameter', () => {
+    expect(normalizeRedirectPath('/programs.data?_routes=routes%2Fprograms')).toBe('/programs')
+  })
+
+  it('keeps real query parameters while dropping _routes', () => {
+    expect(normalizeRedirectPath('/programs.data?from=2026-09-01&_routes=x')).toBe('/programs?from=2026-09-01')
+  })
+
+  it('leaves an ordinary page path untouched', () => {
+    expect(normalizeRedirectPath('/territories')).toBe('/territories')
+  })
+
+  // `.data` only decorates the end of the pathname; a route segment that merely contains the
+  // word must survive intact.
+  it('does not touch a path that merely contains "data"', () => {
+    expect(normalizeRedirectPath('/exports/data')).toBe('/exports/data')
+  })
+
+  // The four below are the reason this does the least possible: the value flows straight into a
+  // Location header, so anything it rewrites is a URL the user is actually sent to. An earlier
+  // draft split on every '?' and round-tripped the query through URLSearchParams, which
+  // truncated, re-encoded and reshaped destinations that had nothing wrong with them.
+  it('keeps a literal question mark inside a query value', () => {
+    expect(normalizeRedirectPath('/search?q=a?b')).toBe('/search?q=a?b')
+  })
+
+  it('does not re-encode a query string it has no reason to touch', () => {
+    expect(normalizeRedirectPath('/search?q=a%20b')).toBe('/search?q=a%20b')
+  })
+
+  it('leaves a valueless query key alone', () => {
+    expect(normalizeRedirectPath('/search?flag')).toBe('/search?flag')
+  })
+
+  it('does not strip a trailing slash from an undecorated path', () => {
+    expect(normalizeRedirectPath('/programs/')).toBe('/programs/')
+  })
+})
+
+describe('safeRedirectUrl — single-fetch targets', () => {
+  it('normalises a .data target rather than sending the user to the loader endpoint', () => {
+    expect(safeRedirectUrl('/programs.data', '/')).toBe('/programs')
+  })
+
+  // The query string is attacker-reachable, so the funnel has to clean it too — not just the
+  // guard that builds the target.
+  it('normalises a hand-edited redirectTo', () => {
+    expect(safeRedirectUrl('/settings/_.data?_routes=root', '/')).toBe('/settings')
+  })
+
+  it('still refuses an absolute URL that ends in .data', () => {
+    expect(safeRedirectUrl('https://evil.com/steal.data', '/')).toBe('/')
+  })
+})

app/shared/utils/safe-redirect.server.ts

@@ -1,12 +1,65 @@
 import logger from '~/shared/infra/logger.server'
 
+/**
+ * Strips React Router's single-fetch decoration off a path.
+ *
+ * A client-side navigation does not fetch `/programs`, it fetches `/programs.data`. When the
+ * session guard bounces such a request it captures that pathname verbatim, so the login page
+ * is handed `?redirectTo=/programs.data` and sends the browser to the loader endpoint after a
+ * successful sign-in — the user lands on raw loader output, or a 404, instead of the page they
+ * were on.
+ *
+ * The rules mirror React Router's own `getNormalizedPath` (lib/server-runtime/urls.ts), which
+ * is internal and cannot be imported: the root route decorates as `/_.data` rather than
+ * `/.data`, and `_routes` is a single-fetch implementation detail that must not survive into a
+ * user-visible URL.
+ */
+const ROOT_DATA_SUFFIX = /_\.data$/
+const DATA_SUFFIX = /\.data$/
+
+export function normalizeRedirectPath(url: string): string {
+  // Split on the FIRST '?' only. `url.split('?')` discards everything after a second one, and a
+  // literal '?' inside a query value is common enough that browsers pass it through unencoded —
+  // truncating it would silently lose part of the destination.
+  const queryAt = url.indexOf('?')
+  const rawPath = queryAt === -1 ? url : url.slice(0, queryAt)
+  const rawSearch = queryAt === -1 ? null : url.slice(queryAt + 1)
+
+  // Only a decorated path is rewritten. Touching every path would quietly turn this into a
+  // general URL normaliser and start "fixing" trailing slashes that were never the problem.
+  let pathname = rawPath
+  if (rawPath.endsWith('.data')) {
+    pathname = rawPath.endsWith('/_.data') ? rawPath.replace(ROOT_DATA_SUFFIX, '') : rawPath.replace(DATA_SUFFIX, '')
+    // `/_.data` strips to `/`, but a nested `/settings/_.data` strips to `/settings/` — trailing
+    // slashes are not how routes are addressed here, so drop it unless the path IS the root.
+    if (pathname.length > 1 && pathname.endsWith('/')) pathname = pathname.slice(0, -1)
+    if (pathname === '') pathname = '/'
+  }
+
+  if (rawSearch == null) return pathname
+
+  // Rebuild the query string only when there is something to remove. Round-tripping through
+  // URLSearchParams rewrites `%20` as `+` and turns a bare `?flag` into `?flag=` — gratuitous
+  // edits to a URL the user is about to be sent to.
+  const params = new URLSearchParams(rawSearch)
+  if (!params.has('_routes')) return `${pathname}?${rawSearch}`
+
+  params.delete('_routes')
+  const search = params.toString()
+  return search ? `${pathname}?${search}` : pathname
+}
+
 /**
  * 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.
+ *
+ * Normalisation happens here rather than only at the call site that builds the target: this is
+ * the funnel every redirect target passes through, including one supplied by the query string,
+ * so a stale bookmark or a hand-edited `?redirectTo=` cannot reintroduce the `.data` landing.
  */
 export function safeRedirectUrl(url: string | null, fallback: string): string {
-  if (url?.startsWith('/') && !url.startsWith('//')) return url
+  if (url?.startsWith('/') && !url.startsWith('//')) return normalizeRedirectPath(url)
   if (url != null && url.length > 0) {
     logger.warn(`safeRedirectUrl: rejected potentially unsafe redirect target (${url})`)
   }

app/tests/assert-test-database.test.ts

@@ -0,0 +1,92 @@
+import { describe, expect, it } from 'vitest'
+import { assertIntegrationDatabases, assertTestDatabase, isTestDatabaseName } from './assert-test-database'
+
+// The integration suite writes into, and in places truncates, whatever database DB_URL names.
+// Nothing stopped it being pointed at a development database, and pointing it at one is how a
+// working dev database ends up holding a pile of `Roles Other 1788220863373` fixtures.
+describe('isTestDatabaseName', () => {
+  it('accepts the name CI uses', () => {
+    expect(isTestDatabaseName('unitae_test')).toBe(true)
+  })
+
+  it.each(['test', 'test_unitae', 'unitae-test', 'unitae_test_2', 'my-test-db'])('accepts %s', name => {
+    expect(isTestDatabaseName(name)).toBe(true)
+  })
+
+  it('refuses the development database', () => {
+    expect(isTestDatabaseName('unitae_dev')).toBe(false)
+  })
+
+  it.each(['unitae', 'production', 'unitae_prod'])('refuses %s', name => {
+    expect(isTestDatabaseName(name)).toBe(false)
+  })
+
+  // Anchored on purpose: a bare substring match would wave through anything merely containing
+  // the letters, and `latest` is a plausible database name.
+  it.each(['latest', 'greatest', 'contest', 'testing'])('refuses %s — token must be delimited', name => {
+    expect(isTestDatabaseName(name)).toBe(false)
+  })
+})
+
+describe('assertTestDatabase', () => {
+  it('passes for a test database', () => {
+    expect(() => assertTestDatabase('postgresql://u:p@localhost:5432/unitae_test')).not.toThrow()
+  })
+
+  it('names the offending database so the fix is obvious', () => {
+    expect(() => assertTestDatabase('postgresql://u:p@localhost:5432/unitae_dev')).toThrow('unitae_dev')
+  })
+
+  it('refuses rather than silently allowing an unset DB_URL', () => {
+    expect(() => assertTestDatabase(undefined)).toThrow()
+  })
+
+  it('refuses a URL it cannot parse', () => {
+    expect(() => assertTestDatabase('not a url')).toThrow()
+  })
+})
+
+// Guarding DB_URL alone is not enough: most integration files build their client from
+// `DB_RUNTIME_URL ?? DB_URL`, so a runtime URL left pointing at a development database sends
+// every write there while DB_URL looks perfectly safe. That combination is exactly how a dev
+// database gets polluted without anyone doing anything obviously wrong.
+describe('assertIntegrationDatabases', () => {
+  const testUrl = 'postgresql://unitae:unitae@localhost:5432/unitae_test'
+  const devUrl = 'postgresql://unitae_app:unitae_app@localhost:5432/unitae_dev'
+
+  it('passes when only DB_URL is set and names a test database', () => {
+    expect(() => assertIntegrationDatabases({ dbUrl: testUrl })).not.toThrow()
+  })
+
+  it('passes when both name the same test database', () => {
+    expect(() =>
+      assertIntegrationDatabases({
+        dbUrl: testUrl,
+        runtimeUrl: 'postgresql://unitae_app:unitae_app@localhost:5432/unitae_test',
+      }),
+    ).not.toThrow()
+  })
+
+  it('refuses a runtime URL pointing at a development database', () => {
+    expect(() => assertIntegrationDatabases({ dbUrl: testUrl, runtimeUrl: devUrl })).toThrow('DB_RUNTIME_URL')
+  })
+
+  it('names the offending database in the runtime case too', () => {
+    expect(() => assertIntegrationDatabases({ dbUrl: testUrl, runtimeUrl: devUrl })).toThrow('unitae_dev')
+  })
+
+  // Both carry a "test" token, so neither trips the name check on its own — but the suite would
+  // split its writes across two databases and clean up only one of them.
+  it('refuses two different test databases', () => {
+    expect(() =>
+      assertIntegrationDatabases({
+        dbUrl: testUrl,
+        runtimeUrl: 'postgresql://unitae_app:unitae_app@localhost:5432/other_test',
+      }),
+    ).toThrow('same database')
+  })
+
+  it('still refuses a bad DB_URL', () => {
+    expect(() => assertIntegrationDatabases({ dbUrl: devUrl })).toThrow('DB_URL')
+  })
+})