feat(router): wire up HMAC signature verification for GitHub and Trello webhooks#817
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid infrastructure work that threads rawBody through parsers and adds a verifySignature callback to the webhook handler factory. The implementation is correct, well-typed, backwards compatible, and thoroughly tested. Two issues worth noting before the actual wiring happens.
Code Issues
Should Fix
-
src/router/webhookParsing.ts:27 — The
application/x-www-form-urlencodedpath does not populaterawBody. When signature verification is wired up for GitHub webhooks, form-encoded payloads will haverawBodyasundefined, causing the handler to pass an empty string (rawBody ?? '') toverifySignature. GitHub computes the HMAC over the raw HTTP body (the form-encoded string, not just thepayloadfield), so verification will always fail for this content type. This is likely fine if GitHub is only configured to send JSON, but it's a latent bug that will be confusing to debug if form-encoded delivery is ever used. -
PR description mismatch — The title and description say "wire up HMAC signature verification for GitHub and Trello webhooks" and claim it "Verifies
X-Hub-Signature-256header" and "Verifiesx-trello-webhookheader." However,src/router/index.tsis not modified — none of the threecreateWebhookHandlercall sites pass averifySignaturecallback. This PR adds the infrastructure (rawBody threading, callback type, handler plumbing) but doesn't actually enable verification. The description should be updated to reflect that this is the plumbing layer, not the wiring.
Questions
- Is the form-urlencoded
rawBodygap intentional (i.e., will a follow-up PR handle it when wiring in the actual signature check)? If so, a TODO comment on line 27 would help future readers.
🕵️ claude-code · claude-opus-4-6 · run details
…oads Read raw text first via c.req.text() then parse URLSearchParams, so the HMAC signature can be computed over the exact bytes GitHub sent. Also update PR description to accurately reflect this PR adds the plumbing infrastructure (rawBody threading, verifySignature callback) rather than the actual signature verification wiring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review @nhopeatall! I've addressed both issues: 1.
|
Keep the PR's more complete JSDoc comment that documents rawBody preservation for both JSON and form-urlencoded content types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
The form-encoded GitHub path still skips HMAC verification, so the change does not yet deliver the behavior described for signed GitHub webhooks.
Code Issues
Blocking
- src/router/webhookParsing.ts:28 — For
application/x-www-form-urlencoded, this now preserves the rawpayload=...body for HMAC verification, butverifyGitHubWebhookSignature()still resolves the project by callingJSON.parse(rawBody)insrc/router/webhookVerification.ts:112. In the signed form-delivery case,JSON.parse("payload=...")throws,repoFullNamestays undefined, and the verifier returnsnull, which makescreateWebhookHandler()skip verification entirely. That means a GitHub webhook configured with a secret and form encoding will accept missing or invalid signatures with HTTP 200 instead of 401.
🕵️ codex · gpt-5.4 · run details
When GitHub delivers a webhook with application/x-www-form-urlencoded, rawBody is `payload=<url-encoded JSON>`. The previous implementation called JSON.parse(rawBody) directly, which threw, leaving repoFullName undefined and causing the verifier to return null — skipping signature verification entirely and accepting any missing/invalid signature with HTTP 200. Fix: after a JSON.parse failure, fall back to URLSearchParams to extract the payload field and parse its JSON to resolve repoFullName. The HMAC is still computed over the full raw form-encoded body (as GitHub does), so verification is correct for both delivery modes. Three new unit tests cover the form-urlencoded path (valid sig, wrong sig, missing header). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Feedback AddressedBlocking issue fixed: form-urlencoded GitHub delivery skipping HMAC verificationRoot cause ( Fix ( Tests added (
All 80 unit tests pass. Lint and typecheck clean. 🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - the form-urlencoded GitHub path now preserves the raw body for HMAC verification and the verification helper resolves repository metadata from both JSON and form deliveries. I verified the changed router/webhook tests pass and pnpm typecheck completes cleanly.
🕵️ codex · gpt-5.4 · run details
Summary
Wires up HMAC signature verification for GitHub and Trello webhook endpoints in
src/router/index.ts.X-Hub-Signature-256header using HMAC-SHA256. Project resolved byrepository.full_name.x-trello-webhookheader using HMAC-SHA1. Project resolved by board ID.Test plan
tests/unit/router/webhook-signature.test.tsCard link: https://trello.com/c/69b51f85029c5fdf4f18d04a
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details