From 8ffaa5b0ca314b0031a639ac03d7da312517e402 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 22 Oct 2019 08:29:19 +0100 Subject: [PATCH] Merge pull request #2271 from overleaf/bg-reject-invalid-sessions reject invalid sessions with middleware GitOrigin-RevId: 07ab8829cbed92bbcb90b2c5f2c9d049e05b77cd --- .../AuthenticationController.js | 4 -- services/web/app/src/infrastructure/Server.js | 2 + .../src/infrastructure/SessionStoreManager.js | 63 +++++++++++-------- .../AuthenticationControllerTests.js | 29 --------- .../src/Security/SessionStoreManagerTests.js | 37 ++++++++--- 5 files changed, 66 insertions(+), 69 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index ece73affe7..8d0eda8eb6 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -8,7 +8,6 @@ const Settings = require('settings-sharelatex') const basicAuth = require('basic-auth-connect') 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') @@ -255,9 +254,6 @@ const AuthenticationController = (module.exports = { }, getSessionUser(req) { - if (!SessionStoreManager.checkValidationToken(req)) { - return null - } const sessionUser = _.get(req, ['session', 'user']) const sessionPassportUser = _.get(req, ['session', 'passport', 'user']) return sessionUser || sessionPassportUser || null diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 9960ad4027..ce41124ee0 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -114,6 +114,8 @@ webRouter.use( // 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()) diff --git a/services/web/app/src/infrastructure/SessionStoreManager.js b/services/web/app/src/infrastructure/SessionStoreManager.js index 72054b0375..17788a2bc0 100644 --- a/services/web/app/src/infrastructure/SessionStoreManager.js +++ b/services/web/app/src/infrastructure/SessionStoreManager.js @@ -7,6 +7,35 @@ function computeValidationToken(req) { 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: sessionToken, + clientToken: clientToken + }, + 'session token validation failed' + ) + Metrics.inc('security.session', 1, { status: 'error' }) + return false + } + } else { + Metrics.inc('security.session', 1, { status: 'missing' }) + } + } + return true // fallback to allowing session +} + module.exports = { enableValidationToken(sessionStore) { // generate an identifier from the sessionID for every new session @@ -23,32 +52,14 @@ module.exports = { } }, - 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: sessionToken, - clientToken: clientToken - }, - 'session token validation failed' - ) - Metrics.inc('security.session', 1, { status: 'error' }) - return false - } - } else { - Metrics.inc('security.session', 1, { status: 'missing' }) - } + validationMiddleware(req, res, next) { + if (!checkValidationToken(req)) { + // the session must exist for it to fail validation + req.session.destroy(() => { + return next(new Error('invalid session')) + }) + } else { + return next() } - return true // fallback to allowing session } } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index e3da9cf62b..0d1d4d45ef 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -48,9 +48,6 @@ describe('AuthenticationController', function() { untrackSession: sinon.stub(), revokeAllUserSessions: sinon.stub().callsArgWith(1, null) }), - '../../infrastructure/SessionStoreManager': (this.SessionStoreManager = { - checkValidationToken: sinon.stub().returns(true) - }), '../../infrastructure/Modules': (this.Modules = { hooks: { fire: sinon.stub().callsArgWith(2, null, []) } }), @@ -322,32 +319,6 @@ describe('AuthenticationController', function() { }) }) - describe('getSessionUser', function() { - it('should accept a valid session', function() { - this.req.session = { - passport: { - user: { _id: 'one' } - } - } - this.SessionStoreManager.checkValidationToken = sinon.stub().returns(true) - const user = this.AuthenticationController.getSessionUser(this.req) - expect(user).to.deep.equal({ _id: 'one' }) - }) - - it('should reject an invalid session', function() { - this.req.session = { - passport: { - user: { _id: 'two' } - } - } - this.SessionStoreManager.checkValidationToken = sinon - .stub() - .returns(false) - const user = this.AuthenticationController.getSessionUser(this.req) - expect(user).to.be.null - }) - }) - describe('doPassportLogin', function() { beforeEach(function() { this.AuthenticationController._recordFailedLogin = sinon.stub() diff --git a/services/web/test/unit/src/Security/SessionStoreManagerTests.js b/services/web/test/unit/src/Security/SessionStoreManagerTests.js index e5e1b1468b..6d2065b116 100644 --- a/services/web/test/unit/src/Security/SessionStoreManagerTests.js +++ b/services/web/test/unit/src/Security/SessionStoreManagerTests.js @@ -23,6 +23,7 @@ describe('SessionStoreManager', function() { this.sessionStore = { generate: sinon.spy(req => { req.session = {} + req.session.destroy = sinon.stub().yields() }) } }) @@ -46,25 +47,41 @@ describe('SessionStoreManager', function() { expect(this.req.session.validationToken).to.equal('v1:6789') }) }) - describe('checkValidationToken', function() { + 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 return true when the session id matches the validation token', function() { - const result = this.SessionStoreManager.checkValidationToken(this.req) - expect(result).to.equal(true) + 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 return false when the session id has changed', function() { + it('should destroy the session and return an error when the session id does not match the validation token', function() { this.req.sessionID = 'abcdefghijklmnopqrstuvwxyz' - const result = this.SessionStoreManager.checkValidationToken(this.req) - expect(result).to.equal(false) + 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(new Error('invalid session')) }) - it('should return true when the session does not have a validation token', function() { + it('should accept the request when the session does not have a validation token', function() { this.req = { sessionID: '123456789', session: {} } - const result = this.SessionStoreManager.checkValidationToken(this.req) - expect(result).to.equal(true) + this.next = sinon.stub() + this.SessionStoreManager.validationMiddleware( + this.req, + this.res, + this.next + ) + expect(this.next).to.be.calledWithExactly() }) }) })