Merge pull request #18152 from overleaf/jpa-stricter-session-validation

[web] stricter session validation

GitOrigin-RevId: 3ef916318fde7f31e3e3fd0f7082dde7a2975a27

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

@@ -31,9 +31,9 @@ function checkValidationToken(req) {
       }
     } else {
       Metrics.inc('security.session', 1, { status: 'missing' })
+      return false
     }
   }
-  return true // fallback to allowing session
 }
 
 module.exports = {
@@ -53,13 +53,22 @@ module.exports = {
   },
 
   validationMiddleware(req, res, next) {
-    if (!req.session.noSessionCallback) {
+    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()
   },

services/web/test/acceptance/src/SessionTests.js

@@ -2,6 +2,8 @@ const { expect } = require('chai')
 const async = require('async')
 const User = require('./helpers/User')
 const redis = require('./helpers/redis')
+const UserSessionsRedis = require('../../../app/src/Features/User/UserSessionsRedis')
+const rclient = UserSessionsRedis.client()
 
 describe('Sessions', function () {
   beforeEach(function (done) {
@@ -481,4 +483,77 @@ describe('Sessions', function () {
       )
     })
   })
+
+  describe('validationToken', function () {
+    const User = require('./helpers/User').promises
+
+    async function tryWithValidationToken(validationToken) {
+      const user = new User()
+      await user.login()
+
+      await checkSessionIsValid(user)
+
+      const [, sid] = user.sessionCookie().value.match(/^s:(.+?)\./)
+      const key = `sess:${sid}`
+      const sess = JSON.parse(await rclient.get(key))
+
+      expect(sess.validationToken).to.equal('v1:' + sid.slice(-4))
+
+      sess.validationToken = validationToken
+      await rclient.set(key, JSON.stringify(sess))
+
+      {
+        // The current code destroys the session and throws an error/500.
+        // Check for login redirect on page reload.
+        await user.doRequest('GET', '/project')
+
+        const { response } = await user.doRequest('GET', '/project')
+        expect(response.statusCode).to.equal(302)
+        expect(response.headers.location).to.equal('/login?')
+      }
+    }
+
+    async function getOtherUsersValidationToken() {
+      const otherUser = new User()
+      await otherUser.login()
+      await checkSessionIsValid(otherUser)
+      const { validationToken } = await otherUser.getSession()
+      expect(validationToken).to.match(/^v1:.{4}$/)
+      return validationToken
+    }
+    async function checkSessionIsValid(user) {
+      const { response } = await user.doRequest('GET', '/project')
+      expect(response.statusCode).to.equal(200)
+    }
+
+    it('should reject the redis value when missing', async function () {
+      await tryWithValidationToken(undefined)
+    })
+    it('should reject the redis value when empty', async function () {
+      await tryWithValidationToken('')
+    })
+    it('should reject the redis value when out of sync', async function () {
+      await tryWithValidationToken(await getOtherUsersValidationToken())
+    })
+    it('should ignore overwrites in app code', async function () {
+      const otherUsersValidationToken = getOtherUsersValidationToken()
+
+      const user = new User()
+      await user.login()
+      await checkSessionIsValid(user)
+
+      const { validationToken: token1 } = await user.getSession()
+      const allowedUpdateValue = 'allowed-update-value'
+      await user.setInSession({
+        validationToken: otherUsersValidationToken,
+        // also update another field to check that the write operation went through
+        allowedUpdate: allowedUpdateValue,
+      })
+      const { validationToken: token2, allowedUpdate } = await user.getSession()
+      expect(allowedUpdate).to.equal(allowedUpdateValue)
+      expect(token1).to.equal(token2)
+
+      await checkSessionIsValid(user)
+    })
+  })
 })

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

@@ -52,6 +52,19 @@ describe('SessionStoreManager', function () {
       )
       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()
@@ -67,15 +80,20 @@ describe('SessionStoreManager', function () {
           .and(sinon.match.has('message', 'invalid session'))
       )
     })
-    it('should accept the request when the session does not have a validation token', function () {
-      this.req = { sessionID: '123456789', 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.next).to.be.calledWithExactly()
+      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 () {