diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index ffc7169bd3..2a0cd5aa30 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/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('', ' (https://haveibeenpwned.com/passwords)') - .replace('<1>', '') - .replace( - '', - ` (${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('', ' (https://haveibeenpwned.com/passwords)') + .replace('<1>', '') + .replace( + '', + ` (${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, + }) + } + } + ) } ) }, diff --git a/services/web/app/src/Features/Security/LoginRateLimiter.js b/services/web/app/src/Features/Security/LoginRateLimiter.js index a80ec3285a..58a351d20a 100644 --- a/services/web/app/src/Features/Security/LoginRateLimiter.js +++ b/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() diff --git a/services/web/app/src/Features/Security/RateLimiterMiddleware.js b/services/web/app/src/Features/Security/RateLimiterMiddleware.js index 6a99ac610b..6398916f60 100644 --- a/services/web/app/src/Features/Security/RateLimiterMiddleware.js +++ b/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 diff --git a/services/web/app/src/infrastructure/RateLimiter.js b/services/web/app/src/infrastructure/RateLimiter.js index 1989b09b10..c3e863baf7 100644 --- a/services/web/app/src/infrastructure/RateLimiter.js +++ b/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, diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 291b214d7c..560c1b48c4 100644 --- a/services/web/app/src/router.js +++ b/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 ) diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index cad13ab815..e96178fd0a 100644 --- a/services/web/config/settings.defaults.js +++ b/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: { diff --git a/services/web/test/acceptance/src/AuthenticationTests.js b/services/web/test/acceptance/src/AuthenticationTests.js index ba56440708..f1523f01cb 100644 --- a/services/web/test/acceptance/src/AuthenticationTests.js +++ b/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) + } + }) + }) }) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 002de7eea5..2390981a44 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/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()