diff --git a/ghost/core/core/server/api/endpoints/automated-email-design.js b/ghost/core/core/server/api/endpoints/automated-email-design.js index 4055db7b116..55e5bec29f3 100644 --- a/ghost/core/core/server/api/endpoints/automated-email-design.js +++ b/ghost/core/core/server/api/endpoints/automated-email-design.js @@ -2,7 +2,9 @@ const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); const tpl = require('@tryghost/tpl'); const models = require('../../models'); +const emailAddressService = require('../../services/email-address'); const {DEFAULT_EMAIL_DESIGN_SETTING_SLUG} = require('../../services/member-welcome-emails/constants'); +const {validateEmailSenderFields} = require('./utils/validate-email-sender-fields'); const messages = { defaultDesignNotFound: 'Default automated email design setting not found.' @@ -69,6 +71,8 @@ const controller = { message: 'The slug field cannot be modified.' }); } + emailAddressService.init(); + validateEmailSenderFields(emailAddressService.service, data); const defaultDesign = await resolveDefaultDesign(frame.options); diff --git a/ghost/core/core/server/api/endpoints/automated-emails.js b/ghost/core/core/server/api/endpoints/automated-emails.js index 4f9dbc0123f..ac21bce2497 100644 --- a/ghost/core/core/server/api/endpoints/automated-emails.js +++ b/ghost/core/core/server/api/endpoints/automated-emails.js @@ -3,7 +3,9 @@ const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const models = require('../../models'); const memberWelcomeEmailService = require('../../services/member-welcome-emails/service'); +const emailAddressService = require('../../services/email-address'); const {DEFAULT_EMAIL_DESIGN_SETTING_SLUG} = require('../../services/member-welcome-emails/constants'); +const {validateEmailSenderFields} = require('./utils/validate-email-sender-fields'); const messages = { automatedEmailNotFound: 'Automated email not found.' @@ -36,8 +38,8 @@ function flattenAutomation(automation, email = automation.related('welcomeEmailA return result; } -async function getDefaultEmailDesignSettings() { - const designSettings = await models.EmailDesignSetting.findOne({slug: DEFAULT_EMAIL_DESIGN_SETTING_SLUG}); +async function getDefaultEmailDesignSettings(options = {}) { + const designSettings = await models.EmailDesignSetting.findOne({slug: DEFAULT_EMAIL_DESIGN_SETTING_SLUG}, options); if (!designSettings?.id) { throw new errors.NotFoundError({ @@ -70,6 +72,10 @@ async function updateEmailDesignSenderFields(email, senderData, options) { return models.EmailDesignSetting.findOne({id}, options); } +function getChangedSenderData(senderData, designSettings) { + return _.pickBy(senderData, (value, field) => value !== designSettings?.get(field)); +} + /** @type {import('@tryghost/api-framework').Controller} */ const controller = { docName: 'automated_emails', @@ -139,6 +145,8 @@ const controller = { const emailData = _.pick(data, EMAIL_FIELDS); const senderData = _.pick(data, SENDER_FIELDS); const automationData = _.pick(data, AUTOMATION_FIELDS); + emailAddressService.init(); + validateEmailSenderFields(emailAddressService.service, senderData); return models.Base.transaction(async (transacting) => { const automation = await models.Automation.add(automationData, {...frame.options, transacting}); @@ -190,8 +198,14 @@ const controller = { }); } let email = automation.related('welcomeEmailAutomatedEmail'); + const hasEmailContent = Boolean(email.id); + const designSettings = hasEmailContent ? email.related('emailDesignSetting') : null; + const changedSenderData = hasEmailContent ? getChangedSenderData(senderData, designSettings) : {}; + + emailAddressService.init(); + validateEmailSenderFields(emailAddressService.service, changedSenderData); - if (Object.keys(emailData).length > 0) { + if (hasEmailContent && Object.keys(emailData).length > 0) { email = await models.WelcomeEmailAutomatedEmail.edit(emailData, { ...frame.options, transacting, @@ -199,11 +213,15 @@ const controller = { }); } - const designSettings = await updateEmailDesignSenderFields( - email, - senderData, - {...frame.options, transacting} - ); + let updatedDesignSettings = designSettings; + + if (hasEmailContent) { + updatedDesignSettings = await updateEmailDesignSenderFields( + email, + changedSenderData, + {...frame.options, transacting} + ); + } if (Object.keys(automationData).length > 0) { automation = await models.Automation.edit(automationData, { @@ -212,7 +230,11 @@ const controller = { }); } - return flattenAutomation(automation, email, designSettings); + if (!hasEmailContent) { + updatedDesignSettings = await getDefaultEmailDesignSettings({...frame.options, transacting}); + } + + return flattenAutomation(automation, email, updatedDesignSettings); }); } }, diff --git a/ghost/core/core/server/api/endpoints/utils/validate-email-sender-fields.ts b/ghost/core/core/server/api/endpoints/utils/validate-email-sender-fields.ts new file mode 100644 index 00000000000..346feddae65 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/utils/validate-email-sender-fields.ts @@ -0,0 +1,42 @@ +import errors from '@tryghost/errors'; +import type {EmailAddressService} from '../../../services/email-address/email-address-service'; +import type {ReadonlyDeep} from 'type-fest'; + +const EMAIL_VALIDATIONS = [ + { + field: 'sender_email', + addressType: 'from' + }, + { + field: 'sender_reply_to', + addressType: 'replyTo' + } +] as const; + +export function validateEmailSenderFields( + emailAddressService: Pick, + data: ReadonlyDeep<{ + sender_email?: string; + sender_reply_to?: string; + }> +): void { + for (const {field, addressType} of EMAIL_VALIDATIONS) { + const value = data[field]; + if (!value) { + continue; + } + + const validated = emailAddressService.validate(value, addressType); + if (!validated.allowed) { + throw new errors.ValidationError({ + message: `You cannot set ${field} to ${value}` + }); + } + + if (validated.verificationEmailRequired) { + throw new errors.ValidationError({ + message: `You cannot set ${field} to ${value} without verification` + }); + } + } +} diff --git a/ghost/core/test/e2e-api/admin/automated-email-design.test.js b/ghost/core/test/e2e-api/admin/automated-email-design.test.js index 341e73a40ad..58ee18df623 100644 --- a/ghost/core/test/e2e-api/admin/automated-email-design.test.js +++ b/ghost/core/test/e2e-api/admin/automated-email-design.test.js @@ -1,6 +1,9 @@ const assert = require('node:assert/strict'); const {agentProvider, fixtureManager, matchers} = require('../../utils/e2e-framework'); const {anyContentVersion, anyObjectId, anyISODateTime, anyErrorId, anyEtag} = matchers; +const sinon = require('sinon'); +const emailAddressService = require('../../../core/server/services/email-address'); +const models = require('../../../core/server/models'); const matchEmailDesignSetting = { id: anyObjectId, @@ -107,6 +110,48 @@ describe('Automated Email Design API', function () { etag: anyEtag }); }); + + it('Rejects disallowed sender email', async function () { + await models.Base.knex('email_design_settings') + .where('slug', 'default-automated-email') + .update({ + background_color: 'light', + sender_name: 'Existing Sender', + sender_email: 'existing@example.com', + sender_reply_to: 'existing-reply@example.com' + }); + + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .returns({allowed: false, verificationEmailRequired: false}); + + try { + await agent + .put('automated_emails/design') + .body({automated_email_design: [{ + background_color: 'dark', + sender_name: 'Custom Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(422); + + sinon.assert.calledOnceWithExactly(validateStub, 'sender@example.com', 'from'); + + const designSettings = await models.Base.knex('email_design_settings') + .where('slug', 'default-automated-email') + .first('background_color', 'sender_name', 'sender_email', 'sender_reply_to'); + + assert.deepEqual(designSettings, { + background_color: 'light', + sender_name: 'Existing Sender', + sender_email: 'existing@example.com', + sender_reply_to: 'existing-reply@example.com' + }); + } finally { + validateStub.restore(); + } + }); }); describe('Permissions', function () { diff --git a/ghost/core/test/e2e-api/admin/automated-emails.test.js b/ghost/core/test/e2e-api/admin/automated-emails.test.js index eab24899ded..6e465bfa3ae 100644 --- a/ghost/core/test/e2e-api/admin/automated-emails.test.js +++ b/ghost/core/test/e2e-api/admin/automated-emails.test.js @@ -5,6 +5,7 @@ const sinon = require('sinon'); const logging = require('@tryghost/logging'); const mailService = require('../../../core/server/services/mail'); const SingleUseTokenProvider = require('../../../core/server/services/members/single-use-token-provider'); +const emailAddressService = require('../../../core/server/services/email-address'); const models = require('../../../core/server/models'); const matchAutomatedEmail = { @@ -80,6 +81,10 @@ describe('Automated Emails API', function () { }); }); + afterEach(function () { + sinon.restore(); + }); + describe('Browse', function () { it('Can browse with no automated emails', async function () { await agent @@ -274,6 +279,75 @@ describe('Automated Emails API', function () { }); }); + it('Rejects disallowed sender email on add', async function () { + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .returns({allowed: false, verificationEmailRequired: false}); + + await agent + .post('automated_emails') + .body({automated_emails: [{ + name: 'Free member welcome flow', + slug: 'member-welcome-email-free', + status: 'inactive', + subject: 'Welcome to the site!', + lexical: JSON.stringify({root: {children: []}}), + sender_name: 'Custom Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(422); + + sinon.assert.calledOnceWithExactly(validateStub, 'sender@example.com', 'from'); + + const designSettings = await models.Base.knex('email_design_settings') + .where('slug', 'default-automated-email') + .first('sender_name', 'sender_email', 'sender_reply_to'); + + assert.deepEqual(designSettings, { + sender_name: null, + sender_email: null, + sender_reply_to: null + }); + }); + + it('Rejects sender reply-to that requires verification on add', async function () { + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .callsFake((email, type) => { + if (email === 'reply@example.com' && type === 'replyTo') { + return {allowed: true, verificationEmailRequired: true}; + } + + return {allowed: true, verificationEmailRequired: false}; + }); + + await agent + .post('automated_emails') + .body({automated_emails: [{ + name: 'Free member welcome flow', + slug: 'member-welcome-email-free', + status: 'inactive', + subject: 'Welcome to the site!', + lexical: JSON.stringify({root: {children: []}}), + sender_name: 'Custom Sender', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(422); + + sinon.assert.calledOnceWithExactly(validateStub, 'reply@example.com', 'replyTo'); + + const designSettings = await models.Base.knex('email_design_settings') + .where('slug', 'default-automated-email') + .first('sender_name', 'sender_email', 'sender_reply_to'); + + assert.deepEqual(designSettings, { + sender_name: null, + sender_email: null, + sender_reply_to: null + }); + }); + it('Validates status on add', async function () { await agent .post('automated_emails') @@ -344,10 +418,6 @@ describe('Automated Emails API', function () { infoStub = sinon.stub(logging, 'info'); }); - afterEach(function () { - sinon.restore(); - }); - it('Logs when a welcome email is created as active', async function () { const {body} = await agent .post('automated_emails') @@ -447,6 +517,146 @@ describe('Automated Emails API', function () { }); }); + it('Rejects disallowed sender email on edit', async function () { + const automatedEmail = await createAutomatedEmail(); + await updateSenderStorage(automatedEmail.id, { + designSettings: { + sender_name: 'Existing Sender', + sender_email: 'existing@example.com', + sender_reply_to: 'existing-reply@example.com' + } + }); + + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .returns({allowed: false, verificationEmailRequired: false}); + + await agent + .put(`automated_emails/${automatedEmail.id}`) + .body({automated_emails: [{ + name: 'Free member welcome flow', + sender_name: 'Custom Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(422); + + sinon.assert.calledOnceWithExactly(validateStub, 'sender@example.com', 'from'); + + const {designSettings} = await getSenderStorage(automatedEmail.id); + assert.deepEqual(designSettings, { + sender_name: 'Existing Sender', + sender_email: 'existing@example.com', + sender_reply_to: 'existing-reply@example.com' + }); + }); + + it('Rejects sender reply-to that requires verification on edit', async function () { + const automatedEmail = await createAutomatedEmail(); + + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .returns({allowed: true, verificationEmailRequired: true}); + + await agent + .put(`automated_emails/${automatedEmail.id}`) + .body({automated_emails: [{ + name: 'Free member welcome flow', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(422); + + sinon.assert.calledOnceWithExactly(validateStub, 'reply@example.com', 'replyTo'); + + const {designSettings} = await getSenderStorage(automatedEmail.id); + assert.deepEqual(designSettings, { + sender_name: null, + sender_email: null, + sender_reply_to: null + }); + }); + + it('Does not validate unchanged sender fields on edit', async function () { + const automatedEmail = await createAutomatedEmail(); + await updateSenderStorage(automatedEmail.id, { + designSettings: { + sender_name: 'Existing Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + } + }); + + emailAddressService.init(); + const validateStub = sinon.stub(emailAddressService.service, 'validate') + .callsFake((email, type) => { + if (email === 'reply@example.com' && type === 'replyTo') { + return {allowed: true, verificationEmailRequired: true}; + } + + return {allowed: true, verificationEmailRequired: false}; + }); + + await agent + .put(`automated_emails/${automatedEmail.id}`) + .body({automated_emails: [{ + ...automatedEmail, + status: 'active', + subject: 'Updated subject', + sender_name: 'Existing Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }]}) + .expectStatus(200); + + sinon.assert.notCalled(validateStub); + + const {designSettings} = await getSenderStorage(automatedEmail.id); + assert.deepEqual(designSettings, { + sender_name: 'Existing Sender', + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }); + + const email = await models.Base.knex('welcome_email_automated_emails') + .where('welcome_email_automation_id', automatedEmail.id) + .first('subject'); + assert.equal(email.subject, 'Updated subject'); + + const automation = await models.Base.knex('automations') + .where('id', automatedEmail.id) + .first('status'); + assert.equal(automation.status, 'active'); + }); + + it('Can enable a legacy automated email without a welcome email content row', async function () { + const automatedEmail = await createAutomatedEmail({status: 'inactive'}); + await models.Base.knex('welcome_email_automated_emails') + .where('welcome_email_automation_id', automatedEmail.id) + .del(); + + const {body} = await agent + .get(`automated_emails/${automatedEmail.id}`) + .expectStatus(200); + + await agent + .put(`automated_emails/${automatedEmail.id}`) + .body({automated_emails: [{ + ...body.automated_emails[0], + status: 'active' + }]}) + .expectStatus(200); + + const welcomeEmailRow = await models.Base.knex('welcome_email_automated_emails') + .where('welcome_email_automation_id', automatedEmail.id) + .first('id'); + assert.equal(welcomeEmailRow, undefined); + + const automation = await models.Base.knex('automations') + .where('id', automatedEmail.id) + .first('status'); + assert.equal(automation.status, 'active'); + }); + it('Validates status on edit', async function () { const automatedEmail = await createAutomatedEmail(); @@ -544,10 +754,6 @@ describe('Automated Emails API', function () { infoStub = sinon.stub(logging, 'info'); }); - afterEach(function () { - sinon.restore(); - }); - it('Logs when a welcome email is enabled', async function () { const automatedEmail = await createAutomatedEmail({status: 'inactive'}); @@ -832,10 +1038,6 @@ describe('Automated Emails API', function () { automatedEmailId = automatedEmail.id; }); - afterEach(function () { - sinon.restore(); - }); - it('Can render preview', async function () { await agent .post(`automated_emails/${automatedEmailId}/preview/`) @@ -1049,10 +1251,6 @@ describe('Automated Emails API', function () { automatedEmailId = automatedEmail.id; }); - afterEach(function () { - sinon.restore(); - }); - it('Can send test email', async function () { await agent .post(`automated_emails/${automatedEmailId}/test/`) diff --git a/ghost/core/test/unit/api/endpoints/utils/validate-email-sender-fields.test.ts b/ghost/core/test/unit/api/endpoints/utils/validate-email-sender-fields.test.ts new file mode 100644 index 00000000000..f9adebe7262 --- /dev/null +++ b/ghost/core/test/unit/api/endpoints/utils/validate-email-sender-fields.test.ts @@ -0,0 +1,87 @@ +import assert from 'node:assert/strict'; +import sinon from 'sinon'; +import type {EmailAddressService} from '../../../../../core/server/services/email-address/email-address-service'; +import {validateEmailSenderFields} from '../../../../../core/server/api/endpoints/utils/validate-email-sender-fields'; + +type ValidationResult = ReturnType; + +function buildEmailAddressService(result: ValidationResult = { + allowed: true, + verificationEmailRequired: false +}) { + const validate = sinon.stub().returns(result); + const emailAddressService: Pick = { + validate + }; + + return { + validate, + emailAddressService + }; +} + +describe('validateEmailSenderFields', function () { + it('does not validate when sender fields are missing', function () { + const {validate, emailAddressService} = buildEmailAddressService(); + + validateEmailSenderFields(emailAddressService, {}); + + sinon.assert.notCalled(validate); + }); + + it('does not validate empty sender fields', function () { + const {validate, emailAddressService} = buildEmailAddressService(); + + validateEmailSenderFields(emailAddressService, { + sender_email: '', + sender_reply_to: '' + }); + + sinon.assert.notCalled(validate); + }); + + it('validates sender email and reply-to with correct address types', function () { + const {validate, emailAddressService} = buildEmailAddressService(); + + validateEmailSenderFields(emailAddressService, { + sender_email: 'sender@example.com', + sender_reply_to: 'reply@example.com' + }); + + sinon.assert.calledTwice(validate); + sinon.assert.calledWithExactly(validate.firstCall, 'sender@example.com', 'from'); + sinon.assert.calledWithExactly(validate.secondCall, 'reply@example.com', 'replyTo'); + }); + + it('throws when sender email is not allowed', function () { + const {emailAddressService} = buildEmailAddressService({ + allowed: false, + verificationEmailRequired: false + }); + + assert.throws(() => { + validateEmailSenderFields(emailAddressService, { + sender_email: 'sender@example.com' + }); + }, { + name: 'ValidationError', + message: 'You cannot set sender_email to sender@example.com' + }); + }); + + it('throws when sender reply-to requires verification', function () { + const {emailAddressService} = buildEmailAddressService({ + allowed: true, + verificationEmailRequired: true + }); + + assert.throws(() => { + validateEmailSenderFields(emailAddressService, { + sender_reply_to: 'reply@example.com' + }); + }, { + name: 'ValidationError', + message: 'You cannot set sender_reply_to to reply@example.com without verification' + }); + }); +});