Merge pull request #16190 from overleaf/revert-15519-em-upgrade-passport

Revert "Upgrade passport"

GitOrigin-RevId: 34a5442d6dae9623463908f92ab103bdc16f1b67

package-lock.json

@@ -30573,20 +30573,15 @@
       }
     },
     "node_modules/passport": {
-      "version": "0.6.0",
-      "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz",
-      "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==",
+      "version": "0.4.1",
+      "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz",
+      "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==",
       "dependencies": {
         "passport-strategy": "1.x.x",
-        "pause": "0.0.1",
-        "utils-merge": "^1.0.1"
+        "pause": "0.0.1"
       },
       "engines": {
         "node": ">= 0.4.0"
-      },
-      "funding": {
-        "type": "github",
-        "url": "https://github.com/sponsors/jaredhanson"
       }
     },
     "node_modules/passport-google-oauth20": {
@@ -43530,7 +43525,7 @@
         "otplib": "^12.0.1",
         "p-limit": "^2.3.0",
         "parse-data-url": "^2.0.0",
-        "passport": "^0.6.0",
+        "passport": "^0.4.1",
         "passport-google-oauth20": "^2.0.0",
         "passport-ldapauth": "^2.1.4",
         "passport-local": "^1.0.0",
@@ -52272,7 +52267,7 @@
         "otplib": "^12.0.1",
         "p-limit": "^2.3.0",
         "parse-data-url": "^2.0.0",
-        "passport": "^0.6.0",
+        "passport": "^0.4.1",
         "passport-google-oauth20": "^2.0.0",
         "passport-ldapauth": "^2.1.4",
         "passport-local": "^1.0.0",
@@ -71002,13 +70997,12 @@
       }
     },
     "passport": {
-      "version": "0.6.0",
-      "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz",
-      "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==",
+      "version": "0.4.1",
+      "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz",
+      "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==",
       "requires": {
         "passport-strategy": "1.x.x",
-        "pause": "0.0.1",
-        "utils-merge": "^1.0.1"
+        "pause": "0.0.1"
       }
     },
     "passport-google-oauth20": {

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -80,36 +80,30 @@ const AuthenticationController = {
     // This function is middleware which wraps the passport.authenticate middleware,
     // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure,
     // and send a `{redir: ""}` response on success
-    passport.authenticate(
-      'local',
-      { keepSessionInfo: true },
-      function (err, user, info) {
-        if (err) {
-          return next(err)
-        }
-        if (user) {
-          // `user` is either a user object or false
-          AuthenticationController.setAuditInfo(req, {
-            method: 'Password login',
-          })
-          return AuthenticationController.finishLogin(user, req, res, next)
+    passport.authenticate('local', function (err, user, info) {
+      if (err) {
+        return next(err)
+      }
+      if (user) {
+        // `user` is either a user object or false
+        AuthenticationController.setAuditInfo(req, { method: 'Password login' })
+        return AuthenticationController.finishLogin(user, req, res, next)
+      } else {
+        if (info.redir != null) {
+          return res.json({ redir: info.redir })
         } else {
-          if (info.redir != null) {
-            return res.json({ redir: info.redir })
-          } else {
-            res.status(info.status || 200)
-            delete info.status
-            const body = { message: info }
-            const { errorReason } = info
-            if (errorReason) {
-              body.errorReason = errorReason
-              delete info.errorReason
-            }
-            return res.json(body)
+          res.status(info.status || 200)
+          delete info.status
+          const body = { message: info }
+          const { errorReason } = info
+          if (errorReason) {
+            body.errorReason = errorReason
+            delete info.errorReason
           }
+          return res.json(body)
         }
       }
-    )(req, res, next)
+    })(req, res, next)
   },
 
   finishLogin(user, req, res, next) {
@@ -563,34 +557,55 @@ const AuthenticationController = {
 }
 
 function _afterLoginSessionSetup(req, user, callback) {
-  req.login(user, { keepSessionInfo: true }, function (err) {
+  if (callback == null) {
+    callback = function () {}
+  }
+  req.login(user, function (err) {
     if (err) {
       OError.tag(err, 'error from req.login', {
         user_id: user._id,
       })
       return callback(err)
     }
-    delete req.session.__tmp
-    delete req.session.csrfSecret
-    req.session.save(function (err) {
+    // Regenerate the session to get a new sessionID (cookie value) to
+    // protect against session fixation attacks
+    const oldSession = req.session
+    req.session.destroy(function (err) {
       if (err) {
-        OError.tag(err, 'error saving regenerated session after login', {
+        OError.tag(err, 'error when trying to destroy old session', {
           user_id: user._id,
         })
         return callback(err)
       }
-      UserSessionsManager.trackSession(user, req.sessionID, function () {})
-      if (!req.deviceHistory) {
-        // Captcha disabled or SSO-based login.
-        return callback()
+      req.sessionStore.generate(req)
+      // Note: the validation token is not writable, so it does not get
+      // transferred to the new session below.
+      for (const key in oldSession) {
+        const value = oldSession[key]
+        if (key !== '__tmp' && key !== 'csrfSecret') {
+          req.session[key] = value
+        }
       }
-      req.deviceHistory.add(user.email)
-      req.deviceHistory
-        .serialize(req.res)
-        .catch(err => {
-          logger.err({ err }, 'cannot serialize deviceHistory')
-        })
-        .finally(() => callback())
+      req.session.save(function (err) {
+        if (err) {
+          OError.tag(err, 'error saving regenerated session after login', {
+            user_id: user._id,
+          })
+          return callback(err)
+        }
+        UserSessionsManager.trackSession(user, req.sessionID, function () {})
+        if (!req.deviceHistory) {
+          // Captcha disabled or SSO-based login.
+          return callback()
+        }
+        req.deviceHistory.add(user.email)
+        req.deviceHistory
+          .serialize(req.res)
+          .catch(err => {
+            logger.err({ err }, 'cannot serialize deviceHistory')
+          })
+          .finally(() => callback())
+      })
     })
   })
 }

services/web/app/src/Features/User/UserController.js

@@ -266,8 +266,7 @@ async function tryDeleteUser(req, res, next) {
   const sessionId = req.sessionID
 
   if (typeof req.logout === 'function') {
-    const logout = promisify(req.logout)
-    await logout()
+    req.logout()
   }
 
   const destroySession = promisify(req.session.destroy.bind(req.session))
@@ -432,10 +431,9 @@ async function doLogout(req) {
   logger.debug({ user }, 'logging out')
   const sessionId = req.sessionID
 
+  // passport logout
   if (typeof req.logout === 'function') {
-    // passport logout
-    const logout = promisify(req.logout.bind(req))
-    await logout()
+    req.logout()
   }
 
   const destroySession = promisify(req.session.destroy.bind(req.session))

services/web/package.json

@@ -137,7 +137,7 @@
     "otplib": "^12.0.1",
     "p-limit": "^2.3.0",
     "parse-data-url": "^2.0.0",
-    "passport": "^0.6.0",
+    "passport": "^0.4.1",
     "passport-google-oauth20": "^2.0.0",
     "passport-ldapauth": "^2.1.4",
     "passport-local": "^1.0.0",

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -77,7 +77,7 @@ describe('AuthenticationController', function () {
         '../User/UserSessionsManager': (this.UserSessionsManager = {
           trackSession: sinon.stub(),
           untrackSession: sinon.stub(),
-          revokeAllUserSessions: sinon.stub().yields(null),
+          revokeAllUserSessions: sinon.stub().callsArgWith(1, null),
         }),
         '../../infrastructure/Modules': (this.Modules = {
           hooks: { fire: sinon.stub().yields(null, []) },
@@ -184,17 +184,17 @@ describe('AuthenticationController', function () {
   describe('passportLogin', function () {
     beforeEach(function () {
       this.info = null
-      this.req.login = sinon.stub().yields(null)
+      this.req.login = sinon.stub().callsArgWith(1, null)
       this.res.json = sinon.stub()
       this.req.session = {
         passport: { user: this.user },
         postLoginRedirect: '/path/to/redir/to',
       }
-      this.req.session.destroy = sinon.stub().yields(null)
-      this.req.session.save = sinon.stub().yields(null)
+      this.req.session.destroy = sinon.stub().callsArgWith(0, null)
+      this.req.session.save = sinon.stub().callsArgWith(0, null)
       this.req.sessionStore = { generate: sinon.stub() }
       this.AuthenticationController.finishLogin = sinon.stub()
-      this.passport.authenticate.yields(null, this.user, this.info)
+      this.passport.authenticate.callsArgWith(1, null, this.user, this.info)
       this.err = new Error('woops')
     })
 
@@ -205,7 +205,7 @@ describe('AuthenticationController', function () {
 
     describe('when authenticate produces an error', function () {
       beforeEach(function () {
-        this.passport.authenticate.yields(this.err)
+        this.passport.authenticate.callsArgWith(1, this.err)
       })
 
       it('should return next with an error', function () {
@@ -221,7 +221,7 @@ describe('AuthenticationController', function () {
     describe('when authenticate produces a user', function () {
       beforeEach(function () {
         this.req.session.postLoginRedirect = 'some_redirect'
-        this.passport.authenticate.yields(null, this.user, this.info)
+        this.passport.authenticate.callsArgWith(1, null, this.user, this.info)
       })
 
       afterEach(function () {
@@ -244,7 +244,7 @@ describe('AuthenticationController', function () {
     describe('when authenticate does not produce a user', function () {
       beforeEach(function () {
         this.info = { text: 'a', type: 'b' }
-        this.passport.authenticate.yields(null, false, this.info)
+        this.passport.authenticate.callsArgWith(1, null, false, this.info)
       })
 
       it('should not call finishLogin', function () {
@@ -273,7 +273,8 @@ describe('AuthenticationController', function () {
     beforeEach(function () {
       this.AuthenticationController._recordFailedLogin = sinon.stub()
       this.AuthenticationController._recordSuccessfulLogin = sinon.stub()
-      this.Modules.hooks.fire = sinon.stub().yields(null, [])
+      this.Modules.hooks.fire = sinon.stub().callsArgWith(3, null, [])
+      // @AuthenticationController.establishUserSession = sinon.stub().callsArg(2)
       this.req.body = {
         email: this.email,
         password: this.password,
@@ -289,7 +290,7 @@ describe('AuthenticationController', function () {
       beforeEach(function () {
         this.Modules.hooks.fire = sinon
           .stub()
-          .yields(null, [null, { redir: '/somewhere' }, null])
+          .callsArgWith(3, null, [null, { redir: '/somewhere' }, null])
       })
 
       it('should stop early and call done with this info object', function (done) {
@@ -310,7 +311,7 @@ describe('AuthenticationController', function () {
 
     describe('when the users rate limit', function () {
       beforeEach(function () {
-        this.LoginRateLimiter.processLoginRequest.yields(null, false)
+        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, false)
       })
 
       it('should block the request if the limit has been exceeded', function (done) {
@@ -329,10 +330,10 @@ describe('AuthenticationController', function () {
     describe('when the user is authenticated', function () {
       beforeEach(function () {
         this.cb = sinon.stub()
-        this.LoginRateLimiter.processLoginRequest.yields(null, true)
+        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true)
         this.AuthenticationManager.authenticate = sinon
           .stub()
-          .yields(null, this.user)
+          .callsArgWith(3, null, this.user)
         this.req.sessionID = Math.random()
       })
 
@@ -360,7 +361,7 @@ describe('AuthenticationController', function () {
         beforeEach(function () {
           this.AuthenticationManager.authenticate = sinon
             .stub()
-            .yields(new AuthenticationErrors.ParallelLoginError())
+            .callsArgWith(3, new AuthenticationErrors.ParallelLoginError())
           this.AuthenticationController.doPassportLogin(
             this.req,
             this.req.body.email,
@@ -436,10 +437,10 @@ describe('AuthenticationController', function () {
 
     describe('when the user is not authenticated', function () {
       beforeEach(function () {
-        this.LoginRateLimiter.processLoginRequest.yields(null, true)
+        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true)
         this.AuthenticationManager.authenticate = sinon
           .stub()
-          .yields(null, null)
+          .callsArgWith(3, null, null)
         this.cb = sinon.stub()
         this.AuthenticationController.doPassportLogin(
           this.req,
@@ -936,7 +937,7 @@ describe('AuthenticationController', function () {
 
   describe('_recordSuccessfulLogin', function () {
     beforeEach(function () {
-      this.UserUpdater.updateUser = sinon.stub().yields()
+      this.UserUpdater.updateUser = sinon.stub().callsArg(2)
       this.AuthenticationController._recordSuccessfulLogin(
         this.user._id,
         this.callback
@@ -1077,8 +1078,8 @@ describe('AuthenticationController', function () {
       this.req.session = {
         passport: { user: { _id: 'one' } },
       }
-      this.req.session.destroy = sinon.stub().yields(null)
-      this.req.session.save = sinon.stub().yields(null)
+      this.req.session.destroy = sinon.stub().callsArgWith(0, null)
+      this.req.session.save = sinon.stub().callsArgWith(0, null)
       this.req.sessionStore = { generate: sinon.stub() }
       this.req.login = sinon.stub().yields(null)
 
@@ -1344,7 +1345,9 @@ describe('AuthenticationController', function () {
 
       describe('when req.session.save produces an error', function () {
         beforeEach(function () {
-          this.req.session.save = sinon.stub().yields(new Error('woops'))
+          this.req.session.save = sinon
+            .stub()
+            .callsArgWith(0, new Error('woops'))
         })
 
         it('should produce an error', function (done) {

services/web/test/unit/src/User/UserControllerTests.js

@@ -173,7 +173,7 @@ describe('UserController', function () {
   describe('tryDeleteUser', function () {
     beforeEach(function () {
       this.req.body.password = 'wat'
-      this.req.logout = sinon.stub().yields()
+      this.req.logout = sinon.stub()
       this.req.session.destroy = sinon.stub().yields()
       this.SessionManager.getLoggedInUserId = sinon
         .stub()