From 2ff81cc5d30bbf2773e60fb1f257b23bdfe4c688 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 31 Aug 2022 10:33:42 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Fixed=20rate=20limiting=20for=20?= =?UTF-8?q?user=20login=20(#15336)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/1074 Rather than relying on the global block to stop malicious actors from enumerating email addresses to determine who is and isn't a user, we want our user login brute force protection to be on an IP basis, rather than tied to the username. --- .../server/web/shared/middleware/brute.js | 17 +---- .../__snapshots__/rate-limiting.test.js.snap | 25 +++++++ .../test/e2e-api/admin/rate-limiting.test.js | 74 +++++++++++++++++++ 3 files changed, 101 insertions(+), 15 deletions(-) create mode 100644 ghost/core/test/e2e-api/admin/__snapshots__/rate-limiting.test.js.snap create mode 100644 ghost/core/test/e2e-api/admin/rate-limiting.test.js diff --git a/ghost/core/core/server/web/shared/middleware/brute.js b/ghost/core/core/server/web/shared/middleware/brute.js index 1b0ced21e3..a1b35146e7 100644 --- a/ghost/core/core/server/web/shared/middleware/brute.js +++ b/ghost/core/core/server/web/shared/middleware/brute.js @@ -29,26 +29,13 @@ module.exports = { })(req, res, next); }, /** - * block per user - * username === email! + * block per ip */ userLogin(req, res, next) { return spamPrevention.userLogin().getMiddleware({ ignoreIP: false, key(_req, _res, _next) { - if (_req.body.username) { - return _next(`${_req.body.username}login`); - } - - if (_req.body.authorizationCode) { - return _next(`${_req.body.authorizationCode}login`); - } - - if (_req.body.refresh_token) { - return _next(`${_req.body.refresh_token}login`); - } - - return _next(); + return _next('user_login'); } })(req, res, next); }, diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/rate-limiting.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/rate-limiting.test.js.snap new file mode 100644 index 0000000000..4730b7bbd4 --- /dev/null +++ b/ghost/core/test/e2e-api/admin/__snapshots__/rate-limiting.test.js.snap @@ -0,0 +1,25 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Sessions API Is rate limited to protect against brute forcing a users password 1: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "286", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Sessions API Is rate limited to protect against brute forcing whether a user exists 1: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "286", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; diff --git a/ghost/core/test/e2e-api/admin/rate-limiting.test.js b/ghost/core/test/e2e-api/admin/rate-limiting.test.js new file mode 100644 index 0000000000..5bba93fdb5 --- /dev/null +++ b/ghost/core/test/e2e-api/admin/rate-limiting.test.js @@ -0,0 +1,74 @@ +const { + agentProvider, + fixtureManager, + matchers: { + anyEtag + }, + dbUtils, + configUtils +} = require('../../utils/e2e-framework'); + +describe('Sessions API', function () { + let agent; + + before(async function () { + agent = await agentProvider.getAdminAPIAgent(); + await fixtureManager.init(); + }); + + it('Is rate limited to protect against brute forcing a users password', 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 agent + .post('session/') + .body({ + grant_type: 'password', + username: 'user@domain.tld', + password: 'parseword' + }); + } + + await agent + .post('session/') + .body({ + grant_type: 'password', + username: 'user@domain.tld', + password: 'parseword' + }) + .expectStatus(429) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); + + it('Is rate limited to protect against brute forcing whether a user exists', 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 agent + .post('session/') + .body({ + grant_type: 'password', + username: `user+${i}@domain.tld`, + password: `parseword` + }); + } + + await agent + .post('session/') + .body({ + grant_type: 'password', + username: 'user@domain.tld', + password: 'parseword' + }) + .expectStatus(429) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); +});