fix: harden identity HTTP transport #9
@@ -29,6 +29,7 @@
|
|||||||
"@ekklesia/typescript-config": "workspace:*",
|
"@ekklesia/typescript-config": "workspace:*",
|
||||||
"@types/node": "24.13.3",
|
"@types/node": "24.13.3",
|
||||||
"@types/pg": "8.20.0",
|
"@types/pg": "8.20.0",
|
||||||
|
"better-auth": "1.6.23",
|
||||||
"pg": "8.22.0",
|
"pg": "8.22.0",
|
||||||
"typescript": "7.0.2",
|
"typescript": "7.0.2",
|
||||||
"vitest": "4.1.10"
|
"vitest": "4.1.10"
|
||||||
|
|||||||
+44
-22
@@ -12,6 +12,7 @@ import { createApiApp } from "./app.js";
|
|||||||
import { type ApiRuntimeConfig, createApiRuntimeConfig } from "./config.js";
|
import { type ApiRuntimeConfig, createApiRuntimeConfig } from "./config.js";
|
||||||
|
|
||||||
const connectionTimeoutMs = 10_000;
|
const connectionTimeoutMs = 10_000;
|
||||||
|
const operationTimeoutMs = 10_000;
|
||||||
|
|
||||||
export interface RunningApi {
|
export interface RunningApi {
|
||||||
readonly app: Hono;
|
readonly app: Hono;
|
||||||
@@ -65,6 +66,7 @@ export async function startApi(
|
|||||||
): Promise<RunningApi> {
|
): Promise<RunningApi> {
|
||||||
const databaseHandle = createIdentityDrizzleDatabase(config.databaseUrl, {
|
const databaseHandle = createIdentityDrizzleDatabase(config.databaseUrl, {
|
||||||
connectionTimeoutMs,
|
connectionTimeoutMs,
|
||||||
|
operationTimeoutMs,
|
||||||
});
|
});
|
||||||
|
|
||||||
let runtime: ReturnType<typeof createIdentityRuntime>;
|
let runtime: ReturnType<typeof createIdentityRuntime>;
|
||||||
@@ -89,21 +91,53 @@ export async function startApi(
|
|||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
let shuttingDown = false;
|
// Memoizes one shared promise rather than a boolean flag: a boolean only prevents a *second*
|
||||||
const shutdown = async (): Promise<void> => {
|
// cleanup from starting, but a caller that arrives while the first cleanup is still in flight
|
||||||
if (shuttingDown) {
|
// would previously get an already-resolved `undefined` immediately, before the listener had
|
||||||
return;
|
// 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<void> | undefined;
|
||||||
|
const shutdown = (): Promise<void> => {
|
||||||
|
if (!shutdownPromise) {
|
||||||
|
shutdownPromise = (async () => {
|
||||||
|
await new Promise<void>((resolve) => {
|
||||||
|
server.close(() => resolve());
|
||||||
|
});
|
||||||
|
await safeCloseDatabase(databaseHandle);
|
||||||
|
})();
|
||||||
}
|
}
|
||||||
shuttingDown = true;
|
return shutdownPromise;
|
||||||
await new Promise<void>((resolve) => {
|
|
||||||
server.close(() => resolve());
|
|
||||||
});
|
|
||||||
await safeCloseDatabase(databaseHandle);
|
|
||||||
};
|
};
|
||||||
|
|
||||||
return Object.freeze({ app, server, shutdown });
|
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<RunningApi, "shutdown">,
|
||||||
|
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(): {
|
function readConfigFromEnvironment(): {
|
||||||
readonly input: Parameters<typeof createApiRuntimeConfig>[0];
|
readonly input: Parameters<typeof createApiRuntimeConfig>[0];
|
||||||
readonly environment: "development" | "production";
|
readonly environment: "development" | "production";
|
||||||
@@ -138,19 +172,7 @@ export async function main(): Promise<void> {
|
|||||||
const boundPort = address && typeof address === "object" ? address.port : config.port;
|
const boundPort = address && typeof address === "object" ? address.port : config.port;
|
||||||
console.log(`Ekklesia API listening on ${config.host}:${boundPort}`);
|
console.log(`Ekklesia API listening on ${config.host}:${boundPort}`);
|
||||||
|
|
||||||
let signalReceived = false;
|
installShutdownSignalHandlers(running);
|
||||||
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);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function isDirectExecution(): boolean {
|
function isDirectExecution(): boolean {
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import { randomBytes } from "node:crypto";
|
import { randomBytes } from "node:crypto";
|
||||||
|
import { request as httpRequest } from "node:http";
|
||||||
import { fileURLToPath } from "node:url";
|
import { fileURLToPath } from "node:url";
|
||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
import {
|
import {
|
||||||
@@ -9,7 +10,7 @@ import {
|
|||||||
import { mkdtemp, rm } from "node:fs/promises";
|
import { mkdtemp, rm } from "node:fs/promises";
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import { Client, type QueryResultRow } from "pg";
|
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 { createApiRuntimeConfig } from "../src/config.js";
|
||||||
import { startApi, type RunningApi } from "../src/server.js";
|
import { startApi, type RunningApi } from "../src/server.js";
|
||||||
|
|
||||||
@@ -245,7 +246,12 @@ async function signUp(
|
|||||||
): Promise<SignUpResult> {
|
): Promise<SignUpResult> {
|
||||||
const response = await fetch(`${origin}/api/auth/sign-up/email`, {
|
const response = await fetch(`${origin}/api/auth/sign-up/email`, {
|
||||||
body: JSON.stringify({ email, name: "Integration Test User", password }),
|
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",
|
method: "POST",
|
||||||
});
|
});
|
||||||
return { body: (await response.clone().json()) as SignUpResult["body"], response };
|
return { body: (await response.clone().json()) as SignUpResult["body"], response };
|
||||||
@@ -283,6 +289,45 @@ async function fetchPrincipal(
|
|||||||
return { body: await response.clone().json(), response };
|
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", () => {
|
describe.sequential("live API — signup, sign-in, principal, sign-out", () => {
|
||||||
let database: { name: string; url: string };
|
let database: { name: string; url: string };
|
||||||
let running: RunningApi;
|
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 () => {
|
it("supports a genuine server-to-server sign-up request that carries no Origin header and no Fetch Metadata headers at all", async () => {
|
||||||
const { response } = await signUp(origin, "[email protected]", password);
|
const { status } = await rawPostRequest(origin, "/api/auth/sign-up/email", {
|
||||||
expect(response.status).toBe(200);
|
email: "[email protected]",
|
||||||
|
name: "Integration Test User",
|
||||||
|
password,
|
||||||
|
});
|
||||||
|
expect(status).toBe(200);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("allows a trusted browser origin for the principal route with credentials", async () => {
|
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();
|
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, "[email protected]", "1234567");
|
||||||
|
expect(response.status).toBe(400);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("accepts an 8-character password", async () => {
|
||||||
|
const { response, body } = await signUp(origin, "[email protected]", "12345678");
|
||||||
|
expect(response.status).toBe(200);
|
||||||
|
expect(body.user.email).toBe("[email protected]");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("accepts a 128-character password", async () => {
|
||||||
|
const { response } = await signUp(origin, "[email protected]", "a".repeat(128));
|
||||||
|
expect(response.status).toBe(200);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects a 129-character password as too long", async () => {
|
||||||
|
const { response } = await signUp(origin, "[email protected]", "a".repeat(129));
|
||||||
|
expect(response.status).toBe(400);
|
||||||
|
});
|
||||||
|
|
||||||
it("leaves Better Auth's handler unavailable outside /api/auth/*", async () => {
|
it("leaves Better Auth's handler unavailable outside /api/auth/*", async () => {
|
||||||
const response = await fetch(`${origin}/sign-in/email`, { method: "POST" });
|
const response = await fetch(`${origin}/sign-in/email`, { method: "POST" });
|
||||||
expect(response.status).toBe(404);
|
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", () => {
|
describe.sequential("live API — startup and migration guarantees", () => {
|
||||||
it("does not create any tables or migration history when started against an unmigrated database", async () => {
|
it("does not create any tables or migration history when started against an unmigrated database", async () => {
|
||||||
const database = await createDisposableDatabase("unmigrated");
|
const database = await createDisposableDatabase("unmigrated");
|
||||||
@@ -629,3 +740,70 @@ describe.sequential("live API — principal infrastructure failure is never down
|
|||||||
await running.shutdown().catch(() => undefined);
|
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, "[email protected]", "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: "[email protected]",
|
||||||
|
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);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -11,6 +11,84 @@ vi.mock("better-auth", () => ({ betterAuth: betterAuthMock }));
|
|||||||
vi.mock("better-auth/adapters/drizzle", () => ({ drizzleAdapter: drizzleAdapterMock }));
|
vi.mock("better-auth/adapters/drizzle", () => ({ drizzleAdapter: drizzleAdapterMock }));
|
||||||
vi.mock("@hono/node-server", () => ({ serve: serveMock }));
|
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:[email protected]: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<string, () => void>; restore: () => void } {
|
||||||
|
const handlers: Record<string, () => 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", () => {
|
describe("Ekklesia API import safety", () => {
|
||||||
const originalEnv = { ...process.env };
|
const originalEnv = { ...process.env };
|
||||||
const originalProcessOn = process.on.bind(process);
|
const originalProcessOn = process.on.bind(process);
|
||||||
@@ -73,3 +151,115 @@ describe("Ekklesia API import safety", () => {
|
|||||||
expect(serveMock).not.toHaveBeenCalled();
|
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:[email protected]/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();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -6,6 +6,8 @@ export type IdentityDrizzleDatabase = NodePgDatabase<typeof identityAdapterSchem
|
|||||||
|
|
||||||
export type IdentityDatabaseErrorCode = "IDENTITY_DATABASE_CONFIGURATION_INVALID";
|
export type IdentityDatabaseErrorCode = "IDENTITY_DATABASE_CONFIGURATION_INVALID";
|
||||||
|
|
||||||
|
const maximumPostgresTimeoutMs = 2_147_483_647;
|
||||||
|
|
||||||
/** Never carries the offending connection string, host, or underlying driver error. */
|
/** Never carries the offending connection string, host, or underlying driver error. */
|
||||||
export class IdentityDatabaseError extends Error {
|
export class IdentityDatabaseError extends Error {
|
||||||
readonly code: IdentityDatabaseErrorCode;
|
readonly code: IdentityDatabaseErrorCode;
|
||||||
@@ -21,6 +23,11 @@ export class IdentityDatabaseError extends Error {
|
|||||||
|
|
||||||
export interface IdentityDatabaseOptions {
|
export interface IdentityDatabaseOptions {
|
||||||
readonly connectionTimeoutMs?: number;
|
readonly connectionTimeoutMs?: number;
|
||||||
|
/** Bounds both `query_timeout` and `statement_timeout`: a hung or unexpectedly slow query on
|
||||||
|
* the identity connection is aborted rather than holding the pool connection (and the caller)
|
||||||
|
* indefinitely. `connectionTimeoutMs` alone only bounds establishing a connection — it does
|
||||||
|
* nothing once a query is already running on one. */
|
||||||
|
readonly operationTimeoutMs?: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface IdentityDatabaseHandle {
|
export interface IdentityDatabaseHandle {
|
||||||
@@ -44,6 +51,24 @@ function validateConnectionString(connectionString: string): string {
|
|||||||
return connectionString;
|
return connectionString;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Mirrors `core/src/migrations/postgres-database.ts`'s timeout validation exactly: an explicit
|
||||||
|
* timeout must be a positive safe integer no larger than PostgreSQL's own maximum, or it is
|
||||||
|
* rejected as invalid configuration rather than silently passed through to `pg`. */
|
||||||
|
function validateTimeout(value: unknown): number | undefined {
|
||||||
|
if (value === undefined) {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
if (
|
||||||
|
typeof value !== "number" ||
|
||||||
|
!Number.isSafeInteger(value) ||
|
||||||
|
value <= 0 ||
|
||||||
|
value > maximumPostgresTimeoutMs
|
||||||
|
) {
|
||||||
|
throw new IdentityDatabaseError("IDENTITY_DATABASE_CONFIGURATION_INVALID");
|
||||||
|
}
|
||||||
|
return value;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Explicitly constructs the Core-owned Drizzle database instance used by the identity factory.
|
* 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
|
* 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 = {},
|
options: IdentityDatabaseOptions = {},
|
||||||
): IdentityDatabaseHandle {
|
): IdentityDatabaseHandle {
|
||||||
const validatedConnectionString = validateConnectionString(connectionString);
|
const validatedConnectionString = validateConnectionString(connectionString);
|
||||||
const poolConfig: PoolConfig = { connectionString: validatedConnectionString };
|
const validatedConnectionTimeoutMs = validateTimeout(options.connectionTimeoutMs);
|
||||||
if (options.connectionTimeoutMs !== undefined) {
|
const validatedOperationTimeoutMs = validateTimeout(options.operationTimeoutMs);
|
||||||
poolConfig.connectionTimeoutMillis = options.connectionTimeoutMs;
|
const poolConfig: PoolConfig = {
|
||||||
}
|
connectionString: validatedConnectionString,
|
||||||
|
...(validatedConnectionTimeoutMs === undefined
|
||||||
|
? {}
|
||||||
|
: { connectionTimeoutMillis: validatedConnectionTimeoutMs }),
|
||||||
|
...(validatedOperationTimeoutMs === undefined
|
||||||
|
? {}
|
||||||
|
: {
|
||||||
|
query_timeout: validatedOperationTimeoutMs,
|
||||||
|
statement_timeout: validatedOperationTimeoutMs,
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
|
||||||
const pool = new Pool(poolConfig);
|
const pool = new Pool(poolConfig);
|
||||||
// pg's own documentation requires an error listener on the pool: an idle client can be
|
// pg's own documentation requires an error listener on the pool: an idle client can be
|
||||||
|
|||||||
@@ -23,6 +23,30 @@ const runtimeErrorMessages: Readonly<Record<IdentityRuntimeErrorCode, string>> =
|
|||||||
|
|
||||||
const identityDatabaseProbeTimeoutMs = 3_000;
|
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. */
|
/** Never carries the underlying Better Auth error, database error, cause, stack, secret, or URL. */
|
||||||
export class IdentityRuntimeError extends Error {
|
export class IdentityRuntimeError extends Error {
|
||||||
readonly code: IdentityRuntimeErrorCode;
|
readonly code: IdentityRuntimeErrorCode;
|
||||||
@@ -113,6 +137,19 @@ export function createIdentityRuntime(
|
|||||||
|
|
||||||
const auth = constructBetterAuth(() =>
|
const auth = constructBetterAuth(() =>
|
||||||
betterAuth({
|
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,
|
baseURL: options.config.baseUrl,
|
||||||
database: drizzleAdapter(options.db, {
|
database: drizzleAdapter(options.db, {
|
||||||
provider: "pg",
|
provider: "pg",
|
||||||
@@ -122,6 +159,29 @@ export function createIdentityRuntime(
|
|||||||
autoSignIn: false,
|
autoSignIn: false,
|
||||||
disableSignUp: false,
|
disableSignUp: false,
|
||||||
enabled: true,
|
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: {
|
rateLimit: {
|
||||||
enabled: true,
|
enabled: true,
|
||||||
|
|||||||
@@ -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<string, unknown>];
|
||||||
|
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", () => {
|
it("registers a pool-level error listener so an idle client failure never becomes an uncaught exception", () => {
|
||||||
const on = vi.fn();
|
const on = vi.fn();
|
||||||
poolConstructor.mockImplementation(function fakePool() {
|
poolConstructor.mockImplementation(function fakePool() {
|
||||||
|
|||||||
@@ -75,9 +75,12 @@ describe("Core identity runtime construction boundary", () => {
|
|||||||
expect(betterAuthMock).toHaveBeenCalledTimes(1);
|
expect(betterAuthMock).toHaveBeenCalledTimes(1);
|
||||||
const [options] = betterAuthMock.mock.calls[0] as [Record<string, unknown>];
|
const [options] = betterAuthMock.mock.calls[0] as [Record<string, unknown>];
|
||||||
expect(Object.keys(options).sort()).toEqual([
|
expect(Object.keys(options).sort()).toEqual([
|
||||||
|
"advanced",
|
||||||
"baseURL",
|
"baseURL",
|
||||||
"database",
|
"database",
|
||||||
"emailAndPassword",
|
"emailAndPassword",
|
||||||
|
"logger",
|
||||||
|
"onAPIError",
|
||||||
"rateLimit",
|
"rateLimit",
|
||||||
"secret",
|
"secret",
|
||||||
"trustedOrigins",
|
"trustedOrigins",
|
||||||
@@ -86,6 +89,8 @@ describe("Core identity runtime construction boundary", () => {
|
|||||||
autoSignIn: false,
|
autoSignIn: false,
|
||||||
disableSignUp: false,
|
disableSignUp: false,
|
||||||
enabled: true,
|
enabled: true,
|
||||||
|
maxPasswordLength: 128,
|
||||||
|
minPasswordLength: 8,
|
||||||
});
|
});
|
||||||
expect(options.rateLimit).toEqual({ enabled: true, max: 100, window: 10 });
|
expect(options.rateLimit).toEqual({ enabled: true, max: 100, window: 10 });
|
||||||
expect(options.baseURL).toBe(validConfig().baseUrl);
|
expect(options.baseURL).toBe(validConfig().baseUrl);
|
||||||
@@ -93,6 +98,96 @@ describe("Core identity runtime construction boundary", () => {
|
|||||||
expect(options.trustedOrigins).toEqual([...validConfig().trustedOrigins]);
|
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<string, unknown>];
|
||||||
|
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<string, unknown>];
|
||||||
|
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<string, unknown>];
|
||||||
|
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<string, unknown>];
|
||||||
|
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", () => {
|
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() });
|
betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() });
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
Auth's own origin/CSRF checks (below) independently protect state-changing calls regardless of
|
||||||
CORS.
|
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
|
Better Auth's own CSRF/origin protections are untouched: `advanced.disableCSRFCheck` and
|
||||||
`advanced.disableOriginCheck` are never set. Verified directly and repeatedly against the pinned
|
`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()`
|
release, through the real, compiled `apps/api` HTTP server (not just a direct `auth.handler()`
|
||||||
|
|||||||
@@ -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 `[email protected]` 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.
|
||||||
@@ -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
|
- [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
|
- [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
|
- [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
|
||||||
|
|||||||
+85
-12
@@ -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
|
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
|
[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.
|
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
|
## 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
|
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)
|
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
|
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
|
cryptography replaces Better Auth's own password hashing. Password length is pinned explicitly to
|
||||||
(8–128 characters) is kept unmodified.
|
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
|
## 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
|
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
|
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.
|
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
|
Better Auth's own CSRF/origin protections are explicitly kept on:
|
||||||
(`advanced.disableCSRFCheck`/`advanced.disableOriginCheck` are never set). Verified directly
|
`core/src/identity/runtime.ts` sets `advanced: { disableCSRFCheck: false, disableOriginCheck:
|
||||||
against the running compiled server: any non-`GET`/`HEAD`/`OPTIONS` request carrying a session
|
false }`. This is deliberately explicit rather than left unset — the pinned release's own default
|
||||||
cookie must also carry a trusted `Origin` (or `Referer`), or Better Auth itself rejects it with
|
is environment-sensitive (`skipOriginCheck` falls back to `true`, silently disabling both origin
|
||||||
`403`. See ADR 0011 for the exact verification method and an important caveat about reproducing
|
and CSRF checking, whenever `isTest()` is true, which Vitest's default `NODE_ENV=test` triggers),
|
||||||
part of this guarantee inside the automated test suite.
|
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
|
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`
|
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
|
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.
|
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
|
## Rate limiting
|
||||||
|
|
||||||
Better Auth's built-in rate limiting is always enabled, including by the executable entry point —
|
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
|
rather than a genuine per-client bucket. This is not yet the final, distributed, production-grade
|
||||||
abuse-control design.
|
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
|
## Graceful shutdown
|
||||||
|
|
||||||
`startApi()` (the composition function `apps/api/src/server.ts` exports, used by both the
|
`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
|
executable entry point and tests) returns `{app, server, shutdown}`. `shutdown()` memoizes one
|
||||||
stops the HTTP listener from accepting new connections and lets in-flight requests finish, then
|
shared promise the first time it is called: every caller, concurrent or sequential, awaits the
|
||||||
closes the PostgreSQL pool. The executable entry point wires `SIGINT`/`SIGTERM` to call it and
|
exact same listener-drain-then-pool-close operation, and that operation runs exactly once no matter
|
||||||
exits afterward; a startup failure after the database pool was already created (identity
|
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
|
construction failure, or the server failing to bind its listen address) always closes that pool
|
||||||
before the failure propagates.
|
before the failure propagates.
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
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).
|
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
|
## What is not operational yet
|
||||||
|
|
||||||
No email verification, password recovery, invitation flow, email delivery, social or OAuth
|
No email verification, password recovery, invitation flow, email delivery, social or OAuth
|
||||||
|
|||||||
+32
-13
@@ -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,
|
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
|
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,
|
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
|
configuration validation (secret, base URL, and trusted-origin rules), bounded/validated
|
||||||
(explicit construction, exact schema mapping, only email/password and rate limiting enabled,
|
connection and operation (`query_timeout`/`statement_timeout`) database timeouts, the Core identity
|
||||||
sanitized failures, and that a database outage during principal resolution is never silently
|
runtime (explicit construction, exact schema mapping, only email/password and rate limiting
|
||||||
reported as anonymous), and Core migration discovery (deterministic ordering, the reserved `core`
|
enabled, an explicit `advanced.disableCSRFCheck`/`disableOriginCheck: false` pair, an explicit
|
||||||
identity, and collision rejection) — all without a live database. `apps/api`'s own unit tests cover
|
error-only sanitized logger that strips Better Auth's own message and arguments regardless of what
|
||||||
its pure Hono app factory (route shapes, CORS allow/deny/preflight, deterministic 404 and sanitized
|
they contain, pinned 8–128 password-length bounds, sanitized failures, and that a database outage
|
||||||
error responses), its runtime configuration, and that importing any of its modules — including the
|
during principal resolution is never silently reported as anonymous), and Core migration discovery
|
||||||
executable server module without calling its exported `main` — never reads the environment,
|
(deterministic ordering, the reserved `core` identity, and collision rejection) — all without a
|
||||||
connects to PostgreSQL, or starts a server.
|
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
|
`pnpm verify` is intentionally Docker-free. It never plans or applies migrations against a live
|
||||||
database.
|
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
|
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
|
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
|
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
|
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`,
|
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
|
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)
|
shutdown.
|
||||||
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.
|
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
|
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:
|
the following pull-request status context to `main` branch protection:
|
||||||
|
|||||||
Generated
+3
@@ -54,6 +54,9 @@ importers:
|
|||||||
'@types/pg':
|
'@types/pg':
|
||||||
specifier: 8.20.0
|
specifier: 8.20.0
|
||||||
version: 8.20.0
|
version: 8.20.0
|
||||||
|
better-auth:
|
||||||
|
specifier: 1.6.23
|
||||||
|
version: 1.6.23([email protected])([email protected](@types/[email protected])([email protected])([email protected]))([email protected])([email protected](@types/[email protected])([email protected](@types/[email protected])([email protected])([email protected])([email protected])([email protected])))
|
||||||
pg:
|
pg:
|
||||||
specifier: 8.22.0
|
specifier: 8.22.0
|
||||||
version: 8.22.0
|
version: 8.22.0
|
||||||
|
|||||||
Reference in New Issue
Block a user