fix(db): lock down exchange_rates writes, drop duplicate JEL index, receipts anon read (#969)
Three Supabase-advisor findings from the 2026-07-09 production log triage: 1. exchange_rates (rls_policy_always_true): the exchange_rates_insert policy was WITH CHECK (true) for authenticated, letting any signed-in user poison the shared FX cache that feeds money math (amount_sek on ingested transactions, invoice SEK conversion). Migration 20260710100000 drops the policy and revokes INSERT from anon/authenticated; only the service role writes the cache now (the 05:00 enable-banking sync cron and the v1 API-key paths both use the service client). writeCachedRate() in lib/currency/riksbanken.ts was already fail-soft and never inspects the upsert result, so user-client paths (bank file import, refresh-exchange-rate) keep returning the fetched rate unchanged when the cache write is rejected; documented and covered by a new unit test. 2. journal_entry_lines (duplicate_index): idx_journal_entry_lines_entry and idx_journal_entry_lines_entry_id are byte-identical btree indexes on (journal_entry_id), verified via pg_indexes on prod. Migration 20260710101000 drops idx_journal_entry_lines_entry (created outside the migration history); the repo-defined _entry_id stays. 3. receipts bucket (public_bucket_allows_listing): receipts_public_read gave anon SELECT over every object in the bucket, enabling anonymous listing. The bucket is unused: no code references it, public.receipts has 0 rows in prod, 2 orphan objects from 2026-02-26. Migration 20260710102000 drops the anon policy; authenticated own-folder policies stay untouched. New tests/pg/db-advisor-lockdowns.pg.test.ts covers all three (authenticated INSERT rejected, SELECT still works, privilege revoked, duplicate index gone, anon cannot list receipts). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -128,6 +128,7 @@ describe('fetchExchangeRate', () => {
|
||||
exactHit?: CacheRow | null
|
||||
latestHit?: CacheRow | null
|
||||
onUpsert?: (row: Record<string, unknown>) => 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<string, unknown>) => {
|
||||
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({
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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';
|
||||
@@ -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;
|
||||
@@ -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;
|
||||
@@ -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()
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user