v0.26.8 feat(migration): v35 auto-RLS event trigger — new tables always secure (#612)

* feat(migration): v35 auto-RLS event trigger — new tables always secure Postgres event trigger that fires on every CREATE TABLE and auto-enables Row Level Security. Prevents the face_detections bug: tables created outside gbrain migrations (Baku, manual SQL, other apps sharing the same Supabase project) were silently unprotected until gbrain doctor caught it. This is the Supabase-recommended approach — no dashboard toggle exists. Migration v35 (auto_rls_event_trigger): - CREATE FUNCTION auto_enable_rls() — event trigger handler - CREATE EVENT TRIGGER auto_rls_on_create_table — fires on ddl_command_end - PGLite: no-op (no RLS engine, no event triggers) Tests (3 cases): - Event trigger exists after migration - New table automatically gets RLS enabled - auto_enable_rls function exists Closes the gap identified in production on 2026-05-04 when face_detections was found without RLS. * feat(migration): v35 — drop FORCE, public-only, bundle backfill, cover CTAS+SELECT INTO Apply the corrections surfaced by /plan-eng-review + /codex consult against the original PR #612. The trigger now matches v24/v29/schema.sql posture (ENABLE only, no FORCE), scopes to the public schema, and covers all three table-creation syntaxes Postgres reports. Bundles a one-time backfill of every existing public.* table without RLS, honoring doctor.ts's GBRAIN:RLS_EXEMPT regex and quoting identifiers via format('%I.%I'). Drops the EXCEPTION wrap inside the trigger so per-table failures abort the offending CREATE TABLE (loud rollback) rather than producing a silent permissive default. Drops the hand-rolled privilege pre-check — the runner already fails loud on permission errors and gates the version bump. Breaking change: operators with intentionally-RLS-off public tables must add the GBRAIN:RLS_EXEMPT comment before upgrading or the backfill will flip them on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Wintermute <wintermute@garrytan.com> Co-authored-by: Garry Tan <garrytan@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-07-27 22:15:33 +00:00 · 2026-05-04 17:31:32 -07:00
parent 058fe69575
commit 9c2dc4cd54
13 changed files with 705 additions and 44 deletions
@@ -154,4 +154,24 @@ describe('doctor command', () => {
    // requirement to write a real justification, not just the prefix.
    expect(rlsBlock).toMatch(/reason=/);
  });
+
+  // v0.26.7 — rls_event_trigger check (post-install drift detector for v35).
+  // Lives AFTER `// 6. Schema version` so the existing `// 5. RLS` slice
+  // tests stay intact (codex correction).
+  test('rls_event_trigger check exists, scoped after schema_version, healthy on (O,A) only', async () => {
+    const source = await Bun.file(new URL('../src/commands/doctor.ts', import.meta.url)).text();
+    const idx7 = source.indexOf('// 7. RLS event trigger');
+    const idx8 = source.indexOf('// 8. Embedding health');
+    expect(idx7).toBeGreaterThan(0);
+    expect(idx8).toBeGreaterThan(idx7);
+    const block = source.slice(idx7, idx8);
+    expect(block).toContain("name: 'rls_event_trigger'");
+    // Healthy set is origin (`O`) or always (`A`). `R` is replica-only and
+    // would not fire in normal sessions; `D` is disabled. Both are warn states.
+    expect(block).toMatch(/evtenabled\s*!==\s*'O'[\s\S]*?evtenabled\s*!==\s*'A'/);
+    // PGLite skip path is required (no event triggers there).
+    expect(block).toMatch(/engine\.kind\s*===\s*'pglite'/);
+    // Recovery command names the migration version explicitly.
+    expect(block).toContain('--force-retry 35');
+  });
 });
@@ -972,15 +972,25 @@ describeE2E('E2E: RLS Verification', () => {
    const conn = getConn();
    const tbl = `gbrain_rls_regression_${suffix}`;
    try {
-      await conn.unsafe(`CREATE TABLE public.${tbl} (id int)`);
-      // Make sure RLS is actually off; CREATE TABLE default is off but be explicit.
-      await conn.unsafe(`ALTER TABLE public.${tbl} DISABLE ROW LEVEL SECURITY`);
-
-      // Init (idempotent) so the CLI has a config to read.
+      // Init first so all migrations (including v35's auto-RLS event trigger
+      // and one-time backfill) are applied. AFTER migrations run, simulate
+      // the post-v35 escape route: operator drops the auto-RLS trigger
+      // (e.g. while debugging) and creates a public table without RLS.
+      // doctor's existing rls check must still flag it. The new
+      // rls_event_trigger check warns separately about the missing trigger.
      Bun.spawnSync({
        cmd: ['bun', 'run', 'src/cli.ts', 'init', '--non-interactive', '--url', process.env.DATABASE_URL!],
        cwd: cliCwd, env: cliEnv(), timeout: 15_000,
      });
+
+      // Drop the trigger so CREATE TABLE doesn't auto-enable RLS, then create
+      // the test table without RLS. ALTER TABLE … DISABLE is a belt-and-
+      // suspenders no-op in this path but matches what an operator would do
+      // if they had toggled RLS off manually after the trigger ran.
+      await conn.unsafe(`DROP EVENT TRIGGER IF EXISTS auto_rls_on_create_table`);
+      await conn.unsafe(`CREATE TABLE public.${tbl} (id int)`);
+      await conn.unsafe(`ALTER TABLE public.${tbl} DISABLE ROW LEVEL SECURITY`);
+
      const result = Bun.spawnSync({
        cmd: ['bun', 'run', 'src/cli.ts', 'doctor', '--json'],
        cwd: cliCwd, env: cliEnv(), timeout: 20_000,
@@ -995,6 +1005,11 @@ describeE2E('E2E: RLS Verification', () => {
      expect(result.exitCode).toBe(1);
    } finally {
      await conn.unsafe(`DROP TABLE IF EXISTS public.${tbl}`);
+      // Restore the trigger via a no-op v35 replay so subsequent tests in
+      // this file (which expect the post-init steady state) don't see drift.
+      const { MIGRATIONS } = await import('../../src/core/migrate.ts');
+      const v35sql = (MIGRATIONS.find(m => m.version === 35)?.sqlFor as any)?.postgres;
+      if (v35sql) await conn.unsafe(v35sql);
    }
  }, 60_000);

@@ -0,0 +1,261 @@
+/**
+ * E2E tests for migration v35: auto_rls_event_trigger.
+ *
+ * Verifies the event trigger auto-enables RLS on newly created public.* tables
+ * across CREATE TABLE / CREATE TABLE AS / SELECT INTO, plus the one-time
+ * backfill of existing public.* tables without RLS (modulo the GBRAIN:RLS_EXEMPT
+ * exemption that doctor honors). Postgres-only — PGLite has no RLS or event
+ * triggers; that no-op is asserted in test/migrate.test.ts.
+ *
+ * setupDB() runs db.initSchema() which applies all migrations including v35,
+ * so the trigger and the backfill have already executed by the time these
+ * tests start.
+ *
+ * Run: DATABASE_URL=... bun test test/e2e/migration-v35-auto-rls.test.ts
+ */
+
+import { describe, test, expect, beforeAll, afterAll } from 'bun:test';
+import { hasDatabase, setupDB, teardownDB, getConn, getEngine, runMigrationsUpTo } from './helpers.ts';
+import { MIGRATIONS, LATEST_VERSION } from '../../src/core/migrate.ts';
+
+const skip = !hasDatabase();
+const describeE2E = skip ? describe.skip : describe;
+
+if (skip) {
+  console.log('Skipping auto-RLS E2E tests (DATABASE_URL not set)');
+}
+
+// Migration v35 lives at index 34 (0-based) in MIGRATIONS.
+const v35 = MIGRATIONS.find(m => m.version === 35);
+const v35Sql = (v35?.sqlFor as any)?.postgres ?? '';
+
+describeE2E('migration v35: auto_rls_event_trigger', () => {
+  beforeAll(async () => {
+    await setupDB();
+    // setupDB() runs db.initSchema() (SCHEMA_SQL only, no migrations).
+    // Advance through every migration so v35 is actually installed.
+    await runMigrationsUpTo(getEngine(), LATEST_VERSION);
+  });
+
+  afterAll(async () => {
+    const conn = getConn();
+    // Clean up every artifact this file creates. Order matters because
+    // _test_v35_scope lives in a non-public schema we drop wholesale.
+    await conn.unsafe(`DROP TABLE IF EXISTS _test_auto_rls_check`);
+    await conn.unsafe(`DROP TABLE IF EXISTS _test_ctas`);
+    await conn.unsafe(`DROP TABLE IF EXISTS _test_select_into`);
+    await conn.unsafe(`DROP TABLE IF EXISTS _test_backfill_plain`);
+    await conn.unsafe(`DROP TABLE IF EXISTS _test_backfill_exempt`);
+    await conn.unsafe(`DROP TABLE IF EXISTS "_test_BackfillCamelCase"`);
+    await conn.unsafe(`DROP SCHEMA IF EXISTS _test_v35_scope CASCADE`);
+    await teardownDB();
+  });
+
+  test('event trigger exists', async () => {
+    const conn = getConn();
+    const triggers = await conn`
+      SELECT evtname FROM pg_event_trigger
+      WHERE evtname = 'auto_rls_on_create_table'
+    `;
+    expect(triggers.length).toBe(1);
+  });
+
+  test('new tables automatically get RLS enabled (CREATE TABLE)', async () => {
+    const conn = getConn();
+    await conn`CREATE TABLE _test_auto_rls_check (id serial PRIMARY KEY, val text)`;
+
+    const result = await conn`
+      SELECT rowsecurity FROM pg_tables
+      WHERE schemaname = 'public' AND tablename = '_test_auto_rls_check'
+    `;
+    expect(result.length).toBe(1);
+    expect(result[0].rowsecurity).toBe(true);
+  });
+
+  test('auto_enable_rls function exists', async () => {
+    const conn = getConn();
+    const funcs = await conn`
+      SELECT proname FROM pg_proc
+      WHERE proname = 'auto_enable_rls'
+    `;
+    expect(funcs.length).toBe(1);
+  });
+
+  test('FORCE RLS is NOT applied (D1: ENABLE only)', async () => {
+    // pg_class.relforcerowsecurity reflects FORCE; rowsecurity reflects ENABLE.
+    // v35 enables only — operators or future migrations can opt FORCE in
+    // explicitly per table if defense-in-depth is desired.
+    const conn = getConn();
+    const result = await conn`
+      SELECT relforcerowsecurity FROM pg_class
+      WHERE relname = '_test_auto_rls_check' AND relkind = 'r'
+    `;
+    expect(result.length).toBe(1);
+    expect(result[0].relforcerowsecurity).toBe(false);
+  });
+
+  test('CREATE TABLE AS triggers auto-RLS (D6)', async () => {
+    // CTAS is a distinct command_tag in Postgres ('CREATE TABLE AS'). The
+    // trigger's WHEN TAG list covers it explicitly.
+    const conn = getConn();
+    await conn`CREATE TABLE _test_ctas AS SELECT 1 AS id, 'hello'::text AS val`;
+
+    const result = await conn`
+      SELECT rowsecurity FROM pg_tables
+      WHERE schemaname = 'public' AND tablename = '_test_ctas'
+    `;
+    expect(result.length).toBe(1);
+    expect(result[0].rowsecurity).toBe(true);
+  });
+
+  test('SELECT INTO triggers auto-RLS (D6)', async () => {
+    // SELECT INTO is the older synonym for CTAS. Postgres tags it 'SELECT INTO'.
+    const conn = getConn();
+    await conn`SELECT 1 AS id, 'world'::text AS val INTO _test_select_into`;
+
+    const result = await conn`
+      SELECT rowsecurity FROM pg_tables
+      WHERE schemaname = 'public' AND tablename = '_test_select_into'
+    `;
+    expect(result.length).toBe(1);
+    expect(result[0].rowsecurity).toBe(true);
+  });
+
+  test('non-public schemas are not touched (D2)', async () => {
+    // Build a private schema and a table inside. The trigger filters
+    // schema_name = 'public' so this table should remain RLS-off.
+    const conn = getConn();
+    await conn`CREATE SCHEMA IF NOT EXISTS _test_v35_scope`;
+    await conn`CREATE TABLE _test_v35_scope.private_tbl (id int)`;
+
+    const result = await conn`
+      SELECT relrowsecurity FROM pg_class c
+      JOIN pg_namespace n ON n.oid = c.relnamespace
+      WHERE n.nspname = '_test_v35_scope' AND c.relname = 'private_tbl'
+    `;
+    expect(result.length).toBe(1);
+    expect(result[0].relrowsecurity).toBe(false);
+  });
+
+  test('replay idempotency: re-running migration leaves exactly one trigger', async () => {
+    const conn = getConn();
+    expect(v35Sql.length).toBeGreaterThan(0);
+    // Re-execute the entire v35 SQL. The DROP EVENT TRIGGER IF EXISTS +
+    // CREATE EVENT TRIGGER pattern must be a clean round-trip. The backfill
+    // DO block runs again too, but is a no-op since RLS is now on everywhere.
+    await conn.unsafe(v35Sql);
+
+    const triggers = await conn`
+      SELECT evtname FROM pg_event_trigger
+      WHERE evtname = 'auto_rls_on_create_table'
+    `;
+    expect(triggers.length).toBe(1);
+
+    const funcs = await conn`SELECT proname FROM pg_proc WHERE proname = 'auto_enable_rls'`;
+    expect(funcs.length).toBe(1);
+  });
+
+  test('backfill enables RLS on pre-existing public.* tables', async () => {
+    const conn = getConn();
+    // Temporarily drop the trigger so we can create a table WITHOUT RLS,
+    // simulating a pre-v35 row.
+    await conn`DROP EVENT TRIGGER IF EXISTS auto_rls_on_create_table`;
+    try {
+      await conn`CREATE TABLE _test_backfill_plain (id serial PRIMARY KEY)`;
+      // Belt-and-suspenders: explicitly disable RLS on this fresh table.
+      await conn`ALTER TABLE _test_backfill_plain DISABLE ROW LEVEL SECURITY`;
+
+      const before = await conn`
+        SELECT rowsecurity FROM pg_tables WHERE tablename = '_test_backfill_plain'
+      `;
+      expect(before[0].rowsecurity).toBe(false);
+
+      // Re-run v35: trigger comes back AND the backfill DO block flips this
+      // table to RLS-on (no exempt comment, schema is public, relkind='r').
+      await conn.unsafe(v35Sql);
+
+      const after = await conn`
+        SELECT rowsecurity FROM pg_tables WHERE tablename = '_test_backfill_plain'
+      `;
+      expect(after[0].rowsecurity).toBe(true);
+    } finally {
+      // Make sure the trigger is restored even if assertions throw.
+      const triggers = await conn`
+        SELECT evtname FROM pg_event_trigger WHERE evtname = 'auto_rls_on_create_table'
+      `;
+      if (triggers.length === 0) {
+        await conn.unsafe(v35Sql);
+      }
+    }
+  });
+
+  test('backfill respects GBRAIN:RLS_EXEMPT comment (matches doctor regex)', async () => {
+    const conn = getConn();
+    await conn`DROP EVENT TRIGGER IF EXISTS auto_rls_on_create_table`;
+    try {
+      await conn`CREATE TABLE _test_backfill_exempt (id serial PRIMARY KEY)`;
+      await conn`ALTER TABLE _test_backfill_exempt DISABLE ROW LEVEL SECURITY`;
+      // Comment must match doctor.ts EXEMPT_RE: /^GBRAIN:RLS_EXEMPT\s+reason=\S.{3,}/
+      // — "test exempt" is 11 chars after `reason=`, well over the .{3,} floor.
+      await conn`COMMENT ON TABLE _test_backfill_exempt IS 'GBRAIN:RLS_EXEMPT reason=test exempt'`;
+
+      // Re-run the migration. Backfill should skip this row.
+      await conn.unsafe(v35Sql);
+
+      const after = await conn`
+        SELECT rowsecurity FROM pg_tables WHERE tablename = '_test_backfill_exempt'
+      `;
+      expect(after[0].rowsecurity).toBe(false);
+    } finally {
+      const triggers = await conn`
+        SELECT evtname FROM pg_event_trigger WHERE evtname = 'auto_rls_on_create_table'
+      `;
+      if (triggers.length === 0) {
+        await conn.unsafe(v35Sql);
+      }
+    }
+  });
+
+  test('backfill quotes mixed-case identifiers safely (%I.%I)', async () => {
+    const conn = getConn();
+    await conn`DROP EVENT TRIGGER IF EXISTS auto_rls_on_create_table`;
+    try {
+      // Mixed-case table names require double-quoting in DDL. If the backfill
+      // used %s with raw concat, ALTER TABLE public._test_BackfillCamelCase
+      // would fail with "relation does not exist" because Postgres folds
+      // unquoted identifiers to lowercase.
+      await conn`CREATE TABLE "_test_BackfillCamelCase" (id serial PRIMARY KEY)`;
+      await conn`ALTER TABLE "_test_BackfillCamelCase" DISABLE ROW LEVEL SECURITY`;
+
+      await conn.unsafe(v35Sql);
+
+      const after = await conn`
+        SELECT relrowsecurity FROM pg_class
+        WHERE relname = '_test_BackfillCamelCase' AND relkind = 'r'
+      `;
+      expect(after.length).toBe(1);
+      expect(after[0].relrowsecurity).toBe(true);
+    } finally {
+      const triggers = await conn`
+        SELECT evtname FROM pg_event_trigger WHERE evtname = 'auto_rls_on_create_table'
+      `;
+      if (triggers.length === 0) {
+        await conn.unsafe(v35Sql);
+      }
+    }
+  });
+
+  test('regression guard: trigger function body does NOT contain EXCEPTION WHEN OTHERS', async () => {
+    // Codex correctly identified that wrapping the per-table EXECUTE in
+    // BEGIN…EXCEPTION WHEN OTHERS… would convert a transactional rollback
+    // (loud) into a silent permissive default (quiet). Pin that by reading
+    // the function body from pg_proc and grepping it.
+    const conn = getConn();
+    const rows = await conn`
+      SELECT prosrc FROM pg_proc WHERE proname = 'auto_enable_rls'
+    `;
+    expect(rows.length).toBe(1);
+    const body = rows[0].prosrc as string;
+    expect(body.toUpperCase()).not.toContain('EXCEPTION WHEN OTHERS');
+  });
+});
@@ -354,6 +354,86 @@ describe('migration v24 — rls_backfill_missing_tables', () => {
  });
 });

+// ─────────────────────────────────────────────────────────────────
+// v0.26.7 — migration v35 structural guards (auto-RLS event trigger)
+// ─────────────────────────────────────────────────────────────────
+//
+// The PR review caught that the original v35 had three correctness issues:
+//   - FORCE ROW LEVEL SECURITY locked out non-BYPASSRLS table owners.
+//   - Trigger fired on Supabase-managed schemas (auth/storage/realtime/...).
+//   - EXCEPTION WHEN OTHERS would silently swallow per-table failures and
+//     replace a transactional rollback (loud) with a permissive default (quiet).
+// These tests pin the corrected shape so a future revert can't reintroduce
+// the original bugs.
+describe('migration v35 — auto_rls_event_trigger structural guards', () => {
+  test('exists with the expected name and SQL shape', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    expect(v35).toBeDefined();
+    expect(v35?.name).toBe('auto_rls_event_trigger');
+    expect((v35?.sqlFor as any)?.postgres?.length).toBeGreaterThan(0);
+  });
+
+  test('uses a PGLite no-op override (no event trigger support on PGLite)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    expect(v35?.sqlFor?.pglite).toBe('');
+  });
+
+  test('does NOT issue FORCE ROW LEVEL SECURITY (D1: ENABLE only)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    expect(sql).not.toMatch(/FORCE\s+ROW\s+LEVEL\s+SECURITY/i);
+    expect(sql).toMatch(/ENABLE\s+ROW\s+LEVEL\s+SECURITY/i);
+  });
+
+  test('trigger function is scoped to schema_name = public (D2)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    expect(sql).toMatch(/schema_name\s*=\s*'public'/);
+  });
+
+  test('WHEN TAG covers CREATE TABLE, CREATE TABLE AS, and SELECT INTO (D6)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    expect(sql).toMatch(/WHEN\s+TAG\s+IN\s*\([^)]*'CREATE TABLE'[^)]*\)/i);
+    expect(sql).toMatch(/'CREATE TABLE AS'/);
+    expect(sql).toMatch(/'SELECT INTO'/);
+  });
+
+  test('does NOT contain EXCEPTION WHEN OTHERS inside the trigger function (D5 reversed)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    // ddl_command_end fires inside the DDL transaction, so a failed ALTER
+    // aborts the offending CREATE TABLE — that's the security guarantee.
+    // Wrapping in EXCEPTION WHEN OTHERS would convert that loud rollback
+    // into a silent permissive default. Pin the absence.
+    expect(sql.toUpperCase()).not.toContain('EXCEPTION WHEN OTHERS');
+  });
+
+  test('backfill block uses %I.%I identifier quoting (codex correction)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    // The backfill iterates pg_class and ALTERs each non-exempt RLS-off public
+    // table. Mixed-case identifiers require %I quoting; raw concat would break.
+    expect(sql).toMatch(/format\(\s*'ALTER TABLE %I\.%I/);
+  });
+
+  test('backfill exemption regex matches the doctor.ts contract', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    // doctor.ts:418 EXEMPT_RE = /^GBRAIN:RLS_EXEMPT\s+reason=\S.{3,}/
+    // The plpgsql side must use the same pattern (via ~) so the two surfaces
+    // honor identical exemptions.
+    expect(sql).toMatch(/'\^GBRAIN:RLS_EXEMPT\\s\+reason=\\S\.\{3,\}'/);
+  });
+
+  test('backfill is gated on rolbypassrls (matches v24 posture)', () => {
+    const v35 = MIGRATIONS.find(m => m.version === 35);
+    const sql = ((v35?.sqlFor as any)?.postgres ?? '') as string;
+    expect(sql).toMatch(/rolbypassrls/);
+    expect(sql).toMatch(/RAISE\s+EXCEPTION/i);
+  });
+});
+
 // ─────────────────────────────────────────────────────────────────
 // REGRESSION TESTS — migrations v8 + v9 perf on duplicate-heavy tables
 // ─────────────────────────────────────────────────────────────────