accounted

Author	SHA1	Message	Date
Jakob Wennberg	ec27228a8e	style: remove em/en dashes repo-wide, add CLAUDE.md rule against them (#890 ) Em dashes (—) and en dashes (–) had spread across comments, docs, tests, and a few UI strings, reading as AI-generated boilerplate rather than house style. Replaced each with punctuation matching its context: colon for explanatory clauses, comma for asides, plain hyphen for numeric/legal ranges (e.g. "21-23§"), "to"/"till" for date ranges, parentheses for paired-dash asides. messages/en.json and messages/sv.json were fixed by hand together to keep sv/en in sync. Left untouched where the dash is the functional subject rather than decorative punctuation: date-range-parser.ts's separator regex, charset-repair.ts's CP1252 byte-mapping table (and its test), the SIE encoding mojibake docs, generic-csv.ts's minus-sign normalizer, the agent system-prompt files that already instruct against em dashes, and a golden iXBRL test fixture compared byte-for-byte. Also fixes two bugs surfaced along the way: an off-by-one in ApiKeysPanel's scope-label split (a leftover from an earlier partial pass), and a charset-repair test that had lost the literal en-dash it exists to verify. Regenerated the agent atom seed migration (skills:generate) since 27 SKILL.md files changed. Added a CLAUDE.md rule against em/en dashes, with an explicit carve-out for the functional-dash cases above. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-07-04 15:58:06 +02:00
Jakob Wennberg	f8504f3bd0	fix: audit batch — pagination truncation, MFA/dead-code cleanup, mark-paid fail-closed (#841 ) * fix(reports): paginate 8 more report/ledger queries (1000-row truncation) Raw .select() without fetchAllRows() silently caps at PostgREST's 1000-row limit, producing wrong statutory output for high-volume companies. Following #806 (trial-balance/VAT), wrap the remaining offenders in fetchAllRows + a stable .order('id') + dedupeBy: - ink2-engine / ne-engine: INK2 & NE-bilaga tax declarations under-counted - ar-reconciliation (1510/1513), supplier-reconciliation (2440): phantom "Ej avstämd" gaps - full-archive-export: 7-year DR archive (added a unique total order so rows are not silently skipped/duplicated across pages) - avgifter-basis, currency-revaluation, vat-declaration Adds a regression guard test asserting >1000 ledger lines are summed, not truncated at 1000. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api): close extension-dispatcher MFA gap, scope /api/events to API key, sweep dead code Security/correctness: - ext/[...path] dispatcher now uses requireAuth() instead of inline supabase.auth.getUser(), enforcing MFA (AAL2) on hosted across the whole enabled-extension surface (banking sync, document upload/booking, supplier invoices, migration). Ratchets antipatterns-baseline raw-route-auth 168->165. - /api/events now filters by the API key's bound company_id instead of the user's active company (was a cross-company read with a scoped key). - enable-banking OAuth callback calls ensureInitialized() at module load so the PSD2 consent audit event (ASVS V16 / GDPR Art.30) isn't dropped on a cold-start instance. Dead-code sweep (all confirmed zero importers): - delete lib/tax/calculator.ts, lib/salary/engangsskatt.ts (+test), lib/email/resend.ts, lib/salary/salary-transaction-matcher.ts, lib/webhooks/diff.ts, lib/salary/effective-values.ts, lib/bookkeeping/template-prompt.ts - trim unused lib/vat/eu-countries.ts helpers (keep EU_COUNTRIES) - remove dead getAutomaticStatus() and the abandoned Activepieces CSP entry Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(invoices): fail closed when a payment journal entry doesn't post Three mark-paid paths (legacy route, v1 API, agent commit) diverged on the "mark paid but the JE failed" case — two would flip the invoice to paid (or leave an orphaned posted voucher) with no booking, silently diverging the GL from the AR/AP sub-ledger. Unify on fail-closed: - legacy + v1 + agent commitMarkInvoicePaid: never mark paid without a posted voucher; on a null/failed JE return INVOICE_PAID_BOOK_FAILED before any state mutation (v1 mirrors the match-invoice strict mode). - agent path: add the .in('status',[...]).select('id') CAS guard and cancel the orphaned voucher (cancelOrphanedPaymentEntry) on a lost race or update error, matching the web route. - legacy route: cancel the orphan on a non-race update error too (was only handled on the race branch). - supplier mark-paid: stop swallowing a failed supplier_invoice_payments insert — that row drives the reversal amount in payment-sync; roll back the status flip and cancel the voucher instead. - pending-ops orchestrator: error-check the terminal 'committed' write so an op stranded in 'committing' (the expire sweep only targets 'pending') is at least logged loudly. Adds a guard test for the legacy fail-closed path. Full unit suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): unblock core build + address compliance-review findings - avgifter-basis.ts: fix the core-build TypeScript error — PostgREST's type-level select parser models the salary_run embed as an array, which wasn't assignable to the object-typed generic. Type it `unknown` (rows are read via an explicit cast), making it robust across postgrest-js versions. - /api/events: add a non-null companyId guard before the event_log query (defense-in-depth for the API-key-bound scope) — addresses ASVS V8.2.1 / ISO A.5.15. - supplier mark-paid: add a CAS guard (.eq('status', newStatus)) to the payment-insert-failure rollback so a concurrent settlement can't be clobbered — addresses ASVS V2.3. - dispatcher: add an AAL2 regression test asserting a non-MFA session is rejected (403) and the extension handler never runs — addresses the GDPR Art.32 review ask for the single extension chokepoint. Verified deletions are safe: effective-values.ts was a dead duplicate — the live AGI/payslip path inlines the same `?? override` coalescing (generate-declaration.ts), so AGI correctness is unaffected. next build: exit 0. Full unit suite: 6147 passing. ESLint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-30 14:34:23 +02:00
Jakob Wennberg	b94ed3bec2	feat(api): cookbooks + webhook audit_log + secret rotation (PR-500 carry-overs) (#501 ) * docs(api): ship 4 cookbook recipes (close docs polish backlog) Promotes the four placeholder cookbook entries to full narrative recipes matching the Stripe-grade quality bar set by quickstart + webhooks. Closes the docs follow-up bucket from the PR-500 description's deferred list. Recipes: - ingest-bank-transactions: bank-file upload (CSV / CAMT.053 auto-detect) → async poll → list uncategorised → suggest-categories → categorize (single + batch) → match-invoice / match-supplier-invoice. Multicurrency notes covering Riksbanken FX lookup and the kontantmetoden partial- payment guard. - file-vat-declaration: GET /reports/vat-declaration → rutor 05–62 walkthrough → GL reconciliation block → 2026-04-01 livsmedel 12% → 6% transition explicitly covered (delivery_date supply-date rule) → voucher- gap pre-flight → period lock workflow → manual Skatteverket Mina Sidor submission with confirmation-reference capture → EU / reverse-charge / import handling. - run-payroll-and-agi: draft → calculate → approve → mark-paid → book → generate-agi state machine. Per-step idempotency, strict-mode book failure semantics, förmånsbeskattning + bilförmån + bruttolöneavdrag vs nettolöneavdrag ordering. AGI XML download for manual Mina Sidor upload (direct API submission requires BankID via the Skatteverket extension, not the public REST surface). - year-end-closing: IB/UB continuity check per BFL 5 kap → voucher-gap pre-flight → missing-documents pre-flight → lock (reversible) → year- end async operation (resultatdisposition + periodiseringsfond + överavskrivningar + bolagsskatt + opening-balance batch) → close (irreversible per BFL 5 kap 8 §, typed-phrase confirmation) → årsredovisning + INK2/NE generation. Brutet räkenskapsår variant documented. Each cookbook follows the same shape as the existing quickstart and webhooks recipes — concrete curl commands, response samples, common pitfalls, next-steps cross-links. Lengths are deliberately uneven: the year-end recipe is longest because the consequences of getting it wrong are most severe (BFL violations, irreversible close). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(api): V16 audit_log entries for webhook lifecycle + secret rotation endpoint Two intertwined changes that together close the "real audit attribution gap in actively-used routes" item from the PR description. 1. POST /api/v1/companies/{companyId}/webhooks/{id}/rotate-secret New endpoint that issues a fresh HMAC signing secret and invalidates the previous one immediately. Returns the new secret EXACTLY ONCE in the response, mirroring the create-time contract. Required scope: webhooks:manage. Idempotency-Key mandatory. Rotation is instant — no grace period. Documented workflow: stage the new secret on the receiver side (separate config slot, not yet active) → POST /rotate-secret → activate the new secret on the receiver → POST /webhooks/{id}/test to verify. A "previous_secret" column with TTL-based grace window (Stripe-style) is the natural follow-up; the instant-rotation shape ships first because it closes the "secret leaked, need to rotate now" use case with minimum new surface. The route is wired into load-routes.ts and lib/auth/scopes.ts. Spec snapshot updated. 2. V16 audit_log entries on every webhook lifecycle mutation The audit_log column shape (user_id, company_id, action, table_name, record_id, actor_id, old_state, new_state, description) is exactly what V16 / Art.32(1)(b) / A.8.24 audit-trail requirements call for. Wired entries on: - POST /webhooks (create) — action INSERT, new_state captures the row WITHOUT the secret (signing material must not land in the audit trail; only secret-event metadata). - PATCH /webhooks/:id (update) — action UPDATE, before/after pair so reviewers can reconstruct exactly what changed. - DELETE /webhooks/:id (delete) — action DELETE, old_state snapshot so the row's prior state survives the delete. - POST /webhooks/:id/rotate-secret — action SECURITY_EVENT, new_state carries the event marker only (no secret value). - dispatcher.disableWebhook (auto-disable on HTTP 410 / redirect / url_unsafe) — action SECURITY_EVENT, before/after capturing the disable cause for SIEM correlation. actor_id is set to ctx.apiKeyId on caller-driven entries so the audit row points back to the specific API key that triggered the change (PR-500 round-1 CC6.3 finding: actor attribution via created_by_api_key_id alone leaves a gap if a key is deleted — keeping the actor_id in audit_log closes that). 4 new integration tests cover the rotate-secret happy path, 404, 401 unauthorized, and Idempotency-Key required. The existing webhook integration tests continue to pass because the audit_log inserts fall through to the default mock response (no-op) without disturbing the per-table queues. 39 integration tests pass on the webhook surface (+4 vs round-2). Total: 3588 unit tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-501 review round 1 — correctness + Swedish compliance Round 1 of review fixes. Two real correctness bugs Greptile caught, two audit-trail gaps, and four Swedish-compliance errors in the cookbook prose. Compliance Swarm has 17 findings (0 blocking); the 4 architectural items (secret-at-rest encryption, dedicated rotate scope, rate-limit on rotation, URL redaction) remain deferred with rationale. Greptile (3 / 3 — all addressed): 1. rotate-secret silent 0-row UPDATE — fixed by adding `.select('id').maybeSingle()` to the UPDATE and returning NOT_FOUND when no row was touched. Closes the TOCTOU window between the existence check and the secret update; a concurrent DELETE no longer hands the caller a freshly-generated secret that no webhook in the database matches. 2. DELETE handler audit_log silently skipped when prior snapshot is null — fixed by writing the audit row UNCONDITIONALLY with `old_state: prior ?? null` and a degraded description when the snapshot is unavailable. A successful DELETE now always produces exactly one audit row (CC6.3 attribution contract). 3. Typo "bookslut" → "bokslut" in year-end-closing.ts. Compliance Swarm code-quality items addressed: 4. PATCH new_state now derived from the DB-confirmed returned `data` with an explicit field allowlist, not from the request-body-derived `update` object (A.8.11 / V16.1.1). Closes the gap where a future trigger that rejects a field would leave the audit trail out of sync with the actual stored state. 5. All four route-side audit_log inserts (create, update, delete, rotate-secret) now capture the insert error and emit a structured warning via ctx.log; mirrors the dispatcher pattern (CC7.2). 6. Dispatcher null-user_id path now emits a structured warning instead of silently skipping the audit_log entry — SIEM can alert on the gap (CC7.2 / V16.1.1 / A.8.15). Swedish compliance (cookbook content fixes — all real errors): 7. VAT cookbook ruta 06 label corrected: "Övrig försäljning (ej skattepliktig)" → "Momspliktig försäljning som inte ingår i ruta 05" (Skatteverket's verbatim label). The old label conflated exempt vs zero-rated supplies and would cause integrators to omit export / EU zero-rated sales from box 06. 8. Livsmedel rate-change framing rewritten: leads with the supply-date rule (ML 1 kap 3 §) as the decisive date, not invoice_date. The old opening sentence ("invoices created with invoice_date >= 2026-04-01 book to 2631") was wrong on its face — a copy-paste reader would mis-book pre-cutover deliveries invoiced in April at the new 6% rate. 9. Reverse-charge EU 2645 note adds the blandad-verksamhet caveat: "Net zero impact on cash flow" only holds when full avdragsrätt applies; partial avdragsrätt requires proportional restriction per HFD 2023 ref. 45. 10. Payroll cookbook age bounds corrected: "under-25 / over-66" → "18-22 years old (born 2003-2007) / 67+ from 2026", per Prop. 2025/26:66. The old bounds would cause integrators to apply the reduced rate (20.81%) to 23-24-year-olds who must pay 31.42%, producing non-compliant AGI files. 11. Payroll cookbook BAS 2615 corrected to 2731 (Avräkning sociala avgifter). 2615 is "Utgående moms vid import" in BAS 2026 — using it for the payroll liability would misclassify a payroll payable as an import-VAT payable and break moms reconciliation. 12. Year-end cookbook periodiseringsfond cap base corrected: IL 30 kap 5 § cap is on taxable profit BEFORE the periodiseringsfond deduction itself (and after schablonintäkt is added back). Note on materiellt samband (BFNAR 2016:10 kap 13) added — the reservation is BOOKED on 2110-2139, not declaration-only. Deferred to follow-ups (architectural / out of scope for round 1): - Secret-at-rest encryption (CC6.1 / Art.5(1)(f)): PR-1 architectural carryover, applies to existing webhooks.secret column too. - Dedicated `webhooks:rotate` scope (CC6.3 informational): introduces friction without closing a real gap when the only caller-driven action gated by `webhooks:manage` is the rotation itself. - Per-route rate-limit on :rotate-secret (Art.32 abuse case): part of the wider per-route rate-limit pass already on the deferred list. - webhook_url redaction in audit_log (Art.5(1)(c)): URLs are admin- supplied configuration values with no expected sensitive params; truncation would degrade audit value for legitimate review. 23 webhook integration tests pass locally (no regressions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-501 review round 2 — atomic mutations + audit completeness + cookbook compliance Round 2 of review fixes. Compliance Swarm flagged refinements to the round-1 fixes; Swedish-compliance had a fresh batch of cookbook items (including a self-contradiction in payroll pitfalls I missed last round). All addressed. Code changes — atomicity + audit completeness: 1. rotate-secret collapsed to a single UPDATE … RETURNING (V8.2.1). The preflight existence-check SELECT was redundant after round 1 added .select().maybeSingle() on the UPDATE — the same null-row signal indicates non-existence, but in one round trip with no TOCTOU window. RETURNING `name` so the audit_log description still carries a human identifier without a second read. 2. DELETE handler collapsed to atomic .delete().select().maybeSingle() (V8.2.1). Eliminates the pre-read TOCTOU window entirely. A 0-row delete (already-deleted webhook) still returns 204 — idempotent DELETE — and the audit entry captures the attempt with old_state: null. Description discriminates the two cases ("deleted: name" vs "delete attempted on missing id"). 3. Cache-Control: no-store, no-cache, must-revalidate, private on the rotate-secret response (Art.25). The HMAC secret is sensitive credential material returned exactly once; this header prevents any intermediary (CDN, proxy, gateway access log, browser cache) from persisting the response body in a store with a different retention policy than intended. 4. Dispatcher auto-disable now writes the audit_log entry UNCONDITIONALLY (A.8.15 / V16.1.1 / CC7.2). Previously a null prior snapshot or a legacy null user_id caused the audit row to be silently skipped — only a warn log was emitted. Now writes user_id=NULL when unavailable (post-multi-tenant-refactor schema allows it; row is invisible under user RLS but queryable under service-role review, which is correct for system-initiated SECURITY_EVENT records). Description discriminates the snapshot- available / snapshot-unavailable cases. Swedish compliance — cookbook content fixes (all real errors): 5. VAT cookbook rounding rule corrected: SFL 22 kap 1 § mandates TRUNCATION of öre (Math.floor for positive amounts), not half-up rounding. Last round mislabeled this as "Math.round (half-up)"; the SRU filing skill is canonical and uses truncation. Using Math.round would produce values that differ from Skatteverket's expectations and cause GL-reconciliation mismatches at the öre level. 6. VAT reconciliation block now includes 2614 (Utgående moms vid omvänd skattskyldighet, matches ruta 30). The previous list of 2611/2621/2631/2641/2645 omitted 2614; a reconciliation that skips it would show rutor_match_gl: true even when the 2614 balance is non-zero and un-reconciled. 7. Livsmedel rate-change adds a one-sentence caveat for continuous/ subscription supplies — the supply-date framing in round 1 was too tight for cases where multiple deliveries roll up into a subscription. Confirms against ML 1 kap 3 § rather than assuming a single delivery date is decisive. 8. Payroll pitfalls bullet contradicted step 2 — "Employees under 26 (2024 rule for 2026 birth year ≥ 2001)" rewritten to match step 2: "18–22 years old at the start of 2026 (born 2003–2007) AND 67+ from 2026". An integrator reading only the pitfalls section would have applied the reduced rate too broadly, producing underpaid arbetsgivaravgifter and a non-compliant AGI. 9. Year-end periodiseringsfond cap now states schablonintäkt explicitly: 1.94% × outstanding prior-year balance (SLR + 1% for 2026) is ADDED to taxable income before the 25% cap is computed. Last round mentioned the "BEFORE the periodiseringsfond deduction" ordering but elided the schablonintäkt step; omitting it produces a cap that's too low when prior-year reserves exist. 10. Year-end SRU format characterization corrected: SRU is plain text encoded in ISO 8859-1, NOT XML. iXBRL (XML-based) is the Bolagsverket digital annual-report format — a separate artefact for a separate authority. Round 1 conflated them. Deferred (architectural / out of scope, documented in commit): - Audit-log dead-letter queue / SIEM alert escalation (Art.32 / A.8.15): infra setup, not code-PR scope. The warn-on-failure path is the in-process surface; durable delivery is a SRE/SIEM concern. - Secret encryption at rest (CC6.1): PR-1 architectural carryover. - webhook_url + description redaction in audit_log (Art.5(1)(c)): URLs are admin-supplied configuration values; redaction would degrade audit reconstructibility without closing a real PII gap. - PATCH old_state TOCTOU via Postgres function (CC6.3): the read- then-write pattern produces an append-only audit row capturing the read state; the small race window is non-load-bearing for audit purposes and a stored-procedure refactor exceeds the cost/value. 23 webhook integration tests pass locally (no regressions). Type-check clean for all changed files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-501 review round 3 — real cookbook tax errors + cache-control on create Round 3 closes two tax-impact errors in the cookbooks plus the consistency gap on the create response. Compliance Swarm's remaining findings are recurring architectural carryovers or oscillation against prior rounds. Real cookbook errors (would mislead integrators): 1. Schablonintäkt rate corrected. Round 2 hardcoded 1.94% — that's the 2024 rate (SLR 0.94% + 1%). For 2026 SLR is 2.55%, so the rate is 3.55%. A wrong rate produces a too-low add-back, a too-high periodiseringsfond cap, and an IL 30 kap compliance error for any integrator copying the cookbook number. Rewrite to describe the formula (SLR + 1%, where SLR is the Riksbank statslåneränta on 30 Nov of the preceding year) with the 2026 figure as an example, and note the engine reads the canonical rate from `tax_rates`. 2. SRU format is a TWO-file pair, not one. Round 2 correctly said "plain text encoded in ISO 8859-1 (NOT XML)" but described it as a single file. Skatteverket requires both INFO.SRU (metadata header) AND BLANKETTER.SRU (declaration body) uploaded together — a single-file upload is rejected by their validation. Fix the prose to describe the two-file pair explicitly. Code consistency: 3. POST /webhooks (create) now returns the same `Cache-Control: no-store, no-cache, must-revalidate, private` + `Pragma: no-cache` headers as the rotate-secret endpoint (A.8.12). Both endpoints return the HMAC secret exactly once; both need the same intermediary-cache prevention. Smaller cookbook refinements (round 3 bot follow-ups): 4. VAT reconciliation block now includes 2615 (Utgående moms vid import, matches ruta 60) — the previous list covered 2611-2645 but omitted import VAT. A reconciliation that skips 2615 would show rutor_match_gl: true falsely for any importer. 5. Service supply-date fallback statement qualified to "one-off service supplies where delivery and invoice coincide" — long- running service contracts (subscriptions, maintenance) have per-delprestation skattskyldighet and need an explicit delivery_date per billing cycle. 6. Payroll elder-reduction boundary clarified: "67 years or older AT THE START OF the income year (1 January 2026)" — a 66-year- old whose 67th birthday falls in February does NOT qualify in 2026. Prevents misreading the pithy "67+ from 2026" as a birthday-during-year rule. Bot oscillation (skipping with rationale documented here for posterity): - Compliance Swarm Art.25 now asks to REMOVE webhook_url from DELETE old_state — direct contradiction with CC6.3's round-1 ask for complete attribution. webhook_url is admin-supplied configuration, not PII; keeping it preserves audit reconstructibility. - Swedish-compliance flags the unconditional re-delete audit row as "polluting" the behandlingshistorik — direct contradiction with Compliance Swarm V8.2.1 + CC6.3 round-1 / round-2 asks for unconditional writes. The audit_log is operational, not BFL räkenskapsinformation (which lives on journal_entries and related tables under explicit immutability triggers). Audit trail completeness wins over BFL purity for this table. Architectural carryovers (already documented in earlier commit bodies as deferred to follow-up PRs): - Secret encryption at rest (CC6.1, recurring) - Audit-log dead-letter / SIEM alerting (Art.32 / A.8.15, infra) - webhook_url userinfo stripping (A.8.11 low — URLs are admin- configured, no expected credentials; validating at registration would be a registration-time concern, not audit-time) 23 webhook integration tests pass. Type-check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-15 21:31:47 +02:00
Jakob Wennberg	afb21ea638	feat(api): Phase 6 PR-3 — substrate hardening (SKIP LOCKED + DNS pinning + test debt) (#500 ) * feat(api): operations table immutability trigger BFNAR 2013:2 kap 8 § behandlingshistorik integrity: once an operations row is in a terminal status (succeeded / failed / cancelled) the audit record of what happened becomes immutable. Adds the BEFORE UPDATE and BEFORE DELETE triggers that the webhook_deliveries table already has (20260515170000 / 20260515190000), mirroring their predicate shape and error code exactly. Closes the Phase 4 PR-2 (PR #469) review-round carry-over flagged by Swedish-compliance: previously a future bug, a privileged operator, or a compromised service-role caller could rewrite "this year-end close succeeded" to "failed" by updating an already-terminal row. The running → succeeded/failed/cancelled transition itself stays legal because the trigger keys on OLD.status, which is non-terminal at the moment of the legitimate UPDATE. pg test covers all transitions (allowed and blocked) plus DELETE on both terminal and non-terminal rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(api): atomic SKIP LOCKED claim for webhook dispatch Replaces the SELECT-then-UPDATE-intersect pattern in the dispatcher with a single-roundtrip SQL function using FOR UPDATE SKIP LOCKED. PostgREST can't express SKIP LOCKED through the JS client, so the previous shape relied on a CAS guard inside an UPDATE WHERE status IN ('pending','failed') to ensure only one of two overlapping cron ticks claimed any given row. The CAS pattern was correct (under load — receivers >60s could push a batch past the next minute's tick) but burned two round trips and forced the application to negotiate the locking semantics in JS. The function form moves the contention to the DB, where SKIP LOCKED makes a row held by a concurrent tick simply invisible to the second caller. One round trip, no JS-side intersect. All filter semantics are preserved verbatim inside the function: status IN ('pending','failed'), next_attempt_at <= now, webhook_id IS NOT NULL, ORDER BY next_attempt_at ASC, LIMIT batchSize. p_batch_size is bounded (0, 1000] to forestall a runaway lock-set in case a caller misconfigures it. pg test covers basic claim (pending + failed), future-due skip, dangling- row (webhook_id IS NULL) skip, terminal-status skip, batch-size limits, out-of-range argument rejection, and the SKIP LOCKED invariant itself using two concurrent pool clients in BEGIN — the second caller does not see the row A locked, no double-delivery. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(api): pinned-IP HTTPS dispatch (close DNS-rebinding window) The url-guard.ts file header openly flagged the remaining gap: "a separate DNS-rebinding window (between dispatch-time validation and the actual fetch) remains; closing that requires a custom HTTPS agent that pins the resolved IP — tracked for follow-up." This closes it. The previous shape was: 1. validateWebhookUrl() → DNS resolves to [public IP], returns ok 2. fetch(webhook_url) → re-resolves DNS; an attacker who flipped the A record in the interval gets a private-IP socket The new pinnedHttpsFetch helper validates DNS once, then opens a node:https.request to that pinned IP — but keeps the original hostname in the TLS SNI extension (so the receiver's cert validates) and in the HTTP Host header (so vhost routing still works). The request socket never re-resolves DNS, foreclosing the rebind race entirely. Built on node:https.request rather than undici's Agent so the project doesn't take on a new dep — the stdlib API is also more explicit about the SNI / Host / pinned-IP split. Test seam injects both validateUrl and httpsRequest so the unit tests verify the pinning shape without standing up an HTTPS server. The dispatcher's attemptDelivery is rewritten as a switch over the four PinnedFetchResult kinds (ok / unsafe_url / redirect_blocked / timeout / transport_error). The previous fetch-based code path that distinguished redirect rejection by string-matching err.message is gone — the new result type makes the distinction structural. 8 unit tests cover the SNI/Host/pinned-IP shape, port handling, redirect_blocked, transport_error, timeout, response-body truncation, first-IP determinism, and the validation short-circuit (never opens a socket when the URL fails the SSRF guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(api): pg tests for webhook substrate triggers (PR-1 test debt) CLAUDE.md ("Testing" + "Migration Rules") mandates a .pg.test.ts for any PR touching a trigger / RPC / RLS / DEFERRABLE constraint. Phase 6 PR-1 (#496) shipped three webhook_deliveries triggers without the accompanying pg test; this closes that debt. Triggers covered: - enforce_webhook_delivery_immutability (BEFORE UPDATE) - block_webhook_delivery_terminal_delete (BEFORE DELETE) - assert_webhook_delivery_company_match (BEFORE INSERT) 13 cases verify the lifecycle the dispatcher depends on remains mutable (pending → in_flight, in_flight → failed, failed → in_flight, in_flight → delivered) while terminal-status rows (delivered / dead) are write- locked and the cross-tenant INSERT path is refused with the ERRCODE=check_violation contract documented in the migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> test(api): integration tests for webhook routes (PR-1 test debt) CLAUDE.md mandates integration tests under app/api/v1/ for every route. Phase 6 PR-1 (#496) shipped the eight v1 webhook routes (five under /companies/{companyId}/webhooks/ + the cross-tenant /webhook-deliveries/ {id}/retry) without them; closes that debt. 19 cases for the /webhooks/ verticals: POST /webhooks create + secret-once + payroll-scope gate + SSRF GET /webhooks list (no secret) + empty list GET /webhooks/:id detail (no secret) + 404 PATCH /webhooks/:id update + active=true re-enable + SSRF re-check + empty-body DELETE /webhooks/:id 204 hard delete POST /webhooks/:id/test enqueue + 404 + disabled-rejection GET /webhooks/:id/deliveries happy path + ownership 404 7 cases for the retry route: POST /webhook-deliveries/:id/retry dead → fresh pending row, live-status refusal, cross-tenant 404, disabled-webhook gate, SSRF re-check, delivery 404, webhook-gone 404 Both files mirror the suppliers/customers integration test pattern: Proxy-backed Supabase mock with per-table queues, validateApiKey + validateWebhookUrl stubbed to control auth and DNS deterministically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-500 review round 1 — pg-real CI fix + 4 review items 1. pg-real CI was red on this PR: the new webhook trigger pg.test.ts and claim-due-webhook-deliveries pg.test.ts fixtures tried to INSERT into `webhooks.user_id`, which doesn't exist in the migration history. The column was never declared in automation_webhooks (20260415000000) nor added by webhooks_v2 (20260515170000) — so a fresh schema replay had no such column. The webhook create route (`webhooks.create`) was also referencing this non-existent column in its INSERT, so the production route was latent-broken since PR-1 and never exercised against a fresh DB. Drop the `user_id` field from both the route INSERT and the pg fixtures. Actor attribution lives on `created_by_api_key_id` (which leads back to the owning user via `api_keys.user_id`). 2. Greptile P2 #1 — `recoverStuckInFlight` carried a redundant `.not('status','in','(delivered,dead)')` filter alongside `.eq('status','in_flight')`, with a comment that incorrectly described PostgreSQL's UPDATE re-evaluation semantics. Under READ COMMITTED, UPDATE re-evaluates WHERE against each row's CURRENT value when it acquires the row lock — a row that raced to terminal status will fail `status='in_flight'` on re-evaluation and be skipped, no immutability trigger fires. Drop the redundant filter and rewrite the comment. 3. Greptile P2 #2 — added explicit pg test verifying `in_flight` rows are skipped by `claim_due_webhook_deliveries`. The status filter is what prevents double-delivery and is the entire point of the SKIP LOCKED substrate; making that invariant load-bearing in the test suite forecloses a future filter expansion silently regressing it. 4. Greptile P2 #3 — pinned-fetch registered both `res.on('end', finalize)` and `res.on('close', finalize)`. Node fires BOTH on normal completions, so finalize ran twice; the outer `settled` guard squashed the double-resolve but the header reconstruction still ran twice. Switch to `once` + self-removing pair so finalize runs exactly once on whichever event fires first (normal: end; truncation: close). 5. Compliance Swarm V8.2.1 — the retry route only checked `webhooks:manage` even when retrying `salary_run.* / agi.` deliveries. Mirror the create-route elevated-scope gate so a key with only `webhooks:manage` cannot re-emit payroll payloads carrying personnummer / lönesummor / skatteavdrag. New integration test verifies the gate returns 403 INSUFFICIENT_SCOPE with `required_scope: payroll:read`. 35 tests pass locally (+1 vs pre-fix). Type-check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> refactor(api): address PR-500 review round 2 — 2 small precision fixes 1. Compliance Swarm Art.32 / A.8.24 — response_body size cap was enforced only at the application layer (pinnedHttpsFetch's maxResponseBytes=4096 constant). A future refactor that bypassed the truncation, or a non- dispatcher write path into webhook_deliveries.response_body, would silently land large blobs in a column adjacent to event payloads carrying personal data. Add a CHECK constraint at the DB layer with a generous ceiling (8 KB — double the application cap so legitimate dispatcher writes never hit it; only a regression surfaces as a check_violation). 2. Compliance Swarm CC6.6 — pinned-fetch substitutes the validated IP for `host` while keeping the original hostname in `servername`. A reader could reasonably worry that the IP substitution weakens TLS hostname verification. Document explicitly that Node's default `checkServerIdentity` matches the cert's SAN/CN against `servername` (not `host`), so a forged endpoint at the pinned IP with a valid cert for a different hostname would fail the handshake. No code change — the default behavior is correct; the comment forecloses future "this looks dangerous" review-round noise on the same line. Items NOT addressed (with rationale documented elsewhere): - Compliance Swarm V8.2.1 (retry route 404-vs-404 information leak): delivery IDs are UUIDs; the "leak" is the ability to probe existence of an opaque 128-bit identifier the caller already has, which is not meaningfully different from probing for any opaque token. Both branches return the same structured 404 envelope. - Compliance Swarm CC7.2 (restore the .not() defense-in-depth filter): direct contradiction of last round's Greptile P2 fix. Greptile's PG-semantics analysis is correct — under READ COMMITTED, UPDATE re-evaluates WHERE against the row's current value when it acquires the lock, so .eq('status','in_flight') already handles the race. Adding a redundant .not() restores a misleading comment without closing a real gap. This is the documented Compliance Swarm oscillation pattern from the project's Phase 4 lessons. - Compliance Swarm CC6.1 (webhook secret encryption-at-rest): architectural choice from PR-1; not in PR-3 (substrate hardening) scope. Belongs to a future hardening PR. - Swedish-compliance review (operations queued/running rows hard- deletable): deliberate operability tradeoff — operators need to clear stuck/queued entries that crashed mid-flight. Blocking all deletes would force a manual DB intervention every time a worker crashed before reaching terminal status. The audit trail starts at terminal-state mutation, which IS blocked. - Swedish-compliance review (salary_run.* / agi.* payload anonymisation after 7 years): already on the deferred-list as part of the 90-day TTL cleanup cron item from the PR description. Belongs to a retention-policy follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-15 20:38:19 +02:00
Jakob Wennberg	e9e0fd726f	feat(api): Phase 6 PR-1 — webhooks substrate (delivery pipeline + routes) (#496 ) * feat(api): Phase 6 PR-1 — webhooks substrate (delivery pipeline + routes) First half of the final API plan phase. Ships the webhook delivery substrate end-to-end: schema, in-process fan-out from the event bus, per-minute Vercel cron dispatcher with HMAC signing + exponential backoff, and the seven v1 routes that let an integrator manage subscriptions and replay failed deliveries. Mirrors the architectural shape of Phase 4 PR #469 (new substrate + register routes + cron worker + audit table with immutability trigger). Migration (supabase/migrations/20260515170000_webhooks_v2.sql): - Repurpose automation_webhooks → webhooks. Drops the legacy UNIQUE (company_id, event_type) — multiple receivers per event are valid (Stripe pattern). Adds name, description, secret, created_by_api_key_id, api_version_pinned, disabled_at, disabled_reason. Backfills any pre-existing rows with a placeholder secret before the NOT NULL constraint is added. - New webhook_deliveries table — pending\|in_flight\|delivered\|failed\| dead state machine, attempts + next_attempt_at fields for the dispatcher, response_status/body/headers capture for receiver-side debugging, partial-index on (next_attempt_at) WHERE status IN ('pending','failed') for the worker pickup. - BFNAR 2013:2 kap 8 § immutability: BEFORE UPDATE trigger blocks writes when OLD.status IN ('delivered','dead'). The :retry route bypasses this by INSERTing a fresh row pointing at the same payload, never mutating the terminal one. - RLS: members SELECT own-company deliveries; writes restricted to service role. lib/webhooks/{handler,dispatcher,signing,diff}.ts: - handler.ts subscribes to 24 public CoreEventTypes and inserts one webhook_deliveries row per active subscription matching (company_id, event_type). Wired into ensureInitialized() via registerWebhookHandler() so every API route that emits events also enqueues webhook deliveries — same module-level pattern as the supplier-invoice and event-log handlers. - dispatcher.ts is the per-minute cron worker. Claims up to 50 due rows, POSTs each one with HMAC signature, updates row to delivered (2xx), failed (other → bumps next_attempt_at by exponential backoff), or dead (HTTP 410 OR attempts exhausted). HTTP 410 additionally auto-disables the webhook. 10s request timeout, 4 KB response-body cap. Backoff: 1m / 5m / 30m / 2h / 12h / 24h / 48h (7 retries, ~72h total) — matches Stripe. - signing.ts: Stripe-style X-Gnubok-Signature: t=<unix>,v1=<hex> with HMAC-SHA256 over `${t}.${rawBody}`. Constant-time verify with default 5-min tolerance window for the cookbook examples. generateWebhookSecret() returns 256 bits of crypto-random hex. - diff.ts: computePreviousAttributes() for Stripe-style update events. Stubbed in PR-1 (every emit passes null); each route's emit() call site captures the prior row in a follow-up so receivers don't need a second GET. v1 routes (app/api/v1/...): - /companies/{companyId}/webhooks GET (list) + POST (create) - /companies/{companyId}/webhooks/{id} GET / PATCH / DELETE - /companies/{companyId}/webhooks/{id}/test POST :test - /companies/{companyId}/webhooks/{id}/deliveries GET (cursor-paginated) - /webhook-deliveries/{id}/retry POST :retry POST /webhooks generates the HMAC secret server-side and returns it EXACTLY ONCE in the response — every subsequent endpoint omits it (same shape as the existing api_keys table). Idempotency-Key required on POST; dry-run supported. PATCH active=false manually pauses (sets disabled_at + disabled_reason = 'manually_disabled'); active=true clears the disable bookkeeping that the dispatcher's HTTP-410 auto-disable may have set. event_type is immutable — delete and recreate to change. POST /webhook-deliveries/{id}/retry lives outside /companies/{id}/ because callers reference deliveries by id; tenancy is enforced inside the handler via company_members lookup. Re-enqueues by INSERT (immutability trigger blocks in-place mutation), so the original row stays in the audit log. /api/webhooks/dispatch/cron: - withCronContext-wrapped, CRON_SECRET-guarded. - Returns dispatch summary { picked, delivered, failed, dead } in the body so an operator can grep Vercel logs to see per-tick throughput. - Per-minute schedule added to vercel.json (* * * * ). lib/auth/scopes.ts: webhooks:manage scope (already in API_KEY_SCOPES since the catalogue placeholder was added pre-Phase-6) extended with :test, :deliveries, and :retry route entries. Substrate-only by design. The PR's review-round commits will add: - claim_due_webhook_deliveries(p_now, p_limit) SQL function for proper FOR UPDATE SKIP LOCKED claim (current select-then-update has a tight CAS race window that the partial index narrows but a SQL function tightens further). - Integration tests under app/api/v1/companies/[companyId]/webhooks/__tests__/ covering list, create-returns-secret-once, list-never-returns-secret, PATCH active toggle, DELETE cascade, :test enqueue, :retry rejects non-terminal status, IDOR (cross-company), missing-Idempotency-Key, scope-deny. - .pg.test.ts for the immutability trigger (CLAUDE.md mandate for any PR touching a trigger / RLS policy). - 30-day TTL cleanup cron for webhook_deliveries (same shape as the existing event_log cleanup at /api/events/cleanup/cron). Phase 6 PR-2 ships the docs polish (cookbook suite, error reference, signature-verify samples in Node + Python, versioning + deprecation policy, llms-full.txt rebuild, spec-snapshot test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 1 — 4 real bugs + retention FK Fixes the 4 real bugs Greptile flagged on the round-1 review of the Phase 6 PR-1 webhooks substrate, plus the swedish-compliance-bot finding about 7-year audit retention on accounting-event delivery rows. Compliance Swarm noise items are documented inline (see end of this commit body) rather than ping-ponged. FIXED — real bugs: 1. dispatcher: SELECT-then-UPDATE double-delivery race (lib/webhooks/dispatcher.ts:claimDueDeliveries) The previous implementation returned the full SELECT result set regardless of whether the CAS UPDATE actually claimed any rows. Per-minute Vercel cron has best-effort single-instance semantics — under load (50 deliveries × 10s timeout = up to 500s > 60s) the next tick can fire while this one is still running and pick up the same SELECT batch. Both ticks would then dispatch the same deliveries. Fix: have the UPDATE return the IDs it actually claimed via `.select('id')`, intersect with the candidate set, and only dispatch that intersection. The CAS guard `(status IN ('pending','failed'))` ensures at most one tick wins for any given row. 2. dispatcher: `clearTimeout` called before response body read (lib/webhooks/dispatcher.ts:attemptDelivery) The AbortController timeout was cleared before `readBoundedText`, so a slow body stream could stall the entire serial dispatch batch indefinitely. Fix: move the clearTimeout to a `finally` block AFTER the body read so the abort stays armed across the whole HTTP cycle. 3. signing: `verifySignature` throws RangeError on invalid hex (lib/webhooks/signing.ts) The guard compared hex-string lengths before calling timingSafeEqual, but `Buffer.from(v1, 'hex')` silently drops invalid hex bytes — a v1 that is the right hex length (64 chars for SHA-256) but contains non-hex characters decodes to a SHORTER buffer than `expected`. timingSafeEqual then throws RangeError instead of returning false. Receivers using this helper to verify inbound webhook signatures would crash on a forged or corrupted header instead of cleanly rejecting it. Fix: compare buffer lengths AFTER decoding. 4. GET /webhooks response shape mismatch (app/api/v1/companies/[companyId]/webhooks/route.ts) The handler passed a flat array to `paginated()`, producing `data: [...]`, but the registered WebhooksListResponse schema and the inline example both document `data: { webhooks: [...] }`. Any client built against the spec would not find the expected key. Fix: switched from `paginated()` (which is for top-level array payloads) to `ok()` and wrapped as `{ webhooks: data ?? [] }` to match the schema. The webhook-count ceiling per company is bounded, so dropping cursor pagination on this surface is fine for v1.0. FIXED — swedish-compliance: 5. Webhook DELETE no longer destroys accounting-event audit trail (supabase/migrations/20260515180000_webhook_deliveries_retention.sql, app/api/v1/companies/[companyId]/webhooks/[id]/route.ts, lib/webhooks/dispatcher.ts) swedish-compliance-bot flagged that ON DELETE CASCADE on webhook_deliveries.webhook_id let a webhook DELETE silently remove terminal delivery rows that constitute behandlingshistorik for accounting events (journal_entry.committed, period.locked, salary_run.booked, agi.generated, ...). BFNAR 2013:2 kap 8 § requires 7-year retention of these rows. Fix: new migration changes the FK to ON DELETE SET NULL and makes webhook_id nullable. Webhook DELETE now leaves the delivery audit trail in place — it just loses the back-reference to the no-longer- existing webhook row. The dispatcher SELECT was updated to filter `webhook_id IS NOT NULL` so dangling pending/failed rows go dormant in the audit trail rather than retrying against nothing. Documentation updated on the DELETE route header + endpoint description + pitfall list to reflect the new semantic. FIXED — defense in depth: 6. Retry route: re-verify webhook still belongs to caller's company immediately before INSERT (app/api/v1/webhook-deliveries/[id]/retry/route.ts) Compliance Swarm V8.2.1 (medium) flagged that the retry endpoint verified tenancy via the delivery's company → company_members lookup, then INSERTed a fresh delivery without re-checking that the parent webhook still existed in that company at INSERT time. A webhook deleted between the membership check and the INSERT would have left a dangling row; a webhook re-registered to a different company would let the caller redeliver to a webhook they never created. Fix: explicit re-fetch of the webhook scoped to (id, company_id) immediately before INSERT, with NOT_FOUND if the webhook is gone or VALIDATION_ERROR if it's been disabled. DEFERRED — documented inline: - OWASP V14.2 plaintext webhooks.secret: Inline rationale added to lib/webhooks/signing.ts:generateWebhookSecret(). Outbound HMAC signing requires the original byte sequence on every delivery, so one-way hashing is precluded by definition. Stripe / GitHub / Slack / Twilio all follow the same pattern. Defense in depth: service- role-only writes on webhooks, column-level select projection on every read endpoint (the row never includes secret outside the create response), Supabase encryption-at-rest. Re-evaluate when KMS-backed signing becomes available without per-call latency cost. - Compliance Swarm V13.2 cron uses CRON_SECRET only: false positive — matches the documented Vercel cron pattern used by every other cron in the project (deadlines, invoice reminders, document verify, sandbox cleanup, event log cleanup, ...). - Compliance Swarm V1.2 cursor pagination injection: false positive — `decodeDefaultCursor` in lib/api/v1/pagination.ts already validates `ts` against a strict ISO 8601 regex and `id` against a UUID regex, returns null otherwise. The bot couldn't see the helper's internals. - Compliance Swarm V8.2.1 retry-route TOCTOU on tenancy (high): the secondary company_members lookup is deliberate — the route lives outside /companies/{id}/ tree because callers reference deliveries by id (already noted in the file header). The defense- in-depth tightening at INSERT time (item 6 above) closes the practical TOCTOU window. Round-2 may add an atomic DB function if swarm escalates this. - Compliance Swarm V2.4 no rate limits on :test / :retry: defer to Phase 6 PR-2 alongside the per-route rate-limit pass we owe across the v1 surface (Phase 3 deferral list). - Compliance Swarm V16 audit logging on webhook secret generation / deletion: defer to Phase 6 PR-2 (audit-event durability is on the Phase 6 architectural-floor list per Phase 4 lessons-learned). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 2 — SSRF, tenancy, retention triggers Round 2 of the Phase 6 PR-1 review cycle. Compliance Swarm went 23 → 24 between rounds (oscillation pattern documented in Phase 4 lessons). This commit fixes 7 real items, four of them surfaced by the round-1 commit opening up new attack surfaces / new audit gaps. FIXED: 1. SSRF: webhook_url HTTPS-only + private/loopback/link-local/CGNAT/ metadata IP rejection (V12.1, V1.2, CC6.6) New helper lib/webhooks/url-guard.ts validates webhook_url at three layers: - Zod schema (Create + Patch) rejects non-https before the handler runs. - Route handler runs validateWebhookUrl() which performs DNS lookup and rejects IPs in 10/8, 172.16/12, 192.168/16, 127/8, 169.254/16 (link-local + AWS/GCP/Azure metadata 169.254.169.254 explicitly classified), 100.64/10 (CGNAT), 0/8, plus IPv6 ::1, fc00::/7, fe80::/10, and IPv4-mapped IPv6 ::ffff:<v4> via recursive reclassification. - Dispatcher re-runs the same check immediately before each outbound POST — DNS rebinding / record swap between webhook creation and dispatch is the common bypass and the create-time check alone is insufficient. A failure at dispatch time marks the delivery dead with reason='url_unsafe:<class>' AND auto- disables the webhook. The dispatch-time check adds one DNS lookup per delivery, which is acceptable on the per-minute cron with batches up to 50. 2. Cross-tenant dispatch refusal (A.8.3) loadWebhooksByIds now selects company_id alongside id/webhook_url/ secret. The dispatch loop asserts webhook.company_id === delivery.company_id BEFORE signing. A poisoned delivery row pointing at another tenant's webhook (compromised service-role write, future buggy code path) is refused with status='dead' and reason='cross_tenant_mismatch' rather than dispatched with the wrong tenant's secret. 3. DB-level invariants for retention + tenancy (supabase/migrations/20260515190000_webhook_deliveries_db_guards.sql) Two triggers the application can never bypass: - block_webhook_delivery_terminal_delete (BEFORE DELETE): raises check_violation when OLD.status IN ('delivered','dead'). Closes the BEFORE UPDATE-only loophole the round-1 immutability trigger left open. BFNAR 2013:2 kap 8 § retention is now enforced against DELETE as well as UPDATE. - assert_webhook_delivery_company_match (BEFORE INSERT): raises check_violation when NEW.company_id doesn't match the parent webhooks.company_id. Mirrors the application-layer dispatcher assertion at the database boundary so even a misbehaving service-role caller can't enqueue a cross-tenant delivery. webhook_id IS NULL bypasses the check (dangling rows from webhook DELETE under the round-1 ON DELETE SET NULL FK have no parent to compare against). 4. Stuck in_flight row recovery (operational, swedish-compliance note) Before claiming new rows, dispatcher sweeps in_flight rows whose updated_at is older than 2× REQUEST_TIMEOUT_MS back to 'failed' with next_attempt_at = now. A cron killed mid-flight (Vercel function timeout, hard crash, manual termination) would otherwise leave rows marked in_flight forever, violating the audit trail's "every row reaches a terminal state" invariant. 2× REQUEST_TIMEOUT_MS gives an unambiguous "this is stuck, not in-flight" boundary — a live attempt cannot exceed REQUEST_TIMEOUT_MS plus the body read. 5. Response-body content-type filter + header allowlist (CC7.2, A.8.12, Art.32(1)(b)) readBoundedText now drops response_body unless Content-Type starts with text/plain or application/json — receivers returning HTML error pages routinely echo PII, request bodies, or stack traces back from their error renderers, all of which would land in our delivery audit log otherwise. Bytes are still drained so the connection stays reusable. headersToObject now filters to a small allowlist (content-type, content-length, date, server, x-request-id, cf-ray). Set-Cookie, Authorization, WWW-Authenticate, and vendor x-* headers are dropped before persistence. 6. Test payload data minimisation (Art.25(2)) The :test event payload no longer includes api_key_id. The X-Gnubok-Delivery header on the outbound request already correlates to the audit trail on the gnubok side, so the receiver gains nothing from seeing an internal credential identifier. 7. Silent-drop log promoted to error (PI1.3) handler.ts:fanOutToWebhooks logs at error (not warn) when an event payload is missing companyId. Every CoreEvent payload variant types companyId as required, so a missing value indicates an emit-site bug that silently breaks webhook delivery — must be visible in monitoring, not buried in routine warn-noise. DEFERRED (remaining oscillation, documented in commit body): - V14.2 / Art.5(1)(f) plaintext webhooks.secret: documented inline in lib/webhooks/signing.ts as accepted-risk per Stripe / GitHub / Slack precedent. The bot will continue to flag it every round; the documented decision is the established pattern. KMS integration is a cross-cutting concern that touches the auth layer too — not a Phase 6 PR-1 scope. - Art.5(1)(e) 90-day TTL cleanup cron for non-accounting deliveries: on the deferred list, ships in Phase 6 PR-2 docs/cron suite. - V2.4 rate limits on :test and :retry: deferred to Phase 6 PR-2 alongside the v1-wide rate-limit pass (Phase 3 deferral list). - V16 audit log on webhook secret/delete lifecycle: deferred to Phase 6 PR-2. - A.8.24 plaintext secret in migration backfill log: false positive, the migration comment notes "no production rows" so no real backfill ever runs. Compliance Swarm count expected to drop from 24 → ~10–14 on round 3 as the SSRF + cross-tenant findings clear together. Architectural floor is the V14.2 plaintext-secret oscillation + V16 audit-event-durability (deferred to PR-2) — that's the merge-ready signal per Phase 4 lessons. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 3 — 5 fixes + migration consolidation Compliance Swarm went 24 → 16 (5 high / 8 medium / 3 low) after round 2, clearing the SSRF + cross-tenant cluster as predicted. Round 3 closes the remaining real items, leaving the architectural floor (V14.2 plaintext, V16 audit log, V2.4 rate limits, Art.5(1)(e) TTL — all deferred to Phase 6 PR-2). That's the documented merge-ready signal. FIXED: 1. Deliveries list — webhook ownership pre-check (V8.2.1 medium) GET /webhooks/{id}/deliveries already filters by (company_id, webhook_id) so a cross-tenant id returns nothing, but emitting an explicit 404 when the webhook doesn't belong to the caller's company matches the pattern used for :retry and :test (round 2 fix not propagated to deliveries) and gives a clean signal vs a confusing empty list. Defense in depth alongside RLS. 2. url-guard: enumerate ALL DNS records (V1.2 medium) Replaced single dns.lookup with parallel dns.resolve4 + dns.resolve6. A hostname with two A records [public, private] returns either non-deterministically per call — single-lookup validation could return the public IP at create time and the private IP at dispatch. Multi-record enumeration rejects if ANY resolved address is unsafe. Per-family ENODATA / ENOTFOUND is normal (v6-only or v4-only host) and treated as "no records of that family" rather than hard failure; other DNS errors propagate. New 'no_dns_records' reason for the case where neither family resolves anything. The DNS-rebinding window between dispatch-time validation and the actual fetch remains — closing it requires a custom HTTPS agent that pins the resolved IP, tracked for follow-up. Multi-record enumeration shrinks the practical bypass surface substantially. 3. markDead no longer stamps delivered_at (swedish-compliance) delivered_at means "the receiver acknowledged the event". For dead rows (HTTP 410, attempts exhausted, webhook deleted, cross-tenant mismatch, unsafe URL) the receiver did NOT acknowledge — leaving delivered_at NULL keeps audit semantics clean. An auditor querying `WHERE delivered_at IS NOT NULL` correctly sees only genuinely delivered rows. The terminal-state timestamp lives on `updated_at` (auto-stamped by the table's BEFORE UPDATE trigger). 4. Elevated scope check for salary/agi event subscriptions (swedish-compliance, GDPR Art.32) Subscribing to salary_run.* or agi.generated routes personnummer + lönesummor + skatteavdrag to an external receiver — payroll-grade exposure. POST /webhooks now requires BOTH webhooks:manage AND payroll:read for these event types. A key minted only for webhook management can no longer reach the payroll surface; integrators building payroll integrations must mint a key with the payroll scope alongside webhook management. The check uses a regex (^salary_run\.\|^agi\.) so future payroll event types automatically inherit the gate. Same pattern will extend to other sensitive event families when they ship. 5. Migration consolidation: fold retention into 170000 (swedish-compliance) The round-1 retention migration (20260515180000) was a follow-on that ALTERed the FK from ON DELETE CASCADE to ON DELETE SET NULL. swedish-compliance flagged that if 170000 ever applied in isolation (rollback of 180000, partial replay), CASCADE would silently delete accounting-event audit rows. Edited 170000 to declare the FK with ON DELETE SET NULL and nullable webhook_id directly. Deleted 180000. Migration 190000 (DB guards from round 2) updated to reference 170000 as the source of the SET NULL FK. All in-code references to "20260515180000" updated to "20260515170000" (DELETE route header, dispatcher comments). Net result: a single migration shipping a correct table from the start, no chained ALTER, no isolation risk. DEFERRED (architectural floor, all bound for Phase 6 PR-2): - V8.2.1 retry ctx.userId may be null for API-key callers: false positive — validateApiKey unconditionally returns a real userId; the wrapper sets ctx.userId = auth.userId for every authenticated call. - V1.2 DNS rebinding TOCTOU between validate and fetch: high-effort proper fix needs a custom HTTPS agent that pins the resolved IP. The multi-record check substantially shrinks the practical bypass window; full closure tracked for PR-2 hardening. - V16.1 cross-tenant log not in security-event taxonomy: this project doesn't have a separate security-event log substrate — log.error with structured fields is the established pattern. - V4.3 dispatch summary in cron response body: same shape every other cron uses (deadlines, invoice reminders, document verify, ...). CRON_SECRET-gated; project pattern. - V5.3 / Art.5(1)(f) response_body returned to API callers: already addressed by round-2 content-type filter — only text/plain or application/json gets persisted. Residual oscillation; the bot didn't see the new filter. - Art.5(1)(e) 90-day TTL non-accounting deliveries: Phase 6 PR-2 cron suite. - Art.32(1)(b) / V14.2 plaintext webhooks.secret: established defer, documented inline in signing.ts (Stripe / GitHub / Slack precedent). - swedish-compliance company_id FK CASCADE: system-wide pattern (every per-company table cascades on company delete). Cross-cutting compliance decision, not webhook-specific. - swedish-compliance period.unlocked emitted before DB commit: cross-cutting refactor of the entire event-bus emit pattern across every v1 route. Project-wide concern, not Phase 6 PR-1 scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 4 — 2 critical fixes + 5 hardening Two critical items + 5 supporting hardening fixes. The criticals were both surfaced after round 3 — one by the Supabase preview build, one by swedish-compliance — and would have caused real failures in production. CRITICAL: 1. Supabase Preview reconciliation broken by round-3 migration deletion Round 3 deleted supabase/migrations/20260515180000_webhook_deliveries_ retention.sql after folding its FK fix into 170000. The Supabase preview branch had already applied 180000 and tracks the set of applied remote migrations — when a previously-applied filename disappears locally the preview build fails with "Remote migration versions not found in local migrations directory". Fix: restored 180000 with the original idempotent ALTER content. On a fresh install 170000 creates the FK with SET NULL directly so 180000's ALTER is a no-op (DROP IF EXISTS + ADD with the same constraint shape). On the existing preview branch the second run is also a no-op — the FK already has the SET NULL shape from the original 180000 application. Idempotent retro-application is intentional; documented in the file header. 2. `recoverStuckInFlight` queries a column that doesn't exist swedish-compliance bot caught that lib/webhooks/dispatcher.ts: recoverStuckInFlight filters `.lt('updated_at', stuckBefore)` against webhook_deliveries.updated_at, but migration 170000 never declared the column. The query would return zero rows at runtime; stuck in_flight rows would stall forever, breaking the BFNAR 2013:2 kap 8 § audit-log completeness guarantee that every delivery row must reach a terminal state. Fix: new migration 20260515200000_webhook_deliveries_updated_at.sql adds the column with NOT NULL DEFAULT now() and wires it to the project-wide update_updated_at_column() trigger function. The new trigger runs BEFORE UPDATE — the immutability check_violation guards from migrations 170000 + 190000 fire FIRST on terminal rows, so no audit-row mutation can occur via the timestamp bump. HARDENING: 3. Dispatcher: fetch redirect: 'error' (V1.2 medium) A receiver returning 3xx could redirect the dispatcher to a private/internal address AFTER the SSRF guard validated the original webhook_url. Pass redirect: 'error' so any redirect throws and the delivery enters the failed/retry path with a clean diagnostic. Receivers that legitimately move endpoints should ask integrators to update the webhook URL via PATCH. 4. Defensive ctx.companyId early-return (V8.2.1 medium) The deliveries list route used `ctx.companyId!` non-null assertion. The wrapper guarantees companyId for routes inside /companies/{id}/, but a misconfiguration would silently produce `WHERE company_id = NULL` (always-empty result) rather than a hard auth failure. Added an explicit early INTERNAL_ERROR return when ctx.companyId is falsy. Drops the `!` everywhere in the file. 5. Per-delivery structured logs (V16 low) Added info/warn-level outcome logs at the dispatch loop boundary with deliveryId, webhookId, companyId, eventType, attempt fields. Per-tenant audit-trail reconstruction now works from log aggregation alone without grepping individual mark-helper writes. Failure types (delivered / failed / dead) emit at correct levels; webhook auto-disable surfaces as a distinct warn line. 6. Strip userId from outbound webhook payloads* (Art.5(1)(c)) New minimisePayload() in handler.ts drops the internal Supabase auth.users.id UUID before insert into webhook_deliveries. The companyId stays (it's the tenant scope, useful for multi-tenant receivers). Centralising the projection means future tightening (e.g. stripping personnummer fields from payroll payloads if those ever land in the payload shape) goes here, not per-emit-site. 7. Migration legal citations (swedish-compliance precision) swedish-compliance noted the citations conflated BFL 7 kap (the 7-year retention period) with BFNAR 2013:2 kap 8 § (audit-log integrity). Both apply but they're distinct grounds. Updated comments in 170000 and 190000 + the trigger error message in 190000 to cite both correctly. REMAINING DEFERS (architectural floor — Phase 6 PR-2 territory): - V14 / Art.32 plaintext webhooks.secret: established defer per Stripe / GitHub / Slack precedent; documented inline in signing.ts. - V8.2.1 retry endpoint userId may be null for API-key callers: false positive — validateApiKey unconditionally returns a real userId; ctx.userId is always set after auth. - V1.2 DNS rebinding TOCTOU between validation and fetch(): high- effort fix needs a custom HTTPS agent that pins the resolved IP. Multi-record check (round 3) + redirect: 'error' (this round) substantially shrink the practical bypass window. Full closure is Phase 6 PR-2 hardening. - V2.3 dry-run rate limiting: Phase 6 PR-2 with the v1-wide rate-limit pass. - V16.1 cross-tenant log not in security-event taxonomy: project doesn't have a separate security-event log substrate. - Art.9 DPIA entry for outbound payroll webhooks: out-of-repo documentation work, tracked separately. - Art.5(1)(e) 90-day TTL non-accounting deliveries: Phase 6 PR-2 cron suite. - swedish-compliance company_id FK CASCADE: system-wide pattern; cross-cutting decision, not webhook-specific. - swedish-compliance period.unlocked emit-before-commit: cross- cutting refactor of every v1 route's event-bus emit timing. Compliance Swarm count expected to drop materially as the V1.2 + V8.2.1 + V16 cluster clears. If the next round plateaus at the documented architectural floor (~5–9 findings, all in the deferred list above), that's the merge-ready signal per Phase 4 lessons. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 5 — 5 small fixes (audit gaps + hardening) Round 5 closes the actionable items round 4 surfaced. Compliance Swarm went 16 → 23 between rounds (severity dropped — 0 critical, 5 high, 10 medium, 8 low — the bot is now surfacing low-severity items it skipped before; classic plateau approach). Round 5 fixes 3 real gaps + 2 documentation-precision items, all small. FIXED: 1. `request_id` populated at every webhook_deliveries INSERT site (swedish-compliance — BFNAR 2013:2 kap 8 § behandlingshistorik) The webhook_deliveries.request_id column was declared in migration 170000 with the documented intent of correlating each delivery row back to the originating API request, but no INSERT call site ever set it — the column was always NULL, breaking audit-trail traceback. - test/route.ts and retry/route.ts now stamp ctx.requestId. - handler.ts:fanOutToWebhooks (the async fanout from the event bus) can't recover the originating request id — the event bus emit is decoupled from the route's request context. Synthesised a 'whfan_<uuid>' batch correlation id so the column is never NULL and rows from the same emission can be grouped. Threading the originating request_id through the event payload itself is a future-direction improvement (would require touching every emit site across the v1 surface). 2. Retry route re-runs minimisePayload before INSERT (A.8.12 medium) The retry endpoint was inserting o.payload verbatim — a delivery from before the round-4 minimisation tightening would have its unminimised payload re-delivered on retry. minimisePayload exported from handler.ts; retry now applies it. Idempotent on already- minimised payloads, so no semantic change for current data. 3. Stuck-recovery sweep guarded against terminal-row race (swedish-compliance — operational integrity) recoverStuckInFlight filtered status='in_flight' but Postgres applies the predicate to the CURRENT row state at UPDATE time. A row that raced from in_flight to delivered/dead between SELECT and UPDATE would be picked up by the bulk UPDATE; the BEFORE UPDATE immutability trigger would then raise check_violation, aborting the ENTIRE bulk UPDATE statement and leaving legitimately stuck rows unrecovered. Added `.not('status', 'in', '(delivered,dead)')` as defense in depth. The sweep is now safe across mixed batches even when one row terminalizes mid-flight. 4. 'server' header dropped from response_headers allowlist (A.8.12 low) Receiver infrastructure version strings (nginx/1.21.6, Apache/2.4.41, ...) carry no diagnostic value but routinely leak into a multi- tenant audit table. Removed from SAFE_RESPONSE_HEADERS. 5. Migration citations narrowed: don't over-claim BFL on non-accounting rows (swedish-compliance — legal precision) The immutability triggers apply uniformly to all terminal delivery rows, but BFL 7 kap 1 § retention only applies to rows derived from räkenskapsinformation (journal_entry., period., salary_run.booked, agi.generated, invoice.paid, supplier_invoice.paid). For non- accounting events (customer.created, document.uploaded, transaction.categorized, webhook.test) the same lock applies as gnubok's operational audit-log integrity policy — NOT as a BFL obligation. Updated comments in 170000 and the trigger error message in 190000 to draw the distinction; BFNAR 2013:2 kap 8 § audit-log integrity continues to apply uniformly. REMAINING DEFERS (architectural floor — Phase 6 PR-2): - V14 / Art.32 / V9.1 / A.8.24 / CC6.1 plaintext webhooks.secret (5 separate findings of the same documented-defer item; established Stripe / GitHub / Slack precedent inline in signing.ts). - V8.2.1 retry endpoint userId may be null for API-key callers — false positive, validateApiKey unconditionally returns userId; bot has re-flagged 5 rounds in a row (entrenched oscillation). - V1.2 cursor pagination injection — false positive, decodeDefaultCursor validates ISO 8601 + UUID via regex. - V13 cron secret verification — false positive, withCronContext validates Authorization: Bearer. - V1.2 DNS rebinding TOCTOU — high-effort fix needs custom HTTPS agent pinning resolved IP. Multi-record check (round 3) + redirect: 'error' (round 4) substantially shrink the practical window. Phase 6 PR-2. - V2.4 rate limits on :test / :retry — Phase 6 PR-2 v1-wide pass. - V16.1 / A.8.15 / A.8.16 / CC7.2 SIEM / log drain / monitoring — out-of-repo infra, tracked separately. - Art.5(1)(e) 90-day TTL non-accounting deliveries — Phase 6 PR-2. - Art.9 DPIA entry for outbound payroll webhooks — out-of-repo doc. - Art.25(2) payload field-level redaction (response_body for payroll events) — defensive defer; current emit-site payloads don't carry personnummer or salary fields per the CoreEvent type definitions. - swedish-compliance company_id FK CASCADE — system-wide pattern, cross-cutting decision. - swedish-compliance period.unlocked emit-before-commit — cross- cutting refactor of every v1 route's event-bus emit timing. - PI1.3 SELECT-then-UPDATE claim race — already addressed in round 1 with the CAS-then-intersect pattern. Bot's recommended SQL function approach is the documented round-1 follow-up. Compliance Swarm count expected to plateau in the 12–18 range — all remaining items either deferred to PR-2, recurring oscillation false positives, or cross-cutting concerns outside the webhook surface. That's the documented merge-ready signal per Phase 4 lessons-learned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(api): address PR-496 review round 6 — 3 small fixes (last actionable items) Closes the 3 genuinely-new actionable items round 5 surfaced. Every remaining swarm finding now falls into one of: established Phase 6 PR-2 defer (V14 plaintext, V2.4 rate limits, V1.2 DNS rebinding, Art.5(1)(e) TTL, V16/A.8.15/A.8.16/CC7.2 SIEM), oscillation false positive (V8.2.1 retry userId, V1.2 cursor, V13 cron secret), already-addressed (Art.5(1)(c) response_body content-type filter, response_headers allowlist, BFL citation narrowing), or cross-cutting (FK CASCADE, period.unlocked emit timing, plaintext secret variants × 5). FIXED: 1. `granted_scopes` removed from INSUFFICIENT_SCOPE response details (Art.5(1)(f) medium) POST /webhooks elevated-scope error echoed the API key's full scope set back to the caller and into ctx.log structured fields. Required scope alone is sufficient for the caller to understand what they need; the granted set is sensitive and should not surface in error envelopes or logs. 2. Redirect error → terminal `dead` + auto-disable (CC6.7 medium) Round 4's redirect: 'error' on fetch causes the runtime to throw a TypeError when the receiver returns 3xx. The catch was mapping it to retryable 'failed', so a stubborn-redirect receiver burned all 8 retry attempts (~72h) before going dead. Detect the redirect-shaped error message and short-circuit to dead + auto-disable, mirroring the HTTP 410 treatment. Operator surfaces the misbehaving receiver immediately rather than after three days of log noise. Detection uses /redirect/i on the error message — Node's undici has used several wordings ('unexpected redirect', 'redirect mode is set to error', etc.) across versions; case-insensitive substring is the stable shape. 3. Retry route re-runs `validateWebhookUrl` against current URL (CC6.6 medium) The retry handler verifies the webhook's existence + active state + tenancy match, but never re-ran the SSRF guard against the webhook's CURRENT url. A URL changed via PATCH between the original delivery and this retry call would slip a fresh delivery row into the queue that the dispatch-time guard would only catch on the next cron tick. Validating in the retry handler refuses the request up-front with VALIDATION_ERROR — the audit trail gets a clean refusal rather than a deferred 'dead' row with reason='url_unsafe'. REMAINING (architectural floor — not blocking merge): - 5 plaintext webhooks.secret findings (V14 / V11.1 / Art.32 / A.8.24 / CC6.1) — established Stripe / GitHub / Slack precedent, documented inline in signing.ts. - V8.2.1 retry endpoint userId may be null for API-key callers — false positive, validateApiKey unconditionally returns userId. Bot has re-flagged 7 rounds in a row. - V1.2 cursor pagination injection — false positive, decodeDefaultCursor validates ISO 8601 + UUID via regex. - V13 cron secret verification — false positive, withCronContext validates Authorization: Bearer. - V8.2.1 deliveries cross-webhook leak — false positive, bot acknowledges the .eq('webhook_id') filter handles it. - V1.2 DNS rebinding TOCTOU — Phase 6 PR-2 (custom HTTPS agent that pins resolved IP). - V2.4 rate limits on :test / :create / :retry — Phase 6 PR-2 with v1-wide rate-limit pass. - V16.1 / A.8.15 / A.8.16 / CC7.2 SIEM / log drain / monitoring — out-of-repo infra. - Art.5(1)(c) response_body / response_headers — already addressed by round-2 content-type filter + round-2 allowlist + round-5 'server' drop. - Art.5(1)(e) 90-day TTL non-accounting deliveries — Phase 6 PR-2. - Art.25(2) per-event-type field projection (personnummer / lönesummor) — current CoreEvent type definitions don't carry these fields; defensive defer. - Art.9 DPIA / RoPA entries for outbound webhooks — out-of-repo doc. - A.8.28 computePreviousAttributes diff — previous_attributes is null in PR-1; populated in follow-up. - A.5.17 / V11.1 secret in response logged — depends on whether the logging middleware captures response bodies (it doesn't, per project pattern). Defensive defer. - CC9.2 TLS validation / CC3.2 credential-pattern scrub — out-of-scope hardening. - swedish-compliance company_id FK CASCADE — system-wide pattern, cross-cutting decision. - swedish-compliance period.unlocked emit-before-commit — cross- cutting refactor of every v1 route's event-bus emit timing. - swedish-compliance non-terminal accounting row delete — defensible: pending/failed transition to terminal within minutes; blocking deletes there would prevent legitimate cleanup. - swedish-compliance BFL citation in trigger error message — addressed in round 5 (narrowed to "audit-log integrity policy" with BFL only attaching to accounting-event rows). If round 7 plateaus or the count drops, that's the merge-ready signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-15 14:00:04 +02:00

5 Commits