Fix session fixation vulnerability and password hash exposure in login#378
Conversation
Regenerate session ID after successful login to prevent session fixation
attacks. Refactor login to callback form of passport.authenticate so
req.session.regenerate() can be called before req.logIn().
Return only safe fields (id, username, email) in the login response
instead of the full req.user document which included the bcrypt hash.
Fix deserializeUser to use .select('-password') so the hash is never
loaded into req.user on subsequent authenticated requests.
Unify auth failure messages to 'Invalid credentials' in the Passport
strategy to prevent user enumeration via distinct error strings.
Strip err.message from 500 error responses in signup and logout
to prevent leaking internal server details to clients.
Closes GitMetricsLab#375
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
mehul-m-prajapati
added
type:accessibility
level:beginner
and removed
level:intermediate
labels
|
🎉🎉 Thank you for your contribution! Your PR #378 has been merged! 🎉🎉 |
2 participants
What is the problem
Two vulnerabilities compound in the authentication flow:
Session fixation:
passport.authenticate('local')was used as direct middleware, which stores the authenticated user into the existing pre-login session ID without regenerating it. An attacker who can plant a known session ID in the victim's browser (via subdomain cookie injection or briefly shared access) becomes fully authenticated as that victim after the victim logs in — no password required.Password hash in login response:
deserializeUsercalledUser.findById(id)with no field projection, loading the full Mongoose document including the bcrypt-hashed password intoreq.user. The login route then returneduser: req.userdirectly, sending the hash over the wire to every client that logs in. This hash can be cracked offline with a dictionary.A third issue fixed in the same pass: distinct error messages (
'Email is invalid'vs'Invalid password') in the Passport strategy allowed an attacker to enumerate which email addresses are registered.What was changed
backend/routes/auth.jspassport.authenticate('local')as middleware to the callback form, enabling explicit error handling and flow control.req.session.regenerate()beforereq.logIn()so the server issues a new session ID on every successful login, destroying the pre-login session server-side.{ id, username, email }in the response body — neverreq.userwhich contains the full document.error: err.messagefrom the signup and logout 500 responses to prevent leaking internal server details.backend/config/passportConfig.js.select('-password')to theUser.findById()call indeserializeUserso the password hash is never loaded into memory on any authenticated request.'Email is invalid'and'Invalid password'with a single generic'Invalid credentials'message to close the user-enumeration vector.Why this approach
Session regeneration must happen before
req.logIn(), not after. Afterregenerate()the old session is destroyed server-side andreq.sessionpoints to a new empty session object;logIn()then writes the user ID into that new session. Reversing the order would leave a window where the old session holds the authenticated state. The callback form ofpassport.authenticateis required because middleware form gives no hook between authentication success and the response, making it impossible to callregenerate()in the correct position.Using
.select('-password')at the database layer (rather than deleting from the returned object) ensures the hash is never allocated in memory at all and cannot leak through any future code path that readsreq.user.How to test
GET /api/auth/logout) and record theconnect.sidcookie value from the response.POST /api/auth/loginwith valid credentials. Observe theSet-Cookieheader —connect.sidmust be a different value from step 1.{ "message": "Login successful", "user": { "id": "...", "username": "...", "email": "..." } }with nopasswordfield at any level.POST /api/auth/loginwith a registered email and wrong password, then with an unregistered email. Both must return{ "message": "Invalid credentials" }with status 401 — identical responses.Edge cases covered
regenerateErrandloginErrare both forwarded tonext()so they surface as 500 responses rather than silent hangs or unhandled promise rejections.info?.messageuses optional chaining so a missing info object from Passport does not throw..select('-password')is applied at query level, not as a post-processing delete, so the hash cannot be exposed by any future spread or serialisation ofreq.user.Verification
Labels:
type:securitylevel:advancedgssoc:approvedCloses #375
Please assign this PR to me under GSSoC 2026.