security(webhook): add HMAC signature verification for Sendblue webhooks#25
security(webhook): add HMAC signature verification for Sendblue webhooks#25VirusSoup wants to merge 3 commits into
Conversation
Verifies HMAC-SHA256 signatures on POST /sendblue/webhook before any payload processing. Accepts signature in x-sendblue-signature, signature, or x-webhook-signature headers (hex or base64). Falls back to a raw shared-secret check on x-webhook-secret header / ?secret= query param for upstreams that cannot sign. All comparisons use crypto.timingSafeEqual to avoid timing oracles. When SENDBLUE_SIGNING_SECRET is unset, verification is disabled and a WARNING is logged at startup so existing local-dev users keep working. When set, missing/invalid signatures get a generic 401 and the failure is logged with the source IP only — never the payload body, never the expected signature. express.json now stashes the raw request body buffer on req.rawBody via its verify hook so HMAC can be computed against the unparsed bytes.
Greptile SummaryThis PR adds HMAC-SHA256 and shared-secret verification for inbound Sendblue webhooks, including timing-safe comparisons, a startup warning when the signing secret is absent, and raw-body capture via an
Confidence Score: 2/5Not safe to merge — the webhook route file has a compilation-blocking syntax error and the express.json ordering change silently breaks Composio HMAC verification for all existing webhook traffic. Two concrete defects on the changed paths: the server/sendblue.ts (route handler replacement is structurally incomplete) and server/index.ts (express.json ordering breaks Composio HMAC) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /sendblue/webhook] --> B[express.json stores raw bytes on req.rawBody]
B --> C{SENDBLUE_SIGNING_SECRET set?}
C -- No --> D[Pass through with warning]
C -- Yes --> E{HMAC header present?}
E -- Yes --> F{verifyHmac passes?}
F -- Yes --> G[next]
F -- No --> H[401]
E -- No --> I{Shared-header present?}
I -- Yes --> J{verifySharedSecret passes?}
J -- Yes --> G
J -- No --> H
I -- No --> H
G --> K[Webhook handler processes payload]
Reviews (3): Last reviewed commit: "Merge branch 'main' into security/001-we..." | Re-trigger Greptile |
P1: drop stale ?secret= reference in .env.example. The query parameter fallback was already removed from the code path; the comment lingered. P2: clientIp no longer trusts X-Forwarded-For. The previous implementation read the header unconditionally, letting any attacker spoof the source IP in security logs. Now uses req.ip — Express resolves that via its trust proxy setting, so without explicit configuration X-Forwarded-For is ignored and the socket address is used. P2: move the rawBody guard inside the SIGNATURE_HEADERS loop. The check previously ran before the shared-secret path, so a request with a valid sb-signing-secret header but a content-type that express.json skips (e.g. text/plain) got a 400 instead of being verified via the shared secret. The HMAC path is the only one that needs raw bytes. P2: drop dead try/catch in decodeSignature. Buffer.from(str, "base64") silently ignores invalid characters and never throws. Replaced with an explicit length check requiring exactly 32 bytes (SHA-256 digest length) on both hex and base64 branches.
- deploy/: Caddyfile (TLS reverse proxy), systemd unit (hardened, runs as boop user), firewall rules (22/80/443), bootstrap.sh (Node 22 + Caddy + repo + Claude Code install) - .claude/commands/: provision.md (one-time server provisioning recipe) and deploy.md (pull-and-restart for ongoing changes) - .claude/settings.json: PreToolUse hooks requiring BOOP-OK for ssh/scp/ rsync to the boop server, HETZNER-OK for hcloud destructive ops, and blocking git add -A/./* (public-repo PII protection) Targets a Hetzner CX23 in nbg1 at boop.planforadventure.com (178.105.87.21). Pairs with the cherry-picked PR raroque#25 HMAC verification.
| router.post("/webhook", sendblueWebhookAuth, async (req, res) => { | ||
| const { content, from_number, is_outbound, message_handle } = req.body ?? {}; | ||
| if (is_outbound || !content || !from_number) { |
There was a problem hiding this comment.
Incomplete route handler replacement — syntax error, file cannot compile
The three added lines open a new router.post handler and an if block that are never closed. The original handler body (lines 344–418) is syntactically inside that if block; after its }); on line 418 the if block and outer async handler remain open, and the single } on line 421 that closes createSendblueRouter is the only remaining closing brace — leaving two unmatched { tokens. TypeScript will reject this file entirely.
The fix is to replace the old handler wholesale: add sendblueWebhookAuth as the second argument of the existing router.post, rather than prepending a new, incomplete one.
| app.use( | ||
| express.json({ | ||
| limit: "2mb", | ||
| verify: (req, _res, buf) => { | ||
| // Stash raw body bytes for HMAC verification on signed webhook routes. | ||
| (req as express.Request & { rawBody?: Buffer }).rawBody = Buffer.from(buf); | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
New global
express.json placed before Composio's express.raw, breaking Composio HMAC
The new express.json middleware (lines 58-66) is mounted before the Composio-specific express.raw at line 77. Express body-parsers set req._body = true on first parse and skip on all subsequent parsers, so the stream is fully consumed at line 58. When the Composio route handler runs, Buffer.isBuffer(req.body) at composio-routes.ts:190 returns false (the body is now a JS object, not a Buffer), rawBody becomes "", and every Composio webhook request fails HMAC verification with a rejected payload.
The fix is to add the verify hook to the existing express.json call at line 78 (after the Composio raw handler) rather than inserting a second, globally-ordered one before it.
Verifies inbound Sendblue webhook requests before any payload processing.
HMAC-SHA256 path (forward-compatible): accepts a signature in
x-sendblue-signature,signature, orx-webhook-signatureheaders (hex or base64, with or withoutsha256=prefix).Shared-secret path (current Sendblue behavior): Sendblue echoes the configured signing secret in the
sb-signing-secretheader. Also acceptsx-webhook-secretas a generic fallback for proxied upstreams that rename the header. Header-only — no query parameters, since those leak into access logs, proxy logs, and ngrok inspection.All comparisons use
crypto.timingSafeEqual. The shared-secret comparison HMAC-hashes both sides before comparing to avoid leaking the secret's length through an early length-mismatch return.When
SENDBLUE_SIGNING_SECRETis unset, verification is disabled and a warning is logged at startup so existing local-dev setups keep working. When set, missing or invalid signatures return a generic 401 and the failure is logged with the source IP only — never the payload body, never the expected secret.express.jsonnow stashes the raw request body buffer onreq.rawBodyvia itsverifyhook so HMAC can be computed against the unparsed bytes.Closes #24