diff --git a/supabase/migrations/20260611120000_gl_lines_rpc_tenant_guard.sql b/supabase/migrations/20260611120000_gl_lines_rpc_tenant_guard.sql new file mode 100644 index 00000000..5bf899aa --- /dev/null +++ b/supabase/migrations/20260611120000_gl_lines_rpc_tenant_guard.sql @@ -0,0 +1,169 @@ +-- Migration: tenant-isolation guard for the GL-line read RPCs. +-- +-- get_unlinked_gl_lines and get_account_gl_lines_for_matching are SECURITY +-- DEFINER (they read journal_entry_lines / journal_entries / transactions as the +-- owner, bypassing RLS) and PostgREST grants EXECUTE to anon + authenticated. +-- Both take p_company_id as a plain argument, so any authenticated — or even +-- anonymous — caller could invoke them DIRECTLY over /rest/v1/rpc with another +-- company's id and read that company's general-ledger lines: a cross-tenant read +-- that bypasses the API routes' requireCompanyId() guard. Flagged in the PR #624 +-- review and confirmed exploitable (anon and authenticated both hold EXECUTE, +-- and neither has rolbypassrls so RLS would normally protect the tables — but +-- SECURITY DEFINER sidesteps it). +-- +-- Fix: enforce INSIDE the function that an anon/authenticated caller is a member +-- of p_company_id (the same boundary RLS enforces via user_company_ids()). +-- Trusted callers are NOT anon/authenticated and are deliberately left untouched: +-- * service_role — the enable-banking reconciliation cron calls +-- get_unlinked_gl_lines via the service client; its JWT role is +-- 'service_role' (and it has rolbypassrls). +-- * direct / superuser DB access — migrations and the pg-real test harness +-- call these on a bare connection with no JWT role claim. +-- For both, auth.role() is not 'anon'/'authenticated', so the added predicate is +-- a no-op and behaviour is byte-for-byte unchanged. Only the PostgREST-exposed +-- anon/authenticated path is constrained, to the caller's own companies. A +-- foreign p_company_id simply yields zero rows — no error, no data leak. +-- +-- Scope: this hardens the two READ RPCs that expose ledger data. The remaining +-- company-scoped SECURITY DEFINER RPCs (commit_journal_entry, next_voucher_number, +-- delete_last_voucher, …) are writes / sequence generators with their own +-- internal authorization; a broader audit of that set is tracked separately. + +-- ------------------------------------------------------------ +-- get_unlinked_gl_lines — unchanged except the trailing tenant guard. +-- Body mirrors 20260605130000_unlinked_gl_lines_exclude_storno_correction.sql. +-- ------------------------------------------------------------ +CREATE OR REPLACE FUNCTION public.get_unlinked_gl_lines( + p_company_id UUID, + p_account_number TEXT DEFAULT '1930', + p_date_from DATE DEFAULT NULL, + p_date_to DATE DEFAULT NULL +) +RETURNS TABLE ( + line_id UUID, + journal_entry_id UUID, + debit_amount NUMERIC, + credit_amount NUMERIC, + line_description TEXT, + entry_date DATE, + voucher_number INT, + voucher_series TEXT, + entry_description TEXT, + source_type TEXT +) +LANGUAGE sql +STABLE +SECURITY DEFINER +SET search_path = public +AS $$ + SELECT + jel.id AS line_id, + je.id AS journal_entry_id, + jel.debit_amount, + jel.credit_amount, + jel.line_description, + je.entry_date, + je.voucher_number, + je.voucher_series, + je.description AS entry_description, + je.source_type + FROM public.journal_entry_lines jel + JOIN public.journal_entries je ON je.id = jel.journal_entry_id + WHERE jel.account_number = p_account_number + AND je.company_id = p_company_id + AND je.status = 'posted' + AND je.source_type IS DISTINCT FROM 'opening_balance' + AND je.source_type IS DISTINCT FROM 'storno' + AND je.source_type IS DISTINCT FROM 'correction' + AND (p_date_from IS NULL OR je.entry_date >= p_date_from) + AND (p_date_to IS NULL OR je.entry_date <= p_date_to) + AND NOT EXISTS ( + SELECT 1 + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + ) + -- Tenant guard (see migration header): anon/authenticated may only read + -- their own companies; service_role / direct DB access bypass. + AND ( + coalesce(auth.role(), '') NOT IN ('anon', 'authenticated') + OR je.company_id IN (SELECT public.user_company_ids()) + ) + ORDER BY je.entry_date, je.voucher_number; +$$; + +-- ------------------------------------------------------------ +-- get_account_gl_lines_for_matching — unchanged except the trailing tenant guard. +-- Body mirrors 20260610120000_gl_lines_for_matching.sql. +-- ------------------------------------------------------------ +CREATE OR REPLACE FUNCTION public.get_account_gl_lines_for_matching( + p_company_id UUID, + p_account_number TEXT DEFAULT '1930', + p_date_from DATE DEFAULT NULL, + p_date_to DATE DEFAULT NULL, + p_include_matched BOOLEAN DEFAULT false +) +RETURNS TABLE ( + line_id UUID, + journal_entry_id UUID, + debit_amount NUMERIC, + credit_amount NUMERIC, + line_description TEXT, + entry_date DATE, + voucher_number INT, + voucher_series TEXT, + entry_description TEXT, + source_type TEXT, + linked_transaction_count INT +) +LANGUAGE sql +STABLE +SECURITY DEFINER +SET search_path = public +AS $$ + SELECT + jel.id AS line_id, + je.id AS journal_entry_id, + jel.debit_amount, + jel.credit_amount, + jel.line_description, + je.entry_date, + je.voucher_number, + je.voucher_series, + je.description AS entry_description, + je.source_type, + ( + SELECT count(*) + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + )::int AS linked_transaction_count + FROM public.journal_entry_lines jel + JOIN public.journal_entries je ON je.id = jel.journal_entry_id + WHERE jel.account_number = p_account_number + AND je.company_id = p_company_id + AND je.status = 'posted' + AND je.source_type IS DISTINCT FROM 'opening_balance' + AND je.source_type IS DISTINCT FROM 'storno' + AND je.source_type IS DISTINCT FROM 'correction' + AND (p_date_from IS NULL OR je.entry_date >= p_date_from) + AND (p_date_to IS NULL OR je.entry_date <= p_date_to) + AND ( + p_include_matched + OR NOT EXISTS ( + SELECT 1 + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + ) + ) + -- Tenant guard (see migration header): anon/authenticated may only read + -- their own companies; service_role / direct DB access bypass. + AND ( + coalesce(auth.role(), '') NOT IN ('anon', 'authenticated') + OR je.company_id IN (SELECT public.user_company_ids()) + ) + ORDER BY je.entry_date, je.voucher_number; +$$; + +NOTIFY pgrst, 'reload schema'; diff --git a/supabase/migrations/20260611130000_gl_lines_rpc_revoke_public_execute.sql b/supabase/migrations/20260611130000_gl_lines_rpc_revoke_public_execute.sql new file mode 100644 index 00000000..935a9984 --- /dev/null +++ b/supabase/migrations/20260611130000_gl_lines_rpc_revoke_public_execute.sql @@ -0,0 +1,25 @@ +-- Migration: least-privilege EXECUTE on the GL-line read RPCs. +-- +-- Follow-up to 20260611120000_gl_lines_rpc_tenant_guard.sql. That migration's +-- in-function guard already closes the DATA leak — an anon/authenticated caller +-- gets zero rows for a company it doesn't belong to. This migration additionally +-- removes the unnecessary EXECUTE *privilege* for unauthenticated callers +-- (defense in depth / least privilege): anon should not even be able to invoke a +-- financial-ledger RPC. +-- +-- Why REVOKE FROM anon alone is NOT enough: Supabase grants EXECUTE on new public +-- functions to PUBLIC *and* explicitly to anon/authenticated/service_role. anon +-- is a member of PUBLIC, so the PUBLIC grant keeps it callable even after its own +-- grant is revoked. Revoke both PUBLIC and anon, then (re)assert the only two +-- legitimate callers: +-- * authenticated — the API routes call via the user's session; the in- +-- function tenant guard scopes them to their own companies. +-- * service_role — the enable-banking reconciliation cron. + +REVOKE EXECUTE ON FUNCTION public.get_unlinked_gl_lines(uuid, text, date, date) FROM PUBLIC, anon; +REVOKE EXECUTE ON FUNCTION public.get_account_gl_lines_for_matching(uuid, text, date, date, boolean) FROM PUBLIC, anon; + +GRANT EXECUTE ON FUNCTION public.get_unlinked_gl_lines(uuid, text, date, date) TO authenticated, service_role; +GRANT EXECUTE ON FUNCTION public.get_account_gl_lines_for_matching(uuid, text, date, date, boolean) TO authenticated, service_role; + +NOTIFY pgrst, 'reload schema'; diff --git a/supabase/migrations/20260611140000_gl_lines_rpc_guard_claims_read.sql b/supabase/migrations/20260611140000_gl_lines_rpc_guard_claims_read.sql new file mode 100644 index 00000000..0b11893f --- /dev/null +++ b/supabase/migrations/20260611140000_gl_lines_rpc_guard_claims_read.sql @@ -0,0 +1,153 @@ +-- Migration: make the GL-line read-RPC tenant guard read the role from the JWT +-- claims object directly, instead of via auth.role(). +-- +-- 20260611120000 used `auth.role()` to detect the caller's role. auth.role() +-- reads the individual `request.jwt.claim.role` GUC first and only falls back to +-- the `request.jwt.claims` object on installs whose auth.role() carries that +-- fallback. PostgREST sets the claims OBJECT (the individual claim.* GUCs are +-- deprecated), and the pg-real test harness sets `request.jwt.claims` (+ an +-- individual `request.jwt.claim.sub` for auth.uid()) but NOT `request.jwt.claim.role`. +-- On an auth.role() without the object fallback that yields NULL, so the guard's +-- `NOT IN ('anon','authenticated')` branch was TRUE and the membership check was +-- skipped — i.e. the guard failed OPEN (caught by a pg-real test: an +-- authenticated non-member could still read another company's lines). +-- +-- Reading `current_setting('request.jwt.claims', true)::jsonb ->> 'role'` +-- directly is what auth.role() itself falls back to, and depends only on the +-- claims object that both PostgREST and the harness reliably set — so the guard +-- enforces correctly in every environment. Behaviour is otherwise identical: +-- service_role and direct/superuser (no claims → NULL → '') still bypass; anon +-- and authenticated are constrained to their own companies. + +CREATE OR REPLACE FUNCTION public.get_unlinked_gl_lines( + p_company_id UUID, + p_account_number TEXT DEFAULT '1930', + p_date_from DATE DEFAULT NULL, + p_date_to DATE DEFAULT NULL +) +RETURNS TABLE ( + line_id UUID, + journal_entry_id UUID, + debit_amount NUMERIC, + credit_amount NUMERIC, + line_description TEXT, + entry_date DATE, + voucher_number INT, + voucher_series TEXT, + entry_description TEXT, + source_type TEXT +) +LANGUAGE sql +STABLE +SECURITY DEFINER +SET search_path = public +AS $$ + SELECT + jel.id AS line_id, + je.id AS journal_entry_id, + jel.debit_amount, + jel.credit_amount, + jel.line_description, + je.entry_date, + je.voucher_number, + je.voucher_series, + je.description AS entry_description, + je.source_type + FROM public.journal_entry_lines jel + JOIN public.journal_entries je ON je.id = jel.journal_entry_id + WHERE jel.account_number = p_account_number + AND je.company_id = p_company_id + AND je.status = 'posted' + AND je.source_type IS DISTINCT FROM 'opening_balance' + AND je.source_type IS DISTINCT FROM 'storno' + AND je.source_type IS DISTINCT FROM 'correction' + AND (p_date_from IS NULL OR je.entry_date >= p_date_from) + AND (p_date_to IS NULL OR je.entry_date <= p_date_to) + AND NOT EXISTS ( + SELECT 1 + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + ) + -- Tenant guard: anon/authenticated may only read their own companies; + -- service_role and direct/superuser access (no JWT role) bypass. + AND ( + coalesce(nullif(current_setting('request.jwt.claims', true), '')::jsonb ->> 'role', '') + NOT IN ('anon', 'authenticated') + OR je.company_id IN (SELECT public.user_company_ids()) + ) + ORDER BY je.entry_date, je.voucher_number; +$$; + +CREATE OR REPLACE FUNCTION public.get_account_gl_lines_for_matching( + p_company_id UUID, + p_account_number TEXT DEFAULT '1930', + p_date_from DATE DEFAULT NULL, + p_date_to DATE DEFAULT NULL, + p_include_matched BOOLEAN DEFAULT false +) +RETURNS TABLE ( + line_id UUID, + journal_entry_id UUID, + debit_amount NUMERIC, + credit_amount NUMERIC, + line_description TEXT, + entry_date DATE, + voucher_number INT, + voucher_series TEXT, + entry_description TEXT, + source_type TEXT, + linked_transaction_count INT +) +LANGUAGE sql +STABLE +SECURITY DEFINER +SET search_path = public +AS $$ + SELECT + jel.id AS line_id, + je.id AS journal_entry_id, + jel.debit_amount, + jel.credit_amount, + jel.line_description, + je.entry_date, + je.voucher_number, + je.voucher_series, + je.description AS entry_description, + je.source_type, + ( + SELECT count(*) + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + )::int AS linked_transaction_count + FROM public.journal_entry_lines jel + JOIN public.journal_entries je ON je.id = jel.journal_entry_id + WHERE jel.account_number = p_account_number + AND je.company_id = p_company_id + AND je.status = 'posted' + AND je.source_type IS DISTINCT FROM 'opening_balance' + AND je.source_type IS DISTINCT FROM 'storno' + AND je.source_type IS DISTINCT FROM 'correction' + AND (p_date_from IS NULL OR je.entry_date >= p_date_from) + AND (p_date_to IS NULL OR je.entry_date <= p_date_to) + AND ( + p_include_matched + OR NOT EXISTS ( + SELECT 1 + FROM public.transactions t + WHERE t.journal_entry_id = je.id + AND t.company_id = p_company_id + ) + ) + -- Tenant guard: anon/authenticated may only read their own companies; + -- service_role and direct/superuser access (no JWT role) bypass. + AND ( + coalesce(nullif(current_setting('request.jwt.claims', true), '')::jsonb ->> 'role', '') + NOT IN ('anon', 'authenticated') + OR je.company_id IN (SELECT public.user_company_ids()) + ) + ORDER BY je.entry_date, je.voucher_number; +$$; + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/gl_lines_rpc_tenant_guard.pg.test.ts b/tests/pg/gl_lines_rpc_tenant_guard.pg.test.ts new file mode 100644 index 00000000..b244348e --- /dev/null +++ b/tests/pg/gl_lines_rpc_tenant_guard.pg.test.ts @@ -0,0 +1,113 @@ +/** + * pg-real test for the GL-line read-RPC tenant guard + * (20260611120000_gl_lines_rpc_tenant_guard.sql). + * + * get_unlinked_gl_lines and get_account_gl_lines_for_matching are SECURITY + * DEFINER and EXECUTE-able by anon/authenticated, so without the guard any + * authenticated user could call them directly with another company's id and read + * its general ledger. The guard enforces membership for anon/authenticated while + * leaving service_role and direct/superuser access (this harness, migrations, + * the reconciliation cron) untouched. + */ +import { describe, it, expect } from 'vitest' +import { randomUUID } from 'node:crypto' +import { getPool, withUserContext } from './setup' +import { seedCompany } from './fixtures' + +async function insertPostedJournalEntry(params: { + userId: string + companyId: string + fiscalPeriodId: string + entryDate: string + voucherNumber: number + amount?: number +}): Promise { + const id = randomUUID() + const amount = params.amount ?? 1500 + await getPool().query( + `INSERT INTO public.journal_entries + (id, user_id, company_id, fiscal_period_id, voucher_number, voucher_series, + entry_date, description, source_type, status) + VALUES ($1, $2, $3, $4, $5, 'A', $6, 'Bank tx', 'bank_transaction', 'posted')`, + [id, params.userId, params.companyId, params.fiscalPeriodId, params.voucherNumber, params.entryDate], + ) + await getPool().query( + `INSERT INTO public.journal_entry_lines + (journal_entry_id, account_number, debit_amount, credit_amount) + VALUES ($1, '1930', $2, 0), + ($1, '2091', 0, $2)`, + [id, amount], + ) + return id +} + +const UNLINKED = `SELECT journal_entry_id FROM public.get_unlinked_gl_lines($1)` +const MATCHING = `SELECT journal_entry_id FROM public.get_account_gl_lines_for_matching($1, '1930', NULL, NULL, true)` + +describe('GL-line read RPCs — tenant-isolation guard', () => { + it('lets a member read its own company but blocks an authenticated non-member', async () => { + const a = await seedCompany() // userA is owner-member of companyA + const b = await seedCompany() // userB belongs to companyB only + const entryA = await insertPostedJournalEntry({ + userId: a.userId, + companyId: a.companyId, + fiscalPeriodId: a.fiscalPeriodId, + entryDate: '2026-03-15', + voucherNumber: 1, + }) + + // Direct / superuser connection (no JWT role) — the trusted bypass that this + // harness, migrations and the service-role cron rely on. Data is visible. + const bare = await getPool().query(UNLINKED, [a.companyId]) + expect(bare.rows.map((r) => r.journal_entry_id)).toContain(entryA) + + // A member of company A sees A's line through both RPCs. + await withUserContext(a.userId, async (client) => { + const u = await client.query(UNLINKED, [a.companyId]) + expect(u.rows.map((r) => r.journal_entry_id)).toContain(entryA) + const m = await client.query(MATCHING, [a.companyId]) + expect(m.rows.map((r) => r.journal_entry_id)).toContain(entryA) + }) + + // A member of company B probing company A gets nothing — the cross-tenant + // read that SECURITY DEFINER + anon/authenticated EXECUTE used to allow. + await withUserContext(b.userId, async (client) => { + const u = await client.query(UNLINKED, [a.companyId]) + expect(u.rows).toHaveLength(0) + const m = await client.query(MATCHING, [a.companyId]) + expect(m.rows).toHaveLength(0) + }) + }) + + it('rejects an anon (unauthenticated) caller outright — EXECUTE revoked from anon + PUBLIC', async () => { + const a = await seedCompany() + await insertPostedJournalEntry({ + userId: a.userId, + companyId: a.companyId, + fiscalPeriodId: a.fiscalPeriodId, + entryDate: '2026-03-15', + voucherNumber: 1, + }) + + // Each probe runs in its own transaction: a permission-denied error aborts + // the transaction, so the two can't share one. anon has no EXECUTE (revoked + // from its own grant AND from PUBLIC, of which anon is a member), so the call + // is rejected at the privilege layer — defense in depth on top of the + // in-function tenant guard. + const callAsAnon = async (sql: string): Promise => { + const client = await getPool().connect() + try { + await client.query('BEGIN') + await client.query(`SELECT set_config('request.jwt.claims', '{"role":"anon"}', true)`) + await client.query('SET LOCAL ROLE anon') + await client.query(sql, [a.companyId]) + } finally { + await client.query('ROLLBACK').catch(() => {}) + client.release() + } + } + + await expect(callAsAnon(UNLINKED)).rejects.toThrow(/permission denied/i) + await expect(callAsAnon(MATCHING)).rejects.toThrow(/permission denied/i) + }) +})