diff --git a/ghost/core/core/server/api/endpoints/member-signin-urls.js b/ghost/core/core/server/api/endpoints/member-signin-urls.js index 02ea1ea3a8..b57120a7d1 100644 --- a/ghost/core/core/server/api/endpoints/member-signin-urls.js +++ b/ghost/core/core/server/api/endpoints/member-signin-urls.js @@ -22,7 +22,7 @@ module.exports = { }); } - const magicLink = await membersService.api.getMagicLink(model.get('email')); + const magicLink = await membersService.api.getMagicLink(model.get('email'), 'signin'); return { member_id: model.get('id'), diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index 13046b7e8f..2196de1cb5 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -157,6 +157,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`); + // Note: don't reset 'member_login', or that would give an easy way around user enumeration by logging in to a manually created account const subscriptions = member && member.subscriptions || []; const action = req.query.action; diff --git a/ghost/core/core/server/web/members/app.js b/ghost/core/core/server/web/members/app.js index a35a12b50c..b2e11872e5 100644 --- a/ghost/core/core/server/web/members/app.js +++ b/ghost/core/core/server/web/members/app.js @@ -48,7 +48,15 @@ module.exports = function setupMembersApp() { membersApp.delete('/api/session', middleware.deleteSession); // NOTE: this is wrapped in a function to ensure we always go via the getter - membersApp.post('/api/send-magic-link', bodyParser.json(), shared.middleware.brute.membersAuth, (req, res, next) => membersService.api.middleware.sendMagicLink(req, res, next)); + membersApp.post( + '/api/send-magic-link', + bodyParser.json(), + // Prevent brute forcing email addresses (user enumeration) + shared.middleware.brute.membersAuthEnumeration, + // Prevent brute forcing passwords for the same email address + shared.middleware.brute.membersAuth, + (req, res, next) => membersService.api.middleware.sendMagicLink(req, res, next) + ); membersApp.post('/api/create-stripe-checkout-session', (req, res, next) => membersService.api.middleware.createCheckoutSession(req, res, next)); membersApp.post('/api/create-stripe-update-session', (req, res, next) => membersService.api.middleware.createCheckoutSetupSession(req, res, next)); membersApp.put('/api/subscriptions/:id', (req, res, next) => membersService.api.middleware.updateSubscription(req, res, next)); 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 80525a2556..b98b9cc066 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 @@ -5,7 +5,7 @@ const errors = require('@tryghost/errors'); const config = require('../../../../../shared/config'); const tpl = require('@tryghost/tpl'); const logging = require('@tryghost/logging'); -const spam = config.get('spam') || {}; +let spam = config.get('spam') || {}; const messages = { forgottenPasswordEmail: { @@ -22,13 +22,13 @@ const messages = { }, tooManyAttempts: 'Too many attempts.' }; - -const spamPrivateBlock = spam.private_block || {}; -const spamGlobalBlock = spam.global_block || {}; -const spamGlobalReset = spam.global_reset || {}; -const spamUserReset = spam.user_reset || {}; -const spamUserLogin = spam.user_login || {}; -const spamContentApiKey = spam.content_api_key || {}; +let spamPrivateBlock = spam.private_block || {}; +let spamGlobalBlock = spam.global_block || {}; +let spamGlobalReset = spam.global_reset || {}; +let spamUserReset = spam.user_reset || {}; +let spamUserLogin = spam.user_login || {}; +let spamMemberLogin = spam.member_login || {}; +let spamContentApiKey = spam.content_api_key || {}; let store; let memoryStore; @@ -37,6 +37,7 @@ let globalResetInstance; let globalBlockInstance; let userLoginInstance; let membersAuthInstance; +let membersAuthEnumerationInstance; let userResetInstance; let contentApiKeyInstance; @@ -152,6 +153,39 @@ const membersAuth = () => { return membersAuthInstance; }; +/** + * This one should have higher limits because it checks across all email addresses + */ +const membersAuthEnumeration = () => { + 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 (!membersAuthEnumerationInstance) { + membersAuthEnumerationInstance = new ExpressBrute(store, + extend({ + attachResetToRequest: true, + failCallback(req, res, next, nextValidRequestDate) { + return next(new errors.TooManyRequestsError({ + message: `Too many different sign-in attempts try again in ${moment(nextValidRequestDate).fromNow(true)}`, + context: tpl(messages.tooManySigninAttempts.context), + help: tpl(messages.tooManySigninAttempts.context) + })); + }, + handleStoreError: handleStoreError + }, pick(spamMemberLogin, spamConfigKeys)) + ); + } + + return membersAuthEnumerationInstance; +}; + // 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 @@ -281,7 +315,29 @@ module.exports = { globalReset: globalReset, userLogin: userLogin, membersAuth: membersAuth, + membersAuthEnumeration: membersAuthEnumeration, userReset: userReset, privateBlog: privateBlog, - contentApiKey: contentApiKey + contentApiKey: contentApiKey, + reset: () => { + store = undefined; + memoryStore = undefined; + privateBlogInstance = undefined; + globalResetInstance = undefined; + globalBlockInstance = undefined; + userLoginInstance = undefined; + membersAuthInstance = undefined; + membersAuthEnumerationInstance = undefined; + userResetInstance = undefined; + contentApiKeyInstance = undefined; + + spam = config.get('spam') || {}; + spamPrivateBlock = spam.private_block || {}; + spamGlobalBlock = spam.global_block || {}; + spamGlobalReset = spam.global_reset || {}; + spamUserReset = spam.user_reset || {}; + spamUserLogin = spam.user_login || {}; + spamMemberLogin = spam.member_login || {}; + spamContentApiKey = spam.content_api_key || {}; + } }; diff --git a/ghost/core/core/server/web/shared/middleware/brute.js b/ghost/core/core/server/web/shared/middleware/brute.js index 6e83f9f2c0..b41583b09b 100644 --- a/ghost/core/core/server/web/shared/middleware/brute.js +++ b/ghost/core/core/server/web/shared/middleware/brute.js @@ -84,6 +84,7 @@ module.exports = { }, /** + * Block too many password guesses for the same email address */ membersAuth(req, res, next) { return spamPrevention.membersAuth().getMiddleware({ @@ -96,5 +97,12 @@ module.exports = { return _next(); } })(req, res, next); + }, + + /** + * Blocks user enumeration + */ + membersAuthEnumeration(req, res, next) { + return spamPrevention.membersAuthEnumeration().prevent(req, res, next); } }; diff --git a/ghost/core/core/shared/config/defaults.json b/ghost/core/core/shared/config/defaults.json index e13c9f9858..dcea553002 100644 --- a/ghost/core/core/shared/config/defaults.json +++ b/ghost/core/core/shared/config/defaults.json @@ -99,6 +99,12 @@ "maxWait": 86400000, "lifetime": 3600, "freeRetries": 99 + }, + "member_login": { + "minWait": 600000, + "maxWait": 43200000, + "lifetime": 43200, + "freeRetries": 8 } }, "caching": { diff --git a/ghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snap b/ghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snap new file mode 100644 index 0000000000..7a44f3606f --- /dev/null +++ b/ghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snap @@ -0,0 +1,55 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`sendMagicLink Throws an error when logging in to a email that does not exist (invite only) 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "No member exists with this e-mail address.", + "property": null, + "type": "BadRequestError", + }, + ], +} +`; + +exports[`sendMagicLink Throws an error when logging in to a email that does not exist 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "No member exists with this e-mail address. Please sign up first.", + "property": null, + "type": "BadRequestError", + }, + ], +} +`; + +exports[`sendMagicLink Throws an error when trying to sign up on an invite only site 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "This site is invite-only, contact the owner for access.", + "property": null, + "type": "BadRequestError", + }, + ], +} +`; diff --git a/ghost/core/test/e2e-api/members/send-magic-link.test.js b/ghost/core/test/e2e-api/members/send-magic-link.test.js index 9419d27fe6..95d2bfc463 100644 --- a/ghost/core/test/e2e-api/members/send-magic-link.test.js +++ b/ghost/core/test/e2e-api/members/send-magic-link.test.js @@ -1,5 +1,7 @@ -const {agentProvider, mockManager, fixtureManager} = require('../../utils/e2e-framework'); +const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework'); const should = require('should'); +const settingsCache = require('../../../core/shared/settings-cache'); +const {anyErrorId} = matchers; let membersAgent, membersService; @@ -21,6 +23,9 @@ describe('sendMagicLink', function () { beforeEach(function () { mockManager.mockMail(); + + // Reset settings + settingsCache.set('members_signup_access', {value: 'all'}); }); afterEach(function () { @@ -35,6 +40,61 @@ describe('sendMagicLink', function () { }) .expectStatus(400); }); + + it('Throws an error when logging in to a email that does not exist', async function () { + const email = 'this-member-does-not-exist@test.com'; + await membersAgent.post('/api/send-magic-link') + .body({ + email, + emailType: 'signin' + }) + .expectStatus(400) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId, + // Add this here because it is easy to be overlooked (we need a human readable error!) + // 'Please sign up first' should be included only when invite only is disabled. + message: 'No member exists with this e-mail address. Please sign up first.' + }] + }); + }); + + it('Throws an error when logging in to a email that does not exist (invite only)', async function () { + settingsCache.set('members_signup_access', {value: 'invite'}); + + const email = 'this-member-does-not-exist@test.com'; + await membersAgent.post('/api/send-magic-link') + .body({ + email, + emailType: 'signin' + }) + .expectStatus(400) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId, + // Add this here because it is easy to be overlooked (we need a human readable error!) + // 'Please sign up first' should NOT be included + message: 'No member exists with this e-mail address.' + }] + }); + }); + + it('Throws an error when trying to sign up on an invite only site', async function () { + settingsCache.set('members_signup_access', {value: 'invite'}); + + const email = 'this-member-does-not-exist@test.com'; + await membersAgent.post('/api/send-magic-link') + .body({ + email, + emailType: 'signup' + }) + .expectStatus(400) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }); + }); it('Creates a valid magic link with tokenData, and without urlHistory', async function () { const email = 'newly-created-user-magic-link-test@test.com'; diff --git a/ghost/core/test/e2e-api/members/signin.test.js b/ghost/core/test/e2e-api/members/signin.test.js index 4fec2f5f90..75b0d7b573 100644 --- a/ghost/core/test/e2e-api/members/signin.test.js +++ b/ghost/core/test/e2e-api/members/signin.test.js @@ -1,8 +1,8 @@ -const {agentProvider, mockManager, fixtureManager, dbUtils, configUtils} = require('../../utils/e2e-framework'); +const {agentProvider, mockManager, fixtureManager, configUtils, resetRateLimits, dbUtils} = require('../../utils/e2e-framework'); const models = require('../../../core/server/models'); const assert = require('assert'); require('should'); -const labsService = require('../../../core/shared/labs'); +const sinon = require('sinon'); let membersAgent, membersService; @@ -17,9 +17,9 @@ async function assertMemberEvents({eventType, memberId, asserts}) { assert.equal(events.length, asserts.length, `Only ${asserts.length} ${eventType} should have been added.`); } -async function getMemberByEmail(email) { +async function getMemberByEmail(email, require = true) { // eslint-disable-next-line dot-notation - return await models['Member'].where('email', email).fetch({require: true}); + return await models['Member'].where('email', email).fetch({require}); } describe('Members Signin', function () { @@ -48,7 +48,7 @@ describe('Members Signin', function () { }); it('Will set a cookie if the token is valid', async function () { - const magicLink = await membersService.api.getMagicLink('member1@test.com'); + const magicLink = await membersService.api.getMagicLink('member1@test.com', 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); @@ -59,7 +59,7 @@ describe('Members Signin', function () { }); it('Will redirect to the free welcome page for signup', async function () { - const magicLink = await membersService.api.getMagicLink('member1@test.com'); + const magicLink = await membersService.api.getMagicLink('member1@test.com', 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); @@ -70,7 +70,7 @@ describe('Members Signin', function () { }); it('Will redirect to the paid welcome page for signup-paid', async function () { - const magicLink = await membersService.api.getMagicLink('paid@test.com'); + const magicLink = await membersService.api.getMagicLink('paid@test.com', 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); @@ -81,7 +81,7 @@ describe('Members Signin', function () { }); it('Will redirect to the free welcome page for subscribe', async function () { - const magicLink = await membersService.api.getMagicLink('member1@test.com'); + const magicLink = await membersService.api.getMagicLink('member1@test.com', 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); @@ -93,7 +93,7 @@ describe('Members Signin', function () { it('Will create a new member on signup', async function () { const email = 'not-existent-member@test.com'; - const magicLink = await membersService.api.getMagicLink(email); + const magicLink = await membersService.api.getMagicLink(email, 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); @@ -120,9 +120,194 @@ describe('Members Signin', function () { }); }); + it('Allows a signin via a signup link', async function () { + // This member should be created by the previous test + const email = 'not-existent-member@test.com'; + + const magicLink = await membersService.api.getMagicLink(email, 'signup'); + const magicLinkUrl = new URL(magicLink); + const token = magicLinkUrl.searchParams.get('token'); + + await membersAgent.get(`/?token=${token}&action=signup`) + .expectStatus(302) + .expectHeader('Location', /\/welcome-free\/$/) + .expectHeader('Set-Cookie', /members-ssr.*/); + }); + + it('Will not create a new member on signin', async function () { + const email = 'not-existent-member-2@test.com'; + const magicLink = await membersService.api.getMagicLink(email, 'signin'); + const magicLinkUrl = new URL(magicLink); + const token = magicLinkUrl.searchParams.get('token'); + + // Note: we deliberately set the wrong action here, because this action should be ignored by the backend + // and only used by the frontend. + await membersAgent.get(`/?token=${token}&action=signup`) + .expectStatus(302) + .expectHeader('Location', /success=false/); + + const member = await getMemberByEmail(email, false); + assert(!member, 'Member should not have been created'); + }); + describe('Rate limiting', function () { - it('Will clear rate limits for members auth', async function () { + let clock; + + beforeEach(async function () { await dbUtils.truncate('brute'); + await resetRateLimits(); + clock = sinon.useFakeTimers(new Date()); + }); + + afterEach(function () { + clock.restore(); + }); + + it('Will rate limit member enumeration', async function () { + // +1 because this is a retry count, so we have one request + the retries, then blocked + const userLoginRateLimit = configUtils.config.get('spam').member_login.freeRetries + 1; + + for (let i = 0; i < userLoginRateLimit; i++) { + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-' + i + '@test.com', + emailType: 'signup' + }) + .expectStatus(201); + } + + // Now we've been rate limited for every email + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'other@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Now we've been rate limited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'one@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Get one of the magic link emails + const mail = mockManager.assert.sentEmail({ + to: 'rate-limiting-test-0@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 works, but we're still rate limited (otherwise this would be an easy escape to allow user enumeration) + await membersAgent.get(magicLink.pathname + magicLink.search); + + // We are still rate limited + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Wait 10 minutes and check if we are still rate limited + clock.tick(10 * 60 * 1000); + + // We should be able to send a new email + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any@test.com', + emailType: 'signup' + }) + .expectStatus(201); + + // But only once + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any2@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Waiting 10 minutes is still enough (fibonacci) + clock.tick(10 * 60 * 1000); + + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any2@test.com', + emailType: 'signup' + }) + .expectStatus(201); + + // Blocked again + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any3@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Waiting 10 minutes is not enough any longer + clock.tick(10 * 60 * 1000); + + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any3@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Waiting 20 minutes is enough + clock.tick(10 * 60 * 1000); + + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any2@test.com', + emailType: 'signup' + }) + .expectStatus(201); + + // Blocked again + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any3@test.com', + emailType: 'signup' + }) + .expectStatus(429); + + // Waiting 12 hours is enough to reset it completely + clock.tick(12 * 60 * 60 * 1000 + 1000); + + // We can try multiple times again + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any4@test.com', + emailType: 'signup' + }) + .expectStatus(201); + + // Blocked again + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'any5@test.com', + emailType: 'signup' + }) + .expectStatus(201); + }); + + it('Will clear rate limits for members auth', async function () { + // Temporary increase the member_login rate limits to a higher number + // because other wise we would hit user enumeration rate limits (this won't get reset after a succeeded login) + // We need to do this here otherwise the middlewares are not setup correctly + configUtils.set('spam:member_login:freeRetries', 40); + + // We need to reset spam instances to apply the configuration change + await resetRateLimits(); + // +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; @@ -131,13 +316,15 @@ describe('Members Signin', function () { .body({ email: 'rate-limiting-test-1@test.com', emailType: 'signup' - }); + }) + .expectStatus(201); await membersAgent.post('/api/send-magic-link') .body({ email: 'rate-limiting-test-2@test.com', emailType: 'signup' - }); + }) + .expectStatus(201); } // Now we've been rate limited @@ -186,13 +373,24 @@ describe('Members Signin', function () { emailType: 'signup' }) .expectStatus(429); + + // Wait 10 minutes and check if we are still rate limited + clock.tick(10 * 60 * 1000); + + // We should be able to send a new email + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'rate-limiting-test-2@test.com', + emailType: 'signup' + }) + .expectStatus(201); }); }); 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'; - const magicLink = await membersService.api.getMagicLink(email, { + const magicLink = await membersService.api.getMagicLink(email, 'signup', { attribution: { id: 'test_source_id', url: '/test-source-url/', diff --git a/ghost/core/test/e2e-frontend/members.test.js b/ghost/core/test/e2e-frontend/members.test.js index 0da5c09441..7fa3fa7fd3 100644 --- a/ghost/core/test/e2e-frontend/members.test.js +++ b/ghost/core/test/e2e-frontend/members.test.js @@ -24,11 +24,14 @@ describe('Front-end members behavior', function () { let request; async function loginAsMember(email) { + // Member should exist, because we are signin in + await models.Member.findOne({email}, {require: true}); + // membersService needs to be required after Ghost start so that settings // are pre-populated with defaults const membersService = require('../../core/server/services/members'); - const signinLink = await membersService.api.getMagicLink(email); + const signinLink = await membersService.api.getMagicLink(email, 'signin'); const signinURL = new URL(signinLink); // request needs a relative path rather than full url with host const signinPath = `${signinURL.pathname}${signinURL.search}`; @@ -411,11 +414,14 @@ describe('Front-end members behavior', function () { describe('as paid member', function () { const email = 'paid@test.com'; before(async function () { + // Member should exist, because we are signin in + await models.Member.findOne({email}, {require: true}); + // membersService needs to be required after Ghost start so that settings // are pre-populated with defaults const membersService = require('../../core/server/services/members'); - const signinLink = await membersService.api.getMagicLink(email); + const signinLink = await membersService.api.getMagicLink(email, 'signin'); const signinURL = new URL(signinLink); // request needs a relative path rather than full url with host const signinPath = `${signinURL.pathname}${signinURL.search}`; diff --git a/ghost/core/test/utils/agents/members-api-test-agent.js b/ghost/core/test/utils/agents/members-api-test-agent.js index 49c70647c9..03d436fc17 100644 --- a/ghost/core/test/utils/agents/members-api-test-agent.js +++ b/ghost/core/test/utils/agents/members-api-test-agent.js @@ -18,7 +18,7 @@ class MembersAPITestAgent extends TestAgent { async loginAs(email) { const membersService = require('../../../core/server/services/members'); - const magicLink = await membersService.api.getMagicLink(email); + const magicLink = await membersService.api.getMagicLink(email, 'signup'); const magicLinkUrl = new URL(magicLink); const token = magicLinkUrl.searchParams.get('token'); diff --git a/ghost/core/test/utils/e2e-framework.js b/ghost/core/test/utils/e2e-framework.js index 222e272bec..b1388d96b9 100644 --- a/ghost/core/test/utils/e2e-framework.js +++ b/ghost/core/test/utils/e2e-framework.js @@ -128,6 +128,15 @@ const getFixture = (type, index = 0) => { return fixtureUtils.DataGenerator.forKnex[type][index]; }; +/** + * Reset rate limit instances (not the brute table) + */ +const resetRateLimits = async () => { + // Reset rate limiting instances + const {spamPrevention} = require('../../core/server/web/shared/middleware/api'); + spamPrevention.reset(); +}; + /** * This function ensures that Ghost's data is reset back to "factory settings" * @@ -140,6 +149,9 @@ const resetData = async () => { // Clear out the database await db.reset({truncate: true}); + + // Reset rate limiting instances (resetting the table is not enough!) + await resetRateLimits(); }; /** @@ -374,5 +386,6 @@ module.exports = { // utilities configUtils: require('./configUtils'), dbUtils: require('./db-utils'), - urlUtils: require('./urlUtils') + urlUtils: require('./urlUtils'), + resetRateLimits }; diff --git a/ghost/magic-link/lib/MagicLink.js b/ghost/magic-link/lib/MagicLink.js index 639cdfb9bc..abbd43563c 100644 --- a/ghost/magic-link/lib/MagicLink.js +++ b/ghost/magic-link/lib/MagicLink.js @@ -83,13 +83,12 @@ class MagicLink { * * @param {object} options * @param {TokenData} options.tokenData - The data for token - * @param {string=} [options.type='signin'] - The type to be passed to the url and content generator functions + * @param {string=} [options.type='signin'] - The type to be passed to the url and content generator functions. This type will also get stored in the token data. * @returns {Promise} - signin URL */ async getMagicLink(options) { - const token = await this.tokenProvider.create(options.tokenData); - - const type = options.type || 'signin'; + const type = options.type ?? 'signin'; + const token = await this.tokenProvider.create({...options.tokenData, type}); return this.getSigninURL(token, type); } diff --git a/ghost/members-api/lib/MembersAPI.js b/ghost/members-api/lib/MembersAPI.js index 2a09513a1b..497a999640 100644 --- a/ghost/members-api/lib/MembersAPI.js +++ b/ghost/members-api/lib/MembersAPI.js @@ -192,11 +192,21 @@ module.exports = function MembersAPI({ type = 'signup'; } } - return magicLinkService.sendMagicLink({email, type, tokenData: Object.assign({email}, tokenData), referrer}); + return magicLinkService.sendMagicLink({email, type, tokenData: Object.assign({email, type}, tokenData), referrer}); } - function getMagicLink(email, tokenData = {}) { - return magicLinkService.getMagicLink({tokenData: {email, ...tokenData}, type: 'signin'}); + /** + * + * @param {string} email + * @param {'signin'|'signup'} type When you specify 'signin' this will prevent the creation of a new member if no member is found with the provided email + * @param {*} [tokenData] Optional token data to add to the token + * @returns + */ + function getMagicLink(email, type, tokenData = {}) { + return magicLinkService.getMagicLink({ + tokenData: {email, ...tokenData}, + type + }); } async function getTokenDataFromMagicLinkToken(token) { @@ -204,7 +214,7 @@ module.exports = function MembersAPI({ } async function getMemberDataFromMagicLinkToken(token) { - const {email, labels = [], name = '', oldEmail, newsletters, attribution, reqIp} = await getTokenDataFromMagicLinkToken(token); + const {email, labels = [], name = '', oldEmail, newsletters, attribution, reqIp, type} = await getTokenDataFromMagicLinkToken(token); if (!email) { return null; } @@ -213,7 +223,7 @@ module.exports = function MembersAPI({ if (member) { await MemberLoginEvent.add({member_id: member.id}); - if (oldEmail) { + if (oldEmail && (!type || type === 'updateEmail')) { // user exists but wants to change their email address await users.update({email}, {id: member.id}); return getMemberIdentityData(email); @@ -221,6 +231,13 @@ module.exports = function MembersAPI({ return member; } + // Note: old tokens can still have a missing type (we can remove this after a couple of weeks) + if (type && !['signup', 'subscribe'].includes(type)) { + // Don't allow sign up + // Note that we use the type from inside the magic token so this behaviour can't be changed + return null; + } + let geolocation; if (reqIp) { try { diff --git a/ghost/members-api/lib/controllers/router.js b/ghost/members-api/lib/controllers/router.js index f7286f498a..84f4548a39 100644 --- a/ghost/members-api/lib/controllers/router.js +++ b/ghost/members-api/lib/controllers/router.js @@ -2,14 +2,19 @@ const tpl = require('@tryghost/tpl'); const logging = require('@tryghost/logging'); const _ = require('lodash'); const {BadRequestError, NoPermissionError, NotFoundError, UnauthorizedError} = require('@tryghost/errors'); +const errors = require('@tryghost/errors'); const messages = { + emailRequired: 'Email is required.', badRequest: 'Bad Request.', notFound: 'Not Found.', offerArchived: 'This offer is archived.', tierArchived: 'This tier is archived.', existingSubscription: 'A subscription exists for this Member.', - unableToCheckout: 'Unable to initiate checkout session' + unableToCheckout: 'Unable to initiate checkout session', + inviteOnly: 'This site is invite-only, contact the owner for access.', + memberNotFound: 'No member exists with this e-mail address.', + memberNotFoundSignUp: 'No member exists with this e-mail address. Please sign up first.' }; module.exports = class RouterController { @@ -401,24 +406,43 @@ module.exports = class RouterController { } async sendMagicLink(req, res) { - const {email, emailType, autoRedirect} = req.body; + const {email, autoRedirect} = req.body; + let {emailType} = req.body; + let referer = req.get('referer'); if (autoRedirect === false){ referer = null; } if (!email) { + throw new errors.BadRequestError({ + message: tpl(messages.emailRequired) + }); + } + + if (!emailType) { + // Default to subscribe form that also allows to login (safe fallback for older clients) + if (!this._allowSelfSignup()) { + emailType = 'signin'; + } else { + emailType = 'subscribe'; + } + } + + if (!['signin', 'signup', 'subscribe'].includes(emailType)) { res.writeHead(400); return res.end('Bad Request.'); } try { - if (!this._allowSelfSignup()) { - const member = await this._memberRepository.get({email}); - if (member) { - const tokenData = {}; - await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, referrer: referer}); + if (emailType === 'signup' || emailType === 'subscribe') { + if (!this._allowSelfSignup()) { + throw new errors.BadRequestError({ + message: tpl(messages.inviteOnly) + }); } - } else { + + // Someone tries to signup with a user that already exists + // -> doesn't really matter: we'll send a login link const tokenData = _.pick(req.body, ['labels', 'name', 'newsletters']); if (req.ip) { tokenData.reqIp = req.ip; @@ -427,19 +451,32 @@ module.exports = class RouterController { tokenData.attribution = await this._memberAttributionService.getAttribution(req.body.urlHistory); await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, referrer: referer}); + + res.writeHead(201); + return res.end('Created.'); } - res.writeHead(201); - return res.end('Created.'); + + // Signin + const member = await this._memberRepository.get({email}); + if (member) { + const tokenData = {}; + await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, referrer: referer}); + res.writeHead(201); + return res.end('Created.'); + } + + throw new errors.BadRequestError({ + message: this._allowSelfSignup() ? tpl(messages.memberNotFoundSignUp) : tpl(messages.memberNotFound) + }); } catch (err) { if (err.code === 'EENVELOPE') { logging.error(err); res.writeHead(400); return res.end('Bad Request.'); } - const statusCode = (err && err.statusCode) || 500; - logging.error(err); - res.writeHead(statusCode); - return res.end('Internal Server Error.'); + + // Let the normal error middleware handle this error + throw err; } } }; diff --git a/ghost/members-ssr/lib/MembersSSR.js b/ghost/members-ssr/lib/MembersSSR.js index a31d93c789..d0b394887d 100644 --- a/ghost/members-ssr/lib/MembersSSR.js +++ b/ghost/members-ssr/lib/MembersSSR.js @@ -217,6 +217,13 @@ class MembersSSR { const token = Array.isArray(query.token) ? query.token[0] : query.token; const member = await this._getMemberDataFromToken(token); + if (!member) { + // The member doesn't exist any longer (could be a sign in token for a member that was deleted) + return Promise.reject(new BadRequestError({ + message: 'Invalid token' + })); + } + // perform and store geoip lookup for members when they log in if (!member.geolocation) { try {