|
| 1 | +-- Issue #512 — database backstop for the revenue-split snapshot. |
| 2 | +-- |
| 3 | +-- #496 fixed retroactive repricing by snapshotting revenue_splits.school_percentage |
| 4 | +-- onto each transaction, but that snapshot is written at exactly ONE call site |
| 5 | +-- (app/api/payments/checkout/route.ts). Every other transaction-insert path |
| 6 | +-- omits it, and nothing fails when they do: computeOwedBalances() silently falls |
| 7 | +-- back to the tenant's CURRENT split, which is the #496 bug returning by a side |
| 8 | +-- door. The number is just quietly wrong. |
| 9 | +-- |
| 10 | +-- This makes `transactions.school_percentage_snapshot` a DATABASE-OWNED column: |
| 11 | +-- the value is computed here, from revenue_splits, and is immutable once set. |
| 12 | +-- No caller writes it and no caller can tamper with it. |
| 13 | +-- |
| 14 | +-- WHY THE COLUMN HAS TO BE DATABASE-OWNED, NOT MERELY DEFAULTED. |
| 15 | +-- |
| 16 | +-- The obvious backstop is "fill it in when the caller omits it, and let an |
| 17 | +-- explicit value win." That cannot be the rule here, because the explicit value |
| 18 | +-- can come from a caller we do not trust. `transactions` grants ALL to |
| 19 | +-- `authenticated` (20260126190500_lms_complete.sql), and the RLS policies in |
| 20 | +-- 20260313025318_rls_transactions.sql restrict only WHICH ROWS a user may write, |
| 21 | +-- never which columns: |
| 22 | +-- |
| 23 | +-- CREATE POLICY "Users can create own transactions" |
| 24 | +-- ON public.transactions FOR INSERT TO authenticated |
| 25 | +-- WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id()); |
| 26 | +-- |
| 27 | +-- So a student can POST /rest/v1/transactions with school_percentage_snapshot: |
| 28 | +-- 100 — or UPDATE their own row to set it later — and that number flows straight |
| 29 | +-- into computeOwedBalances()'s grossOwed, inflating what the platform believes it |
| 30 | +-- owes their school. A backstop that defers to the explicit value would preserve |
| 31 | +-- exactly the write we most need to override. Hence: computed on INSERT, frozen |
| 32 | +-- on UPDATE. |
| 33 | +-- |
| 34 | +-- (The column-privilege alternative — REVOKE INSERT, UPDATE (school_percentage_ |
| 35 | +-- snapshot) — is a no-op while the table-level grant is held. Closing it that way |
| 36 | +-- means revoking table-level INSERT/UPDATE from `authenticated` and re-granting an |
| 37 | +-- explicit column list, which silently breaks every column omitted from that list. |
| 38 | +-- Owning the value in a trigger is the narrower change.) |
| 39 | +-- |
| 40 | +-- WHY UPDATE, not INSERT alone. |
| 41 | +-- |
| 42 | +-- payment_provider — the field getPayoutsOwed() filters on — is also written by |
| 43 | +-- UPDATE, after the row exists: |
| 44 | +-- |
| 45 | +-- lib/payments/webhook-dispatch.ts:205 .update({ status, payment_provider }) |
| 46 | +-- app/api/stripe/webhook/route.ts:406 same shape, Stripe subscriptions |
| 47 | +-- |
| 48 | +-- dispatchBillingEvent is the shared activation path for PayPal, the unified |
| 49 | +-- provider webhook (Lemon Squeezy et al), and Stripe. PayPal and Lemon Squeezy are |
| 50 | +-- precisely the settlesToPlatformAccount providers the payout computation reads. |
| 51 | +-- So a row inserted before this migration, carrying a NULL snapshot, can be turned |
| 52 | +-- into a platform-settled transaction by a webhook and arrive in the payout math |
| 53 | +-- with nothing snapshotted. Covering UPDATE closes that path. |
| 54 | +-- |
| 55 | +-- WHAT IT DELIBERATELY DOES NOT DO. |
| 56 | +-- |
| 57 | +-- 1. It never re-stamps a snapshot that already exists. Re-stamping a historical |
| 58 | +-- transaction with a newer split would BE the #496 bug, not a fix for it. On |
| 59 | +-- UPDATE the previous value is restored verbatim, so the row is frozen from the |
| 60 | +-- moment it is first written — against tampering and against drift alike. |
| 61 | +-- |
| 62 | +-- 2. It does not backfill existing NULLs, and it does not fill them on unrelated |
| 63 | +-- updates. Rows predating 20260724140000 are documented (in that migration and |
| 64 | +-- in lib/payments/payouts-owed.ts) to fall back to the tenant's current split. |
| 65 | +-- Stamping today's split onto a year-old sale is the same retroactive repricing |
| 66 | +-- #496 removed — it does not stop being that because it happens one row at a |
| 67 | +-- time, when something incidental (a refund, an archive) touches the row. A |
| 68 | +-- legacy NULL is therefore filled in exactly ONE case: the row is being |
| 69 | +-- activated by a provider right now (payment_provider is changing), which is |
| 70 | +-- the only moment at which today's split is the honest answer. |
| 71 | +-- |
| 72 | +-- A CHECK constraint was the alternative offered in the issue. It cannot read |
| 73 | +-- revenue_splits, and a check like `payment_provider IS NULL OR snapshot IS NOT |
| 74 | +-- NULL` would hard-fail the webhook UPDATE above for any legacy NULL-snapshot row |
| 75 | +-- a provider later activates — converting a quiet mispricing into a failed |
| 76 | +-- activation and a lost sale. Filling the value beats rejecting the row. |
| 77 | +-- |
| 78 | +-- The pre-existing after_transaction_insert / after_transaction_update triggers |
| 79 | +-- are both AFTER, so these BEFORE triggers compose with them without ordering |
| 80 | +-- concerns. |
| 81 | + |
| 82 | +-- The fallback mirrors DEFAULT_SCHOOL_PERCENTAGE in lib/payments/payouts-owed.ts, |
| 83 | +-- the revenue_splits.school_percentage column default, the 20% platform default in |
| 84 | +-- app/api/stripe/create-payment-intent/route.ts, and the row seeded by |
| 85 | +-- 20260216212440_create_revenue_infrastructure.sql. It is used only when a tenant |
| 86 | +-- has no revenue_splits row at all. (tests/unit/transaction-split-snapshot- |
| 87 | +-- backstop.test.ts fails if this number and the TypeScript constant drift apart.) |
| 88 | +-- |
| 89 | +-- SECURITY DEFINER, deliberately — but not for the reason the surrounding code |
| 90 | +-- comments claim. `app/api/payments/checkout/route.ts` says revenue_splits is |
| 91 | +-- "super-admin-only under RLS"; it isn't. Despite its name, the SELECT policy from |
| 92 | +-- 20260216212440 is: |
| 93 | +-- |
| 94 | +-- "Admins can view own revenue split": tenant_id = get_tenant_id() OR is_super_admin() |
| 95 | +-- |
| 96 | +-- — i.e. every member of the tenant, students included, can read their school's |
| 97 | +-- split (verified against a local database). So an INVOKER function would in fact |
| 98 | +-- work today, because the transactions INSERT policy already pins |
| 99 | +-- NEW.tenant_id = get_tenant_id(). |
| 100 | +-- |
| 101 | +-- That is exactly why it must not be INVOKER. The correctness of this trigger would |
| 102 | +-- then rest on a SELECT policy whose own name says it should be narrower — and the |
| 103 | +-- day someone tightens it to match the name, this function stops seeing the row and |
| 104 | +-- silently stamps the 80 fallback onto every checkout for every tenant. Wrong |
| 105 | +-- payout numbers, no error, no failing test. DEFINER removes that coupling. |
| 106 | +-- search_path is pinned for the usual reason. |
| 107 | +CREATE OR REPLACE FUNCTION public.set_transaction_split_snapshot() |
| 108 | +RETURNS trigger |
| 109 | +LANGUAGE plpgsql |
| 110 | +SECURITY DEFINER |
| 111 | +SET search_path TO 'public' |
| 112 | +AS $function$ |
| 113 | +DECLARE |
| 114 | + v_school_percentage numeric; |
| 115 | +BEGIN |
| 116 | + IF TG_OP = 'UPDATE' THEN |
| 117 | + -- Already snapshotted: frozen. Restoring OLD rather than comparing-and- |
| 118 | + -- raising means a tampering UPDATE is neutralised instead of failing the |
| 119 | + -- whole webhook that happened to carry it. |
| 120 | + IF OLD.school_percentage_snapshot IS NOT NULL THEN |
| 121 | + NEW.school_percentage_snapshot := OLD.school_percentage_snapshot; |
| 122 | + RETURN NEW; |
| 123 | + END IF; |
| 124 | + |
| 125 | + -- Legacy NULL row. Fill it only if a provider is activating it right now; |
| 126 | + -- otherwise keep the documented NULL fallback and discard whatever the |
| 127 | + -- caller tried to put there. |
| 128 | + IF NEW.payment_provider IS NOT DISTINCT FROM OLD.payment_provider THEN |
| 129 | + NEW.school_percentage_snapshot := NULL; |
| 130 | + RETURN NEW; |
| 131 | + END IF; |
| 132 | + END IF; |
| 133 | + |
| 134 | + -- INSERT, or the legacy-row activation case above. Either way the value is |
| 135 | + -- computed here; anything the caller supplied is ignored. |
| 136 | + IF NEW.tenant_id IS NULL THEN |
| 137 | + NEW.school_percentage_snapshot := NULL; |
| 138 | + RETURN NEW; |
| 139 | + END IF; |
| 140 | + |
| 141 | + SELECT school_percentage INTO v_school_percentage |
| 142 | + FROM revenue_splits |
| 143 | + WHERE tenant_id = NEW.tenant_id; |
| 144 | + |
| 145 | + NEW.school_percentage_snapshot := COALESCE(v_school_percentage, 80); |
| 146 | + |
| 147 | + RETURN NEW; |
| 148 | +END; |
| 149 | +$function$; |
| 150 | + |
| 151 | +COMMENT ON FUNCTION public.set_transaction_split_snapshot() IS |
| 152 | + '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.'; |
| 153 | + |
| 154 | +-- Named without the _insert suffix in an earlier revision of this migration. |
| 155 | +DROP TRIGGER IF EXISTS before_transaction_split_snapshot ON transactions; |
| 156 | + |
| 157 | +-- No WHEN clause: the point is to override whatever the caller sent, so this has |
| 158 | +-- to run on every insert. It is one index lookup on revenue_splits (UNIQUE |
| 159 | +-- (tenant_id), 20260216212440) against a table only written on payment events. |
| 160 | +DROP TRIGGER IF EXISTS before_transaction_split_snapshot_insert ON transactions; |
| 161 | +CREATE TRIGGER before_transaction_split_snapshot_insert |
| 162 | + BEFORE INSERT ON transactions |
| 163 | + FOR EACH ROW |
| 164 | + EXECUTE FUNCTION public.set_transaction_split_snapshot(); |
| 165 | + |
| 166 | +-- OLD cannot be referenced in a WHEN clause on a combined INSERT OR UPDATE |
| 167 | +-- trigger, which is why this is a second trigger rather than one. The clause |
| 168 | +-- keeps ordinary status-only updates from calling the function at all: it fires |
| 169 | +-- only when someone is touching the snapshot, or when a provider is activating |
| 170 | +-- the row. |
| 171 | +DROP TRIGGER IF EXISTS before_transaction_split_snapshot_update ON transactions; |
| 172 | +CREATE TRIGGER before_transaction_split_snapshot_update |
| 173 | + BEFORE UPDATE ON transactions |
| 174 | + FOR EACH ROW |
| 175 | + WHEN ( |
| 176 | + NEW.school_percentage_snapshot IS DISTINCT FROM OLD.school_percentage_snapshot |
| 177 | + OR NEW.payment_provider IS DISTINCT FROM OLD.payment_provider |
| 178 | + ) |
| 179 | + EXECUTE FUNCTION public.set_transaction_split_snapshot(); |
0 commit comments