fix(payments): lock supplier payment batch inserts to the RPC and log the raw error behind create_failed (#2282)

* fix(payments): lock supplier payment batch inserts to the RPC and log the raw error behind create_failed Two residuals from PR #1989 (atomic create_supplier_payment_batch RPC). Root cause 1: the original table migration (20260810160748) left member INSERT policies on supplier_payment_batches and supplier_payment_batch_items. The RPC is SECURITY DEFINER and never consulted them, so their only effect was to let any company member insert straight through PostgREST (browser devtools, a raw JWT call) and skip the RPC's invoice locking, in-transaction active-batch recheck and header/items totals consistency. The single write path existed in code only, not in the database. Fix 1: new migration 20260904121000 drops "insert own-company supplier_payment_batches" and "insert own-company supplier_payment_batch_items". SELECT policies on both tables and the UPDATE policy on batches (the cancel route) are untouched. No application code inserts into either table. Root cause 2: createSupplierPaymentBatch discarded the RPC error object and returned a bare create_failed, so the tenant guard (42501), a constraint violation inside the SECURITY DEFINER body and a PostgREST schema-cache miss after a deploy (PGRST202) were indistinguishable from each other and from an empty payload or an unmapped refusal code. Fix 2: log the raw error (code, message, details, hint) plus companyId, batchId and item count through lib/logger before each of the three create_failed returns. The client-facing result is unchanged; debtor_snapshot and the item rows (IBAN, payee data) are never logged. Tests: pg-real asserts the exact remaining policy set, that a member's and the owner's direct INSERT into either table is refused by RLS (42501), and that the same member still creates through the RPC and cancels through UPDATE. Unit tests assert the logger receives the raw error fields and that create_failed is still returned. Fixes #2060 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * docs(decisions): carry the ten-issue batch decision lines in one PR Append the decision lines for PRs #2272 through #2282 here so the other nine PRs in the batch do not touch DECISIONS.md and stay mergeable in any order (the union merge driver is ignored by GitHub). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * fix(payments): redact and bound raw RPC error text before logging Addresses the Superagent P2 on PR #2282 (lib/payments/batch-service.ts): message, details and hint from Postgres/PostgREST were logged verbatim, and Postgres quotes the entire failing row in details on CHECK and NOT NULL violations ("Failing row contains (..., SE45..., Anna Andersson, ...)"), so payee and account data could reach the log line. Excluding debtor_snapshot and the item rows did not cover the error text itself. Fix: a call-site helper, boundedRedactedText, runs each of the three text fields through lib/observability/redact.ts redactString (SE IBANs, personnummer, emails, API keys), drops any "Failing row contains (...)" payload whole (no pattern catches a payee name), and bounds the result to 500 chars, redaction before bounding so a cut IBAN cannot leave a digit fragment behind. The SQLSTATE code stays verbatim; the client-facing create_failed result is unchanged. Test: rejected RPC error carrying an IBAN in message, the full failing row (IBAN, payee name, account) in details and an oversized hint with the IBAN straddling the bound; asserts the serialized log context contains none of them, the row payload is replaced, and the hint is <= 500 chars ending in [TRUNCATED]. DECISIONS.md line for #2060 updated accordingly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * fix(payments): drop the dotAll regex flag, tsconfig targets ES2017 The failing-row pattern used the `s` flag, which TypeScript rejects below es2018 (TS1501) and broke Build (zero extensions). `[\s\S]*` matches across newlines on every target. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * fix(payments): log code and message only for a failed batch RPC Reworks the logging half of #2060 from first principles. The diagnostic value of a failed create_supplier_payment_batch call lies in the SQLSTATE code and the message: the RPC's own RAISE text, "violates check constraint <name>", "duplicate key value violates unique constraint <name>". details is exactly where Postgres puts row data ("Failing row contains (...)", "Key (...)=(...)") and hint adds nothing operational, so neither is logged at all. That removes the payee/account exposure Superagent flagged on #2282 without the bespoke redact-and-bound helper, its regex and the TS-target workaround it needed: boundedRedactedText, FAILING_ROW_PATTERN, RPC_ERROR_TEXT_MAX and TRUNCATED are deleted, and the redact import goes with them. The logger's own redaction stays as the safety net for message. Client-facing result unchanged (create_failed). Test: an RPC error carrying an IBAN and a payee name in details and hint; the serialized log context contains neither field in any shape, and rpcError is exactly { code, message }. Exact-match and PGRST202 tests updated to the two-field shape. DECISIONS.md line for #2060 rewritten. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * docs(decisions): record the first-principles rework of the ten-issue batch Replace the decision lines for #2263, #2250, #2256 and #2211 with the reworked shapes, add the shared customer-share definition for #2248, and note the CLAUDE.md principle (#2283) that drove the rework. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u * docs(decisions): note the fiscal-year selection cap on #2280 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qgLgdt4mLmha1ZLFMwq1u --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-04 17:19:18 +02:00
parent 68fee7dbe7
commit db289e3bdc
5 changed files with 419 additions and 5 deletions
@@ -2,7 +2,7 @@ import { randomUUID } from 'node:crypto'
 import type { PoolClient } from 'pg'
 import { describe, expect, it } from 'vitest'
 import { getClient, getPool, withUserContext } from './setup'
-import { seedCompany, insertAuthUser } from './fixtures'
+import { seedCompany, insertAuthUser, insertCompanyMember } from './fixtures'

 // pg-real coverage for 20260810160748_supplier_payment_batches.sql: RLS
 // isolation on both tables, the FK RESTRICT that keeps invoices referenced by
@@ -14,6 +14,16 @@ import { seedCompany, insertAuthUser } from './fixtures'
 // the atomic create RPC (happy path, in-transaction active-batch recheck,
 // header + items rolling back together, FOR UPDATE serialization of two
 // concurrent creates, tenant guard and actor pinning, EXECUTE privileges).
+//
+// And 20260904121000_supplier_payment_batches_drop_insert_policies.sql
+// (#2060): the member INSERT policies on both tables are gone, so the
+// SECURITY DEFINER RPC is the only write path in the database as well as in
+// code, while SELECT (both tables) and the batches UPDATE (cancel) stay.
+//
+// Every fixture that seeds a batch or an item directly (insertBatch,
+// insertItem, seedBatchWithItem) runs on the plain pool, i.e. the superuser
+// connection outside withUserContext, so none of them ever relied on the
+// dropped INSERT policies.

 async function insertSupplier(companyId: string, userId: string): Promise<string> {
  const id = randomUUID()
@@ -683,3 +693,154 @@ describe('create_supplier_payment_batch RPC', () => {
    expect(rows[0]).toEqual({ anon_can: false, authenticated_can: true, service_role_can: true })
  })
 })
+
+// ---------------------------------------------------------------------------
+// INSERT locked to the RPC (#2060)
+// ---------------------------------------------------------------------------
+
+/** A plain 'member' (not the owner seedCompany creates): the least-privileged
+ *  role the RPC still accepts as a writer. */
+async function seedMember(companyId: string): Promise<string> {
+  const userId = await insertAuthUser()
+  await insertCompanyMember({ companyId, userId, role: 'member' })
+  return userId
+}
+
+async function captureError(
+  run: () => Promise<unknown>,
+): Promise<{ code?: string; message?: string } | null> {
+  try {
+    await run()
+    return null
+  } catch (err) {
+    return err as { code?: string; message?: string }
+  }
+}
+
+const DIRECT_BATCH_INSERT = `
+  INSERT INTO public.supplier_payment_batches
+    (id, company_id, user_id, format, total_amount, item_count, msg_id, debtor_snapshot)
+  VALUES ($1, $2, $3, 'pain001', 737.5, 1, 'X', '{}')`
+
+const DIRECT_ITEM_INSERT = `
+  INSERT INTO public.supplier_payment_batch_items
+    (batch_id, company_id, supplier_invoice_id, amount, payment_date,
+     payee_type, payee_bankgiro, payee_name, reference_type, reference)
+  VALUES ($1, $2, $3, 737.5, '2099-08-15', 'bankgiro', '50501055',
+          'Derome Bygg AB', 'invoice_number', 'CD3014794407')`
+
+describe('supplier_payment_batches: INSERT locked to the RPC (#2060)', () => {
+  it('leaves exactly the SELECT policies and the batches UPDATE policy in the catalog', async () => {
+    const { rows } = await getPool().query<{
+      tablename: string
+      policyname: string
+      cmd: string
+    }>(
+      `SELECT tablename, policyname, cmd
+         FROM pg_policies
+        WHERE schemaname = 'public'
+          AND tablename IN ('supplier_payment_batches', 'supplier_payment_batch_items')
+        ORDER BY tablename, cmd, policyname`,
+    )
+    expect(rows).toEqual([
+      {
+        tablename: 'supplier_payment_batch_items',
+        policyname: 'view own-company supplier_payment_batch_items',
+        cmd: 'SELECT',
+      },
+      {
+        tablename: 'supplier_payment_batches',
+        policyname: 'view own-company supplier_payment_batches',
+        cmd: 'SELECT',
+      },
+      {
+        tablename: 'supplier_payment_batches',
+        policyname: 'update own-company supplier_payment_batches',
+        cmd: 'UPDATE',
+      },
+    ])
+  })
+
+  it('refuses a direct INSERT into supplier_payment_batches from a member and from the owner', async () => {
+    const ctx = await seedCompany()
+    const memberId = await seedMember(ctx.companyId)
+
+    for (const userId of [memberId, ctx.userId]) {
+      const err = await captureError(() =>
+        withUserContext(userId, (client) =>
+          client.query(DIRECT_BATCH_INSERT, [randomUUID(), ctx.companyId, userId]),
+        ),
+      )
+      expect(err?.code).toBe('42501')
+      expect(err?.message).toMatch(/row-level security/)
+    }
+  })
+
+  it('refuses a direct INSERT into supplier_payment_batch_items from a member and from the owner', async () => {
+    // The batch itself is seeded on the superuser pool; only the item insert
+    // runs under the member's JWT.
+    const ctx = await seedBatchWithItem()
+    const memberId = await seedMember(ctx.companyId)
+    const otherInvoice = await insertSupplierInvoice(ctx.companyId, ctx.userId, ctx.supplierId)
+
+    for (const userId of [memberId, ctx.userId]) {
+      const err = await captureError(() =>
+        withUserContext(userId, (client) =>
+          client.query(DIRECT_ITEM_INSERT, [ctx.batchId, ctx.companyId, otherInvoice]),
+        ),
+      )
+      expect(err?.code).toBe('42501')
+      expect(err?.message).toMatch(/row-level security/)
+    }
+    expect(await countItems(null, ctx.batchId)).toBe(1)
+  })
+
+  it('still creates through the RPC and cancels through UPDATE for the member whose direct INSERT was refused', async () => {
+    const ctx = await seedInvoiceOnly()
+    const memberId = await seedMember(ctx.companyId)
+    const batchId = randomUUID()
+
+    const outcome = await withUserContext(memberId, async (client) => {
+      // Same session, same authenticated role: the direct write is refused...
+      await client.query('SAVEPOINT direct_insert')
+      const direct = await captureError(() =>
+        client.query(DIRECT_BATCH_INSERT, [batchId, ctx.companyId, memberId]),
+      )
+      await client.query('ROLLBACK TO SAVEPOINT direct_insert')
+
+      // ...while the SECURITY DEFINER RPC, which never consulted the dropped
+      // policies, still lands header + items for the same caller.
+      const result = await callRpc(client, {
+        companyId: ctx.companyId,
+        batchId,
+        items: itemsPayload(ctx.invoiceId),
+      })
+      const items = await countItems(client, batchId)
+
+      // The kept UPDATE policy: the member cancels the batch the RPC created
+      // (the cancel route's compare-and-set), and the kept SELECT policy shows
+      // the result.
+      const cancel = await client.query(
+        `UPDATE public.supplier_payment_batches
+            SET status = 'cancelled', cancelled_at = now(), cancelled_by = $2
+          WHERE id = $1 AND status = 'created'`,
+        [batchId, memberId],
+      )
+      const visible = await client.query<{ status: string }>(
+        `SELECT status FROM public.supplier_payment_batches WHERE id = $1`,
+        [batchId],
+      )
+      return { direct, result, items, cancelled: cancel.rowCount, visible: visible.rows }
+    })
+
+    expect(outcome.direct?.code).toBe('42501')
+    expect(outcome.direct?.message).toMatch(/row-level security/)
+    expect(outcome.result.ok).toBe(true)
+    if (!outcome.result.ok) throw new Error('unreachable')
+    expect(outcome.result.batch.id).toBe(batchId)
+    expect(outcome.result.batch.user_id).toBe(memberId)
+    expect(outcome.items).toBe(1)
+    expect(outcome.cancelled).toBe(1)
+    expect(outcome.visible).toEqual([{ status: 'cancelled' }])
+  })
+})