-
Notifications
You must be signed in to change notification settings - Fork 202
fix(session): persist production sessions with Mongo-backed session store #552
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||||||||||||||
| const MongoStore = require('connect-mongo'); | ||||||||||||||||||
|
|
||||||||||||||||||
| const SESSION_TTL_SECONDS = 14 * 24 * 60 * 60; | ||||||||||||||||||
|
|
||||||||||||||||||
| 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'); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return { | ||||||||||||||||||
| secret: sessionSecret, | ||||||||||||||||||
| resave: false, | ||||||||||||||||||
| saveUninitialized: false, | ||||||||||||||||||
| store: storeFactory.create({ | ||||||||||||||||||
| mongoUrl, | ||||||||||||||||||
| ttl: SESSION_TTL_SECONDS, | ||||||||||||||||||
| }), | ||||||||||||||||||
| cookie: { | ||||||||||||||||||
| secure: nodeEnv === 'production', | ||||||||||||||||||
| }, | ||||||||||||||||||
|
Comment on lines
+23
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 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:
Set
Proposed fix cookie: {
secure: nodeEnv === 'production',
+ sameSite: nodeEnv === 'production' ? 'none' : 'lax',
+ httpOnly: true,
},📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| module.exports = { | ||||||||||||||||||
| SESSION_TTL_SECONDS, | ||||||||||||||||||
| createSessionConfig, | ||||||||||||||||||
| }; | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| const express = require('express'); | ||
| const request = require('supertest'); | ||
| const session = require('express-session'); | ||
|
|
||
| const { SESSION_TTL_SECONDS, createSessionConfig } = require('../backend/config/session'); | ||
|
|
||
| class TestSessionStore extends session.Store { | ||
| constructor() { | ||
| super(); | ||
| this.sessions = new Map(); | ||
| } | ||
|
|
||
| get(sid, callback) { | ||
| const sessionData = this.sessions.get(sid); | ||
| callback(null, sessionData ? JSON.parse(sessionData) : null); | ||
| } | ||
|
|
||
| set(sid, sessionData, callback) { | ||
| this.sessions.set(sid, JSON.stringify(sessionData)); | ||
| callback(null); | ||
| } | ||
|
|
||
| destroy(sid, callback) { | ||
| this.sessions.delete(sid); | ||
| callback(null); | ||
| } | ||
| } | ||
|
|
||
| function createStoreFactory(store = new TestSessionStore()) { | ||
| const calls = []; | ||
|
|
||
| return { | ||
| calls, | ||
| store, | ||
| create(options) { | ||
| calls.push(options); | ||
| return store; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe('Session configuration', () => { | ||
| it('initializes connect-mongo with the configured Mongo URL and TTL', () => { | ||
| const storeFactory = createStoreFactory(); | ||
|
|
||
| const config = createSessionConfig({ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| sessionSecret: 'test-secret', | ||
| storeFactory, | ||
| }); | ||
|
|
||
| expect(storeFactory.calls).toEqual([{ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| ttl: SESSION_TTL_SECONDS, | ||
| }]); | ||
| expect(config.store).toBe(storeFactory.store); | ||
| expect(SESSION_TTL_SECONDS).toBe(14 * 24 * 60 * 60); | ||
| }); | ||
|
|
||
| it('does not fall back to express-session MemoryStore', () => { | ||
| const config = createSessionConfig({ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| sessionSecret: 'test-secret', | ||
| storeFactory: createStoreFactory(), | ||
| }); | ||
|
|
||
| expect(config.store).toBeDefined(); | ||
| expect(config.store instanceof session.MemoryStore).toBeFalse(); | ||
| }); | ||
|
|
||
| it('uses secure cookies in production only', () => { | ||
| const productionConfig = createSessionConfig({ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| sessionSecret: 'test-secret', | ||
| nodeEnv: 'production', | ||
| storeFactory: createStoreFactory(), | ||
| }); | ||
| const developmentConfig = createSessionConfig({ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| sessionSecret: 'test-secret', | ||
| nodeEnv: 'development', | ||
| storeFactory: createStoreFactory(), | ||
| }); | ||
|
|
||
| expect(productionConfig.cookie.secure).toBeTrue(); | ||
| expect(developmentConfig.cookie.secure).toBeFalse(); | ||
| }); | ||
|
|
||
| it('requires MongoDB configuration for persistent sessions', () => { | ||
| expect(() => createSessionConfig({ | ||
| mongoUrl: '', | ||
| sessionSecret: 'test-secret', | ||
| storeFactory: createStoreFactory(), | ||
| })).toThrowError(/MONGO_URI/); | ||
| }); | ||
|
|
||
| it('persists session data through the configured store', async () => { | ||
| const app = express(); | ||
|
|
||
| app.use(session(createSessionConfig({ | ||
| mongoUrl: 'mongodb://127.0.0.1:27017/github_tracker_test', | ||
| sessionSecret: 'test-secret', | ||
| storeFactory: createStoreFactory(), | ||
| }))); | ||
| app.get('/count', (req, res) => { | ||
| req.session.views = (req.session.views || 0) + 1; | ||
| res.json({ views: req.session.views }); | ||
| }); | ||
|
|
||
| const agent = request.agent(app); | ||
|
|
||
| await agent.get('/count').expect(200, { views: 1 }); | ||
| await agent.get('/count').expect(200, { views: 2 }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
In express-session v1.18.x, is thesecretoption 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:
Fail fast if
SESSION_SECRETis missing before configuringexpress-session.express-sessionrequires thesecretoption; passing an undefinedsessionSecretwill 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