fix(security): pin transaction status and columns for user-scoped writes (#528)

.claude/gh-workflow.config.json

@@ -1,5 +1,5 @@
 {
   "projectBoard": null,
-  "prTemplate": null,
+  "prTemplate": ".github/pull_request_template.md",
   "branchConvention": "<type>/<slug>-<N>"
 }

app/[locale]/(public)/checkout/actions.ts

@@ -1,6 +1,7 @@
 'use server';
 
 import { createClient as createServerClient } from '@/lib/supabase/server';
+import { createAdminClient } from '@/lib/supabase/admin';
 import { revalidatePath } from 'next/cache';
 import { getCurrentUserId, getCurrentTenantId } from '@/lib/supabase/tenant'
 import { headers } from 'next/headers';
@@ -26,6 +27,16 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
     }
     const tenantId = await getCurrentTenantId();
 
+    // Since #528 the transactions INSERT policy pins `status = 'pending'` for the
+    // user-scoped client, because a student could otherwise POST a 'successful'
+    // row straight to PostgREST and let the after_transaction_insert trigger hand
+    // them the course. This action legitimately needs to write 'successful', so
+    // the two inserts below use the admin client. Per CLAUDE.md, that shifts the
+    // tenant check to us: `userId` comes from the session, and the product/plan
+    // lookups that follow are tenant-scoped reads on the USER-scoped client, so a
+    // caller can only reach rows their own tenant owns.
+    const adminClient = createAdminClient();
+
     try {
         if (courseId) {
             // Get product for this course (pick first match, scoped to tenant)
@@ -49,7 +60,7 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
             // Create the transaction. The after_transaction_insert trigger
             // enrolls the user (entitlements + enrollment record) — no explicit
             // RPC call here, the trigger is the single enrollment path.
-            const { data: transaction, error: txError } = await supabase
+            const { data: transaction, error: txError } = await adminClient
                 .from('transactions')
                 .insert({
                     user_id: userId,
@@ -97,7 +108,7 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
             // Create the transaction. The after_transaction_insert trigger
             // creates the subscription + entitlements (or extends an existing
             // subscription on renewal).
-            const { data: transaction, error: txError } = await supabase
+            const { data: transaction, error: txError } = await adminClient
                 .from('transactions')
                 .insert({
                     user_id: userId,

supabase/migrations/20260725170000_transactions_column_hardening.sql

@@ -0,0 +1,141 @@
+-- Issue #528 — transactions RLS restricts rows but not columns.
+--
+-- `transactions` grants table-level INSERT/UPDATE to `authenticated`
+-- (20260126190500_lms_complete.sql), and the RLS policies in
+-- 20260313025318_rls_transactions.sql constrain 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 an authenticated student can POST /rest/v1/transactions with any column
+-- values they like, provided user_id is theirs and tenant_id is their current
+-- tenant. Two consequences, both reached without touching a payment provider:
+--
+--   1. FREE ENROLMENT. `trigger_manage_transactions` (AFTER INSERT/UPDATE) calls
+--      enroll_user(NEW.user_id, NEW.product_id) whenever product_id is set and
+--      status = 'successful', and handle_new_subscription(...) on the plan_id
+--      branch. A self-inserted 'successful' row therefore writes `entitlements`
+--      — i.e. paid course access — with no money involved.
+--   2. FABRICATED PAYOUT LIABILITY. getPayoutsOwed() (app/actions/platform/
+--      payouts.ts:52) reads transactions WHERE status IN ('successful','refunded')
+--      AND payment_provider IN (paypal, lemonsqueezy, binance). A self-inserted
+--      row carrying one of those providers lands in grossOwed, so the platform's
+--      own payout dashboard reports it owes a school money for a sale that never
+--      happened.
+--
+-- #512 (20260725110000) closed `school_percentage_snapshot` specifically, by
+-- making the database compute and freeze that one column. The rest of the row is
+-- still caller-controlled. This migration closes the two impacts above.
+--
+-- THE SHAPE OF THE FIX.
+--
+-- Both impacts are gated on the SAME thing: a row reaching 'successful' (or, for
+-- the payout half, 'refunded'). Neither is reachable from a 'pending' row. So the
+-- invariant worth enforcing is narrow and checkable — an untrusted caller may
+-- open a transaction, but may not declare it paid. Only the webhook, verify and
+-- reconcile paths, which all run on the service-role client and bypass RLS
+-- entirely, may do that.
+--
+-- Two layers, because they defend against different mechanics:
+--
+--   (a) INSERT policy pins status = 'pending'. Closes the self-INSERT path.
+--
+--   (b) UPDATE is restricted to the three columns a legitimate user-scoped caller
+--       actually writes, and the policy pins the reachable statuses. Closes the
+--       self-ESCALATE path — owning a genuine pending row is otherwise a second
+--       route to the same place, since after_transaction_update fires the very
+--       same trigger.
+--
+-- WHY (b) NEEDS A COLUMN GRANT AND NOT JUST A POLICY.
+--
+-- An RLS WITH CHECK sees only the NEW row; it cannot compare NEW to OLD. So a
+-- policy alone cannot make a column immutable. Without the column grant a student
+-- can open a legitimate pending transaction for a cheap product, PATCH its
+-- product_id to an expensive one, pay the cheap price, and let the provider's
+-- webhook flip the row to 'successful' — at which point the trigger enrols them
+-- in the expensive course. Column privileges close that mechanically, at the
+-- grant layer, without needing OLD.
+--
+-- Note the ordering requirement: a bare REVOKE ... (column) is a NO-OP while the
+-- table-level grant is held, so table-level UPDATE must be revoked FIRST and the
+-- allowed columns re-granted after.
+
+BEGIN;
+
+-- ---------------------------------------------------------------------------
+-- (a) INSERT — an untrusted caller may only open a PENDING transaction.
+-- ---------------------------------------------------------------------------
+--
+-- Permissive policies OR together, so this has to replace the existing policy
+-- rather than sit alongside it: an additional policy would widen the grant, not
+-- narrow it.
+--
+-- Callers unaffected (verified against every application write site):
+--   app/api/payments/checkout/route.ts:233          status: 'pending'
+--   app/api/stripe/create-payment-intent/route.ts:140  status: 'pending'
+--
+-- Callers that bypass RLS and are therefore untouched: every createAdminClient()
+-- / service-role path (the Stripe webhook, lib/payments/webhook-dispatch.ts, the
+-- Solana and Binance reconcilers, the two cron routes, payment-requests.ts,
+-- admin/binance-personal.ts), plus grant_free_subscription() — SECURITY DEFINER
+-- runs as the table owner and the table is not FORCE ROW LEVEL SECURITY, so the
+-- free-plan path still inserts its zero-amount 'successful' row.
+
+DROP POLICY IF EXISTS "Users can create own transactions" ON public.transactions;
+
+CREATE POLICY "Users can create own transactions"
+  ON public.transactions FOR INSERT TO authenticated
+  WITH CHECK (
+    (SELECT auth.uid()) = user_id
+    AND tenant_id = (SELECT get_tenant_id())
+    AND status = 'pending'
+  );
+
+-- ---------------------------------------------------------------------------
+-- (b) UPDATE — narrow to the three columns a user-scoped caller actually writes.
+-- ---------------------------------------------------------------------------
+--
+-- The complete set of user-scoped UPDATE sites and the columns they write:
+--   app/api/payments/checkout/route.ts:320            provider_subscription_id
+--   app/api/payments/checkout/route.ts:337            status -> 'failed'
+--   app/api/stripe/create-payment-intent/route.ts:187 provider_subscription_id
+--   app/api/stripe/create-payment-intent/route.ts:203 status -> 'failed'
+--   app/api/stripe/create-payment-intent/route.ts:241 stripe_payment_intent_id
+--
+-- Everything else — amount, currency, product_id, plan_id, payment_provider,
+-- user_id, tenant_id, the settlement_* columns, provider_charge_id,
+-- provider_metadata, payment_method, transaction_date, school_percentage_snapshot
+-- — becomes unwritable by `authenticated`. A future user-scoped update of any of
+-- them fails loudly with `permission denied for column`, which is the intended
+-- trade-off: this is a payments table, and a silent success is the worse outcome.
+
+REVOKE UPDATE ON TABLE public.transactions FROM authenticated;
+
+GRANT UPDATE (status, provider_subscription_id, stripe_payment_intent_id)
+  ON TABLE public.transactions TO authenticated;
+
+-- USING pins the rows a user may touch at all to their own still-pending ones;
+-- every legitimate user-scoped update above runs against a row created moments
+-- earlier in the same request, so it is always still 'pending'. WITH CHECK pins
+-- where that row may land: 'pending' (a metadata-only update leaves it alone) or
+-- 'failed' (the provider-session rollback). 'successful' and 'refunded' — the two
+-- statuses the trigger and the payout query care about — are unreachable.
+
+DROP POLICY IF EXISTS "Users can update own transactions" ON public.transactions;
+
+CREATE POLICY "Users can update own transactions"
+  ON public.transactions FOR UPDATE TO authenticated
+  USING (
+    (SELECT auth.uid()) = user_id
+    AND tenant_id = (SELECT get_tenant_id())
+    AND status = 'pending'
+  )
+  WITH CHECK (
+    (SELECT auth.uid()) = user_id
+    AND tenant_id = (SELECT get_tenant_id())
+    AND status IN ('pending', 'failed')
+  );
+
+COMMIT;

supabase/migrations/rollback/20260725170000_transactions_column_hardening.down.sql

@@ -0,0 +1,33 @@
+-- Rollback for 20260725170000_transactions_column_hardening.sql (issue #528).
+--
+-- NOT APPLIED BY DEFAULT. Running this restores the policies and grants exactly
+-- as 20260313025318_rls_transactions.sql left them — which means it REOPENS both
+-- impacts described in #528: an authenticated student regains the ability to
+-- self-insert a 'successful' transaction (free course access via the
+-- after_transaction_insert trigger) and to fabricate a platform-settled sale that
+-- inflates getPayoutsOwed(). Only run it if the column grant has broken a
+-- legitimate write path and the fix has to be re-cut.
+
+BEGIN;
+
+DROP POLICY IF EXISTS "Users can create own transactions" ON public.transactions;
+
+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());
+
+-- Drop the column-level grants before restoring the table-level one, so no
+-- narrower privilege is left dangling behind the broad grant.
+REVOKE UPDATE (status, provider_subscription_id, stripe_payment_intent_id)
+  ON TABLE public.transactions FROM authenticated;
+
+GRANT UPDATE ON TABLE public.transactions TO authenticated;
+
+DROP POLICY IF EXISTS "Users can update own transactions" ON public.transactions;
+
+CREATE POLICY "Users can update own transactions"
+  ON public.transactions FOR UPDATE TO authenticated
+  USING (auth.uid() = user_id AND tenant_id = get_tenant_id())
+  WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id());
+
+COMMIT;

supabase/snippets/verify_transactions_column_hardening.sql

@@ -0,0 +1,83 @@
+-- Verify 20260725170000_transactions_column_hardening.sql (issue #528).
+--
+-- Asserts the FINAL grant + policy state on public.transactions rather than the
+-- text of the migration, so it also catches a later migration re-widening things.
+-- Every row should report PASS. Run with:
+--
+--   docker exec -i supabase_db_lms-front psql -U postgres -d postgres \
+--     -f - < supabase/snippets/verify_transactions_column_hardening.sql
+--
+-- (or paste into the Supabase SQL editor for the cloud project).
+
+\echo '== 1. authenticated must NOT hold table-level INSERT/UPDATE beyond the column grant =='
+SELECT
+  'table-level UPDATE revoked from authenticated' AS check,
+  CASE WHEN NOT has_table_privilege('authenticated', 'public.transactions', 'UPDATE')
+       THEN 'PASS' ELSE 'FAIL' END AS result;
+
+\echo '== 2. exactly the three intended columns are UPDATE-able by authenticated =='
+SELECT
+  'column UPDATE grants' AS check,
+  CASE WHEN (
+    SELECT array_agg(a.attname::text ORDER BY a.attname)
+    FROM pg_attribute a
+    WHERE a.attrelid = 'public.transactions'::regclass
+      AND a.attnum > 0 AND NOT a.attisdropped
+      AND has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
+  ) = ARRAY['provider_subscription_id', 'status', 'stripe_payment_intent_id']
+  THEN 'PASS' ELSE 'FAIL' END AS result,
+  (
+    SELECT string_agg(a.attname, ', ' ORDER BY a.attname)
+    FROM pg_attribute a
+    WHERE a.attrelid = 'public.transactions'::regclass
+      AND a.attnum > 0 AND NOT a.attisdropped
+      AND has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
+  ) AS actual;
+
+\echo '== 3. financial columns are NOT UPDATE-able by authenticated =='
+SELECT
+  a.attname AS column,
+  CASE WHEN has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
+       THEN 'FAIL' ELSE 'PASS' END AS result
+FROM pg_attribute a
+WHERE a.attrelid = 'public.transactions'::regclass
+  AND a.attname IN ('amount', 'currency', 'product_id', 'plan_id', 'payment_provider',
+                    'user_id', 'tenant_id', 'settlement_base', 'settlement_currency',
+                    'school_percentage_snapshot')
+ORDER BY a.attname;
+
+\echo '== 4. the INSERT policy pins status = pending =='
+SELECT
+  polname AS policy,
+  CASE WHEN pg_get_expr(polwithcheck, polrelid) LIKE '%pending%'
+       THEN 'PASS' ELSE 'FAIL' END AS result,
+  pg_get_expr(polwithcheck, polrelid) AS with_check
+FROM pg_policy
+WHERE polrelid = 'public.transactions'::regclass
+  AND polcmd = 'a';
+
+\echo '== 5. the UPDATE policy cannot reach successful/refunded =='
+SELECT
+  polname AS policy,
+  CASE WHEN pg_get_expr(polqual, polrelid) LIKE '%pending%'
+        AND pg_get_expr(polwithcheck, polrelid) LIKE '%pending%'
+        AND pg_get_expr(polwithcheck, polrelid) LIKE '%failed%'
+        AND pg_get_expr(polwithcheck, polrelid) NOT LIKE '%successful%'
+        AND pg_get_expr(polwithcheck, polrelid) NOT LIKE '%refunded%'
+       THEN 'PASS' ELSE 'FAIL' END AS result,
+  pg_get_expr(polqual, polrelid) AS using_expr,
+  pg_get_expr(polwithcheck, polrelid) AS with_check
+FROM pg_policy
+WHERE polrelid = 'public.transactions'::regclass
+  AND polcmd = 'w';
+
+\echo '== 6. RLS is still enabled, and there is still no DELETE path for authenticated =='
+SELECT
+  'RLS enabled' AS check,
+  CASE WHEN relrowsecurity THEN 'PASS' ELSE 'FAIL' END AS result
+FROM pg_class WHERE oid = 'public.transactions'::regclass
+UNION ALL
+SELECT
+  'DELETE not granted to authenticated',
+  CASE WHEN NOT has_table_privilege('authenticated', 'public.transactions', 'DELETE')
+       THEN 'PASS' ELSE 'FAIL' END;