[Security] Fix session cookie flags and strip internal errors from API responses#460
Conversation
Session cookie had no security attributes: no httpOnly meant JS could read the session ID via document.cookie; no secure meant it travelled over plain HTTP; no sameSite meant cross-site requests carried it, enabling CSRF. Added httpOnly, secure (production-gated), sameSite strict, and a 24-hour maxAge to the express-session cookie config. Both 500 handlers in routes/auth.js returned err.message verbatim, leaking MongoDB internals (collection name, index names, key values) to callers on any trigger-able error. Removed the error field from both responses so only the generic message is returned. Fixes GitMetricsLab#447
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 43 minutes. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. 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 |
|
dont need this as we will rewrite whole backend |
|
@mehul-m-prajapati why was my code not merged? did i make any mistakes in my code? please let me know |
|
@advikdivekar : Because you are trying to farm points by doing small PRs, your all changes can be done in one PR. Moreover, these changes were already done, so its not required for now |
I raised them as separate issues so went for separate PRs as well but I'll make sure to not follow that practice from here on, apologies from my end, I'll make sure it doesn't happen and make meaningful PRs no matter how many issues I have raised. |
2 participants
Problem
This PR addresses two independent but related HIGH-severity vulnerabilities in the backend.
1 — Session cookie missing security attributes (
backend/server.jslines 19–23)The
express-sessionconfig had nocookieblock, so the resultingconnect.sidcookie carried none of the standard security flags:httpOnly—document.cookiereturned the raw session ID from the browser console, meaning any XSS gadget or compromised npm package could exfiltrate it and achieve full account takeover.secure— the cookie was transmitted over plain HTTP, allowing a network observer on shared Wi-Fi or a transparent proxy to capture and replay it.sameSite— cross-site requests (e.g. an<img>on a third-party page targeting/api/auth/logout) automatically carried the session cookie, enabling CSRF on all current and future authenticated endpoints.2 — Raw MongoDB error internals returned in 500 responses (
backend/routes/auth.jslines 29 and 44)Both 500 error handlers returned
err.messageverbatim:{ message: 'Error creating user', error: err.message }— a duplicate-key violation exposes the full MongoDB error string including collection name, index name, and key value:E11000 duplicate key error collection: githubTracker.users index: email_1 dup key: { email: "..." }.{ message: 'Logout failed', error: err.message }— any session store error leaks internal implementation details.This fingerprints the database engine, collection structure, and index naming, narrowing the attack surface for further exploitation.
Changes
backend/server.jscookieblock to theexpress-sessionconfiguration:httpOnly: true— session ID invisible to JavaScript in all environmentssecure: process.env.NODE_ENV === 'production'— HTTPS-only in production, HTTP allowed in local devsameSite: 'strict'— cross-site requests never carry the cookiemaxAge: 24 * 60 * 60 * 1000— 24-hour session expirybackend/routes/auth.jserror: err.messagefrom the signup 500 responseerror: err.messagefrom the logout 500 responseBoth handlers now return only the generic message string — no internal state is disclosed.
Why this approach fixes the root cause
The cookie flags operate at the browser level before any application logic runs — they cannot be bypassed by an attacker who has already obtained a gadget in the page without the
httpOnlyflag. Strippingerr.messagefrom 500 responses is a boundary decision: internal errors belong in server-side logs, not in API responses visible to callers.Steps to test
npm startinbackend/).POST /api/auth/loginwith valid credentials. Open DevTools → Application → Cookies — confirmconnect.sidshowsHttpOnlyandSameSite=Strict.document.cookie— confirmconnect.sidis not present.{ "message": "Error creating user" }with no MongoDB error string.NODE_ENV=productionand restart — confirm theSecureflag now appears on the cookie.Edge cases covered
secureisfalseso login still works without HTTPS.secure: trueenforces HTTPS-only cookie transmission.httpOnly: trueblocks cookie access in all environments.Regression check
No route logic, validators, passport config, or tests were changed. Session serialization/deserialization in
passportConfig.jsis untouched. The cookie attribute additions are transparent to all code that reads fromreq.sessionorreq.user.Please review and merge this under GSSoC 2026.