Merge pull request #9983 from overleaf/jpa-web-fix-password-upgrade

[web] fix process for upgrading of password hashes

GitOrigin-RevId: 3bc99dbd8601c190d758080d70ea1a465bd9e542

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

@@ -24,6 +24,17 @@ const _checkWriteResult = function (result, callback) {
   }
 }
 
+function _validatePasswordNotTooLong(password) {
+  // bcrypt has a hard limit of 72 characters.
+  if (password.length > 72) {
+    return new InvalidPasswordError({
+      message: 'password is too long',
+      info: { code: 'too_long' },
+    })
+  }
+  return null
+}
+
 const AuthenticationManager = {
   _checkUserPassword(query, password, callback) {
     // Using Mongoose for legacy reasons here. The returned User instance
@@ -140,6 +151,10 @@ const AuthenticationManager = {
         info: { code: 'too_long' },
       })
     }
+    const passwordLengthError = _validatePasswordNotTooLong(password)
+    if (passwordLengthError) {
+      return passwordLengthError
+    }
     if (
       !allowAnyChars &&
       !AuthenticationManager._passwordCharactersAreValid(password)
@@ -176,13 +191,18 @@ const AuthenticationManager = {
     // check current number of rounds and rehash if necessary
     const currentRounds = bcrypt.getRounds(hashedPassword)
     if (currentRounds < BCRYPT_ROUNDS) {
-      AuthenticationManager.setUserPassword(user, password, callback)
+      AuthenticationManager._setUserPasswordInMongo(user, password, callback)
     } else {
       callback()
     }
   },
 
   hashPassword(password, callback) {
+    // Double-check the size to avoid truncating in bcrypt.
+    const error = _validatePasswordNotTooLong(password)
+    if (error) {
+      return callback(error)
+    }
     bcrypt.genSalt(BCRYPT_ROUNDS, BCRYPT_MINOR_VERSION, function (error, salt) {
       if (error) {
         return callback(error)
@@ -211,35 +231,37 @@ const AuthenticationManager = {
         if (match) {
           return callback(new PasswordMustBeDifferentError())
         }
-        this.hashPassword(password, function (error, hash) {
-          if (error) {
-            return callback(error)
-          }
-          db.users.updateOne(
-            {
-              _id: ObjectId(user._id.toString()),
-            },
-            {
-              $set: {
-                hashedPassword: hash,
-              },
-              $unset: {
-                password: true,
-              },
-            },
-            function (updateError, result) {
-              if (updateError) {
-                return callback(updateError)
-              }
-              _checkWriteResult(result, callback)
-              HaveIBeenPwned.checkPasswordForReuseInBackground(password)
-            }
-          )
-        })
+        this._setUserPasswordInMongo(user, password, callback)
+        HaveIBeenPwned.checkPasswordForReuseInBackground(password)
       }
     )
   },
 
+  _setUserPasswordInMongo(user, password, callback) {
+    this.hashPassword(password, function (error, hash) {
+      if (error) {
+        return callback(error)
+      }
+      db.users.updateOne(
+        { _id: ObjectId(user._id.toString()) },
+        {
+          $set: {
+            hashedPassword: hash,
+          },
+          $unset: {
+            password: true,
+          },
+        },
+        function (updateError, result) {
+          if (updateError) {
+            return callback(updateError)
+          }
+          _checkWriteResult(result, callback)
+        }
+      )
+    })
+  },
+
   _passwordCharactersAreValid(password) {
     let digits, letters, lettersUp, symbols
     if (

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

@@ -217,6 +217,16 @@ describe('AuthenticationManager', function () {
     })
   })
 
+  describe('hashPassword', function () {
+    it('should block too long passwords', function (done) {
+      this.AuthenticationManager.hashPassword('x'.repeat(100), err => {
+        expect(err).to.exist
+        expect(err.message).to.equal('password is too long')
+        done()
+      })
+    })
+  })
+
   describe('authenticate', function () {
     describe('when the user exists in the database', function () {
       beforeEach(function () {
@@ -277,7 +287,7 @@ describe('AuthenticationManager', function () {
           this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf'
           this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true)
           this.bcrypt.getRounds = sinon.stub().returns(1)
-          this.AuthenticationManager.setUserPassword = sinon
+          this.AuthenticationManager._setUserPasswordInMongo = sinon
             .stub()
             .callsArgWith(2, null)
           this.AuthenticationManager.authenticate(
@@ -305,7 +315,7 @@ describe('AuthenticationManager', function () {
         })
 
         it('should set the users password (with a higher number of rounds)', function () {
-          this.AuthenticationManager.setUserPassword
+          this.AuthenticationManager._setUserPasswordInMongo
             .calledWith(this.user, this.unencryptedPassword)
             .should.equal(true)
         })