Merge pull request #3892 from overleaf/sk-reroll-csrf

Regenerate CSRF token on login

GitOrigin-RevId: 501582b34794a822f4c9fe3af2575b5756511e06

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

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

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

@@ -89,15 +89,20 @@ const tryFollowInviteLink = (user, link, callback) => {
 }
 
 const tryAcceptInvite = (user, invite, callback) => {
-  user.request.post(
+  user.getCsrfToken(err => {
-    {
+    if (err) {
-      uri: `/project/${invite.projectId}/invite/token/${invite.token}/accept`,
+      return callback(err)
-      json: {
+    }
-        token: invite.token
+    user.request.post(
-      }
+      {
-    },
+        uri: `/project/${invite.projectId}/invite/token/${invite.token}/accept`,
-    callback
+        json: {
-  )
+          token: invite.token
+        }
+      },
+      callback
+    )
+  })
 }
 
 const tryRegisterUser = (user, email, callback) => {

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() {
-      it('should fetch csrfToken', async function() {
+      const userHelper = new UserHelper()
-        const userHelper = new UserHelper()
+      await userHelper.getCsrfToken()
-        await userHelper.getCsrfToken()
+      expect(userHelper.csrfToken).to.be.a.string
-        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)
-      })
     })
   })
 

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

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
   }

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,