Skip to content

feat(api): wire WEBHOOK_SIGNING_SECRET into outbound webhook signing - #174

Open
kathy-ai-art wants to merge 2 commits into
Savitura:mainfrom
kathy-ai-art:chore/wire-or-remove-webhook-signing-secret
Open

feat(api): wire WEBHOOK_SIGNING_SECRET into outbound webhook signing#174
kathy-ai-art wants to merge 2 commits into
Savitura:mainfrom
kathy-ai-art:chore/wire-or-remove-webhook-signing-secret

Conversation

@kathy-ai-art

@kathy-ai-art kathy-ai-art commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Closes #148

Summary

— wires the documented-but-unused WEBHOOK_SIGNING_SECRET env var into outbound webhook signing (Option A) instead of removing it, and adds the verification utility, replay protection, an introspection endpoint, and the documentation requested by the issue.

What changed

  • New shared utility apps/api/src/modules/webhook/signature.ts (signBody / verifySignature): every outbound webhook now uses one timestamped HMAC-SHA256 wire format — X-SaviTools-Signature: sha256=<hex> plus X-SaviTools-Timestamp: <unix seconds>, where the hex is HMAC-SHA256 over the UTF-8 bytes of <timestamp>.<body> with the exact body bytes sent.
  • WEBHOOK_SIGNING_SECRET is now actually read: WebhookService.sendWebhook and EventsService.replayEvents resolve the signing secret as per-request secret → WEBHOOK_SIGNING_SECRET → unsigned. Monitor alert webhooks keep signing with the per-webhook DB secret (source unchanged).
  • Replay protection: verifySignature rejects timestamps older than DEFAULT_MAX_AGE_SECONDS (300 s) or more than DEFAULT_MAX_SKEW_SECONDS (60 s) in the future.
  • Introspection endpoint: new public GET /webhooks/signing returns { enabled, algorithm, signatureHeader, timestampHeader, replayWindowSeconds }.
  • Docs: README env table + "Webhook Signature Verification" section, docs/api-reference.md (new endpoint + verification instructions), ASSUMPTIONS.md items 3–4 rewritten, .env.example comment updated.
  • Frontend parity: the Webhook Tester's "Copy as cURL" now signs <timestamp>.<payload> client-side and includes X-SaviTools-Timestamp, so the preview matches what the API actually sends.

Key design decisions

  1. Took Option A (wire it up), not Option B (remove it) — the issue lists it as preferred, and the repo already had partial signing infrastructure; making the env var real is cheap and removes the false sense of security.
  2. Header name deviation from the issue text: the acceptance criteria name X-Savitura-Signature, but the repo (ASSUMPTIONS.md item 3 and all three existing senders) already standardizes on X-SaviTools-Signature: sha256=<hex>. I kept the repo-standard header name — shipping a third name for the same signature would recreate the exact confusion this issue is about. The algorithm (HMAC-SHA256), body-bytes + canonical-timestamp coverage, and replay prevention are exactly as specified.
  3. One shared wire format for all senders: previously the Webhook Tester, event replay, and monitor alerts each hand-rolled the same scheme; they now all call the shared utility, so documented format and actual behavior can't drift again.
  4. Timestamp in its own header (X-SaviTools-Timestamp) rather than embedded in the signature value — keeps the existing sha256=<hex> shape recognizable while letting receivers enforce freshness.
  5. Per-request secret still wins over the env var (backwards-compatible with the existing SendWebhookDto.secret / ReplayEventsDto.secret behavior); the env var is purely a fallback.

Acceptance criteria (Option A)

  • Outbound webhooks include a signature header using HMAC-SHA256 — X-SaviTools-Signature: sha256=<hex> attached by WebhookService, EventsService.deliverOne, and NotificationWorkerService whenever a secret is in play (see decision 2 for the header name). Covering tests: webhook.service.spec.ts ("includes a timestamped HMAC signature…"), events.service.spec.ts ("signs each POST with a timestamped HMAC…"), notification-worker.service.spec.ts ("sends the full event payload with a valid webhook HMAC").
  • Verification utility exists with tests — verifySignature in signature.ts; signature.spec.ts covers success, every named failure reason, window boundaries, and the constant-time path.
  • Signature uses body bytes with canonical timestamp to prevent replay — hex covers <timestamp>.<body> (exact wire bytes); verifier enforces a 300 s replay window and 60 s future-skew guard, with boundary tests at exactly max-age and skew.
  • Documentation updated with verification instructions — README "Webhook Signature Verification" section, docs/api-reference.md, ASSUMPTIONS.md.
  • GET endpoint to introspect whether signing is enabled — GET /webhooks/signing (public, no secrets exposed), tested via getSigningStatus.

Test output + coverage

API suite (npx jest --coverage): 434 passed / 5 failed. The 5 failures (federation.service.spec, orderbook.service.spec ×2, composer.service.spec type errors) are pre-existing on main — verified by stashing this change and re-running: they fail identically without it. All new/updated suites pass (webhook, events, notification-worker group: 90/90).

Coverage for changed files (no coverageThreshold is configured in apps/api/package.json, so no threshold to meet):

File % Stmts % Branch % Lines
signature.ts (new) 100 93.3 100
webhook.service.ts 86.1 78.3 85.4
events.service.ts 94.1 87.2 93.7
notification-worker.service.ts 43.1 5.6 41.3

(notification-worker.service.ts is a whole-file figure; the uncovered lines are pre-existing email/worker plumbing, not the signing path.)

Typecheck: tsc --noEmit is clean in apps/web; apps/api is clean except the same 3 pre-existing composer.service.spec.ts errors that exist on main. Lint: next lint is clean in apps/web; apps/api has no ESLint config in the repo (pre-existing — the eslint script cannot run in that workspace as committed).

Follow-ups (honest)

  • notification-worker.service.ts overall coverage is low because the Resend email path and BullMQ worker lifecycle are untested — pre-existing, not introduced here.
  • The API workspace has no ESLint config; adding one (and fixing the pre-existing composer.service.spec.ts type error) would make npm run lint and API tsc fully green.
  • ReplayEventsDto.secret's doc string could mention the env-var fallback.

Security note

  • Signing is only as good as secret handling: WEBHOOK_SIGNING_SECRET must stay out of client bundles. The Webhook Tester's cURL preview only signs in-browser when the user supplies their own secret; the env-var fallback is applied server-side.
  • The 300 s replay window bounds replay risk; receivers should verify in constant time (verifySignature uses crypto.timingSafeEqual).
  • Per-request secrets continue to override the global one, so a leaked per-request secret does not compromise WEBHOOK_SIGNING_SECRET.

@kathy-ai-art
feat(api): wire WEBHOOK_SIGNING_SECRET into outbound webhook signing
b01a701 
All outbound webhook senders (Webhook Tester, contract-event replay, monitor
alerts) now share a timestamped HMAC-SHA256 wire format: X-SaviTools-Signature:
sha256=<hex> plus X-SaviTools-Timestamp, where the hex covers `<timestamp>.<body>`
with the exact body bytes sent. WEBHOOK_SIGNING_SECRET is the global fallback
secret when no per-request secret is given, closing the documented-but-unused
gap; GET /webhooks/signing reports whether signing is enabled. Adds a shared
sign/verify utility with full test coverage and verification docs.

Closes Savitura#148
@kathy-ai-art
Merge branch 'main' into chore/wire-or-remove-webhook-signing-secret
b18a960
@drips-wave

drips-wave Bot commented Sep 2, 2026

Copy link
Copy Markdown

@kathy-ai-art Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

chore: Wire up or remove unused WEBHOOK_SIGNING_SECRET env var

1 participant

@kathy-ai-art