Skip to content

fix(db): close the PostgREST Data API surface (migration 027) - #170

Merged
zaridan merged 5 commits into
mainfrom
claude/review-requested-3rm4hu
Jul 31, 2026
Merged

fix(db): close the PostgREST Data API surface (migration 027)#170
zaridan merged 5 commits into
mainfrom
claude/review-requested-3rm4hu

Conversation

@zaridan

@zaridan zaridan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What happened

A Supabase advisor email reported one publicly accessible table. Checking the project found the problem was broader: all 19 tables in public had Row-Level Security disabled and full SELECT/INSERT/UPDATE/DELETE/TRUNCATE granted to both anon and authenticated.

Supabase exposes public through PostgREST, and the anon key is public by design (it ships to browsers). So this was unauthenticated read and write against production.

Blast radius

Reads were partly cushioned — mailbox_oauth_tokens holds AES-256-GCM ciphertext (src/store/token-crypto.ts) keyed outside the database, so a dump yields ciphertext, not usable Gmail credentials. Conversation and thread bodies were plaintext.

The write path was the real severity: anon could INSERT into agents, agent_auth_identities, agent_mailbox_access, and webauthn_credentials — enough to self-provision an authenticated Agent with mailbox access, which walks straight past the encryption. anon also held TRUNCATE on everything.

Why closing it is safe

Nothing in this codebase uses the Data API. The app reaches Postgres directly over the pooler (DATABASE_URL, src/db/postgres.ts) and uses Supabase Storage with the service_role key (src/providers/adapters/supabase-storage/). There is no anon-key client anywhere in the repo — grepped repo-wide including web/, zero hits. It was pure attack surface with zero application value.

The change

Migration 027 applies both layers, because they fail independently — RLS alone would be undone by a future permissive policy, revoked grants alone by Supabase re-running its stock bootstrap:

  1. ENABLE ROW LEVEL SECURITY on every table, spelled out one ALTER TABLE per table so the set is reviewable in the diff and a future table fails loudly by omission. No policies attached — deny-by-default.
  2. Revoke the anon/authenticated grants, including ALTER DEFAULT PRIVILEGES so tables created by future migrations don't arrive pre-granted.

Tables are owned by postgres, which bypasses RLS unless FORCE ROW LEVEL SECURITY is set (deliberately not set), so the application is unaffected.

The splitter change is load-bearing, not drive-by

The revokes are wrapped in a pg_roles-guarded DO $$ ... $$ block: anon/authenticated are Supabase-created roles that don't exist in PGlite, which the test suite migrates against, and an unguarded REVOKE ... FROM anon is a hard error when the role is missing — it would fail every test that migrates.

That block is why splitStatements had to learn about dollar quoting. Its body is full of semicolons, and the old naive split(';') tore it into invalid fragments. The scanner now tracks single-quoted literals (with '' escaping), quoted identifiers, dollar-quoted bodies (tag matched exactly), and nested block comments. Migrations 001–026 tokenize identically to before.

The file's own doc comment had anticipated exactly this ("a smarter splitter would be warranted" the first time a migration needed a function body).

Production was fixed ahead of this PR

Per the maintainer's call, the same lockdown was applied directly to the production project to close live exposure rather than wait on review. Verified after:

  • 19/19 tables RLS-enabled
  • 0 remaining anon/authenticated table grants; has_table_privilege false for SELECT/INSERT/DELETE on the sensitive tables
  • service_role access intact
  • all ERROR-level security advisors clear

The advisor now reports rls_enabled_no_policy at INFO for each table. That is the intended end state, not an outstanding item — no policies is the point, since nothing should reach these tables through PostgREST.

Evidence on tampering

Data inspection found no signs of it: 2 agents, 1 mailbox, 27 threads, 23 conversations, and webauthn_credentials empty (so no credential injection occurred). This checked state, not access logs — it is not proof of non-access.

Verification

  • src/db/migrate.test.ts — 38/38 pass, including three new tests: RLS on every public table (named-not-counted, with a non-vacuity guard), migrate() succeeding on PGlite with the roles absent, and the DO block surviving statement splitting.
  • npm run typecheck — clean.
  • npm run lint — clean.
  • Full npm test was still running locally when this was opened (PGlite spins a fresh Postgres per test); CI covers it here.

Follow-up for the maintainer

Disabling the PostgREST Data API entirely for the project is a dashboard setting I can't change from here — worth doing as defence in depth, since it removes the surface at the source rather than relying on grants staying correct.


Generated by Claude Code

Summary by CodeRabbit

  • Security
    • Strengthened production database access controls by enabling row-level security (RLS) across all public tables.
    • Restricted default and existing API roles by revoking broad privileges and applying guarded permission changes.
    • Added protections to ensure future tables follow the same RLS-enabled requirement.
  • Bug Fixes
    • Improved migration SQL execution to correctly handle semicolons within quoted strings, identifiers, dollar-quoted blocks, and nested comments.
  • Documentation
    • Updated mailbox connection and decision notes to reflect the new RLS and privilege model, including operational cautions about database connections.

A Supabase advisor alert reported one publicly accessible table. The
actual state was broader: all 19 tables in `public` had RLS disabled and
full SELECT/INSERT/UPDATE/DELETE/TRUNCATE granted to `anon` and
`authenticated`. Supabase exposes `public` through PostgREST and the anon
key is public by design, so that was unauthenticated read and write
against production — including INSERT into `agents`,
`agent_auth_identities`, `agent_mailbox_access` and
`webauthn_credentials`, enough to self-provision an authenticated Agent.

Nothing in this codebase uses that surface. The app reaches Postgres
directly over the pooler (`DATABASE_URL`) and Supabase Storage with the
`service_role` key; there is no anon-key client anywhere in the repo. The
Data API was pure attack surface with zero application value, which is
what makes closing it entirely safe.

Migration 027 applies both layers, because they fail independently:
enable RLS on every table (deny-by-default, no policies), and revoke the
anon/authenticated grants including via ALTER DEFAULT PRIVILEGES so
future tables do not arrive pre-granted. Tables are owned by `postgres`,
which bypasses RLS unless FORCE ROW LEVEL SECURITY is set — deliberately
not set — so the application is unaffected.

The revokes are wrapped in a pg_roles-guarded DO block: `anon` and
`authenticated` are Supabase-created roles that do not exist in PGlite,
which the test suite migrates against, and an unguarded REVOKE errors
when the role is missing. That block is why `splitStatements` had to
learn about dollar quoting — its body is full of semicolons, and the old
naive `split(';')` tore it into invalid fragments. The scanner now tracks
quoted literals, quoted identifiers, dollar-quoted bodies, and nested
comments, so migrations 001-026 tokenize exactly as before.

The same lockdown was applied directly to the production project ahead of
this landing, to close live exposure rather than wait on review. Verified
after: 19/19 tables RLS-enabled, zero anon/authenticated grants remaining,
`service_role` access intact, and all ERROR-level security advisors clear.

Inspection found no evidence of tampering — that checked state, not access
logs, so it is not proof of non-access.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDobgW78NBTFSggU82EzVW
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 958b7f3e-1780-4847-88be-581f5881bd06

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This change adds migration 027 to enable RLS on public tables and revoke Supabase API role privileges, enhances SQL statement splitting for quoted content, updates migration tests, and documents the related security decision.

Changes

Data API lockdown

Layer / File(s) Summary
Lockdown migration and decision record
src/db/migrate.ts, docs/decisions/README.md, specs/mail/mailbox-connection.md
Migration 027 enables RLS and conditionally revokes anon/authenticated privileges, while project documentation records the architecture, deployment invariant, and follow-on migration requirements.
Dollar-quoted SQL splitting
src/db/migrate.ts
splitStatements preserves semicolons inside strings, identifiers, dollar-quoted blocks, and comments.
Migration registration and behavior tests
src/db/migrate.test.ts
Tests cover migration registration, idempotency, RLS activation, role handling, search-path behavior, and SQL splitting contexts.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MigrationRunner
  participant StatementSplitter
  participant Database
  MigrationRunner->>StatementSplitter: Split migration 027 SQL
  StatementSplitter-->>MigrationRunner: Return intact SQL statements
  MigrationRunner->>Database: Execute RLS and privilege changes
  Database-->>MigrationRunner: Record migration 27
Loading

Possibly related PRs

  • Helpthread/helpthread#29: Both changes extend the database migration framework and depend on its SQL statement parsing behavior.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: migration 027 closing the exposed PostgREST Data API surface.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/review-requested-3rm4hu

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

An independent adversarial review of the previous commit found real
defects. Fixes, in order of severity:

- The hand-written SQL lexer had no direct test coverage and was
  unexported, so it could not be unit-tested. It is now exported and
  covered by 11 adversarial cases: dollar-quote tags, nested `$$` inside
  `$body$`, `$1` placeholders, `''` escaping, `--` inside a literal,
  nested block comments, and unterminated quotes/comments/bodies.

- The lexer mis-tokenized `E'...'` escape strings: a backslash-escaped
  quote (`E'a\';b'`) terminated the literal early, splitting one
  statement into two invalid fragments. It now scans escape strings with
  backslash awareness, guarded by a token-boundary check so an
  identifier ending in `e` is not mistaken for one.

- The DO block hardcoded `IN SCHEMA public`, but `PostgresDb` supports a
  `schema` option under which no application table lives in `public`. In
  that mode RLS was enabled but the grants were never revoked — half the
  defence-in-depth silently no-opping. Both halves now target
  `current_schema()`.

- `ALTER DEFAULT PRIVILEGES ... REVOKE` was described as protection
  against Supabase re-running its stock bootstrap. That is wrong: it
  deletes a default-ACL entry rather than installing a deny, and without
  `FOR ROLE` it binds only to the migrating role. The doc comment and
  decision log now state the real, narrower guarantee. `TYPES` added to
  the revoked defaults.

- The revoke path (roles present) was untested — every prior assertion
  about REVOKE passed vacuously, since PGlite has no Supabase roles. A
  new test creates `anon`/`authenticated`, re-grants stock-Supabase
  privileges, re-runs the migration, and asserts the grants are stripped
  and `has_table_privilege` is false.

- `specs/mail/mailbox-connection.md` still asserted "RLS is disabled on
  all 19 tables" and that enabling it without policies "would break the
  engine's own access" — both now false, and the second was the belief
  that let this sit open. Corrected.

- Recorded the deployment invariant this promotes: `DATABASE_URL` must
  connect as the table-owning role. It does today, but violating it now
  fails silently (zero rows, not an error) and no test can catch it,
  because PGlite also runs as owner.

The review separately proved by execution that migrations 001-026
tokenize byte-identically under the new lexer; only 027 differs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDobgW78NBTFSggU82EzVW

zaridan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Second commit (7acc7f8) addresses findings from an independent adversarial review of 5455a28. Auto-incremental review is disabled on this repo, so requesting one explicitly — the new commit changes the SQL lexer and the migration body, which is the part most worth a second pass.


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

@zaridan I’ll perform an incremental review of the latest changes, focusing on the SQL lexer’s quoting/comment edge cases and migration 027’s privilege/RLS behavior.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…lves

A second adversarial review — of the previous fix commit, which had had no
independent review of its own — found that commit had introduced a
regression.

Switching the revokes from a hardcoded `public` to `current_schema()`
made the two halves of migration 027 resolve their schema by DIFFERENT
rules. The 19 `ALTER TABLE ... ENABLE ROW LEVEL SECURITY` statements are
unqualified, so they resolve by scanning search_path for a schema that
CONTAINS the table; `current_schema()` is merely the first existing entry
on that path. Where those differ, RLS is enabled in one schema while the
revokes land in another, and the migration commits reporting success with
every PostgREST grant still in place.

Both halves now derive from the same anchor: the namespace of
`'conversations'::regclass`, which resolves by exactly the rule the
unqualified ALTER TABLEs use, so the two cannot diverge by construction.

The accompanying doc comment claimed both halves were "written against
current_schema() ... so they target the same place" — asserting precisely
the invariant the code did not hold. Rewritten to describe the real
mechanism and why the distinction is load-bearing.

Also from the review:
- Added a regression test for the above, verified to FAIL against the
  `current_schema()` version. It replays migration 027's DO block rather
  than calling migrate(), because re-running migrate() under a shadowed
  search_path makes its own `CREATE TABLE IF NOT EXISTS _migrations` land
  in the leading schema and re-bootstrap every table there — which
  measures schema bootstrapping, not schema resolution. (The first draft
  of this test made exactly that mistake and passed for the wrong reason.)
- Asserted `pg_default_acl` is clear, covering the ALTER DEFAULT
  PRIVILEGES half — including the `ON TYPES` line added last commit with
  no coverage.
- Documented the one accepted lexer false negative: `E'...'` directly
  after a dollar-quote terminator is not treated as an escape string,
  because `$` must stay in the token-boundary look-behind so `foo$e'x'`
  is not misread. Postgres resolves this by longest-match; we take the
  false negative over breaking the commoner case.
- Generalized the in-code standing rule from "adds a table to `public`"
  to "adds a table", matching the decision log.
- specs/mail/mailbox-connection.md no longer states the owner-bypass
  claim unconditionally: it now records that `DATABASE_URL` must connect
  as the table-owning role and that violating it fails silently with
  empty result sets rather than an error. Dropped the "resolved" date
  from a change still sitting on an unmerged branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDobgW78NBTFSggU82EzVW

zaridan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Third commit (1987152). The previous request hit a rate limit before it ran, so 7acc7f8 and 1987152 are both still unreviewed by CodeRabbit.

Worth a close look at src/db/migrate.ts: 7acc7f8 switched migration 027's revokes to current_schema(), which introduced a silent-failure bug — the unqualified ALTER TABLEs resolve by scanning search_path for a schema containing the table, while current_schema() is just the first entry, so the two halves could target different schemas and the migration would still report success. 1987152 anchors both to 'conversations'::regclass.


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

@zaridan I’ll perform an incremental review of the unreviewed commits, with particular attention to migration 027’s regclass-based schema anchoring and SQL lexer edge cases in src/db/migrate.ts.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/db/migrate.test.ts (1)

1651-1654: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Fail loudly if the anchor string ever moves.

indexOf returning -1 makes slice(-1) yield the final character, so renaming the $migration027$ tag turns this regression test into a query of a stray newline that still "passes" the later assertion vacuously — precisely the failure mode the test exists to catch.

♻️ Proposed guard
-    const roleGuardedRevokes = MIGRATION_027_SQL.slice(
-      MIGRATION_027_SQL.indexOf('DO $migration027$'),
-    )
+    const doBlockStart = MIGRATION_027_SQL.indexOf('DO $migration027$')
+    expect(doBlockStart).toBeGreaterThan(-1)
+    const roleGuardedRevokes = MIGRATION_027_SQL.slice(doBlockStart)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/db/migrate.test.ts` around lines 1651 - 1654, Validate that
MIGRATION_027_SQL contains the 'DO $migration027$' anchor before slicing in the
roleGuardedRevokes setup, and fail the test explicitly if indexOf returns -1;
only call db.query with the anchored SQL when the guard succeeds.
src/db/migrate.ts (1)

1576-1600: 🔒 Security & Privacy | 🔵 Trivial

Worth stating the grantor invariant next to the connection-owner invariant.

REVOKE only removes ACL entries whose grantor is the executing role (or a role it can act as). If Supabase's bootstrap grants were made by supabase_admin and DATABASE_URL connects as postgres without that membership, these statements emit a warning and complete successfully while the grants stay in place — the same silent-success failure mode the comment above already warns about for schema resolution. Production was verified out-of-band per the PR description, so this is an operational note rather than a defect: consider adding a post-migration assertion (or a documented check) that information_schema.role_table_grants is empty for anon/authenticated.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/db/migrate.ts` around lines 1576 - 1600, Document or assert the grantor
requirement alongside the existing connection-owner handling in the migration
flow containing the role revocations: verify that the executing role can revoke
grants created by the bootstrap grantor, and add a post-migration check using
information_schema.role_table_grants to confirm anon and authenticated have no
remaining table grants. Preserve the current revoke statements and make any
failed verification explicit rather than allowing silent success.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/decisions/README.md`:
- Around line 84-89: The README decision text incorrectly claims Migration 027
uses current_schema(); update the Migration 027 description to state that it
resolves the target schema from 'conversations'::regclass and intentionally
avoids current_schema() because it can diverge from unqualified name resolution.
Preserve the surrounding explanation of RLS, privilege revocation, default
privileges, and production lockdown.

In `@src/db/migrate.ts`:
- Around line 1580-1583: Update the schema revoke statement in the migration’s
privilege-lockdown block to use PostgreSQL’s ALL ROUTINES target instead of ALL
FUNCTIONS, covering both functions and procedures. Leave the separate ALTER
DEFAULT PRIVILEGES statement unchanged.

---

Nitpick comments:
In `@src/db/migrate.test.ts`:
- Around line 1651-1654: Validate that MIGRATION_027_SQL contains the 'DO
$migration027$' anchor before slicing in the roleGuardedRevokes setup, and fail
the test explicitly if indexOf returns -1; only call db.query with the anchored
SQL when the guard succeeds.

In `@src/db/migrate.ts`:
- Around line 1576-1600: Document or assert the grantor requirement alongside
the existing connection-owner handling in the migration flow containing the role
revocations: verify that the executing role can revoke grants created by the
bootstrap grantor, and add a post-migration check using
information_schema.role_table_grants to confirm anon and authenticated have no
remaining table grants. Preserve the current revoke statements and make any
failed verification explicit rather than allowing silent success.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d3d2e68-f3a9-44db-be0f-ff6a94444fe4

📥 Commits

Reviewing files that changed from the base of the PR and between 5455a28 and 1987152.

📒 Files selected for processing (4)
  • docs/decisions/README.md
  • specs/mail/mailbox-connection.md
  • src/db/migrate.test.ts
  • src/db/migrate.ts

Comment thread docs/decisions/README.md Outdated
Comment thread src/db/migrate.ts
Four findings from CodeRabbit's review of the branch.

- The decision log still described migration 027 as resolving its target
  schema with `current_schema()`. That was corrected in the code's doc
  comment last commit but missed here, so the decision record documented
  the REJECTED approach as the decision — the surest way to have it
  reintroduced later. Now states the regclass anchor and why
  `current_schema()` was rejected.

- `REVOKE ALL ON ALL FUNCTIONS IN SCHEMA` covers functions and aggregates
  but NOT procedures, so any procedure's grants would have survived a
  lockdown that claims to be exhaustive. Switched to `ALL ROUTINES`, which
  covers both. (The `ALTER DEFAULT PRIVILEGES ... ON FUNCTIONS` line needs
  no change — there, FUNCTIONS and ROUTINES are synonyms.)

- REVOKE only strips ACL entries whose GRANTOR is the executing role;
  against entries granted by another role it emits a warning and completes
  successfully. Supabase's bootstrap grants run as `supabase_admin`, so a
  deployment whose migrating role is not the grantor would get a migration
  that reports a lockdown it did not perform. That is the same
  silent-success shape as the search_path divergence fixed last commit, in
  a migration whose entire purpose is closing a hole nobody noticed — so
  it now verifies via aclexplode and RAISEs with a remediation hint rather
  than trusting the REVOKEs landed. Verified the guard actually fires by
  replaying the block with the REVOKEs neutered (it raises, naming the
  count and schema) and unmodified (it stays silent).

- The regression test sliced on `indexOf(...)` without checking for -1,
  which would have made `slice(-1)` quietly test a stray newline. Asserts
  the anchor first.

Also fixed a self-inflicted break: the SQL comment introduced here used
backticks inside the JS template literal holding the migration, which
terminated the string and turned the SQL into JS. Caught by typecheck.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDobgW78NBTFSggU82EzVW

zaridan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

f421971 addresses your four findings. Flagging the one that changes runtime behavior rather than wording: migration 027 now ends with an aclexplode check that RAISE EXCEPTIONs if any anon/authenticated privilege survives the revokes — so a grantor mismatch aborts the migration instead of reporting a lockdown it did not perform. That can fail a deploy, so it's worth confirming the query can't false-positive (e.g. on views/matviews, or privileges held via PUBLIC rather than granted to the roles directly).


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== PR head and requested commit ==='
git rev-parse --short HEAD
git show --no-ext-diff --format=fuller --stat f421971

printf '%s\n' '=== Migration module outline ==='
ast-grep outline src/db/migrate.ts --items all

printf '%s\n' '=== Relevant migration SQL and verifier contexts ==='
rg -n -i -C 12 \
  "MIGRATION_027|aclexplode|RAISE EXCEPTION|relkind|information_schema|has_table_privilege|anon|authenticated|PUBLIC" \
  src/db/migrate.ts src/db/migrate.test.ts

printf '%s\n' '=== Commit patch for relevant files ==='
git diff --no-ext-diff f421971^ f421971 -- src/db/migrate.ts src/db/migrate.test.ts

printf '%s\n' '=== Tests surrounding migration 027 ==='
rg -n -C 8 "migration 027|MIGRATION_027|lock_down_data_api|aclexplode" src/db/migrate.test.ts

Length of output: 48597


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '=== Complete migration 027 SQL ==='
sed -n '1536,1628p' src/db/migrate.ts

printf '%s\n' '=== All verifier-related test code and history ==='
rg -n -i -C 10 \
  "aclexplode|leftover|REVOKEs neutered|neuter|privilege\\(s\\).*remain|PUBLIC|GRANT ALL ON.*PUBLIC|GRANT .* TO PUBLIC|relacl" \
  src/db/migrate.test.ts
git log --oneline --all -- src/db/migrate.test.ts | head -30
git log -S'aclexplode' --oneline --all -- src/db/migrate.ts src/db/migrate.test.ts
git show --no-ext-diff --format=medium f421971^:src/db/migrate.test.ts | \
  rg -n -i -C 8 "aclexplode|leftover|neuter|PUBLIC|GRANT.*TO PUBLIC|MIGRATION_027"

printf '%s\n' '=== PostgreSQL documentation/source available in sandbox ==='
( rg -n -i -C 3 \
  "ALL TABLES IN SCHEMA|PUBLIC.*privileg|aclexplode|aclitem.*PUBLIC|grant.*public" \
  /usr/share/doc /usr/share/postgresql 2>/dev/null | head -240 ) || true

Length of output: 21244


@zaridan I found one remaining gap in the verifier:

src/db/migrate.ts Line 1616 joins aclexplode(c.relacl).grantee to pg_roles, so it intentionally sees only ACLs granted directly to anon or authenticated. A PUBLIC relation grant has the pseudo-role grantee (OID 0), does not join to pg_roles, and therefore is not counted.

That is not a false positive: the relation-kind list correctly covers the relation types targeted by REVOKE ALL ON ALL TABLES IN SCHEMA, including views and materialized views. It is a false negative: REVOKE ... FROM anon, authenticated leaves a GRANT ... TO PUBLIC intact, while anon and authenticated retain that effective privilege through PUBLIC. The migration would report success despite its stated postcondition.

The verifier should either include acl.grantee = 0 as a failure condition (with remediation instructing the operator to revoke from PUBLIC) or migration 027 should explicitly revoke the relevant table privileges from PUBLIC if that is within its intended lockdown scope. A regression test should grant a privilege on a target relation to PUBLIC, replay the migration body, and assert that it fails rather than records migration 027 as successful.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Two more review rounds — an independent adversarial pass on 1987152 and a
CodeRabbit pass on f421971 — found defects in both, including one in the
verifier added last commit.

- **The verifier had a false negative for PUBLIC.** It joined
  aclexplode(relacl).grantee to pg_roles, so it saw only ACLs granted
  directly to anon/authenticated. A privilege granted to the PUBLIC
  pseudo-role (grantee OID 0, no pg_roles row) is held by every role: anon
  keeps effective access with no ACL entry of its own to revoke, and the
  migration reports success anyway — the exact silent-success failure the
  verifier exists to prevent. Verified: with only `GRANT SELECT ... TO
  PUBLIC`, has_table_privilege('anon', ...) is true while a direct-grants
  count reads 0. Migration 027 now also revokes relation privileges from
  PUBLIC, and the verifier counts grantee = 0. Routines are deliberately
  excluded from the PUBLIC revoke — Postgres grants EXECUTE to PUBLIC by
  default there and stripping it could break an extension in this schema;
  table data is what the PostgREST surface exposes.

- **The pg_default_acl assertion was vacuous.** pg_default_acl held zero
  rows for the entire test — `GRANT ALL ON ALL TABLES` does not create a
  default-ACL entry, only `ALTER DEFAULT PRIVILEGES ... GRANT` does — so
  it asserted 0 out of an empty table and would have passed with all four
  ALTER DEFAULT PRIVILEGES statements deleted from the migration. A false
  safety net over the half of the migration whose guarantees are weakest.
  The test now seeds a real entry first; verified it fails without the
  revokes (1 entry survives).

- **Overstated scope, now corrected.** The DO-block comment and the
  regression test presented the search_path divergence as a reachable
  production failure. It is not reachable through migrate(): migrate()'s
  own CREATE TABLE IF NOT EXISTS _migrations uses creation semantics, so a
  shadowed search_path makes it re-bootstrap every table into the leading
  schema, after which both resolution rules agree. Divergence needs
  _migrations in one schema and app tables in another, which no path here
  produces. The regclass anchor is still right — it keeps the migration
  correct on its own terms — but it is defence, not a bug fix, and is now
  documented as such.

- **The RLS failure mode was described wrongly.** Both docs claimed a
  least-privilege DATABASE_URL would fail silently with "an empty inbox,
  not an error". Only reads are quiet; every write raises `new row
  violates row-level security policy`. Verified both halves. The docs sent
  an operator hunting a silent symptom when the real signal is loud and
  arrives first via inbound ingest.

- Restored "no non-test caller" on splitStatements (the previous rewording
  was less accurate — the test IS the caller, and is why it is exported),
  and added the missing non-vacuity precondition to the search_path test.

New PUBLIC regression test asserts the precondition that makes it
meaningful — reachable by anon, invisible to a direct-grants check — then
that the migration body closes it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDobgW78NBTFSggU82EzVW
@zaridan
zaridan merged commit 2ca7f2e into main Jul 31, 2026
5 checks passed
zaridan added a commit that referenced this pull request Aug 1, 2026
…ables

`lock_down_data_api` (#170) took migration id 27 on main while this branch was
in review, and this branch already used 27 and 28. Two problems existed only in
the combination — neither branch was wrong on its own.

**The IMAP tables would never have been created.** `id` is the applied-once
key. With main's 027 already recorded, shipping a second 027 would have been
read as already-applied and SKIPPED: no tables, no error, IMAP intake simply
dead on the next deploy. HT-101's migrations are now 028 (`imap_transport`) and
029 (`conversation_mailbox_id`), with a comment at the array explaining why the
numbering must not be "tidied" back.

**The three IMAP tables would have shipped without RLS.** Migration 027 enables
row-level security on every table that existed when it ran; ours are created
afterwards, so it cannot reach them. Without this they are queryable through
the Supabase PostgREST Data API, and `imap_mailbox_credentials` holds encrypted
app passwords. Migration 028 now enables RLS on all three, per the standing
rule 027's own doc states: "a migration that adds a table MUST also ENABLE ROW
LEVEL SECURITY on it."

Main's own RLS test is what caught this — it asserts no table in `public` lacks
RLS and failed against the merge. Kept as-is; it now covers HT-101's tables too.

Conflicts resolved by taking main's `migrate.ts`/`migrate.test.ts` wholesale and
re-applying HT-101's two migrations and its migration test on top, rather than
hand-splicing the conflict regions — an earlier splice mangled a doc block and a
template literal.

Gates on the merged tree: typecheck, web typecheck, web build, lint, gitleaks
all exit 0; 87 files / 1742 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants