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', () => {