Jakob Wennberg
e9e0fd726f
* 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>
34 lines
1.5 KiB
SQL
34 lines
1.5 KiB
SQL
-- Migration: webhook_deliveries_retention
|
|
--
|
|
-- Originally added in PR #496 round 1 to ALTER the webhook_deliveries.webhook_id
|
|
-- FK from ON DELETE CASCADE to ON DELETE SET NULL (BFNAR 2013:2 kap 8 §
|
|
-- behandlingshistorik + BFL 7 kap retention).
|
|
--
|
|
-- In round 3 the FK declaration was folded directly into migration
|
|
-- 20260515170000 (clean schema state for fresh installs). This file is
|
|
-- retained for migration-history continuity — Supabase preview branches
|
|
-- track the set of applied remote migrations and fail reconciliation if
|
|
-- a previously-applied filename disappears locally.
|
|
--
|
|
-- The body below is fully idempotent:
|
|
-- - On a fresh install: 170000 creates the FK with SET NULL; this
|
|
-- migration's ALTER is a no-op (DROP IF EXISTS + ADD with the same
|
|
-- constraint shape).
|
|
-- - On a preview branch that applied the original 170000 (CASCADE) +
|
|
-- this 180000 (the original ALTER): the column is already nullable
|
|
-- and the FK is already SET NULL; ALTER is a no-op.
|
|
-- - Idempotent retro-application is intentional so neither path
|
|
-- diverges from the canonical post-migration schema state.
|
|
|
|
ALTER TABLE public.webhook_deliveries
|
|
ALTER COLUMN webhook_id DROP NOT NULL;
|
|
|
|
ALTER TABLE public.webhook_deliveries
|
|
DROP CONSTRAINT IF EXISTS webhook_deliveries_webhook_id_fkey;
|
|
|
|
ALTER TABLE public.webhook_deliveries
|
|
ADD CONSTRAINT webhook_deliveries_webhook_id_fkey
|
|
FOREIGN KEY (webhook_id) REFERENCES public.webhooks(id) ON DELETE SET NULL;
|
|
|
|
NOTIFY pgrst, 'reload schema';
|