fix: add authentication to incident webhook endpoint#1600
Open
Akshita-2307 wants to merge 1 commit into
Open
Conversation
|
Someone is attempting to deploy a commit to the Nisshchaya's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThe incident webhook POST handler is hardened with two security checks: conditional HMAC authentication using an environment-configured secret header, and explicit parsing and validation of the required ChangesWebhook security hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🎉 Thanks for your contribution, @Akshita-2307!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
1 similar comment
🎉 Thanks for your contribution, @Akshita-2307!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/integrations/incidents/webhook/route.ts`:
- Around line 33-38: The code currently uses unsafe defaults for installationId,
owner, and repo; remove the fallback defaults and instead validate presence and
correctness: for installationId call url.searchParams.get("installationId") and
if null or parseInt(...,10) yields NaN return NextResponse.json({ error:
"Missing or invalid installationId" }, { status: 400 }); similarly require
url.searchParams.get("owner") and url.searchParams.get("repo") to be
non-null/non-empty and if missing return 400 (e.g. NextResponse.json({ error:
"Missing owner" }, { status: 400 }) and "Missing repo" respectively); update the
code paths referencing installationId, owner, and repo to use these validated
values (no default "1", "owner", or "repo").
- Around line 10-16: The current secret header check must be replaced with
HMAC-SHA256 verification: require INCIDENT_WEBHOOK_SECRET (fail closed if
unset), read the raw request body (use the Request object raw bytes), compute
HMAC-SHA256 with the webhookSecret and compare to the x-incident-signature-256
header using crypto.timingSafeEqual for constant-time comparison, reject if
missing/invalid; also add replay protection by validating an
x-incident-timestamp header within an allowed skew window before accepting.
Update the code paths referencing webhookSecret and the header check in route.ts
to perform these steps and return 401 on any failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16fe0fb5-1bba-4396-a406-95d5f1d4026d
📒 Files selected for processing (1)
app/api/integrations/incidents/webhook/route.ts
Comment on lines
+10
to
+16
| const webhookSecret = process.env.INCIDENT_WEBHOOK_SECRET; | ||
| if (webhookSecret) { | ||
| const headerSecret = req.headers.get("x-webhook-secret"); | ||
| if (!headerSecret || headerSecret !== webhookSecret) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Security implementation does not match issue #1539 requirements.
Several critical gaps exist between this implementation and the security requirements:
-
No HMAC signature verification: Issue
#1539requires verifying an HMAC-SHA256 signature (x-incident-signature-256) over the raw request body. The current implementation compares a plain secret transmitted in headers, which:- Exposes the secret in transit (even over HTTPS, logs/proxies may capture headers)
- Provides no request body integrity verification
- Offers no replay protection
-
Conditional authentication bypass: When
INCIDENT_WEBHOOK_SECRETis unset, the endpoint remains fully unauthenticated. This should fail closed. -
Timing attack vulnerability: Using
!==for secret comparison leaks timing information. Usecrypto.timingSafeEqual().
🔒 Proposed fix implementing HMAC verification
+import crypto from "crypto";
import { NextRequest, NextResponse } from "next/server";
// ... other imports
export async function POST(req: NextRequest) {
try {
const webhookSecret = process.env.INCIDENT_WEBHOOK_SECRET;
- if (webhookSecret) {
- const headerSecret = req.headers.get("x-webhook-secret");
- if (!headerSecret || headerSecret !== webhookSecret) {
- return NextResponse.json({ error: "Unauthorized" }, { status: 401 });
- }
+ if (!webhookSecret) {
+ console.error("[WebhookRoute] INCIDENT_WEBHOOK_SECRET not configured");
+ return NextResponse.json({ error: "Webhook not configured" }, { status: 500 });
}
+
+ const signature = req.headers.get("x-incident-signature-256");
+ if (!signature) {
+ return NextResponse.json({ error: "Missing signature" }, { status: 401 });
+ }
+
+ const rawBody = await req.text();
+ const expectedSignature = "sha256=" + crypto
+ .createHmac("sha256", webhookSecret)
+ .update(rawBody)
+ .digest("hex");
+
+ const sigBuffer = Buffer.from(signature);
+ const expectedBuffer = Buffer.from(expectedSignature);
+ if (sigBuffer.length !== expectedBuffer.length ||
+ !crypto.timingSafeEqual(sigBuffer, expectedBuffer)) {
+ return NextResponse.json({ error: "Invalid signature" }, { status: 401 });
+ }
+
+ const payload = JSON.parse(rawBody);
-
- const payload = await req.json();📝 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.
Suggested change
| const webhookSecret = process.env.INCIDENT_WEBHOOK_SECRET; | |
| if (webhookSecret) { | |
| const headerSecret = req.headers.get("x-webhook-secret"); | |
| if (!headerSecret || headerSecret !== webhookSecret) { | |
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | |
| } | |
| } | |
| const webhookSecret = process.env.INCIDENT_WEBHOOK_SECRET; | |
| if (!webhookSecret) { | |
| console.error("[WebhookRoute] INCIDENT_WEBHOOK_SECRET not configured"); | |
| return NextResponse.json({ error: "Webhook not configured" }, { status: 500 }); | |
| } | |
| const signature = req.headers.get("x-incident-signature-256"); | |
| if (!signature) { | |
| return NextResponse.json({ error: "Missing signature" }, { status: 401 }); | |
| } | |
| const rawBody = await req.text(); | |
| const expectedSignature = "sha256=" + crypto | |
| .createHmac("sha256", webhookSecret) | |
| .update(rawBody) | |
| .digest("hex"); | |
| const sigBuffer = Buffer.from(signature); | |
| const expectedBuffer = Buffer.from(expectedSignature); | |
| if (sigBuffer.length !== expectedBuffer.length || | |
| !crypto.timingSafeEqual(sigBuffer, expectedBuffer)) { | |
| return NextResponse.json({ error: "Invalid signature" }, { status: 401 }); | |
| } | |
| const payload = JSON.parse(rawBody); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/integrations/incidents/webhook/route.ts` around lines 10 - 16, The
current secret header check must be replaced with HMAC-SHA256 verification:
require INCIDENT_WEBHOOK_SECRET (fail closed if unset), read the raw request
body (use the Request object raw bytes), compute HMAC-SHA256 with the
webhookSecret and compare to the x-incident-signature-256 header using
crypto.timingSafeEqual for constant-time comparison, reject if missing/invalid;
also add replay protection by validating an x-incident-timestamp header within
an allowed skew window before accepting. Update the code paths referencing
webhookSecret and the header check in route.ts to perform these steps and return
401 on any failure.
Comment on lines
33
to
38
| const installationId = parseInt(url.searchParams.get("installationId") || "1", 10); | ||
| if (isNaN(installationId)) { | ||
| return NextResponse.json({ error: "Invalid installationId" }, { status: 400 }); | ||
| } | ||
| const owner = url.searchParams.get("owner") || "owner"; | ||
| const repo = url.searchParams.get("repo") || "repo"; |
Contributor
There was a problem hiding this comment.
Unsafe defaults for installationId, owner, and repo violate issue requirements.
Issue #1539 explicitly states: "Reject missing or invalid installationId, owner, and repo (do not rely on unsafe defaults)."
Current behavior:
installationIddefaults to"1"when missing—theisNaNcheck only catches malformed strings like"abc", not absent parametersownerdefaults to"owner"repodefaults to"repo"
These defaults allow attackers to omit parameters and trigger rollback automation against arbitrary repository contexts.
🛡️ Proposed fix to reject missing parameters
const url = new URL(req.url);
- const installationId = parseInt(url.searchParams.get("installationId") || "1", 10);
- if (isNaN(installationId)) {
- return NextResponse.json({ error: "Invalid installationId" }, { status: 400 });
+ const installationIdParam = url.searchParams.get("installationId");
+ const ownerParam = url.searchParams.get("owner");
+ const repoParam = url.searchParams.get("repo");
+
+ if (!installationIdParam || !ownerParam || !repoParam) {
+ return NextResponse.json(
+ { error: "Missing required parameters: installationId, owner, repo" },
+ { status: 400 }
+ );
}
- const owner = url.searchParams.get("owner") || "owner";
- const repo = url.searchParams.get("repo") || "repo";
+
+ const installationId = parseInt(installationIdParam, 10);
+ if (isNaN(installationId) || installationId <= 0) {
+ return NextResponse.json({ error: "Invalid installationId" }, { status: 400 });
+ }
+
+ const owner = ownerParam;
+ const repo = repoParam;📝 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.
Suggested change
| const installationId = parseInt(url.searchParams.get("installationId") || "1", 10); | |
| if (isNaN(installationId)) { | |
| return NextResponse.json({ error: "Invalid installationId" }, { status: 400 }); | |
| } | |
| const owner = url.searchParams.get("owner") || "owner"; | |
| const repo = url.searchParams.get("repo") || "repo"; | |
| const installationIdParam = url.searchParams.get("installationId"); | |
| const ownerParam = url.searchParams.get("owner"); | |
| const repoParam = url.searchParams.get("repo"); | |
| if (!installationIdParam || !ownerParam || !repoParam) { | |
| return NextResponse.json( | |
| { error: "Missing required parameters: installationId, owner, repo" }, | |
| { status: 400 } | |
| ); | |
| } | |
| const installationId = parseInt(installationIdParam, 10); | |
| if (isNaN(installationId) || installationId <= 0) { | |
| return NextResponse.json({ error: "Invalid installationId" }, { status: 400 }); | |
| } | |
| const owner = ownerParam; | |
| const repo = repoParam; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/integrations/incidents/webhook/route.ts` around lines 33 - 38, The
code currently uses unsafe defaults for installationId, owner, and repo; remove
the fallback defaults and instead validate presence and correctness: for
installationId call url.searchParams.get("installationId") and if null or
parseInt(...,10) yields NaN return NextResponse.json({ error: "Missing or
invalid installationId" }, { status: 400 }); similarly require
url.searchParams.get("owner") and url.searchParams.get("repo") to be
non-null/non-empty and if missing return 400 (e.g. NextResponse.json({ error:
"Missing owner" }, { status: 400 }) and "Missing repo" respectively); update the
code paths referencing installationId, owner, and repo to use these validated
values (no default "1", "owner", or "repo").
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The incident webhook accepted unauthenticated POST requests and allowed installationId, owner, and repo via query params with unsafe defaults. Added x-webhook-secret header validation via INCIDENT_WEBHOOK_SECRET env var, and validates installationId as a number.
Closes #1539
Summary by CodeRabbit