diff --git a/settings.json.template b/settings.json.template index 8c242a311..4d68a10db 100644 --- a/settings.json.template +++ b/settings.json.template @@ -463,7 +463,13 @@ * Automatic session refreshes can be disabled (not recommended) by setting * this to null. */ - "sessionRefreshInterval": 86400000 // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + "sessionRefreshInterval": 86400000, // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + + /* + * Whether to periodically clean up expired and stale sessions from the + * database. Set to false to disable. Default: true. + */ + "sessionCleanup": true }, /* diff --git a/src/node/db/SessionStore.ts b/src/node/db/SessionStore.ts index 15eb5a971..2bbfd5a89 100644 --- a/src/node/db/SessionStore.ts +++ b/src/node/db/SessionStore.ts @@ -9,6 +9,9 @@ const util = require('util'); const logger = log4js.getLogger('SessionStore'); +// How often to run the cleanup of expired/stale sessions. +const CLEANUP_INTERVAL_MS = 60 * 60 * 1000; // 1 hour + class SessionStore extends expressSession.Store { /** * @param {?number} [refresh] - How often (in milliseconds) `touch()` will update a session's @@ -30,10 +33,79 @@ class SessionStore extends expressSession.Store { // equal to `db`. // - `timeout`: Timeout ID for a timeout that will clean up the database record. this._expirations = new Map(); + this._cleanupTimer = null; + this._cleanupRunning = false; + } + + /** + * Start periodic cleanup of expired/stale sessions from the database. + * Uses chained setTimeout (not setInterval) to prevent overlapping runs. + */ + startCleanup() { + this._scheduleCleanup(5000); // First run 5s after startup. + } + + _scheduleCleanup(delay: number) { + this._cleanupTimer = setTimeout(async () => { + try { + await this._cleanup(); + } catch (err) { + logger.error('Session cleanup error:', err); + } + // Schedule the next run only after this one completes. + this._scheduleCleanup(CLEANUP_INTERVAL_MS); + }, delay); + // Don't prevent Node.js from exiting. + if (this._cleanupTimer.unref) this._cleanupTimer.unref(); } shutdown() { for (const {timeout} of this._expirations.values()) clearTimeout(timeout); + if (this._cleanupTimer) { + clearTimeout(this._cleanupTimer); + this._cleanupTimer = null; + } + } + + /** + * Remove expired and empty sessions from the database. + * + * - Sessions with an `expires` date in the past are removed (expired). + * - Sessions with no expiry that contain no data beyond the default cookie are removed. + * These are the empty sessions that accumulate indefinitely (bug #5010) — they have + * `{cookie: {path: "/", _expires: null, ...}}` and nothing else. + */ + async _cleanup() { + const keys = await DB.findKeys('sessionstorage:*', null); + if (!keys || keys.length === 0) return; + const now = Date.now(); + let removed = 0; + for (const key of keys) { + const sess = await DB.get(key); + if (!sess) { + await DB.remove(key); + removed++; + continue; + } + const expires = sess.cookie?.expires; + if (expires) { + // Session has an expiry — remove if expired. + if (new Date(expires).getTime() <= now) { + await DB.remove(key); + removed++; + } + } else { + // Session has no expiry and no user data beyond the cookie — remove as empty/stale. + const hasData = Object.keys(sess).some((k) => k !== 'cookie'); + if (!hasData) { + await DB.remove(key); + removed++; + } + } + } + if (removed > 0) { + logger.info(`Session cleanup: removed ${removed} expired/stale sessions out of ${keys.length}`); + } } async _updateExpirations(sid: string, sess: any, updateDbExp = true) { diff --git a/src/node/hooks/express.ts b/src/node/hooks/express.ts index 554ff6a56..ac2b90305 100644 --- a/src/node/hooks/express.ts +++ b/src/node/hooks/express.ts @@ -200,7 +200,11 @@ exports.restartServer = async () => { app.use(cookieParser(secret, {})); - sessionStore = new SessionStore(settings.cookie.sessionRefreshInterval); + const store = new SessionStore(settings.cookie.sessionRefreshInterval); + if (settings.cookie.sessionCleanup !== false) { + store.startCleanup(); + } + sessionStore = store; exports.sessionMiddleware = expressSession({ rolling: true, secret, diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index 1124fdebb..efefff9e6 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -255,6 +255,7 @@ export type SettingsType = { prefix: string, sameSite: boolean | "lax" | "strict" | "none" | undefined, sessionLifetime: number, + sessionCleanup: boolean, sessionRefreshInterval: number, }, requireAuthentication: boolean, @@ -534,6 +535,7 @@ const settings: SettingsType = { prefix: '', sameSite: 'lax', sessionLifetime: 10 * 24 * 60 * 60 * 1000, + sessionCleanup: true, sessionRefreshInterval: 1 * 24 * 60 * 60 * 1000, }, /* diff --git a/src/tests/backend/specs/SessionStore.ts b/src/tests/backend/specs/SessionStore.ts index 5dfc44ff2..9f5266dc2 100644 --- a/src/tests/backend/specs/SessionStore.ts +++ b/src/tests/backend/specs/SessionStore.ts @@ -12,6 +12,9 @@ type Session = { destroy: (sid:string|null) => void; touch: (sid:string|null, sess:any, sess2:any) => void; shutdown: () => void; + startCleanup: () => void; + _cleanup: () => Promise; + _cleanupTimer: any; } describe(__filename, function () { @@ -243,4 +246,55 @@ describe(__filename, function () { assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess)); }); }); + + // Regression tests for https://github.com/ether/etherpad-lite/issues/5010 + describe('cleanup', function () { + it('removes expired sessions', async function () { + const expiredSid = `cleanup_expired_${common.randomString()}`; + await db.set(`sessionstorage:${expiredSid}`, { + cookie: {path: '/', expires: new Date(1).toJSON(), httpOnly: true}, + }); + await ss!._cleanup(); + assert(await db.get(`sessionstorage:${expiredSid}`) == null); + }); + + it('removes empty sessions with no expiry', async function () { + const emptySid = `cleanup_empty_${common.randomString()}`; + await db.set(`sessionstorage:${emptySid}`, { + cookie: {path: '/', _expires: null, originalMaxAge: null, httpOnly: true}, + }); + await ss!._cleanup(); + assert(await db.get(`sessionstorage:${emptySid}`) == null); + }); + + it('preserves sessions with user data and no expiry', async function () { + const dataSid = `cleanup_data_${common.randomString()}`; + const sess = { + cookie: {path: '/', _expires: null, httpOnly: true}, + user: {name: 'test'}, + }; + await db.set(`sessionstorage:${dataSid}`, sess); + await ss!._cleanup(); + assert.equal(JSON.stringify(await db.get(`sessionstorage:${dataSid}`)), JSON.stringify(sess)); + await db.remove(`sessionstorage:${dataSid}`); + }); + + it('preserves non-expired sessions', async function () { + const validSid = `cleanup_valid_${common.randomString()}`; + const sess = { + cookie: {path: '/', expires: new Date(Date.now() + 60000).toJSON(), httpOnly: true}, + }; + await db.set(`sessionstorage:${validSid}`, sess); + await ss!._cleanup(); + assert.equal(JSON.stringify(await db.get(`sessionstorage:${validSid}`)), JSON.stringify(sess)); + await db.remove(`sessionstorage:${validSid}`); + }); + + it('shutdown cancels pending cleanup timer', async function () { + ss!.startCleanup(); + ss!.shutdown(); + // After shutdown, the timer should be cleared. + assert(ss!._cleanupTimer == null); + }); + }); });