diff --git a/DECISIONS.md b/DECISIONS.md index f9bedd45..d83f9a8e 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -630,3 +630,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-27] Docs export (#1247): scripts/export-docs-to-website.mts stubs `server-only` via a Module._load hook instead of untangling the import chain. The chain is real (lib/api/v1/load-routes -> every v1 route -> lib/init -> posthog-observability -> posthog-server) and the script only reads exported markdown builders, so breaking the chain would mean restructuring route imports for a build-time script's benefit. [2026-07-27] Webhook delivery latency (#1201): implemented option (a), an emit-triggered kick of the existing dispatchDueDeliveries, not option (b), an authenticated SSE stream over event_log. The kick is a new lib/webhooks/dispatch-kick.ts wired into fanOutToWebhooks plus the two routes that enqueue a delivery directly (the :test verb and the manual delivery retry), and it does NOT close #1201: the issue asks for a realtime stream for API consumers and that remains open. Three constraints shaped it. It is never awaited: eventBus.emit is awaited at ~99 call sites including journal_entry.committed, and each delivery can burn a 10 s receiver timeout, so awaiting would put a stranger's HTTP endpoint on the critical path of committing a verifikat. It is coalesced per function instance, because a bulk operation emits once per row and would otherwise schedule one claim round trip per row. Its batch size is 5 rather than the cron's 50, because this work runs on the tail of a user-facing request. The SKIP LOCKED claim keeps a kick and the cron from claiming the same row at the same moment, but that is a claim-time guarantee only: the RPC autocommits before any POST, so a later cycle's recoverStuckInFlight sweep can re-arm a row still queued behind an earlier serial loop. Delivery stays at-least-once as the public docs already promise, and the kick's batch of 5 opens a narrower window than the cron's existing batch of 50 against the same 20 s stuck threshold; an early draft of this change claimed double delivery was impossible, which was wrong and is now corrected in the code comments. Scheduling uses next/server after() with a deferred-microtask fallback outside a request scope, mirroring the enable-banking callback; the fallback must stay deferred rather than inline, or the coalescing flag clears before the next kick in the same tick can see it. No API_V1_VERSION bump: no new event types and no payload change, only latency. [2026-07-27] Bolagsskatt add-back (#1051): sumPostedYearEndDispositions now adds back 78xx planenlig avskrivning alongside 88xx and 7533, and excludes fiscal_periods.closing_entry_id from its fetch. Shipping only this "Stage 1" half of the issue: it corrects the tax base and the periodiseringsfond 25 % cap with no migration and no displayed-figure change. The issue's other half (making /rapporter show bokslut entries by moving generateIncomeStatement to excludeFinalClosingEntry) is deliberately NOT done here: it duplicates the exclusion in the kpi_report_aggregates RPC (so it needs a migration plus a pg test), it changes displayed profit for every company that ran the bokslut flow, and it requires removing the add-back at four call sites, including the one that caused the original too-high-tax customer bug. The closing-entry exclusion is part of Stage 1 rather than a follow-up because closing verifikat do carry 78xx/88xx/7533 reversal lines on production, so without it the new add-back silently cancels itself once the year is closed. The issue's stated constraint that source_type='year_end' is load-bearing for the iXBRL RR/BR split is stale: build-input.ts and arsredovisning/build-data.ts already moved to excludeFinalClosingEntry. +[2026-07-27] Documents bucket WORM (#1208): dropped the production-only `users_delete_own_documents` DELETE policy on storage.objects and pinned the invariant with a name-agnostic pg-real test, rather than adding a storage DELETE policy for authenticated users or building the orphan-cleanup script the issue asks for. The policy existed in no migration (dashboard drift, alongside `users_read_own_documents` / `users_upload_own_documents`, which production has INSTEAD of this repo's `documents_select_own` / `documents_insert_own`) and let any user delete, with a normal browser token, the storage bytes of documents linked to posted verifikat: rakenskapsinformation under BFL 7 kap 2 §. Neither deleteDocument()'s linked-check nor block_document_deletion() reaches that far; both protect the row, and the row survives pointing at nothing. Verified reproducible against a local replay of the full migration stream: with the policy present the uploader's own DELETE removes a legacy-layout object, with it dropped the DELETE matches zero rows. Dropping it breaks nothing because every in-app remove() on this bucket has run service-role since #1215. The two legacy read/insert policies are deliberately left alone: the Phase B backfill from 20260726092000 has not run, so dropping the legacy SELECT would make most existing documents unreadable. That is Phase C. The 326 orphan objects (49.5 MB) the issue also describes are NOT cleaned up here: irreversible deletion against 7-year-retention data is not worth 49 MB without a separate report-only pass. The test asserts no DELETE and no UPDATE policy over the bucket under ANY name, because the hole arrived under a name this repo never used. diff --git a/supabase/migrations/20260727190000_drop_documents_bucket_delete_policy.sql b/supabase/migrations/20260727190000_drop_documents_bucket_delete_policy.sql new file mode 100644 index 00000000..1270f463 --- /dev/null +++ b/supabase/migrations/20260727190000_drop_documents_bucket_delete_policy.sql @@ -0,0 +1,65 @@ +-- Close a client-side DELETE hole in the `documents` storage bucket. +-- +-- THE HOLE +-- +-- 20240101000024_storage_bucket_policies.sql documents the bucket as WORM: +-- "No UPDATE or DELETE policies - WORM compliance". That comment describes +-- the repo, not production. Production carries a DELETE policy on +-- storage.objects that exists in NO migration file: +-- +-- users_delete_own_documents +-- FOR DELETE TO authenticated +-- USING (bucket_id = 'documents' +-- AND (storage.foldername(name))[2] = auth.uid()::text) +-- +-- It was created outside the migration stream (dashboard drift), alongside +-- `users_read_own_documents` and `users_upload_own_documents`, which likewise +-- appear in no migration and which production has INSTEAD of this repo's +-- `documents_select_own` / `documents_insert_own`. +-- +-- Under that policy the uploading user, holding nothing but their normal +-- browser token, can DELETE the storage bytes of every document they uploaded +-- under the legacy `documents/{userId}/...` layout: including documents +-- linked to a posted verifikat, i.e. rakenskapsinformation under the BFL +-- 7 kap 2 § seven-year retention duty. None of the three guards we rely on +-- reach that far: deleteDocument()'s linked-check and the +-- block_document_deletion() trigger both protect the document_attachments +-- ROW, and the row survives, still pointing at an object that is gone. +-- +-- WHY DROPPING IT IS SAFE +-- +-- Nothing in the application deletes a documents object with a user-bound +-- client. Every remove() call on this bucket runs on the service role, which +-- is covered by service_role_all_documents and unaffected by this drop: +-- lib/core/documents/document-service.ts uploadDocument cleanup, +-- createNewVersion cleanup, +-- deleteDocument +-- -> all three on createServiceClientNoCookies() since #1215 +-- extensions/general/mcp-server/server.ts audit-package sign-failure cleanup +-- -> MCP paths already hold a service-role client +-- scripts/backfill-document-storage-paths.ts +-- -> builds its own service-role client +-- +-- SCOPE +-- +-- Only the DELETE policy goes. `users_read_own_documents` and +-- `users_upload_own_documents` stay: the Phase B backfill in +-- 20260726092000_documents_bucket_company_scope.sql has not run, the large +-- majority of document_attachments rows are still on the legacy key layout, +-- and dropping the legacy SELECT policy now would make those documents +-- unreadable. Retiring them is Phase C, gated on the backfill reporting zero +-- legacy-prefix objects, and it is deliberately not bundled here. +-- +-- IF EXISTS, because the policy is production-only: on a fresh install, a +-- preview branch or a self-hosted database this is a no-op, which is the +-- correct outcome. tests/pg/documents-bucket-worm.pg.test.ts pins the +-- invariant so a DELETE policy over this bucket cannot come back through a +-- migration unnoticed. +-- +-- Refs: #1208, #1207, #1215, 20260726092000_documents_bucket_company_scope.sql + +DROP POLICY IF EXISTS users_delete_own_documents ON storage.objects; + +-- Defensive: the same hole under this repo's own naming convention. Never +-- created by any migration here, but a dashboard edit could have used it. +DROP POLICY IF EXISTS documents_delete_own ON storage.objects; diff --git a/tests/pg/documents-bucket-worm.pg.test.ts b/tests/pg/documents-bucket-worm.pg.test.ts new file mode 100644 index 00000000..6f6eadde --- /dev/null +++ b/tests/pg/documents-bucket-worm.pg.test.ts @@ -0,0 +1,254 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { getPool, withUserContext } from './setup' +import { insertAuthUser, insertCompany, insertCompanyMember } from './fixtures' + +/** + * WORM ratchet for the `documents` storage bucket, after + * 20260727190000_drop_documents_bucket_delete_policy.sql. + * + * The bug this guards: production carried a `users_delete_own_documents` + * policy (FOR DELETE TO authenticated, USING bucket_id = 'documents' AND + * (storage.foldername(name))[2] = auth.uid()::text) that existed in no + * migration file. It let the uploading user delete the storage bytes of any + * document they had uploaded under the legacy `documents/{userId}/...` + * layout, including documents linked to a posted verifikat: those are + * rakenskapsinformation under the BFL 7 kap 2 § seven-year retention duty. + * + * The application-layer guard in deleteDocument() and the + * block_document_deletion() trigger both protect the document_attachments + * ROW, not the object, so neither one closes this. Deletion of a documents + * object must stay a server-side, service-role code path. + * + * These assertions are name-agnostic on purpose: the hole arrived under a + * name this repo never used, so pinning a name would not have caught it. + */ +describe('documents bucket: WORM (no client-side DELETE)', () => { + const objectNames: string[] = [] + + let owner: string + let company: string + let legacyKey: string + let companyScopedKey: string + + async function seedObject(name: string): Promise { + // owner is populated deliberately: storage-api stamps the uploader there + // in production, and an owner-based policy (`USING (auth.uid() = owner)`, + // Supabase's stock delete template) matches nothing when it is NULL. A + // fixture without an owner would let that policy shape pass this suite by + // comparing against NULL rather than by being absent. + await getPool().query( + `INSERT INTO storage.objects (bucket_id, name, owner) VALUES ('documents', $1, $2)`, + [name, owner], + ) + objectNames.push(name) + } + + beforeAll(async () => { + await getPool().query( + `INSERT INTO storage.buckets (id, name, public) + VALUES ('documents', 'documents', false) + ON CONFLICT (id) DO NOTHING`, + ) + + // Real Supabase grants these to `authenticated`; the bare CI image may + // not. Without the DELETE grant the deletion assertion below would pass + // for the wrong reason (permission denied, not RLS). + await getPool() + .query(`GRANT SELECT, INSERT, DELETE ON storage.objects TO authenticated`) + .catch(() => {}) + + owner = await insertAuthUser() + company = await insertCompany({ createdBy: owner, name: 'WORM Test AB' }) + await insertCompanyMember({ companyId: company, userId: owner, role: 'owner' }) + + legacyKey = `documents/${owner}/1700000000100_kvitto.pdf` + companyScopedKey = `documents/${company}/${owner}/1700000000101_kvitto.pdf` + await seedObject(legacyKey) + await seedObject(companyScopedKey) + }) + + afterAll(async () => { + const sweep = (sql: string, params: unknown[]) => + getPool() + .query(sql, params) + .catch(() => {}) + + if (objectNames.length > 0) { + await sweep(`DELETE FROM storage.objects WHERE name = ANY($1::text[])`, [objectNames]) + } + await sweep(`DELETE FROM public.company_members WHERE company_id = $1`, [company]) + await sweep(`DELETE FROM public.companies WHERE id = $1`, [company]) + await sweep(`DELETE FROM auth.users WHERE id = $1`, [owner]) + }) + + /** + * Every policy on storage.objects that can destroy or rewrite an object and + * is not demonstrably scoped away from the documents bucket, with both its + * USING and WITH CHECK expressions and its grantee roles. + * + * polcmd '*' (FOR ALL) is included deliberately: it grants DELETE and + * UPDATE just as effectively as 'd' and 'w', and it is the shape the one + * legitimate policy here (service_role_all_documents) already uses, so a + * hostile FOR ALL policy would look unremarkable in the catalogue. + * + * WITH CHECK is read as well as USING: an UPDATE policy can carry its + * bucket restriction in either, and a policy whose USING is permissive + * would be invisible to a polqual-only check. + */ + async function destructivePoliciesOverDocuments(): Promise { + const res = await getPool().query<{ + polname: string + cmd: string + expr: string + roles: string[] + }>( + `SELECT p.polname, + p.polcmd::text AS cmd, + coalesce(pg_get_expr(p.polqual, p.polrelid), '') + || ' ' || coalesce(pg_get_expr(p.polwithcheck, p.polrelid), '') AS expr, + -- ::text is load-bearing: rolname is of type name, and + -- node-postgres hands back a raw "{authenticated}" string for a + -- name[] column instead of parsing it into a JS array. + ARRAY(SELECT rolname::text FROM pg_roles WHERE oid = ANY (p.polroles)) AS roles + FROM pg_policy p + WHERE p.polrelid = 'storage.objects'::regclass + AND p.polcmd IN ('d', 'w', '*')`, + ) + + return res.rows + .filter((r) => { + // In scope unless the policy provably cannot reach this bucket. Only + // a bucket_id predicate naming some OTHER bucket exempts it: an + // expression that never mentions bucket_id covers every bucket, + // documents included. That is exactly Supabase's stock "Enable delete + // for users based on user_id" template, `USING (auth.uid() = owner)`, + // which carries no bucket clause at all, so gating on the bucket name + // alone would wave through the widest possible hole. + // + // The bucket check is a substring rather than the exact + // `bucket_id = 'documents'` shape pg_get_expr emits today, since + // `bucket_id::text = 'documents'` or a reversed comparison would slip + // past a stricter match. For a WORM ratchet a false alarm is cheap and + // a silent hole is not. + if (!r.expr.includes('documents') && r.expr.includes('bucket_id')) return false + // An empty role array means the policy is granted to PUBLIC (oid 0 + // has no pg_roles row), which is the most permissive case there is, + // so it must NOT be read as "no client roles". + if (r.roles.length === 0) return true + // service_role bypasses RLS anyway and is how the application does + // its authorized deletes; every other grantee is client-reachable. + return r.roles.some((role) => role !== 'service_role') + }) + .map((r) => `${r.polname} (${r.cmd})`) + } + + it('no client-reachable DELETE, UPDATE or FOR ALL policy covers the documents bucket', async () => { + // receipts_delete is a different bucket and is intentionally deletable: + // receipts are pre-bookkeeping scratch, not rakenskapsinformation. An + // UPDATE policy would be as damaging as a DELETE one: it lets a user + // rewrite an object in place, defeating the version chain. + expect(await destructivePoliciesOverDocuments()).toEqual([]) + }) + + it('the ratchet sees a FOR ALL policy, which is how the real hole could return', async () => { + // Proves the assertion above is not vacuous. The dropped policy was + // FOR DELETE, but nothing stops the next dashboard edit from being + // FOR ALL, and that is the shape a polcmd IN ('d','w') check misses. + await getPool().query( + `CREATE POLICY worm_ratchet_probe ON storage.objects + FOR ALL TO authenticated + USING (bucket_id = 'documents') + WITH CHECK (bucket_id = 'documents')`, + ) + try { + expect(await destructivePoliciesOverDocuments()).toEqual(['worm_ratchet_probe (*)']) + } finally { + await getPool().query(`DROP POLICY IF EXISTS worm_ratchet_probe ON storage.objects`) + } + // ... and the catalogue is clean again once the probe is gone. + expect(await destructivePoliciesOverDocuments()).toEqual([]) + }) + + it('the ratchet sees a bucketless policy, which covers documents by omission', async () => { + // Supabase's stock "Enable delete for users based on user_id" template is + // `USING (auth.uid() = owner)` with no bucket clause, so it grants delete + // over EVERY bucket. Naming no bucket must not read as naming a safe one. + await getPool().query( + `CREATE POLICY worm_ratchet_bucketless ON storage.objects + FOR DELETE TO authenticated + USING (auth.uid() = owner)`, + ) + try { + expect(await destructivePoliciesOverDocuments()).toEqual([ + 'worm_ratchet_bucketless (d)', + ]) + + // And it is not merely reported: it really would let the uploader + // destroy their own rakenskapsinformation, which is why the catalogue + // assertion has to catch it. + await withUserContext(owner, async (client) => { + const res = await client.query( + `DELETE FROM storage.objects WHERE bucket_id = 'documents' AND name = $1`, + [legacyKey], + ) + expect(res.rowCount).toBe(1) + }) + } finally { + await getPool().query(`DROP POLICY IF EXISTS worm_ratchet_bucketless ON storage.objects`) + } + expect(await destructivePoliciesOverDocuments()).toEqual([]) + }) + + it('a policy scoped to another bucket is not flagged', async () => { + // The counterweight to the rule above: receipts_delete is real, lives in + // migration 20260710102000, and must not trip this ratchet. A ratchet + // that cries wolf on unrelated buckets gets switched off. + await getPool().query( + `CREATE POLICY worm_ratchet_other_bucket ON storage.objects + FOR DELETE TO authenticated + USING (bucket_id = 'sie-files')`, + ) + try { + expect(await destructivePoliciesOverDocuments()).toEqual([]) + } finally { + await getPool().query(`DROP POLICY IF EXISTS worm_ratchet_other_bucket ON storage.objects`) + } + }) + + it('the uploader cannot delete their own legacy-layout object', async () => { + // The exact production shape: [2] of `documents/{userId}/...` is the + // uploader's auth.uid(), which is what the dropped policy matched on. + await withUserContext(owner, async (client) => { + const res = await client.query( + `DELETE FROM storage.objects WHERE bucket_id = 'documents' AND name = $1`, + [legacyKey], + ) + // RLS filters the row out rather than raising: the DELETE reports + // success having removed nothing. That silence is why the hole was + // invisible from the application side. + expect(res.rowCount).toBe(0) + }) + + const after = await getPool().query( + `SELECT 1 FROM storage.objects WHERE bucket_id = 'documents' AND name = $1`, + [legacyKey], + ) + expect(after.rowCount).toBe(1) + }) + + it('a company member cannot delete a company-scoped object either', async () => { + await withUserContext(owner, async (client) => { + const res = await client.query( + `DELETE FROM storage.objects WHERE bucket_id = 'documents' AND name = $1`, + [companyScopedKey], + ) + expect(res.rowCount).toBe(0) + }) + + const after = await getPool().query( + `SELECT 1 FROM storage.objects WHERE bucket_id = 'documents' AND name = $1`, + [companyScopedKey], + ) + expect(after.rowCount).toBe(1) + }) +})