From 28de1720c111d6974ed06e3b85446c9b0d5c5f4c Mon Sep 17 00:00:00 2001 From: "Fabien \"egg\" O'Carroll" Date: Tue, 4 Oct 2022 17:17:26 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Fixed=20magic=20link=20endpoint?= =?UTF-8?q?=20sending=20multiple=20emails?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/2024 Without validation it was possible to send a string of comma separated email addresses to the endpoint, and an email would be sent to each address, bypassing any rate limiting. This bug does not allow for an authentication bypass exploit. It is purely a spam email concern. Credit: Sandip Maity --- .../e2e-api/members/send-magic-link.test.js | 9 +++ ghost/magic-link/lib/MagicLink.js | 12 +++- ghost/magic-link/package.json | 2 + ghost/magic-link/test/index.test.js | 64 ++++++++++++++----- 4 files changed, 70 insertions(+), 17 deletions(-) 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 37527bbe79..9419d27fe6 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 @@ -27,6 +27,15 @@ describe('sendMagicLink', function () { mockManager.restore(); }); + it('Errors when passed multiple emails', async function () { + await membersAgent.post('/api/send-magic-link') + .body({ + email: 'one@test.com,two@test.com', + emailType: 'signup' + }) + .expectStatus(400); + }); + it('Creates a valid magic link with tokenData, and without urlHistory', async function () { const email = 'newly-created-user-magic-link-test@test.com'; await membersAgent.post('/api/send-magic-link') diff --git a/ghost/magic-link/lib/MagicLink.js b/ghost/magic-link/lib/MagicLink.js index 1445c9d2c6..639cdfb9bc 100644 --- a/ghost/magic-link/lib/MagicLink.js +++ b/ghost/magic-link/lib/MagicLink.js @@ -1,4 +1,9 @@ -const {IncorrectUsageError} = require('@tryghost/errors'); +const {IncorrectUsageError, BadRequestError} = require('@tryghost/errors'); +const {isEmail} = require('@tryghost/validator'); +const tpl = require('@tryghost/tpl'); +const messages = { + invalidEmail: 'Email is not valid' +}; /** * @typedef { import('nodemailer').Transporter } MailTransporter @@ -52,6 +57,11 @@ class MagicLink { * @returns {Promise<{token: Token, info: SentMessageInfo}>} */ async sendMagicLink(options) { + if (!isEmail(options.email)) { + throw new BadRequestError({ + message: tpl(messages.invalidEmail) + }); + } const token = await this.tokenProvider.create(options.tokenData); const type = options.type || 'signin'; diff --git a/ghost/magic-link/package.json b/ghost/magic-link/package.json index 4e5c3c79d3..b4a89949a8 100644 --- a/ghost/magic-link/package.json +++ b/ghost/magic-link/package.json @@ -25,6 +25,8 @@ }, "dependencies": { "@tryghost/errors": "1.2.17", + "@tryghost/tpl": "0.1.18", + "@tryghost/validator": "0.1.29", "jsonwebtoken": "8.5.1" } } diff --git a/ghost/magic-link/test/index.test.js b/ghost/magic-link/test/index.test.js index f1136463a1..6b234f4742 100644 --- a/ghost/magic-link/test/index.test.js +++ b/ghost/magic-link/test/index.test.js @@ -1,4 +1,4 @@ -const should = require('should'); +const assert = require('assert'); const sinon = require('sinon'); const MagicLink = require('../'); const crypto = require('crypto'); @@ -8,10 +8,42 @@ const secret = crypto.randomBytes(64); describe('MagicLink', function () { it('Exports a function', function () { - should.equal(typeof MagicLink, 'function'); + assert.equal(typeof MagicLink, 'function'); }); describe('#sendMagicLink', function () { + it('Throws when passed comma separated emails', async function () { + const options = { + tokenProvider: new MagicLink.JWTTokenProvider(secret), + getSigninURL: sandbox.stub().returns('FAKEURL'), + getText: sandbox.stub().returns('SOMETEXT'), + getHTML: sandbox.stub().returns('SOMEHTML'), + getSubject: sandbox.stub().returns('SOMESUBJECT'), + transporter: { + sendMail: sandbox.stub().resolves() + } + }; + const service = new MagicLink(options); + + const args = { + email: 'one@email.com,two@email.com', + tokenData: { + id: '420' + }, + type: 'blazeit', + referrer: 'https://whatever.com' + }; + + let errored = false; + try { + await service.sendMagicLink(args); + } catch (err) { + errored = true; + } finally { + assert(errored, 'sendMagicLink should error when given comma separated emails'); + } + }); + it('Sends an email to the user with a link generated from getSigninURL(token, type)', async function () { const options = { tokenProvider: new MagicLink.JWTTokenProvider(secret), @@ -35,23 +67,23 @@ describe('MagicLink', function () { }; const {token} = await service.sendMagicLink(args); - should.ok(options.getSigninURL.calledOnce); - should.ok(options.getSigninURL.firstCall.calledWithExactly(token, 'blazeit', 'https://whatever.com')); + assert(options.getSigninURL.calledOnce); + assert(options.getSigninURL.firstCall.calledWithExactly(token, 'blazeit', 'https://whatever.com')); - should.ok(options.getText.calledOnce); - should.ok(options.getText.firstCall.calledWithExactly('FAKEURL', 'blazeit', 'test@example.com')); + assert(options.getText.calledOnce); + assert(options.getText.firstCall.calledWithExactly('FAKEURL', 'blazeit', 'test@example.com')); - should.ok(options.getHTML.calledOnce); - should.ok(options.getHTML.firstCall.calledWithExactly('FAKEURL', 'blazeit', 'test@example.com')); + assert(options.getHTML.calledOnce); + assert(options.getHTML.firstCall.calledWithExactly('FAKEURL', 'blazeit', 'test@example.com')); - should.ok(options.getSubject.calledOnce); - should.ok(options.getSubject.firstCall.calledWithExactly('blazeit')); + assert(options.getSubject.calledOnce); + assert(options.getSubject.firstCall.calledWithExactly('blazeit')); - should.ok(options.transporter.sendMail.calledOnce); - should.equal(options.transporter.sendMail.firstCall.args[0].to, args.email); - should.equal(options.transporter.sendMail.firstCall.args[0].subject, options.getSubject.firstCall.returnValue); - should.equal(options.transporter.sendMail.firstCall.args[0].text, options.getText.firstCall.returnValue); - should.equal(options.transporter.sendMail.firstCall.args[0].html, options.getHTML.firstCall.returnValue); + assert(options.transporter.sendMail.calledOnce); + assert.equal(options.transporter.sendMail.firstCall.args[0].to, args.email); + assert.equal(options.transporter.sendMail.firstCall.args[0].subject, options.getSubject.firstCall.returnValue); + assert.equal(options.transporter.sendMail.firstCall.args[0].text, options.getText.firstCall.returnValue); + assert.equal(options.transporter.sendMail.firstCall.args[0].html, options.getHTML.firstCall.returnValue); }); }); @@ -78,7 +110,7 @@ describe('MagicLink', function () { const {token} = await service.sendMagicLink(args); const data = await service.getDataFromToken(token); - should.deepEqual(data.id, args.tokenData.id); + assert.deepEqual(data.id, args.tokenData.id); }); }); });