diff --git a/core/src/migrations/postgres-database.ts b/core/src/migrations/postgres-database.ts index 8645e05..7e038da 100644 --- a/core/src/migrations/postgres-database.ts +++ b/core/src/migrations/postgres-database.ts @@ -94,20 +94,46 @@ export const MIGRATION_CONNECTION_TIMEOUT_MS = 10_000; export const MIGRATION_OPERATION_TIMEOUT_MS = 10_000; /** - * Connection-string parameters the pinned `pg@8.22.0` would let silently override the timeouts - * selected above. Its `ConnectionParameters` merges as - * `Object.assign({}, config, parse(config.connectionString))`, so the **URL wins** over explicitly - * passed options — verified directly against the pinned driver, where an explicit - * `statement_timeout: 10000` alongside `?statement_timeout=0` produced a server-reported - * `statement_timeout` of `0`. Rejecting these is therefore not stylistic: without it the policy - * above is advisory. + * Connection-string parameters that must not reach the pinned driver, established by measuring the + * installed `pg@8.22.0`/`pg-connection-string@2.14.0` against a real PostgreSQL and a real + * blackhole listener rather than by reading the merge and assuming. * - * Deliberately narrow — only settings that actually conflict with the timeouts this boundary - * selects. `options` is included solely because it can carry `-c statement_timeout=...`, and is - * rejected only when it does. + * `ConnectionParameters` merges as `Object.assign({}, config, parse(config.connectionString))` and + * `pg-connection-string` retains *arbitrary* query keys, so any of these lands in the driver's + * configuration. What each one actually does differs, and only the first genuinely weakens policy: + * + * - `statement_timeout` — **wins outright.** `?statement_timeout=0` produced a server-reported + * `statement_timeout` of `0` (disabled) and `?statement_timeout=900000` produced `15min`, against + * an explicit `statement_timeout: 10000`. This is the one real downgrade. + * - `query_timeout` — overrides the explicit field, but does not weaken the bound; it breaks the + * client. `client.query` computes its read timer as + * `queryConfig.query_timeout || connectionParameters.query_timeout`, and the parsed URL value is + * the *string* `"0"` — truthy — so every query gets a 0 ms read timer and fails + * nondeterministically with `Query read timeout`. Rejected so an operator gets a fixed + * configuration error instead of intermittent, unexplained query failures. + * - `connectionTimeoutMillis` — pollutes `ConnectionParameters.connect_timeout` (10 → `0` or `900` + * seconds), but that field is only consumed by `getLibpqConnectionString()`, which the pure-JS + * client never calls. Measured against a blackhole listener, the effective connect timer is + * **unchanged**: an explicit 500 ms still expired at ~500 ms with `?connectionTimeoutMillis=0` + * and with `=900000`. Rejected as unsupported and as defense in depth — no legitimate + * `DATABASE_URL` carries a driver option, and the polluted value would take effect on the native + * binding — not because it currently defeats the deadline. + * - `connect_timeout` — no effect at all: `ConnectionParameters` recomputes it from + * `connectionTimeoutMillis`, so `?connect_timeout=0` left it at the derived `10`. Rejected as + * unsupported; earlier revisions of this comment wrongly claimed it overrode the explicit field. + * - `options` carrying `statement_timeout` — measured **not** to override the named startup + * parameter: `?options=-c statement_timeout=0`, `=900s`, and a backslash-escaped spelling all + * still produced `10s`, as did an inherited `PGOPTIONS`. Rejected as unsupported rather than + * because it wins. + * + * Deliberately narrow: unrelated parameters (`application_name`, and anything else that does not + * name a timeout this boundary selects) are untouched. `PGCONNECT_TIMEOUT` needs no handling — the + * driver consults it only when `connectionTimeoutMillis` is `undefined`, and this boundary always + * sets it; an inherited `PGCONNECT_TIMEOUT=0` was measured to change nothing. */ const CONFLICTING_TIMEOUT_PARAMETERS = Object.freeze([ "connect_timeout", + "connectiontimeoutmillis", "query_timeout", "statement_timeout", ]); diff --git a/core/tests/postgres-migration-database.test.ts b/core/tests/postgres-migration-database.test.ts index 6e1f799..e061d78 100644 --- a/core/tests/postgres-migration-database.test.ts +++ b/core/tests/postgres-migration-database.test.ts @@ -219,3 +219,80 @@ describe("PostgreSQL migration timeout policy", () => { 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(); + }); +}); diff --git a/core/tests/postgres-migration.integration.ts b/core/tests/postgres-migration.integration.ts index 825621e..ada4ebb 100644 --- a/core/tests/postgres-migration.integration.ts +++ b/core/tests/postgres-migration.integration.ts @@ -1635,3 +1635,219 @@ describe.sequential("live PostgreSQL migration command hardening", () => { }); }); }); + +/** + * Pass 4 correction: what each timeout-shaped connection-string key and inherited environment + * variable *actually* does to the pinned `pg@8.22.0`, measured rather than inferred from its merge. + * + * The distinction matters: the boundary rejects several keys, but only one of them genuinely + * weakens the selected policy. Documenting a key as "overrides the explicit field" when it does not + * is the kind of claim this suite exists to prevent. + * + * Every listener, socket, client, and disposable database is closed in `finally`. + */ +describe.sequential("live PostgreSQL migration timeout precedence", () => { + const productionClientFields = Object.freeze({ + connectionTimeoutMillis: 10_000, + query_timeout: 10_000, + statement_timeout: 10_000, + }); + + it("does not let a URL weaken the effective connect timer, whichever spelling is used", async () => { + const sockets: net.Socket[] = []; + const listener = net.createServer((socket) => { + sockets.push(socket); + }); + try { + await new Promise((resolve, reject) => { + listener.once("error", reject); + listener.listen(0, "127.0.0.1", () => resolve()); + }); + const address = listener.address(); + if (!address || typeof address !== "object") { + throw harnessError("The blackhole listener has no bound address."); + } + const base = `postgresql://ekklesia:ekklesia@127.0.0.1:${address.port}/ekklesia`; + + // A short explicit deadline stands in for the production 10 s so the measurement is fast. + for (const query of [ + "", + "?connectionTimeoutMillis=0", + "?connectionTimeoutMillis=900000", + "?connect_timeout=0", + ]) { + const client = new Client({ + connectionString: `${base}${query}`, + connectionTimeoutMillis: 500, + }); + client.on("error", () => undefined); + const startedAt = Date.now(); + let settled = false; + try { + await client.connect(); + } catch { + settled = true; + } + const elapsed = Date.now() - startedAt; + try { + await client.end(); + } catch { + // Already unusable. + } + + // The pure-JS client reads `connectionTimeoutMillis` from the object handed to its + // constructor, before any connection-string merge; the merged value only reaches + // `ConnectionParameters.connect_timeout`, which solely feeds `getLibpqConnectionString()`. + expect(settled, `expected the connect timer to fire for ${JSON.stringify(query)}`).toBe( + true, + ); + expect(elapsed, `unexpected timer for ${JSON.stringify(query)}`).toBeLessThan(3_000); + } + + // ...but the driver-visible field really is polluted, which is why the boundary rejects the + // key rather than relying on the pure-JS path staying this way. `connectionParameters` is + // real on the pinned client but absent from its published types, so it is read through a + // narrow structural view rather than asserted away. + const connectTimeoutOf = (connectionString: string): string => { + const client = new Client({ connectionString, ...productionClientFields }) as unknown as { + readonly connectionParameters: { readonly connect_timeout: unknown }; + }; + (client as unknown as Client).on("error", () => undefined); + return String(client.connectionParameters.connect_timeout); + }; + expect(connectTimeoutOf(`${base}?connectionTimeoutMillis=0`)).toBe("0"); + expect(connectTimeoutOf(base)).toBe("10"); + } finally { + for (const socket of sockets) { + socket.destroy(); + } + await new Promise((resolve) => { + listener.close(() => resolve()); + }); + } + }); + + it("records what each timeout source does to the real server's statement_timeout", async () => { + await withIsolatedDatabase("timeout_sources", async (database) => { + async function effectiveStatementTimeout( + query: string, + environment: Readonly> = {}, + ): Promise { + const saved = new Map(); + for (const [key, value] of Object.entries(environment)) { + saved.set(key, process.env[key]); + process.env[key] = value; + } + const client = new Client({ + connectionString: `${database.url}${query}`, + ...productionClientFields, + }); + client.on("error", () => undefined); + try { + await client.connect(); + const result = await client.query<{ statement_timeout: string }>( + "show statement_timeout", + ); + return result.rows[0]?.statement_timeout ?? "unknown"; + } catch { + return "connection failed"; + } finally { + try { + await client.end(); + } catch { + // Already unusable. + } + for (const [key, value] of saved) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } + } + + // The baseline the policy selects. + expect(await effectiveStatementTimeout("")).toBe("10s"); + + // The one key that genuinely wins, and the reason it must stay rejected. + expect(await effectiveStatementTimeout("?statement_timeout=0")).toBe("0"); + expect(await effectiveStatementTimeout("?statement_timeout=900000")).toBe("15min"); + + // `query_timeout` from the URL *does* override the explicit field, but it does not weaken the + // bound — it breaks the client. `client.query` computes its read timer as + // `queryConfig.query_timeout || connectionParameters.query_timeout`, and the parsed URL value + // is the *string* `"0"`, which is truthy, so every query gets a 0 ms read timer and fails + // nondeterministically ("Query read timeout") depending on which wins the race. The + // deterministic, assertable fact is the parameter the URL installed. + const racyClient = new Client({ + connectionString: `${database.url}?query_timeout=0`, + ...productionClientFields, + }) as unknown as { readonly connectionParameters: { readonly query_timeout: unknown } }; + (racyClient as unknown as Client).on("error", () => undefined); + expect(racyClient.connectionParameters.query_timeout).toBe("0"); + + // `options` does *not* override the named startup parameter — including a backslash-escaped + // spelling — so rejecting it is unsupported-configuration hygiene, not a precedence fix. + expect( + await effectiveStatementTimeout(`?options=${encodeURIComponent("-c statement_timeout=0")}`), + ).toBe("10s"); + expect( + await effectiveStatementTimeout( + `?options=${encodeURIComponent("-c statement_timeout=900s")}`, + ), + ).toBe("10s"); + expect( + await effectiveStatementTimeout( + `?options=${encodeURIComponent("-c statement_timeout\\=0")}`, + ), + ).toBe("10s"); + + // Inherited environment cannot reach the selected policy either. + expect(await effectiveStatementTimeout("", { PGOPTIONS: "-c statement_timeout=0" })).toBe( + "10s", + ); + expect(await effectiveStatementTimeout("", { PGCONNECT_TIMEOUT: "0" })).toBe("10s"); + + // An unrelated parameter changes nothing. + expect(await effectiveStatementTimeout("?application_name=ekklesia")).toBe("10s"); + }); + }); + + it("refuses every rejected key at the shared boundary before a client is constructed", async () => { + await withIsolatedDatabase("timeout_boundary", async (database) => { + for (const query of [ + "?connectionTimeoutMillis=0", + "?connectionTimeoutMillis=900000", + "?connect_timeout=0", + "?query_timeout=0", + "?statement_timeout=0", + `?options=${encodeURIComponent("-c statement_timeout=0")}`, + ]) { + let captured: unknown; + try { + createPostgresMigrationDatabase(`${database.url}${query}`); + } catch (error) { + captured = error; + } + expect(captured, `expected ${query} rejected`).toBeInstanceOf( + PostgresMigrationDatabaseError, + ); + expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" }); + const publicText = `${String(captured)}\n${(captured as Error).stack ?? ""}`; + expect(publicText).not.toContain(database.name); + expect(publicText).not.toContain("127.0.0.1"); + } + + // The clean URL the commands actually use still works, end to end, with the real defaults. + const applied = await applyModuleMigrations({ + aggregate: aggregate({ + id: "people", + migrations: [migration("people", 0, { sql: "SELECT 1;", tag: "0000_ok" })], + }), + database: createPostgresMigrationDatabase(database.url), + }); + expect(applied.appliedCount).toBe(1); + }); + }); +}); diff --git a/docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md b/docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md index 33e92a3..14c54c6 100644 --- a/docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md +++ b/docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md @@ -144,15 +144,32 @@ lock-wait deadline and error codes, and the Pass 2 TLS/hostname/certificate poli as are Doctor's and the bootstrap CLI's own timeouts. No timeout environment variable or flag was added. -Explicit constructor fields do **not** win under the pinned driver. `pg@8.22.0` builds parameters as -`Object.assign({}, config, parse(config.connectionString))`, so the connection string overrides -passed options — verified directly: an explicit `statement_timeout: 10000` alongside -`?statement_timeout=0` produced a server-reported `statement_timeout` of `0`, while a clean URL -produced `10s`. A `DATABASE_URL` carrying `connect_timeout`, `query_timeout`, `statement_timeout`, -or an `options` value that sets `statement_timeout` is therefore refused with the existing fixed -`POSTGRES_MIGRATION_CONFIGURATION_INVALID`, before any client is constructed and without echoing the -URL, parameter, or value. The rejection is deliberately narrow — only settings that actually conflict -with the selected policy; unrelated parameters are untouched. +A connection string can reach those values, because `pg@8.22.0` builds parameters as +`Object.assign({}, config, parse(config.connectionString))` and `pg-connection-string` retains +*arbitrary* query keys. What each key actually does was measured against the installed driver, a +real blackhole listener, and a real PostgreSQL rather than inferred from that merge, and only one of +them genuinely weakens the policy: + +| Source | Measured effect on the selected policy | +| --- | --- | +| `?statement_timeout=0` / `=900000` | **Wins.** Server reported `0` (disabled) and `15min` against an explicit `statement_timeout: 10000`. The one real downgrade. | +| `?query_timeout=0` | Overrides the explicit field, but breaks the client rather than loosening it: the parsed value is the string `"0"`, which is truthy, so every query gets a 0 ms read timer and fails nondeterministically with `Query read timeout`. | +| `?connectionTimeoutMillis=0` / `=900000` | Pollutes `ConnectionParameters.connect_timeout` (10 → `0`/`900` s), but that field feeds only `getLibpqConnectionString()`, which the pure-JS client never calls. The **effective** connect timer is unchanged — an explicit 500 ms still expired at ~500 ms with either value. | +| `?connect_timeout=0` | No effect: `ConnectionParameters` recomputes it from `connectionTimeoutMillis`. | +| `?options=-c statement_timeout=…` (incl. a backslash-escaped spelling) | No effect: the named startup parameter wins. Server still reported `10s`. | +| inherited `PGOPTIONS=-c statement_timeout=0` | No effect. Server still reported `10s`. | +| inherited `PGCONNECT_TIMEOUT=0` | No effect: consulted only when `connectionTimeoutMillis` is `undefined`, which this boundary never leaves unset. | +| unrelated parameters (`application_name`, …) | No effect; deliberately untouched. | + +A `DATABASE_URL` carrying `statement_timeout`, `query_timeout`, `connectionTimeoutMillis`, +`connect_timeout`, or an `options` value naming `statement_timeout` is refused with the existing +fixed `POSTGRES_MIGRATION_CONFIGURATION_INVALID`, before any client is constructed and without +echoing the URL, parameter, or value. Only the first is a precedence fix; the rest are rejected as +**unsupported configuration and defense in depth** — no legitimate `DATABASE_URL` carries a driver +option, `connectionTimeoutMillis` would take effect on the native binding, and `query_timeout` +produces intermittent failures an operator would struggle to diagnose. Stating that honestly matters: +an earlier revision of this ADR claimed `connect_timeout` overrode the explicit field, which +measurement disproved. ### Each migration is one PostgreSQL-parsed batch diff --git a/docs/development/identity.md b/docs/development/identity.md index 91b145d..b3dc27d 100644 --- a/docs/development/identity.md +++ b/docs/development/identity.md @@ -85,11 +85,14 @@ pnpm doctor # includes Core's migration in its discovery/plan chec Both `migrations:plan` and `migrations:apply` connect with fixed, finite defaults — a 10-second connection deadline, a 10-second client query timeout, and a 10-second PostgreSQL `statement_timeout` — applied at the one shared construction boundary so no command can omit them. -A `DATABASE_URL` that tries to set `connect_timeout`, `query_timeout`, `statement_timeout`, or an -`options` value carrying `statement_timeout` is refused with a sanitized configuration error, -because the pinned driver lets the connection string override explicitly passed options. This bounds -each individual operation; it does **not** promise that a migration completes within ten seconds -across retries or cleanup. Migrations remain explicit operator work and never run at application +A `DATABASE_URL` that tries to set `statement_timeout`, `query_timeout`, `connectionTimeoutMillis`, +`connect_timeout`, or an `options` value naming `statement_timeout` is refused with a sanitized +configuration error. Only `statement_timeout` actually overrides the selected policy under the +pinned driver (measured: `?statement_timeout=0` made the server report `0`); the others are rejected +as unsupported configuration and defense in depth — `query_timeout` breaks the client rather than +loosening it, and `connectionTimeoutMillis` pollutes a driver field the pure-JS client never +consults. Unrelated parameters are untouched. This bounds each individual operation; it does **not** +promise that a migration completes within ten seconds across retries or cleanup. Migrations remain explicit operator work and never run at application startup. Each pending migration's SQL is sent to PostgreSQL as **one complete batch**, inside the existing