Merge pull request #18153 from overleaf/jpa-validate-session-in-store

[web] check for redis connection being out of sync in session store

GitOrigin-RevId: c271e88d4e1fbcb0f7a57f4775e8ef88b70b16a8

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)

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

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

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

@@ -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,
-}

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
     )

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

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

@@ -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)
-    })
-  })
-})