Skip to content

Commit 7986d10

Browse files
committed
Fix session fixation and password hash exposure in login flow
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 #375
1 parent 8d17610 commit 7986d10

2 files changed

Lines changed: 26 additions & 9 deletions

File tree

backend/config/passportConfig.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,13 @@ passport.use(
99
try {
1010
const user = await User.findOne( {email} );
1111
if (!user) {
12-
return done(null, false, { message: 'Email is invalid '});
12+
// Generic message prevents user enumeration
13+
return done(null, false, { message: 'Invalid credentials' });
1314
}
1415

1516
const isMatch = await user.comparePassword(password);
1617
if (!isMatch) {
17-
return done(null, false, { message: 'Invalid password' });
18+
return done(null, false, { message: 'Invalid credentials' });
1819
}
1920

2021
return done(null, {
@@ -34,10 +35,10 @@ passport.serializeUser((user, done) => {
3435
done(null, user.id);
3536
});
3637

37-
// Deserialize user (retrieve user from session)
38+
// Deserialize user — exclude password hash from req.user on every request
3839
passport.deserializeUser(async (id, done) => {
3940
try {
40-
const user = await User.findById(id);
41+
const user = await User.findById(id).select('-password');
4142
done(null, user);
4243
} catch (err) {
4344
done(err, null);

backend/routes/auth.js

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,29 @@ router.post("/signup", validateRequest(signupSchema), async (req, res) => {
2626
return res.status(400).json({ message: 'User already exists' });
2727
}
2828

29-
res.status(500).json({ message: 'Error creating user', error: err.message });
29+
res.status(500).json({ message: 'Error creating user' });
3030
}
3131
});
3232

33-
// Login route
34-
router.post("/login", validateRequest(loginSchema), passport.authenticate('local'), (req, res) => {
35-
res.status(200).json( { message: 'Login successful', user: req.user } );
33+
// Login route — session is regenerated after successful authentication
34+
// to prevent session fixation; only safe fields returned in the response
35+
router.post("/login", validateRequest(loginSchema), (req, res, next) => {
36+
passport.authenticate('local', (err, user, info) => {
37+
if (err) return next(err);
38+
if (!user) return res.status(401).json({ message: info?.message || 'Invalid credentials' });
39+
40+
req.session.regenerate((regenerateErr) => {
41+
if (regenerateErr) return next(regenerateErr);
42+
43+
req.logIn(user, (loginErr) => {
44+
if (loginErr) return next(loginErr);
45+
res.status(200).json({
46+
message: 'Login successful',
47+
user: { id: user.id, username: user.username, email: user.email },
48+
});
49+
});
50+
});
51+
})(req, res, next);
3652
});
3753

3854
// Logout route
@@ -41,7 +57,7 @@ router.get("/logout", (req, res) => {
4157
req.logout((err) => {
4258

4359
if (err)
44-
return res.status(500).json({ message: 'Logout failed', error: err.message });
60+
return res.status(500).json({ message: 'Logout failed' });
4561
else
4662
res.status(200).json({ message: 'Logged out successfully' });
4763
});

0 commit comments

Comments
 (0)