Fixed webhook endpoint which was not verifying x-hub-signature-256 #701
Conversation
Implemented the GitHub webhook signature verification
WalkthroughAdds HMAC signature verification to the GitHub webhook endpoint using SHA-256 hashing and timing-safe comparison. The webhook handler now reads request body as text, extracts the signature header, validates it against the payload, and rejects requests with invalid or missing signatures before processing events. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/web/src/app/api/(server)/webhook/route.ts (1)
107-118: Consider wrapping webhook processing in try-catch for comprehensive error handling.Based on the Stripe webhook handler pattern (in
packages/web/src/app/api/(server)/stripe/route.ts), consider wrapping the entire webhook processing logic in a try-catch block. This ensures any unexpected errors are caught and logged appropriately, preventing unhandled exceptions from crashing the handler.🔎 Example pattern from Stripe handler
export const POST = async (request: NextRequest) => { try { const rawBody = await request.text(); // ... signature verification and processing ... return Response.json({ status: 'ok' }); } catch (error) { logger.error('Error processing webhook', error); return new Response('Internal Server Error', { status: 500 }); } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web/src/app/api/(server)/webhook/route.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/web/src/app/api/(server)/webhook/route.ts (1)
packages/web/src/app/api/(server)/stripe/route.ts (1)
POST(12-105)
🔇 Additional comments (1)
packages/web/src/app/api/(server)/webhook/route.ts (1)
5-5: LGTM! Necessary imports for webhook signature verification.The
cryptoimport provides HMAC functionality for signature verification, andWebhookEventDefinitionsupports the existing type guards.Also applies to: 13-13
| const verifyGitHubSignature = (payload: string, signature: string, secret: string): boolean => { | ||
| const hmac = crypto.createHmac('sha256', secret); | ||
| const digest = 'sha256=' + hmac.update(payload).digest('hex'); | ||
| return crypto.timingSafeEqual(Buffer.from(digest), Buffer.from(signature)); | ||
| }; |
There was a problem hiding this comment.
Critical: Add error handling for length mismatch in timingSafeEqual.
crypto.timingSafeEqual throws an error if the two buffers have different lengths. If an attacker or a malformed webhook sends a signature with unexpected length, this function will throw an unhandled exception, causing a 500 error instead of properly rejecting the request.
🔎 Proposed fix with try-catch and length validation
const verifyGitHubSignature = (payload: string, signature: string, secret: string): boolean => {
+ try {
const hmac = crypto.createHmac('sha256', secret);
const digest = 'sha256=' + hmac.update(payload).digest('hex');
+ if (digest.length !== signature.length) {
+ return false;
+ }
return crypto.timingSafeEqual(Buffer.from(digest), Buffer.from(signature));
+ } catch (error) {
+ logger.warn('Error verifying GitHub signature', error);
+ return false;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/app/api/(server)/webhook/route.ts around lines 100 to 104,
the verifyGitHubSignature function currently calls crypto.timingSafeEqual
without guarding against differing buffer lengths which causes it to throw;
update the function to first validate the signature format (ensure it starts
with "sha256="), compute the expected digest, convert both expected and provided
signatures to Buffers, immediately return false if their lengths differ, and
wrap the timingSafeEqual call in a try/catch that returns false on any error so
malformed or malicious signatures never cause an exception to bubble up.
| return new Response('Unauthorized', { status: 401 }); | ||
| } | ||
|
|
||
| const body = JSON.parse(rawBody); |
There was a problem hiding this comment.
Critical: Add error handling for JSON parsing.
JSON.parse can throw an error if the request body is not valid JSON. An attacker could send a malformed JSON payload with a valid signature (or if signature verification fails elsewhere), causing an unhandled exception.
🔎 Proposed fix with try-catch
+ let body;
+ try {
- const body = JSON.parse(rawBody);
+ body = JSON.parse(rawBody);
+ } catch (error) {
+ logger.warn('Failed to parse webhook body as JSON', error);
+ return new Response('Bad Request', { status: 400 });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = JSON.parse(rawBody); | |
| let body; | |
| try { | |
| body = JSON.parse(rawBody); | |
| } catch (error) { | |
| logger.warn('Failed to parse webhook body as JSON', error); | |
| return new Response('Bad Request', { status: 400 }); | |
| } |
🤖 Prompt for AI Agents
In packages/web/src/app/api/(server)/webhook/route.ts around line 118,
JSON.parse(rawBody) can throw on malformed input; wrap the parse in a try-catch,
catch the error, log the parsing error (with context), and return a proper HTTP
response (400 Bad Request) or similar error response instead of letting the
exception bubble; ensure subsequent logic (signature verification/processing)
only runs when parsing succeeds.
| const verifyGitHubSignature = (payload: string, signature: string, secret: string): boolean => { | ||
| const hmac = crypto.createHmac('sha256', secret); | ||
| const digest = 'sha256=' + hmac.update(payload).digest('hex'); | ||
| return crypto.timingSafeEqual(Buffer.from(digest), Buffer.from(signature)); |
There was a problem hiding this comment.
should probably catch in here and return false if it throws
3 participants
What was happening earlier
The GitHub webhook handler processes incoming webhook payloads without explicitly
verifying the x-hub-signature-256 header against a shared secret.
If the webhook endpoint is publicly accessible, a forged request could trigger
webhook-driven workflows (e.g. review agents) without originating from GitHub.
Resolved
Implemented the GitHub webhook signature verification.
The handler now extracts the raw request body and verifies it against the x-hub-signature-256 header using the GITHUB_REVIEW_AGENT_APP_WEBHOOK_SECRET. This prevents forged payloads from triggering the review agent.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.