Merge pull request #12269 from overleaf/jk-enable-password-similarity-check

[web] Enforce password similarity check

GitOrigin-RevId: 1bc4efebba401663c1db9d209dc560560f160ce0

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

@@ -216,19 +216,26 @@ const AuthenticationManager = {
       })
     }
     if (typeof email === 'string' && email !== '') {
+      const startOfEmail = email.split('@')[0]
+      if (
+        password.includes(email) ||
+        password.includes(startOfEmail) ||
+        email.includes(password)
+      ) {
+        return new InvalidPasswordError({
+          message: 'password contains part of email address',
+          info: { code: 'contains_email' },
+        })
+      }
       try {
-        const substringError =
-          AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-            password,
-            email
-          )
-        if (substringError) {
-          Metrics.inc('password-contains-substring-of-email')
-        }
         const passwordTooSimilarError =
           AuthenticationManager._validatePasswordNotTooSimilar(password, email)
         if (passwordTooSimilarError) {
           Metrics.inc('password-too-similar-to-email')
+          return new InvalidPasswordError({
+            message: 'password is too similar to email address',
+            info: { code: 'too_similar' },
+          })
         }
       } catch (error) {
         logger.error(
@@ -237,17 +244,6 @@ const AuthenticationManager = {
         )
       }
       // TODO: remove this check once the password-too-similar checks are active?
-      const startOfEmail = email.split('@')[0]
-      if (
-        password.includes(email) ||
-        password.includes(startOfEmail) ||
-        email.includes(password)
-      ) {
-        return new InvalidPasswordError({
-          message: 'password contains part of email address',
-          info: { code: 'contains_email' },
-        })
-      }
     }
     return null
   },
@@ -393,50 +389,18 @@ const AuthenticationManager = {
     const stringsToCheck = [email]
       .concat(email.split(/\W+/))
       .concat(email.split(/@/))
-    let largestSimilarity = 0
-    let err = null
     for (const emailPart of stringsToCheck) {
       if (!_exceedsMaximumLengthRatio(password, MAX_SIMILARITY, emailPart)) {
         const similarity = DiffHelper.stringSimilarity(password, emailPart)
-        const similarityOneDecimalPlace = Math.floor(similarity * 10) / 10
-        largestSimilarity = Math.max(
-          largestSimilarity,
-          similarityOneDecimalPlace
-        )
         if (similarity > MAX_SIMILARITY) {
           logger.warn(
             { email, emailPart, similarity, maxSimilarity: MAX_SIMILARITY },
             'Password too similar to email'
           )
-          err = new Error('password is too similar to email')
+          return new Error('password is too similar to email')
         }
       }
     }
-    Metrics.inc('password-validation-similarity', 1, {
-      similarity: largestSimilarity,
-    })
-    return err
-  },
-
-  _validatePasswordNotContainsEmailSubstrings(password, email) {
-    password = password.toLowerCase()
-    email = email.toLowerCase()
-    const chunkLength = 4
-
-    if (email.length < chunkLength) {
-      return
-    }
-
-    let chunk
-    for (let i = 0; i <= email.length - chunkLength; i++) {
-      chunk = email.slice(i, i + chunkLength)
-      if (password.indexOf(chunk) !== -1) {
-        return new InvalidPasswordError({
-          message: 'password contains part of email address',
-          info: { code: 'contains_email' },
-        })
-      }
-    }
   },
 }
 

services/web/app/src/Features/PasswordReset/PasswordResetController.js

@@ -28,6 +28,12 @@ async function setNewUserPassword(req, res, next) {
           text: req.i18n.translate('invalid_password_contains_email'),
         },
       })
+    } else if (err?.info?.code === 'too_similar') {
+      return res.status(400).json({
+        message: {
+          text: req.i18n.translate('invalid_password_too_similar'),
+        },
+      })
     } else {
       return res.status(400).json({
         message: { text: err.message },

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

@@ -102,6 +102,12 @@ async function changePassword(req, res, next) {
           res,
           req.i18n.translate('invalid_password_contains_email')
         )
+      } else if (error?.info?.code === 'too_similar') {
+        return HttpErrorHandler.badRequest(
+          req,
+          res,
+          req.i18n.translate('invalid_password_too_similar')
+        )
       } else {
         return HttpErrorHandler.badRequest(req, res, error.message)
       }

services/web/locales/en.json

@@ -751,6 +751,7 @@
   "invalid_password_not_set": "Password is required",
   "invalid_password_too_long": "Maximum password length __maxLength__ exceeded",
   "invalid_password_too_short": "Password too short, minimum __minLength__",
+  "invalid_password_too_similar": "Password is too similar to email address",
   "invalid_request": "Invalid Request. Please correct the data and try again.",
   "invalid_zip_file": "Invalid zip file",
   "invite_more_collabs": "Invite more collaborators",

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

@@ -6,7 +6,7 @@ describe('PasswordReset', function () {
   let email, response, user, userHelper, token, emailQuery
   beforeEach(async function () {
     userHelper = new UserHelper()
-    email = userHelper.getDefaultEmail()
+    email = 'somecooluser@example.com'
     emailQuery = `?email=${encodeURIComponent(email)}`
     userHelper = await UserHelper.createUser({ email })
     user = userHelper.user
@@ -204,6 +204,29 @@ describe('PasswordReset', function () {
         })
       })
 
+      it('should flag password too similar to email', async function () {
+        const localPart = email.split('@').shift()
+        const localPartReversed = localPart.split('').reverse().join('')
+        // send bad password
+        response = await userHelper.fetch('/user/password/set', {
+          method: 'POST',
+          headers: {
+            'Content-Type': 'application/json',
+            Accept: 'application/json',
+          },
+          body: JSON.stringify({
+            passwordResetToken: token,
+            password: `${localPartReversed}123`,
+            email,
+          }),
+        })
+        expect(response.status).to.equal(400)
+        const body = await response.json()
+        expect(body).to.deep.equal({
+          message: { text: 'Password is too similar to email address' },
+        })
+      })
+
       it('should be able to retry after providing an invalid password', async function () {
         // send bad password
         response = await userHelper.fetch('/user/password/set', {

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

@@ -9,7 +9,7 @@ describe('PasswordUpdate', function () {
   })
   beforeEach(async function () {
     userHelper = new UserHelper()
-    email = userHelper.getDefaultEmail()
+    email = 'somecooluser@example.com'
     password = 'old-password'
     userHelper = await UserHelper.createUser({ email, password })
     userHelper = await UserHelper.loginUser({
@@ -140,5 +140,35 @@ describe('PasswordUpdate', function () {
         expect(auditLog).to.deep.equal([])
       })
     })
+    describe('new password is too similar to email', function () {
+      beforeEach(async function () {
+        response = await userHelper.fetch('/user/password/update', {
+          method: 'POST',
+          headers: {
+            'Content-Type': 'application/json',
+            Accept: 'application/json',
+          },
+          body: JSON.stringify({
+            currentPassword: password,
+            newPassword1: 'coolusersome123',
+            newPassword2: 'coolusersome123',
+          }),
+        })
+        userHelper = await UserHelper.getUser({ email })
+      })
+      it('should return 400', async function () {
+        expect(response.status).to.equal(400)
+      })
+      it('should return error message', async function () {
+        const body = await response.json()
+        expect(body.message).to.equal(
+          'Password is too similar to email address'
+        )
+      })
+      it('should not update audit log', async function () {
+        const auditLog = userHelper.getAuditLogWithoutNoise()
+        expect(auditLog).to.deep.equal([])
+      })
+    })
   })
 })

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

@@ -685,62 +685,6 @@ describe('AuthenticationManager', function () {
     })
   })
 
-  describe('_validatePasswordNotContainsEmailSubstrings', function () {
-    it('should return nothing for a dissimilar password', function () {
-      const password = 'fublmqgaeohhvd8'
-      const email = 'someuser@example.com'
-      const error =
-        this.AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-          password,
-          email
-        )
-      expect(error).to.not.exist
-    })
-
-    it('should return an error for password that is same as email', function () {
-      const email = 'someuser@example.com'
-      const error =
-        this.AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-          email,
-          email
-        )
-      expect(error).to.exist
-    })
-
-    it('should return an error for a password with a substring of email', function () {
-      const password = 'cooluser1253'
-      const email = 'somecooluser@example.com'
-      const error =
-        this.AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-          password,
-          email
-        )
-      expect(error).to.exist
-    })
-
-    it('should return an error for a password with a substring of email, regardless of case', function () {
-      const password = 'coOLUSer1253'
-      const email = 'somecooluser@example.com'
-      const error =
-        this.AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-          password,
-          email
-        )
-      expect(error).to.exist
-    })
-
-    it('should return nothing for a password containing first two characters of email', function () {
-      const password = 'lmgaesopxzqg'
-      const email = 'someuser@example.com'
-      const error =
-        this.AuthenticationManager._validatePasswordNotContainsEmailSubstrings(
-          password,
-          email
-        )
-      expect(error).to.not.exist
-    })
-  })
-
   describe('_validatePasswordNotTooSimilar', function () {
     beforeEach(function () {
       this.metrics.inc.reset()
@@ -778,21 +722,6 @@ describe('AuthenticationManager', function () {
       }
     })
 
-    it('should send a metric with a rounded similarity score when password is too similar to email', function () {
-      const password = 'su2oe1em3re'
-      const email = 'someuser@example.com'
-      const error = this.AuthenticationManager._validatePasswordNotTooSimilar(
-        password,
-        email
-      )
-      expect(
-        this.metrics.inc.calledWith('password-validation-similarity', 1, {
-          similarity: 0.7,
-        })
-      ).to.equal(true)
-      expect(error).to.exist
-    })
-
     it('should return nothing when the password different from email', function () {
       const password = '58WyLvr'
       const email = 'someuser@example.com'
@@ -979,12 +908,13 @@ describe('AuthenticationManager', function () {
         this.metrics.inc.reset()
       })
 
-      it('should send a metric when the password is too similar to the email', function (done) {
+      it('should produce an error when the password is too similar to the email', function (done) {
         this.AuthenticationManager.setUserPassword(
           this.user,
           this.password,
           err => {
-            expect(err).to.not.exist
+            expect(err).to.exist
+            expect(err?.info?.code).to.equal('too_similar')
             expect(
               this.metrics.inc.calledWith('password-too-similar-to-email')
             ).to.equal(true)
@@ -993,12 +923,13 @@ describe('AuthenticationManager', function () {
         )
       })
 
-      it('should send a metric when the password is too similar to the email, regardless of case', function (done) {
+      it('should produce an error when the password is too similar to the email, regardless of case', function (done) {
         this.AuthenticationManager.setUserPassword(
           this.user,
           this.password.toUpperCase(),
           err => {
-            expect(err).to.not.exist
+            expect(err).to.exist
+            expect(err?.info?.code).to.equal('too_similar')
             expect(
               this.metrics.inc.calledWith('password-too-similar-to-email')
             ).to.equal(true)
@@ -1008,30 +939,6 @@ describe('AuthenticationManager', function () {
       })
     })
 
-    describe('password contains substring of email', function () {
-      beforeEach(function () {
-        this.user.email = 'somecooluser@example.com'
-        this.password = 'somecoolfhzxk'
-        this.metrics.inc.reset()
-      })
-
-      it('should send a metric when the password contains substring of the email', function (done) {
-        this.AuthenticationManager.setUserPassword(
-          this.user,
-          this.password,
-          err => {
-            expect(err).to.not.exist
-            expect(
-              this.metrics.inc.calledWith(
-                'password-contains-substring-of-email'
-              )
-            ).to.equal(true)
-            done()
-          }
-        )
-      })
-    })
-
     describe('successful password set attempt', function () {
       beforeEach(function () {
         this.metrics.inc.reset()
@@ -1069,14 +976,9 @@ describe('AuthenticationManager', function () {
         ).to.equal(false)
       })
 
-      it('should not send a metric for password-contains-substring-of-email', function () {
-        expect(
-          this.metrics.inc.calledWith('password-contains-substring-of-email')
-        ).to.equal(false)
-      })
-
       it('should call the callback', function () {
         this.callback.called.should.equal(true)
+        expect(this.callback.lastCall.args[0]).to.not.be.instanceOf(Error)
       })
     })
   })