diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index a9446718bc..af24dfec28 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -11,7 +11,6 @@ const basicAuth = require('basic-auth') const tsscmp = require('tsscmp') const UserHandler = require('../User/UserHandler') const UserSessionsManager = require('../User/UserSessionsManager') -const SessionStoreManager = require('../../infrastructure/SessionStoreManager') const Analytics = require('../Analytics/AnalyticsManager') const passport = require('passport') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') @@ -409,31 +408,6 @@ const AuthenticationController = { return expressify(middleware) }, - validateUserSession: function () { - // Middleware to check that the user's session is still good on key actions, - // such as opening a a project. Could be used to check that session has not - // exceeded a maximum lifetime (req.session.session_created), or for session - // hijacking checks (e.g. change of ip address, req.session.ip_address). For - // now, just check that the session has been loaded from the session store - // correctly. - return function (req, res, next) { - // check that the session store is returning valid results - if (req.session && !SessionStoreManager.hasValidationToken(req)) { - // Force user to update session by destroying the current one. - // A new session will be created on the next request. - req.session.destroy(() => { - // need to destroy the existing session and generate a new one - // otherwise they will already be logged in when they are redirected - // to the login page - if (acceptsJson(req)) return send401WithChallenge(res) - AuthenticationController._redirectToLoginOrRegisterPage(req, res) - }) - } else { - next() - } - } - }, - _globalLoginWhitelist: [], addEndpointToLoginWhitelist(endpoint) { return AuthenticationController._globalLoginWhitelist.push(endpoint) diff --git a/services/web/app/src/infrastructure/CustomSessionStore.js b/services/web/app/src/infrastructure/CustomSessionStore.js index ee8623ac0e..5c8110573e 100644 --- a/services/web/app/src/infrastructure/CustomSessionStore.js +++ b/services/web/app/src/infrastructure/CustomSessionStore.js @@ -4,6 +4,7 @@ const metrics = require('@overleaf/metrics') const logger = require('@overleaf/logger') const Settings = require('@overleaf/settings') const SessionManager = require('../Features/Authentication/SessionManager') +const Metrics = require('@overleaf/metrics') const MAX_SESSION_SIZE_THRESHOLD = 4096 @@ -55,22 +56,30 @@ class CustomSessionStore extends RedisStore { } } - // Override the get, set, touch, and destroy methods to record metrics get(sid, cb) { - super.get(sid, (err, ...args) => { - if (args[0]) { - CustomSessionStore.metric('get', args[0]) - } - cb(err, ...args) + super.get(sid, (err, sess) => { + if (err || !sess || !checkValidationToken(sid, sess)) return cb(err, null) + CustomSessionStore.metric('get', sess) + cb(null, sess) }) } set(sid, sess, cb) { + // Refresh the validation token just before writing to Redis + // This will ensure that the token is always matching to the sessionID that we write the session value for. + // Potential reasons for missing/mismatching token: + // - brand-new session + // - cycling of the sessionID as part of the login flow + // - upgrade from a client side session to a redis session + // - accidental writes in the app code + sess.validationToken = computeValidationToken(sid) + CustomSessionStore.metric('set', sess) const originalId = sess.req.signedCookies[Settings.cookieName] if (sid === originalId || sid === sess.req.newSessionId) { this.#updateInPlaceStore.set(sid, sess, cb) } else { + Metrics.inc('security.session', 1, { status: 'new' }) // Multiple writes can get issued with the new sid. Keep track of it. Object.defineProperty(sess.req, 'newSessionId', { value: sid }) this.#initialSetStore.set(sid, sess, cb) @@ -89,6 +98,35 @@ class CustomSessionStore extends RedisStore { } } +function computeValidationToken(sid) { + // This should be a deterministic function of the client-side sessionID, + // prepended with a version number in case we want to change it later. + return 'v1:' + sid.slice(-4) +} + +function checkValidationToken(sid, sess) { + const sessionToken = sess.validationToken + if (sessionToken) { + const clientToken = computeValidationToken(sid) + // Reject sessions where the validation token is out of sync with the sessionID. + // If you change the method for computing the token (above) then you need to either check or ignore previous versions of the token. + if (sessionToken === clientToken) { + Metrics.inc('security.session', 1, { status: 'ok' }) + return true + } else { + logger.warn( + { sid, sessionToken, clientToken }, + 'session token validation failed' + ) + Metrics.inc('security.session', 1, { status: 'error' }) + return false + } + } else { + Metrics.inc('security.session', 1, { status: 'missing' }) + return false + } +} + // Helper function to return a redacted version of session object // so we can identify the largest keys without exposing sensitive // data diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 067db600a6..843bbed3a0 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -14,7 +14,6 @@ const HttpPermissionsPolicyMiddleware = require('./HttpPermissionsPolicy') const sessionsRedisClient = UserSessionsRedis.client() const SessionAutostartMiddleware = require('./SessionAutostartMiddleware') -const SessionStoreManager = require('./SessionStoreManager') const AnalyticsManager = require('../Features/Analytics/AnalyticsManager') const session = require('express-session') const CustomSessionStore = require('./CustomSessionStore') @@ -181,11 +180,6 @@ if (Features.hasFeature('saas')) { webRouter.use(AnalyticsManager.analyticsIdMiddleware) } -// patch the session store to generate a validation token for every new session -SessionStoreManager.enableValidationToken(sessionStore) -// use middleware to reject all requests with invalid tokens -webRouter.use(SessionStoreManager.validationMiddleware) - // passport webRouter.use(passport.initialize()) webRouter.use(passport.session()) diff --git a/services/web/app/src/infrastructure/SessionStoreManager.js b/services/web/app/src/infrastructure/SessionStoreManager.js deleted file mode 100644 index 48487ec1ab..0000000000 --- a/services/web/app/src/infrastructure/SessionStoreManager.js +++ /dev/null @@ -1,86 +0,0 @@ -const Metrics = require('@overleaf/metrics') -const logger = require('@overleaf/logger') - -function computeValidationToken(req) { - // this should be a deterministic function of the client-side sessionID, - // prepended with a version number in case we want to change it later - return 'v1:' + req.sessionID.slice(-4) -} - -function checkValidationToken(req) { - if (req.session) { - const sessionToken = req.session.validationToken - if (sessionToken) { - const clientToken = computeValidationToken(req) - // Reject invalid sessions. If you change the method for computing the - // token (above) then you need to either check or ignore previous - // versions of the token. - if (sessionToken === clientToken) { - Metrics.inc('security.session', 1, { status: 'ok' }) - return true - } else { - logger.error( - { - sessionToken, - clientToken, - }, - 'session token validation failed' - ) - Metrics.inc('security.session', 1, { status: 'error' }) - return false - } - } else { - Metrics.inc('security.session', 1, { status: 'missing' }) - return false - } - } -} - -module.exports = { - enableValidationToken(sessionStore) { - // generate an identifier from the sessionID for every new session - const originalGenerate = sessionStore.generate - sessionStore.generate = function (req) { - originalGenerate(req) - // add the validation token as a property that cannot be overwritten - Object.defineProperty(req.session, 'validationToken', { - value: computeValidationToken(req), - enumerable: true, - writable: false, - }) - Metrics.inc('security.session', 1, { status: 'new' }) - } - }, - - validationMiddleware(req, res, next) { - if ( - !req.session.noSessionCallback && - req.session.constructor.name !== 'ClientSession' - ) { - if (!checkValidationToken(req)) { - // the session must exist for it to fail validation - return req.session.destroy(() => { - return next(new Error('invalid session')) - }) - } - // add the validation token as a property that cannot be overwritten - Object.defineProperty(req.session, 'validationToken', { - value: req.session.validationToken, - enumerable: true, - writable: false, - }) - } - next() - }, - - hasValidationToken(req) { - if (req && req.session && req.session.validationToken) { - return true - } else { - return false - } - }, - - computeValidationToken, - checkValidationToken, -} diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 8de376a984..484b44e644 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -513,7 +513,6 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { RateLimiterMiddleware.rateLimit(openProjectRateLimiter, { params: ['Project_id'], }), - AuthenticationController.validateUserSession(), AuthorizationMiddleware.ensureUserCanReadProject, ProjectController.loadEditor ) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 0d47c30f1f..3d149e8738 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -92,8 +92,6 @@ describe('AuthenticationController', function () { identifyUser: sinon.stub(), getIdsFromSession: sinon.stub().returns({ userId: this.user._id }), }), - '../../infrastructure/SessionStoreManager': (this.SessionStoreManager = - {}), '@overleaf/settings': (this.Settings = { siteUrl: 'http://www.foo.bar', httpAuthUsers: this.httpAuthUsers, @@ -599,55 +597,6 @@ describe('AuthenticationController', function () { }) }) - describe('validateUserSession', function () { - beforeEach(function () { - this.user = { - _id: 'user-id-123', - email: 'user@overleaf.com', - } - this.middleware = this.AuthenticationController.validateUserSession() - }) - - describe('when the user has a session token', function () { - beforeEach(function () { - this.req.user = this.user - this.SessionStoreManager.hasValidationToken = sinon.stub().returns(true) - this.middleware(this.req, this.res, this.next) - }) - - it('should call the next method in the chain', function () { - this.next.called.should.equal(true) - }) - }) - - describe('when the user does not have a session token', function () { - beforeEach(function () { - this.req.session = { - user: this.user, - destroy: sinon.stub().yields(), - } - this.req.user = this.user - this.AuthenticationController._redirectToLoginOrRegisterPage = - sinon.stub() - this.req.query = {} - this.SessionStoreManager.hasValidationToken = sinon - .stub() - .returns(false) - this.middleware(this.req, this.res, this.next) - }) - - it('should destroy the current session', function () { - this.req.session.destroy.called.should.equal(true) - }) - - it('should redirect to the register or login page', function () { - this.AuthenticationController._redirectToLoginOrRegisterPage - .calledWith(this.req, this.res) - .should.equal(true) - }) - }) - }) - describe('requireOauth', function () { beforeEach(function () { this.res.json = sinon.stub() diff --git a/services/web/test/unit/src/Security/SessionStoreManagerTests.js b/services/web/test/unit/src/Security/SessionStoreManagerTests.js deleted file mode 100644 index c7672ec86c..0000000000 --- a/services/web/test/unit/src/Security/SessionStoreManagerTests.js +++ /dev/null @@ -1,119 +0,0 @@ -const sinon = require('sinon') -const { expect } = require('chai') -const modulePath = '../../../../app/src/infrastructure/SessionStoreManager.js' -const SandboxedModule = require('sandboxed-module') - -describe('SessionStoreManager', function () { - beforeEach(function () { - this.SessionStoreManager = SandboxedModule.require(modulePath, { - requires: { - '@overleaf/metrics': (this.Metrics = { inc: sinon.stub() }), - }, - }) - this.sessionStore = { - generate: sinon.spy(req => { - req.session = {} - req.session.destroy = sinon.stub().yields() - }), - } - }) - describe('enableValidationToken', function () { - beforeEach(function () { - this.originalGenerate = this.sessionStore.generate - this.SessionStoreManager.enableValidationToken(this.sessionStore) - }) - it('should set up a wrapper around the generate function', function () { - expect(this.sessionStore.generate).to.not.equal(this.originalGenerate) - }) - it('should add a validationToken when the generate function is called', function () { - this.req = { sessionID: '123456789' } - this.sessionStore.generate(this.req) - expect(this.req.session.validationToken).to.equal('v1:6789') - }) - it('should not allow the token to be overwritten', function () { - this.req = { sessionID: '123456789' } - this.sessionStore.generate(this.req) - this.req.session.validationToken = 'try-to-overwrite-token' - expect(this.req.session.validationToken).to.equal('v1:6789') - }) - }) - describe('validationMiddleware', function () { - this.beforeEach(function () { - this.SessionStoreManager.enableValidationToken(this.sessionStore) - this.req = { sessionID: '123456789' } - this.next = sinon.stub() - this.sessionStore.generate(this.req) - }) - it('should accept the request when the session id matches the validation token', function () { - this.SessionStoreManager.validationMiddleware( - this.req, - this.res, - this.next - ) - expect(this.next).to.be.calledWithExactly() - }) - it('should not allow the token to be overwritten', function () { - this.req = { - sessionID: '123456789', - session: { validationToken: 'v1:6789' }, - } - this.SessionStoreManager.validationMiddleware( - this.req, - this.res, - this.next - ) - this.req.session.validationToken = 'try-to-overwrite-token' - expect(this.req.session.validationToken).to.equal('v1:6789') - }) - it('should destroy the session and return an error when the session id does not match the validation token', function () { - this.req.sessionID = 'abcdefghijklmnopqrstuvwxyz' - this.next = sinon.stub() - this.SessionStoreManager.validationMiddleware( - this.req, - this.res, - this.next - ) - expect(this.req.session.destroy).to.be.called - expect(this.next).to.be.calledWithExactly( - sinon.match - .instanceOf(Error) - .and(sinon.match.has('message', 'invalid session')) - ) - }) - it('should destroy the request when the session does not have a validation token', function () { - this.req.session = { destroy: sinon.stub().yields() } - this.next = sinon.stub() - this.SessionStoreManager.validationMiddleware( - this.req, - this.res, - this.next - ) - expect(this.req.session.destroy).to.be.called - expect(this.next).to.be.calledWithExactly( - sinon.match - .instanceOf(Error) - .and(sinon.match.has('message', 'invalid session')) - ) - }) - }) - describe('hasValidationToken', function () { - this.beforeEach(function () { - this.req = { sessionID: '123456789' } - }) - it('should return true when the session is valid', function () { - this.req.session = { validationToken: 'v1:6789' } - const result = this.SessionStoreManager.hasValidationToken(this.req) - expect(result).to.equal(true) - }) - it('should return false when the session is valid', function () { - this.req.session = { validationToken: 'v1:abcd' } - const result = this.SessionStoreManager.hasValidationToken(this.req) - expect(result).to.equal(true) - }) - it('should return false when the validation token is missing', function () { - this.req.session = {} - const result = this.SessionStoreManager.hasValidationToken(this.req) - expect(result).to.equal(false) - }) - }) -})