fix: distinguish identity session failures
Principal resolution could not tell a broken session store from a genuinely anonymous request, and did unnecessary database work for callers that never presented a session at all. Both are corrected here. Any cookie previously triggered the ambiguity check, so `Cookie: x=y` on an otherwise anonymous request caused an identity database round trip an unauthenticated caller could drive with a header they chose themselves. `hasConfiguredSessionCookie` now parses the Cookie header per RFC 6265 (split on `;`, name is the text before the first `=`, trimmed) and compares the whole name exactly — never `includes`, a substring test, or an unanchored regular expression — so `better-auth.session_token_x`, `x_better-auth.session_token`, a differently-cased spelling, and a percent-encoded lookalike are all correctly not the session cookie. A duplicated name still reports true: ambiguity fails safe towards checking, and reporting true never grants access on its own. The cookie name is not a second hard-coded copy. One `betterAuthOptions` object now feeds both `betterAuth()` and Better Auth's own supported `getCookies` helper, so the detected name — including the `__Secure-` prefix an https baseURL implies — is derived from the exact configuration Better Auth itself uses, and drift is structurally impossible rather than merely tested. The readiness check itself proved almost nothing: `select 1` still succeeded against a dropped session table, a drifted or removed column, or a revoked SELECT privilege, so Better Auth's ambiguous null was reported as a genuinely anonymous request. `probeSessionStore` now plans a real read of the session relation and the exact columns the adapter depends on, under `where false`: PostgreSQL resolves the relation, every column, and the caller's privilege at plan time while matching zero rows, so no session row, token, or user id can be read, logged, or enumerated by the check. It uses the injected pool, never a second connection, and stays bounded by the existing timeout. A missing relation, dropped column, revoked privilege, unreachable connection, and query timeout now all produce the same sanitized 503 with no relation name, column name, SQL, driver text, host, or stack — proven against real PostgreSQL, since that is the only place a broken store looks exactly like an anonymous request. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
ekklesia
co-authored by
Claude Opus 5
Claude Opus 5
parent
3d6c85b0f8
commit
7a0f624fc3
@@ -9,6 +9,7 @@ vi.mock("better-auth", () => ({ betterAuth: betterAuthMock }));
|
||||
vi.mock("better-auth/adapters/drizzle", () => ({ drizzleAdapter: drizzleAdapterMock }));
|
||||
|
||||
import type { IdentityConfig } from "../src/identity/config.js";
|
||||
import type { IdentityDrizzleDatabase } from "../src/identity/database.js";
|
||||
import {
|
||||
createIdentityRuntime,
|
||||
IdentityRuntimeError,
|
||||
@@ -26,8 +27,43 @@ function validConfig(overrides: Partial<IdentityConfig> = {}): IdentityConfig {
|
||||
});
|
||||
}
|
||||
|
||||
function fakeDb(overrides: Partial<{ execute: ReturnType<typeof vi.fn> }> = {}) {
|
||||
return { __fakeDb: true, execute: vi.fn().mockResolvedValue(undefined), ...overrides } as never;
|
||||
interface FakeDbOptions {
|
||||
/** Makes the session-store readiness read reject, standing in for a missing relation, a dropped
|
||||
* column, a denied `SELECT` privilege, an unavailable connection, or a query timeout — every one
|
||||
* of which surfaces to this seam identically, as a rejected query. */
|
||||
readonly sessionProbeError?: Error;
|
||||
}
|
||||
|
||||
/**
|
||||
* A fake Identity Drizzle view that records every query actually issued, so a test can prove the
|
||||
* *absence* of identity database work — not merely that a particular helper was not called. Both
|
||||
* the legacy `execute` seam and the `select(...).from(...).where(...)` chain the session-store
|
||||
* readiness check uses are recorded into one ordered list.
|
||||
*/
|
||||
function fakeDb(options: FakeDbOptions = {}) {
|
||||
const queries: string[] = [];
|
||||
const execute = vi.fn(() => {
|
||||
queries.push("execute");
|
||||
return Promise.resolve(undefined);
|
||||
});
|
||||
// Drizzle's query builder is awaited directly, so the terminal `where(...)` returns a real
|
||||
// Promise rather than a hand-rolled thenable object.
|
||||
const select = vi.fn((columns: Record<string, unknown>) => {
|
||||
queries.push(`select:${Object.keys(columns).sort().join(",")}`);
|
||||
const builder = {
|
||||
from: vi.fn(() => builder),
|
||||
where: vi.fn(() =>
|
||||
options.sessionProbeError
|
||||
? Promise.reject(options.sessionProbeError)
|
||||
: Promise.resolve([] as unknown[]),
|
||||
),
|
||||
};
|
||||
return builder;
|
||||
});
|
||||
const db = { __fakeDb: true, execute, queries, select };
|
||||
// The runtime only ever uses the narrow surface faked above; the cast keeps the fake honest
|
||||
// about that instead of stubbing all of `NodePgDatabase`, while `queries` stays observable.
|
||||
return db as unknown as IdentityDrizzleDatabase & { readonly queries: string[] };
|
||||
}
|
||||
|
||||
describe("Core identity runtime construction boundary", () => {
|
||||
@@ -327,65 +363,152 @@ describe("Core identity runtime — resolvePrincipal", () => {
|
||||
drizzleAdapterMock.mockClear();
|
||||
});
|
||||
|
||||
it("maps a valid session to an authenticated principal containing only the opaque user id", async () => {
|
||||
/** The configured production (https) session-cookie name, derived by Better Auth's own
|
||||
* `getCookies` helper from the exact same `baseUrl` this suite's `validConfig` uses. */
|
||||
const productionSessionCookie = "__Secure-better-auth.session_token";
|
||||
|
||||
it("maps a valid session to an authenticated principal containing only the opaque user id, without any follow-up probe", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue({
|
||||
session: { id: "sess_1", token: "tok" },
|
||||
user: { email: "[email protected]", id: "user_123", name: "Person" },
|
||||
});
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
const db = fakeDb();
|
||||
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db: fakeDb() });
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db });
|
||||
const principal = await runtime.resolvePrincipal(
|
||||
new Headers({ cookie: "better-auth.session_token=tok" }),
|
||||
new Headers({ cookie: `${productionSessionCookie}=tok` }),
|
||||
);
|
||||
|
||||
expect(principal).toEqual({ kind: "authenticated", userId: "user_123" });
|
||||
expect(Object.keys(principal)).toEqual(["kind", "userId"]);
|
||||
expect(Object.isFrozen(principal)).toBe(true);
|
||||
// A resolved session needs no readiness confirmation — the store demonstrably worked.
|
||||
expect(db.queries).toEqual([]);
|
||||
});
|
||||
|
||||
it("maps a missing session with no cookie header to anonymous without probing the database", async () => {
|
||||
it("performs zero identity database work for no cookie, unrelated cookies, or session-cookie lookalikes", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue(null);
|
||||
const execute = vi.fn().mockResolvedValue(undefined);
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db: fakeDb({ execute }) });
|
||||
const principal = await runtime.resolvePrincipal(new Headers());
|
||||
for (const cookie of [
|
||||
undefined,
|
||||
"x=y",
|
||||
"a=1; b=2; c=3",
|
||||
// Lookalikes: a prefix, a suffix, a differently-cased spelling, a percent-encoded name, and
|
||||
// the unprefixed development name (wrong for this https configuration).
|
||||
`${productionSessionCookie}_extra=v`,
|
||||
`extra_${productionSessionCookie}=v`,
|
||||
`${productionSessionCookie.toUpperCase()}=v`,
|
||||
"__Secure-better-auth%2Esession_token=v",
|
||||
"better-auth.session_token=v",
|
||||
]) {
|
||||
const db = fakeDb();
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db });
|
||||
const principal = await runtime.resolvePrincipal(
|
||||
cookie === undefined ? new Headers() : new Headers({ cookie }),
|
||||
);
|
||||
|
||||
expect(principal).toEqual({ kind: "anonymous" });
|
||||
expect(execute).not.toHaveBeenCalled();
|
||||
expect(principal).toEqual({ kind: "anonymous" });
|
||||
expect(db.queries).toEqual([]);
|
||||
}
|
||||
});
|
||||
|
||||
it("maps an expired/revoked/malformed session (cookie present, null session) to anonymous when the database is reachable", async () => {
|
||||
it("reads the real session relation and its expected columns only when the configured session cookie was presented", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue(null);
|
||||
const execute = vi.fn().mockResolvedValue(undefined);
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
const db = fakeDb();
|
||||
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db: fakeDb({ execute }) });
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db });
|
||||
const principal = await runtime.resolvePrincipal(
|
||||
new Headers({ cookie: "better-auth.session_token=stale-or-garbage" }),
|
||||
new Headers({ cookie: `${productionSessionCookie}=stale-or-garbage` }),
|
||||
);
|
||||
|
||||
// A healthy session store plus an unresolvable token is a genuinely anonymous caller.
|
||||
expect(principal).toEqual({ kind: "anonymous" });
|
||||
expect(execute).toHaveBeenCalledTimes(1);
|
||||
// The readiness check is a real read of the session relation's expected columns — never
|
||||
// `select 1`, and never a generic pool ping.
|
||||
expect(db.queries).toEqual(["select:expiresAt,id,token,userId"]);
|
||||
});
|
||||
|
||||
it("never downgrades an infrastructure failure to anonymous: throws when the database is unreachable", async () => {
|
||||
it("detects the session cookie among several cookies and with irregular whitespace", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue(null);
|
||||
const execute = vi.fn().mockRejectedValue(new Error("connection terminated unexpectedly"));
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db: fakeDb({ execute }) });
|
||||
|
||||
let captured: unknown;
|
||||
try {
|
||||
await runtime.resolvePrincipal(new Headers({ cookie: "better-auth.session_token=x" }));
|
||||
} catch (error) {
|
||||
captured = error;
|
||||
for (const cookie of [
|
||||
`a=1; ${productionSessionCookie}=tok; b=2`,
|
||||
` ${productionSessionCookie}=tok `,
|
||||
`a=1;${productionSessionCookie}=tok`,
|
||||
// A duplicated session cookie is ambiguous and must fail safe *towards* checking the store.
|
||||
`${productionSessionCookie}=one; ${productionSessionCookie}=two`,
|
||||
// A valueless session cookie still means Better Auth saw the name.
|
||||
`${productionSessionCookie}`,
|
||||
]) {
|
||||
const db = fakeDb();
|
||||
const runtime = createIdentityRuntime({ config: validConfig(), db });
|
||||
await runtime.resolvePrincipal(new Headers({ cookie }));
|
||||
expect(db.queries).toEqual(["select:expiresAt,id,token,userId"]);
|
||||
}
|
||||
});
|
||||
|
||||
expect(captured).toBeInstanceOf(IdentityRuntimeError);
|
||||
expect(captured).toMatchObject({ code: "PRINCIPAL_RESOLUTION_FAILED" });
|
||||
it("uses the unprefixed development cookie name when the configured base URL is http", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue(null);
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
const developmentConfig = validConfig({
|
||||
baseUrl: "http://localhost:3000",
|
||||
trustedOrigins: Object.freeze(["http://localhost:3000"]),
|
||||
});
|
||||
|
||||
const unprefixed = fakeDb();
|
||||
await createIdentityRuntime({ config: developmentConfig, db: unprefixed }).resolvePrincipal(
|
||||
new Headers({ cookie: "better-auth.session_token=tok" }),
|
||||
);
|
||||
expect(unprefixed.queries).toEqual(["select:expiresAt,id,token,userId"]);
|
||||
|
||||
// The production `__Secure-` name is *not* what an http deployment reads.
|
||||
const prefixed = fakeDb();
|
||||
await createIdentityRuntime({ config: developmentConfig, db: prefixed }).resolvePrincipal(
|
||||
new Headers({ cookie: `${productionSessionCookie}=tok` }),
|
||||
);
|
||||
expect(prefixed.queries).toEqual([]);
|
||||
});
|
||||
|
||||
it("never downgrades a broken session store to anonymous, and leaks no database detail", async () => {
|
||||
const getSession = vi.fn().mockResolvedValue(null);
|
||||
betterAuthMock.mockReturnValue({ api: { getSession }, handler: vi.fn() });
|
||||
|
||||
// Every distinguishable session-store failure reaches this seam as a rejected read: a missing
|
||||
// relation, a dropped column, a denied privilege, a dead connection, and a timeout alike.
|
||||
const hostileFailures = [
|
||||
new Error('relation "ekklesia_core_session" does not exist'),
|
||||
new Error('column "token" does not exist'),
|
||||
new Error("permission denied for table ekklesia_core_session at 10.1.2.3:5432"),
|
||||
new Error("connection terminated unexpectedly"),
|
||||
new Error("timed out"),
|
||||
];
|
||||
for (const sessionProbeError of hostileFailures) {
|
||||
const runtime = createIdentityRuntime({
|
||||
config: validConfig(),
|
||||
db: fakeDb({ sessionProbeError }),
|
||||
});
|
||||
|
||||
let captured: unknown;
|
||||
try {
|
||||
await runtime.resolvePrincipal(new Headers({ cookie: `${productionSessionCookie}=x` }));
|
||||
} catch (error) {
|
||||
captured = error;
|
||||
}
|
||||
|
||||
expect(captured).toBeInstanceOf(IdentityRuntimeError);
|
||||
expect(captured).toMatchObject({ code: "PRINCIPAL_RESOLUTION_FAILED" });
|
||||
const publicText = `${String(captured)}\n${
|
||||
captured instanceof Error ? (captured.stack ?? "") : ""
|
||||
}\n${JSON.stringify(captured)}`;
|
||||
expect(publicText).not.toContain("ekklesia_core_session");
|
||||
expect(publicText).not.toContain("permission denied");
|
||||
expect(publicText).not.toContain("10.1.2.3");
|
||||
expect(publicText).not.toContain("does not exist");
|
||||
}
|
||||
});
|
||||
|
||||
it("treats an unexpected throw from getSession itself as a sanitized infrastructure failure", async () => {
|
||||
|
||||
Reference in New Issue
Block a user