* 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>
