fix(auth): harden session cookie, strip password from req.user, change logout to POST#575
Closed
anshul23102 wants to merge 1 commit into
Closed
Conversation
…e logout to POST Fixes GitMetricsLab#554 - Add httpOnly, Secure, and SameSite=strict to the session cookie so the token is inaccessible to JavaScript and cannot be sent in cross-origin requests. The Secure flag is conditioned on NODE_ENV=production to keep local development functional over HTTP. Fixes GitMetricsLab#555 - Pass .select('-password -__v').lean() to deserializeUser so the bcrypt hash is never attached to req.user. Using .lean() also returns a plain object instead of a full Mongoose document, preventing accidental exposure of model methods on the request context. Fixes GitMetricsLab#556 - Change the logout endpoint from GET to POST. GET is a safe, idempotent method that browsers trigger automatically (img src, link prefetch, CSS url). Any third-party page could embed a zero-pixel image pointing at the logout URL to silently sign out authenticated users. POST requires an explicit fetch or form submission, which CORS and SameSite=strict will block from cross-origin callers. The handler now also calls req.session.destroy() and clears the cookie to ensure the session is fully invalidated server-side.
This was referenced May 27, 2026
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Review limit reached
More reviews will be available in 50 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 1
🧹 Nitpick comments (1)
backend/config/passportConfig.js (1)
43-43: ⚡ Quick winPrefer an allowlist projection here.
select('-password -__v')still forwards every otherUserfield intoreq.user, so any future sensitive field becomes exposed by default. Select the exact fields handlers need instead of excluding just two known fields.Suggested change
- const user = await User.findById(id).select('-password -__v').lean(); + const user = await User.findById(id) + .select('_id username email') // expand only with explicitly safe fields + .lean();🤖 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 `@backend/config/passportConfig.js` at line 43, The current deserialize logic uses User.findById(id).select('-password -__v').lean(), which excludes only two fields and risks leaking future sensitive fields; change this to an allowlist projection on User.findById (e.g., explicitly select the exact fields your handlers require such as '_id', 'email', 'name', roles, etc.) so req.user only contains those properties—update the .select call in the same function where const user is assigned (the User.findById(...) line) to list required fields explicitly.
🤖 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 `@backend/routes/auth.js`:
- Around line 46-51: The route currently returns raw error messages from the
auth/session store (err.message and destroyErr.message) to clients; change this
to log the full error server-side (use console.error or the app logger) inside
the error branches for the callback that handles logout and req.session.destroy,
and replace the JSON response bodies from res.status(500).json({ message: ...,
error: ... }) with a generic res.status(500).json({ message: 'Internal server
error' }) so no internal details are leaked from the logout handler or
req.session.destroy callback.
---
Nitpick comments:
In `@backend/config/passportConfig.js`:
- Line 43: The current deserialize logic uses
User.findById(id).select('-password -__v').lean(), which excludes only two
fields and risks leaking future sensitive fields; change this to an allowlist
projection on User.findById (e.g., explicitly select the exact fields your
handlers require such as '_id', 'email', 'name', roles, etc.) so req.user only
contains those properties—update the .select call in the same function where
const user is assigned (the User.findById(...) line) to list required fields
explicitly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a48941c4-0c0b-4d52-8984-51c5db4580ee
📒 Files selected for processing (3)
backend/config/passportConfig.jsbackend/routes/auth.jsbackend/server.js
Comment on lines
46
to
+51
| if (err) | ||
| return res.status(500).json({ message: 'Logout failed', error: err.message }); | ||
| else | ||
|
|
||
| req.session.destroy((destroyErr) => { | ||
| if (destroyErr) { | ||
| return res.status(500).json({ message: 'Session cleanup failed', error: destroyErr.message }); |
Contributor
There was a problem hiding this comment.
Don’t return raw auth/session errors to the client.
These responses expose internal Passport or session-store details on an auth endpoint. Return a generic 500 body here and log the underlying error server-side instead.
Suggested change
+const logger = require("../logger");
...
- return res.status(500).json({ message: 'Logout failed', error: err.message });
+ logger.error('Logout failed', err);
+ return res.status(500).json({ message: 'Logout failed' });
...
- return res.status(500).json({ message: 'Session cleanup failed', error: destroyErr.message });
+ logger.error('Session cleanup failed', destroyErr);
+ return res.status(500).json({ message: 'Session cleanup failed' });📝 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
| if (err) | |
| return res.status(500).json({ message: 'Logout failed', error: err.message }); | |
| else | |
| req.session.destroy((destroyErr) => { | |
| if (destroyErr) { | |
| return res.status(500).json({ message: 'Session cleanup failed', error: destroyErr.message }); | |
| const logger = require("../logger"); | |
| // ... rest of file imports ... | |
| if (err) { | |
| logger.error('Logout failed', err); | |
| return res.status(500).json({ message: 'Logout failed' }); | |
| } | |
| req.session.destroy((destroyErr) => { | |
| if (destroyErr) { | |
| logger.error('Session cleanup failed', destroyErr); | |
| return res.status(500).json({ message: 'Session cleanup failed' }); |
🤖 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 `@backend/routes/auth.js` around lines 46 - 51, The route currently returns raw
error messages from the auth/session store (err.message and destroyErr.message)
to clients; change this to log the full error server-side (use console.error or
the app logger) inside the error branches for the callback that handles logout
and req.session.destroy, and replace the JSON response bodies from
res.status(500).json({ message: ..., error: ... }) with a generic
res.status(500).json({ message: 'Internal server error' }) so no internal
details are leaked from the logout handler or req.session.destroy callback.
Contributor
Author
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.
Summary
Fixes three independent security issues in the backend authentication layer.
Fix 1 - Session cookie hardening (#554)
express-sessionwas configured with nocookieoptions, so the session cookie was issued with browser defaults (nohttpOnly, noSecure, noSameSite).Changes in
backend/server.js:httpOnly: trueso JavaScript cannot read the cookie (XSS session hijack prevention)secure: isProductionso the cookie is only sent over HTTPS in productionsameSite: 'strict'so the cookie is not sent with any cross-origin request (CSRF prevention)maxAge: 24has an explicit TTLFix 2 - Remove password hash from req.user (#555)
deserializeUsercalledUser.findById(id)with no field projection, attaching the full Mongoose document (includingpassword) toreq.useron every authenticated request.Changes in
backend/config/passportConfig.js:.select('-password -__v')to exclude the bcrypt hash and internal version key.lean()to return a plain object instead of a Mongoose document, preventing accidental exposure of model methodsFix 3 - Change logout from GET to POST (#556)
GET /api/auth/logoutis exploitable via CSRF: any third-party page can embed<img src=".../logout">to silently sign out authenticated users.Changes in
backend/routes/auth.js:router.gettorouter.postreq.session.destroy()to fully invalidate the server-side sessionres.clearCookie('connect.sid')to remove the cookie from the browserTest Plan
httpOnly,Secure(production), andSameSite=Strictattributes in browser DevToolsreq.userin any route handler does not contain apasswordfieldGET /api/auth/logoutreturns 404;POST /api/auth/logoutreturns 200 and cookie is clearedImportant
GSSoC'26 contribution - Please add relevant labels (
gssoc26,level:intermediate,type:security) to help with point tracking. Thank you!Summary by CodeRabbit