diff --git a/CHANGELOG.md b/CHANGELOG.md index ad489422..79f741d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,17 @@ ### Notable enhancements and fixes +* Improvements to login session management: + * `sessionstorage:*` database records are automatically deleted when the login + session expires (with some exceptions that will be fixed in the future). + * Requests for static content (e.g., `/robots.txt`) and special pages (e.g., + the HTTP API, `/stats`) no longer create login session state. * The following settings from `settings.json` are now applied as expected (they were unintentionally ignored before): * `padOptions.lang` * `padOptions.showChat` * `padOptions.userColor` * `padOptions.userName` -* Requests for static content (e.g., `/robots.txt`) and special pages (e.g., the - HTTP API, `/stats`) no longer cause the server to generate database records - intended to manage browser sessions (`sessionstorage:*`). * Fixed the return value of the `getText` HTTP API when called with a specific revision. * Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`. diff --git a/src/node/db/SessionStore.js b/src/node/db/SessionStore.js index 195233f2..40e5e90d 100644 --- a/src/node/db/SessionStore.js +++ b/src/node/db/SessionStore.js @@ -26,11 +26,17 @@ class SessionStore extends Store { // - `db`: Session expiration as recorded in the database (ms since epoch, not a Date). // - `real`: Actual session expiration (ms since epoch, not a Date). Always greater than or // equal to `db`. + // - `timeout`: Timeout ID for a timeout that will clean up the database record. this._expirations = new Map(); } + shutdown() { + for (const {timeout} of this._expirations.values()) clearTimeout(timeout); + } + async _updateExpirations(sid, sess, updateDbExp = true) { const exp = this._expirations.get(sid) || {}; + clearTimeout(exp.timeout); const {cookie: {expires} = {}} = sess || {}; if (expires) { const sessExp = new Date(expires).getTime(); @@ -41,6 +47,15 @@ class SessionStore extends Store { // If reading from the database, update the expiration with the latest value from touch() so // that touch() appears to write to the database every time even though it doesn't. if (typeof expires === 'string') sess.cookie.expires = new Date(exp.real).toJSON(); + // Use this._get(), not this._destroy(), to destroy the DB record for the expired session. + // This is done in case multiple Etherpad instances are sharing the same database and users + // are bouncing between the instances. By using this._get(), this instance will query the DB + // for the latest expiration time written by any of the instances, ensuring that the record + // isn't prematurely deleted if the expiration time was updated by a different Etherpad + // instance. (Important caveat: Client-side database caching, which ueberdb does by default, + // could still cause the record to be prematurely deleted because this instance might get a + // stale expiration time from cache.) + exp.timeout = setTimeout(() => this._get(sid), exp.real - now); this._expirations.set(sid, exp); } else { this._expirations.delete(sid); @@ -66,6 +81,7 @@ class SessionStore extends Store { async _destroy(sid) { logger.debug(`DESTROY ${sid}`); + clearTimeout((this._expirations.get(sid) || {}).timeout); this._expirations.delete(sid); await DB.remove(`sessionstorage:${sid}`); } diff --git a/src/node/hooks/express.js b/src/node/hooks/express.js index 179d37a8..fa68443a 100644 --- a/src/node/hooks/express.js +++ b/src/node/hooks/express.js @@ -16,6 +16,7 @@ const webaccess = require('./express/webaccess'); const logger = log4js.getLogger('http'); let serverName; +let sessionStore; const sockets = new Set(); const socketsEvents = new events.EventEmitter(); const startTime = stats.settableGauge('httpStartTime'); @@ -23,32 +24,35 @@ const startTime = stats.settableGauge('httpStartTime'); exports.server = null; const closeServer = async () => { - if (exports.server == null) return; - logger.info('Closing HTTP server...'); - // Call exports.server.close() to reject new connections but don't await just yet because the - // Promise won't resolve until all preexisting connections are closed. - const p = util.promisify(exports.server.close.bind(exports.server))(); - await hooks.aCallAll('expressCloseServer'); - // Give existing connections some time to close on their own before forcibly terminating. The time - // should be long enough to avoid interrupting most preexisting transmissions but short enough to - // avoid a noticeable outage. - const timeout = setTimeout(async () => { - logger.info(`Forcibly terminating remaining ${sockets.size} HTTP connections...`); - for (const socket of sockets) socket.destroy(new Error('HTTP server is closing')); - }, 5000); - let lastLogged = 0; - while (sockets.size > 0) { - if (Date.now() - lastLogged > 1000) { // Rate limit to avoid filling logs. - logger.info(`Waiting for ${sockets.size} HTTP clients to disconnect...`); - lastLogged = Date.now(); + if (exports.server != null) { + logger.info('Closing HTTP server...'); + // Call exports.server.close() to reject new connections but don't await just yet because the + // Promise won't resolve until all preexisting connections are closed. + const p = util.promisify(exports.server.close.bind(exports.server))(); + await hooks.aCallAll('expressCloseServer'); + // Give existing connections some time to close on their own before forcibly terminating. The + // time should be long enough to avoid interrupting most preexisting transmissions but short + // enough to avoid a noticeable outage. + const timeout = setTimeout(async () => { + logger.info(`Forcibly terminating remaining ${sockets.size} HTTP connections...`); + for (const socket of sockets) socket.destroy(new Error('HTTP server is closing')); + }, 5000); + let lastLogged = 0; + while (sockets.size > 0) { + if (Date.now() - lastLogged > 1000) { // Rate limit to avoid filling logs. + logger.info(`Waiting for ${sockets.size} HTTP clients to disconnect...`); + lastLogged = Date.now(); + } + await events.once(socketsEvents, 'updated'); } - await events.once(socketsEvents, 'updated'); + await p; + clearTimeout(timeout); + exports.server = null; + startTime.setValue(0); + logger.info('HTTP server closed'); } - await p; - clearTimeout(timeout); - exports.server = null; - startTime.setValue(0); - logger.info('HTTP server closed'); + if (sessionStore) sessionStore.shutdown(); + sessionStore = null; }; exports.createServer = async () => { @@ -172,9 +176,10 @@ exports.restartServer = async () => { app.use(cookieParser(settings.sessionKey, {})); + sessionStore = new SessionStore(); exports.sessionMiddleware = expressSession({ secret: settings.sessionKey, - store: new SessionStore(), + store: sessionStore, resave: false, saveUninitialized: true, // Set the cookie name to a javascript identifier compatible string. Makes code handling it diff --git a/src/tests/backend/specs/SessionStore.js b/src/tests/backend/specs/SessionStore.js index 26460c9d..dbf79c10 100644 --- a/src/tests/backend/specs/SessionStore.js +++ b/src/tests/backend/specs/SessionStore.js @@ -25,7 +25,10 @@ describe(__filename, function () { }); afterEach(async function () { - if (ss != null && sid != null) await destroy(); + if (ss != null) { + if (sid != null) await destroy(); + ss.shutdown(); + } sid = null; ss = null; }); @@ -46,6 +49,9 @@ describe(__filename, function () { const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}}; await set(sess); assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess)); + await new Promise((resolve) => setTimeout(resolve, 110)); + // Writing should start a timeout. + assert(await db.get(`sessionstorage:${sid}`) == null); }); it('set of already expired session', async function () { @@ -54,6 +60,24 @@ describe(__filename, function () { // No record should have been created. assert(await db.get(`sessionstorage:${sid}`) == null); }); + + it('switch from non-expiring to expiring', async function () { + const sess = {foo: 'bar'}; + await set(sess); + const sess2 = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}}; + await set(sess2); + await new Promise((resolve) => setTimeout(resolve, 110)); + assert(await db.get(`sessionstorage:${sid}`) == null); + }); + + it('switch from expiring to non-expiring', async function () { + const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}}; + await set(sess); + const sess2 = {foo: 'bar'}; + await set(sess2); + await new Promise((resolve) => setTimeout(resolve, 110)); + assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess2)); + }); }); describe('get', function () { @@ -77,6 +101,9 @@ describe(__filename, function () { const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}}; await db.set(`sessionstorage:${sid}`, sess); assert.equal(JSON.stringify(await get()), JSON.stringify(sess)); + await new Promise((resolve) => setTimeout(resolve, 110)); + // Reading should start a timeout. + assert(await db.get(`sessionstorage:${sid}`) == null); }); it('get of record from previous run (already expired)', async function () { @@ -99,6 +126,18 @@ describe(__filename, function () { }); }); + describe('shutdown', function () { + it('shutdown cancels timeouts', async function () { + const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}}; + await set(sess); + assert.equal(JSON.stringify(await get()), JSON.stringify(sess)); + ss.shutdown(); + await new Promise((resolve) => setTimeout(resolve, 110)); + // The record should not have been automatically purged. + assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess)); + }); + }); + describe('destroy', function () { it('destroy deletes the database record', async function () { const sess = {cookie: {expires: new Date(Date.now() + 100)}};