fix(auth): handle GET OAuth callback with undefined req.body on Express 5#3496
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis change addresses an Express 5 compatibility issue where OAuth GET callbacks crash because Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3496 +/- ##
=======================================
Coverage 85.99% 85.99%
=======================================
Files 116 116
Lines 2957 2957
Branches 829 829
=======================================
Hits 2543 2543
Misses 328 328
Partials 86 86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes an Express 5 regression where req.body can be undefined on GET OAuth callbacks, causing the classic web OAuth callback handler to crash before invoking Passport.
Changes:
- Guard
req.bodyaccess inoauthCallbackwith optional chaining to avoid crashes on GET callbacks. - Add a regression test that calls
oauthCallbackwith noreq.body(Express 5 GET-style request).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/auth/controllers/auth.controller.js | Prevents crash by optional-chaining req.body access in the client-side OAuth branch check. |
| modules/auth/tests/auth.integration.tests.js | Adds a test covering the Express 5 GET callback scenario where req.body is missing. |
| const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( | ||
| (strategy, callback) => () => callback(null, { id: 'mock-get-cb-user' }), | ||
| ); | ||
| const cookies = {}; | ||
| const redirectCalls = []; | ||
| const mockReq = { params: { strategy: 'google' } }; | ||
| const mockRes = { | ||
| cookie(name, val, opts) { cookies[name] = { val, opts }; return this; }, | ||
| redirect(code, url) { redirectCalls.push({ code, url }); }, | ||
| }; | ||
|
|
||
| await AuthController.oauthCallback(mockReq, mockRes, () => {}); | ||
|
|
||
| expect(cookies.TOKEN).toBeDefined(); | ||
| expect(redirectCalls[0]).toMatchObject({ code: 302 }); | ||
| authenticateSpy.mockRestore(); | ||
| }); |
| await AuthController.oauthCallback(mockReq, mockRes, () => {}); | ||
|
|
||
| expect(cookies.TOKEN).toBeDefined(); | ||
| expect(redirectCalls[0]).toMatchObject({ code: 302 }); |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully addresses the crash in the OAuth callback handler encountered with Express 5 by ensuring req.body is handled safely via optional chaining. All acceptance criteria, including the addition of integration tests for both the new edge case and existing manual strategy logic, have been met.
Codacy reports that the changes are up to standards. No security flaws or major logic bugs were identified. The only finding is a minor suggestion regarding code duplication in the newly added tests to improve long-term maintainability.
Test suggestions
- OAuth callback handling with undefined req.body (Express 5 GET flow)
- OAuth callback with valid req.body for manual strategy verification
🗒️ Improve review quality by adding custom instructions
| ); | ||
| const cookies = {}; | ||
| const redirectCalls = []; | ||
| const mockReq = { params: { strategy: 'google' } }; |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: This test boilerplate for mocking the response object and tracking redirects is duplicated from lines 613-622. Extracting this into a shared helper function would simplify the test suite.
Try running the following prompt in your IDE agent:
Refactor the integration tests in modules/auth/tests/auth.integration.tests.js by creating a setupMockAuthResponse helper function that returns the cookies, redirectCalls, and mockRes object. Then, update the tests at lines 613 and 635 to use this helper.
Summary
req.bodyaccess inoauthCallback— Express 5 leavesreq.bodyundefined on GET callbacks, crashing the classic web OAuth flow before reachingpassport.authenticate().bodyfield on request).Closes #3492
Context
Discovered while enabling Google OAuth on trawl (
api.trawl.me). First real signin attempt returned:Root cause: Express 5 no longer initializes
req.bodyto{}when no body-parser middleware applies to the request. The existing unit test atauth.integration.tests.js:613passedbody: {}explicitly, hiding the bug.Apple OAuth is unaffected (uses POST
form_post→ body populated byexpress.urlencoded).Test plan
npm run test:integration— 260/260 green/update-all-projects, re-test Google signin end-to-end on trawl.meRelated follow-ups (separate PRs)
emailVerified: trueSummary by CodeRabbit
Bug Fixes
Tests