fix: harden identity HTTP transport
This commit is contained in:
@@ -6,6 +6,8 @@ export type IdentityDrizzleDatabase = NodePgDatabase<typeof identityAdapterSchem
|
||||
|
||||
export type IdentityDatabaseErrorCode = "IDENTITY_DATABASE_CONFIGURATION_INVALID";
|
||||
|
||||
const maximumPostgresTimeoutMs = 2_147_483_647;
|
||||
|
||||
/** Never carries the offending connection string, host, or underlying driver error. */
|
||||
export class IdentityDatabaseError extends Error {
|
||||
readonly code: IdentityDatabaseErrorCode;
|
||||
@@ -21,6 +23,11 @@ export class IdentityDatabaseError extends Error {
|
||||
|
||||
export interface IdentityDatabaseOptions {
|
||||
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 {
|
||||
@@ -44,6 +51,24 @@ function validateConnectionString(connectionString: string): string {
|
||||
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.
|
||||
* 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
|
||||
|
||||
@@ -23,6 +23,30 @@ const runtimeErrorMessages: Readonly<Record<IdentityRuntimeErrorCode, string>> =
|
||||
|
||||
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,
|
||||
|
||||
@@ -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", () => {
|
||||
const on = vi.fn();
|
||||
poolConstructor.mockImplementation(function fakePool() {
|
||||
|
||||
@@ -75,9 +75,12 @@ describe("Core identity runtime construction boundary", () => {
|
||||
expect(betterAuthMock).toHaveBeenCalledTimes(1);
|
||||
const [options] = betterAuthMock.mock.calls[0] as [Record<string, unknown>];
|
||||
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<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", () => {
|
||||
betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() });
|
||||
|
||||
|
||||
Reference in New Issue
Block a user