Merge pull request #2249 from overleaf/bg-create-session-validation-token-for-all-sessions

move session validation into SessionStoreManager

GitOrigin-RevId: 438ec738f0ccaf9f072629d6203a334cb8439b83

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -8,6 +8,7 @@ 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')
@@ -42,51 +43,6 @@ const AuthenticationController = (module.exports = {
     cb(null, user)
   },
 
-  computeSessionValidationToken(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)
-  },
-
-  setSessionValidationToken(req) {
-    req.session.validationToken = AuthenticationController.computeSessionValidationToken(
-      req
-    )
-  },
-
-  checkSessionValidationToken(req) {
-    if (req.session) {
-      const token = req.session.validationToken
-      if (token) {
-        const clientToken = AuthenticationController.computeSessionValidationToken(
-          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 (token === clientToken) {
-          Metrics.inc('security.session', 1, { status: 'ok' })
-          return true
-        } else {
-          logger.error(
-            {
-              token: token,
-              clientToken: clientToken,
-              req: req,
-              sessionID: req.sessionID
-            },
-            '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
-  },
-
   afterLoginSessionSetup(req, user, callback) {
     if (callback == null) {
       callback = function() {}
@@ -108,14 +64,14 @@ const AuthenticationController = (module.exports = {
           return callback(err)
         }
         req.sessionStore.generate(req)
+        // Note: the validation token is not writable, so it does not get
+        // transferred to the new session below.
         for (let key in oldSession) {
           const value = oldSession[key]
           if (key !== '__tmp') {
             req.session[key] = value
           }
         }
-        // generate an identifier from the sessionID
-        AuthenticationController.setSessionValidationToken(req)
         req.session.save(function(err) {
           if (err) {
             logger.warn(
@@ -299,7 +255,7 @@ const AuthenticationController = (module.exports = {
   },
 
   getSessionUser(req) {
-    if (!AuthenticationController.checkSessionValidationToken(req)) {
+    if (!SessionStoreManager.checkValidationToken(req)) {
       return null
     }
     const sessionUser = _.get(req, ['session', 'user'])

services/web/app/src/infrastructure/Server.js

@@ -13,6 +13,7 @@ const Csrf = require('./Csrf')
 
 const sessionsRedisClient = UserSessionsRedis.client()
 
+const SessionStoreManager = require('./SessionStoreManager')
 const session = require('express-session')
 const RedisStore = require('connect-redis')(session)
 const bodyParser = require('body-parser')
@@ -111,6 +112,9 @@ webRouter.use(
   })
 )
 
+// patch the session store to generate a validation token for every new session
+SessionStoreManager.enableValidationToken(sessionStore)
+
 // passport
 webRouter.use(passport.initialize())
 webRouter.use(passport.session())

services/web/app/src/infrastructure/SessionStoreManager.js

@@ -0,0 +1,54 @@
+const Metrics = require('metrics-sharelatex')
+const logger = require('logger-sharelatex')
+
+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)
+}
+
+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' })
+    }
+  },
+
+  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
+  }
+}

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -48,6 +48,9 @@ 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, []) }
         }),
@@ -296,16 +299,6 @@ describe('AuthenticationController', function() {
       })
     })
 
-    it('should set a vaildation token on the session', function(done) {
-      this.call(err => {
-        if (err) {
-          return done(err)
-        }
-        expect(this.req.session).to.have.property('validationToken', 'v1:omid')
-        done()
-      })
-    })
-
     describe('when req.session.save produces an error', function() {
       beforeEach(function() {
         this.req.session.save = sinon.stub().callsArgWith(0, new Error('woops'))
@@ -330,45 +323,29 @@ describe('AuthenticationController', function() {
   })
 
   describe('getSessionUser', function() {
-    it('should accept a session without a validation token', 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 validation token', function() {
-      this.req.sessionID = 'nabuchodonosorroidebabylone'
+    it('should reject an invalid session', function() {
       this.req.session = {
-        validationToken: 'v1:gasp', // does not match last 4 characters of session id
         passport: {
           user: { _id: 'two' }
         }
       }
+      this.SessionStoreManager.checkValidationToken = sinon
+        .stub()
+        .returns(false)
       const user = this.AuthenticationController.getSessionUser(this.req)
       expect(user).to.be.null
     })
-
-    it('should accept a valid validation token', function() {
-      this.req.sessionID = 'nabuchodonosorroidebabylone'
-      this.req.session = {
-        validationToken: 'v1:lone', // matches last 4 characters of session id
-        passport: {
-          user: { _id: 'three' }
-        }
-      }
-      const user = this.AuthenticationController.getSessionUser(this.req)
-      expect(user).to.deep.equal({ _id: 'three' })
-    })
-
-    it('should work with legacy sessions', function() {
-      this.req.session = { user: { _id: 'one' } }
-      const user = this.AuthenticationController.getSessionUser(this.req)
-      expect(user).to.deep.equal({ _id: 'one' })
-    })
   })
 
   describe('doPassportLogin', function() {

services/web/test/unit/src/Security/SessionStoreManagerTests.js

@@ -0,0 +1,70 @@
+const sinon = require('sinon')
+const chai = require('chai')
+const { expect } = chai
+const modulePath = '../../../../app/src/infrastructure/SessionStoreManager.js'
+const SandboxedModule = require('sandboxed-module')
+
+describe('SessionStoreManager', function() {
+  beforeEach(function() {
+    this.SessionStoreManager = SandboxedModule.require(modulePath, {
+      globals: {
+        console: console
+      },
+      requires: {
+        'metrics-sharelatex': (this.Metrics = { inc: sinon.stub() }),
+        'logger-sharelatex': (this.logger = {
+          log: sinon.stub(),
+          warn: sinon.stub(),
+          error: sinon.stub(),
+          err: sinon.stub()
+        })
+      }
+    })
+    this.sessionStore = {
+      generate: sinon.spy(req => {
+        req.session = {}
+      })
+    }
+  })
+  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('checkValidationToken', function() {
+    this.beforeEach(function() {
+      this.SessionStoreManager.enableValidationToken(this.sessionStore)
+      this.req = { sessionID: '123456789' }
+      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 return false when the session id has changed', function() {
+      this.req.sessionID = 'abcdefghijklmnopqrstuvwxyz'
+      const result = this.SessionStoreManager.checkValidationToken(this.req)
+      expect(result).to.equal(false)
+    })
+    it('should return true 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)
+    })
+  })
+})