fix: prevent potential request body double-read in webhook handler#666
fix: prevent potential request body double-read in webhook handler#666itsmiso-ai wants to merge 1 commit into
Conversation
Move the body read (request.arrayBuffer()) before calling authorizeRequest() so that if authorizeRequest ever consumes the body stream, HMAC verification still operates on the original payload rather than an empty buffer. Also reset processPrFollowupEvents mock in beforeEach to prevent test interference from the "returns 500 on processing error" test case. Add regression test that verifies body integrity is preserved when the request body is consumed before authorization. Fixes #656 Signed-off-by: Saffron <[email protected]>
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — routed smart (risk match: public_route_changes)
Recommendation: Approve
This PR correctly addresses the security concern in issue PR 656 by reordering the request body read to occur before authorizeRequest(). The change is minimal (4-line net change), targeted, and includes a regression test.
Change-by-Change Findings
src/app/api/pr-followup/webhook/route.ts
- What changed: Moved
request.arrayBuffer()to execute beforeauthorizeRequest(), with a clarifying comment. - Security impact: Prevents a potential HMAC verification bypass where an empty body would silently pass signature verification if
authorizeRequestever consumed the request stream. - Behavior preserved: Authorization still returns 401 for unauthenticated requests; HMAC verification still occurs after auth.
src/app/api/pr-followup/webhook/route.test.ts
- What changed: Added a regression test that clones the request, reads the body independently before calling
POST, and verifies the body was not consumed. - Coverage: Tests the defensive posture—verifies the handler still works correctly even if authorization reads the stream.
Required Checks
✅ verify route access controls are in place
Confirmed. The authorizeRequest(request) call remains in place (line 193 in the diff) and still returns 401 when !auth.authorized. Authorization is unchanged—the fix only reorders when the body is read. No route-level security has been removed or weakened.
✅ check for unintended public endpoints
Confirmed. No new endpoints were added. The webhook route at /api/pr-followup/webhook remains the only modified route and continues to enforce bearer token authentication via authorizeRequest(). The webhook is an inbound GitHub webhook endpoint; it is not exposed as a public consumer-facing API beyond its existing GitHub webhook purpose.
Standards Compliance
- API routes: Returns appropriate HTTP status codes (401 for unauthorized, 400 for missing headers).
- Error handling: Uses
errorResponse()pattern consistent with the codebase. - No commit of secrets: No secrets were introduced.
- Auth consistency: Uses the established
authorizeRequesthelper fromlib/auth.ts.
Linked Issue Fit
Issue PR 656 acceptance criteria:
- "Move the body read before authorization" — ✅ Done.
request.arrayBuffer()now executes beforeauthorizeRequest(). - "Add a test that verifies body integrity after authorization" — ✅ Done. Regression test added at line 199 of the test file.
The implementation fully satisfies the issue requirements.
CI Results
All checks passed: Build, Docker Build, Tests, Lint, Typecheck.
Findings
None.
1 participant
What
Moves
request.arrayBuffer()to execute beforeauthorizeRequest()in the PR follow-up webhook handler, and adds a test verifying body integrity when authorization consumes the stream.Why
If
authorizeRequestever reads/consumes the request body, the subsequent…Fixes #656
Opened by foreman on review GO (workload wl-misospace-dispatch-656).