From 4e947a88ce1320be3724232e5cc50231c88812c2 Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 22 Sep 2021 13:32:02 +0200 Subject: [PATCH] Fixed security hole in email address change flow refs https://github.com/TryGhost/Ghost/security/advisories/GHSA-65p7-pjj8-ggmr The email address change flow was built on top of the unauthenticated signin/signup flow. This meant that ownership of the email being changed wasn't verified and allowed a malicious actore to change the email address of arbitrary accounts to an email address which they controlled. We remove the ability to change email addresses from the signin/signup flow and instead create a dedicated, authenticated flow for changing email address. --- ghost/members-api/lib/MembersAPI.js | 9 +++- ghost/members-api/lib/controllers/member.js | 56 ++++++++++++++++----- ghost/members-api/lib/controllers/router.js | 24 +++------ 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/ghost/members-api/lib/MembersAPI.js b/ghost/members-api/lib/MembersAPI.js index 40f310a0e4..cd570ce841 100644 --- a/ghost/members-api/lib/MembersAPI.js +++ b/ghost/members-api/lib/MembersAPI.js @@ -156,8 +156,9 @@ module.exports = function MembersAPI({ const memberController = new MemberController({ memberRepository, StripePrice, - stripeAPIService, - tokenService + tokenService, + sendEmailWithMagicLink, + allowSelfSignup }); const routerController = new RouterController({ @@ -356,6 +357,10 @@ module.exports = function MembersAPI({ body.json(), (req, res) => MembersAnalyticsIngress.createEvents(req, res) ), + updateEmailAddress: Router().use( + body.json(), + (req, res) => memberController.updateEmailAddress(req, res) + ), updateSubscription: Router({mergeParams: true}).use( body.json(), (req, res) => memberController.updateSubscription(req, res) diff --git a/ghost/members-api/lib/controllers/member.js b/ghost/members-api/lib/controllers/member.js index 2c98ef2dec..63284bf21d 100644 --- a/ghost/members-api/lib/controllers/member.js +++ b/ghost/members-api/lib/controllers/member.js @@ -1,25 +1,57 @@ const errors = require('@tryghost/ignition-errors'); -/** - * MemberController - * - * @param {object} deps - * @param {any} deps.memberRepository - * @param {any} deps.StripePrice - * @param {any} deps.stripeApiService - * @param {any} deps.tokenService - */ module.exports = class MemberController { + /** + * @param {object} deps + * @param {any} deps.memberRepository + * @param {any} deps.StripePrice + * @param {any} deps.tokenService + * @param {any} deps.sendEmailWithMagicLink + * @param {boolean} deps.allowSelfSignup + */ constructor({ memberRepository, StripePrice, - stripeAPIService, - tokenService + tokenService, + sendEmailWithMagicLink, + allowSelfSignup }) { this._memberRepository = memberRepository; this._StripePrice = StripePrice; - this._stripeApiService = stripeAPIService; this._tokenService = tokenService; + this._sendEmailWithMagicLink = sendEmailWithMagicLink; + this._allowSelfSignup = allowSelfSignup; + } + + async updateEmailAddress(req, res) { + const identity = req.body.identity; + const email = req.body.email; + const options = { + forceEmailType: true + }; + + if (!identity) { + res.writeHead(403); + return res.end('No Permission.'); + } + + let tokenData = {}; + try { + const member = await this._memberRepository.getByToken(identity); + tokenData.oldEmail = member.get('email'); + } catch (err) { + res.writeHead(401); + res.end('Unauthorized.'); + } + + try { + await this._sendEmailWithMagicLink({email, tokenData, requestedType: 'updateEmail', options}); + res.writeHead(201); + return res.end('Created.'); + } catch (err) { + res.writeHead(500); + return res.end('Internal Server Error.'); + } } async updateSubscription(req, res) { diff --git a/ghost/members-api/lib/controllers/router.js b/ghost/members-api/lib/controllers/router.js index a6e2730b07..10c8dc4c0e 100644 --- a/ghost/members-api/lib/controllers/router.js +++ b/ghost/members-api/lib/controllers/router.js @@ -1,6 +1,5 @@ const common = require('../../lib/common'); const _ = require('lodash'); -const errors = require('@tryghost/ignition-errors'); /** * RouterController @@ -219,33 +218,22 @@ module.exports = class RouterController { } async sendMagicLink(req, res) { - const {email, emailType, oldEmail, requestSrc} = req.body; - let forceEmailType = false; + const {email, emailType, requestSrc} = req.body; if (!email) { res.writeHead(400); return res.end('Bad Request.'); } try { - if (oldEmail) { - const existingMember = await this._memberRepository.get({email}); - if (existingMember) { - throw new errors.BadRequestError({ - message: 'This email is already associated with a member' - }); - } - forceEmailType = true; - } - if (!this._allowSelfSignup) { - const member = oldEmail ? await this._memberRepository.get({oldEmail}) : await this._memberRepository.get({email}); + const member = await this._memberRepository.get({email}); if (member) { - const tokenData = _.pick(req.body, ['oldEmail']); - await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, requestSrc, options: {forceEmailType}}); + const tokenData = {}; + await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, requestSrc}); } } else { - const tokenData = _.pick(req.body, ['labels', 'name', 'oldEmail']); - await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, requestSrc, options: {forceEmailType}}); + const tokenData = _.pick(req.body, ['labels', 'name']); + await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, requestSrc}); } res.writeHead(201); return res.end('Created.');