Skip to content

Commit 4de7f65

Browse files
guillermoscriptclaude
guillermoscript
and
claude
authored
fix(security): pin transaction status and columns for user-scoped writes (#528) (#537)
`transactions` grants table-level INSERT/UPDATE to `authenticated`, and its RLS policies constrain only which ROWS a user may write, never which columns. An authenticated student could therefore POST a row with any column values, provided user_id was theirs and tenant_id was their current tenant. Two impacts, both reproduced against a local database and both reachable without touching a payment provider: 1. Free enrolment. `trigger_manage_transactions` (AFTER INSERT/UPDATE) calls `enroll_user()` whenever product_id is set and status = 'successful', so a self-inserted row wrote `entitlements` — paid course access, no money. 2. Fabricated payout liability. `getPayoutsOwed()` reads transactions with status IN ('successful','refunded') and a platform-settled provider, so a self-inserted row made the payout dashboard report a debt to a school for a sale that never happened. Both are gated on the row reaching 'successful' (or 'refunded'), and neither is reachable from a 'pending' row, so the fix pins that: - The INSERT policy now requires status = 'pending'. - Table-level UPDATE is revoked from `authenticated` and re-granted on only the three columns a user-scoped caller actually writes (status, provider_subscription_id, stripe_payment_intent_id); the UPDATE policy restricts the reachable statuses to 'pending' and 'failed'. The column grant is not redundant with the policy: an RLS WITH CHECK cannot compare NEW to OLD, so a policy alone cannot make a column immutable. Without it a student could open a pending transaction for a cheap product, PATCH its product_id to an expensive one, pay the cheap price, and let the provider webhook flip the row to 'successful'. `enrollUser()` in the mock checkout action is the only legitimate user-scoped caller that needs to write 'successful', so its two inserts move to the admin client; it already validates tenant ownership before writing. Service-role paths (webhooks, reconcilers, crons) and the SECURITY DEFINER `grant_free_subscription()` bypass RLS and are unaffected — verified live. Claude-Session: https://claude.ai/code/session_01YUmcqh3asHLsstAso4UNdV Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 3318403 commit 4de7f65

5 files changed

Lines changed: 271 additions & 3 deletions

File tree

‎.claude/gh-workflow.config.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
22
"projectBoard": null,
3-
"prTemplate": null,
3+
"prTemplate": ".github/pull_request_template.md",
44
"branchConvention": "<type>/<slug>-<N>"
55
}

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use server';
22

33
import { createClient as createServerClient } from '@/lib/supabase/server';
4+
import { createAdminClient } from '@/lib/supabase/admin';
45
import { revalidatePath } from 'next/cache';
56
import { getCurrentUserId, getCurrentTenantId } from '@/lib/supabase/tenant'
67
import { headers } from 'next/headers';
@@ -26,6 +27,16 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
2627
}
2728
const tenantId = await getCurrentTenantId();
2829

30+
// Since #528 the transactions INSERT policy pins `status = 'pending'` for the
31+
// user-scoped client, because a student could otherwise POST a 'successful'
32+
// row straight to PostgREST and let the after_transaction_insert trigger hand
33+
// them the course. This action legitimately needs to write 'successful', so
34+
// the two inserts below use the admin client. Per CLAUDE.md, that shifts the
35+
// tenant check to us: `userId` comes from the session, and the product/plan
36+
// lookups that follow are tenant-scoped reads on the USER-scoped client, so a
37+
// caller can only reach rows their own tenant owns.
38+
const adminClient = createAdminClient();
39+
2940
try {
3041
if (courseId) {
3142
// Get product for this course (pick first match, scoped to tenant)
@@ -49,7 +60,7 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
4960
// Create the transaction. The after_transaction_insert trigger
5061
// enrolls the user (entitlements + enrollment record) — no explicit
5162
// RPC call here, the trigger is the single enrollment path.
52-
const { data: transaction, error: txError } = await supabase
63+
const { data: transaction, error: txError } = await adminClient
5364
.from('transactions')
5465
.insert({
5566
user_id: userId,
@@ -97,7 +108,7 @@ export async function enrollUser(courseId?: string, planId?: string, paymentMeth
97108
// Create the transaction. The after_transaction_insert trigger
98109
// creates the subscription + entitlements (or extends an existing
99110
// subscription on renewal).
100-
const { data: transaction, error: txError } = await supabase
111+
const { data: transaction, error: txError } = await adminClient
101112
.from('transactions')
102113
.insert({
103114
user_id: userId,

‎supabase/migrations/20260725170000_transactions_column_hardening.sql‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
-- Issue #528 — transactions RLS restricts rows but not columns.
2+
--
3+
-- `transactions` grants table-level INSERT/UPDATE to `authenticated`
4+
-- (20260126190500_lms_complete.sql), and the RLS policies in
5+
-- 20260313025318_rls_transactions.sql constrain only WHICH ROWS a user may
6+
-- write, never which columns:
7+
--
8+
-- CREATE POLICY "Users can create own transactions"
9+
-- ON public.transactions FOR INSERT TO authenticated
10+
-- WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id());
11+
--
12+
-- So an authenticated student can POST /rest/v1/transactions with any column
13+
-- values they like, provided user_id is theirs and tenant_id is their current
14+
-- tenant. Two consequences, both reached without touching a payment provider:
15+
--
16+
-- 1. FREE ENROLMENT. `trigger_manage_transactions` (AFTER INSERT/UPDATE) calls
17+
-- enroll_user(NEW.user_id, NEW.product_id) whenever product_id is set and
18+
-- status = 'successful', and handle_new_subscription(...) on the plan_id
19+
-- branch. A self-inserted 'successful' row therefore writes `entitlements`
20+
-- — i.e. paid course access — with no money involved.
21+
-- 2. FABRICATED PAYOUT LIABILITY. getPayoutsOwed() (app/actions/platform/
22+
-- payouts.ts:52) reads transactions WHERE status IN ('successful','refunded')
23+
-- AND payment_provider IN (paypal, lemonsqueezy, binance). A self-inserted
24+
-- row carrying one of those providers lands in grossOwed, so the platform's
25+
-- own payout dashboard reports it owes a school money for a sale that never
26+
-- happened.
27+
--
28+
-- #512 (20260725110000) closed `school_percentage_snapshot` specifically, by
29+
-- making the database compute and freeze that one column. The rest of the row is
30+
-- still caller-controlled. This migration closes the two impacts above.
31+
--
32+
-- THE SHAPE OF THE FIX.
33+
--
34+
-- Both impacts are gated on the SAME thing: a row reaching 'successful' (or, for
35+
-- the payout half, 'refunded'). Neither is reachable from a 'pending' row. So the
36+
-- invariant worth enforcing is narrow and checkable — an untrusted caller may
37+
-- open a transaction, but may not declare it paid. Only the webhook, verify and
38+
-- reconcile paths, which all run on the service-role client and bypass RLS
39+
-- entirely, may do that.
40+
--
41+
-- Two layers, because they defend against different mechanics:
42+
--
43+
-- (a) INSERT policy pins status = 'pending'. Closes the self-INSERT path.
44+
--
45+
-- (b) UPDATE is restricted to the three columns a legitimate user-scoped caller
46+
-- actually writes, and the policy pins the reachable statuses. Closes the
47+
-- self-ESCALATE path — owning a genuine pending row is otherwise a second
48+
-- route to the same place, since after_transaction_update fires the very
49+
-- same trigger.
50+
--
51+
-- WHY (b) NEEDS A COLUMN GRANT AND NOT JUST A POLICY.
52+
--
53+
-- An RLS WITH CHECK sees only the NEW row; it cannot compare NEW to OLD. So a
54+
-- policy alone cannot make a column immutable. Without the column grant a student
55+
-- can open a legitimate pending transaction for a cheap product, PATCH its
56+
-- product_id to an expensive one, pay the cheap price, and let the provider's
57+
-- webhook flip the row to 'successful' — at which point the trigger enrols them
58+
-- in the expensive course. Column privileges close that mechanically, at the
59+
-- grant layer, without needing OLD.
60+
--
61+
-- Note the ordering requirement: a bare REVOKE ... (column) is a NO-OP while the
62+
-- table-level grant is held, so table-level UPDATE must be revoked FIRST and the
63+
-- allowed columns re-granted after.
64+
65+
BEGIN;
66+
67+
-- ---------------------------------------------------------------------------
68+
-- (a) INSERT — an untrusted caller may only open a PENDING transaction.
69+
-- ---------------------------------------------------------------------------
70+
--
71+
-- Permissive policies OR together, so this has to replace the existing policy
72+
-- rather than sit alongside it: an additional policy would widen the grant, not
73+
-- narrow it.
74+
--
75+
-- Callers unaffected (verified against every application write site):
76+
-- app/api/payments/checkout/route.ts:233 status: 'pending'
77+
-- app/api/stripe/create-payment-intent/route.ts:140 status: 'pending'
78+
--
79+
-- Callers that bypass RLS and are therefore untouched: every createAdminClient()
80+
-- / service-role path (the Stripe webhook, lib/payments/webhook-dispatch.ts, the
81+
-- Solana and Binance reconcilers, the two cron routes, payment-requests.ts,
82+
-- admin/binance-personal.ts), plus grant_free_subscription() — SECURITY DEFINER
83+
-- runs as the table owner and the table is not FORCE ROW LEVEL SECURITY, so the
84+
-- free-plan path still inserts its zero-amount 'successful' row.
85+
86+
DROP POLICY IF EXISTS "Users can create own transactions" ON public.transactions;
87+
88+
CREATE POLICY "Users can create own transactions"
89+
ON public.transactions FOR INSERT TO authenticated
90+
WITH CHECK (
91+
(SELECT auth.uid()) = user_id
92+
AND tenant_id = (SELECT get_tenant_id())
93+
AND status = 'pending'
94+
);
95+
96+
-- ---------------------------------------------------------------------------
97+
-- (b) UPDATE — narrow to the three columns a user-scoped caller actually writes.
98+
-- ---------------------------------------------------------------------------
99+
--
100+
-- The complete set of user-scoped UPDATE sites and the columns they write:
101+
-- app/api/payments/checkout/route.ts:320 provider_subscription_id
102+
-- app/api/payments/checkout/route.ts:337 status -> 'failed'
103+
-- app/api/stripe/create-payment-intent/route.ts:187 provider_subscription_id
104+
-- app/api/stripe/create-payment-intent/route.ts:203 status -> 'failed'
105+
-- app/api/stripe/create-payment-intent/route.ts:241 stripe_payment_intent_id
106+
--
107+
-- Everything else — amount, currency, product_id, plan_id, payment_provider,
108+
-- user_id, tenant_id, the settlement_* columns, provider_charge_id,
109+
-- provider_metadata, payment_method, transaction_date, school_percentage_snapshot
110+
-- — becomes unwritable by `authenticated`. A future user-scoped update of any of
111+
-- them fails loudly with `permission denied for column`, which is the intended
112+
-- trade-off: this is a payments table, and a silent success is the worse outcome.
113+
114+
REVOKE UPDATE ON TABLE public.transactions FROM authenticated;
115+
116+
GRANT UPDATE (status, provider_subscription_id, stripe_payment_intent_id)
117+
ON TABLE public.transactions TO authenticated;
118+
119+
-- USING pins the rows a user may touch at all to their own still-pending ones;
120+
-- every legitimate user-scoped update above runs against a row created moments
121+
-- earlier in the same request, so it is always still 'pending'. WITH CHECK pins
122+
-- where that row may land: 'pending' (a metadata-only update leaves it alone) or
123+
-- 'failed' (the provider-session rollback). 'successful' and 'refunded' — the two
124+
-- statuses the trigger and the payout query care about — are unreachable.
125+
126+
DROP POLICY IF EXISTS "Users can update own transactions" ON public.transactions;
127+
128+
CREATE POLICY "Users can update own transactions"
129+
ON public.transactions FOR UPDATE TO authenticated
130+
USING (
131+
(SELECT auth.uid()) = user_id
132+
AND tenant_id = (SELECT get_tenant_id())
133+
AND status = 'pending'
134+
)
135+
WITH CHECK (
136+
(SELECT auth.uid()) = user_id
137+
AND tenant_id = (SELECT get_tenant_id())
138+
AND status IN ('pending', 'failed')
139+
);
140+
141+
COMMIT;

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
-- Rollback for 20260725170000_transactions_column_hardening.sql (issue #528).
2+
--
3+
-- NOT APPLIED BY DEFAULT. Running this restores the policies and grants exactly
4+
-- as 20260313025318_rls_transactions.sql left them — which means it REOPENS both
5+
-- impacts described in #528: an authenticated student regains the ability to
6+
-- self-insert a 'successful' transaction (free course access via the
7+
-- after_transaction_insert trigger) and to fabricate a platform-settled sale that
8+
-- inflates getPayoutsOwed(). Only run it if the column grant has broken a
9+
-- legitimate write path and the fix has to be re-cut.
10+
11+
BEGIN;
12+
13+
DROP POLICY IF EXISTS "Users can create own transactions" ON public.transactions;
14+
15+
CREATE POLICY "Users can create own transactions"
16+
ON public.transactions FOR INSERT TO authenticated
17+
WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id());
18+
19+
-- Drop the column-level grants before restoring the table-level one, so no
20+
-- narrower privilege is left dangling behind the broad grant.
21+
REVOKE UPDATE (status, provider_subscription_id, stripe_payment_intent_id)
22+
ON TABLE public.transactions FROM authenticated;
23+
24+
GRANT UPDATE ON TABLE public.transactions TO authenticated;
25+
26+
DROP POLICY IF EXISTS "Users can update own transactions" ON public.transactions;
27+
28+
CREATE POLICY "Users can update own transactions"
29+
ON public.transactions FOR UPDATE TO authenticated
30+
USING (auth.uid() = user_id AND tenant_id = get_tenant_id())
31+
WITH CHECK (auth.uid() = user_id AND tenant_id = get_tenant_id());
32+
33+
COMMIT;

‎supabase/snippets/verify_transactions_column_hardening.sql‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
-- Verify 20260725170000_transactions_column_hardening.sql (issue #528).
2+
--
3+
-- Asserts the FINAL grant + policy state on public.transactions rather than the
4+
-- text of the migration, so it also catches a later migration re-widening things.
5+
-- Every row should report PASS. Run with:
6+
--
7+
-- docker exec -i supabase_db_lms-front psql -U postgres -d postgres \
8+
-- -f - < supabase/snippets/verify_transactions_column_hardening.sql
9+
--
10+
-- (or paste into the Supabase SQL editor for the cloud project).
11+
12+
\echo '== 1. authenticated must NOT hold table-level INSERT/UPDATE beyond the column grant =='
13+
SELECT
14+
'table-level UPDATE revoked from authenticated' AS check,
15+
CASE WHEN NOT has_table_privilege('authenticated', 'public.transactions', 'UPDATE')
16+
THEN 'PASS' ELSE 'FAIL' END AS result;
17+
18+
\echo '== 2. exactly the three intended columns are UPDATE-able by authenticated =='
19+
SELECT
20+
'column UPDATE grants' AS check,
21+
CASE WHEN (
22+
SELECT array_agg(a.attname::text ORDER BY a.attname)
23+
FROM pg_attribute a
24+
WHERE a.attrelid = 'public.transactions'::regclass
25+
AND a.attnum > 0 AND NOT a.attisdropped
26+
AND has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
27+
) = ARRAY['provider_subscription_id', 'status', 'stripe_payment_intent_id']
28+
THEN 'PASS' ELSE 'FAIL' END AS result,
29+
(
30+
SELECT string_agg(a.attname, ', ' ORDER BY a.attname)
31+
FROM pg_attribute a
32+
WHERE a.attrelid = 'public.transactions'::regclass
33+
AND a.attnum > 0 AND NOT a.attisdropped
34+
AND has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
35+
) AS actual;
36+
37+
\echo '== 3. financial columns are NOT UPDATE-able by authenticated =='
38+
SELECT
39+
a.attname AS column,
40+
CASE WHEN has_column_privilege('authenticated', a.attrelid, a.attname, 'UPDATE')
41+
THEN 'FAIL' ELSE 'PASS' END AS result
42+
FROM pg_attribute a
43+
WHERE a.attrelid = 'public.transactions'::regclass
44+
AND a.attname IN ('amount', 'currency', 'product_id', 'plan_id', 'payment_provider',
45+
'user_id', 'tenant_id', 'settlement_base', 'settlement_currency',
46+
'school_percentage_snapshot')
47+
ORDER BY a.attname;
48+
49+
\echo '== 4. the INSERT policy pins status = pending =='
50+
SELECT
51+
polname AS policy,
52+
CASE WHEN pg_get_expr(polwithcheck, polrelid) LIKE '%pending%'
53+
THEN 'PASS' ELSE 'FAIL' END AS result,
54+
pg_get_expr(polwithcheck, polrelid) AS with_check
55+
FROM pg_policy
56+
WHERE polrelid = 'public.transactions'::regclass
57+
AND polcmd = 'a';
58+
59+
\echo '== 5. the UPDATE policy cannot reach successful/refunded =='
60+
SELECT
61+
polname AS policy,
62+
CASE WHEN pg_get_expr(polqual, polrelid) LIKE '%pending%'
63+
AND pg_get_expr(polwithcheck, polrelid) LIKE '%pending%'
64+
AND pg_get_expr(polwithcheck, polrelid) LIKE '%failed%'
65+
AND pg_get_expr(polwithcheck, polrelid) NOT LIKE '%successful%'
66+
AND pg_get_expr(polwithcheck, polrelid) NOT LIKE '%refunded%'
67+
THEN 'PASS' ELSE 'FAIL' END AS result,
68+
pg_get_expr(polqual, polrelid) AS using_expr,
69+
pg_get_expr(polwithcheck, polrelid) AS with_check
70+
FROM pg_policy
71+
WHERE polrelid = 'public.transactions'::regclass
72+
AND polcmd = 'w';
73+
74+
\echo '== 6. RLS is still enabled, and there is still no DELETE path for authenticated =='
75+
SELECT
76+
'RLS enabled' AS check,
77+
CASE WHEN relrowsecurity THEN 'PASS' ELSE 'FAIL' END AS result
78+
FROM pg_class WHERE oid = 'public.transactions'::regclass
79+
UNION ALL
80+
SELECT
81+
'DELETE not granted to authenticated',
82+
CASE WHEN NOT has_table_privilege('authenticated', 'public.transactions', 'DELETE')
83+
THEN 'PASS' ELSE 'FAIL' END;

0 commit comments

Comments
 (0)