Merge pull request #2271 from overleaf/bg-reject-invalid-sessions

reject invalid sessions with middleware

GitOrigin-RevId: 07ab8829cbed92bbcb90b2c5f2c9d049e05b77cd

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

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())

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) {
+  validationMiddleware(req, res, next) {
-    if (req.session) {
+    if (!checkValidationToken(req)) {
-      const sessionToken = req.session.validationToken
+      // the session must exist for it to fail validation
-      if (sessionToken) {
+      req.session.destroy(() => {
-        const clientToken = computeValidationToken(req)
+        return next(new Error('invalid session'))
-        // Reject invalid sessions. If you change the method for computing the
+      })
-        // token (above) then you need to either check or ignore previous
+    } else {
-        // versions of the token.
+      return next()
-        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
   }
 }

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()

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() {
+    it('should accept the request when the session id matches the validation token', function() {
-      const result = this.SessionStoreManager.checkValidationToken(this.req)
+      this.SessionStoreManager.validationMiddleware(
-      expect(result).to.equal(true)
+        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)
+      this.next = sinon.stub()
-      expect(result).to.equal(false)
+      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)
+      this.next = sinon.stub()
-      expect(result).to.equal(true)
+      this.SessionStoreManager.validationMiddleware(
+        this.req,
+        this.res,
+        this.next
+      )
+      expect(this.next).to.be.calledWithExactly()
     })
   })
 })