fix(security): apply the transactions INSERT lockdown to cloud and make migration drift detectable (#541)

docs/MIGRATIONS.md

@@ -18,6 +18,86 @@ Edit the generated file with your SQL. Always make migrations **idempotent** whe
 
 ## Applying Migrations
 
+### The rule: cloud changes go through `supabase db push`
+
+**Never apply a migration to cloud with the MCP `apply_migration` tool, the SQL
+editor, or a raw Management API call unless you also stamp it with the version
+from its filename.** This is not style preference — it is what keeps drift
+detectable.
+
+`supabase_migrations.schema_migrations` is the only record of what cloud has
+run. `supabase db push` stamps each migration with **the timestamp from its
+filename**. Every ad-hoc path stamps a **fresh** timestamp instead. Once the
+ledger holds a stamp that matches no file in `supabase/migrations/`, the
+question "what is missing from cloud?" stops having a usable answer: the
+re-stamped migrations show up as *pending* (false positives), and a genuinely
+missing migration is indistinguishable from them.
+
+That is not hypothetical. It is how issue #541 happened:
+
+| Repo file | Cloud stamp before #541 |
+|---|---|
+| `20260721120000_add_binance_personal_provider` | `20260725213441` |
+| `20260725110000_transaction_split_snapshot_backstop` | `20260725213508` |
+| `20260725160000_entitlement_gated_enrollment_inserts` | `20260725213610` |
+| `20260725170000_transactions_column_hardening` | `20260725192946` |
+| `20260725180000_transactions_insert_lockdown` | **never applied** |
+
+Four migrations were applied through `apply_migration` and re-stamped. The
+fifth — the `transactions` INSERT lockdown from #538, a `severity:critical`
+payments fix — was never applied at all, and hid among the four false
+positives for a day. Diffing the repo against the ledger reported all five as
+pending, so the real gap looked like more of the same noise.
+
+If you do have to apply SQL out of band, **immediately** repair the stamp:
+
+```bash
+# Preferred — the CLI's purpose-built command for this
+supabase migration repair --status applied <VERSION_FROM_FILENAME>
+supabase migration repair --status reverted <WRONG_FRESH_STAMP>
+```
+
+Then run `npm run verify:cloud` (below) and confirm it is clean before moving on.
+
+### Verifying cloud matches this repo
+
+```bash
+SUPABASE_ACCESS_TOKEN=sbp_... npm run verify:cloud
+```
+
+Queries the **live** database and exits non-zero on drift. It asserts both
+halves of the problem above:
+
+- **Ledger integrity** — every repo migration is stamped on cloud under its own
+  filename, and cloud carries no stamp that matches no file.
+- **Payment invariants** (#512 / #528 / #538) — `authenticated` and `anon` hold
+  no INSERT on `transactions`, no column-level INSERT grant survives,
+  `service_role` still has INSERT, the `authenticated` UPDATE grant is exactly
+  `(status, provider_subscription_id, stripe_payment_intent_id)`, the INSERT
+  policy pins `status` plus all four `settlement_*` columns to NULL, and both
+  `before_transaction_split_snapshot_*` triggers exist and are enabled.
+
+It reads catalog state, not migration text, so it also catches a *later*
+migration re-widening something — including a schema dump re-applying the
+original `GRANT ALL ON TABLE transactions TO authenticated`.
+
+**Run it from `master`, after merging.** The two ledger checks compare cloud
+against the migrations in your *current checkout*. On a feature branch, any
+migration another in-flight branch has already applied to cloud shows up as an
+orphan, and any migration on your branch not yet applied shows up as pending —
+neither is real drift. The payment-invariant checks are branch-independent and
+meaningful anywhere.
+
+The token is a [personal access token](https://supabase.com/dashboard/account/tokens).
+It uses the Management API over HTTPS rather than a Postgres connection because
+port 5432 is blocked on some networks this project is developed from — the same
+reason Option 2 below exists.
+
+> Unit tests cannot do this job. `tests/unit/verify-cloud-schema.test.ts` and
+> `tests/unit/transaction-split-snapshot-backstop.test.ts` prove the *rules* are
+> right; neither opens a database connection. A green `npm run test:unit` says
+> nothing about whether a migration was ever applied. Only `verify:cloud` does.
+
 ### Option 1: Supabase CLI (preferred)
 
 ```bash
@@ -30,7 +110,10 @@ This connects via the Supabase connection pooler and applies all pending migrati
 
 ### Option 2: Management API (fallback)
 
-When the CLI can't connect, push migrations via the Supabase Management API:
+When the CLI can't connect, push migrations via the Supabase Management API.
+**This is the path that creates drift** — the `INSERT INTO schema_migrations`
+line below is not optional bookkeeping, it is the entire reason this fallback is
+safe to use:
 
 ```bash
 # 1. Get your access token (stored in macOS keychain by the CLI)
@@ -91,16 +174,22 @@ process.stdout.write(JSON.stringify({query: 'SELECT version, name FROM supabase_
 
 ### What's pending locally?
 
-Compare local files against remote records:
+```bash
+SUPABASE_ACCESS_TOKEN=sbp_... npm run verify:cloud
+```
+
+Use this rather than eyeballing a diff. The manual comparison —
 
 ```bash
-# Local migration versions
 ls supabase/migrations/*.sql | sed 's|.*/||' | sed 's/_.*//'
-
-# Remote versions (use one of the methods above)
 ```
 
-Any local version not in the remote list needs to be applied.
+— is only trustworthy when the ledger is clean. If any migration was ever
+applied out of band, a local version missing from the remote list means *either*
+"never applied" *or* "applied under a different stamp", and nothing in the diff
+tells you which. `verify:cloud` reports the two cases separately (`not on cloud`
+vs `orphan stamps`) and additionally checks that the payment invariants actually
+hold, which no filename comparison can do.
 
 ## Running the Production Seed
 
@@ -158,15 +247,26 @@ DELETE FROM courses WHERE author_id NOT IN (SELECT id FROM profiles);
 
 ### Migration version mismatch
 
-If the remote `schema_migrations` table has different version numbers than local filenames (e.g. migrations were applied via dashboard with auto-generated timestamps):
+If the remote `schema_migrations` table has different version numbers than local filenames (e.g. migrations were applied via dashboard or `apply_migration` with auto-generated timestamps), prefer the CLI's purpose-built command:
+
+```bash
+supabase migration repair --status applied   <LOCAL_VERSION>
+supabase migration repair --status reverted  <REMOTE_VERSION>
+```
+
+If the CLI cannot reach the pooler, the equivalent write is:
 
 ```sql
--- Update the remote record to match the local filename
+-- Re-stamp the remote record to match the local filename.
+-- Verify FIRST that the DDL really is live (query the grants/policies/triggers
+-- it created) — this only repairs the ledger, it does not apply anything.
 UPDATE supabase_migrations.schema_migrations
 SET version = '<LOCAL_VERSION>'
 WHERE version = '<REMOTE_VERSION>' AND name = '<NAME>';
 ```
 
+Confirm with `npm run verify:cloud` afterwards.
+
 ### Prepared statement errors (port 6543)
 
 If you try the session pooler (port 6543) and get `prepared statement already exists`, use port 5432 or the Management API instead. The session pooler doesn't work well with the Supabase CLI's connection handling.

package.json

@@ -17,6 +17,7 @@
     "db:reset": "supabase db reset",
     "db:push": "supabase db push",
     "db:types": "supabase gen types typescript --linked --schema public > lib/database.types.ts",
+    "verify:cloud": "tsx scripts/verify-cloud-schema.ts",
     "mcp:build": "cd mcp-server && npm run build",
     "backup-course": "tsx scripts/backup-english-course.ts",
     "restore-course": "tsx scripts/restore-english-course.ts",

scripts/lib/verify-cloud-schema-checks.ts

@@ -0,0 +1,181 @@
+/**
+ * Issue #541 — the assertion half of `npm run verify:cloud`, kept pure.
+ *
+ * Split out from `scripts/verify-cloud-schema.ts` so the rules can be exercised
+ * without a network round-trip or a live database. That matters here for a
+ * specific reason: the acceptance criterion for #541 is "the check FAILS if you
+ * re-grant INSERT", and the honest way to demonstrate that is a test that feeds
+ * in a re-granted fixture — not briefly re-opening a write grant on the
+ * payments table of the only production database this project has.
+ *
+ * `tests/unit/verify-cloud-schema.test.ts` drives every rule below from both a
+ * healthy fixture and a drifted one.
+ */
+
+export type MigrationLedger = {
+  /** Version prefixes of `supabase/migrations/*.sql`, e.g. `20260725180000`. */
+  repoVersions: string[]
+  /** `version` column of `supabase_migrations.schema_migrations` on cloud. */
+  cloudVersions: string[]
+}
+
+export type TransactionPrivileges = {
+  auth_insert: boolean
+  anon_insert: boolean
+  service_insert: boolean
+  /** Comma-joined column list, or null when no column-level grant exists. */
+  auth_col_insert: string | null
+  auth_col_update: string | null
+}
+
+export type InsertPolicy = { policyname: string; with_check: string | null }
+export type TriggerState = { tgname: string; enabled: boolean }
+
+export type CloudFacts = {
+  ledger: MigrationLedger
+  priv: TransactionPrivileges
+  insertPolicies: InsertPolicy[]
+  triggers: TriggerState[]
+}
+
+export type Check = { name: string; ok: boolean; detail: string }
+
+/** The exact `authenticated` UPDATE column grant #528 established. */
+export const EXPECTED_UPDATE_COLS =
+  'provider_subscription_id, status, stripe_payment_intent_id'
+
+/** What the #538 INSERT policy must pin. Rendered form, after normalisation. */
+export const REQUIRED_POLICY_PINS = [
+  "status = 'pending'",
+  'settlement_currency IS NULL',
+  'settlement_base IS NULL',
+  'settlement_mint IS NULL',
+  'settlement_sol_usd IS NULL',
+]
+
+export const SPLIT_SNAPSHOT_TRIGGERS = [
+  'before_transaction_split_snapshot_insert',
+  'before_transaction_split_snapshot_update',
+]
+
+/**
+ * Strip the catalog's type annotations and collapse whitespace, so a pin written
+ * `status = 'pending'` matches how Postgres renders it back
+ * (`status = 'pending'::transaction_status`).
+ */
+export function normalisePolicyExpr(expr: string): string {
+  return expr.replace(/::[a-z_]+/g, '').replace(/\s+/g, ' ')
+}
+
+export function evaluateChecks(facts: CloudFacts): Check[] {
+  const checks: Check[] = []
+  const add = (name: string, ok: boolean, detail: string) =>
+    checks.push({ name, ok, detail })
+
+  // -------------------------------------------------------------------------
+  // A. Ledger integrity — what makes any future drift detectable at all.
+  // -------------------------------------------------------------------------
+  const cloudSet = new Set(facts.ledger.cloudVersions)
+  const repoSet = new Set(facts.ledger.repoVersions)
+  const pending = facts.ledger.repoVersions.filter((v) => !cloudSet.has(v))
+  const orphans = facts.ledger.cloudVersions.filter((v) => !repoSet.has(v))
+
+  add(
+    'every repo migration is applied to cloud',
+    pending.length === 0,
+    pending.length
+      ? `not on cloud: ${pending.join(', ')}`
+      : `all ${facts.ledger.repoVersions.length} applied`
+  )
+
+  // An orphan stamp is the failure mode that hid #538: it matches no file, so it
+  // pads the ledger and makes the pending list above untrustworthy.
+  //
+  // RUN THIS FROM `master`, AFTER MERGING. Both lists compare the ledger against
+  // the migrations present on the CURRENT checkout, so on a feature branch every
+  // migration another in-flight branch has already applied to cloud reads as an
+  // orphan, and every migration on this branch not yet applied reads as pending.
+  // Neither is drift. The detail line below says so rather than leaving someone
+  // to rediscover it and conclude the check is noisy.
+  add(
+    'cloud carries no migration stamp that matches no repo file',
+    orphans.length === 0,
+    orphans.length
+      ? `orphan stamps (applied ad hoc, not via db push — or applied by an ` +
+        `in-flight branch this checkout does not have; re-check on master): ` +
+        orphans.join(', ')
+      : 'ledger matches repo filenames exactly'
+  )
+
+  // -------------------------------------------------------------------------
+  // B. The #528 / #538 payment invariants, against live catalog state.
+  // -------------------------------------------------------------------------
+  add(
+    'authenticated holds no INSERT on transactions (#538)',
+    facts.priv.auth_insert === false,
+    facts.priv.auth_insert
+      ? 'GRANT IS PRESENT — a caller can open a self-priced row'
+      : 'revoked'
+  )
+
+  add(
+    'anon holds no INSERT on transactions (#538)',
+    facts.priv.anon_insert === false,
+    facts.priv.anon_insert ? 'GRANT IS PRESENT' : 'revoked'
+  )
+
+  // A table-level revoke leaves no column grants, but a later migration could
+  // add one and reopen the path without restoring the table grant.
+  add(
+    'no column-level INSERT grant to authenticated survives',
+    facts.priv.auth_col_insert === null,
+    facts.priv.auth_col_insert ? `columns: ${facts.priv.auth_col_insert}` : '(none)'
+  )
+
+  // The lockdown must not have taken the server-side writers with it.
+  add(
+    'service_role retains INSERT (webhooks, reconcilers, crons, admin routes)',
+    facts.priv.service_insert === true,
+    facts.priv.service_insert
+      ? 'granted'
+      : 'REVOKED — every payment write path is broken'
+  )
+
+  add(
+    `authenticated UPDATE grant is exactly (${EXPECTED_UPDATE_COLS}) (#528)`,
+    facts.priv.auth_col_update === EXPECTED_UPDATE_COLS,
+    `actual: ${facts.priv.auth_col_update ?? '(none)'}`
+  )
+
+  // The policy is unreachable while the grant is revoked. It is asserted anyway
+  // because the revoke is one `GRANT INSERT` away from being undone, and these
+  // pins are what keep the #538 under-quote impossible if that ever happens.
+  if (facts.insertPolicies.length === 0) {
+    add(
+      'transactions INSERT policy pins status + all four settlement columns (#538)',
+      false,
+      'no INSERT policy found on public.transactions'
+    )
+  } else {
+    for (const p of facts.insertPolicies) {
+      const normalised = normalisePolicyExpr(p.with_check ?? '')
+      const missing = REQUIRED_POLICY_PINS.filter((pin) => !normalised.includes(pin))
+      add(
+        `INSERT policy "${p.policyname}" pins status + all four settlement columns (#538)`,
+        missing.length === 0,
+        missing.length ? `missing pins: ${missing.join('; ')}` : 'all pins present'
+      )
+    }
+  }
+
+  for (const name of SPLIT_SNAPSHOT_TRIGGERS) {
+    const row = facts.triggers.find((t) => t.tgname === name)
+    add(
+      `trigger ${name} exists and is enabled (#512)`,
+      row?.enabled === true,
+      !row ? 'MISSING' : row.enabled ? 'enabled' : 'DISABLED'
+    )
+  }
+
+  return checks
+}