feat(api): add GitHub webhook routes for push and pull_request - #213
feat(api): add GitHub webhook routes for push and pull_request#213bbornino wants to merge 3 commits into
Conversation
Add POST /webhooks/github/push and POST /webhooks/github/pull_request, each verifying the X-Hub-Signature-256 header (HMAC-SHA256 over the raw request body, constant-time compared) before enqueuing the raw payload as a BullMQ job. The push route additionally filters to refs/heads/main, logging and returning early for other branches without enqueuing. Request bodies are validated loosely (just that they're JSON objects, plus the one `ref` field the push route itself needs) since interpreting the payload contents is playfulprogramming#206's job, not this one's.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const deliveryId = Array.isArray(deliveryIdHeader) | ||
| ? deliveryIdHeader[0] | ||
| : (deliveryIdHeader ?? crypto.randomUUID()); |
There was a problem hiding this comment.
Instead of falling back to randomUUID, can we create a typebox schema for the expected headers (with both x-hub-signature-256 and x-github-delivery) and pass it into the schema and type parameter above?
Then, request.headers["x-github-delivery"] should just be typed as a string - we don't need to support the undefined/array cases - and fastify/typebox will handle the validation.
Replaces the manual Array.isArray/crypto.randomUUID fallback for x-github-delivery with a typebox headers schema on both routes, requiring x-hub-signature-256 and x-github-delivery as plain strings. Fastify/typebox now reject requests missing either header before the handler runs, so request.headers["x-github-delivery"] is used directly with no fallback needed. verify-signature.ts's own defensive header handling is left as-is: its preValidation hook runs before Fastify's schema validation, so it can't rely on the new schema having already guaranteed the header's presence.
2 participants
Summary
Adds two new webhook receiver routes:
POST /webhooks/github/push— receives GitHub'spusheventPOST /webhooks/github/pull_request— receives GitHub'spull_requesteventBoth verify the request's authenticity, then enqueue the raw payload as a BullMQ job for later processing. Interpreting the payload contents is explicitly out of scope for this PR — that's #206's job.
apps/worker/src/tasks/is untouched; the newWEBHOOK_PUSH/WEBHOOK_PULL_REQUESTjob types have no processor yet.Signature verification
Per GitHub's docs on validating webhook deliveries, each request's
X-Hub-Signature-256header is checked against an HMAC-SHA256 of the raw request body (not a re-serialized version of the parsed JSON — GitHub signs the exact bytes it sent, so a route-scopedaddContentTypeParsercaptures the raw string beforeJSON.parse). The comparison usescrypto.timingSafeEqual, following the same constant-time pattern already used inshouldBypassRateLimit(apps/api/src/plugins/rate-limit/index.ts). A missing or invalid signature returns401.This required a new required env var,
GITHUB_WEBHOOK_SECRET, added topackages/common/src/env.ts'sEnvSchemaand documented in.env.example. It gets generated when configuring the webhook in the repo's GitHub settings.Branch filtering
The push route additionally checks
ref === "refs/heads/main"per the issue's note — a push to any other branch is logged and returns200without enqueuing a job.Request body schemas
Kept intentionally loose (
additionalProperties: true, just validating it's a JSON object) rather than fully typing GitHub's large payload shapes — again, deferring to #206. The push route's schema does type the one field it actually reads (ref).Job types
WEBHOOK_PUSH/WEBHOOK_PULL_REQUESTadded toTasksinpackages/bullmq/src/tasks/types.ts, with minimalTaskInputs(unknown— raw payload, untyped on purpose) andTaskOutputs(object) entries. No processor implementation.Testing
apps/api/src/routes/webhooks/verify-signature.test.ts— unit tests for the signature-verification helper itself (valid/invalid/tampered/missing/array-header cases)apps/api/src/routes/webhooks/push.test.ts/pull-request.test.ts— route-level tests viaapp.inject(), mockingcreateJob, covering: valid signature → job enqueued, non-main branch → skipped, invalid/missing signature →401pnpm test:unit,pnpm run build:all, andpnpm run prettierall pass clean.Open question for @fennifith
This route's scope overlaps conceptually with the older #7 ("Webhook to import/sync content from GitHub with the db"), which has no cross-reference to #205. Does #205 supersede #7, or are they meant to coexist (e.g. #7 being the eventual sync-on-webhook logic that #206 will implement on top of these routes)? Want to make sure we're not duplicating effort or leaving #7 stale if this closes it out in spirit.