diff --git a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js index fa5c3d12e5..2bf2877345 100644 --- a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js +++ b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js @@ -24,11 +24,7 @@ module.exports = CaptchaMiddleware = { ) { return next() } - const inviteAndCaptchaDisabled = - action === 'invite' && Settings.recaptcha.disabled.invite - const registerAndCaptchaDisabled = - action === 'register' && Settings.recaptcha.disabled.register - if (inviteAndCaptchaDisabled || registerAndCaptchaDisabled) { + if (Settings.recaptcha.disabled[action]) { return next() } const response = req.body['g-recaptcha-response'] diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 07071e602c..5484e5f98b 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -1,5 +1,4 @@ const PasswordResetHandler = require('./PasswordResetHandler') -const RateLimiter = require('../../infrastructure/RateLimiter') const AuthenticationController = require('../Authentication/AuthenticationController') const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') @@ -61,44 +60,31 @@ module.exports = { }, requestReset(req, res, next) { - const email = req.body.email.trim().toLowerCase() - const opts = { - endpointName: 'password_reset_rate_limit', - timeInterval: 60, - subjectName: req.ip, - throttle: 6, + const email = EmailsHelper.parseEmail(req.body.email) + if (!email) { + return res.status(400).send({ + message: req.i18n.translate('must_be_email_address'), + }) } - RateLimiter.addCount(opts, (err, canContinue) => { + PasswordResetHandler.generateAndEmailResetToken(email, (err, status) => { if (err != null) { - return next( - new OError('rate-limit password reset failed').withCause(err) - ) - } - if (!canContinue) { - return res.status(429).send({ - message: req.i18n.translate('rate_limit_hit_wait'), + OError.tag(err, 'failed to generate and email password reset token', { + email, + }) + next(err) + } else if (status === 'primary') { + res.status(200).send({ + message: { text: req.i18n.translate('password_reset_email_sent') }, + }) + } else if (status === 'secondary') { + res.status(404).send({ + message: req.i18n.translate('secondary_email_password_reset'), + }) + } else { + res.status(404).send({ + message: req.i18n.translate('cant_find_email'), }) } - PasswordResetHandler.generateAndEmailResetToken(email, (err, status) => { - if (err != null) { - OError.tag(err, 'failed to generate and email password reset token', { - email, - }) - next(err) - } else if (status === 'primary') { - res.status(200).send({ - message: { text: req.i18n.translate('password_reset_email_sent') }, - }) - } else if (status === 'secondary') { - res.status(404).send({ - message: req.i18n.translate('secondary_email_password_reset'), - }) - } else { - res.status(404).send({ - message: req.i18n.translate('cant_find_email'), - }) - } - }) }) }, diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js index f618998e87..1fed0c8fe7 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js @@ -1,9 +1,18 @@ const PasswordResetController = require('./PasswordResetController') const AuthenticationController = require('../Authentication/AuthenticationController') +const CaptchaMiddleware = require('../../Features/Captcha/CaptchaMiddleware') +const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const { Joi, validate } = require('../../infrastructure/Validation') module.exports = { apply(webRouter, apiRouter) { + const rateLimit = RateLimiterMiddleware.rateLimit({ + endpointName: 'password_reset_rate_limit', + ipOnly: true, + maxRequests: 6, + timeInterval: 60, + }) + webRouter.get( '/user/password/reset', PasswordResetController.renderRequestResetForm @@ -15,6 +24,8 @@ module.exports = { email: Joi.string().required(), }), }), + rateLimit, + CaptchaMiddleware.validateCaptcha('passwordReset'), PasswordResetController.requestReset ) AuthenticationController.addEndpointToLoginWhitelist('/user/password/reset') @@ -31,6 +42,7 @@ module.exports = { passwordResetToken: Joi.string().required(), }), }), + rateLimit, PasswordResetController.setNewUserPassword ) AuthenticationController.addEndpointToLoginWhitelist('/user/password/set') @@ -42,6 +54,8 @@ module.exports = { email: Joi.string().required(), }), }), + rateLimit, + CaptchaMiddleware.validateCaptcha('passwordReset'), PasswordResetController.requestReset ) }, diff --git a/services/web/app/views/user/passwordReset.pug b/services/web/app/views/user/passwordReset.pug index 38c9922ba7..cd8075193d 100644 --- a/services/web/app/views/user/passwordReset.pug +++ b/services/web/app/views/user/passwordReset.pug @@ -4,6 +4,18 @@ block vars - metadata = { viewport: true } block content + - var showCaptcha = settings.recaptcha && settings.recaptcha.siteKey && !(settings.recaptcha.disabled && settings.recaptcha.disabled.passwordReset) + + if showCaptcha + script(type="text/javascript", nonce=scriptNonce, src="https://www.recaptcha.net/recaptcha/api.js?render=explicit") + div( + id="recaptcha" + class="g-recaptcha" + data-sitekey=settings.recaptcha.siteKey + data-size="invisible" + data-badge="inline" + ) + main.content.content-alt .container .row @@ -17,6 +29,8 @@ block content name="passwordResetForm" action="/user/password/reset", method="POST", + captcha=(showCaptcha ? '' : false), + captcha-action-name=(showCaptcha ? "passwordReset" : false), ng-cloak ) input(type="hidden", name="_csrf", value=csrfToken) diff --git a/services/web/app/views/user/reconfirm.pug b/services/web/app/views/user/reconfirm.pug index 6fca34e5b0..e91cf4c10a 100644 --- a/services/web/app/views/user/reconfirm.pug +++ b/services/web/app/views/user/reconfirm.pug @@ -2,6 +2,18 @@ extends ../layout block content - var email = reconfirm_email ? reconfirm_email : "" + - var showCaptcha = settings.recaptcha && settings.recaptcha.siteKey && !(settings.recaptcha.disabled && settings.recaptcha.disabled.passwordReset) + + if showCaptcha + script(type="text/javascript", nonce=scriptNonce, src="https://www.recaptcha.net/recaptcha/api.js?render=explicit") + div( + id="recaptcha" + class="g-recaptcha" + data-sitekey=settings.recaptcha.siteKey + data-size="invisible" + data-badge="inline" + ) + main.content.content-alt .container .row @@ -19,6 +31,8 @@ block content ng-cloak ng-init="email='"+email+"'" aria-label=translate('request_reconfirmation_email') + captcha=(showCaptcha ? '' : false), + captcha-action-name=(showCaptcha ? "passwordReset" : false), ) input(type="hidden", name="_csrf", value=csrfToken) form-messages(for="reconfirmAccountForm" role="alert") diff --git a/services/web/app/views/user/setPassword.pug b/services/web/app/views/user/setPassword.pug index 554030731a..a0e1541783 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -32,6 +32,8 @@ block content | Request a new password reset email .alert.alert-danger(ng-switch-when="400") | #{translate('invalid_password')} + .alert.alert-danger(ng-switch-when="429") + | #{translate('rate_limit_hit_wait')} .alert.alert-danger(ng-switch-default) | #{translate('error_performing_request')} diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index d5ca69b101..cc3f3438b8 100644 --- a/services/web/config/settings.defaults.js +++ b/services/web/config/settings.defaults.js @@ -558,6 +558,7 @@ module.exports = { disabled: { invite: true, login: true, + passwordReset: true, register: true, }, }, diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index e378ea93ed..d8ceae03aa 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js @@ -38,7 +38,6 @@ describe('PasswordResetController', function () { .resolves({ found: true, reset: true, userID: this.user_id }), }, } - this.RateLimiter = { addCount: sinon.stub() } this.UserSessionsManager = { promises: { revokeAllUserSessions: sinon.stub().resolves(), @@ -53,7 +52,6 @@ describe('PasswordResetController', function () { requires: { '@overleaf/settings': this.settings, './PasswordResetHandler': this.PasswordResetHandler, - '../../infrastructure/RateLimiter': this.RateLimiter, '../Authentication/AuthenticationController': (this.AuthenticationController = { getLoggedInUserId: sinon.stub(), finishLogin: sinon.stub(), @@ -70,23 +68,7 @@ describe('PasswordResetController', function () { }) describe('requestReset', function () { - it('should error if the rate limit is hit', function (done) { - this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( - 1, - null, - 'primary' - ) - this.RateLimiter.addCount.callsArgWith(1, null, false) - this.PasswordResetController.requestReset(this.req, this.res) - this.PasswordResetHandler.generateAndEmailResetToken - .calledWith(this.email) - .should.equal(false) - this.res.statusCode.should.equal(429) - done() - }) - it('should tell the handler to process that email', function (done) { - this.RateLimiter.addCount.callsArgWith(1, null, true) this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( 1, null, @@ -101,7 +83,6 @@ describe('PasswordResetController', function () { }) it('should send a 500 if there is an error', function (done) { - this.RateLimiter.addCount.callsArgWith(1, null, true) this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( 1, new Error('error') @@ -113,7 +94,6 @@ describe('PasswordResetController', function () { }) it("should send a 404 if the email doesn't exist", function (done) { - this.RateLimiter.addCount.callsArgWith(1, null, true) this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( 1, null, @@ -125,7 +105,6 @@ describe('PasswordResetController', function () { }) it('should send a 404 if the email is registered as a secondard email', function (done) { - this.RateLimiter.addCount.callsArgWith(1, null, true) this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( 1, null, @@ -139,7 +118,6 @@ describe('PasswordResetController', function () { it('should normalize the email address', function (done) { this.email = ' UPperCaseEMAILWithSpacesAround@example.Com ' this.req.body.email = this.email - this.RateLimiter.addCount.callsArgWith(1, null, true) this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith( 1, null,