[Security] Harden session cookie with HttpOnly, Secure, and SameSite flags#456
Closed
advikdivekar wants to merge 1 commit into
Closed
[Security] Harden session cookie with HttpOnly, Secure, and SameSite flags#456advikdivekar wants to merge 1 commit into
advikdivekar wants to merge 1 commit into
Conversation
Set httpOnly to block JS access, secure to enforce HTTPS-only transmission in production, and sameSite to prevent cross-site requests from carrying the session cookie. Add NODE_ENV=production to the production Dockerfile so the secure flag activates correctly when deployed. Fixes GitMetricsLab#373
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/server.js (1)
19-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a production-grade session store (
store)
backend/server.jsconfig forapp.use(session({ ... }))omitsstore, soexpress-sessionuses the default in-memory store. In production this won’t survive restarts and won’t work across multiple app instances, making the 24hmaxAgemisleading for authenticated sessions.🤖 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/server.js` around lines 19 - 29, The session middleware block (app.use(session({...}))) currently omits the store option and thus uses the default in-memory store; replace that by wiring a production-grade session store (e.g., RedisStore from connect-redis or MongoStore from connect-mongo) and pass an instantiated store object into the session configuration under the store key; initialize the chosen store (e.g., create a Redis client and new RedisStore({ client })) before calling app.use(session(...)), read credentials from env vars, and keep existing settings (secret, cookie, resave, saveUninitialized) while ensuring secure cookie behavior in production.
🤖 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/server.js`:
- Around line 25-26: Express is configuring session cookies with secure:
process.env.NODE_ENV === 'production' but never sets app.set('trust proxy',
...), so when HTTPS is terminated upstream the app won't see requests as secure
and cookies may not be sent; add a call like app.set('trust proxy', 1) (or
app.set('trust proxy', process.env.TRUST_PROXY || 1) / conditional when
process.env.NODE_ENV === 'production') in the server initialization (before
express-session is used) to ensure req.secure is populated, and confirm the
ingress/proxy forwards X-Forwarded-Proto.
---
Outside diff comments:
In `@backend/server.js`:
- Around line 19-29: The session middleware block (app.use(session({...})))
currently omits the store option and thus uses the default in-memory store;
replace that by wiring a production-grade session store (e.g., RedisStore from
connect-redis or MongoStore from connect-mongo) and pass an instantiated store
object into the session configuration under the store key; initialize the chosen
store (e.g., create a Redis client and new RedisStore({ client })) before
calling app.use(session(...)), read credentials from env vars, and keep existing
settings (secret, cookie, resave, saveUninitialized) while ensuring secure
cookie behavior in production.
🪄 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: b997b2ce-b852-4c3c-b32d-a7bb0bf97e36
📒 Files selected for processing (2)
backend/Dockerfile.prodbackend/server.js
Comment on lines
+25
to
+26
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: process.env.NODE_ENV === 'production' ? 'strict' : 'lax', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the backend already trusts an upstream proxy
rg -nP --type=js -C2 "app\.set\(\s*['\"]trust proxy['\"]|proxy\s*:\s*true" backendRepository: GitMetricsLab/github_tracker
Length of output: 54
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== trust proxy (repo-wide) =="
rg -n --type=js "trust proxy" .
echo "== express-session usage (repo-wide) =="
rg -n --type=js "express-session" .
echo "== session cookie options (repo-wide) =="
rg -n --type=js "cookie\s*:\s*\{[^}]*secure" . || trueRepository: GitMetricsLab/github_tracker
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -u
echo "== trust proxy (repo-wide) =="
rg -n --type=js "trust proxy" . || true
echo "== express-session usage (repo-wide) =="
rg -n --type=js "express-session" . || true
echo "== session middleware / cookie secure options (repo-wide) =="
rg -n --type=js -S "cookie\s*:\s*\{|\bcookie\b.*secure:|secure:\s*process\.env\.NODE_ENV\s*===\s*'production'|secure:\s*true" .
echo "== likely app/session config files (backend) =="
ls -la backendRepository: GitMetricsLab/github_tracker
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba backend/server.js | sed -n '1,90p'Repository: GitMetricsLab/github_tracker
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=1 && NR<=90 {print NR ":" $0}' backend/server.jsRepository: GitMetricsLab/github_tracker
Length of output: 1416
Set trust proxy when behind a TLS-terminating reverse proxy/ingress.
backend/server.js configures express-session cookies with secure: process.env.NODE_ENV === 'production', but the Express app never sets app.set('trust proxy', ...). If HTTPS is terminated upstream, Express may not treat requests as secure, and the cookie may not be set in production—leading to broken sessions/auth. Add app.set('trust proxy', <value>) (and ensure the proxy forwards X-Forwarded-Proto) when deployed behind a proxy.
🤖 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/server.js` around lines 25 - 26, Express is configuring session
cookies with secure: process.env.NODE_ENV === 'production' but never sets
app.set('trust proxy', ...), so when HTTPS is terminated upstream the app won't
see requests as secure and cookies may not be sent; add a call like
app.set('trust proxy', 1) (or app.set('trust proxy', process.env.TRUST_PROXY ||
1) / conditional when process.env.NODE_ENV === 'production') in the server
initialization (before express-session is used) to ensure req.secure is
populated, and confirm the ingress/proxy forwards X-Forwarded-Proto.
Contributor
Author
|
@mehul-m-prajapati why was my code not merged? did i make any mistakes in my code? please let me know |
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
2 participants
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.
Problem
backend/server.jsconfiguredexpress-sessionwith no cookie security attributes. The resultingconnect.sidcookie had:HttpOnlyflag — JavaScript running in the page (including any XSS gadget or compromised npm package) could readdocument.cookieand exfiltrate the session ID, achieving full account takeover.Secureflag — the cookie was transmitted over plain HTTP, allowing any network observer on shared Wi-Fi, an ISP, or a transparent proxy to capture and replay the session ID.SameSiteflag — cross-site requests (e.g. an<img>tag on a third-party page pointing at/api/auth/logout) automatically carried the cookie, enabling CSRF attacks on all current and future authenticated endpoints.Additionally,
backend/Dockerfile.proddid not setNODE_ENV=production, so thesecure: process.env.NODE_ENV === 'production'check would never activate in the containerised deployment.Changes
backend/server.jscookieblock to theexpress-sessionconfiguration with:httpOnly: true— cookie is invisible to JavaScript in all environmentssecure: process.env.NODE_ENV === 'production'— HTTPS-only in productionsameSite: 'strict'in production,'lax'in development — blocks cross-site cookie sending while keeping local dev workflow functionalmaxAge: 24 * 60 * 60 * 1000— 24-hour expirybackend/Dockerfile.prodENV NODE_ENV=productionso thesecureflag correctly activates when the container is deployed.Why this approach fixes the root cause
The three flags address three independent attack vectors at the browser level before any application logic runs. Setting
secureconditionally onNODE_ENVpreserves the local development experience (HTTP over localhost) while enforcing HTTPS-only in production. SettingsameSite: 'strict'in production eliminates the CSRF surface on all authenticated endpoints without requiring per-endpoint CSRF token middleware.Steps to test
npm startinbackend/)./api/auth/loginwith valid credentials.Set-Cookieresponse header — confirm it containsHttpOnly,SameSite=Lax(dev), and noSecureflag in dev mode.docker build -f Dockerfile.prod -t tracker-prod .)./api/auth/login— confirmSet-Cookienow showsSecure; HttpOnly; SameSite=Strict.document.cookie— confirmconnect.sidis not present.Edge cases covered
SameSite=Laxand noSecureflag so login still works without HTTPS.NODE_ENV=productionis set in the Dockerfile, activatingSecureandSameSite=Strictautomatically.HttpOnlyblocks cookie access in all environments.Regression check
No existing routes, middleware, or tests were modified. Session serialization/deserialization logic in
passportConfig.jsis unchanged. The cookie attribute changes are transparent to all application code that reads fromreq.sessionorreq.user.Please review and merge this under GSSoC 2026.
Summary by CodeRabbit