Skip to content

fix(auth): harden session cookie, strip password from req.user, change logout to POST #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anshul23102 wants to merge 1 commit into from
Closed

fix(auth): harden session cookie, strip password from req.user, change logout to POST #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion backend/config/passportConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ passport.serializeUser((user, done) => {
});

// Deserialize user (retrieve user from session)
// Select only the fields needed for request handling. Excluding password
// prevents the bcrypt hash from being attached to req.user and accidentally
// serialized into API responses.
passport.deserializeUser(async (id, done) => {
try {
const user = await User.findById(id);
const user = await User.findById(id).select('-password -__v').lean();
done(null, user);
} catch (err) {
done(err, null);
Expand Down
13 changes: 11 additions & 2 deletions backend/routes/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,23 @@ router.post("/login", validateRequest(loginSchema), passport.authenticate('local
});

// Logout route
router.get("/logout", (req, res) => {
// POST is required here: GET is a safe/idempotent method that browsers trigger
// automatically (img src, prefetch, etc.), which would allow any third-party
// page to force-logout authenticated users via a passive CSRF request.
router.post("/logout", (req, res) => {

req.logout((err) => {

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 });
Comment on lines 46 to +51
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

}
res.clearCookie('connect.sid');
res.status(200).json({ message: 'Logged out successfully' });
});
});
});

Expand Down
8 changes: 8 additions & 0 deletions backend/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,18 @@ app.use(cors({

// Middleware
app.use(bodyParser.json());
const isProduction = process.env.NODE_ENV === 'production';

app.use(session({
secret: process.env.SESSION_SECRET,
resave: false,
saveUninitialized: false,
cookie: {
httpOnly: true,
secure: isProduction,
sameSite: 'strict',
maxAge: 24 * 60 * 60 * 1000, // 24 hours
},
}));
app.use(passport.initialize());
app.use(passport.session());
Expand Down
Loading