fix(session): persist production sessions with Mongo-backed session store#552
fix(session): persist production sessions with Mongo-backed session store#552Ridanshi wants to merge 1 commit into
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe PR centralizes session configuration into a dedicated module that wires ChangesMongoDB-Backed Session Store
Sequence DiagramsequenceDiagram
participant StartupEnv as Startup Environment
participant Server as backend/server.js
participant ConfigFactory as createSessionConfig
participant MongoStore as connect-mongo Store
participant MongoDB as MongoDB Instance
participant UserRequest as User Request
participant SessionMW as express-session Middleware
StartupEnv->>Server: MONGO_URI, SESSION_SECRET, NODE_ENV
Server->>ConfigFactory: Call createSessionConfig()
ConfigFactory->>MongoStore: new MongoStore({ mongoUrl, ttl })
ConfigFactory-->>Server: SessionOptions with MongoDB store
Server->>SessionMW: Mount session(createSessionConfig())
UserRequest->>SessionMW: Incoming request
SessionMW->>MongoStore: Check/retrieve session
MongoStore->>MongoDB: Query session data
MongoDB-->>MongoStore: Session doc
MongoStore-->>SessionMW: Session data
SessionMW-->>UserRequest: Request continues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/session.config.spec.cjs (1)
89-95: ⚡ Quick winAdd a regression test for missing
sessionSecret.You already guard missing
mongoUrl; add the same forsessionSecretso the fail-fast contract stays enforced.Suggested test addition
it('requires MongoDB configuration for persistent sessions', () => { expect(() => createSessionConfig({ mongoUrl: '', sessionSecret: 'test-secret', storeFactory: createStoreFactory(), })).toThrowError(/MONGO_URI/); }); + + it('requires SESSION_SECRET for session middleware', () => { + expect(() => createSessionConfig({ + mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', + sessionSecret: '', + storeFactory: createStoreFactory(), + })).toThrowError(/SESSION_SECRET/); + });🤖 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 `@spec/session.config.spec.cjs` around lines 89 - 95, Add a regression test that asserts createSessionConfig throws when sessionSecret is empty, mirroring the existing mongoUrl test: call createSessionConfig with mongoUrl set to a valid value (or left as in other tests), sessionSecret: '' and storeFactory: createStoreFactory(), and expect it toThrowError(/SESSION_SECRET|sessionSecret/) to enforce the fail-fast contract for missing session secrets; place the test next to the existing "requires MongoDB configuration for persistent sessions" test and reuse the same patterns/helpers.
🤖 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/config/session.js`:
- Around line 11-17: Validate that sessionSecret is present and throw a clear
error before returning the express-session config: add a check for sessionSecret
(the value derived from SESSION_SECRET) similar to the mongoUrl check and throw
new Error('SESSION_SECRET is required to configure the session store') if
missing so you fail fast before express-session is configured; keep this
validation alongside the existing mongoUrl check and prior to returning the
object containing secret: sessionSecret.
- Around line 23-25: The session cookie config currently sets only secure;
update the cookie options in the session configuration (the cookie object) to
include sameSite: 'none' when nodeEnv === 'production' so cross-site
credentialed requests work; keep secure: nodeEnv === 'production' and for
non-production environments leave sameSite unset or set to a more restrictive
value (e.g., 'lax') as appropriate. Ensure this change is applied to the cookie
object used by your session middleware (the cookie property in the session
config).
---
Nitpick comments:
In `@spec/session.config.spec.cjs`:
- Around line 89-95: Add a regression test that asserts createSessionConfig
throws when sessionSecret is empty, mirroring the existing mongoUrl test: call
createSessionConfig with mongoUrl set to a valid value (or left as in other
tests), sessionSecret: '' and storeFactory: createStoreFactory(), and expect it
toThrowError(/SESSION_SECRET|sessionSecret/) to enforce the fail-fast contract
for missing session secrets; place the test next to the existing "requires
MongoDB configuration for persistent sessions" test and reuse the same
patterns/helpers.
🪄 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: 213a17fe-5ec2-4504-a0af-fa73f20de9ab
📒 Files selected for processing (6)
README.mdbackend/config/session.jsbackend/package.jsonbackend/server.jspackage.jsonspec/session.config.spec.cjs
| if (!mongoUrl) { | ||
| throw new Error('MONGO_URI is required to configure the session store'); | ||
| } | ||
|
|
||
| return { | ||
| secret: sessionSecret, | ||
| resave: false, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In express-session v1.18.x, is the secret option required, and what happens when it is omitted?
💡 Result:
In express-session v1.18.x, the secret option is a required configuration [1][2][3]. It is used to sign the session ID cookie, which helps prevent tampering [3][4]. If the secret option is omitted, the middleware will fail to initialize properly. Specifically, the library explicitly checks for the presence of a secret during request handling and will trigger an error if none is found [5]. When a request is made, if no secret is provided (and no fallback is available), the middleware will call next with an error: "secret option required for sessions" [5]. Previous versions of the library may have allowed a fallback to req.secret (often provided by cookie-parser), but this practice is deprecated and will result in a warning; relying on it is not supported [6][7][5].
Citations:
- 1: https://www.npmjs.com/package/express-session
- 2: https://github.com/expressjs/session
- 3: https://expressjs.com/en/resources/middleware/session/
- 4: https://github.com/expressjs/session/blob/54ff0efb05e2ef54186502c4786da66c6fd4c8c9/README.md
- 5: https://github.com/expressjs/session/blob/master/index.js
- 6: https://stackoverflow.com/questions/68624104/express-session-deprecated-req-secret-provide-secret-option
- 7: https://forum.freecodecamp.org/t/advanced-node-and-express-implement-the-serialization-of-a-passport-user-error-express-session-deprecated-req-secret-provide-secret-option-server-js/470453
Fail fast if SESSION_SECRET is missing before configuring express-session.
express-session requires the secret option; passing an undefined sessionSecret will cause the middleware to error when handling requests (instead of clear config validation in this module).
Proposed fix
function createSessionConfig({
mongoUrl = process.env.MONGO_URI,
sessionSecret = process.env.SESSION_SECRET,
nodeEnv = process.env.NODE_ENV,
storeFactory = MongoStore,
} = {}) {
if (!mongoUrl) {
throw new Error('MONGO_URI is required to configure the session store');
}
+ if (!sessionSecret) {
+ throw new Error('SESSION_SECRET is required to configure session middleware');
+ }
return {🤖 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/config/session.js` around lines 11 - 17, Validate that sessionSecret
is present and throw a clear error before returning the express-session config:
add a check for sessionSecret (the value derived from SESSION_SECRET) similar to
the mongoUrl check and throw new Error('SESSION_SECRET is required to configure
the session store') if missing so you fail fast before express-session is
configured; keep this validation alongside the existing mongoUrl check and prior
to returning the object containing secret: sessionSecret.
| cookie: { | ||
| secure: nodeEnv === 'production', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For modern browsers, what SameSite value is required for cookies to be sent on cross-site fetch/XHR requests with credentials?
💡 Result:
For modern browsers, to ensure that cookies are sent on cross-site fetch or XHR requests (typically initiated via APIs like fetch or XMLHttpRequest), the cookie must be set with the SameSite=None attribute [1][2][3]. Additionally, when SameSite=None is used, the following requirements must be met: 1. The Secure attribute must also be included, meaning the cookie will only be transmitted over encrypted (HTTPS) connections [1][4][3]. 2. For the request itself, the client must explicitly opt-in to sending credentials. For fetch, this is done by setting the credentials option to 'include' [5][4][6]. For XMLHttpRequest, the withCredentials property must be set to true [4]. 3. The server must also correctly configure Cross-Origin Resource Sharing (CORS) headers, specifically setting Access-Control-Allow-Credentials to true [5][4]. Without setting SameSite=None, modern browsers will typically default to Lax, which restricts cookies to same-site contexts and top-level navigations, excluding them from cross-site subresource requests like fetch/XHR [1][7][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie
- 2: https://web.dev/articles/samesite-cookies-explained
- 3: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Cookies
- 4: https://stackoverflow.com/questions/46288437/set-cookies-for-cross-origin-requests
- 5: https://zellwk.com/blog/fetch-credentials/
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker
- 7: https://portswigger.net/web-security/csrf/bypassing-samesite-restrictions
Set sameSite for production session cookies when using cross-site credentialed requests.
secure alone isn’t sufficient for modern browsers to send cookies on cross-site fetch/XHR with credentials; that requires SameSite=None (and Secure). Add sameSite: 'none' in production (and keep secure enabled there).
Proposed fix
cookie: {
secure: nodeEnv === 'production',
+ sameSite: nodeEnv === 'production' ? 'none' : 'lax',
+ httpOnly: true,
},📝 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.
| cookie: { | |
| secure: nodeEnv === 'production', | |
| }, | |
| cookie: { | |
| secure: nodeEnv === 'production', | |
| sameSite: nodeEnv === 'production' ? 'none' : 'lax', | |
| httpOnly: true, | |
| }, |
🤖 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/config/session.js` around lines 23 - 25, The session cookie config
currently sets only secure; update the cookie options in the session
configuration (the cookie object) to include sameSite: 'none' when nodeEnv ===
'production' so cross-site credentialed requests work; keep secure: nodeEnv ===
'production' and for non-production environments leave sameSite unset or set to
a more restrictive value (e.g., 'lax') as appropriate. Ensure this change is
applied to the cookie object used by your session middleware (the cookie
property in the session config).
1 participant
Closes #453
Summary
Replaces the default
express-sessionMemoryStorefallback with a persistent Mongo-backed session store usingconnect-mongo.The backend previously initialized
express-sessionwithout a configuredstore, causing Express to silently fall back to the built-in in-memoryMemoryStore, which is explicitly not production-safe.This PR introduces durable Mongo-backed session persistence while preserving the existing session/auth behavior.
Problems Fixed
The previous implementation caused:
Because all sessions existed only in process memory.
Implementation
Added shared session configuration using:
Key changes:
MemoryStorefallbackMONGO_URIExisting behavior preserved:
resave: falsesaveUninitialized: falseFiles Changed
backend/config/session.jsbackend/server.jsbackend/package.jsonpackage.jsonspec/session.config.spec.cjsREADME.mdTests
Focused regression coverage added for:
Verification
Result:
Full backend suite was also attempted:
Existing unrelated MongoDB-dependent auth/model tests timed out locally, but all newly-added session persistence tests passed successfully.
Notes
This PR intentionally keeps the scope limited to production session persistence and infrastructure hardening.
No JWT migration, auth architecture rewrite, or unrelated middleware refactors were introduced.
Summary by CodeRabbit
Documentation
Improvements
Tests