diff --git a/docs/pr-review-fix-queue.md b/docs/pr-review-fix-queue.md index c1f54059..c7042358 100644 --- a/docs/pr-review-fix-queue.md +++ b/docs/pr-review-fix-queue.md @@ -46,7 +46,12 @@ Configuration: - `check_run` — failing CI checks - `pull_request` — merge state changes -Signature verification uses HMAC-SHA256 with `WEBHOOK_SECRET`. If not set, verification is skipped (e.g., behind an API gateway). +Signature verification uses HMAC-SHA256 with `WEBHOOK_SECRET`. + +**Fail-closed default:** If `WEBHOOK_SECRET` is not configured, requests are +rejected with a 503 response unless `WEBHOOK_GATEWAY_MODE` is explicitly set to +`"true"` (indicating the endpoint is behind an API gateway that handles its own +authentication and signature verification). ## Feedback classification diff --git a/src/app/api/pr-followup/webhook/route.test.ts b/src/app/api/pr-followup/webhook/route.test.ts index 62aecc85..4d79e2f3 100644 --- a/src/app/api/pr-followup/webhook/route.test.ts +++ b/src/app/api/pr-followup/webhook/route.test.ts @@ -25,6 +25,7 @@ vi.mock("@/lib/pr-followup-ingestion", async (importOriginal) => ({ import { POST } from "./route"; import { resetAuthCaches } from "@/lib/auth"; +import crypto from "node:crypto"; function postRequest(body: unknown, headers: Record = {}) { return POST( @@ -41,13 +42,108 @@ describe("POST /api/pr-followup/webhook", () => { beforeEach(() => { delete process.env.DISPATCH_AUTH_MODE; delete process.env.WEBHOOK_SECRET; - delete process.env.WEBHOOK_GATEWAY_MODE; + // Default to gateway mode so existing tests pass without signature headers. + // Signature-specific tests below explicitly unset this. + process.env.WEBHOOK_GATEWAY_MODE = "true"; resetAuthCaches(); vi.clearAllMocks(); mocks.prFixQueueClient.mockReturnValue({}); mocks.processPrFollowupEvents.mockResolvedValue({ enqueued: 1, skipped: 0 }); }); + describe("signature verification (fail-closed default)", () => { + it("rejects with 503 when neither WEBHOOK_SECRET nor WEBHOOK_GATEWAY_MODE is configured", async () => { + delete process.env.WEBHOOK_GATEWAY_MODE; + + const res = await postRequest( + { action: "submitted", review: { state: "CHANGES_REQUESTED" } }, + { + Authorization: `Bearer ${mockToken}`, + "x-github-event": "pull_request_review", + }, + ); + + expect(res.status).toBe(503); + const body = await res.json(); + expect(body.error).toContain("not configured"); + }); + + it("processes without signature when WEBHOOK_GATEWAY_MODE is true", async () => { + // WEBHOOK_GATEWAY_MODE is already "true" from beforeEach + delete process.env.WEBHOOK_SECRET; + + const res = await postRequest( + { action: "submitted", review: { state: "CHANGES_REQUESTED" } }, + { + Authorization: `Bearer ${mockToken}`, + "x-github-event": "pull_request_review", + }, + ); + + expect(res.status).toBe(200); + }); + + it("rejects with 401 when WEBHOOK_SECRET is set but no signature header", async () => { + delete process.env.WEBHOOK_GATEWAY_MODE; + process.env.WEBHOOK_SECRET = "test-secret"; + + const res = await postRequest( + { action: "submitted", review: { state: "CHANGES_REQUESTED" } }, + { + Authorization: `Bearer ${mockToken}`, + "x-github-event": "pull_request_review", + }, + ); + + expect(res.status).toBe(401); + const body = await res.json(); + expect(body.error).toContain("Missing x-hub-signature-256"); + }); + + it("rejects with 401 when signature is invalid", async () => { + delete process.env.WEBHOOK_GATEWAY_MODE; + process.env.WEBHOOK_SECRET = "test-secret"; + + const res = await postRequest( + { action: "submitted", review: { state: "CHANGES_REQUESTED" } }, + { + Authorization: `Bearer ${mockToken}`, + "x-github-event": "pull_request_review", + "x-hub-signature-256": "sha256=invalid", + }, + ); + + expect(res.status).toBe(401); + const body = await res.json(); + expect(body.error).toContain("Invalid webhook signature"); + }); + + it("processes successfully with valid signature", async () => { + delete process.env.WEBHOOK_GATEWAY_MODE; + process.env.WEBHOOK_SECRET = "test-secret"; + + const payload = { action: "submitted", review: { state: "CHANGES_REQUESTED" } }; + const bodyStr = JSON.stringify(payload); + const sig = + "sha256=" + crypto.createHmac("sha256", "test-secret").update(bodyStr).digest("hex"); + + // Use a direct Request so the body bytes are exactly what we computed the HMAC over. + const req = new Request("http://localhost/api/pr-followup/webhook", { + method: "POST", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${mockToken}`, + "x-github-event": "pull_request_review", + "x-hub-signature-256": sig, + }, + body: bodyStr, + }); + const res = await POST(req); + + expect(res.status).toBe(200); + }); + }); + it("returns 401 when no auth header is present", async () => { const res = await postRequest({}, { "x-github-event": "pull_request_review" }); diff --git a/src/app/api/pr-followup/webhook/route.ts b/src/app/api/pr-followup/webhook/route.ts index f122521a..63c0bf9b 100644 --- a/src/app/api/pr-followup/webhook/route.ts +++ b/src/app/api/pr-followup/webhook/route.ts @@ -20,25 +20,35 @@ import { enforceRateLimit } from "@/lib/rate-limit"; * with the WEBHOOK_SECRET environment variable. * * Default behavior is fail-closed: if WEBHOOK_SECRET is not configured, - * requests are rejected unless WEBHOOK_GATEWAY_MODE is explicitly set to "true", + * requests are rejected (503) unless WEBHOOK_GATEWAY_MODE is explicitly set to "true", * which indicates the endpoint is behind a gateway that performs its own * authentication and signature verification. */ -/** Is webhook signature verification enabled (fail-closed default)? */ -function isSignatureVerificationEnabled(): boolean { +/** + * Determine signature verification mode. + * + * - "verify": WEBHOOK_SECRET is set — verify HMAC-SHA256 signature + * - "skip": WEBHOOK_GATEWAY_MODE is "true" — skip verification (behind API gateway) + * - "reject": neither configured — fail-closed, reject all requests + */ +function getSignatureVerificationMode(): "verify" | "skip" | "reject" { const secret = process.env.WEBHOOK_SECRET; - if (secret) return true; - // Gateway mode: caller explicitly opts out of local signature verification - return process.env.WEBHOOK_GATEWAY_MODE === "true"; + if (secret) return "verify"; + if (process.env.WEBHOOK_GATEWAY_MODE === "true") return "skip"; + // Fail-closed: reject requests when neither WEBHOOK_SECRET nor WEBHOOK_GATEWAY_MODE is configured + return "reject"; } function verifyWebhookSignature(secret: string, payload: Buffer, signature: string): boolean { if (!signature.startsWith("sha256=")) return false; - const expected = signature.slice(9); + const expected = signature.slice(7); const hmac = createHmac("sha256", secret); hmac.update(payload); const computed = hmac.digest("hex"); + + // Constant-time comparison; timingSafeEqual requires equal-length buffers + if (computed.length !== expected.length) return false; return timingSafeEqual(Buffer.from(computed), Buffer.from(expected)); } @@ -204,14 +214,20 @@ export async function POST(request: Request) { // Webhook signature verification: fail-closed by default. // If WEBHOOK_SECRET is set, always verify. If not set, only skip when // WEBHOOK_GATEWAY_MODE=true (explicit opt-out for gateway deployments). - const sigVerificationEnabled = isSignatureVerificationEnabled(); - if (sigVerificationEnabled) { - const webhookSecret = process.env.WEBHOOK_SECRET; + const sigMode = getSignatureVerificationMode(); + if (sigMode === "reject") { + return errorResponse( + "Webhook signature verification is not configured. Set WEBHOOK_SECRET or enable WEBHOOK_GATEWAY_MODE.", + 503, + ); + } + if (sigMode === "verify") { + const webhookSecret = process.env.WEBHOOK_SECRET!; const signature = request.headers.get("x-hub-signature-256"); if (!signature) { return errorResponse("Missing x-hub-signature-256 header", 401); } - if (!verifyWebhookSignature(webhookSecret!, payload, signature)) { + if (!verifyWebhookSignature(webhookSecret, payload, signature)) { return errorResponse("Invalid webhook signature", 401); } }