diff --git a/lib/currency/__tests__/riksbanken.test.ts b/lib/currency/__tests__/riksbanken.test.ts index 57c1b124..13c41e3f 100644 --- a/lib/currency/__tests__/riksbanken.test.ts +++ b/lib/currency/__tests__/riksbanken.test.ts @@ -128,6 +128,7 @@ describe('fetchExchangeRate', () => { exactHit?: CacheRow | null latestHit?: CacheRow | null onUpsert?: (row: Record) => void + upsertError?: { code: string; message: string } }) { // .maybeSingle() terminates both the exact lookup and the latest // lookup. Shared across from() calls so the once-queue holds: the @@ -147,7 +148,7 @@ describe('fetchExchangeRate', () => { maybeSingle, upsert: vi.fn((row: Record) => { opts.onUpsert?.(row) - return Promise.resolve({ data: null, error: null }) + return Promise.resolve({ data: null, error: opts.upsertError ?? null }) }), })), } as never @@ -182,6 +183,22 @@ describe('fetchExchangeRate', () => { }) }) + it('still returns the fetched rate when the cache write is rejected by RLS', async () => { + // Since migration 20260710100000, INSERT on exchange_rates is + // service-role only. supabase-js reports the RLS rejection as a + // resolved { error }, not a throw: the rate must come back anyway. + vi.spyOn(global, 'fetch').mockResolvedValueOnce( + new Response(JSON.stringify([{ value: '11.42', date: '2025-01-15' }]), { status: 200 }) + ) + const supabase = makeSupabase({ + upsertError: { code: '42501', message: 'permission denied for table exchange_rates' }, + }) + + const result = await fetchExchangeRate('EUR', new Date('2025-01-15'), supabase) + + expect(result).toEqual({ currency: 'EUR', rate: 11.42, date: '2025-01-15' }) + }) + it('falls back to the most recent cached observation when Riksbanken is down', async () => { vi.spyOn(global, 'fetch').mockRejectedValue(new Error('Network error')) const supabase = makeSupabase({ diff --git a/lib/currency/riksbanken.ts b/lib/currency/riksbanken.ts index 2b561bb4..550da339 100644 --- a/lib/currency/riksbanken.ts +++ b/lib/currency/riksbanken.ts @@ -52,6 +52,16 @@ async function readCachedRate( } } +/** + * Best-effort cache write. INSERT on exchange_rates is service-role only + * (migration 20260710100000): the shared cache feeds money math, so a user + * client must not be able to poison a (currency, rate_date) pair for every + * tenant. When called with an authenticated client (bank file import, + * refresh-exchange-rate, manual bank sync) the upsert is rejected by RLS; + * supabase-js reports that as a resolved { error }, not a throw, and the + * result is deliberately not inspected: the fetched rate is returned to the + * caller regardless, and the service-role sync cron fills the cache instead. + */ async function writeCachedRate( supabase: SupabaseClient, currency: Currency, @@ -70,7 +80,7 @@ async function writeCachedRate( { onConflict: 'currency,rate_date', ignoreDuplicates: true }, ) } catch { - // best-effort + // best-effort: a cache write failure must never block the rate } } diff --git a/supabase/migrations/20260710100000_exchange_rates_service_role_writes.sql b/supabase/migrations/20260710100000_exchange_rates_service_role_writes.sql new file mode 100644 index 00000000..038aa56a --- /dev/null +++ b/supabase/migrations/20260710100000_exchange_rates_service_role_writes.sql @@ -0,0 +1,28 @@ +-- Lock down writes to the shared exchange-rate cache. +-- pg-test: covered-by tests/pg/db-advisor-lockdowns.pg.test.ts +-- +-- Supabase advisor (rls_policy_always_true): exchange_rates_insert allowed +-- any authenticated user to INSERT arbitrary rows (WITH CHECK (true)) into a +-- cache that feeds money math (amount_sek on ingested transactions, invoice +-- SEK conversion). Because the table is shared across all tenants and reads +-- take the first row for a (currency, rate_date) pair (UNIQUE constraint + +-- ignoreDuplicates on the writer), one malicious or buggy client could +-- poison a rate for every company. +-- +-- New design: only the service role writes the cache. The 05:00 +-- enable-banking sync cron (service role, the primary cache filler) bypasses +-- RLS and keeps working. User-client paths (bank file import execute, +-- refresh-exchange-rate, manual bank sync) still attempt the write; it is +-- now rejected and deliberately ignored: lib/currency/riksbanken.ts +-- writeCachedRate() is fail-soft and never inspects the upsert result, so +-- the fetched rate is returned to the caller exactly as before. Reads +-- (exchange_rates_select) are unchanged: the data is public reference data. +DROP POLICY IF EXISTS "exchange_rates_insert" ON public.exchange_rates; + +-- Defense in depth: revoke the table privilege as well, so a future +-- always-true policy cannot silently reopen the hole. With RLS enabled and +-- no INSERT policy this is already denied; the REVOKE makes the intent +-- explicit and the rejection deterministic (permission denied). +REVOKE INSERT ON public.exchange_rates FROM anon, authenticated; + +NOTIFY pgrst, 'reload schema'; diff --git a/supabase/migrations/20260710101000_drop_duplicate_jel_entry_index.sql b/supabase/migrations/20260710101000_drop_duplicate_jel_entry_index.sql new file mode 100644 index 00000000..041253c6 --- /dev/null +++ b/supabase/migrations/20260710101000_drop_duplicate_jel_entry_index.sql @@ -0,0 +1,18 @@ +-- Drop the duplicate journal_entry_lines(journal_entry_id) index. +-- +-- Supabase linter (duplicate_index): idx_journal_entry_lines_entry and +-- idx_journal_entry_lines_entry_id are identical. Verified 2026-07-09 via +-- pg_indexes on prod; both read: +-- CREATE INDEX ... ON public.journal_entry_lines USING btree (journal_entry_id) +-- +-- idx_journal_entry_lines_entry_id is the one this repo defines (migration +-- 20240101000002_bookkeeping.sql), so it stays. idx_journal_entry_lines_entry +-- exists only on databases where it was created outside the migration +-- history, hence IF EXISTS: fresh-from-migrations databases (pg-real CI, +-- preview branches) never had it. +-- +-- Plain DROP INDEX, not CONCURRENTLY: CONCURRENTLY cannot run inside the +-- migration transaction. The ACCESS EXCLUSIVE lock on journal_entry_lines is +-- momentary; dropping an index is a catalog-only operation with no table +-- rewrite. +DROP INDEX IF EXISTS public.idx_journal_entry_lines_entry; diff --git a/supabase/migrations/20260710102000_receipts_bucket_drop_public_read.sql b/supabase/migrations/20260710102000_receipts_bucket_drop_public_read.sql new file mode 100644 index 00000000..79c6b756 --- /dev/null +++ b/supabase/migrations/20260710102000_receipts_bucket_drop_public_read.sql @@ -0,0 +1,22 @@ +-- Remove anonymous read/listing access to the receipts storage bucket. +-- pg-test: covered-by tests/pg/db-advisor-lockdowns.pg.test.ts +-- +-- Supabase advisor (public_bucket_allows_listing): the receipts storage +-- bucket is public and carried an anon SELECT policy on storage.objects +-- (receipts_public_read, USING bucket_id = 'receipts'), which let anonymous +-- clients LIST every object in the bucket through the storage API. +-- +-- Investigation (2026-07-09): the bucket is unused. No code references it +-- (no storage.from('receipts'), no getPublicUrl or signed URL against it), +-- the public.receipts table has 0 rows in prod, and the bucket holds 2 +-- orphan objects from 2026-02-26 (early development). Nothing depends on +-- anonymous reads or listing, so the policy can go. Authenticated own-folder +-- policies (receipts_select/insert/delete) are untouched, and direct +-- public-URL object reads are served without RLS while the bucket's public +-- flag stays on; whether to flip the bucket private or delete it outright is +-- a separate product decision, documented in the PR. +-- +-- The bucket and its policies were created outside the migration history +-- (dashboard), hence IF EXISTS: fresh-from-migrations databases (pg-real CI, +-- preview branches) do not have the policy. +DROP POLICY IF EXISTS "receipts_public_read" ON storage.objects; diff --git a/tests/pg/db-advisor-lockdowns.pg.test.ts b/tests/pg/db-advisor-lockdowns.pg.test.ts new file mode 100644 index 00000000..90b1ce12 --- /dev/null +++ b/tests/pg/db-advisor-lockdowns.pg.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'vitest' +import { getClient, getPool, withUserContext } from '@/tests/pg/setup' +import { seedCompany } from '@/tests/pg/fixtures' + +// Covers the 2026-07-09 Supabase-advisor lockdowns: +// - 20260710100000: exchange_rates INSERT is service-role only +// - 20260710101000: duplicate journal_entry_lines(journal_entry_id) index dropped +// - 20260710102000: receipts storage bucket has no anon read/listing policy +describe('db-advisor lockdowns.pg', () => { + describe('exchange_rates: writes are service-role only', () => { + it('rejects INSERT from an authenticated user', async () => { + const { userId } = await seedCompany() + await withUserContext(userId, async (client) => { + await expect( + client.query( + `INSERT INTO public.exchange_rates (currency, rate_date, rate, observation_date) + VALUES ('EUR', '2026-01-02', 999.99, '2026-01-02')`, + ), + ).rejects.toThrow(/permission denied|row-level security/i) + }) + }) + + it('still lets authenticated users SELECT cached rates', async () => { + // Seeding through the superuser pool stands in for the service-role + // writer: both bypass RLS. + await getPool().query( + `INSERT INTO public.exchange_rates (currency, rate_date, rate, observation_date) + VALUES ('EUR', '2026-01-05', 11.42, '2026-01-05') + ON CONFLICT (currency, rate_date) DO NOTHING`, + ) + const { userId } = await seedCompany() + const rows = await withUserContext(userId, async (client) => { + const res = await client.query<{ rate: string }>( + `SELECT rate FROM public.exchange_rates + WHERE currency = 'EUR' AND rate_date = '2026-01-05'`, + ) + return res.rows + }) + expect(rows).toHaveLength(1) + expect(Number(rows[0]!.rate)).toBeCloseTo(11.42) + }) + + it('has no INSERT policy and no INSERT privilege for anon/authenticated', async () => { + const pol = await getPool().query( + `SELECT polname FROM pg_policy + WHERE polrelid = 'public.exchange_rates'::regclass AND polcmd = 'a'`, + ) + expect(pol.rows).toHaveLength(0) + + const priv = await getPool().query<{ auth_can: boolean; anon_can: boolean }>( + `SELECT has_table_privilege('authenticated', 'public.exchange_rates', 'INSERT') AS auth_can, + has_table_privilege('anon', 'public.exchange_rates', 'INSERT') AS anon_can`, + ) + expect(priv.rows[0]!.auth_can).toBe(false) + expect(priv.rows[0]!.anon_can).toBe(false) + }) + }) + + describe('journal_entry_lines: duplicate index dropped', () => { + it('keeps idx_journal_entry_lines_entry_id and not idx_journal_entry_lines_entry', async () => { + const res = await getPool().query<{ indexname: string }>( + `SELECT indexname FROM pg_indexes + WHERE schemaname = 'public' AND tablename = 'journal_entry_lines' + AND indexname IN ('idx_journal_entry_lines_entry', 'idx_journal_entry_lines_entry_id')`, + ) + const names = res.rows.map((r) => r.indexname) + expect(names).toContain('idx_journal_entry_lines_entry_id') + expect(names).not.toContain('idx_journal_entry_lines_entry') + }) + }) + + describe('receipts storage bucket: anon cannot read or list', () => { + it('storage.objects has no receipts_public_read policy', async () => { + const res = await getPool().query( + `SELECT polname FROM pg_policy + WHERE polrelid = 'storage.objects'::regclass AND polname = 'receipts_public_read'`, + ) + expect(res.rows).toHaveLength(0) + }) + + it('anon listing of the receipts bucket returns nothing', async () => { + await getPool().query( + `INSERT INTO storage.buckets (id, name, public) + VALUES ('receipts', 'receipts', true) + ON CONFLICT (id) DO NOTHING`, + ) + await getPool().query( + `INSERT INTO storage.objects (bucket_id, name) + VALUES ('receipts', 'someone-elses-folder/receipt.jpg') + ON CONFLICT (bucket_id, name) DO NOTHING`, + ) + const client = await getClient() + try { + await client.query('BEGIN') + await client.query(`SELECT set_config('request.jwt.claims', '{"role":"anon"}', true)`) + await client.query('SET LOCAL ROLE anon') + // Either outcome proves listing is impossible for anon: zero rows via + // RLS (no anon SELECT policy left), or no table privilege at all. + try { + const res = await client.query( + `SELECT name FROM storage.objects WHERE bucket_id = 'receipts'`, + ) + expect(res.rows).toHaveLength(0) + } catch (err) { + expect(String(err)).toMatch(/permission denied/i) + } + } finally { + await client.query('ROLLBACK').catch(() => {}) + client.release() + } + }) + }) +})