From c9f782a3fc3c2744197f0c561b759b72e0b14007 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 31 Aug 2022 10:33:42 -0400 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=94=92=20Fixed=20rate=20limiting=20fo?= =?UTF-8?q?r=20user=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 + }); + }); +}); From c4041e46c8722be149fcf0342649f1ad6f9df2e8 Mon Sep 17 00:00:00 2001 From: Rishabh Garg Date: Thu, 1 Sep 2022 20:00:37 +0530 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20email=20alerts=20for?= =?UTF-8?q?=20paid=20members=20on=20import=20(#15347)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Team/issues/1868 - email alerts should not be sent out when paid subscriptions are created via our importer --- ghost/members-api/lib/repositories/member.js | 13 +- .../test/unit/lib/repositories/member.test.js | 217 +++++++++++++++++- 2 files changed, 224 insertions(+), 6 deletions(-) diff --git a/ghost/members-api/lib/repositories/member.js b/ghost/members-api/lib/repositories/member.js index eeeed52406..b9e075ba97 100644 --- a/ghost/members-api/lib/repositories/member.js +++ b/ghost/members-api/lib/repositories/member.js @@ -407,7 +407,7 @@ module.exports = class MemberRepository { productsToAdd = _.differenceWith(incomingProductIds, existingProductIds); productsToRemove = _.differenceWith(existingProductIds, incomingProductIds); const productsToModify = productsToAdd.concat(productsToRemove); - + if (productsToModify.length !== 0) { // Load active subscriptions information await initialMember.load( @@ -417,9 +417,9 @@ module.exports = class MemberRepository { 'stripeSubscriptions.stripePrice.stripeProduct', 'stripeSubscriptions.stripePrice.stripeProduct.product' ], sharedOptions); - + const exisitingSubscriptions = initialMember.related('stripeSubscriptions')?.models ?? []; - + if (productsToRemove.length > 0) { // Only allow to delete comped products without a subscription attached to them // Other products should be removed by canceling them via the related stripe subscription @@ -445,7 +445,7 @@ module.exports = class MemberRepository { const existingActiveSubscriptions = exisitingSubscriptions.filter((subscription) => { return this.isActiveSubscriptionStatus(subscription.get('status')); }); - + if (existingActiveSubscriptions.length) { throw new errors.BadRequestError({message: tpl(messages.addProductWithActiveSubscription)}); } @@ -964,8 +964,11 @@ module.exports = class MemberRepository { ...eventData }, options); + const context = options?.context || {}; + const source = this._resolveContextSource(context); + // Notify paid member subscription start - if (this._labsService.isSet('emailAlerts')) { + if (this._labsService.isSet('emailAlerts') && ['member', 'api'].includes(source)) { await this.staffService.notifyPaidSubscriptionStart({ member: member.toJSON(), offer: offer ? this._offerRepository.toJSON(offer) : null, diff --git a/ghost/members-api/test/unit/lib/repositories/member.test.js b/ghost/members-api/test/unit/lib/repositories/member.test.js index 548d9f0254..0de52cf60d 100644 --- a/ghost/members-api/test/unit/lib/repositories/member.test.js +++ b/ghost/members-api/test/unit/lib/repositories/member.test.js @@ -1,4 +1,5 @@ const assert = require('assert'); +const sinon = require('sinon'); const MemberRepository = require('../../../../lib/repositories/member'); describe('MemberRepository', function () { @@ -33,7 +34,7 @@ describe('MemberRepository', function () { api_key: true }); assert.equal(source, 'api'); - + source = repo._resolveContextSource({ api_key: true }); @@ -49,4 +50,218 @@ describe('MemberRepository', function () { assert.equal(source, 'member'); }); }); + + describe('linkSubscription', function (){ + let Member; + let staffService; + let notifySpy; + let MemberPaidSubscriptionEvent; + let StripeCustomerSubscription; + let MemberProductEvent; + let stripeAPIService; + let productRepository; + let labsService; + let subscriptionData; + + beforeEach(async function () { + notifySpy = sinon.spy(); + subscriptionData = { + id: 'sub_123', + customer: 'cus_123', + status: 'active', + items: { + type: 'list', + data: [{ + id: 'item_123', + price: { + id: 'price_123', + product: 'product_123', + active: true, + nickname: 'Monthly', + currency: 'usd', + recurring: { + interval: 'month' + }, + unit_amount: 500, + type: 'recurring' + } + }] + }, + start_date: Date.now() / 1000, + current_period_end: Date.now() / 1000 + (60 * 60 * 24 * 31), + cancel_at_period_end: false + }; + + Member = { + findOne: sinon.stub().resolves({ + related: () => { + return { + query: sinon.stub().returns({ + fetchOne: sinon.stub().resolves({}) + }), + toJSON: sinon.stub().returns([]), + fetch: sinon.stub().resolves({ + toJSON: sinon.stub().returns({}) + }) + }; + }, + toJSON: sinon.stub().returns({}) + }), + edit: sinon.stub().resolves({ + attributes: {}, + _previousAttributes: {} + }) + }; + staffService = { + notifyPaidSubscriptionStart: notifySpy + }; + MemberPaidSubscriptionEvent = { + add: sinon.stub().resolves() + }; + StripeCustomerSubscription = { + add: sinon.stub().resolves({ + get: sinon.stub().returns() + }) + }; + MemberProductEvent = { + add: sinon.stub().resolves({}) + }; + + stripeAPIService = { + configured: true, + getSubscription: sinon.stub().resolves(subscriptionData) + }; + + productRepository = { + get: sinon.stub().resolves({ + get: sinon.stub().returns(), + toJSON: sinon.stub().returns({}) + }), + update: sinon.stub().resolves({}) + }; + + labsService = { + isSet: sinon.stub().returns(true) + }; + }); + + it('triggers email alert for member context', async function (){ + const repo = new MemberRepository({ + stripeAPIService, + StripeCustomerSubscription, + MemberPaidSubscriptionEvent, + MemberProductEvent, + staffService, + productRepository, + labsService, + Member + }); + + sinon.stub(repo, 'getSubscriptionByStripeID').resolves(null); + + await repo.linkSubscription({ + subscription: subscriptionData + }, { + transacting: true, + context: {} + }); + notifySpy.calledOnce.should.be.true(); + }); + + it('triggers email alert for api context', async function (){ + const repo = new MemberRepository({ + stripeAPIService, + StripeCustomerSubscription, + MemberPaidSubscriptionEvent, + MemberProductEvent, + staffService, + productRepository, + labsService, + Member + }); + + sinon.stub(repo, 'getSubscriptionByStripeID').resolves(null); + + await repo.linkSubscription({ + subscription: subscriptionData + }, { + transacting: true, + context: {api_key: 'abc'} + }); + notifySpy.calledOnce.should.be.true(); + }); + + it('does not trigger email alert for importer context', async function (){ + const repo = new MemberRepository({ + stripeAPIService, + StripeCustomerSubscription, + MemberPaidSubscriptionEvent, + MemberProductEvent, + staffService, + productRepository, + labsService, + Member + }); + + sinon.stub(repo, 'getSubscriptionByStripeID').resolves(null); + + await repo.linkSubscription({ + subscription: subscriptionData + }, { + transacting: true, + context: {importer: true} + }); + notifySpy.calledOnce.should.be.false(); + }); + + it('does not trigger email alert for admin context', async function (){ + const repo = new MemberRepository({ + stripeAPIService, + StripeCustomerSubscription, + MemberPaidSubscriptionEvent, + MemberProductEvent, + staffService, + productRepository, + labsService, + Member + }); + + sinon.stub(repo, 'getSubscriptionByStripeID').resolves(null); + + await repo.linkSubscription({ + subscription: subscriptionData + }, { + transacting: true, + context: {user: {}} + }); + notifySpy.calledOnce.should.be.false(); + }); + + it('does not trigger email alert for internal context', async function (){ + const repo = new MemberRepository({ + stripeAPIService, + StripeCustomerSubscription, + MemberPaidSubscriptionEvent, + MemberProductEvent, + staffService, + productRepository, + labsService, + Member + }); + + sinon.stub(repo, 'getSubscriptionByStripeID').resolves(null); + + await repo.linkSubscription({ + subscription: subscriptionData + }, { + transacting: true, + context: {internal: true} + }); + notifySpy.calledOnce.should.be.false(); + }); + + afterEach(function () { + sinon.restore(); + }); + }); }); From 7650ecafebd9505654cb8cf43f0d31c0647bad7c Mon Sep 17 00:00:00 2001 From: Ghost CI <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 1 Sep 2022 15:36:17 +0100 Subject: [PATCH 3/3] v5.12.3 --- ghost/admin/package.json | 2 +- ghost/core/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ghost/admin/package.json b/ghost/admin/package.json index f7411dc4fa..262a16b31e 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.12.2", + "version": "5.12.3", "description": "Ember.js admin client for Ghost", "author": "Ghost Foundation", "homepage": "http://ghost.org", diff --git a/ghost/core/package.json b/ghost/core/package.json index c40140ffe9..5c80eced01 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.12.2", + "version": "5.12.3", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org",