From 04fa863f9fb650476a1bc1ccaacf0e347e69c153 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 9 Apr 2021 09:44:26 +0100 Subject: [PATCH] Merge pull request #3892 from overleaf/sk-reroll-csrf Regenerate CSRF token on login GitOrigin-RevId: 501582b34794a822f4c9fe3af2575b5756511e06 --- .../AuthenticationController.js | 2 +- .../acceptance/src/AuthenticationTests.js | 37 +++++++++++++++++++ .../test/acceptance/src/ProjectInviteTests.js | 23 +++++++----- .../test/acceptance/src/UserHelperTests.js | 20 ++-------- .../web/test/acceptance/src/helpers/User.js | 13 ++++++- .../test/acceptance/src/helpers/UserHelper.js | 5 +-- .../AuthenticationControllerTests.js | 10 +++++ 7 files changed, 80 insertions(+), 30 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 5d472f457c..9a41b33de1 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -489,7 +489,7 @@ function _afterLoginSessionSetup(req, user, callback) { // transferred to the new session below. for (let key in oldSession) { const value = oldSession[key] - if (key !== '__tmp') { + if (key !== '__tmp' && key !== 'csrfSecret') { req.session[key] = value } } diff --git a/services/web/test/acceptance/src/AuthenticationTests.js b/services/web/test/acceptance/src/AuthenticationTests.js index fd2df6dbc9..445813b813 100644 --- a/services/web/test/acceptance/src/AuthenticationTests.js +++ b/services/web/test/acceptance/src/AuthenticationTests.js @@ -8,6 +8,43 @@ describe('Authentication', function() { user = new User() }) + describe('CSRF regeneration on login', function() { + it('should prevent use of csrf token from before login', function(done) { + user.logout(err => { + if (err) { + return done(err) + } + user.getCsrfToken(err => { + if (err) { + return done(err) + } + const oldToken = user.csrfToken + user.login(err => { + if (err) { + return done(err) + } + expect(oldToken === user.csrfToken).to.equal(false) + user.request.post( + { + headers: { + 'x-csrf-token': oldToken + }, + url: '/project/new', + json: { projectName: 'test' } + }, + (err, response, body) => { + expect(err).to.not.exist + expect(response.statusCode).to.equal(403) + expect(body).to.equal('Forbidden') + done() + } + ) + }) + }) + }) + }) + }) + describe('login', function() { beforeEach('doLogin', async function() { await user.login() diff --git a/services/web/test/acceptance/src/ProjectInviteTests.js b/services/web/test/acceptance/src/ProjectInviteTests.js index da11a94464..87c3d2e7d6 100644 --- a/services/web/test/acceptance/src/ProjectInviteTests.js +++ b/services/web/test/acceptance/src/ProjectInviteTests.js @@ -89,15 +89,20 @@ const tryFollowInviteLink = (user, link, callback) => { } const tryAcceptInvite = (user, invite, callback) => { - user.request.post( - { - uri: `/project/${invite.projectId}/invite/token/${invite.token}/accept`, - json: { - token: invite.token - } - }, - callback - ) + user.getCsrfToken(err => { + if (err) { + return callback(err) + } + user.request.post( + { + uri: `/project/${invite.projectId}/invite/token/${invite.token}/accept`, + json: { + token: invite.token + } + }, + callback + ) + }) } const tryRegisterUser = (user, email, callback) => { diff --git a/services/web/test/acceptance/src/UserHelperTests.js b/services/web/test/acceptance/src/UserHelperTests.js index b0a4b5c666..9bddee37b9 100644 --- a/services/web/test/acceptance/src/UserHelperTests.js +++ b/services/web/test/acceptance/src/UserHelperTests.js @@ -160,22 +160,10 @@ describe('UserHelper', function() { }) describe('getCsrfToken', function() { - describe('when the csrfToken is not cached', function() { - it('should fetch csrfToken', async function() { - const userHelper = new UserHelper() - await userHelper.getCsrfToken() - expect(userHelper.csrfToken).to.be.a.string - }) - }) - - describe('when the csrfToken is cached', function() { - it('should return cached csrfToken', async function() { - let userHelper = new UserHelper() - await userHelper.getCsrfToken() - const csrfToken = userHelper._csrfToken - await userHelper.getCsrfToken() - expect(csrfToken).to.equal(userHelper._csrfToken) - }) + it('should fetch csrfToken', async function() { + const userHelper = new UserHelper() + await userHelper.getCsrfToken() + expect(userHelper.csrfToken).to.be.a.string }) }) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 1e5d473837..cef21804a0 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -113,7 +113,18 @@ class User { url: settings.enableLegacyLogin ? '/login/legacy' : '/login', json: { email, password: this.password } }, - callback + (error, response, body) => { + if (error != null) { + return callback(error) + } + // get new csrf token, then return result of login + this.getCsrfToken(err => { + if (err) { + return callback(err) + } + callback(null, response, body) + }) + } ) }) }) diff --git a/services/web/test/acceptance/src/helpers/UserHelper.js b/services/web/test/acceptance/src/helpers/UserHelper.js index 460c116973..cbbf82f81d 100644 --- a/services/web/test/acceptance/src/helpers/UserHelper.js +++ b/services/web/test/acceptance/src/helpers/UserHelper.js @@ -123,9 +123,6 @@ class UserHelper { * Requests csrf token unless already cached in internal state */ async getCsrfToken() { - if (this._csrfToken) { - return - } // get csrf token from api and store const response = await this.request.get('/dev/csrf') this._csrfToken = response.body @@ -255,6 +252,7 @@ class UserHelper { if (!userHelper.user) { throw new Error(`user not found for email: ${userData.email}`) } + await userHelper.getCsrfToken() return userHelper } @@ -298,6 +296,7 @@ class UserHelper { if (!userHelper.user) { throw new Error(`user not found for email: ${userData.email}`) } + await userHelper.getCsrfToken() return userHelper } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 98c3bc6c71..cee6e16cca 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -1236,6 +1236,16 @@ describe('AuthenticationController', function() { this.req.login.callCount.should.equal(1) }) + it('should erase the CSRF secret', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + expect(this.req.session.csrfSecret).to.not.exist + }) + it('should call req.session.save', function() { this.AuthenticationController.finishLogin( this.user,