diff --git a/bunfig.toml b/bunfig.toml index 3dd8ebb9a..4ba831f7e 100644 --- a/bunfig.toml +++ b/bunfig.toml @@ -18,4 +18,15 @@ timeout = 60_000 # runs, so audit-emitting code paths (content-sanity, shell-audit, etc.) # can't leak fixture events into the operator's real ~/.gbrain/audit/. See # test/helpers/audit-dir-preload.ts for the full rationale. -preload = ["./test/helpers/legacy-embedding-preload.ts", "./test/helpers/audit-dir-preload.ts"] +# +# #3485: vet DATABASE_URL before any test file loads. The guard used to be +# reachable only from setupDB(), so the 17 files that build a PostgresEngine +# straight from process.env.DATABASE_URL never ran it — several of which +# TRUNCATE/DROP/ALTER whatever answers. A process-wide precondition covers +# them all, and covers the next such file automatically. See +# test/helpers/db-guard-preload.ts. +preload = [ + "./test/helpers/legacy-embedding-preload.ts", + "./test/helpers/audit-dir-preload.ts", + "./test/helpers/db-guard-preload.ts", +] diff --git a/test/e2e/helpers.ts b/test/e2e/helpers.ts index 0d0fe762b..7277394d9 100644 --- a/test/e2e/helpers.ts +++ b/test/e2e/helpers.ts @@ -67,38 +67,14 @@ export function hasDatabase(): boolean { } /** - * Production guard: setupDB() TRUNCATEs every data table on whatever - * DATABASE_URL points at, and run-e2e.sh deliberately preserves an exported - * DATABASE_URL — so a developer with a production URL in their environment - * would wipe their real brain by running the suite. Refuse unless the - * database name identifies itself as a test database ("test" as a word - * segment, e.g. gbrain_test — the CI/.env.testing.example convention), or - * the operator explicitly opts the exact name in via GBRAIN_E2E_ALLOW_DB. - * - * Exported for unit testing; pure — no connection is made. + * The DATABASE_URL production guard moved to `test/helpers/db-guard.ts` in + * #3485 so that files which never import these helpers can reach it — and so + * `bunfig.toml`'s preload can enforce it process-wide instead of relying on + * each file to remember. Re-exported here for the existing callers and for + * `test/e2e/db-guard.test.ts`. */ -export function assertSafeE2eDatabaseUrl( - url: string, - env: Record = process.env, -): void { - let dbName: string; - try { - dbName = decodeURIComponent(new URL(url).pathname.replace(/^\//, '')); - } catch { - throw new Error(`E2E guard: DATABASE_URL is not a parseable URL; refusing to run destructive setup.`); - } - if (!dbName) { - throw new Error(`E2E guard: DATABASE_URL has no database name; refusing to run destructive setup.`); - } - if (/(^|[_-])test([_-]|$)/i.test(dbName)) return; - if (env.GBRAIN_E2E_ALLOW_DB && env.GBRAIN_E2E_ALLOW_DB === dbName) return; - throw new Error( - `E2E guard: database "${dbName}" does not look like a test database ` + - `(expected "test" as a name segment, e.g. gbrain_test). setupDB() would ` + - `TRUNCATE every data table in it. If this is intentional, set ` + - `GBRAIN_E2E_ALLOW_DB=${dbName} to opt in explicitly.`, - ); -} +import { assertSafeE2eDatabaseUrl } from '../helpers/db-guard.ts'; +export { assertSafeE2eDatabaseUrl }; /** * Connect to DB, run schema init, truncate all tables. diff --git a/test/helpers/db-guard-preload.test.ts b/test/helpers/db-guard-preload.test.ts new file mode 100644 index 000000000..c8d925971 --- /dev/null +++ b/test/helpers/db-guard-preload.test.ts @@ -0,0 +1,57 @@ +/** + * Pins the #3485 fix in place. + * + * The vulnerability was never that the guard logic was wrong — it was that + * the guard was only *reachable* from `setupDB()`, so the 17 test files that + * build a PostgresEngine straight from `process.env.DATABASE_URL` never ran + * it. The fix is a `bunfig.toml` preload, and a preload is exactly the kind + * of thing a future refactor drops without noticing, because nothing imports + * it. Hence this test: it asserts the wiring, not the logic. + * + * `test/e2e/db-guard.test.ts` covers the guard's behavior. + */ +import { describe, test, expect } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { assertSafeE2eDatabaseUrl } from './db-guard.ts'; + +const REPO_ROOT = resolve(import.meta.dir, '../..'); + +describe('#3485: DATABASE_URL guard is enforced process-wide', () => { + test('bunfig.toml preloads the db guard', () => { + const bunfig = readFileSync(resolve(REPO_ROOT, 'bunfig.toml'), 'utf-8'); + // Tolerant of formatting (single-line or multi-line array), strict about presence. + expect(bunfig).toContain('./test/helpers/db-guard-preload.ts'); + }); + + test('the preload actually calls the guard', () => { + const preload = readFileSync(resolve(REPO_ROOT, 'test/helpers/db-guard-preload.ts'), 'utf-8'); + expect(preload).toContain('assertSafeE2eDatabaseUrl'); + // Must be conditional on DATABASE_URL — an unconditional call would break + // every no-DB run. + expect(preload).toContain('process.env.DATABASE_URL'); + }); + + test('the guard still rejects a production-shaped database name', () => { + // Sanity: if this ever stops throwing, the preload is a no-op and the + // wiring assertions above are worthless. + expect(() => + assertSafeE2eDatabaseUrl('postgresql://u:p@db.example.com:5432/gbrain', {}), + ).toThrow(/does not look like a test database/); + }); + + test('and accepts the CI/.env.testing convention', () => { + expect(() => + assertSafeE2eDatabaseUrl('postgresql://postgres:postgres@localhost:5433/gbrain_test', {}), + ).not.toThrow(); + }); + + test('this very process passed the guard', () => { + // If DATABASE_URL is set at all, the preload ran before this file loaded + // and did not throw — otherwise the process would have aborted. Assert the + // invariant explicitly so the guarantee is visible in test output. + if (process.env.DATABASE_URL) { + expect(() => assertSafeE2eDatabaseUrl(process.env.DATABASE_URL!, process.env)).not.toThrow(); + } + }); +}); diff --git a/test/helpers/db-guard-preload.ts b/test/helpers/db-guard-preload.ts new file mode 100644 index 000000000..34780b08f --- /dev/null +++ b/test/helpers/db-guard-preload.ts @@ -0,0 +1,55 @@ +/** + * Pre-test setup: vet `DATABASE_URL` ONCE, before any test file loads, and + * refuse to run the suite at all when it points at something that isn't + * name-shaped like a test database. + * + * Why this exists (#3485): `assertSafeE2eDatabaseUrl` was reachable only + * from `setupDB()` in `test/e2e/helpers.ts`. 17 test files build a + * `PostgresEngine` directly from `process.env.DATABASE_URL` and never call + * it, so for those files the check simply never executed. Several then run + * genuinely destructive SQL against whatever answered: + * + * test/e2e/postgres-bootstrap.test.ts TRUNCATE ... CASCADE, + * DROP TABLE sources CASCADE, + * ALTER TABLE pages DROP COLUMN + * test/phantom-redirect-engine-parity.test.ts TRUNCATE facts, DELETE FROM pages + * test/e2e/multimodal-postgres.test.ts unscoped DELETE from 3 tables + * test/e2e/embedding-column-postgres.test.ts unscoped DELETE FROM content_chunks + * ... plus the eval-capture / eval-contradictions / facts-fence / + * mcp-budget files, and every file that calls initSchema() and so + * applies migrations to a brain that never asked for them. + * + * Two things made this sharper than a latent hazard. `gbrain init` writes + * DATABASE_URL into `~/.gbrain/.env`, so a developer's real brain is a + * plausible value. And `test/phantom-redirect-engine-parity.test.ts` sits in + * `test/`, not `test/e2e/` — `scripts/test-shard.sh`'s collection excludes + * `test/e2e/*` and `*.serial.test.ts` but not top-level `test/*`, so a plain + * `bun test` picks it up with none of the E2E ritual. + * + * Fix shape: a process-wide precondition instead of a per-file call. This + * covers all 17 files, needs no edit to any of them, and — the reason it is + * the right seam — automatically covers the next file someone writes. A + * per-file `assertSafeE2eDatabaseUrl()` line is exactly the ritual that was + * already being skipped. Same mechanism #2823 used for `GBRAIN_AUDIT_DIR`. + * + * No DATABASE_URL means no risk and no-op: the DB-backed suites all skip + * themselves in that case. + * + * Imported by `bunfig.toml` via + * `preload = [..., "./test/helpers/db-guard-preload.ts"]`. + */ +import { assertSafeE2eDatabaseUrl } from './db-guard.ts'; + +if (process.env.DATABASE_URL) { + // Throwing from a preload aborts the whole `bun test` process, which is the + // intent — a wrong DATABASE_URL is not something to warn about and proceed + // past, because the first beforeAll that runs may already have destroyed data. + assertSafeE2eDatabaseUrl( + process.env.DATABASE_URL, + process.env, + 'the DB-backed test files TRUNCATE, DELETE, DROP TABLE and apply migrations against it', + ); + if (process.env.GBRAIN_DEBUG_PRELOAD === '1') { + console.error('[db-guard-preload] DATABASE_URL accepted'); + } +} diff --git a/test/helpers/db-guard.ts b/test/helpers/db-guard.ts new file mode 100644 index 000000000..87a2e9444 --- /dev/null +++ b/test/helpers/db-guard.ts @@ -0,0 +1,54 @@ +/** + * The DATABASE_URL production guard — single source of truth. + * + * Lives here rather than in `test/e2e/helpers.ts` (#3485) because the guard + * is needed by files that never import those helpers. `assertSafeE2eDatabaseUrl` + * used to be reachable only from `setupDB()`, so the 17 test files that build + * a `PostgresEngine` straight from `process.env.DATABASE_URL` never ran it — + * and several of them issue `TRUNCATE ... CASCADE`, unscoped `DELETE FROM + * pages`, `DROP TABLE`, and `ALTER TABLE ... DROP COLUMN`. `gbrain init` + * writes DATABASE_URL into `~/.gbrain/.env`, so a developer with a real brain + * in their environment was one `bun test` away from losing it. + * + * Enforcement is `test/helpers/db-guard-preload.ts`, wired into + * `bunfig.toml`'s `preload`, which calls this once before any test file + * loads. That is deliberately the same mechanism #2823 used to stop tests + * leaking into the operator's real `~/.gbrain/audit/`: a process-wide + * precondition beats a per-file ritual nobody remembers to perform. + * + * Pure — makes no connection — so it is cheap to call redundantly and safe + * to unit test. + */ + +/** + * Refuse to proceed unless the database name identifies itself as a test + * database ("test" as a word segment, e.g. `gbrain_test` — the + * CI/.env.testing.example convention), or the operator explicitly opts the + * exact name in via `GBRAIN_E2E_ALLOW_DB`. + * + * @param url the DATABASE_URL to vet + * @param env env source, injectable for tests + * @param what what would happen if we proceeded, for the error message + */ +export function assertSafeE2eDatabaseUrl( + url: string, + env: Record = process.env, + what = 'setupDB() would TRUNCATE every data table in it', +): void { + let dbName: string; + try { + dbName = decodeURIComponent(new URL(url).pathname.replace(/^\//, '')); + } catch { + throw new Error(`E2E guard: DATABASE_URL is not a parseable URL; refusing to run destructive setup.`); + } + if (!dbName) { + throw new Error(`E2E guard: DATABASE_URL has no database name; refusing to run destructive setup.`); + } + if (/(^|[_-])test([_-]|$)/i.test(dbName)) return; + if (env.GBRAIN_E2E_ALLOW_DB && env.GBRAIN_E2E_ALLOW_DB === dbName) return; + throw new Error( + `E2E guard: database "${dbName}" does not look like a test database ` + + `(expected "test" as a name segment, e.g. gbrain_test). ${what}. ` + + `If this is intentional, set GBRAIN_E2E_ALLOW_DB=${dbName} to opt in explicitly.`, + ); +}