From d6cdfb372785b9211851a32a5355f6ec4eee6262 Mon Sep 17 00:00:00 2001 From: mindsers Date: Sun, 11 Oct 2026 09:11:46 +0200 Subject: [PATCH 1/4] test(multi-tenant): pin the unknown-host fix at the request entry and by outcome Nothing executed handleRequest, so putting the throwing locale lookup back in entry.server left every test green. A test now hands it the redirect resolveDocumentLocale returns and expects that response back. The base-domain and look-alike-host tests asserted that a query was not made. They now let the slug lookup answer and assert what the caller gets, which fails on the old code without a spy. findCongregation was compared with a function that delegates to it; it asserts the mapped fields instead. A subdomain is resolved with UNITAE_BASE_URL written with and without its scheme, as the self-hosting guide writes it, and /livez is requested through the router so losing its route registration is caught. --- app/entry.server.test.ts | 42 +++++++++++ app/shared/domain/congregation.server.test.ts | 72 +++++++++++++------ app/tests/e2e/health.spec.ts | 6 ++ 3 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 app/entry.server.test.ts diff --git a/app/entry.server.test.ts b/app/entry.server.test.ts new file mode 100644 index 00000000..0e558b19 --- /dev/null +++ b/app/entry.server.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it, vi } from 'vitest' + +// boot.server probes the database when it is evaluated; handleRequest only needs it to have run. +vi.mock('~/shared/infra/boot.server', () => ({})) + +vi.mock('~/shared/infra/logger.server', () => ({ + default: { warn: vi.fn(), error: vi.fn(), info: vi.fn() }, +})) + +vi.mock('~/i18n/paraglide/runtime', () => ({ + baseLocale: 'fr', + isLocale: (value: unknown) => value === 'fr' || value === 'en', + overwriteGetLocale: vi.fn(), +})) + +vi.mock('~/shared/utils/locale.server', () => ({ + resolveDocumentLocale: vi.fn(), +})) + +const { default: handleRequest } = await import('./entry.server') +const { resolveDocumentLocale } = await import('~/shared/utils/locale.server') + +describe('handleRequest', () => { + // Regression: a subdomain no congregation owns was a 500 on every unknown path, because the + // redirect was thrown from here. It has to come back from resolveDocumentLocale and be the answer. + it('answers with the redirect resolveDocumentLocale hands back instead of rendering', async () => { + const redirect = new Response(null, { status: 302, headers: { Location: '/congregation-not-found' } }) + vi.mocked(resolveDocumentLocale).mockResolvedValue({ redirect }) + const responseHeaders = new Headers() + + const response = await handleRequest( + new Request('https://inconnu.unitae.app/nope'), + 404, + responseHeaders, + {} as never, + {} as never, + ) + + expect(response).toBe(redirect) + expect(responseHeaders.has('Set-Cookie')).toBe(false) + }) +}) diff --git a/app/shared/domain/congregation.server.test.ts b/app/shared/domain/congregation.server.test.ts index 6dae73f9..60c6787c 100644 --- a/app/shared/domain/congregation.server.test.ts +++ b/app/shared/domain/congregation.server.test.ts @@ -20,6 +20,13 @@ beforeEach(() => { vi.resetAllMocks() }) +// Answers the slug lookup the way the table would: the row for that slug, or nothing. A test that +// reads the wrong slug from the host then gets no congregation instead of whatever was mocked. +function mockSlugLookup(rows: Record) { + vi.mocked(db.congregation.findUnique).mockImplementation(((args: { where: { slug?: string } }) => + Promise.resolve(rows[args.where.slug ?? ''] ?? null)) as never) +} + const baseCongregation = { id: 1, name: 'Congrégation Test', @@ -180,11 +187,20 @@ describe('findCongregation', () => { await expect(findCongregation(999)).resolves.toBeNull() }) - it('returns the same congregation as resolveCongregation when it exists', async () => { - vi.mocked(db.congregation.findUnique).mockResolvedValue(baseCongregation as never) + it('returns the mapped congregation when it exists', async () => { + vi.mocked(db.congregation.findUnique).mockResolvedValue({ + ...baseCongregation, + locale: 'en', + timezone: 'Europe/London', + } as never) - const found = await findCongregation(1) - expect(found).toEqual(await resolveCongregation(1)) + await expect(findCongregation(1)).resolves.toMatchObject({ + id: 1, + slug: 'test', + displayName: 'Congrégation Test', + locale: 'en', + timezone: 'Europe/London', + }) }) }) @@ -242,26 +258,41 @@ describe('resolveCongregationFromRequest', () => { }) }) - // Regression: the base domain itself was read as the slug "unitae.app" and redirected to - // /congregation-not-found (it serves /setup in production). - it('looks up no slug for the base domain itself', async () => { + // The self-hosting guide writes UNITAE_BASE_URL with its scheme: a subdomain has to resolve either way. + it.each([ + 'unitae.app', + 'https://unitae.app', + 'http://unitae.app', + ])('resolves a subdomain when UNITAE_BASE_URL is %s', async baseUrl => { + process.env.UNITAE_MULTI_TENANT = 'true' + process.env.UNITAE_BASE_URL = baseUrl + mockSlugLookup({ 'tenant-alpha': { id: 1, slug: 'tenant-alpha' } }) + + const result = await resolveCongregationFromRequest(makeRequest('https://tenant-alpha.unitae.app/')) + expect(result).toEqual({ id: 1, slug: 'tenant-alpha' }) + }) + + // Regression: the base domain itself was read as the slug "unitae.app", so every lookup on it + // threw the /congregation-not-found redirect. + it('resolves no congregation for the base domain itself', async () => { process.env.UNITAE_MULTI_TENANT = 'true' process.env.UNITAE_BASE_URL = 'https://unitae.app' + // Whatever slug is read from the host would find this row: none may be read. + vi.mocked(db.congregation.findUnique).mockResolvedValue({ id: 9, slug: 'any-slug' } as never) vi.mocked(db.congregation.findFirst).mockResolvedValue(null as never) - const result = await resolveCongregationFromRequest(makeRequest('https://unitae.app/setup')) + const result = await resolveCongregationFromRequest(makeRequest('https://unitae.app/')) expect(result).toBeNull() - expect(db.congregation.findUnique).not.toHaveBeenCalled() }) it('treats a host that only ends with the base domain as a custom domain, not a subdomain', async () => { process.env.UNITAE_MULTI_TENANT = 'true' process.env.UNITAE_BASE_URL = 'unitae.app' + vi.mocked(db.congregation.findUnique).mockResolvedValue({ id: 9, slug: 'any-slug' } as never) vi.mocked(db.congregation.findFirst).mockResolvedValue({ id: 3, slug: 'lyon' } as never) const result = await resolveCongregationFromRequest(makeRequest('https://notunitae.app/')) expect(result).toEqual({ id: 3, slug: 'lyon' }) - expect(db.congregation.findUnique).not.toHaveBeenCalled() }) it('retourne null pour le domaine racine sans slug ni domaine personnalisé', async () => { @@ -298,24 +329,25 @@ describe('getBrandingName — deterministic single-tenant pick', () => { }) describe('getBrandingName — multi-tenant host', () => { - it('looks up no slug for the base domain itself', async () => { + it('brands the base domain as the platform, not as a congregation', async () => { process.env.UNITAE_MULTI_TENANT = 'true' process.env.UNITAE_BASE_URL = 'unitae.app' + // Whatever slug is read from the host would find this row: none may be read. + vi.mocked(db.congregation.findUnique).mockResolvedValue({ name: 'Slug Match', displayName: null } as never) vi.mocked(db.congregation.findFirst).mockResolvedValue(null as never) - await expect(getBrandingName(new Request('https://unitae.app/setup'))).resolves.toBe('Unitae') - expect(db.congregation.findUnique).not.toHaveBeenCalled() + await expect(getBrandingName(new Request('https://unitae.app/'))).resolves.toBe('Unitae') }) - it("brands a subdomain with its congregation's name", async () => { + it.each([ + 'unitae.app', + 'https://unitae.app', + 'http://unitae.app', + ])("brands a subdomain with its congregation's name when UNITAE_BASE_URL is %s", async baseUrl => { process.env.UNITAE_MULTI_TENANT = 'true' - process.env.UNITAE_BASE_URL = 'unitae.app' - vi.mocked(db.congregation.findUnique).mockResolvedValue({ name: 'Lyon Centre', displayName: null } as never) + process.env.UNITAE_BASE_URL = baseUrl + mockSlugLookup({ lyon: { name: 'Lyon Centre', displayName: null } }) await expect(getBrandingName(new Request('https://lyon.unitae.app/login'))).resolves.toBe('Lyon Centre') - expect(db.congregation.findUnique).toHaveBeenCalledWith({ - where: { slug: 'lyon' }, - select: { name: true, displayName: true }, - }) }) }) diff --git a/app/tests/e2e/health.spec.ts b/app/tests/e2e/health.spec.ts index 9737ecdd..eacc5eff 100644 --- a/app/tests/e2e/health.spec.ts +++ b/app/tests/e2e/health.spec.ts @@ -6,4 +6,10 @@ test.describe('Health check', () => { // We just verify the server is running and responds expect(response.status()).toBeLessThan(500) }) + + test('the liveness endpoint answers', async ({ request }) => { + const response = await request.get('/livez') + expect(response.status()).toBe(200) + expect(await response.text()).toBe('ok') + }) }) From 13f010d2c8a14070aac3e37b9dc43ce258362599 Mon Sep 17 00:00:00 2001 From: mindsers Date: Sun, 11 Oct 2026 09:14:03 +0200 Subject: [PATCH 2/4] fix(i18n): keep a failed locale lookup from changing the visitor's language resolveDocumentLocale answered a failed lookup with the default locale, and entry.server wrote whatever it got to the locale cookie: after one database error, a member of an English congregation read the app in French until their next full page load. A fallback now renders in the locale the visitor's cookie already holds, or the base one, and is not written back. Only a redirect is answered as one. Any other Response thrown by the lookup was handed to the visitor as the page; it is logged and falls back like the other failures. The log line carries the host and the path, so a failure can be tied to the tenant it happened on. The locale is validated where it is resolved, so entry.server no longer checks it again against a second default. --- app/entry.server.test.ts | 37 ++++++++++++++++ app/entry.server.tsx | 11 +++-- app/shared/utils/locale.server.test.ts | 58 ++++++++++++++++++++++---- app/shared/utils/locale.server.ts | 46 ++++++++++++++------ 4 files changed, 129 insertions(+), 23 deletions(-) diff --git a/app/entry.server.test.ts b/app/entry.server.test.ts index 0e558b19..5069e812 100644 --- a/app/entry.server.test.ts +++ b/app/entry.server.test.ts @@ -17,6 +17,14 @@ vi.mock('~/shared/utils/locale.server', () => ({ resolveDocumentLocale: vi.fn(), })) +// Rendering is React's part: report the shell ready at once, so handleRequest answers. +vi.mock('react-dom/server', () => ({ + renderToPipeableStream: (_element: unknown, options: { onShellReady?: () => void; onAllReady?: () => void }) => { + queueMicrotask(() => (options.onShellReady ?? options.onAllReady)?.()) + return { pipe: (body: { end: () => void }) => body.end(), abort: () => {} } + }, +})) + const { default: handleRequest } = await import('./entry.server') const { resolveDocumentLocale } = await import('~/shared/utils/locale.server') @@ -39,4 +47,33 @@ describe('handleRequest', () => { expect(response).toBe(redirect) expect(responseHeaders.has('Set-Cookie')).toBe(false) }) + + it('remembers a resolved locale in the locale cookie', async () => { + vi.mocked(resolveDocumentLocale).mockResolvedValue({ locale: 'en', persist: true }) + + const response = await handleRequest( + new Request('https://lyon.unitae.app/login'), + 200, + new Headers(), + {} as never, + {} as never, + ) + + expect(response.headers.get('Set-Cookie')).toContain('PARAGLIDE_LOCALE=en;') + }) + + // A failed lookup renders in a fallback locale. Writing that one down replaced the visitor's own. + it('leaves the locale cookie alone when the locale is a fallback', async () => { + vi.mocked(resolveDocumentLocale).mockResolvedValue({ locale: 'fr', persist: false }) + + const response = await handleRequest( + new Request('https://lyon.unitae.app/login'), + 200, + new Headers(), + {} as never, + {} as never, + ) + + expect(response.headers.get('Set-Cookie')).toBeNull() + }) }) diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 7b28e79c..00ba25b8 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -8,7 +8,7 @@ import { renderToPipeableStream } from 'react-dom/server' import type { EntryContext, RouterContextProvider } from 'react-router' import { ServerRouter } from 'react-router' -import { baseLocale, isLocale, type Locale, overwriteGetLocale } from '~/i18n/paraglide/runtime' +import { baseLocale, type Locale, overwriteGetLocale } from '~/i18n/paraglide/runtime' import logger from '~/shared/infra/logger.server' import { resolveDocumentLocale } from '~/shared/utils/locale.server' @@ -30,11 +30,14 @@ export default async function handleRequest( const resolved = await resolveDocumentLocale(request) // A subdomain no congregation owns: send the visitor to the page that says so, as the loaders do. if ('redirect' in resolved) return resolved.redirect - const locale = isLocale(resolved.locale) ? resolved.locale : baseLocale + const { locale } = resolved // Set the PARAGLIDE_LOCALE cookie via HTTP header so the client-side - // cookie strategy can read it during hydration (before any JS executes) - responseHeaders.append('Set-Cookie', `PARAGLIDE_LOCALE=${locale}; Path=/; Max-Age=34560000; SameSite=Lax`) + // cookie strategy can read it during hydration (before any JS executes). + // A fallback locale is not written: it would replace the one the visitor already has. + if (resolved.persist) { + responseHeaders.append('Set-Cookie', `PARAGLIDE_LOCALE=${locale}; Path=/; Max-Age=34560000; SameSite=Lax`) + } return localeStore.run(locale, () => { return isbot(request.headers.get('user-agent') || '') diff --git a/app/shared/utils/locale.server.test.ts b/app/shared/utils/locale.server.test.ts index c3192f2f..3b12d943 100644 --- a/app/shared/utils/locale.server.test.ts +++ b/app/shared/utils/locale.server.test.ts @@ -20,6 +20,11 @@ vi.mock('~/shared/infra/logger.server', () => ({ default: { warn: vi.fn(), error: vi.fn(), info: vi.fn() }, })) +vi.mock('~/i18n/paraglide/runtime', () => ({ + baseLocale: 'fr', + isLocale: (value: unknown) => value === 'fr' || value === 'en', +})) + const { resolveDocumentLocale, resolveLocaleFromRequest } = await import('./locale.server') const { getSession } = await import('~/features/authentication/server/session.server') const { unscopedDb: db } = await import('~/shared/infra/db.server') @@ -30,8 +35,8 @@ beforeEach(() => { delete process.env.UNITAE_MULTI_TENANT }) -function makeRequest(url = 'http://localhost:5173/board') { - return new Request(url) +function makeRequest(url = 'http://localhost:5173/board', headers: Record = {}) { + return new Request(url, { headers }) } function mockSession(userId: number | typeof NaN) { @@ -153,12 +158,21 @@ describe("resolveLocaleFromRequest — the session's congregation no longer exis describe('resolveDocumentLocale', () => { const notFoundRedirect = () => new Response(null, { status: 302, headers: { Location: '/congregation-not-found' } }) - it("returns the request's locale", async () => { + it('returns the resolved locale, to be remembered', async () => { mockSession(42) vi.mocked(db.userAccount.findUnique).mockResolvedValue({ congregationId: 1 } as never) vi.mocked(findCongregation).mockResolvedValue({ locale: 'en' } as never) - await expect(resolveDocumentLocale(makeRequest())).resolves.toEqual({ locale: 'en' }) + await expect(resolveDocumentLocale(makeRequest())).resolves.toEqual({ locale: 'en', persist: true }) + }) + + // Congregation.locale is a free-text column: what comes out of it is not always a locale the app ships. + it('renders in the base locale when the congregation holds a locale the app does not ship', async () => { + mockSession(42) + vi.mocked(db.userAccount.findUnique).mockResolvedValue({ congregationId: 1 } as never) + vi.mocked(findCongregation).mockResolvedValue({ locale: 'de' } as never) + + await expect(resolveDocumentLocale(makeRequest())).resolves.toEqual({ locale: 'fr', persist: true }) }) // Regression: any page on a subdomain no congregation owns, and any unknown path there, was a 500. @@ -180,14 +194,44 @@ describe('resolveDocumentLocale', () => { await expect( resolveDocumentLocale(makeRequest('https://inconnu.unitae.app/congregation-not-found')), - ).resolves.toEqual({ locale: 'fr' }) + ).resolves.toEqual({ locale: 'fr', persist: false }) }) - it('falls back to the default locale when the lookup fails', async () => { + it('falls back to the base locale when the lookup fails, without remembering it', async () => { mockSession(42) vi.mocked(db.userAccount.findUnique).mockRejectedValue(new Error('connection refused')) - await expect(resolveDocumentLocale(makeRequest())).resolves.toEqual({ locale: 'fr' }) + await expect(resolveDocumentLocale(makeRequest())).resolves.toEqual({ locale: 'fr', persist: false }) + }) + + // The fallback is nobody's locale. Written to the locale cookie, it switched an English + // congregation's member to French until their next full page load. + it("keeps the visitor's own locale when the lookup fails", async () => { + mockSession(42) + vi.mocked(db.userAccount.findUnique).mockRejectedValue(new Error('connection refused')) + const request = makeRequest(undefined, { Cookie: 'theme=dark; PARAGLIDE_LOCALE=en' }) + + await expect(resolveDocumentLocale(request)).resolves.toEqual({ locale: 'en', persist: false }) + }) + + it('ignores a locale cookie the app does not ship', async () => { + mockSession(42) + vi.mocked(db.userAccount.findUnique).mockRejectedValue(new Error('connection refused')) + const request = makeRequest(undefined, { Cookie: 'PARAGLIDE_LOCALE=de' }) + + await expect(resolveDocumentLocale(request)).resolves.toEqual({ locale: 'fr', persist: false }) + }) + + // Only a redirect may be answered as one. Any other Response thrown by the lookup would have + // been sent to the visitor as the page. + it('falls back instead of answering with a thrown Response that is not a redirect', async () => { + mockSession(NaN) + vi.mocked(resolveCongregationFromRequest).mockRejectedValue(new Response('forbidden', { status: 403 })) + + await expect(resolveDocumentLocale(makeRequest('https://inconnu.unitae.app/nope'))).resolves.toEqual({ + locale: 'fr', + persist: false, + }) }) }) diff --git a/app/shared/utils/locale.server.ts b/app/shared/utils/locale.server.ts index d9acc949..b01317ca 100644 --- a/app/shared/utils/locale.server.ts +++ b/app/shared/utils/locale.server.ts @@ -1,9 +1,11 @@ import { getSession } from '~/features/authentication/server/session.server' +import { baseLocale, isLocale, type Locale } from '~/i18n/paraglide/runtime' import { findCongregation, resolveCongregationFromRequest } from '~/shared/domain/congregation.server' import { unscopedDb } from '~/shared/infra/db.server' import logger from '~/shared/infra/logger.server' const DEFAULT_LOCALE = 'fr' +const LOCALE_COOKIE = /(?:^|;\s*)PARAGLIDE_LOCALE=([^;]+)/ export async function resolveLocaleFromRequest(request: Request): Promise { // 1. Try session → user → congregation → locale (authenticated users) @@ -49,7 +51,11 @@ export async function resolveLocaleFromRequest(request: Request): Promise { try { - return { locale: await resolveLocaleFromRequest(request) } + const locale = await resolveLocaleFromRequest(request) + return { locale: isLocale(locale) ? locale : baseLocale, persist: true } } catch (error) { - if (error instanceof Response) { - return redirectsToItself(request, error) ? { locale: DEFAULT_LOCALE } : { redirect: error } + if (!isRedirect(error)) { + const { host, pathname } = new URL(request.url) + logger.error('resolveDocumentLocale: locale lookup failed, rendering in a fallback locale', { + // A Response has no enumerable fields: logged as it is, it would read `{}`. + error: error instanceof Response ? `Response ${error.status}` : error, + host, + path: pathname, + }) + } else if (!redirectsToItself(request, error)) { + return { redirect: error } } - logger.error('resolveDocumentLocale: locale lookup failed, rendering in the default locale', { error }) - return { locale: DEFAULT_LOCALE } + return { locale: localeFromCookie(request) ?? baseLocale, persist: false } } } -function redirectsToItself(request: Request, response: Response): boolean { - const location = response.headers.get('Location') - if (location == null) return false +function isRedirect(error: unknown): error is Response { + return error instanceof Response && error.status >= 300 && error.status < 400 && error.headers.has('Location') +} + +function redirectsToItself(request: Request, redirect: Response): boolean { const current = new URL(request.url) - return new URL(location, current).pathname === current.pathname + return new URL(redirect.headers.get('Location') ?? '', current).pathname === current.pathname +} + +function localeFromCookie(request: Request): Locale | null { + const value = LOCALE_COOKIE.exec(request.headers.get('Cookie') ?? '')?.[1] + return isLocale(value) ? value : null } From 6806c66824c89dcb2d9679f46486b321310098ee Mon Sep 17 00:00:00 2001 From: mindsers Date: Sun, 11 Oct 2026 09:16:09 +0200 Subject: [PATCH 3/4] fix(multi-tenant): read the base domain out of UNITAE_BASE_URL however it is written The suffix a subdomain is matched against was UNITAE_BASE_URL with its scheme cut off. Written with a port or a trailing slash, it matched no host: every congregation subdomain was served as the platform root, without the check that a session belongs to the congregation in the URL, and with nothing in the logs. Set but left empty, it did the same. The hostname is now parsed out of the value, so a scheme, a port, a trailing slash or capitals all resolve, and an empty value reads as unset. validateEnv warns about a value that names no host. It does not refuse it: an instance that only serves custom domains runs without a base domain. A host with nothing before the base domain's dot no longer yields an empty slug. --- app/shared/domain/congregation.server.test.ts | 17 +++++++ app/shared/domain/congregation.server.ts | 8 +-- app/shared/utils/env.server.test.ts | 49 ++++++++++++++++++- app/shared/utils/env.server.ts | 30 ++++++++++++ 4 files changed, 99 insertions(+), 5 deletions(-) diff --git a/app/shared/domain/congregation.server.test.ts b/app/shared/domain/congregation.server.test.ts index 60c6787c..d63574c2 100644 --- a/app/shared/domain/congregation.server.test.ts +++ b/app/shared/domain/congregation.server.test.ts @@ -263,6 +263,9 @@ describe('resolveCongregationFromRequest', () => { 'unitae.app', 'https://unitae.app', 'http://unitae.app', + 'https://unitae.app/', + 'https://unitae.app:8443', + 'https://Unitae.App', ])('resolves a subdomain when UNITAE_BASE_URL is %s', async baseUrl => { process.env.UNITAE_MULTI_TENANT = 'true' process.env.UNITAE_BASE_URL = baseUrl @@ -272,6 +275,17 @@ describe('resolveCongregationFromRequest', () => { expect(result).toEqual({ id: 1, slug: 'tenant-alpha' }) }) + // Set but left empty, the variable matched no host at all: every subdomain was served as the + // platform root, with nothing to say why. + it('reads an empty UNITAE_BASE_URL as unset', async () => { + process.env.UNITAE_MULTI_TENANT = 'true' + process.env.UNITAE_BASE_URL = '' + mockSlugLookup({ 'tenant-alpha': { id: 1, slug: 'tenant-alpha' } }) + + const result = await resolveCongregationFromRequest(makeRequest('https://tenant-alpha.unitae.app/')) + expect(result).toEqual({ id: 1, slug: 'tenant-alpha' }) + }) + // Regression: the base domain itself was read as the slug "unitae.app", so every lookup on it // threw the /congregation-not-found redirect. it('resolves no congregation for the base domain itself', async () => { @@ -343,6 +357,9 @@ describe('getBrandingName — multi-tenant host', () => { 'unitae.app', 'https://unitae.app', 'http://unitae.app', + 'https://unitae.app/', + 'https://unitae.app:8443', + 'https://Unitae.App', ])("brands a subdomain with its congregation's name when UNITAE_BASE_URL is %s", async baseUrl => { process.env.UNITAE_MULTI_TENANT = 'true' process.env.UNITAE_BASE_URL = baseUrl diff --git a/app/shared/domain/congregation.server.ts b/app/shared/domain/congregation.server.ts index 6f0dbab0..d6544590 100644 --- a/app/shared/domain/congregation.server.ts +++ b/app/shared/domain/congregation.server.ts @@ -1,6 +1,7 @@ import { redirect } from 'react-router' import { unscopedDb } from '~/shared/infra/db.server' +import { getBaseDomain } from '~/shared/utils/env.server' const DEFAULT_PLATFORM_NAME = 'Unitae' const DEFAULT_EMAIL_FROM = process.env.UNITAE_EMAIL_FROM ?? 'Unitae ' @@ -91,12 +92,11 @@ export function getPlatformName(): string { /** * The congregation slug in a `.` hostname. `null` for the base domain itself — it * is the platform, not a congregation — and for any other host, including one that merely ends with - * the same letters (`notunitae.app`). + * the same letters (`notunitae.app`) or has nothing before the dot. */ function slugFromHostname(hostname: string): string | null { - const appBaseUrl = (process.env.UNITAE_BASE_URL ?? 'unitae.app').replace('https://', '').replace('http://', '') - const suffix = `.${appBaseUrl}` - return hostname.endsWith(suffix) ? hostname.slice(0, -suffix.length) : null + const suffix = `.${getBaseDomain()}` + return hostname.endsWith(suffix) ? hostname.slice(0, -suffix.length) || null : null } /** diff --git a/app/shared/utils/env.server.test.ts b/app/shared/utils/env.server.test.ts index 5ad11da8..4a24a81e 100644 --- a/app/shared/utils/env.server.test.ts +++ b/app/shared/utils/env.server.test.ts @@ -4,7 +4,14 @@ vi.mock('~/shared/infra/logger.server', () => ({ default: { warn: vi.fn(), info: vi.fn(), error: vi.fn() }, })) -const ENV_KEYS = ['DB_URL', 'UNITAE_SESSION_SECRET', 'DB_RUNTIME_URL', 'UNITAE_CRON_SECRET'] as const +const ENV_KEYS = [ + 'DB_URL', + 'UNITAE_SESSION_SECRET', + 'DB_RUNTIME_URL', + 'UNITAE_CRON_SECRET', + 'UNITAE_BASE_URL', + 'UNITAE_MULTI_TENANT', +] as const describe('env.server', () => { const original: Record = {} @@ -166,4 +173,44 @@ describe('env.server', () => { expect(getSessionSecrets()).toEqual([]) }) }) + + describe('getBaseDomain', () => { + it.each([ + ['unitae.example.com', 'unitae.example.com'], + ['https://unitae.example.com', 'unitae.example.com'], + ['http://unitae.example.com', 'unitae.example.com'], + ['https://unitae.example.com/', 'unitae.example.com'], + ['https://unitae.example.com:8443', 'unitae.example.com'], + ['https://Unitae.Example.com', 'unitae.example.com'], + ])('reads the hostname out of %s', async (value, hostname) => { + const { getBaseDomain } = await import('./env.server') + process.env.UNITAE_BASE_URL = value + expect(getBaseDomain()).toBe(hostname) + }) + + it('falls back to unitae.app when the var is unset', async () => { + const { getBaseDomain } = await import('./env.server') + expect(getBaseDomain()).toBe('unitae.app') + }) + + it.each(['', 'not a host'])('falls back to unitae.app when the var is %j', async value => { + const { getBaseDomain } = await import('./env.server') + process.env.UNITAE_BASE_URL = value + expect(getBaseDomain()).toBe('unitae.app') + }) + }) + + // A value that names no host leaves subdomains resolved against the default domain. Boot reports + // it and carries on: an instance that only serves custom domains runs fine without one. + it.each([ + '', + 'not a host', + ])('validateEnv does not refuse a multi-tenant boot when UNITAE_BASE_URL is %j', async value => { + const { validateEnv } = await import('./env.server') + process.env.DB_URL = 'postgresql://localhost/db' + process.env.UNITAE_SESSION_SECRET = 'a-sufficiently-long-test-session-secret-value' + process.env.UNITAE_MULTI_TENANT = 'true' + process.env.UNITAE_BASE_URL = value + expect(() => validateEnv()).not.toThrow() + }) }) diff --git a/app/shared/utils/env.server.ts b/app/shared/utils/env.server.ts index 9868c8bb..0036120b 100644 --- a/app/shared/utils/env.server.ts +++ b/app/shared/utils/env.server.ts @@ -2,6 +2,7 @@ import logger from '~/shared/infra/logger.server' const MIN_SESSION_SECRET_LENGTH = 32 const PLACEHOLDER_SESSION_SECRETS = ['change-me-with-a-real-secret-key'] +const DEFAULT_BASE_DOMAIN = 'unitae.app' // Fail closed: treat anything that is not an explicit development/test environment (including an // unset or misspelled NODE_ENV) as production, mirroring the RLS guard (see rls-guard.server.ts). @@ -32,6 +33,33 @@ export function getSessionSecrets(): string[] { .filter(Boolean) } +function parseHostname(value: string): string | null { + try { + return new URL(value.includes('://') ? value : `https://${value}`).hostname || null + } catch { + return null + } +} + +// The hostname in UNITAE_BASE_URL, whether it is written with a scheme, a port or a trailing slash +// or without. Non-throwing for the same reason as getSessionSecrets: it runs on every multi-tenant +// request, so validateEnv() reports a value that names no host once, at boot. +export function getBaseDomain(): string { + return parseHostname(process.env.UNITAE_BASE_URL || DEFAULT_BASE_DOMAIN) ?? DEFAULT_BASE_DOMAIN +} + +// Reported, not refused: an instance that only serves custom domains runs fine without a base +// domain, so a boot failure here would break setups that work. +function warnOnUnusableBaseUrl() { + const baseUrl = process.env.UNITAE_BASE_URL + if (process.env.UNITAE_MULTI_TENANT !== 'true' || baseUrl === undefined || parseHostname(baseUrl) != null) return + + logger.warn( + `UNITAE_BASE_URL names no host: congregation subdomains are resolved against ${DEFAULT_BASE_DOMAIN}. ` + + 'Set it to your root domain, e.g. https://unitae.example.com.', + ) +} + // Enforces that every configured session secret (current + any rotated previous ones) is long // enough and is not the shipped placeholder. Fails closed in production; warns in development so // local setups keep working. Presence is checked first via requireEnv. @@ -77,4 +105,6 @@ export function validateEnv() { if (!process.env.UNITAE_CRON_SECRET) { logger.warn('UNITAE_CRON_SECRET is not set. Cron endpoints will reject all requests.') } + + warnOnUnusableBaseUrl() } From 11d53c12186e69ca7c1638906554fe8e4076926d Mon Sep 17 00:00:00 2001 From: mindsers Date: Sun, 11 Oct 2026 09:17:21 +0200 Subject: [PATCH 4/4] docs(multi-tenant): say what the missing-congregation branch guards, and fix the probe guide Five comments described the missing-congregation branch as a session outliving its congregation, with a 500 on every page. The foreign key forbids an account without its congregation, and once the accounts are gone the guard already redirected: the branch only covers a deletion racing a request. The comments say so now, and findCongregation's doc names its real callers. "As the loaders do" named no loader, and a test file repeated the JSDoc on what React Router does with a throw in entry.server. In the health-monitoring guide, the Compose example probed with curl, which the runtime image does not ship, so the container reported unhealthy forever. It uses Node's fetch, as docker/docker-compose.yml does. The guide also says a liveness restart takes several failed probes, and drops a readiness delay the startup probe already covers. --- app/entry.server.tsx | 4 +++- .../server/session.server.test.ts | 6 ++--- .../authentication/server/session.server.ts | 4 +++- app/shared/domain/congregation.server.ts | 13 ++++++----- app/shared/utils/locale.server.test.ts | 8 +++---- app/shared/utils/locale.server.ts | 2 +- app/shell/livez.test.ts | 2 +- app/shell/livez.tsx | 4 ++-- docs/self-hosting/health-monitoring.md | 22 ++++++++++++++----- 9 files changed, 40 insertions(+), 25 deletions(-) diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 00ba25b8..77d6bd34 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -28,7 +28,9 @@ export default async function handleRequest( _loadContext: RouterContextProvider, ) { const resolved = await resolveDocumentLocale(request) - // A subdomain no congregation owns: send the visitor to the page that says so, as the loaders do. + // A subdomain no congregation owns. The sign-in loaders and the session guard send it to + // /congregation-not-found themselves; this covers the documents that get here without either + // (public shell pages, unmatched paths). if ('redirect' in resolved) return resolved.redirect const { locale } = resolved diff --git a/app/features/authentication/server/session.server.test.ts b/app/features/authentication/server/session.server.test.ts index bd89f308..46e2c8ca 100644 --- a/app/features/authentication/server/session.server.test.ts +++ b/app/features/authentication/server/session.server.test.ts @@ -244,9 +244,9 @@ describe('verifySession', () => { expect(response.headers.get('Location')).toBe('/login?redirectTo=%2Fterritories%2F1%3Fx%3D2') }) - // Regression: a session outliving its congregation used to throw a plain Error out of the - // guard, which every authenticated page answered with a 500. - it("redirects to /login and drops the session when the account's congregation no longer exists", async () => { + // The account's congregation row is missing: a deletion racing the request. The guard used to let + // resolveCongregation's plain Error through, a 500 instead of the sign-in page. + it('sends an account whose congregation is gone to /login, without a return URL or its session', async () => { setSession({ userId: '42', sessionEpoch: '0' }) vi.mocked(db.userAccount.findUnique).mockResolvedValue(fakeUser as never) vi.mocked(resolveCongregationFromRequest).mockResolvedValue(null as never) diff --git a/app/features/authentication/server/session.server.ts b/app/features/authentication/server/session.server.ts index bf017ad7..7db8f63d 100644 --- a/app/features/authentication/server/session.server.ts +++ b/app/features/authentication/server/session.server.ts @@ -139,7 +139,9 @@ export async function verifySession(request: Request) { const congregation = await findCongregation(user.congregationId) if (congregation == null) { - // The cookie outlived its congregation: drop it and send the visitor to sign in again. + // No congregation behind this account. The foreign key forbids that as a lasting state, so a + // deletion is racing this request: drop the cookie and send the visitor to sign in, without a + // return URL. logger.warn(`verifySession: congregation ${user.congregationId} of userId ${user.id} no longer exists`) return redirectToLogin(session) } diff --git a/app/shared/domain/congregation.server.ts b/app/shared/domain/congregation.server.ts index d6544590..0fdb5195 100644 --- a/app/shared/domain/congregation.server.ts +++ b/app/shared/domain/congregation.server.ts @@ -42,8 +42,10 @@ export async function resolveCongregation(congregationId: number): Promise { const congregation = await unscopedDb.congregation.findUnique({ @@ -102,9 +104,10 @@ function slugFromHostname(hostname: string): string | null { /** * Resolves the congregation matching the subdomain or custom domain from the request. * - * - Returns `null` in single-tenant mode, or for a host that is neither a subdomain of the base domain - * nor a congregation's custom domain (the base domain itself among them). - * - Redirects to `/congregation-not-found` if a slug is present but doesn't match any congregation. + * - Returns `null` in single-tenant mode, and for any host that is neither `.` + * nor a congregation's custom domain. That includes the base domain itself. + * - Throws a redirect to `/congregation-not-found` for `.` when no congregation + * has that slug. The custom-domain lookup is not tried in that case. */ export async function resolveCongregationFromRequest(request: Request): Promise<{ id: number; slug: string } | null> { if (process.env.UNITAE_MULTI_TENANT !== 'true') return null diff --git a/app/shared/utils/locale.server.test.ts b/app/shared/utils/locale.server.test.ts index 3b12d943..c43d48b1 100644 --- a/app/shared/utils/locale.server.test.ts +++ b/app/shared/utils/locale.server.test.ts @@ -138,8 +138,8 @@ describe('resolveLocaleFromRequest', () => { }) }) -// Regression: a session whose congregation was deleted threw out of the locale lookup, which runs -// for every page, so that visitor got a 500 everywhere instead of the sign-in page. +// The same race as in verifySession: the account's congregation row is missing. resolveCongregation's +// Error used to escape the locale lookup, which runs for every document. describe("resolveLocaleFromRequest — the session's congregation no longer exists", () => { it("falls through to the request's congregation", async () => { mockSession(42) @@ -152,9 +152,7 @@ describe("resolveLocaleFromRequest — the session's congregation no longer exis }) }) -// resolveDocumentLocale runs in entry.server's handleRequest, after the loaders: a Response thrown -// there is not turned into the response, React Router re-renders, the lookup throws again and the -// visitor gets the last-resort 500. Every outcome has to come back as a value. +// resolveDocumentLocale must never throw: its JSDoc says what a throw costs in entry.server. describe('resolveDocumentLocale', () => { const notFoundRedirect = () => new Response(null, { status: 302, headers: { Location: '/congregation-not-found' } }) diff --git a/app/shared/utils/locale.server.ts b/app/shared/utils/locale.server.ts index b01317ca..0e83ba24 100644 --- a/app/shared/utils/locale.server.ts +++ b/app/shared/utils/locale.server.ts @@ -19,7 +19,7 @@ export async function resolveLocaleFromRequest(request: Request): Promise { - it('answers 200 ok without a cacheable body', async () => { + it('answers 200 ok and forbids caching', async () => { const response = loader() expect(response.status).toBe(200) diff --git a/app/shell/livez.tsx b/app/shell/livez.tsx index 5e189a54..33df1d2a 100644 --- a/app/shell/livez.tsx +++ b/app/shell/livez.tsx @@ -1,7 +1,7 @@ // Liveness: answers as long as the server process can serve a request, and checks nothing else. // /health also needs PostgreSQL and Redis; as a liveness probe it would restart every pod at once -// when either one blips, which brings neither back. Use /health for readiness, this for liveness -// and startup. +// when either one stays unreachable for a few probe periods, which brings neither back. Use /health +// for readiness, this for liveness and startup. export function loader() { return new Response('ok', { status: 200, diff --git a/docs/self-hosting/health-monitoring.md b/docs/self-hosting/health-monitoring.md index 4e79a6d0..dc258491 100644 --- a/docs/self-hosting/health-monitoring.md +++ b/docs/self-hosting/health-monitoring.md @@ -20,8 +20,9 @@ Checks nothing but the web process itself: it answers as long as the server can - **200 OK** — `text/plain` body `ok`, `Cache-Control: no-store` Use it for liveness and startup probes, and `/health` for readiness. A liveness probe on `/health` -restarts every web pod at once when PostgreSQL or Redis is briefly unreachable, which brings neither -back; readiness only takes the pods out of the load balancer until the dependencies answer again. +restarts every web pod at once when PostgreSQL or Redis stays unreachable for a few probe periods, +which brings neither back; readiness only takes the pods out of the load balancer until the +dependencies answer again. Neither endpoint needs to be public: the orchestrator reaches them on the container port. @@ -39,13 +40,23 @@ services: web: # ... healthcheck: - test: ["CMD", "curl", "-sf", "http://localhost:8080/health"] + # The runtime image has no curl: Node's own fetch does the probe. + test: + [ + 'CMD', + 'node', + '-e', + "fetch('http://localhost:8080/health').then(r => process.exit(r.ok ? 0 : 1)).catch(() => process.exit(1))", + ] interval: 30s - timeout: 5s + timeout: 10s retries: 3 - start_period: 10s ``` +Compose reports this status and uses it to order `depends_on`; it does not restart an unhealthy +container. If your setup does (Docker Swarm, an autoheal sidecar), probe `/livez` instead, for the +reason given above. + ### Kubernetes probes ```yaml @@ -64,7 +75,6 @@ readinessProbe: httpGet: path: /health port: 8080 - initialDelaySeconds: 5 periodSeconds: 10 ```