Files
Ekklesia/core/tests/postgres-migration-database.test.ts
T
ekklesiaandClaude Opus 5 6912680113 fix: prevent migration timeout overrides
pg-connection-string retains arbitrary query keys, so the driver's own
configuration key connectionTimeoutMillis reaches ConnectionParameters straight
from a URL — and it was not on the rejected list, while spellings that do
nothing were. Rejecting it closes that gap.

The precedence claims that guard was built on were partly wrong, so every source
was measured against the installed [email protected].0, a real blackhole listener, and a
real PostgreSQL rather than inferred from the merge:

  statement_timeout=0 / =900000   server reported 0 and 15min — wins outright
  query_timeout=0                 overrides the field, but the parsed value is
                                  the string "0", which is truthy, so every
                                  query gets a 0 ms read timer and fails
                                  nondeterministically — it breaks the client
                                  rather than loosening the bound
  connectionTimeoutMillis=0/900000 pollutes connectionParameters.connect_timeout
                                  (10 -> 0 or 900 s), but that field only feeds
                                  getLibpqConnectionString(), which the pure-JS
                                  client never calls; the effective timer was
                                  unchanged, with an explicit 500 ms still
                                  expiring at ~500 ms either way
  connect_timeout=0               no effect: recomputed from the explicit field
  options=-c statement_timeout=…  no effect, including a backslash-escaped
                                  spelling — the named startup parameter wins
  PGOPTIONS / PGCONNECT_TIMEOUT   no effect
  unrelated parameters            no effect, deliberately untouched

Only statement_timeout is a genuine downgrade. The rest are rejected as
unsupported configuration and defense in depth: no legitimate DATABASE_URL
carries a driver option, the polluted connect_timeout would take effect on the
native binding, and query_timeout produces intermittent failures an operator
could not diagnose. That distinction is now stated where it was previously
asserted wrongly — the earlier comment and ADR text claimed connect_timeout
overrode the explicit field, which measurement disproved.

Rejection still happens at the one shared construction boundary, before any
pg.Client exists, with the existing fixed POSTGRES_MIGRATION_CONFIGURATION_INVALID
and no URL, host, port, credential, parameter, or value in the message. The
10 s defaults, narrower test overrides, Pass 2 TLS policy and its URL-conflict
checks, and every unrelated parameter are unchanged.

Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-28 19:01:46 -05:00

299 lines
11 KiB
TypeScript

import { describe, expect, it, vi } from "vitest";
const clientConstructor = vi.hoisted(() => vi.fn());
vi.mock("pg", () => ({ Client: clientConstructor }));
import {
createPostgresMigrationDatabase,
MIGRATION_CONNECTION_TIMEOUT_MS,
MIGRATION_OPERATION_TIMEOUT_MS,
PostgresMigrationDatabaseError,
} from "../src/migrations/postgres-database.js";
describe("PostgreSQL migration database composition", () => {
it("does not construct a client or execute SQL merely because the adapter is imported", () => {
expect(clientConstructor).not.toHaveBeenCalled();
});
it.each([
undefined,
"",
"not-a-database-url",
"https://admin:[email protected]/database",
])("rejects missing or invalid configuration without exposing it (%s)", (databaseUrl) => {
let captured: unknown;
try {
createPostgresMigrationDatabase(databaseUrl);
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
const publicText = `${String(captured)}\n${
captured instanceof Error ? (captured.stack ?? "") : ""
}\n${JSON.stringify(captured)}`;
if (databaseUrl) {
expect(publicText).not.toContain(databaseUrl);
}
expect(clientConstructor).not.toHaveBeenCalled();
});
it("creates a lazy database composition without opening a connection", () => {
const database = createPostgresMigrationDatabase(
"postgresql://operator:[email protected]/ekklesia",
);
expect(Object.isFrozen(database)).toBe(true);
expect(clientConstructor).not.toHaveBeenCalled();
expect(JSON.stringify(database)).not.toContain("operator");
expect(JSON.stringify(database)).not.toContain("do-not-log");
expect(JSON.stringify(database)).not.toContain("example.test");
});
it.each([
["connectionTimeoutMs", 0],
["connectionTimeoutMs", -1],
["connectionTimeoutMs", 1.5],
["connectionTimeoutMs", Number.NaN],
["connectionTimeoutMs", Number.POSITIVE_INFINITY],
["connectionTimeoutMs", 2_147_483_648],
["operationTimeoutMs", 0],
["operationTimeoutMs", -1],
["operationTimeoutMs", 1.5],
["operationTimeoutMs", Number.NaN],
["operationTimeoutMs", Number.POSITIVE_INFINITY],
["operationTimeoutMs", 2_147_483_648],
] as const)("rejects an invalid %s value without constructing a client", (name, value) => {
let captured: unknown;
try {
createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
[name]: value,
});
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
expect(clientConstructor).not.toHaveBeenCalled();
});
it("passes bounded connection and operation timeouts to the PostgreSQL client", async () => {
const client = {
connect: vi.fn().mockResolvedValue(undefined),
end: vi.fn().mockResolvedValue(undefined),
on: vi.fn(),
};
const MockClient = class {
readonly connect = client.connect;
readonly end = client.end;
readonly on = client.on;
};
clientConstructor.mockImplementation(MockClient as unknown as () => typeof client);
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
connectionTimeoutMs: 10_000,
operationTimeoutMs: 12_000,
});
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: 10_000,
query_timeout: 12_000,
statement_timeout: 12_000,
});
expect(client.on).toHaveBeenCalledWith("error", expect.any(Function));
expect(client.connect).toHaveBeenCalledOnce();
await session.close();
expect(client.end).toHaveBeenCalledOnce();
});
});
/**
* Pass 4: finite migration timeouts live at this one construction boundary, and the pinned driver's
* connection-string precedence cannot weaken them.
*/
describe("PostgreSQL migration timeout policy", () => {
function mockClient() {
const client = {
connect: vi.fn().mockResolvedValue(undefined),
end: vi.fn().mockResolvedValue(undefined),
on: vi.fn(),
};
clientConstructor.mockImplementation(
class {
readonly connect = client.connect;
readonly end = client.end;
readonly on = client.on;
} as unknown as () => typeof client,
);
return client;
}
it("applies the fixed 10-second defaults when a caller omits them", async () => {
mockClient();
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia");
const session = await database.openSession();
// This is exactly what `migrations:plan` and `migrations:apply` construct: no timeout options.
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: MIGRATION_CONNECTION_TIMEOUT_MS,
query_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
});
expect(MIGRATION_CONNECTION_TIMEOUT_MS).toBe(10_000);
expect(MIGRATION_OPERATION_TIMEOUT_MS).toBe(10_000);
await session.close();
});
it("still honours an explicit narrower test override", async () => {
mockClient();
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
connectionTimeoutMs: 250,
operationTimeoutMs: 500,
});
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: 250,
query_timeout: 500,
statement_timeout: 500,
});
await session.close();
});
it.each([
"postgresql://example.test/ekklesia?statement_timeout=0",
"postgresql://example.test/ekklesia?STATEMENT_TIMEOUT=0",
"postgresql://example.test/ekklesia?query_timeout=0",
"postgresql://example.test/ekklesia?connect_timeout=0",
"postgresql://example.test/ekklesia?options=-c%20statement_timeout%3D0",
"postgresql://example.test/ekklesia?statement_timeout=900000",
])("refuses a connection string that would override the policy (%s)", (databaseUrl) => {
// The pinned [email protected] merges as `Object.assign({}, config, parse(connectionString))`, so the
// URL wins over the explicit values above — verified against the real driver, where an explicit
// `statement_timeout: 10000` alongside `?statement_timeout=0` produced a server-reported `0`.
const constructionsBefore = clientConstructor.mock.calls.length;
let captured: unknown;
try {
createPostgresMigrationDatabase(databaseUrl);
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
// The rejection never echoes the URL, the offending parameter, or its value.
const publicText = `${String(captured)}\n${
captured instanceof Error ? (captured.stack ?? "") : ""
}`;
expect(publicText).not.toContain("example.test");
expect(publicText).not.toContain("statement_timeout=");
expect(publicText).not.toContain("900000");
// Rejected before any client is constructed, so no connection is ever attempted.
expect(clientConstructor.mock.calls.length).toBe(constructionsBefore);
});
it("leaves unrelated connection-string parameters alone", async () => {
mockClient();
const database = createPostgresMigrationDatabase(
"postgresql://example.test/ekklesia?application_name=ekklesia&lock_timeout=5000",
);
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith(
expect.objectContaining({ statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS }),
);
await session.close();
});
});
/**
* Pass 4 correction: `pg-connection-string` retains *arbitrary* query keys, so the driver's own
* configuration key `connectionTimeoutMillis` reaches `ConnectionParameters` from a URL. It was not
* on the rejected list. These pin the corrected boundary; the measured effect of each key on the
* real driver and a real server lives in `core/tests/postgres-migration.integration.ts`.
*/
describe("PostgreSQL migration timeout precedence", () => {
it.each([
"postgresql://example.test/ekklesia?connectionTimeoutMillis=0",
"postgresql://example.test/ekklesia?connectionTimeoutMillis=900000",
"postgresql://example.test/ekklesia?connectionTimeoutMillis=",
"postgresql://example.test/ekklesia?connectionTimeoutMillis=-1",
"postgresql://example.test/ekklesia?connectionTimeoutMillis=abc",
"postgresql://example.test/ekklesia?CONNECTIONTIMEOUTMILLIS=0",
"postgresql://example.test/ekklesia?connectiontimeoutmillis=0",
])("refuses the driver's own connectionTimeoutMillis key in a URL (%s)", (databaseUrl) => {
const constructionsBefore = clientConstructor.mock.calls.length;
let captured: unknown;
try {
createPostgresMigrationDatabase(databaseUrl);
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
// Rejected before any client exists, so no connection is ever attempted.
expect(clientConstructor.mock.calls.length).toBe(constructionsBefore);
const publicText = `${String(captured)}\n${
captured instanceof Error ? (captured.stack ?? "") : ""
}`;
expect(publicText).not.toContain("example.test");
expect(publicText).not.toContain("connectionTimeoutMillis");
expect(publicText).not.toContain("900000");
});
it("also refuses it alongside a narrower explicit test override", () => {
let captured: unknown;
try {
createPostgresMigrationDatabase(
"postgresql://example.test/ekklesia?connectionTimeoutMillis=0",
{ connectionTimeoutMs: 300, operationTimeoutMs: 300 },
);
} catch (error) {
captured = error;
}
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
});
it("still accepts unrelated parameters and produces the exact expected client configuration", async () => {
const client = {
connect: vi.fn().mockResolvedValue(undefined),
end: vi.fn().mockResolvedValue(undefined),
on: vi.fn(),
};
clientConstructor.mockImplementation(
class {
readonly connect = client.connect;
readonly end = client.end;
readonly on = client.on;
} as unknown as () => typeof client,
);
const url = "postgresql://example.test/ekklesia?application_name=ekklesia&lock_timeout=5000";
const session = await createPostgresMigrationDatabase(url).openSession();
expect(clientConstructor).toHaveBeenLastCalledWith({
connectionString: url,
connectionTimeoutMillis: MIGRATION_CONNECTION_TIMEOUT_MS,
query_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
});
await session.close();
});
});