fix(db): make the split snapshot database-owned, not caller-supplied (#512)

Hardens the trigger backstop from #525. Two changes, both found by reviewing
that PR against a local database.

1. The snapshot column is client-writable. `transactions` grants ALL to
   `authenticated` and its RLS policies (20260313025318) restrict which ROWS a
   user may write, never which columns — so a student can POST their own
   transaction with `school_percentage_snapshot: 100`, or UPDATE it in later,
   and inflate what the platform believes it owes their school. A backstop
   whose rule is "an explicit value always wins" preserves exactly the write we
   most need to override. Verified against the local DB: with #525 as
   submitted, both the INSERT and the UPDATE tamper survive at 100 where the
   real split is 70.

   The column is now computed by the database on INSERT (no WHEN guard, so a
   supplied value cannot survive) and frozen on UPDATE (the previous value is
   restored verbatim).

2. The UPDATE branch was a lazy, non-deterministic backfill. Once the trigger
   is installed every new row is snapshotted at INSERT, so the UPDATE branch
   can only ever fire on legacy NULL rows — stamping TODAY's split on them at
   whatever arbitrary moment something incidental (a refund, an archive) first
   touches the row. That is the retroactive repricing #496 removed, applied to
   an unpredictable subset of history. It now fills a legacy NULL in exactly
   one case: a provider is activating the row (`payment_provider` changing),
   the one moment at which today's split is the honest answer.

   This needs two triggers rather than one, because OLD cannot be referenced in
   a WHEN clause on a combined INSERT OR UPDATE trigger.

Also corrects a claim repeated in the migration, the checkout route and #525's
description: `revenue_splits` is NOT super-admin-only under RLS. Its SELECT
policy is `tenant_id = get_tenant_id() OR is_super_admin()` — every tenant
member, students included, can read their school's split. SECURITY DEFINER is
kept for the opposite reason: so this trigger does not silently depend on a
permissive policy whose own name contradicts it, and stamp the 80 fallback onto
every checkout the day someone tightens it.

Verification (local DB, not just static): the verify snippet now covers seven
cases including both tamper paths and the no-lazy-backfill rule, and all seven
pass; the rollback script drops cleanly and the migration re-applies. The
snippet is relabelled LOCAL ONLY — it needs ALTER TABLE ... DISABLE TRIGGER,
which locks the live payments table. The unit tests are relabelled as static
drift guards, since a green run says nothing about whether the trigger works.

Co-Authored-By: Jiho Lee <optional.int@kakao.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NffLeUiW78cokcUXrt68n

Author: 3 people

app/api/payments/checkout/route.ts

@@ -207,9 +207,18 @@ export async function POST(req: NextRequest) {
 
     // Snapshot the tenant's CURRENT revenue split onto this transaction (#496)
     // so a later plan change doesn't retroactively reprice it in the payouts
-    // computation — revenue_splits is super-admin-only under RLS, so this
-    // needs the admin client even though the rest of this route uses the
-    // user-scoped one.
+    // computation. The admin client is belt-and-braces, not a requirement: this
+    // comment used to say revenue_splits is "super-admin-only under RLS", which
+    // is wrong — the SELECT policy is `tenant_id = get_tenant_id() OR
+    // is_super_admin()`, so the user-scoped client could read it too (#512).
+    //
+    // Since #512 the DATABASE owns this column: the BEFORE INSERT trigger in
+    // 20260725110000_transaction_split_snapshot_backstop.sql recomputes it from
+    // the same table and ignores whatever we send, because the column is
+    // writable by any authenticated client (transactions RLS restricts rows, not
+    // columns) and a student-supplied value must not survive. This write is kept
+    // deliberately: it computes the identical number, and it keeps the rollback
+    // migration a safe lever — drop the trigger and this path still snapshots.
     const adminClientForSplit = createAdminClient()
     const { data: revenueSplit } = await adminClientForSplit
       .from('revenue_splits')

lib/payments/payouts-owed.ts

@@ -16,19 +16,31 @@
  * whole sum. A plan change (which rewrites `revenue_splits`) therefore only
  * affects transactions created after the change.
  *
- * That snapshot is also enforced in the database (issue #512): a
- * BEFORE INSERT OR UPDATE trigger on `transactions` fills it from
- * `revenue_splits` whenever a caller omits it, so a transaction-insert path
- * that forgets — or a webhook that sets `payment_provider` on an existing row
- * post-insert — cannot silently fall back to the current-split behaviour.
- * The trigger never overwrites a non-NULL snapshot, so the app-layer write
- * stays authoritative and history is never re-stamped.
+ * Since issue #512 that snapshot is OWNED BY THE DATABASE, not by the caller.
+ * `20260725110000_transaction_split_snapshot_backstop.sql` computes it from
+ * `revenue_splits` on INSERT and freezes it on UPDATE, so:
+ *
+ *   - a transaction-insert path that forgets it (four of the five do) can't
+ *     silently fall back to the current-split behaviour;
+ *   - a webhook that sets `payment_provider` on an existing row post-insert
+ *     (`lib/payments/webhook-dispatch.ts`) can't either;
+ *   - and no client can supply the number. `transactions` grants ALL to
+ *     `authenticated` and its RLS policies restrict which ROWS a user may
+ *     write, never which columns — so without the trigger a student could set
+ *     `school_percentage_snapshot: 100` on their own transaction and inflate
+ *     `grossOwed` below.
+ *
+ * The value never changes once written, so history is never re-stamped —
+ * re-stamping would be the #496 bug, not a fix for it.
  *
  * Transactions predating the snapshot column (`schoolPercentageSnapshot:
  * null`) still fall back to the tenant's current `schoolPercentage` — the
  * same behavior this module had before #496, so old data isn't retroactively
  * wrong either way. They were deliberately not backfilled: stamping today's
- * split onto history would manufacture the very repricing #496 removed.
+ * split onto history would manufacture the very repricing #496 removed. Such
+ * a row is snapshotted in exactly one situation — a provider activating it
+ * (its `payment_provider` changing), the one moment at which today's split is
+ * the honest answer for it.
  *
  * Balances are grouped PER CURRENCY (issue #497) — a tenant with both USD and
  * EUR sales owes two separate numbers, never one meaningless summed total.

supabase/migrations/20260725110000_transaction_split_snapshot_backstop.sql

@@ -7,50 +7,103 @@
 -- back to the tenant's CURRENT split, which is the #496 bug returning by a side
 -- door. The number is just quietly wrong.
 --
--- WHY INSERT *OR UPDATE*, not INSERT alone.
+-- This makes `transactions.school_percentage_snapshot` a DATABASE-OWNED column:
+-- the value is computed here, from revenue_splits, and is immutable once set.
+-- No caller writes it and no caller can tamper with it.
 --
--- The obvious backstop is BEFORE INSERT, on the reasoning that the unsnapshotted
--- insert sites are harmless because they leave payment_provider NULL (they write
--- payment_method instead) or use Stripe Connect, and getPayoutsOwed() filters on
--- platform-settled payment_provider values. That reasoning holds for the insert
--- and fails for the row's lifetime: payment_provider is also written by UPDATE,
--- after the row exists —
+-- WHY THE COLUMN HAS TO BE DATABASE-OWNED, NOT MERELY DEFAULTED.
+--
+-- The obvious backstop is "fill it in when the caller omits it, and let an
+-- explicit value win." That cannot be the rule here, because the explicit value
+-- can come from a caller we do not trust. `transactions` grants ALL to
+-- `authenticated` (20260126190500_lms_complete.sql), and the RLS policies in
+-- 20260313025318_rls_transactions.sql restrict only WHICH ROWS a user may write,
+-- never which columns:
+--
+--   CREATE POLICY "Users can create own transactions"
+--     ON public.transactions FOR INSERT TO authenticated
+--     WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id());
+--
+-- So a student can POST /rest/v1/transactions with school_percentage_snapshot:
+-- 100 — or UPDATE their own row to set it later — and that number flows straight
+-- into computeOwedBalances()'s grossOwed, inflating what the platform believes it
+-- owes their school. A backstop that defers to the explicit value would preserve
+-- exactly the write we most need to override. Hence: computed on INSERT, frozen
+-- on UPDATE.
+--
+-- (The column-privilege alternative — REVOKE INSERT, UPDATE (school_percentage_
+-- snapshot) — is a no-op while the table-level grant is held. Closing it that way
+-- means revoking table-level INSERT/UPDATE from `authenticated` and re-granting an
+-- explicit column list, which silently breaks every column omitted from that list.
+-- Owning the value in a trigger is the narrower change.)
+--
+-- WHY UPDATE, not INSERT alone.
+--
+-- payment_provider — the field getPayoutsOwed() filters on — is also written by
+-- UPDATE, after the row exists:
 --
 --   lib/payments/webhook-dispatch.ts:205  .update({ status, payment_provider })
 --   app/api/stripe/webhook/route.ts:406   same shape, Stripe subscriptions
 --
 -- dispatchBillingEvent is the shared activation path for PayPal, the unified
--- provider webhook (Lemon Squeezy et al), and Stripe. PayPal and Lemon Squeezy
--- are precisely the settlesToPlatformAccount providers the payout computation
--- reads. So a row can be inserted with payment_provider NULL, sail past a
--- BEFORE INSERT trigger with nothing to do, and only then be turned into a
--- platform-settled transaction by a webhook — arriving in the payout math with
--- a NULL snapshot. Covering UPDATE closes that path.
+-- provider webhook (Lemon Squeezy et al), and Stripe. PayPal and Lemon Squeezy are
+-- precisely the settlesToPlatformAccount providers the payout computation reads.
+-- So a row inserted before this migration, carrying a NULL snapshot, can be turned
+-- into a platform-settled transaction by a webhook and arrive in the payout math
+-- with nothing snapshotted. Covering UPDATE closes that path.
 --
 -- WHAT IT DELIBERATELY DOES NOT DO.
 --
--- 1. It never overwrites an existing snapshot. On UPDATE it acts only while the
---    value is still NULL, so a historical transaction is never re-stamped with a
---    newer split — re-stamping would BE the #496 bug, not a fix for it. The
---    app-layer write therefore stays authoritative; this only fills gaps.
+-- 1. It never re-stamps a snapshot that already exists. Re-stamping a historical
+--    transaction with a newer split would BE the #496 bug, not a fix for it. On
+--    UPDATE the previous value is restored verbatim, so the row is frozen from the
+--    moment it is first written — against tampering and against drift alike.
 --
--- 2. It does not backfill existing NULLs. Rows predating 20260724140000 are
---    documented (in that migration and in lib/payments/payouts-owed.ts) to fall
---    back to the tenant's current split. Backfilling would stamp TODAY's split
---    onto history and manufacture exactly the retroactive repricing #496
---    removed — and it would look authoritative while doing it, which is worse
---    than a documented fallback.
+-- 2. It does not backfill existing NULLs, and it does not fill them on unrelated
+--    updates. Rows predating 20260724140000 are documented (in that migration and
+--    in lib/payments/payouts-owed.ts) to fall back to the tenant's current split.
+--    Stamping today's split onto a year-old sale is the same retroactive repricing
+--    #496 removed — it does not stop being that because it happens one row at a
+--    time, when something incidental (a refund, an archive) touches the row. A
+--    legacy NULL is therefore filled in exactly ONE case: the row is being
+--    activated by a provider right now (payment_provider is changing), which is
+--    the only moment at which today's split is the honest answer.
 --
 -- A CHECK constraint was the alternative offered in the issue. It cannot read
 -- revenue_splits, and a check like `payment_provider IS NULL OR snapshot IS NOT
--- NULL` would hard-fail the webhook UPDATE above for any legacy NULL-snapshot
--- row a provider later activates — converting a quiet mispricing into a failed
+-- NULL` would hard-fail the webhook UPDATE above for any legacy NULL-snapshot row
+-- a provider later activates — converting a quiet mispricing into a failed
 -- activation and a lost sale. Filling the value beats rejecting the row.
+--
+-- The pre-existing after_transaction_insert / after_transaction_update triggers
+-- are both AFTER, so these BEFORE triggers compose with them without ordering
+-- concerns.
 
--- Mirrors DEFAULT_SCHOOL_PERCENTAGE in lib/payments/payouts-owed.ts, the 20%
--- platform default in app/api/stripe/create-payment-intent/route.ts, and the
--- row seeded by 20260216212440_create_revenue_infrastructure.sql. Used only
--- when a tenant has no revenue_splits row at all.
+-- The fallback mirrors DEFAULT_SCHOOL_PERCENTAGE in lib/payments/payouts-owed.ts,
+-- the revenue_splits.school_percentage column default, the 20% platform default in
+-- app/api/stripe/create-payment-intent/route.ts, and the row seeded by
+-- 20260216212440_create_revenue_infrastructure.sql. It is used only when a tenant
+-- has no revenue_splits row at all. (tests/unit/transaction-split-snapshot-
+-- backstop.test.ts fails if this number and the TypeScript constant drift apart.)
+--
+-- SECURITY DEFINER, deliberately — but not for the reason the surrounding code
+-- comments claim. `app/api/payments/checkout/route.ts` says revenue_splits is
+-- "super-admin-only under RLS"; it isn't. Despite its name, the SELECT policy from
+-- 20260216212440 is:
+--
+--   "Admins can view own revenue split": tenant_id = get_tenant_id() OR is_super_admin()
+--
+-- — i.e. every member of the tenant, students included, can read their school's
+-- split (verified against a local database). So an INVOKER function would in fact
+-- work today, because the transactions INSERT policy already pins
+-- NEW.tenant_id = get_tenant_id().
+--
+-- That is exactly why it must not be INVOKER. The correctness of this trigger would
+-- then rest on a SELECT policy whose own name says it should be narrower — and the
+-- day someone tightens it to match the name, this function stops seeing the row and
+-- silently stamps the 80 fallback onto every checkout for every tenant. Wrong
+-- payout numbers, no error, no failing test. DEFINER removes that coupling.
+-- search_path is pinned for the usual reason.
 CREATE OR REPLACE FUNCTION public.set_transaction_split_snapshot()
 RETURNS trigger
 LANGUAGE plpgsql
@@ -60,12 +113,28 @@ AS $function$
 DECLARE
   v_school_percentage numeric;
 BEGIN
-  -- Explicit app-layer value wins, always.
-  IF NEW.school_percentage_snapshot IS NOT NULL THEN
-    RETURN NEW;
+  IF TG_OP = 'UPDATE' THEN
+    -- Already snapshotted: frozen. Restoring OLD rather than comparing-and-
+    -- raising means a tampering UPDATE is neutralised instead of failing the
+    -- whole webhook that happened to carry it.
+    IF OLD.school_percentage_snapshot IS NOT NULL THEN
+      NEW.school_percentage_snapshot := OLD.school_percentage_snapshot;
+      RETURN NEW;
+    END IF;
+
+    -- Legacy NULL row. Fill it only if a provider is activating it right now;
+    -- otherwise keep the documented NULL fallback and discard whatever the
+    -- caller tried to put there.
+    IF NEW.payment_provider IS NOT DISTINCT FROM OLD.payment_provider THEN
+      NEW.school_percentage_snapshot := NULL;
+      RETURN NEW;
+    END IF;
   END IF;
 
+  -- INSERT, or the legacy-row activation case above. Either way the value is
+  -- computed here; anything the caller supplied is ignored.
   IF NEW.tenant_id IS NULL THEN
+    NEW.school_percentage_snapshot := NULL;
     RETURN NEW;
   END IF;
 
@@ -80,11 +149,31 @@ END;
 $function$;
 
 COMMENT ON FUNCTION public.set_transaction_split_snapshot() IS
-  'Backstop for #496 (issue #512): fills transactions.school_percentage_snapshot from revenue_splits when a caller omits it. Never overwrites a non-NULL snapshot, so historical rows are not repriced. Fires on UPDATE too, because payment_provider is set post-insert by the webhook activation path (lib/payments/webhook-dispatch.ts).';
+  'Backstop for #496 (issue #512): owns transactions.school_percentage_snapshot. Computes it from revenue_splits on INSERT and freezes it on UPDATE, so neither an insert path that forgets nor a client that tampers (transactions RLS restricts rows, not columns) can affect the payout computation. Legacy NULL rows are filled only when a provider activates them (payment_provider changing), never on an incidental update.';
 
+-- Named without the _insert suffix in an earlier revision of this migration.
 DROP TRIGGER IF EXISTS before_transaction_split_snapshot ON transactions;
-CREATE TRIGGER before_transaction_split_snapshot
-  BEFORE INSERT OR UPDATE ON transactions
+
+-- No WHEN clause: the point is to override whatever the caller sent, so this has
+-- to run on every insert. It is one index lookup on revenue_splits (UNIQUE
+-- (tenant_id), 20260216212440) against a table only written on payment events.
+DROP TRIGGER IF EXISTS before_transaction_split_snapshot_insert ON transactions;
+CREATE TRIGGER before_transaction_split_snapshot_insert
+  BEFORE INSERT ON transactions
+  FOR EACH ROW
+  EXECUTE FUNCTION public.set_transaction_split_snapshot();
+
+-- OLD cannot be referenced in a WHEN clause on a combined INSERT OR UPDATE
+-- trigger, which is why this is a second trigger rather than one. The clause
+-- keeps ordinary status-only updates from calling the function at all: it fires
+-- only when someone is touching the snapshot, or when a provider is activating
+-- the row.
+DROP TRIGGER IF EXISTS before_transaction_split_snapshot_update ON transactions;
+CREATE TRIGGER before_transaction_split_snapshot_update
+  BEFORE UPDATE ON transactions
   FOR EACH ROW
-  WHEN (NEW.school_percentage_snapshot IS NULL)
+  WHEN (
+    NEW.school_percentage_snapshot IS DISTINCT FROM OLD.school_percentage_snapshot
+    OR NEW.payment_provider IS DISTINCT FROM OLD.payment_provider
+  )
   EXECUTE FUNCTION public.set_transaction_split_snapshot();

supabase/migrations/rollback/20260725110000_transaction_split_snapshot_backstop.down.sql

@@ -1,15 +1,28 @@
 -- Rollback for 20260725110000_transaction_split_snapshot_backstop.sql (#512).
 --
--- Removes the database backstop that fills transactions.school_percentage_snapshot.
--- After this runs, only app/api/payments/checkout/route.ts sets the snapshot, and
--- any other path that creates a platform-settled transaction (or a webhook that
--- sets payment_provider on an existing row) silently falls back to the tenant's
--- CURRENT split in computeOwedBalances — i.e. the #496 retroactive-repricing bug
--- returns, quietly and with no failure. Only run this to unblock an incident.
---
--- Snapshots already written by the trigger are left in place: they record the
+-- Removes the database ownership of transactions.school_percentage_snapshot.
+-- After this runs:
+--
+--   1. Only app/api/payments/checkout/route.ts sets the snapshot. Any other path
+--      that creates a platform-settled transaction — or a webhook that sets
+--      payment_provider on an existing row — silently falls back to the tenant's
+--      CURRENT split in computeOwedBalances, i.e. the #496 retroactive-repricing
+--      bug returns, quietly and with no failure.
+--   2. The column becomes client-writable again. `transactions` grants ALL to
+--      `authenticated` and its RLS policies restrict rows, not columns, so a
+--      student can set school_percentage_snapshot on their own transaction and
+--      inflate what the platform believes it owes their school.
+--
+-- (2) is the reason to treat this as an incident-only lever rather than a clean
+-- revert. If you run it, watch /platform/payouts for balances that move without a
+-- corresponding sale.
+--
+-- Snapshots already written by the triggers are left in place: they record the
 -- split that was genuinely in effect, and clearing them would reintroduce the
 -- fallback for those rows too.
 
+DROP TRIGGER IF EXISTS before_transaction_split_snapshot_insert ON transactions;
+DROP TRIGGER IF EXISTS before_transaction_split_snapshot_update ON transactions;
+-- Name used by the first revision of the up-migration; harmless if absent.
 DROP TRIGGER IF EXISTS before_transaction_split_snapshot ON transactions;
 DROP FUNCTION IF EXISTS public.set_transaction_split_snapshot();