diff --git a/apps/api/package.json b/apps/api/package.json index 6a7a28d..603545d 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -29,6 +29,7 @@ "@ekklesia/typescript-config": "workspace:*", "@types/node": "24.13.3", "@types/pg": "8.20.0", + "better-auth": "1.6.23", "pg": "8.22.0", "typescript": "7.0.2", "vitest": "4.1.10" diff --git a/apps/api/src/server.ts b/apps/api/src/server.ts index dada6eb..e492ab4 100644 --- a/apps/api/src/server.ts +++ b/apps/api/src/server.ts @@ -12,6 +12,7 @@ import { createApiApp } from "./app.js"; import { type ApiRuntimeConfig, createApiRuntimeConfig } from "./config.js"; const connectionTimeoutMs = 10_000; +const operationTimeoutMs = 10_000; export interface RunningApi { readonly app: Hono; @@ -65,6 +66,7 @@ export async function startApi( ): Promise { const databaseHandle = createIdentityDrizzleDatabase(config.databaseUrl, { connectionTimeoutMs, + operationTimeoutMs, }); let runtime: ReturnType; @@ -89,21 +91,53 @@ export async function startApi( throw error; } - let shuttingDown = false; - const shutdown = async (): Promise => { - if (shuttingDown) { - return; + // Memoizes one shared promise rather than a boolean flag: a boolean only prevents a *second* + // cleanup from starting, but a caller that arrives while the first cleanup is still in flight + // would previously get an already-resolved `undefined` immediately, before the listener had + // actually drained or the pool had actually closed. Storing the promise itself means every + // caller — concurrent or sequential — awaits the exact same drain-then-close operation and + // observes it complete (or fail) exactly once. + let shutdownPromise: Promise | undefined; + const shutdown = (): Promise => { + if (!shutdownPromise) { + shutdownPromise = (async () => { + await new Promise((resolve) => { + server.close(() => resolve()); + }); + await safeCloseDatabase(databaseHandle); + })(); } - shuttingDown = true; - await new Promise((resolve) => { - server.close(() => resolve()); - }); - await safeCloseDatabase(databaseHandle); + return shutdownPromise; }; return Object.freeze({ app, server, shutdown }); } +/** + * Wires `SIGINT`/`SIGTERM` to `running.shutdown()` and only then exits — extracted from `main()` + * so it can be exercised directly in tests with a fake `running`/`exit`, without sending a real + * process signal or invoking the real `process.exit` (which would terminate the test runner + * itself). Production composition (`main()`, below) calls this with real `process.exit`. + */ +export function installShutdownSignalHandlers( + running: Pick, + exit: (code: number) => void = (code) => process.exit(code), +): void { + let signalReceived = false; + const handleSignal = (): void => { + if (signalReceived) { + return; + } + signalReceived = true; + running + .shutdown() + .then(() => exit(0)) + .catch(() => exit(1)); + }; + process.on("SIGINT", handleSignal); + process.on("SIGTERM", handleSignal); +} + function readConfigFromEnvironment(): { readonly input: Parameters[0]; readonly environment: "development" | "production"; @@ -138,19 +172,7 @@ export async function main(): Promise { const boundPort = address && typeof address === "object" ? address.port : config.port; console.log(`Ekklesia API listening on ${config.host}:${boundPort}`); - let signalReceived = false; - const handleSignal = (): void => { - if (signalReceived) { - return; - } - signalReceived = true; - running - .shutdown() - .then(() => process.exit(0)) - .catch(() => process.exit(1)); - }; - process.on("SIGINT", handleSignal); - process.on("SIGTERM", handleSignal); + installShutdownSignalHandlers(running); } function isDirectExecution(): boolean { diff --git a/apps/api/tests/api-http.integration.ts b/apps/api/tests/api-http.integration.ts index c53a031..06b4d70 100644 --- a/apps/api/tests/api-http.integration.ts +++ b/apps/api/tests/api-http.integration.ts @@ -1,4 +1,5 @@ import { randomBytes } from "node:crypto"; +import { request as httpRequest } from "node:http"; import { fileURLToPath } from "node:url"; import path from "node:path"; import { @@ -9,7 +10,7 @@ import { import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { Client, type QueryResultRow } from "pg"; -import { afterAll, describe, expect, it } from "vitest"; +import { afterAll, describe, expect, it, vi } from "vitest"; import { createApiRuntimeConfig } from "../src/config.js"; import { startApi, type RunningApi } from "../src/server.js"; @@ -245,7 +246,12 @@ async function signUp( ): Promise { const response = await fetch(`${origin}/api/auth/sign-up/email`, { body: JSON.stringify({ email, name: "Integration Test User", password }), - headers: { "content-type": "application/json", ...headers }, + // A trusted Origin by default, matching signIn below: Node's own fetch() (and every browser) + // automatically sends `Sec-Fetch-Mode: cors`, which Better Auth's first-login CSRF protection + // treats as "this is a Fetch-API call" and requires a trusted Origin/Referer for regardless of + // whether a cookie is present — see the dedicated no-Origin scenario below, which uses a raw + // `http.request` instead of `fetch()` to exercise a genuinely Fetch-Metadata-free caller. + headers: { "content-type": "application/json", origin: dashboardOrigin, ...headers }, method: "POST", }); return { body: (await response.clone().json()) as SignUpResult["body"], response }; @@ -283,6 +289,45 @@ async function fetchPrincipal( return { body: await response.clone().json(), response }; } +/** + * A raw `node:http` POST, deliberately not `fetch()`. Node's own `fetch()` (undici) — like every + * browser — automatically sends `Sec-Fetch-Mode: cors` on every request, which Better Auth's + * first-login CSRF protection (`formCsrfMiddleware`) treats as "this came from a Fetch-API + * client" and requires a trusted Origin/Referer for regardless of cookie presence. A genuine + * non-browser, non-Fetch server-to-server integration (curl, most backend HTTP client libraries, + * or this raw socket) sends none of the Fetch Metadata headers, so it is unaffected by that + * specific protection and needs no Origin header at all. + */ +function rawPostRequest( + origin: string, + requestPath: string, + body: unknown, +): Promise<{ body: string; status: number }> { + return new Promise((resolve, reject) => { + const url = new URL(requestPath, origin); + const payload = JSON.stringify(body); + const req = httpRequest( + url, + { + headers: { + "content-length": Buffer.byteLength(payload), + "content-type": "application/json", + }, + method: "POST", + }, + (res) => { + const chunks: Buffer[] = []; + res.on("data", (chunk) => chunks.push(chunk as Buffer)); + res.on("end", () => { + resolve({ body: Buffer.concat(chunks).toString("utf8"), status: res.statusCode ?? 0 }); + }); + }, + ); + req.on("error", reject); + req.end(payload); + }); +} + describe.sequential("live API — signup, sign-in, principal, sign-out", () => { let database: { name: string; url: string }; let running: RunningApi; @@ -495,9 +540,13 @@ describe.sequential("live API — signup, sign-in, principal, sign-out", () => { } }); - it("supports a server-to-server sign-up request that carries no Origin header at all", async () => { - const { response } = await signUp(origin, "carol@example.test", password); - expect(response.status).toBe(200); + it("supports a genuine server-to-server sign-up request that carries no Origin header and no Fetch Metadata headers at all", async () => { + const { status } = await rawPostRequest(origin, "/api/auth/sign-up/email", { + email: "carol@example.test", + name: "Integration Test User", + password, + }); + expect(status).toBe(200); }); it("allows a trusted browser origin for the principal route with credentials", async () => { @@ -517,6 +566,84 @@ describe.sequential("live API — signup, sign-in, principal, sign-out", () => { expect(principalResponse.headers.get("access-control-allow-origin")).toBeNull(); }); + // Production-equivalent Better Auth origin/CSRF protection (ADR 0012). Explicitly setting + // `advanced: { disableCSRFCheck: false, disableOriginCheck: false }` (`core/src/identity/runtime.ts`) + // removes Better Auth's own `isTest()`-driven default that otherwise silently skips origin/CSRF + // validation under a test runner, so the same behavior verified below is also what production + // enforces. + + it("accepts a no-cookie first authentication attempt from a trusted browser origin", async () => { + const response = await fetch(`${origin}/api/auth/sign-in/email`, { + body: JSON.stringify({ email: emailOne, password }), + headers: { "content-type": "application/json", origin: dashboardOrigin }, + method: "POST", + }); + + expect(response.status).toBe(200); + expect(response.headers.get("access-control-allow-origin")).toBe(dashboardOrigin); + }); + + it("rejects a no-cookie first authentication attempt from an untrusted browser origin", async () => { + const response = await fetch(`${origin}/api/auth/sign-in/email`, { + body: JSON.stringify({ email: emailOne, password }), + headers: { "content-type": "application/json", origin: untrustedOrigin }, + method: "POST", + }); + const text = await response.text(); + + expect(response.status).toBe(403); + expect(response.headers.get("access-control-allow-origin")).toBeNull(); + expect(text).not.toContain(database.url); + expect(text.toLowerCase()).not.toContain("stack"); + }); + + it("rejects a cookie-bearing state-changing request from an untrusted browser origin, and does not invalidate the session", async () => { + const { cookiePair } = await signIn(origin, emailOne, password); + expect(cookiePair).toBeDefined(); + + const response = await fetch(`${origin}/api/auth/sign-out`, { + body: "{}", + headers: { + "content-type": "application/json", + cookie: cookiePair as string, + origin: untrustedOrigin, + }, + method: "POST", + }); + const text = await response.text(); + + expect(response.status).toBe(403); + expect(response.headers.get("access-control-allow-origin")).toBeNull(); + expect(text).not.toContain(database.url); + expect(text.toLowerCase()).not.toContain("stack"); + + // The forged/rejected sign-out must never have reached sign-out logic: the session presented + // alongside it must still be valid. + const { body } = await fetchPrincipal(origin, { cookie: cookiePair as string }); + expect(body).toMatchObject({ kind: "authenticated" }); + }); + + it("rejects a 7-character password as too short", async () => { + const { response } = await signUp(origin, "pw-short@example.test", "1234567"); + expect(response.status).toBe(400); + }); + + it("accepts an 8-character password", async () => { + const { response, body } = await signUp(origin, "pw-min@example.test", "12345678"); + expect(response.status).toBe(200); + expect(body.user.email).toBe("pw-min@example.test"); + }); + + it("accepts a 128-character password", async () => { + const { response } = await signUp(origin, "pw-max@example.test", "a".repeat(128)); + expect(response.status).toBe(200); + }); + + it("rejects a 129-character password as too long", async () => { + const { response } = await signUp(origin, "pw-over@example.test", "a".repeat(129)); + expect(response.status).toBe(400); + }); + it("leaves Better Auth's handler unavailable outside /api/auth/*", async () => { const response = await fetch(`${origin}/sign-in/email`, { method: "POST" }); expect(response.status).toBe(404); @@ -533,22 +660,6 @@ describe.sequential("live API — signup, sign-in, principal, sign-out", () => { }); }); -// Better Auth's own server-side rejection of a cookie-bearing state-changing request from an -// untrusted origin (403 INVALID_ORIGIN) was verified directly and repeatedly against this exact -// pinned release and this exact compiled `apps/api` server — signup, sign-in, then a sign-out -// attempt with the valid session cookie and an untrusted Origin header reliably returns 403, and -// the presented session remains valid afterward — but only when driven by a plain Node script -// against the running server. The identical scenario could not be made to reproduce reliably when -// driven through Vitest specifically (the rejection silently does not fire), even in a single -// isolated test with its own fresh database and API instance, and even with `NODE_ENV` forced to -// `"production"` inside the test process. The cause was not pinned down to a documented, -// reproducible contract inside the time available for this slice, so no automated assertion is -// made for this specific request shape here rather than commit a flaky or misleading test. What -// *is* covered by an automated test above is the layer this repository's own code owns: CORS -// never returns `Access-Control-Allow-Origin` for an untrusted origin, which is the actual -// browser-enforced boundary preventing a malicious page from reading a response. See ADR 0011 for -// the full account, including the exact manual reproduction used. - describe.sequential("live API — startup and migration guarantees", () => { it("does not create any tables or migration history when started against an unmigrated database", async () => { const database = await createDisposableDatabase("unmigrated"); @@ -629,3 +740,70 @@ describe.sequential("live API — principal infrastructure failure is never down await running.shutdown().catch(() => undefined); }); }); + +describe.sequential("live API — Better Auth logging never leaks operational detail", () => { + it("emits no raw exception, SQL, connection detail, origin header, or stack trace when a real identity database failure occurs", async () => { + const database = await createDisposableDatabase("logging_leak"); + await applyCoreMigrations(database.url); + const running = await startMigratedApi(database.url); + const origin = apiOrigin(running); + + await signUp(origin, "frank@example.test", "correct horse battery staple"); + + // Force the database itself unreachable out from under the running API pool, then drive a + // request through Better Auth's own database-backed handler (not Ekklesia's own principal + // route) so Better Auth's internal logging — configured in `core/src/identity/runtime.ts` — + // is the thing actually being exercised here, not Core's own error handling. Without + // `onAPIError: { throw: true }`, the pinned release's own `better-call` transport dependency + // would catch this internally and print it raw via an unconditional + // `console.error("# SERVER_ERROR: ", error)` that no `logger` configuration can reach; with + // it, the failure instead propagates out of `auth.handler()` and becomes a sanitized + // `IdentityRuntimeError` at `handleAuthRequest`'s own boundary, and nothing is logged at all. + await dropDatabase(database.name); + + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + try { + const response = await fetch(`${origin}/api/auth/sign-in/email`, { + body: JSON.stringify({ + email: "frank@example.test", + password: "correct horse battery staple", + }), + headers: { "content-type": "application/json", origin: dashboardOrigin }, + method: "POST", + }); + const body = await response.clone().json(); + + expect(response.status).not.toBe(200); + expect(body).toEqual({ code: "AUTH_REQUEST_FAILED" }); + + const emitted = [...errorSpy.mock.calls, ...warnSpy.mock.calls, ...logSpy.mock.calls] + .flat() + .map((value) => + value instanceof Error ? `${value.message}\n${value.stack ?? ""}` : String(value), + ) + .join("\n"); + + // The primary guarantee: onAPIError.throw routes this exact failure out to Core's own + // sanitized boundary before Better Auth (or its own transport dependency) ever gets a + // chance to log anything about it, so nothing is emitted at all. + expect(emitted).toBe(""); + // Defense in depth, kept even though the assertion above is currently sufficient: if a + // future change causes something to be logged after all, it must still never contain any + // of this. + expect(emitted).not.toContain(database.url); + expect(emitted).not.toContain(database.name); + expect(emitted.toLowerCase()).not.toContain("password"); + expect(emitted.toLowerCase()).not.toContain("stack"); + expect(emitted).not.toMatch(/select|insert|update|delete|drizzle/iu); + expect(emitted).not.toContain(dashboardOrigin); + expect(emitted).not.toContain(identityBaseUrl); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + logSpy.mockRestore(); + await running.shutdown().catch(() => undefined); + } + }); +}); diff --git a/apps/api/tests/lifecycle.test.ts b/apps/api/tests/lifecycle.test.ts index 8a51c40..e52d31d 100644 --- a/apps/api/tests/lifecycle.test.ts +++ b/apps/api/tests/lifecycle.test.ts @@ -11,6 +11,84 @@ vi.mock("better-auth", () => ({ betterAuth: betterAuthMock })); vi.mock("better-auth/adapters/drizzle", () => ({ drizzleAdapter: drizzleAdapterMock })); vi.mock("@hono/node-server", () => ({ serve: serveMock })); +import { createApiRuntimeConfig } from "../src/config.js"; +import { installShutdownSignalHandlers, startApi } from "../src/server.js"; + +function validConfig() { + return createApiRuntimeConfig( + { + databaseUrl: "postgresql://ekklesia:ekklesia@127.0.0.1:5432/ekklesia", + host: "127.0.0.1", + identity: { + baseUrl: "https://identity.example.test", + secret: "correct-horse-battery-staple-0123456789", + trustedOrigins: ["https://identity.example.test"], + }, + port: 0, + }, + { environment: "production" }, + ); +} + +/** A minimal fake `ServerType`: enough of Node's `http.Server` shape for `bindServer`/`shutdown` + * to drive without a real socket. `close` and `once` are spies so tests can assert call counts; + * `triggerError` simulates the server's own `'error'` event (e.g. a bind failure). */ +function fakeServer() { + let errorHandler: ((error: Error) => void) | undefined; + const server = { + address: () => ({ address: "127.0.0.1", family: "IPv4", port: 4_000 }), + close: vi.fn((callback?: () => void) => { + callback?.(); + return server; + }), + listening: true, + once: vi.fn((event: string, handler: (error: Error) => void) => { + if (event === "error") { + errorHandler = handler; + } + return server; + }), + }; + return { + server, + triggerError: (error: Error) => errorHandler?.(error), + }; +} + +/** Resolves the listening callback asynchronously, matching real `@hono/node-server` behavior + * (the callback fires on the `listening` event, after `serve()` itself has already returned) — + * resolving synchronously would reference `bindServer`'s own `server` const before it is + * initialized. */ +function mockSuccessfulServe() { + const { server, triggerError } = fakeServer(); + serveMock.mockImplementation((_options: unknown, listeningListener?: () => void) => { + queueMicrotask(() => listeningListener?.()); + return server; + }); + return { server, triggerError }; +} + +function mockFailingServe(error: Error) { + const { server, triggerError } = fakeServer(); + serveMock.mockImplementation(() => { + queueMicrotask(() => triggerError(error)); + return server; + }); + return server; +} + +/** Captures signal handlers registered via `process.on` without ever installing a real one, so a + * test can trigger "SIGINT"/"SIGTERM" by calling the captured function directly. */ +function mockProcessOn(): { handlers: Record void>; restore: () => void } { + const handlers: Record void> = {}; + const fake = vi.fn((event: string, handler: () => void) => { + handlers[event] = handler; + return process; + }); + const onSpy = vi.spyOn(process, "on").mockImplementation(fake as unknown as typeof process.on); + return { handlers, restore: () => onSpy.mockRestore() }; +} + describe("Ekklesia API import safety", () => { const originalEnv = { ...process.env }; const originalProcessOn = process.on.bind(process); @@ -73,3 +151,115 @@ describe("Ekklesia API import safety", () => { expect(serveMock).not.toHaveBeenCalled(); }); }); + +describe("Ekklesia API shutdown lifecycle", () => { + beforeEach(() => { + poolConstructor.mockReset(); + betterAuthMock.mockReset(); + drizzleAdapterMock.mockClear(); + serveMock.mockReset(); + }); + + it("makes every concurrent shutdown caller await the exact same server-close and pool-close, each exactly once", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + const { server } = mockSuccessfulServe(); + + const running = await startApi(validConfig()); + + await Promise.all([running.shutdown(), running.shutdown(), running.shutdown()]); + + expect(server.close).toHaveBeenCalledTimes(1); + expect(poolEnd).toHaveBeenCalledTimes(1); + }); + + it("keeps shutdown idempotent for calls made after the first has already resolved", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + const { server } = mockSuccessfulServe(); + + const running = await startApi(validConfig()); + await running.shutdown(); + await running.shutdown(); + + expect(server.close).toHaveBeenCalledTimes(1); + expect(poolEnd).toHaveBeenCalledTimes(1); + }); + + it("closes an already-created database pool when identity construction fails during startup", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockImplementation(() => { + throw new Error("postgresql://admin:secret@db.internal.example/x"); + }); + + await expect(startApi(validConfig())).rejects.toMatchObject({ + code: "IDENTITY_CONSTRUCTION_FAILED", + }); + + expect(poolEnd).toHaveBeenCalledTimes(1); + expect(serveMock).not.toHaveBeenCalled(); + }); + + it("closes an already-created database pool when the server fails to bind its listen address", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + mockFailingServe(new Error("EADDRINUSE")); + + await expect(startApi(validConfig())).rejects.toThrow(); + + expect(poolEnd).toHaveBeenCalledTimes(1); + }); + + it("wires SIGINT/SIGTERM to the same shutdown lifecycle and only calls the injected exit, never the real process.exit", async () => { + const { handlers, restore } = mockProcessOn(); + const shutdown = vi.fn().mockResolvedValue(undefined); + const exit = vi.fn(); + + installShutdownSignalHandlers({ shutdown }, exit); + + expect(handlers.SIGINT).toEqual(expect.any(Function)); + expect(handlers.SIGTERM).toEqual(expect.any(Function)); + + handlers.SIGINT?.(); + await Promise.resolve(); + await Promise.resolve(); + + expect(shutdown).toHaveBeenCalledTimes(1); + expect(exit).toHaveBeenCalledWith(0); + + // A second signal after the first must not trigger a second shutdown. + handlers.SIGTERM?.(); + await Promise.resolve(); + expect(shutdown).toHaveBeenCalledTimes(1); + expect(exit).toHaveBeenCalledTimes(1); + + restore(); + }); + + it("exits with a failure code, without throwing, when signal-triggered shutdown itself fails", async () => { + const { handlers, restore } = mockProcessOn(); + const shutdown = vi.fn().mockRejectedValue(new Error("pool close failed")); + const exit = vi.fn(); + + installShutdownSignalHandlers({ shutdown }, exit); + handlers.SIGTERM?.(); + await Promise.resolve(); + await Promise.resolve(); + + expect(exit).toHaveBeenCalledWith(1); + + restore(); + }); +}); diff --git a/core/src/identity/database.ts b/core/src/identity/database.ts index f3c2e4f..5f9b110 100644 --- a/core/src/identity/database.ts +++ b/core/src/identity/database.ts @@ -6,6 +6,8 @@ export type IdentityDrizzleDatabase = NodePgDatabase maximumPostgresTimeoutMs + ) { + throw new IdentityDatabaseError("IDENTITY_DATABASE_CONFIGURATION_INVALID"); + } + return value; +} + /** * Explicitly constructs the Core-owned Drizzle database instance used by the identity factory. * Building the `pg.Pool` performs no network I/O until a query is issued, and this function is @@ -54,10 +79,20 @@ export function createIdentityDrizzleDatabase( options: IdentityDatabaseOptions = {}, ): IdentityDatabaseHandle { const validatedConnectionString = validateConnectionString(connectionString); - const poolConfig: PoolConfig = { connectionString: validatedConnectionString }; - if (options.connectionTimeoutMs !== undefined) { - poolConfig.connectionTimeoutMillis = options.connectionTimeoutMs; - } + const validatedConnectionTimeoutMs = validateTimeout(options.connectionTimeoutMs); + const validatedOperationTimeoutMs = validateTimeout(options.operationTimeoutMs); + const poolConfig: PoolConfig = { + connectionString: validatedConnectionString, + ...(validatedConnectionTimeoutMs === undefined + ? {} + : { connectionTimeoutMillis: validatedConnectionTimeoutMs }), + ...(validatedOperationTimeoutMs === undefined + ? {} + : { + query_timeout: validatedOperationTimeoutMs, + statement_timeout: validatedOperationTimeoutMs, + }), + }; const pool = new Pool(poolConfig); // pg's own documentation requires an error listener on the pool: an idle client can be diff --git a/core/src/identity/runtime.ts b/core/src/identity/runtime.ts index 919758a..723b2df 100644 --- a/core/src/identity/runtime.ts +++ b/core/src/identity/runtime.ts @@ -23,6 +23,30 @@ const runtimeErrorMessages: Readonly> = const identityDatabaseProbeTimeoutMs = 3_000; +/** + * A fixed, generic replacement for whatever Better Auth actually logged. Better Auth's own log + * call sites are not a stable contract Core can audit line by line, and several of them pass a + * raw internal exception or interpolate user-controlled input directly into the message (for + * example the origin-check middleware logs `Invalid origin: ${originHeader}`). Rather than trying + * to sanitize Better Auth's message/argument payload case by case, the payload is dropped + * entirely and only this fixed string is ever written. + */ +const sanitizedBetterAuthLogMessage = "[identity] Better Auth reported an error."; + +/** + * Explicitly replaces Better Auth's default logger so its own message text and arguments — which + * may include a raw exception, SQL, a connection string, a host/port, a filesystem path, a + * session token, a password, or a user-controlled origin/referer header — are never written + * anywhere. Only the fixed, generic message above is ever emitted, and only for error-level + * events; this is deliberately not just "quiet under NODE_ENV=test": the same replacement applies + * in every environment, so production logging is exactly as sanitized as test logging. + */ +function sanitizedBetterAuthLogger(level: string): void { + if (level === "error") { + console.error(sanitizedBetterAuthLogMessage); + } +} + /** Never carries the underlying Better Auth error, database error, cause, stack, secret, or URL. */ export class IdentityRuntimeError extends Error { readonly code: IdentityRuntimeErrorCode; @@ -113,6 +137,19 @@ export function createIdentityRuntime( const auth = constructBetterAuth(() => betterAuth({ + advanced: { + // Better Auth's own default is environment-sensitive: `skipOriginCheck` falls back to + // `true` whenever `isTest()` is true (NODE_ENV=test, which Vitest sets, or TEST=1) unless + // `disableOriginCheck` is explicitly set — and origin skip in turn also silently skips + // CSRF validation as long as `disableCSRFCheck` is left unset (Better Auth's documented + // backward-compatibility coupling). Left implicit, this means origin/CSRF protection is + // silently *off* under the test runner and *on* in production: the exact same code path + // behaves differently in the two places it matters most to verify. Setting both flags to + // `false` explicitly removes the `isTest()` branch entirely, so the identical protection + // is enforced in every environment, including under Vitest. + disableCSRFCheck: false, + disableOriginCheck: false, + }, baseURL: options.config.baseUrl, database: drizzleAdapter(options.db, { provider: "pg", @@ -122,6 +159,29 @@ export function createIdentityRuntime( autoSignIn: false, disableSignUp: false, enabled: true, + // Pinned explicitly rather than left on Better Auth's fallback default (`|| 8` / `|| 128` + // in the pinned release): an implicit default can change silently on a future Better Auth + // upgrade, and Ekklesia's own tests assert this exact 8–128 boundary. + maxPasswordLength: 128, + minPasswordLength: 8, + }, + logger: { + disabled: false, + level: "error", + log: (level) => sanitizedBetterAuthLogger(level), + }, + // A second, independent layer alongside `logger` above: the pinned release's own transport + // dependency (`better-call`'s router) has an unconditional `console.error("# SERVER_ERROR: + // ", error)` fallback for any thrown value it does not recognize as one of Better Auth's own + // `APIError`s — this fires *before* Better Auth's `logger` option is ever consulted, so no + // logger configuration can reach it. `onAPIError.throw: true` closes that gap: Better Auth's + // own `APIError`s (wrong password, invalid origin, disabled sign-up, ...) still resolve to + // their normal HTTP response completely unaffected, but a genuinely unexpected failure (for + // example a database outage) now propagates out of `auth.handler()` instead of being caught + // and printed raw internally, so it reaches `handleAuthRequest`'s own try/catch below and + // becomes a sanitized `IdentityRuntimeError` like any other unexpected failure. + onAPIError: { + throw: true, }, rateLimit: { enabled: true, diff --git a/core/tests/identity-database.test.ts b/core/tests/identity-database.test.ts index 4b50432..5e07508 100644 --- a/core/tests/identity-database.test.ts +++ b/core/tests/identity-database.test.ts @@ -65,6 +65,65 @@ describe("Core identity Drizzle database composition", () => { ); }); + it("passes the operation timeout through to the pool configuration as both query_timeout and statement_timeout", () => { + poolConstructor.mockImplementation(function fakePool() { + return { end: async () => undefined, on: vi.fn() }; + }); + poolConstructor.mockClear(); + + createIdentityDrizzleDatabase("postgresql://example.test/ekklesia", { + operationTimeoutMs: 7_500, + }); + + expect(poolConstructor).toHaveBeenCalledWith( + expect.objectContaining({ query_timeout: 7_500, statement_timeout: 7_500 }), + ); + }); + + it("does not set query_timeout, statement_timeout, or connectionTimeoutMillis when no timeout is configured", () => { + poolConstructor.mockImplementation(function fakePool() { + return { end: async () => undefined, on: vi.fn() }; + }); + poolConstructor.mockClear(); + + createIdentityDrizzleDatabase("postgresql://example.test/ekklesia"); + + const [poolConfig] = poolConstructor.mock.calls[0] as [Record]; + expect(poolConfig).not.toHaveProperty("query_timeout"); + expect(poolConfig).not.toHaveProperty("statement_timeout"); + expect(poolConfig).not.toHaveProperty("connectionTimeoutMillis"); + }); + + it.each([ + ["operationTimeoutMs", 0], + ["operationTimeoutMs", -1], + ["operationTimeoutMs", Number.NaN], + ["operationTimeoutMs", 1.5], + ["operationTimeoutMs", 2_147_483_648], + ["connectionTimeoutMs", 0], + ["connectionTimeoutMs", -1], + ["connectionTimeoutMs", Number.NaN], + ] as const)("rejects an invalid %s (%s) without constructing a pool and without leaking the value", (optionName, value) => { + poolConstructor.mockClear(); + + let captured: unknown; + try { + createIdentityDrizzleDatabase("postgresql://example.test/ekklesia", { + [optionName]: value, + }); + } catch (error) { + captured = error; + } + + expect(captured).toBeInstanceOf(IdentityDatabaseError); + expect(captured).toMatchObject({ code: "IDENTITY_DATABASE_CONFIGURATION_INVALID" }); + const publicText = `${String(captured)}\n${ + captured instanceof Error ? (captured.stack ?? "") : "" + }`; + expect(publicText).not.toContain(String(value)); + expect(poolConstructor).not.toHaveBeenCalled(); + }); + it("registers a pool-level error listener so an idle client failure never becomes an uncaught exception", () => { const on = vi.fn(); poolConstructor.mockImplementation(function fakePool() { diff --git a/core/tests/identity-runtime.test.ts b/core/tests/identity-runtime.test.ts index 8f2caed..84c8eb0 100644 --- a/core/tests/identity-runtime.test.ts +++ b/core/tests/identity-runtime.test.ts @@ -75,9 +75,12 @@ describe("Core identity runtime construction boundary", () => { expect(betterAuthMock).toHaveBeenCalledTimes(1); const [options] = betterAuthMock.mock.calls[0] as [Record]; expect(Object.keys(options).sort()).toEqual([ + "advanced", "baseURL", "database", "emailAndPassword", + "logger", + "onAPIError", "rateLimit", "secret", "trustedOrigins", @@ -86,6 +89,8 @@ describe("Core identity runtime construction boundary", () => { autoSignIn: false, disableSignUp: false, enabled: true, + maxPasswordLength: 128, + minPasswordLength: 8, }); expect(options.rateLimit).toEqual({ enabled: true, max: 100, window: 10 }); expect(options.baseURL).toBe(validConfig().baseUrl); @@ -93,6 +98,96 @@ describe("Core identity runtime construction boundary", () => { expect(options.trustedOrigins).toEqual([...validConfig().trustedOrigins]); }); + it("explicitly disables Better Auth's default test-environment origin/CSRF skip", () => { + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + + createIdentityRuntime({ config: validConfig(), db: fakeDb() }); + + const [options] = betterAuthMock.mock.calls[0] as [Record]; + expect(options.advanced).toEqual({ disableCSRFCheck: false, disableOriginCheck: false }); + }); + + it("configures onAPIError.throw so an unexpected failure escapes Better Auth's own unsanitized fallback logger instead of being swallowed internally", () => { + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + + createIdentityRuntime({ config: validConfig(), db: fakeDb() }); + + const [options] = betterAuthMock.mock.calls[0] as [Record]; + expect(options.onAPIError).toEqual({ throw: true }); + }); + + it("configures an explicit, error-only Better Auth logger that never forwards Better Auth's own message or arguments", () => { + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + + createIdentityRuntime({ config: validConfig(), db: fakeDb() }); + + const [options] = betterAuthMock.mock.calls[0] as [Record]; + const loggerOption = options.logger as { + disabled?: boolean; + level?: string; + log?: (...args: unknown[]) => void; + }; + expect(loggerOption.disabled).toBe(false); + expect(loggerOption.level).toBe("error"); + expect(typeof loggerOption.log).toBe("function"); + + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + try { + const hostileException = new Error( + "password=hunter2 host=db.internal.example port=5432 statement_timeout at /var/app/src/secret.ts:10:5", + ); + loggerOption.log?.( + "error", + "Invalid origin: https://evil.example.test", + hostileException, + "select * from ekklesia_core_session", + ); + + expect(errorSpy).toHaveBeenCalledTimes(1); + const emitted = errorSpy.mock.calls + .flat() + .map((value) => + value instanceof Error ? `${value.message}\n${value.stack ?? ""}` : String(value), + ) + .join("\n"); + expect(emitted).not.toContain("hunter2"); + expect(emitted).not.toContain("db.internal.example"); + expect(emitted).not.toContain("5432"); + expect(emitted).not.toContain("/var/app"); + expect(emitted).not.toContain("evil.example.test"); + expect(emitted.toLowerCase()).not.toContain("select"); + expect(emitted.toLowerCase()).not.toContain("stack"); + } finally { + errorSpy.mockRestore(); + } + }); + + it("never publishes a Better Auth log at or below warn level", () => { + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + + createIdentityRuntime({ config: validConfig(), db: fakeDb() }); + + const [options] = betterAuthMock.mock.calls[0] as [Record]; + const loggerOption = options.logger as { log?: (...args: unknown[]) => void }; + + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + try { + loggerOption.log?.("warn", "a warning", "detail"); + loggerOption.log?.("info", "some info", "detail"); + loggerOption.log?.("debug", "some debug", "detail"); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(warnSpy).not.toHaveBeenCalled(); + expect(logSpy).not.toHaveBeenCalled(); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + logSpy.mockRestore(); + } + }); + it("only widens the sign-in/sign-up rate-limit window when a narrow override is explicitly injected", () => { betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); diff --git a/docs/architecture/decisions/0011-identity-http-transport-and-public-enrollment.md b/docs/architecture/decisions/0011-identity-http-transport-and-public-enrollment.md index fb83cf5..9e31117 100644 --- a/docs/architecture/decisions/0011-identity-http-transport-and-public-enrollment.md +++ b/docs/architecture/decisions/0011-identity-http-transport-and-public-enrollment.md @@ -223,6 +223,16 @@ for an unrecognized origin does not prevent the server from processing the reque Auth's own origin/CSRF checks (below) independently protect state-changing calls regardless of CORS. +> **Correction — see [ADR 0012](0012-identity-origin-csrf-protection-under-test.md).** The +> paragraphs below describe what was verified and, honestly, not yet understood at the time this +> ADR was accepted. ADR 0012 found the root cause of the Vitest non-reproduction described here +> (the pinned Better Auth release silently skips its own origin/CSRF checks whenever `isTest()` is +> true unless `advanced.disableOriginCheck` is explicitly set — Vitest's default `NODE_ENV=test` +> triggers exactly that), fixed it by setting `advanced.disableCSRFCheck`/`disableOriginCheck` +> explicitly to `false`, and added the automated coverage this ADR states does not exist. This +> section is left as originally accepted, as the historical record; ADR 0012 is now authoritative +> for this behavior. + Better Auth's own CSRF/origin protections are untouched: `advanced.disableCSRFCheck` and `advanced.disableOriginCheck` are never set. Verified directly and repeatedly against the pinned release, through the real, compiled `apps/api` HTTP server (not just a direct `auth.handler()` diff --git a/docs/architecture/decisions/0012-identity-origin-csrf-protection-under-test.md b/docs/architecture/decisions/0012-identity-origin-csrf-protection-under-test.md new file mode 100644 index 0000000..805fb4a --- /dev/null +++ b/docs/architecture/decisions/0012-identity-origin-csrf-protection-under-test.md @@ -0,0 +1,132 @@ +# 0012: Production-equivalent identity origin/CSRF protection under test + +- Status: Accepted +- Date: 2026-07-21 +- Amends: [0011](0011-identity-http-transport-and-public-enrollment.md) + +## Context + +ADR 0011 stated that Better Auth's own server-side origin/CSRF rejection was real, verified +production behavior, but could not be reproduced reliably under Vitest, and left the discrepancy +undiagnosed rather than commit a flaky or misleading automated test. A follow-up corrective slice +(`phase-4/identity-http-hardening`) revisited that gap, alongside four related hardening items +found during the same review: Better Auth's default logger could emit raw exceptions and +user-controlled input; the identity database pool had no bound on a hung query; `apps/api`'s +shutdown had a narrow idempotency gap under concurrent callers; and the email/password length +boundary was left on an implicit Better Auth default rather than an explicit one. This ADR covers +only the origin/CSRF root cause and fix, since it is the one item that changes a previously +documented architectural guarantee; the other four are implementation hardening, documented in +[Identity](../../development/identity.md) and [API](../../development/api.md) rather than a new +ADR. + +## Decision + +### Root cause: Better Auth silently disables origin/CSRF checking under a test runner by default + +Reading the pinned `better-auth@1.6.23` release's own `create-context.mjs` directly: + +```js +skipOriginCheck: options.advanced?.disableOriginCheck !== void 0 + ? options.advanced.disableOriginCheck + : isTest() ? true : false, +``` + +`isTest()` (from `@better-auth/core/env`) is `nodeENV === "test" || toBoolean(env.TEST)`. Vitest +sets `NODE_ENV=test` by default. So whenever `advanced.disableOriginCheck` is left unset — exactly +Ekklesia's configuration before this ADR — Better Auth silently sets `skipOriginCheck: true` under +Vitest and `false` in production: the identical code path behaves differently in the one place most +important to verify it and the one place it actually matters. Better Auth's own +`shouldSkipCSRFForBackwardCompat` then also skips CSRF validation whenever `skipOriginCheck` is +`true` and `advanced.disableCSRFCheck` is left unset, coupling the two. This is Better Auth's own +documented default, read from `isTest()` at Better Auth construction time — not a Vitest bug, a +race condition, or anything specific to this repository's test harness. It explains ADR 0011's +mystery well enough to act on: whatever precisely made that ADR's own `NODE_ENV`-forcing attempt +unreliable (`isTest()` also honors a separate `TEST` environment variable, which a `NODE_ENV` +override alone would not clear, and which Vitest or its surrounding tooling may set independently), +an explicit `advanced.disableOriginCheck`/`disableCSRFCheck` value removes the `isTest()` branch +entirely and cannot be affected by any test-runner environment variable. + +### Fix: pin both flags explicitly + +`core/src/identity/runtime.ts` now passes: + +```ts +advanced: { + disableCSRFCheck: false, + disableOriginCheck: false, +}, +``` + +This removes the `isTest()` branch entirely — the flags are always explicit, so identical +protection is enforced in every environment, including under Vitest. No other origin/CSRF-related +option changes: `trustedOrigins` is still Phase 4A's single `IdentityConfig.trustedOrigins` list, +cookies are still Better Auth's host-only defaults, and CORS is still the exact-match, no-wildcard +policy ADR 0011 already established. + +### Automated coverage replaces the manual-only caveat + +`apps/api/tests/api-http.integration.ts` now asserts, against the real compiled server and a real +PostgreSQL-backed session, all four scenarios ADR 0011 could previously only claim from manual +verification: + +- A no-cookie first authentication attempt from a trusted browser origin succeeds. +- A no-cookie first authentication attempt from an untrusted browser origin is rejected (`403`), + with no `Access-Control-Allow-Origin` header and no leaked internal detail in the body. +- A cookie-bearing state-changing request (sign-out) from an untrusted browser origin is rejected + the same way, and — critically — the presented session remains valid afterward, proving the + rejected request never reached sign-out logic. +- A denied origin never receives a permissive CORS header, in either of the above cases. + +The manual-reproduction paragraphs in ADR 0011 are left intact as the historical record of what was +verified and not yet understood at the time; a short correction note there points here. + +### A narrower consequence: Fetch-API clients now need a trusted origin on sign-in/sign-up even without a cookie + +Enabling the checks surfaced a real, previously-untested distinction. Better Auth's first-login +CSRF protection (`formCsrfMiddleware`, applied only to `/sign-in/email` and `/sign-up/email`) does +not key off cookies alone: it also treats the presence of any `Sec-Fetch-*` header as evidence the +caller is a Fetch-API client (browser or Node's own `fetch()`, which sends `Sec-Fetch-Mode: cors` +automatically) and requires a trusted `Origin`/`Referer` in that case regardless of cookie +presence. A previously-committed test claiming a *server-to-server* sign-up with no `Origin` header +succeeds was actually exercising Node's `fetch()`, and once the check was genuinely enabled it +correctly failed. The test now uses a raw `node:http` request (which sends no `Sec-Fetch-*` +headers, matching curl and most backend HTTP client libraries) to prove the real claim: a *non-Fetch* +server-to-server caller needs no `Origin` header; a Fetch-API caller (browser or Node's `fetch()`) +does, on these two specific routes, cookie or not. See [API](../../development/api.md#origin-cors-and-cookies). + +### A related discovery: Better Auth's own transport dependency logs raw errors outside any logger option + +Verifying the logging hardening item from the same corrective slice against a real induced identity +database failure surfaced a second, independent gap: `better-call` (the low-level HTTP dispatch +library Better Auth is built on) has an unconditional `console.error("# SERVER_ERROR: ", error)` +fallback for any error it does not recognize as a Better Auth `APIError`, which fires before Better +Auth's own `logger` option is ever consulted. `core/src/identity/runtime.ts` now also sets +`onAPIError: { throw: true }`, which lets Better Auth's own deliberate `APIError`s (wrong password, +invalid origin, ...) resolve normally while routing genuinely unexpected failures out to Core's +already-sanitized `handleAuthRequest` boundary instead. This is a logging fix, not an origin/CSRF +one; the full account is in [API](../../development/api.md#logging). + +## Alternatives + +- Setting `NODE_ENV=production` inside the test process (as ADR 0011's own investigation already + tried) was not a viable fix even before the root cause was known: it is fragile (couples + unrelated behavior to a global environment variable inside the test runner) and, as this ADR's + investigation showed, does not actually address the mechanism — Better Auth reads the same + `isTest()` signal regardless, and pinning `NODE_ENV` inside the same process it already runs in + is not a meaningfully different configuration from leaving `advanced.disableOriginCheck` unset. +- Leaving the gap as a documented manual-only guarantee (ADR 0011's original position) was rejected + once the root cause was known and fixable: an explicit, narrow configuration change closes it + completely, so there is no remaining reason to accept unverified production behavior. +- Disabling Better Auth's built-in origin/CSRF checks and reimplementing origin validation directly + in `apps/api`'s CORS layer was rejected: it would duplicate a check Better Auth already performs + correctly once configured explicitly, and would weaken the cookie-bearing-request guarantee CORS + alone cannot provide (CORS is a browser-enforced, response-header mechanism; it does not stop a + request from being processed server-side). + +## Consequences + +Better Auth's origin/CSRF protection is now genuinely production-equivalent under the automated +test suite, closing the gap ADR 0011 left open. Future changes to authentication endpoints can rely +on `pnpm test:migrations:integration` to actually exercise this guarantee rather than requiring a +separate manual verification pass. No route, response shape, cookie policy, or CORS behavior +changed for any caller already sending a trusted `Origin` header. diff --git a/docs/architecture/decisions/README.md b/docs/architecture/decisions/README.md index 8703757..e257026 100644 --- a/docs/architecture/decisions/README.md +++ b/docs/architecture/decisions/README.md @@ -20,3 +20,4 @@ ADRs capture durable choices that affect more than one package or phase. - [0009: Transactional module migration execution](0009-transactional-module-migrations.md) — Accepted - [0010: Core identity architecture and Better Auth adoption](0010-core-identity-and-better-auth-adoption.md) — Accepted - [0011: Identity HTTP transport and public enrollment policy](0011-identity-http-transport-and-public-enrollment.md) — Accepted +- [0012: Production-equivalent identity origin/CSRF protection under test](0012-identity-origin-csrf-protection-under-test.md) — Accepted diff --git a/docs/development/api.md b/docs/development/api.md index f71fc0c..9710b10 100644 --- a/docs/development/api.md +++ b/docs/development/api.md @@ -5,6 +5,10 @@ one Ekklesia-owned route to resolve the current principal. It does not add a Das Mobile Surface, and it does not add roles, permissions, or a platform-owner concept — see [ADR 0011](../architecture/decisions/0011-identity-http-transport-and-public-enrollment.md) for the full architectural decision and [Identity](identity.md) for the identity foundation this builds on. +A follow-up corrective slice hardened this transport (sanitized Better Auth logging, +production-equivalent origin/CSRF protection under the test runner, bounded identity database +timeouts, idempotent shutdown, and pinned password-length boundaries) without changing any of the +above; see [ADR 0012](../architecture/decisions/0012-identity-origin-csrf-protection-under-test.md). ## What this is, and is not @@ -109,8 +113,12 @@ is `null`. The caller must call `/api/auth/sign-in/email` explicitly afterward. per the pinned Better Auth release, `autoSignIn: false` activates its built-in email-enumeration protection, so a duplicate sign-up returns the same `200` shape (with a fresh, unrelated random id) whether or not the email was already registered, rather than revealing which. No custom -cryptography replaces Better Auth's own password hashing, and its default password-length policy -(8–128 characters) is kept unmodified. +cryptography replaces Better Auth's own password hashing. Password length is pinned explicitly to +8–128 characters (`minPasswordLength`/`maxPasswordLength` in `core/src/identity/runtime.ts`) rather +than left on Better Auth's fallback default — the pinned release's default happens to be the same +8–128 range, but an implicit default can change silently on a future Better Auth upgrade, while an +explicit value cannot. `apps/api/tests/api-http.integration.ts` proves the exact boundary against a +real server: 7 characters rejected, 8 accepted, 128 accepted, 129 rejected. ## Origin, CORS, and cookies @@ -121,19 +129,67 @@ since `Access-Control-Allow-Origin: *` cannot be combined with `Access-Control-A true`); a disallowed origin never receives the header at all, though the request itself may still be processed — CORS is a browser-enforced, response-header mechanism, not a server-side gate. A request with no `Origin` header at all (native or server-to-server clients) is unaffected by CORS. +Note this is narrower than it sounds for `/api/auth/sign-in/email` and `/api/auth/sign-up/email` +specifically: any client built on the Fetch API — every browser, and also Node's own `fetch()` — +automatically sends a `Sec-Fetch-Mode: cors` header on every request, and Better Auth's first-login +CSRF protection treats that as "this came from a Fetch client" and requires a trusted `Origin` (or +`Referer`) on those two routes regardless of whether a cookie is present. A genuinely non-Fetch +caller (curl, or most backend HTTP client libraries, which send none of the `Sec-Fetch-*` headers) +is unaffected and needs no `Origin` header at all — see the dedicated raw-socket test in +`apps/api/tests/api-http.integration.ts` for the exact distinction proven against a real server. -Better Auth's own CSRF/origin protections are never disabled -(`advanced.disableCSRFCheck`/`advanced.disableOriginCheck` are never set). Verified directly -against the running compiled server: any non-`GET`/`HEAD`/`OPTIONS` request carrying a session -cookie must also carry a trusted `Origin` (or `Referer`), or Better Auth itself rejects it with -`403`. See ADR 0011 for the exact verification method and an important caveat about reproducing -part of this guarantee inside the automated test suite. +Better Auth's own CSRF/origin protections are explicitly kept on: +`core/src/identity/runtime.ts` sets `advanced: { disableCSRFCheck: false, disableOriginCheck: +false }`. This is deliberately explicit rather than left unset — the pinned release's own default +is environment-sensitive (`skipOriginCheck` falls back to `true`, silently disabling both origin +and CSRF checking, whenever `isTest()` is true, which Vitest's default `NODE_ENV=test` triggers), +so an unset config would mean origin/CSRF protection is silently off under the test runner and on +in production. Setting both flags explicitly removes that environment-sensitive branch entirely, +so `pnpm test:migrations:integration` now automatically proves the exact production behavior: any +non-`GET`/`HEAD`/`OPTIONS` request carrying a session cookie must also carry a trusted `Origin` (or +`Referer`), or Better Auth rejects it with `403` (and the presented session remains valid +afterward); a no-cookie first authentication attempt from an untrusted origin is rejected the same +way; a trusted origin is accepted in both cases; and a denied origin never receives a permissive +CORS header. See [ADR 0012](../architecture/decisions/0012-identity-origin-csrf-protection-under-test.md) +for the root-cause investigation and the full account of what changed. Cookies keep Better Auth's defaults: `HttpOnly` always, `SameSite=Lax` always, `Secure` when the configured base URL is `https://`. Cross-subdomain cookies are not enabled and no cookie `Domain` attribute is set, so every cookie is host-only. Trusted proxy headers are not configured in this slice — a deployment behind a reverse proxy is future work. +## Logging + +Two independent, complementary layers, both configured explicitly in `core/src/identity/runtime.ts` +rather than left on any environment-sensitive default: + +1. **`logger`.** Better Auth's own default (`level: "warn"`, plain `console.*` output) is replaced. + The pinned release's log call sites are not a stable, fully-audited contract — several pass a + raw internal exception, and at least one (the origin-check middleware) interpolates a + user-controlled `Origin` header directly into the message text. Rather than sanitize each call + site's message and arguments individually, Core's logger configuration drops the message and + every argument entirely and emits only a fixed, generic string + (`"[identity] Better Auth reported an error."`) via `console.error`, and only for error-level + events — nothing is ever emitted for warn/info/debug. +2. **`onAPIError: { throw: true }`.** Discovered while testing (1) above: the pinned release's own + HTTP transport dependency, `better-call`'s router, has an *unconditional* + `console.error("# SERVER_ERROR: ", error)` fallback for any thrown value it does not recognize + as one of Better Auth's own `APIError`s — this fires before Better Auth's `logger` option is + ever consulted, so no `logger` configuration can reach it. `onAPIError.throw: true` closes that + gap: Better Auth's own deliberate `APIError`s (wrong password, invalid origin, disabled sign-up, + password out of bounds, ...) still resolve to their normal, correct HTTP response completely + unaffected, but a genuinely unexpected failure (for example a database outage mid-request) now + propagates out of `auth.handler()` instead of being caught and printed raw internally by + `better-call`, reaching `handleAuthRequest`'s own try/catch and becoming a sanitized + `IdentityRuntimeError` like any other unexpected failure — and nothing is logged at all. + +Both apply in every environment, including under Vitest: neither is a `NODE_ENV=test` special case. +A focused unit test (`core/tests/identity-runtime.test.ts`) simulates a hostile log call (a raw +exception carrying a password, a host, a port, a filesystem path, and a rejected origin) and asserts +none of it reaches `console.error`, and separately asserts `onAPIError` is configured; a live +integration test forces a real identity-database failure through Better Auth's own sign-in handler +and asserts nothing at all reaches `console.error`/`warn`/`log` in the real process. + ## Rate limiting Better Auth's built-in rate limiting is always enabled, including by the executable entry point — @@ -146,13 +202,30 @@ trusted-proxy IP policy is configured, Better Auth may fall back to a single sha rather than a genuine per-client bucket. This is not yet the final, distributed, production-grade abuse-control design. +## Database connection limits + +`apps/api/src/server.ts` configures the identity PostgreSQL pool (`createIdentityDrizzleDatabase`, +`core/src/identity/database.ts`) with both a connection timeout and an operation timeout (10 +seconds each), the same bounded, validated pattern `core/src/migrations/postgres-database.ts` +already uses. `connectionTimeoutMillis` alone only bounds *establishing* a connection; it does +nothing once a query is already running on one. The operation timeout additionally sets `pg`'s +`query_timeout` and `statement_timeout`, so a hung or unexpectedly slow query is aborted rather than +holding a pool connection (and its caller) indefinitely. An invalid timeout (non-integer, zero, +negative, or larger than PostgreSQL's own maximum) is rejected as configuration error before any +pool is constructed, sanitized the same way an invalid connection string already is. + ## Graceful shutdown `startApi()` (the composition function `apps/api/src/server.ts` exports, used by both the -executable entry point and tests) returns `{app, server, shutdown}`. `shutdown()` is idempotent: it -stops the HTTP listener from accepting new connections and lets in-flight requests finish, then -closes the PostgreSQL pool. The executable entry point wires `SIGINT`/`SIGTERM` to call it and -exits afterward; a startup failure after the database pool was already created (identity +executable entry point and tests) returns `{app, server, shutdown}`. `shutdown()` memoizes one +shared promise the first time it is called: every caller, concurrent or sequential, awaits the +exact same listener-drain-then-pool-close operation, and that operation runs exactly once no matter +how many times `shutdown()` is called. It stops the HTTP listener from accepting new connections and +lets in-flight requests finish, then closes the PostgreSQL pool. The executable entry point's +`SIGINT`/`SIGTERM` wiring is extracted into `installShutdownSignalHandlers(running, exit)` — the +same function production composition uses, but independently testable with a fake `running`/`exit` +so a test can trigger "a signal arrived" without sending a real process signal or invoking the real +`process.exit`. A startup failure after the database pool was already created (identity construction failure, or the server failing to bind its listen address) always closes that pool before the failure propagates. diff --git a/docs/development/identity.md b/docs/development/identity.md index 35b34e4..8b32990 100644 --- a/docs/development/identity.md +++ b/docs/development/identity.md @@ -149,6 +149,23 @@ exported from `@ekklesia/core/identity`, exposes `auth.api`, Better Auth's sessi Drizzle adapter, the raw database, a session token, or Better Auth configuration. `apps/api` is the only current consumer of this boundary; see [API](api.md). +Construction also explicitly configures four things the pinned release would otherwise leave on an +environment-sensitive or implicit default, all covered by [ADR 0012](../architecture/decisions/0012-identity-origin-csrf-protection-under-test.md) +and [API](api.md#logging): +`advanced.disableCSRFCheck`/`disableOriginCheck` are both pinned to `false` (the default silently +skips both under a test runner), Better Auth's `logger` is replaced with one that drops its message +and arguments and emits only a fixed, generic string for error-level events, `onAPIError: { throw: +true }` routes a genuinely unexpected failure out to `handleAuthRequest`'s own sanitized boundary +instead of letting Better Auth's `better-call` transport dependency log it raw through a path no +`logger` configuration can reach, and `emailAndPassword.minPasswordLength`/`maxPasswordLength` are +pinned to `8`/`128` instead of left on Better Auth's fallback default. + +`createIdentityDrizzleDatabase` (`core/src/identity/database.ts`) also accepts an optional +`operationTimeoutMs`, applied as both `pg`'s `query_timeout` and `statement_timeout` — bounding a +hung or slow query, which `connectionTimeoutMs` alone cannot do since it only bounds establishing +the connection. `apps/api` sets both timeouts explicitly (see [API](api.md)); an invalid value is +rejected the same sanitized way an invalid connection string already is. + ## What is not operational yet No email verification, password recovery, invitation flow, email delivery, social or OAuth diff --git a/docs/development/testing.md b/docs/development/testing.md index 712ace8..57a1743 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -14,15 +14,21 @@ module migration discovery, identity schema drift detection, formatting, linting Migration-runner unit tests use a narrow database/session port to prove deterministic planning, approval, transaction orchestration, cleanup, and sanitized errors. They do not claim to prove PostgreSQL behavior. Identity unit tests cover branded opaque-ID contracts, principal shapes, -configuration validation (secret, base URL, and trusted-origin rules), the Core identity runtime -(explicit construction, exact schema mapping, only email/password and rate limiting enabled, -sanitized failures, and that a database outage during principal resolution is never silently -reported as anonymous), and Core migration discovery (deterministic ordering, the reserved `core` -identity, and collision rejection) — all without a live database. `apps/api`'s own unit tests cover -its pure Hono app factory (route shapes, CORS allow/deny/preflight, deterministic 404 and sanitized -error responses), its runtime configuration, and that importing any of its modules — including the -executable server module without calling its exported `main` — never reads the environment, -connects to PostgreSQL, or starts a server. +configuration validation (secret, base URL, and trusted-origin rules), bounded/validated +connection and operation (`query_timeout`/`statement_timeout`) database timeouts, the Core identity +runtime (explicit construction, exact schema mapping, only email/password and rate limiting +enabled, an explicit `advanced.disableCSRFCheck`/`disableOriginCheck: false` pair, an explicit +error-only sanitized logger that strips Better Auth's own message and arguments regardless of what +they contain, pinned 8–128 password-length bounds, sanitized failures, and that a database outage +during principal resolution is never silently reported as anonymous), and Core migration discovery +(deterministic ordering, the reserved `core` identity, and collision rejection) — all without a +live database. `apps/api`'s own unit tests cover its pure Hono app factory (route shapes, CORS +allow/deny/preflight, deterministic 404 and sanitized error responses), its runtime configuration, +its idempotent shutdown lifecycle (concurrent callers share one server-close-then-pool-close +promise; a startup failure after the pool was already created still closes it; `SIGINT`/`SIGTERM` +wiring is exercised through an injectable exit function rather than the real `process.exit`), and +that importing any of its modules — including the executable server module without calling its +exported `main` — never reads the environment, connects to PostgreSQL, or starts a server. `pnpm verify` is intentionally Docker-free. It never plans or applies migrations against a live database. @@ -65,13 +71,26 @@ automatic session, per `autoSignIn: false`), explicit sign-in producing a host-o anonymous shapes, sign-out actually invalidating the session, a malformed or missing cookie resolving as anonymous, duplicate sign-up creating no second identity row, two distinct accounts producing identical principal shapes with no elevation and no owner/role/permission data at the -database level, no plaintext password ever stored, CORS never returning an allow-origin header for +database level, no plaintext password ever stored, the exact 8–128 password-length boundary (7 +rejected, 8 accepted, 128 accepted, 129 rejected), CORS never returning an allow-origin header for an untrusted origin, Better Auth's handler being unreachable outside `/api/auth/*`, a database outage during principal resolution producing a sanitized `503` rather than a false anonymous `200`, starting the API against an unmigrated database creating no tables at all, and a clean listener/pool -shutdown. See [ADR 0011](../architecture/decisions/0011-identity-http-transport-and-public-enrollment.md) -for one CSRF/origin guarantee that was verified manually against the real server but could not be -reproduced reliably inside this test runner, and is documented rather than silently asserted. +shutdown. + +The same suite also proves Better Auth's own origin/CSRF protection is production-equivalent under +the test runner, not just in a plain Node script: a no-cookie first authentication attempt succeeds +from a trusted browser origin and is rejected (`403`, no permissive CORS header, no leaked detail) +from an untrusted one; a cookie-bearing state-changing request (sign-out) is rejected the same way +from an untrusted origin, and the presented session remains valid afterward, proving the rejected +request never reached sign-out logic. See +[ADR 0012](../architecture/decisions/0012-identity-origin-csrf-protection-under-test.md) for why +this previously could not be reproduced reliably under Vitest (Better Auth's own default silently +skips these checks whenever `isTest()` is true, unless `advanced.disableOriginCheck` is set +explicitly) and how the fix removes that gap entirely. A further scenario forces a real identity +database failure and asserts, against actual `console.error`/`warn`/`log` output, that Better +Auth's own internal error logging — configured explicitly in `core/src/identity/runtime.ts` — never +emits a raw exception, SQL, connection detail, or the rejected request's origin. CI runs the same command in the dedicated `migration-smoke` job. After this change is merged, add the following pull-request status context to `main` branch protection: diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 51a14a1..e754f0e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -54,6 +54,9 @@ importers: '@types/pg': specifier: 8.20.0 version: 8.20.0 + better-auth: + specifier: 1.6.23 + version: 1.6.23(drizzle-kit@0.31.10)(drizzle-orm@0.45.2(@types/pg@8.20.0)(kysely@0.29.4)(pg@8.22.0))(pg@8.22.0)(vitest@4.1.10(@types/node@24.13.3)(vite@8.1.4(@types/node@24.13.3)(esbuild@0.28.1)(jiti@2.7.0)(tsx@4.23.1)(yaml@2.9.0))) pg: specifier: 8.22.0 version: 8.22.0