[web] Make rate-limit on login consistent, prevent "trim/case bypass" (#19555)

* Replace `LoginRateLimiter.processLoginRequest` call by use of `RateLimiterMiddleware`

* Lowercase the email to avoid rate-limit bypass

* Remove unit test "when the users rate limit"

* Use `EmailHelper.parseEmail` to normalize email in `processLoginRequest`

This should address the `trim()` bypass

* Use `.trim().toLowerCase()` instead of `EmailHelper.parseEmail`

We can't use `EmailHelper.parseEmail`, else it breaks the test (and feature): "with username that does not look like an email"

* Add acceptance test for rate limit

* Add comment on rate limits

* Rename `rateLimiter` to `rateLimiterLoginEmail` for clarity

* Make the login rate limits configurable from the settings

GitOrigin-RevId: cf1c3a416745f2b007c85014a5084570d4a049a7

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

@@ -212,91 +212,77 @@ const AuthenticationController = {
         if (info != null) {
           return done(null, false, info)
         }
-        LoginRateLimiter.processLoginRequest(email, function (err, isAllowed) {
-          if (err) {
-            return done(err)
-          }
-          if (!isAllowed) {
-            logger.debug({ email }, 'too many login requests')
-            return done(null, null, {
-              text: req.i18n.translate('to_many_login_requests_2_mins'),
-              type: 'error',
-              key: 'to-many-login-requests-2-mins',
-              status: 429,
-            })
-          }
-          const { fromKnownDevice } = AuthenticationController.getAuditInfo(req)
-          const auditLog = {
-            ipAddress: req.ip,
-            info: { method: 'Password login', fromKnownDevice },
-          }
-          AuthenticationManager.authenticate(
-            { email },
-            password,
-            auditLog,
-            {
-              enforceHIBPCheck: !fromKnownDevice,
-            },
-            function (error, user, isPasswordReused) {
-              if (error != null) {
-                if (error instanceof ParallelLoginError) {
-                  return done(null, false, { status: 429 })
-                } else if (error instanceof PasswordReusedError) {
-                  const text = `${req.i18n
-                    .translate(
-                      'password_compromised_try_again_or_use_known_device_or_reset'
-                    )
-                    .replace('<0>', '')
-                    .replace('</0>', ' (https://haveibeenpwned.com/passwords)')
-                    .replace('<1>', '')
-                    .replace(
-                      '</1>',
-                      ` (${Settings.siteUrl}/user/password/reset)`
-                    )}.`
-                  return done(null, false, {
-                    status: 400,
-                    type: 'error',
-                    key: 'password-compromised',
-                    text,
-                  })
-                }
-                return done(error)
-              }
-              if (
-                user &&
-                AuthenticationController.captchaRequiredForLogin(req, user)
-              ) {
-                done(null, false, {
-                  text: req.i18n.translate('cannot_verify_user_not_robot'),
-                  type: 'error',
-                  errorReason: 'cannot_verify_user_not_robot',
-                  status: 400,
-                })
-              } else if (user) {
-                if (
-                  isPasswordReused &&
-                  AuthenticationController.getRedirectFromSession(req) == null
-                ) {
-                  AuthenticationController.setRedirectInSession(
-                    req,
-                    '/compromised-password'
+        const { fromKnownDevice } = AuthenticationController.getAuditInfo(req)
+        const auditLog = {
+          ipAddress: req.ip,
+          info: { method: 'Password login', fromKnownDevice },
+        }
+        AuthenticationManager.authenticate(
+          { email },
+          password,
+          auditLog,
+          {
+            enforceHIBPCheck: !fromKnownDevice,
+          },
+          function (error, user, isPasswordReused) {
+            if (error != null) {
+              if (error instanceof ParallelLoginError) {
+                return done(null, false, { status: 429 })
+              } else if (error instanceof PasswordReusedError) {
+                const text = `${req.i18n
+                  .translate(
+                    'password_compromised_try_again_or_use_known_device_or_reset'
                   )
-                }
-
-                // async actions
-                done(null, user)
-              } else {
-                AuthenticationController._recordFailedLogin()
-                logger.debug({ email }, 'failed log in')
-                done(null, false, {
+                  .replace('<0>', '')
+                  .replace('</0>', ' (https://haveibeenpwned.com/passwords)')
+                  .replace('<1>', '')
+                  .replace(
+                    '</1>',
+                    ` (${Settings.siteUrl}/user/password/reset)`
+                  )}.`
+                return done(null, false, {
+                  status: 400,
                   type: 'error',
-                  key: 'invalid-password-retry-or-reset',
-                  status: 401,
+                  key: 'password-compromised',
+                  text,
                 })
               }
+              return done(error)
             }
-          )
-        })
+            if (
+              user &&
+              AuthenticationController.captchaRequiredForLogin(req, user)
+            ) {
+              done(null, false, {
+                text: req.i18n.translate('cannot_verify_user_not_robot'),
+                type: 'error',
+                errorReason: 'cannot_verify_user_not_robot',
+                status: 400,
+              })
+            } else if (user) {
+              if (
+                isPasswordReused &&
+                AuthenticationController.getRedirectFromSession(req) == null
+              ) {
+                AuthenticationController.setRedirectInSession(
+                  req,
+                  '/compromised-password'
+                )
+              }
+
+              // async actions
+              done(null, user)
+            } else {
+              AuthenticationController._recordFailedLogin()
+              logger.debug({ email }, 'failed log in')
+              done(null, false, {
+                type: 'error',
+                key: 'invalid-password-retry-or-reset',
+                status: 401,
+              })
+            }
+          }
+        )
       }
     )
   },

services/web/app/src/Features/Security/LoginRateLimiter.js

@@ -1,14 +1,18 @@
 const { RateLimiter } = require('../../infrastructure/RateLimiter')
 const { promisifyAll } = require('@overleaf/promise-utils')
+const Settings = require('@overleaf/settings')
 
-const rateLimiter = new RateLimiter('login', {
-  points: 10,
-  duration: 120,
-})
+const rateLimiterLoginEmail = new RateLimiter(
+  'login',
+  Settings.rateLimit?.login?.email || {
+    points: 10,
+    duration: 120,
+  }
+)
 
 function processLoginRequest(email, callback) {
-  rateLimiter
-    .consume(email, 1, { method: 'email' })
+  rateLimiterLoginEmail
+    .consume(email.trim().toLowerCase(), 1, { method: 'email' })
     .then(() => {
       callback(null, true)
     })
@@ -22,7 +26,7 @@ function processLoginRequest(email, callback) {
 }
 
 function recordSuccessfulLogin(email, callback) {
-  rateLimiter
+  rateLimiterLoginEmail
     .delete(email)
     .then(() => {
       callback()

services/web/app/src/Features/Security/RateLimiterMiddleware.js

@@ -56,7 +56,7 @@ function rateLimit(rateLimiter, opts = {}) {
   }
 }
 
-function loginRateLimit(req, res, next) {
+function loginRateLimitEmail(req, res, next) {
   const { email } = req.body
   if (!email) {
     return next()
@@ -83,7 +83,7 @@ function loginRateLimit(req, res, next) {
 
 const RateLimiterMiddleware = {
   rateLimit,
-  loginRateLimit,
+  loginRateLimitEmail,
 }
 
 module.exports = RateLimiterMiddleware

services/web/app/src/infrastructure/RateLimiter.js

@@ -120,11 +120,14 @@ const openProjectRateLimiter = new RateLimiter('open-project', {
 })
 
 // Keep in sync with the can-skip-captcha options.
-const overleafLoginRateLimiter = new RateLimiter('overleaf-login', {
-  points: 20,
-  subnetPoints: 200,
-  duration: 60,
-})
+const overleafLoginRateLimiter = new RateLimiter(
+  'overleaf-login',
+  Settings.rateLimit?.login?.ip || {
+    points: 20,
+    subnetPoints: 200,
+    duration: 60,
+  }
+)
 
 module.exports = {
   RateLimiter,

services/web/app/src/router.js

@@ -41,6 +41,7 @@ const Modules = require('./infrastructure/Modules')
 const {
   RateLimiter,
   openProjectRateLimiter,
+  overleafLoginRateLimiter,
 } = require('./infrastructure/RateLimiter')
 const RateLimiterMiddleware = require('./Features/Security/RateLimiterMiddleware')
 const InactiveProjectController = require('./Features/InactiveData/InactiveProjectController')
@@ -221,6 +222,8 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
 
   webRouter.post(
     '/login',
+    RateLimiterMiddleware.rateLimit(overleafLoginRateLimiter), // rate limit IP (20 / 60s)
+    RateLimiterMiddleware.loginRateLimitEmail, // rate limit email (10 / 120s)
     CaptchaMiddleware.validateCaptcha('login'),
     AuthenticationController.passportLogin
   )
@@ -238,6 +241,8 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
     webRouter.get('/login/legacy', UserPagesController.loginPage)
     webRouter.post(
       '/login/legacy',
+      RateLimiterMiddleware.rateLimit(overleafLoginRateLimiter), // rate limit IP (20 / 60s)
+      RateLimiterMiddleware.loginRateLimitEmail, // rate limit email (10 / 120s)
       CaptchaMiddleware.validateCaptcha('login'),
       AuthenticationController.passportLogin
     )

services/web/config/settings.defaults.js

@@ -727,6 +727,10 @@ module.exports = {
       everyone: process.env.RATE_LIMIT_AUTO_COMPILE_EVERYONE || 100,
       standard: process.env.RATE_LIMIT_AUTO_COMPILE_STANDARD || 25,
     },
+    login: {
+      ip: { points: 20, subnetPoints: 200, duration: 60 },
+      email: { points: 10, duration: 120 },
+    },
   },
 
   analytics: {

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

@@ -107,4 +107,45 @@ describe('Authentication', function () {
       expect(auditLogEntry.ipAddress).to.equal('127.0.0.1')
     })
   })
+
+  describe('rate-limit', function () {
+    beforeEach('fetchCsrfToken', async function () {
+      await user.login()
+      await user.logout()
+      await user.getCsrfToken()
+    })
+    const tryLogin = async (i = 0) => {
+      const {
+        response: { statusCode },
+      } = await user.doRequest('POST', {
+        url: Settings.enableLegacyLogin ? '/login/legacy' : '/login',
+        json: {
+          email: `${user.email}${' '.repeat(i)}`,
+          password: 'wrong-password',
+          'g-recaptcha-response': 'valid',
+        },
+      })
+      return statusCode
+    }
+    it('should return 429 after 10 unsuccessful login attempts', async function () {
+      for (let i = 0; i < 10; i++) {
+        const statusCode = await tryLogin()
+        expect(statusCode).to.equal(401)
+      }
+      for (let i = 0; i < 10; i++) {
+        const statusCode = await tryLogin()
+        expect(statusCode).to.equal(429)
+      }
+    })
+    it('ignore extra spaces in email address', async function () {
+      for (let i = 0; i < 10; i++) {
+        const statusCode = await tryLogin(i)
+        expect(statusCode).to.equal(401)
+      }
+      for (let i = 0; i < 10; i++) {
+        const statusCode = await tryLogin(i)
+        expect(statusCode).to.equal(429)
+      }
+    })
+  })
 })

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

@@ -388,24 +388,6 @@ describe('AuthenticationController', function () {
       })
     })
 
-    describe('when the users rate limit', function () {
-      beforeEach(function () {
-        this.LoginRateLimiter.processLoginRequest.yields(null, false)
-      })
-
-      it('should block the request if the limit has been exceeded', function (done) {
-        this.AuthenticationController.doPassportLogin(
-          this.req,
-          this.req.body.email,
-          this.req.body.password,
-          this.cb
-        )
-        this.cb.callCount.should.equal(1)
-        this.cb.calledWith(null, null).should.equal(true)
-        done()
-      })
-    })
-
     describe('when the user is authenticated', function () {
       beforeEach(function () {
         this.cb = sinon.stub()