From dd6241489fd08ce92f8a2fb94e656320caf8f475 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 22 Apr 2026 16:48:12 -0700 Subject: [PATCH] fix(v0.18.1): address codex review (orchestrator wiring + fail-closed + identifier escape) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes from `/codex` review of the merged diff: 1. HIGH — wire migration v24 into the `gbrain apply-migrations` upgrade path. Without an orchestrator entry, `gbrain upgrade`'s post-upgrade step runs `apply-migrations --yes`, which walks the registry in `src/commands/migrations/index.ts`. The registry stopped at v0_18_0, so v24 never fired on upgrade (connectEngine and doctor do not call initSchema). New `v0_18_1.ts` orchestrator mirrors v0.18.0's Phase A: shells out to `gbrain init --migrate-only`, which triggers initSchema → runMigrations → v24 applies. Registered in the migrations array. 2. HIGH — fail loudly when v24 runs under a non-BYPASSRLS role instead of RAISE WARNING-then-silently-bumping-version. The runner at migrate.ts:773 unconditionally calls `setConfig('version', String(m.version))` when a migration completes without throwing, so a WARNING-and-continue path would permanently lock the backfill out: schema_version=24 on the next run means `m.version > current` is false and v24 is skipped forever, even after the role gets BYPASSRLS. Changed `RAISE WARNING` → `RAISE EXCEPTION` so the transaction aborts, schema_version stays at 23, and a subsequent initSchema retries cleanly after the role is fixed. Test asserts the SQL uses EXCEPTION and does not use WARNING. 3. MEDIUM — escape double-quote characters in the remediation SQL output. doctor.ts was building `ALTER TABLE "public"."${n}"` with `n` un-escaped, so a pathological table name containing a literal `"` would break out of the quoted identifier and produce invalid copy-paste SQL. Double the `"` before interpolating, matching Postgres quoted-identifier escaping rules. Extremely rare in practice, cheap to get right. 4. LOW — CHANGELOG cleanup: corrected the upgrade-behavior claim (v24 runs via `apply-migrations --yes` through the new orchestrator, not during `gbrain doctor`) and split the "tables with RLS" row into two metrics (21 base-schema tables + 2 migration-only budget_* tables = 23 managed total, all covered). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 17 +++++--- src/commands/doctor.ts | 7 ++- src/commands/migrations/index.ts | 2 + src/commands/migrations/v0_18_1.ts | 69 ++++++++++++++++++++++++++++++ src/core/migrate.ts | 8 +++- test/migrate.test.ts | 13 +++++- 6 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 src/commands/migrations/v0_18_1.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f6c43d81d..a99dbda93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ All notable changes to GBrain will be documented in this file. A follow-up to v0.18.0 that closes a latent security gap the multi-source work didn't touch. `gbrain` itself had been quietly shipping 10 public tables without RLS for months: `access_tokens`, `mcp_request_log`, `minion_inbox`, `minion_attachments`, `subagent_messages`, `subagent_tool_executions`, `subagent_rate_leases`, `gbrain_cycle_locks`, `budget_ledger`, `budget_reservations`. On Supabase, every one of those was reachable by the anon key. `access_tokens` and the subagent conversation history tables are the ones that stand out. -This release closes the gap on three fronts at once: the check gets widened (so future misses surface), the base schema gets the missing 8 `ENABLE RLS` statements for tables it tracks (so fresh installs are secure), and new schema migration v24 `rls_backfill_missing_tables` enables RLS on all 10 of the affected tables in existing brains automatically when `gbrain doctor` or `gbrain apply-migrations` runs. The sources + file_migration_ledger tables introduced in v0.18.0 already had RLS from day one. +This release closes the gap on three fronts at once: the check gets widened (so future misses surface), the base schema gets the missing 8 `ENABLE RLS` statements for tables it tracks (so fresh installs are secure), and new schema migration v24 `rls_backfill_missing_tables` enables RLS on all 10 of the affected tables in existing brains automatically. The v0.18.1 orchestrator (`src/commands/migrations/v0_18_1.ts`) invokes `gbrain init --migrate-only` during `gbrain apply-migrations --yes`, which is what `gbrain post-upgrade` calls; v24 runs there, not inside `gbrain doctor` itself. The `sources` and `file_migration_ledger` tables introduced in v0.18.0 already had RLS from day one. The severity also upgrades from `warn` to `fail`. Missing RLS is a security issue, not a suggestion. `gbrain doctor` will exit 1 when any public table is missing RLS. If you wrap `gbrain doctor` in a cron or CI health check, expect it to turn red if anything is still open. @@ -22,8 +22,9 @@ Measured against v0.18.0. | Metric | BEFORE v0.18.1 | AFTER v0.18.1 | Δ | |--------|----------------|---------------|---| | Tables in `public` covered by the doctor RLS check | 10 (hardcoded list) | all (every `pg_tables` row) | +100% coverage | -| gbrain-managed public tables with RLS on fresh install | 13 of 21 | 21 of 21 | +8 in schema | -| Tables backfilled by migration v24 on existing brains | 0 | 10 | budget_ledger + budget_reservations covered too | +| gbrain-managed public tables with RLS after full migration chain | 13 of 23 | 23 of 23 | +10 total | +| — of which added in base schema v0.18.1 | 13 of 21 | 21 of 21 | +8 in schema | +| — of which added via migration v24 (budget_ledger + budget_reservations, still migration-only) | 0 | 10 | migration-only tables now covered | | `gbrain doctor` severity for missing RLS | warn (exit 0) | fail (exit 1) | breaking | | Escape hatch for intentional anon-readable tables | None (silent misses) | `GBRAIN:RLS_EXEMPT reason=...` pg comment | new capability | | Identifier-safe remediation SQL (hyphens, reserved words) | No (`ALTER TABLE public.`) | Yes (`ALTER TABLE "public".""`) | correctness | @@ -42,9 +43,10 @@ Credit: Garry's OpenClaw for the original PR widening the check (#336). Codex fo ## To take advantage of v0.18.1 -`gbrain upgrade` should do this automatically. Migration v24 runs on the next -`initSchema()` call, which happens on every CLI or long-running command startup. If -`gbrain doctor` warns about a partial migration or still reports missing RLS: +`gbrain upgrade` should do this automatically. It runs `gbrain post-upgrade`, +which calls `gbrain apply-migrations --yes`, which runs the v0.18.1 orchestrator +(`gbrain init --migrate-only` → schema migration v24 applied). If `gbrain doctor` +still reports missing RLS after upgrade: 1. **Apply migrations manually:** ```bash @@ -74,7 +76,8 @@ Credit: Garry's OpenClaw for the original PR widening the check (#336). Codex fo ### Itemized changes - **Schema fix (fresh installs):** `src/schema.sql` now enables RLS on all 21 public tables in the base schema (13 before v0.18.1, +8 added here: `access_tokens`, `mcp_request_log`, `minion_inbox`, `minion_attachments`, `subagent_messages`, `subagent_tool_executions`, `subagent_rate_leases`, `gbrain_cycle_locks`). `src/core/schema-embedded.ts` regenerated. -- **Schema migration (existing installs):** New migration `v24 rls_backfill_missing_tables` in `src/core/migrate.ts` enables RLS on all 10 affected tables idempotently (the 8 above plus `budget_ledger` and `budget_reservations`, which remain migration-only per v12). Gated on `rolbypassrls` — if the current role does not hold BYPASSRLS, the migration raises a warning and skips, matching the pattern used by the base schema's RLS block. Numbered v24 to slot after v0.18.0's v20-v23 sources-migration wave. +- **Schema migration (existing installs):** New migration `v24 rls_backfill_missing_tables` in `src/core/migrate.ts` enables RLS on all 10 affected tables idempotently (the 8 above plus `budget_ledger` and `budget_reservations`, which remain migration-only per v12). Gated on `rolbypassrls`; if the current role does not hold BYPASSRLS, the migration `RAISE EXCEPTION`s and aborts so `schema_version` stays at 23. Next `initSchema` call after switching to a bypass role retries cleanly. Numbered v24 to slot after v0.18.0's v20-v23 sources-migration wave. +- **Upgrade orchestrator:** New `src/commands/migrations/v0_18_1.ts` wires v24 into the `gbrain apply-migrations --yes` path via `gbrain init --migrate-only` (mirrors v0.18.0's Phase A). Without this, `gbrain doctor` and `connectEngine()` never call `initSchema()`, so v24 would sit in the registry but never apply on upgrade. - **Doctor check widened:** `src/commands/doctor.ts` RLS check now scans every public table from `pg_tables`, not a hardcoded 10-name allowlist. Severity upgraded `warn → fail`. Success message shows table count. Failure message includes per-table quoted `ALTER TABLE "public"."" ENABLE ROW LEVEL SECURITY;` remediation SQL. - **Escape hatch — "write it in blood":** Doctor reads `obj_description` for each non-RLS public table. Tables whose comment matches `^GBRAIN:RLS_EXEMPT\s+reason=\S.{3,}` count as explicitly exempt. Exempt tables are enumerated by name on every successful doctor run so the exemption list never goes invisible. No CLI subcommand — deliberate friction; operators must set the comment in psql. - **PGLite skip:** PGLite is embedded and single-user with no PostgREST; the RLS check now skips on PGLite with an explicit `ok` message ("Skipped — no PostgREST exposure, RLS not applicable") instead of the misleading `warn` it emitted before. Partial polish: pgvector, jsonb_integrity, and markdown_body_completeness checks still hit the same `getConnection()` throw → warn pattern on PGLite. Separate follow-up. diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index f1778f7ee..ddd2542d6 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -341,8 +341,13 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo }); } else { const names = gaps.join(', '); + // Double-escape " inside identifiers so a pathological table name + // like `weird"table` renders as `"weird""table"` in the remediation + // SQL (matches how Postgres parses quoted identifiers). Doubling + // any existing " is the minimum needed to keep the output valid + // copy-paste SQL. Extremely rare in practice but cheap to get right. const fixes = gaps - .map(n => `ALTER TABLE "public"."${n}" ENABLE ROW LEVEL SECURITY;`) + .map(n => `ALTER TABLE "public"."${n.replace(/"/g, '""')}" ENABLE ROW LEVEL SECURITY;`) .join(' '); const exemptInfo = exempt.length > 0 ? ` (${exempt.length} other table(s) explicitly exempt.)` diff --git a/src/commands/migrations/index.ts b/src/commands/migrations/index.ts index 8bf2d6b95..d40e0b84a 100644 --- a/src/commands/migrations/index.ts +++ b/src/commands/migrations/index.ts @@ -19,6 +19,7 @@ import { v0_13_1 } from './v0_13_1.ts'; import { v0_14_0 } from './v0_14_0.ts'; import { v0_16_0 } from './v0_16_0.ts'; import { v0_18_0 } from './v0_18_0.ts'; +import { v0_18_1 } from './v0_18_1.ts'; export const migrations: Migration[] = [ v0_11_0, @@ -29,6 +30,7 @@ export const migrations: Migration[] = [ v0_14_0, v0_16_0, v0_18_0, + v0_18_1, ]; /** Look up a migration by exact version string. */ diff --git a/src/commands/migrations/v0_18_1.ts b/src/commands/migrations/v0_18_1.ts new file mode 100644 index 000000000..231f95158 --- /dev/null +++ b/src/commands/migrations/v0_18_1.ts @@ -0,0 +1,69 @@ +/** + * v0.18.1 migration orchestrator — RLS hardening. + * + * v0.18.1 ships one new schema migration: v24 `rls_backfill_missing_tables`. + * It enables Row Level Security on 10 gbrain-managed public tables that + * shipped without it: access_tokens, mcp_request_log, minion_inbox, + * minion_attachments, subagent_messages, subagent_tool_executions, + * subagent_rate_leases, gbrain_cycle_locks, budget_ledger, budget_reservations. + * + * Phase structure mirrors v0.18.0: + * A. Schema — `gbrain init --migrate-only` runs the migration chain, + * picking up v24 on brains currently at v23 (post-v0.18.0) or earlier. + * + * Without this orchestrator, the `apply-migrations` registry stops at + * v0.18.0 and the low-level schema migration in src/core/migrate.ts never + * fires on upgrade, because doctor + connectEngine never call initSchema(). + */ + +import { execSync } from 'child_process'; +import type { Migration, OrchestratorOpts, OrchestratorResult, OrchestratorPhaseResult } from './types.ts'; + +// ── Phase A — Schema ──────────────────────────────────────── + +function phaseASchema(opts: OrchestratorOpts): OrchestratorPhaseResult { + if (opts.dryRun) return { name: 'schema', status: 'skipped', detail: 'dry-run' }; + try { + execSync('gbrain init --migrate-only', { stdio: 'inherit', timeout: 600_000, env: process.env }); + return { name: 'schema', status: 'complete' }; + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + return { name: 'schema', status: 'failed', detail: msg }; + } +} + +// ── Orchestrator ──────────────────────────────────────────── + +async function orchestrator(opts: OrchestratorOpts): Promise { + const phases: OrchestratorPhaseResult[] = []; + phases.push(phaseASchema(opts)); + + const anyFailed = phases.some(p => p.status === 'failed'); + const status: OrchestratorResult['status'] = anyFailed ? 'partial' : 'complete'; + + return { + version: '0.18.1', + status, + phases, + pending_host_work: 0, + }; +} + +// ── Export ────────────────────────────────────────────────── + +export const v0_18_1: Migration = { + version: '0.18.1', + featurePitch: { + headline: 'Row Level Security hardened on all public tables + escape hatch.', + description: + 'v0.18.1 fixes a latent security gap: 10 gbrain-managed public tables ' + + 'shipped without RLS. On Supabase, they were reachable by the anon key. ' + + 'Migration v24 backfills RLS on existing brains automatically when ' + + '`gbrain apply-migrations` runs. `gbrain doctor` now scans every ' + + 'public table (no hardcoded allowlist) and exits 1 on gaps. For tables ' + + 'that should stay anon-readable on purpose, operators set a ' + + '`GBRAIN:RLS_EXEMPT reason=` comment via psql. See ' + + 'docs/guides/rls-and-you.md.', + }, + orchestrator, +}; diff --git a/src/core/migrate.ts b/src/core/migrate.ts index 7958c25e7..2f2d82fc3 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -726,7 +726,13 @@ export const MIGRATIONS: Migration[] = [ ALTER TABLE budget_reservations ENABLE ROW LEVEL SECURITY; RAISE NOTICE 'v24: RLS enabled on 10 backfill tables (role % has BYPASSRLS)', current_user; ELSE - RAISE WARNING 'v24: Skipping RLS backfill: role % does not have BYPASSRLS privilege. Run as postgres role to enable.', current_user; + -- Fail the migration loudly instead of WARNING + version-bump. + -- The runner unconditionally records schema_version on success, + -- so a silent WARNING here would permanently lock the backfill out + -- on future runs even after switching to a bypass role. Raising + -- aborts the transaction, leaves schema_version at the prior value, + -- and lets the next invocation retry after the role is fixed. + RAISE EXCEPTION 'v24 rls_backfill_missing_tables: role % does not have BYPASSRLS privilege — cannot enable RLS safely. Re-run as postgres (or another BYPASSRLS role). The migration will retry automatically on the next initSchema call.', current_user; END IF; END $$; `, diff --git a/test/migrate.test.ts b/test/migrate.test.ts index 55ff42880..047e9c4e7 100644 --- a/test/migrate.test.ts +++ b/test/migrate.test.ts @@ -219,7 +219,18 @@ describe('migration v24 — rls_backfill_missing_tables', () => { const sql = v24!.sql || ''; expect(sql).toContain('rolbypassrls'); expect(sql).toContain('IF has_bypass THEN'); - expect(sql).toMatch(/RAISE WARNING[^;]*BYPASSRLS/); + }); + + // Codex found: if v24 RAISE WARNINGs instead of raising on non-BYPASSRLS, + // the migration runner still bumps schema_version to 24, permanently + // skipping the backfill on future runs even after the role is fixed. + // The fix is to raise loudly so the transaction aborts, version stays + // at 23, and the next initSchema call retries after role reassignment. + test('fails loudly on non-BYPASSRLS roles instead of silently bumping version', () => { + const v24 = MIGRATIONS.find(m => m.version === 24); + const sql = v24!.sql || ''; + expect(sql).toMatch(/RAISE EXCEPTION[^;]*BYPASSRLS/); + expect(sql).not.toMatch(/RAISE WARNING[^;]*BYPASSRLS/); }); test('LATEST_VERSION has caught up to 24', () => {