From 683314b439dd370644c21fa64fcf2febde5bcac0 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Sun, 30 Aug 2026 11:51:27 +0200 Subject: [PATCH] fix(vat): restore least-privilege ACL on get_vat_ruta_source_lines after #2016 (#2026) Migration 20260828172003 dropped the 9-arg get_vat_ruta_source_lines (which carried REVOKE FROM PUBLIC, anon and GRANT TO authenticated, service_role) and recreated the 11-arg signature without an ACL, so anon regained EXECUTE by default. This restates the ACL in a new migration and pins it with has_function_privilege assertions in the pg-real test. Follow-up to #2016. --- DECISIONS.md | 1 + ...260829090500_vat_ruta_source_lines_acl.sql | 30 +++++++++++++++++++ .../vat-ruta-drilldown-reconcile.pg.test.ts | 26 ++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 supabase/migrations/20260829090500_vat_ruta_source_lines_acl.sql diff --git a/DECISIONS.md b/DECISIONS.md index 28e21b93..ed6de962 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1338,5 +1338,6 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-28] /migrate SIE guard skips company-info-only runs (all entity flags false) and the wizard derives "SIE already imported" from the preview OR this session's successful /import-sie results: company info writes no accounts, balances or subledger rows, so the BFL rationale does not apply; and the one-shot preview went stale after phase 1 succeeded and phase 2 failed, falsely blocking an entities-only retry (#2000 review). [2026-08-28] get_vat_ruta_source_lines (the VAT ruta drill-down) now applies the same four exclusions as get_vat_declaration_totals (the filed figure): posted closing entries, source_type 'vat_settlement', the two kontantmetod year-end reversals, and settlement-SHAPED entries (a line on a ruta account plus a line on 2650/1650). It previously filtered on company, status and date only, so expanding a ruta listed verifikat that are not in the number it claims to explain, with no total on the panel to reveal the mismatch. Measured on prod 2026-08-28: 322 posted/reversed entries carrying 26xx lines across 214 companies sit in those excluded classes. A momsdeklaration is räkenskapsinformation (BFL 5 kap.) and this drill-down is what substantiates a filed figure, so the two must agree exactly. The exclusion CTEs are lifted VERBATIM from the figure rather than re-derived: any divergence reintroduces exactly this bug, and an identical copy is easy to diff when the figure changes. Settlement-shape is detected against journal_entry_lines directly instead of through the figure's vat_lines CTE, which is EQUIVALENT not a shortcut (p_ruta_accounts = VAT_ACCOUNTS and p_net_accounts = ['2650','1650'] are both strict subsets of the figure's p_accounts, so restricting to vat_lines first cannot change which entries match); that keeps p_accounts meaning "the accounts of the ruta being expanded" without a fourth account parameter. opening_balance entries are deliberately NOT excluded: the figure exempts them from `shaped`, which keeps their lines IN the totals, so dropping them here would break the equality in the other direction (pinned by its own test). VAT_ACCOUNTS is now exported from lib/reports/vat-declaration.ts so the route detects shape from the same list the figure uses; a second copy is what let the two disagree. DROP + CREATE OR REPLACE, not CREATE OR REPLACE alone: the signature gains p_ruta_accounts/p_net_accounts and adding parameters registers a second overload PostgREST cannot choose between (trap documented in 20260421140000); OR REPLACE on the new arity keeps the file re-runnable. Verified the new pg test actually catches the bug by reinstalling the old body and watching 3 of 4 tests fail with the real misreporting (2611: drill-down 250/240 vs figure 0/200), then restoring. [2026-08-28] Bankavstamning NULL-link fix scoped to transfer legs with contradicting sign (20260828220000): the naive rule (NULL counts only for the primary account) and the formula-only variant (drop far-leg-settled vouchers from unexplained) were both simulated against prod and rejected; the naive rule worsened 4 of 11 affected cards (worst -37 000 kr false alarm on single-leg vouchers with no user action available), the formula variant blew up healthy cards by up to 474 550 kr. The shipped three-condition rule changes 24 vouchers on 7 cards in 6 companies, all verified per-card. +[2026-08-29] get_vat_ruta_source_lines ACL restored in a NEW migration (20260829090500) rather than by editing 20260828172003: that file DROPped the 9-arg overload and CREATEd the 11-arg one without restating REVOKE/GRANT, and DROP FUNCTION discards the ACL, so the new signature silently fell back to EXECUTE for PUBLIC (anon included); the migration is already applied on prod, so a follow-up file is the only compliant path. Rule going forward: every DROP + CREATE of an RPC must restate its REVOKE ALL FROM PUBLIC, anon / GRANT EXECUTE TO authenticated, service_role, and tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts now pins it with has_function_privilege (anon false, authenticated and service_role true, exactly one overload). [2026-08-29] PR #1756 replacement (rebind on PSD2 remap, amends the 2026-07-09 #916 entry): when upsertFromPsd2 resolves a duplicate row for the same connection+uid, the duplicate's MOVABLE transactions (unbooked, unmatched, not anchored via transaction_voucher_links or a payment row: the #1570 single-row move gate) are rebound onto the promoted row BEFORE the duplicate is resolved, so categorize/booking proposes the ledger the user just mapped instead of the overflow slot; a duplicate that still holds booked or anchored rows is demoted to manual as before and never deleted (their vouchers carry the old 19xx line, and the #1643 orphan guards handle the released twin). The contributor's unconditional rebind-all-then-delete was narrowed for that reason. [2026-08-29] Database errors now keep their SQLSTATE: new lib/errors/db-error.ts (dbError/errorCauseTag), applied at the 54 `throw new Error(\`Database error: ${err.message}\`)` sites in the MCP server AND, far more importantly, at lib/supabase/fetch-all.ts:74 where `throw new Error(error.message)` was the single highest-traffic strip point in the codebase (31 callers; every paginated read). isTransientFailure() checks the driver code FIRST and 57014 (statement timeout) is already in TRANSIENT_SQLSTATES, so discarding it turned a retryable timeout into UNKNOWN_ERROR ("Något gick fel. Försök igen."), which an agent cannot dispatch on. Traced end to end: gnubok_query_journal -> fetchEntryLines -> fetchAllRows (code stripped here) -> the tool's own sanitizeDbError, which ALREADY had a correct TRANSIENT_ERROR branch with a "retry or narrow with date_from/date_to" hint that could never fire because getStructuredError saw an anonymous Error. Measured on prod over 60 days with bot actors excluded: 1 024 real-agent failures, 645 UNKNOWN_ERROR across 60 actors and 57 companies; query_journal failed 164 times at p50 8 110 ms while every other failing tool sat at 1-315 ms; 82 retry streaks, 462 wasted repeat calls, 53.1% of error calls inside a streak. fetch-all passes context=null so the driver message stays VERBATIM (sanitizeDbError and other callers match on the existing text; this change adds the code, it does not reword). Attaching `code` is safe because extractCode() only accepts /^[A-Z_]+$/ and every SQLSTATE/PostgREST code contains digits, so it cannot hijack the application error registry (pinned by a test). dbError also never renders the literal "undefined": a driver-level failure with no message produced "Database error: undefined", the string that made these unsearchable. errorCauseTag() returns a PII-safe SQLSTATE for telemetry; the raw driver message can quote row values in a constraint violation and belongs in the server log, never in event_log. NOT ratcheted: check:types reports 538 vs baseline 539 because main fixed an unrelated error in own-account-detector.test.ts after the baseline was set; the gate only fails on an INCREASE, so the baseline is left alone rather than adding unrelated churn to this diff. diff --git a/supabase/migrations/20260829090500_vat_ruta_source_lines_acl.sql b/supabase/migrations/20260829090500_vat_ruta_source_lines_acl.sql new file mode 100644 index 00000000..6e296dbe --- /dev/null +++ b/supabase/migrations/20260829090500_vat_ruta_source_lines_acl.sql @@ -0,0 +1,30 @@ +-- Follow-up to 20260828172003 (#2016): restore the least-privilege ACL on +-- get_vat_ruta_source_lines. +-- +-- That migration DROPped the 9-arg overload and CREATEd the 11-arg one +-- (p_ruta_accounts / p_net_accounts). DROP FUNCTION discards the function's +-- ACL along with the function, and the CREATE did not restate the +-- REVOKE/GRANT that 20260721103000 had put on the old signature, so the new +-- function fell back to the Postgres default: EXECUTE granted to PUBLIC, which +-- includes anon. Practical exposure is nil (SECURITY INVOKER, and the +-- company_members RLS behind user_company_ids() returns zero rows to anon), +-- but every other tenant-scoped read RPC in this repo is explicitly revoked +-- from PUBLIC and anon (the sibling get_vat_declaration_totals restates its +-- ACL in 20260813124510), and the drill-down should not be the one exception. +-- +-- 20260828172003 is already applied on production, so this is a new file +-- rather than an edit. Idempotent: REVOKE and GRANT can be re-run freely. +-- +-- Rule this pins: every DROP + CREATE of an RPC must restate its REVOKE/GRANT, +-- because the ACL does not survive the DROP. +-- +-- pg-test: tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts + +REVOKE ALL ON FUNCTION public.get_vat_ruta_source_lines( + uuid, date, date, text[], text[], text[], date, integer, uuid, uuid, integer +) FROM PUBLIC, anon; +GRANT EXECUTE ON FUNCTION public.get_vat_ruta_source_lines( + uuid, date, date, text[], text[], text[], date, integer, uuid, uuid, integer +) TO authenticated, service_role; + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts b/tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts index 62ec213e..d9069631 100644 --- a/tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts +++ b/tests/pg/vat-ruta-drilldown-reconcile.pg.test.ts @@ -224,4 +224,30 @@ describe('VAT ruta drill-down reconciles with the declaration figure', () => { const lines = await drillDown(companyId, ['2641']) expect(lines.map((l) => l.description)).toContain('Ingående balans') }, 30_000) + + it('grants EXECUTE to authenticated and service_role but not anon', async () => { + // 20260828172003 DROPped the 9-arg overload and CREATEd this 11-arg one + // without restating the REVOKE/GRANT from 20260721103000; DROP FUNCTION + // discards the ACL, so the new signature fell back to EXECUTE for PUBLIC + // (which includes anon). 20260829090500 restores least privilege. The + // overload count pins the other half of that migration: exactly one + // signature, so PostgREST never has to choose. + const { rows } = await getPool().query<{ + anon_can: boolean + authenticated_can: boolean + service_role_can: boolean + overloads: string + }>( + `SELECT has_function_privilege('anon', 'public.get_vat_ruta_source_lines(uuid,date,date,text[],text[],text[],date,integer,uuid,uuid,integer)', 'EXECUTE') AS anon_can, + has_function_privilege('authenticated', 'public.get_vat_ruta_source_lines(uuid,date,date,text[],text[],text[],date,integer,uuid,uuid,integer)', 'EXECUTE') AS authenticated_can, + has_function_privilege('service_role', 'public.get_vat_ruta_source_lines(uuid,date,date,text[],text[],text[],date,integer,uuid,uuid,integer)', 'EXECUTE') AS service_role_can, + (SELECT count(*) FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = 'get_vat_ruta_source_lines')::text AS overloads`, + ) + expect(rows[0]!.anon_can).toBe(false) + expect(rows[0]!.authenticated_can).toBe(true) + expect(rows[0]!.service_role_can).toBe(true) + expect(rows[0]!.overloads).toBe('1') + }, 30_000) })