From e4cbb3d24d278d99c0de7290c14847ae55f59829 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Thu, 1 Sep 2022 08:54:14 -0400 Subject: [PATCH] Reset magic link rate limiting upon successful login (#15345) refs https://github.com/TryGhost/Team/issues/1771 We don't have access to `req.brute.reset` due to the way the flow works, we have one endpoint which sends an email with a magic link, and another route which handles the login. We don't want to apply brute force protection to both because our rate limiting is designed for API requests not web page visits (which is how login is handled). Because of this we require access to the underlying ExpressBrute instance exposed by the spam-protection module, so that we can perform the reset. --- .../server/services/members/middleware.js | 2 + .../shared/middleware/api/spam-prevention.js | 32 +++++++++ .../server/web/shared/middleware/brute.js | 2 +- .../core/test/e2e-api/members/signin.test.js | 72 ++++++++++++++++++- .../services/members/middleware.test.js | 6 +- 5 files changed, 109 insertions(+), 5 deletions(-) diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index 6944564aa2..f898a547f8 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -3,6 +3,7 @@ const logging = require('@tryghost/logging'); const membersService = require('./service'); const models = require('../../models'); const urlUtils = require('../../../shared/url-utils'); +const spamPrevention = require('../../web/shared/middleware/api/spam-prevention'); const {formattedMemberResponse} = require('./utils'); // @TODO: This piece of middleware actually belongs to the frontend, not to the member app @@ -155,6 +156,7 @@ const createSessionFromMagicLink = async function (req, res, next) { try { const member = await membersService.ssr.exchangeTokenForSession(req, res); + spamPrevention.membersAuth().reset(req.ip, `${member.email}login`); const subscriptions = member && member.subscriptions || []; const action = req.query.action; diff --git a/ghost/core/core/server/web/shared/middleware/api/spam-prevention.js b/ghost/core/core/server/web/shared/middleware/api/spam-prevention.js index 82f1a32631..80525a2556 100644 --- a/ghost/core/core/server/web/shared/middleware/api/spam-prevention.js +++ b/ghost/core/core/server/web/shared/middleware/api/spam-prevention.js @@ -36,6 +36,7 @@ let privateBlogInstance; let globalResetInstance; let globalBlockInstance; let userLoginInstance; +let membersAuthInstance; let userResetInstance; let contentApiKeyInstance; @@ -121,6 +122,36 @@ const globalReset = () => { return globalResetInstance; }; +const membersAuth = () => { + const ExpressBrute = require('express-brute'); + const BruteKnex = require('brute-knex'); + const db = require('../../../../data/db'); + + store = store || new BruteKnex({ + tablename: 'brute', + createTable: false, + knex: db.knex + }); + + if (!membersAuthInstance) { + membersAuthInstance = new ExpressBrute(store, + extend({ + attachResetToRequest: true, + failCallback(req, res, next, nextValidRequestDate) { + return next(new errors.TooManyRequestsError({ + message: `Too many sign-in attempts try again in ${moment(nextValidRequestDate).fromNow(true)}`, + context: tpl(messages.tooManySigninAttempts.context), + help: tpl(messages.tooManySigninAttempts.context) + })); + }, + handleStoreError: handleStoreError + }, pick(spamUserLogin, spamConfigKeys)) + ); + } + + return membersAuthInstance; +}; + // Stops login attempts for a user+IP pair with an increasing time period starting from 10 minutes // and rising to a week in a fibonnaci sequence // The user+IP count is reset when on successful login @@ -249,6 +280,7 @@ module.exports = { globalBlock: globalBlock, globalReset: globalReset, userLogin: userLogin, + membersAuth: membersAuth, userReset: userReset, privateBlog: privateBlog, contentApiKey: contentApiKey diff --git a/ghost/core/core/server/web/shared/middleware/brute.js b/ghost/core/core/server/web/shared/middleware/brute.js index a1b35146e7..6e83f9f2c0 100644 --- a/ghost/core/core/server/web/shared/middleware/brute.js +++ b/ghost/core/core/server/web/shared/middleware/brute.js @@ -86,7 +86,7 @@ module.exports = { /** */ membersAuth(req, res, next) { - return spamPrevention.userLogin().getMiddleware({ + return spamPrevention.membersAuth().getMiddleware({ ignoreIP: false, key(_req, _res, _next) { if (_req.body.email) { diff --git a/ghost/core/test/e2e-api/members/signin.test.js b/ghost/core/test/e2e-api/members/signin.test.js index 9ffe2f148c..4fec2f5f90 100644 --- a/ghost/core/test/e2e-api/members/signin.test.js +++ b/ghost/core/test/e2e-api/members/signin.test.js @@ -1,4 +1,4 @@ -const {agentProvider, mockManager, fixtureManager} = require('../../utils/e2e-framework'); +const {agentProvider, mockManager, fixtureManager, dbUtils, configUtils} = require('../../utils/e2e-framework'); const models = require('../../../core/server/models'); const assert = require('assert'); require('should'); @@ -33,6 +33,7 @@ describe('Members Signin', function () { }); beforeEach(function () { + mockManager.mockMail(); mockManager.mockStripe(); }); @@ -119,6 +120,75 @@ describe('Members Signin', function () { }); }); + describe('Rate limiting', function () { + it('Will clear rate limits for members auth', async function () { + await dbUtils.truncate('brute'); + // +1 because this is a retry count, so we have one request + the retries, then blocked + const userLoginRateLimit = configUtils.config.get('spam').user_login.freeRetries + 1; + + for (let i = 0; i < userLoginRateLimit; i++) { + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-1@test.com', + emailType: 'signup' + }); + + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-2@test.com', + emailType: 'signup' + }); + } + + // Now we've been rate limited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-1@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Now we've been rate limited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-2@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Get one of the magic link emails + const mail = mockManager.assert.sentEmail({ + to: 'rate-limiting-test-1@test.com', + subject: /Complete your sign up to Ghost!/ + }); + + // Get link from email + const [url] = mail.text.match(/https?:\/\/[^\s]+/); + + const magicLink = new URL(url); + + // Login + await membersAgent.get(magicLink.pathname + magicLink.search); + + // The first member has been un ratelimited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-1@test.com', + emailType: 'signup' + }) + .expectEmptyBody() + .expectStatus(201); + + // The second is still rate limited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-2@test.com', + emailType: 'signup' + }) + .expectStatus(429); + }); + }); + describe('Member attribution', function () { it('Will create a member attribution if magic link contains an attribution source', async function () { const email = 'non-existent-member@test.com'; diff --git a/ghost/core/test/unit/server/services/members/middleware.test.js b/ghost/core/test/unit/server/services/members/middleware.test.js index 55f350fdf7..b5a6860458 100644 --- a/ghost/core/test/unit/server/services/members/middleware.test.js +++ b/ghost/core/test/unit/server/services/members/middleware.test.js @@ -107,7 +107,7 @@ describe('Members Service Middleware', function () { req.query = {token: 'test', action: 'signup'}; // Fake token handling failure - membersService.ssr.exchangeTokenForSession.resolves(); + membersService.ssr.exchangeTokenForSession.resolves({}); // Fake welcome page for free tier models.Product.findOne.resolves({ @@ -153,7 +153,7 @@ describe('Members Service Middleware', function () { req.query = {token: 'test', action: 'signin', r: 'https://site.com/blah/my-post/'}; // Fake token handling failure - membersService.ssr.exchangeTokenForSession.resolves(); + membersService.ssr.exchangeTokenForSession.resolves({}); // Call the middleware await membersMiddleware.createSessionFromMagicLink(req, res, next); @@ -169,7 +169,7 @@ describe('Members Service Middleware', function () { req.query = {token: 'test', action: 'signin', r: 'https://external.com/whatever/'}; // Fake token handling failure - membersService.ssr.exchangeTokenForSession.resolves(); + membersService.ssr.exchangeTokenForSession.resolves({}); // Call the middleware await membersMiddleware.createSessionFromMagicLink(req, res, next);