From 0576e02127340c8cfddd1ada8d8ad785bc74b286 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 2 May 2024 11:35:53 +0100 Subject: [PATCH] Merge pull request #18152 from overleaf/jpa-stricter-session-validation [web] stricter session validation GitOrigin-RevId: 3ef916318fde7f31e3e3fd0f7082dde7a2975a27 --- .../src/infrastructure/SessionStoreManager.js | 13 +++- .../web/test/acceptance/src/SessionTests.js | 75 +++++++++++++++++++ .../src/Security/SessionStoreManagerTests.js | 24 +++++- 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/services/web/app/src/infrastructure/SessionStoreManager.js b/services/web/app/src/infrastructure/SessionStoreManager.js index 168c0091a9..48487ec1ab 100644 --- a/services/web/app/src/infrastructure/SessionStoreManager.js +++ b/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() }, diff --git a/services/web/test/acceptance/src/SessionTests.js b/services/web/test/acceptance/src/SessionTests.js index 2822f784c7..9109ccb444 100644 --- a/services/web/test/acceptance/src/SessionTests.js +++ b/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) + }) + }) }) diff --git a/services/web/test/unit/src/Security/SessionStoreManagerTests.js b/services/web/test/unit/src/Security/SessionStoreManagerTests.js index a2f48f14aa..c7672ec86c 100644 --- a/services/web/test/unit/src/Security/SessionStoreManagerTests.js +++ b/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 () {