|
| 1 | +-- Issue #538 — an untrusted caller may not open a transaction at all. |
| 2 | +-- |
| 3 | +-- #528 (20260725170000) established the invariant "an untrusted caller may open |
| 4 | +-- a transaction, but may not declare it paid": the INSERT policy pins |
| 5 | +-- `status = 'pending'`, and table-level UPDATE is revoked in favour of a |
| 6 | +-- three-column grant. It deliberately said nothing about WHAT a pending row |
| 7 | +-- claims it is owed, and the Solana settlement columns are exactly that: |
| 8 | +-- |
| 9 | +-- settlement_currency text -- 'sol' | 'usdc' |
| 10 | +-- settlement_base bigint -- lamports / USDC base units |
| 11 | +-- settlement_mint text -- SPL mint; NULL = native SOL |
| 12 | +-- settlement_sol_usd numeric(20,8) -- locked quote (audit) |
| 13 | +-- |
| 14 | +-- The on-chain payment is verified against the row's OWN settlement_base |
| 15 | +-- (lib/payments/solana-reconcile.ts:104 → verifySplitTransfer({ totalBase })), |
| 16 | +-- and the Solana Pay transaction handed to the wallet is built from it too |
| 17 | +-- (app/api/payments/solana/tx/route.ts:119). So a student could skip the |
| 18 | +-- checkout route and POST /rest/v1/transactions directly: |
| 19 | +-- |
| 20 | +-- { user_id: <self>, tenant_id: <own tenant>, product_id: <a $49 course>, |
| 21 | +-- amount: 100, status: 'pending', payment_provider: 'solana', |
| 22 | +-- settlement_currency: 'sol', settlement_base: 1 } |
| 23 | +-- |
| 24 | +-- The row is 'pending', so it passed the #528 policy cleanly (reproduced against |
| 25 | +-- a local database: the insert above succeeded as role `authenticated` with |
| 26 | +-- alice's claims). They then pay 1 lamport, /api/payments/solana/verify finds |
| 27 | +-- the transfer satisfies settlement_base, and flips the row to 'successful' on |
| 28 | +-- the service-role client — firing trigger_manage_transactions and granting the |
| 29 | +-- entitlement. Full course access for a fraction of a cent, through the |
| 30 | +-- legitimate payment path. |
| 31 | +-- |
| 32 | +-- WHICH COLUMNS ARE LOAD-BEARING (established before choosing a control). |
| 33 | +-- |
| 34 | +-- settlement_base the figure compared on-chain, and the amount the QR |
| 35 | +-- asks the wallet for. Load-bearing in both paths. |
| 36 | +-- settlement_currency selects `decimals` (6 for usdc, else 9) — wrong |
| 37 | +-- decimals move the verified amount by 10^3. |
| 38 | +-- settlement_mint selects which SPL token counts (NULL = native SOL); a |
| 39 | +-- caller-chosen mint is one the attacker can mint freely. |
| 40 | +-- settlement_sol_usd nothing reads it. Audit record of the locked quote only. |
| 41 | +-- amount the fallback both paths use when settlement_base IS |
| 42 | +-- NULL (legacy rows), and the figure getPayoutsOwed() |
| 43 | +-- sums. No webhook compares it against what the provider |
| 44 | +-- actually charged (lib/payments/webhook-dispatch.ts |
| 45 | +-- never reads it), so for Lemon Squeezy / PayPal / |
| 46 | +-- Binance an under-quoted `amount` does not buy access — |
| 47 | +-- those charge from their own catalogue — it understates |
| 48 | +-- the school's payout instead. |
| 49 | +-- |
| 50 | +-- THE ROOT CAUSE IS THE GRANT, NOT THE COLUMNS. |
| 51 | +-- |
| 52 | +-- #528 could not narrow INSERT columns because the table-level INSERT grant to |
| 53 | +-- `authenticated` is what two API routes rely on. But nothing in the BROWSER |
| 54 | +-- inserts into `transactions` — every legitimate insert already happens on the |
| 55 | +-- server, and the complete list is: |
| 56 | +-- |
| 57 | +-- app/api/payments/checkout/route.ts user-scoped client |
| 58 | +-- app/api/stripe/create-payment-intent/route.ts user-scoped client |
| 59 | +-- app/[locale]/(public)/checkout/actions.ts admin client (since #528) |
| 60 | +-- app/actions/payment-requests.ts:359 admin client |
| 61 | +-- lib/payments/webhook-dispatch.ts, the Stripe webhook, the Solana/Binance |
| 62 | +-- reconcilers, both cron routes, scripts/*, tests/playwright/* |
| 63 | +-- service role |
| 64 | +-- grant_free_subscription() SECURITY DEFINER |
| 65 | +-- |
| 66 | +-- The first two are the only reason the grant exists, and both already derive |
| 67 | +-- every financial field server-side from trusted sources: `amount`, `currency` |
| 68 | +-- and `payment_provider` come from tenant-scoped reads of `products` / `plans`, |
| 69 | +-- and the settlement figures from `getSolUsdPrice()` — a live off-platform quote. |
| 70 | +-- Moving those two inserts onto the admin client (this commit) leaves the grant |
| 71 | +-- with no legitimate user, so it goes. |
| 72 | +-- |
| 73 | +-- WHY NOT THE OTHER TWO DIRECTIONS THE ISSUE OFFERS. |
| 74 | +-- |
| 75 | +-- A BEFORE INSERT trigger recomputing the values (the #512 pattern for |
| 76 | +-- school_percentage_snapshot) would overwrite legitimate ones: completing a |
| 77 | +-- payment request inserts the admin-confirmed `payment_amount`, and |
| 78 | +-- grant_free_subscription() inserts a zero-amount row for its plan. Rewriting |
| 79 | +-- or rejecting those converts a quiet mispricing into a broken payment path. |
| 80 | +-- It also cannot help with native SOL at all: settlement_base is derived from |
| 81 | +-- a live SOL/USD quote a trigger cannot obtain. |
| 82 | +-- |
| 83 | +-- A SECURITY DEFINER RPC has the same blind spot from the other side. It |
| 84 | +-- cannot fetch the quote either, so it would have to accept the rate as a |
| 85 | +-- caller parameter — and the rate is precisely the value that must not be |
| 86 | +-- caller-supplied, since settlement_base is computed from it. That moves the |
| 87 | +-- vulnerability rather than closing it. Inserting on the admin client keeps |
| 88 | +-- the quote from crossing the client boundary in either direction. |
| 89 | +-- |
| 90 | +-- WHAT THIS LEAVES REACHABLE. Nothing, for an untrusted caller: INSERT is denied |
| 91 | +-- outright here, and #528 already made `amount` and all four settlement_* |
| 92 | +-- columns unwritable on UPDATE via the column grant. Both halves are needed — |
| 93 | +-- either alone leaves a route to the same row. |
| 94 | + |
| 95 | +BEGIN; |
| 96 | + |
| 97 | +-- --------------------------------------------------------------------------- |
| 98 | +-- (a) Revoke INSERT. `POST /rest/v1/transactions` now fails with |
| 99 | +-- `permission denied for table transactions`. |
| 100 | +-- --------------------------------------------------------------------------- |
| 101 | +-- |
| 102 | +-- A future user-scoped insert will fail loudly with that error rather than |
| 103 | +-- silently writing a caller-priced row. That is the intended trade-off on a |
| 104 | +-- payments table, and the same one #528 accepted for UPDATE. |
| 105 | +-- |
| 106 | +-- `anon` is included for hygiene rather than to close a live path: the RLS |
| 107 | +-- policies on this table are all `TO authenticated`, so an anonymous insert was |
| 108 | +-- already rejected. Holding a grant that no policy can satisfy only invites a |
| 109 | +-- later policy widening to become an unnoticed hole. |
| 110 | +-- |
| 111 | +-- Untouched, deliberately: service_role (every webhook / reconciler / cron and |
| 112 | +-- the four admin-client call sites), and the table owner — SECURITY DEFINER |
| 113 | +-- functions run as the owner, so grant_free_subscription() still inserts its |
| 114 | +-- zero-amount 'successful' row for the free-plan path. |
| 115 | + |
| 116 | +REVOKE INSERT ON TABLE public.transactions FROM authenticated, anon; |
| 117 | + |
| 118 | +-- The sequence grant is now unusable for its only purpose. Left in place: it is |
| 119 | +-- harmless without INSERT, and revoking it would add a second thing to restore |
| 120 | +-- if this migration is ever rolled back. |
| 121 | + |
| 122 | +-- --------------------------------------------------------------------------- |
| 123 | +-- (b) Keep #528's INSERT policy, and pin the settlement columns NULL in it. |
| 124 | +-- --------------------------------------------------------------------------- |
| 125 | +-- |
| 126 | +-- This policy is unreachable while (a) holds — no grant, no policy evaluation. |
| 127 | +-- It is retained and tightened because (a) is a single `GRANT INSERT` away from |
| 128 | +-- being undone: a rollback of this migration, a future schema dump that |
| 129 | +-- re-applies the original `GRANT ALL ON TABLE transactions TO authenticated` |
| 130 | +-- from 20260126190500_lms_complete.sql, or someone resolving a `permission |
| 131 | +-- denied` error the direct way. In any of those cases the under-quote in this |
| 132 | +-- issue stays impossible, because a user-scoped INSERT can no longer carry a |
| 133 | +-- settlement claim at all — the columns must be NULL, which sends both Solana |
| 134 | +-- paths down their legacy fallback (verify against `amount`) instead of |
| 135 | +-- honouring a caller-supplied figure. |
| 136 | +-- |
| 137 | +-- Note what the policy still cannot express: `amount` matching the product's |
| 138 | +-- price. A WITH CHECK could subquery `products`/`plans` for it, but that would |
| 139 | +-- couple this policy to those tables' SELECT policies — the coupling #512 |
| 140 | +-- explicitly refused for the same reason (tighten the other policy, and this one |
| 141 | +-- starts silently rejecting or accepting the wrong thing). With the grant |
| 142 | +-- revoked, `amount` is server-owned; if the grant is ever restored, `amount` |
| 143 | +-- becomes caller-controlled again and that is a payout-accuracy problem to |
| 144 | +-- reopen, not a course-access one. |
| 145 | + |
| 146 | +DROP POLICY IF EXISTS "Users can create own transactions" ON public.transactions; |
| 147 | + |
| 148 | +CREATE POLICY "Users can create own transactions" |
| 149 | + ON public.transactions FOR INSERT TO authenticated |
| 150 | + WITH CHECK ( |
| 151 | + (SELECT auth.uid()) = user_id |
| 152 | + AND tenant_id = (SELECT get_tenant_id()) |
| 153 | + AND status = 'pending' |
| 154 | + AND settlement_currency IS NULL |
| 155 | + AND settlement_base IS NULL |
| 156 | + AND settlement_mint IS NULL |
| 157 | + AND settlement_sol_usd IS NULL |
| 158 | + ); |
| 159 | + |
| 160 | +COMMENT ON COLUMN public.transactions.settlement_base IS |
| 161 | + 'Locked integer base amount the wallet must pay: lamports (sol) or token base units (usdc). Set once at checkout; /tx and /verify read this, never re-quote. Issue #538: server-owned — the on-chain payment is verified against this figure, so it is written only by a service-role client (app/api/payments/checkout/route.ts) and is unwritable by `authenticated` on both INSERT (grant revoked, 20260725180000) and UPDATE (column grant, 20260725170000).'; |
| 162 | + |
| 163 | +COMMIT; |
0 commit comments