Hey Jamie, Hey Jamie,
Unsubscribe", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "newsletter_id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "opened_count": 0, + "plaintext": "Hey Jamie, Hey Jamie +Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=preview]", + "post_id": "618ba1ffbe2896088840a6e3", + "recipient_filter": "all", + "reply_to": null, + "source": "{}", + "source_type": "lexical", + "status": "submitted", + "subject": "Test email", + "submitted_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "track_clicks": false, + "track_opens": false, + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + ], +} +`; + +exports[`Emails API Does default replacements on the HTML body of an old email 2: [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": "923", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/emails.test.js b/ghost/core/test/e2e-api/admin/emails.test.js index 2cbd36f08c..b4889f1315 100644 --- a/ghost/core/test/e2e-api/admin/emails.test.js +++ b/ghost/core/test/e2e-api/admin/emails.test.js @@ -3,6 +3,8 @@ const {nullable, anything, anyContentVersion, anyEtag, anyObjectId, anyUuid, any const assert = require('assert'); const sinon = require('sinon'); const logging = require('@tryghost/logging'); +const jobManager = require('../../../core/server/services/jobs/job-service'); +const models = require('../../../core/server/models'); const matchEmail = { id: anyObjectId, @@ -12,6 +14,11 @@ const matchEmail = { submitted_at: anyISODateTime }; +const matchEmailNewsletter = { + ...matchEmail, + newsletter_id: anyObjectId +}; + const matchBatch = { id: anyObjectId, provider_id: anyString, @@ -30,13 +37,13 @@ describe('Emails API', function () { before(async function () { agent = await agentProvider.getAdminAPIAgent(); - await fixtureManager.init('posts', 'members', 'members:emails:failed'); + await fixtureManager.init('posts', 'newsletters', 'members', 'members:emails:failed'); await agent.loginAsOwner(); }); beforeEach(function () { mockManager.mockEvents(); - mockManager.mockLabsDisabled('emailStability'); + mockManager.mockMailgun(); }); afterEach(function () { @@ -82,26 +89,10 @@ describe('Emails API', function () { etag: anyEtag }); + await jobManager.allSettled(); mockManager.assert.emittedEvent('email.edited'); }); - it('Errors when retrying an email that was successful', async function () { - const loggingStub = sinon.stub(logging, 'error'); - await agent - .put(`emails/${fixtureManager.get('emails', 0).id}/retry`) - .expectStatus(400) - .matchBodySnapshot({ - errors: [{ - id: anyErrorId - }] - }) - .matchHeaderSnapshot({ - 'content-version': anyContentVersion, - etag: anyEtag - }); - sinon.assert.calledOnce(loggingStub); - }); - it('Can browse email batches', async function () { await agent .get(`emails/${fixtureManager.get('emails', 0).id}/batches/`) @@ -195,4 +186,45 @@ describe('Emails API', function () { etag: anyEtag }); }); + + // Older Ghost emails still have a html body and plaintext body set. + it('Does default replacements on the HTML body of an old email', async function () { + const html = 'Hey %%{first_name, "there"}%%, Hey %%{first_name}%%,
Unsubscribe'; + const plaintext = 'Hey %%{first_name, "there"}%%, Hey %%{first_name}%%\nUnsubscribe [%%{unsubscribe_url}%%]'; + + // Create this email model in the database + const email = await models.Email.add({ + post_id: fixtureManager.get('posts', 2).id, + newsletter_id: fixtureManager.get('newsletters', 0).id, + status: 'submitted', + submitted_at: new Date(), + track_opens: false, + track_clicks: false, + feedback_enabled: false, + recipient_filter: 'all', + subject: 'Test email', + from: 'support@example.com', + replyTo: null, + email_count: 1, + source: '{}', + source_type: 'lexical', + html, + plaintext + }); + + const {body} = await agent + .get(`emails/${email.id}/`) + .expectStatus(200) + .matchBodySnapshot({ + emails: [matchEmailNewsletter] + }) + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }); + + // Simple check that there are not %%{ leftover (in case the snapshots gets updated without noticing what this test is checking) + assert.equal(body.emails[0].html.includes('%%{'), false); + assert.equal(body.emails[0].plaintext.includes('%%{'), false); + }); }); diff --git a/ghost/core/test/integration/services/email-service/batch-sending.test.js b/ghost/core/test/integration/services/email-service/batch-sending.test.js index 50883e0fc6..b62be3609f 100644 --- a/ghost/core/test/integration/services/email-service/batch-sending.test.js +++ b/ghost/core/test/integration/services/email-service/batch-sending.test.js @@ -14,10 +14,12 @@ const mobileDocWithFreeMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["em const mobileDocWithPaidMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:-free","alignment":"left","html":"This is for paid members only
"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[1,"p",[[0,[],0,"Bye."]]]],"ghostVersion":"4.0"}'; const mobileDocWithPaidAndFreeMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"This is for free members only
"}],["email-cta",{"showButton":false,"showDividers":true,"segment":"status:-free","alignment":"left","html":"This is for paid members only
"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[10,1],[1,"p",[[0,[],0,"Bye."]]]],"ghostVersion":"4.0"}'; const mobileDocWithFreeMemberOnlyAndPaywall = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"This is for free members only
"}],["paywall",{}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[1,"p",[[0,[],0,"Bye."]]],[10,1],[1,"p",[[0,[],0,"This is after the paywall."]]]],"ghostVersion":"4.0"}'; +const mobileDocWithReplacements = '{"version":"0.3.1","atoms":[],"cards":[["email",{"html":"Hey {first_name, \\"there\\"}, Hey {first_name},
"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello {first_name},"]]],[10,0]],"ghostVersion":"4.0"}'; const configUtils = require('../../../utils/configUtils'); const {settingsCache} = require('../../../../core/server/services/settings-helpers'); const DomainEvents = require('@tryghost/domain-events'); +const emailService = require('../../../../core/server/services/email-service'); let agent; let stubbedSend; @@ -183,17 +185,18 @@ describe('Batch sending tests', function () { }); mockManager.mockMail(); mockManager.mockMailgun(function () { + // Allows for setting stubbedSend during tests return stubbedSend.call(this, ...arguments); }); }); afterEach(async function () { - mockManager.restore(); await configUtils.restore(); await models.Settings.edit([{ key: 'email_verification_required', value: false }], {context: {internal: true}}); + mockManager.restore(); }); before(async function () { @@ -213,12 +216,12 @@ describe('Batch sending tests', function () { }); after(async function () { + mockManager.restore(); await ghostServer.stop(); }); it('Can send a scheduled post email', async function () { // Prepare a post and email model - const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); const emailModel = await createPublishedPostEmail(); assert.equal(emailModel.get('source_type'), 'mobiledoc'); @@ -226,10 +229,10 @@ describe('Batch sending tests', function () { assert(emailModel.get('from')); // Await sending job - await completedPromise; + await jobManager.allSettled(); await emailModel.refresh(); - assert.equal(emailModel.get('status'), 'submitted', 'This email should be in submitted state: ' + (emailModel.get('error') ?? 'No error')); + assert.equal(emailModel.get('status'), 'submitted'); assert.equal(emailModel.get('email_count'), 4); // Did we create batches? @@ -260,6 +263,33 @@ describe('Batch sending tests', function () { assert.equal(memberIds.length, _.uniq(memberIds).length); }); + it('Protects the email job from being run multiple times at the same time', async function () { + // Prepare a post and email model + const emailModel = await createPublishedPostEmail(); + + assert.equal(emailModel.get('source_type'), 'mobiledoc'); + assert(emailModel.get('subject')); + assert(emailModel.get('from')); + + // Retry sending a couple of times + const promises = []; + for (let i = 0; i < 100; i++) { + promises.push(emailService.service.retryEmail(emailModel)); + } + await Promise.all(promises); + + // Await sending job + await jobManager.allSettled(); + + await emailModel.refresh(); + assert.equal(emailModel.get('status'), 'submitted'); + assert.equal(emailModel.get('email_count'), 4); + + // Did we create batches? + const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`}); + assert.equal(batches.models.length, 1); + }); + it('Doesn\'t include members created after the email in the batches', async function () { // If we create a new member (e.g. a member that was imported) after the email was created, they should not be included in the email const addStub = sinon.stub(models.Email, 'add'); @@ -482,7 +512,6 @@ describe('Batch sending tests', function () { }; // Prepare a post and email model - let completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); const emailModel = await createPublishedPostEmail(); assert.equal(emailModel.get('source_type'), 'mobiledoc'); @@ -490,7 +519,7 @@ describe('Batch sending tests', function () { assert(emailModel.get('from')); // Await sending job - await completedPromise; + await jobManager.allSettled(); await emailModel.refresh(); assert.equal(emailModel.get('status'), 'failed'); @@ -545,9 +574,8 @@ describe('Batch sending tests', function () { let memberIds = emailRecipients.map(recipient => recipient.get('member_id')); assert.equal(memberIds.length, _.uniq(memberIds).length); - completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); await retryEmail(emailModel.id); - await completedPromise; + await jobManager.allSettled(); await emailModel.refresh(); batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`}); @@ -732,6 +760,67 @@ describe('Batch sending tests', function () { }); }); + describe('Replacements', function () { + it('Does replace with and without fallback in both plaintext and html for member without name', async function () { + // Create a new member without a first_name + await models.Member.add({ + email: 'replacements-test-1@example.com', + labels: [{name: 'replacements-tests'}], + newsletters: [{ + id: fixtureManager.get('newsletters', 0).id + }] + }); + + const {html, plaintext} = await sendEmail({ + mobiledoc: mobileDocWithReplacements + }, 'label:replacements-tests'); + + // Outside the email card, {first_name} is not replaced + assert.match(html, /Hello {first_name},/); + + // Inside the email card with and without fallback, it is replaced + assert.match(html, /Hey there, Hey ,/); + + // The unsubscribe link is replaced + assert.match(html, / { - return this.createUnsubscribeUrl(member.uuid, {newsletterUuid: newsletter.get('uuid')}); + return this.createUnsubscribeUrl(member.uuid, {newsletterUuid}); } }, { @@ -402,7 +400,7 @@ class EmailRenderer { replacements.push({ id: replacementStr, originalId: recipientProperty, - token: new RegExp(escapeRegExp(replacementMatch), 'g'), + token: new RegExp(escapeRegExp(replacementMatch).replace(/(?:"|")/g, '(?:"|")'), 'g'), getValue: fallback ? (member => definition.getValue(member) || fallback) : definition.getValue }); } diff --git a/ghost/email-service/lib/email-service.js b/ghost/email-service/lib/email-service.js index 0b37ff2ca9..ac151df892 100644 --- a/ghost/email-service/lib/email-service.js +++ b/ghost/email-service/lib/email-service.js @@ -161,6 +161,21 @@ class EmailService { return email; } + /** + * @return {import('./email-renderer').MemberLike} + */ + getDefaultExampleMember() { + /** + * @type {import('./email-renderer').MemberLike} + */ + return { + id: 'example-id', + uuid: 'example-uuid', + email: 'jamie@example.com', + name: 'Jamie Larson' + }; + } + /** * @private * @param {string} [email] (optional) Search for a member with this email address and use it as the example. If not found, defaults to the default but still uses the provided email address. @@ -170,12 +185,7 @@ class EmailService { /** * @type {import('./email-renderer').MemberLike} */ - const exampleMember = { - id: 'example-id', - uuid: 'example-uuid', - email: 'jamie@example.com', - name: 'Jamie Larson' - }; + const exampleMember = this.getDefaultExampleMember(); // fetch any matching members so that replacements use expected values if (email) { @@ -194,6 +204,22 @@ class EmailService { return exampleMember; } + /** + * Do a manual replacement of tokens with values for a member (normally only used for previews) + * + * @param {string} htmlOrPlaintext + * @param {import('./email-renderer').ReplacementDefinition[]} replacements + * @param {import('./email-renderer').MemberLike} member + * @return {string} + */ + replaceDefinitions(htmlOrPlaintext, replacements, member) { + // Do manual replacements with an example member + for (const replacement of replacements) { + htmlOrPlaintext = htmlOrPlaintext.replace(replacement.token, replacement.getValue(member)); + } + return htmlOrPlaintext; + } + /** * * @param {*} post @@ -207,16 +233,10 @@ class EmailService { const subject = this.#emailRenderer.getSubject(post); let {html, plaintext, replacements} = await this.#emailRenderer.renderBody(post, newsletter, segment, {clickTrackingEnabled: false}); - // Do manual replacements with an example member - for (const replacement of replacements) { - html = html.replace(replacement.token, replacement.getValue(exampleMember)); - plaintext = plaintext.replace(replacement.token, replacement.getValue(exampleMember)); - } - return { subject, - html, - plaintext + html: this.replaceDefinitions(html, replacements, exampleMember), + plaintext: this.replaceDefinitions(plaintext, replacements, exampleMember) }; } diff --git a/ghost/email-service/package.json b/ghost/email-service/package.json index f233284bee..5613c104ed 100644 --- a/ghost/email-service/package.json +++ b/ghost/email-service/package.json @@ -21,7 +21,8 @@ "c8": "7.13.0", "mocha": "10.2.0", "should": "13.2.3", - "sinon": "15.0.1" + "sinon": "15.0.1", + "html-validate": "7.13.2" }, "dependencies": { "@tryghost/color-utils": "0.1.22", diff --git a/ghost/email-service/test/email-renderer.test.js b/ghost/email-service/test/email-renderer.test.js index 7bb808ef8f..8b875500ad 100644 --- a/ghost/email-service/test/email-renderer.test.js +++ b/ghost/email-service/test/email-renderer.test.js @@ -5,6 +5,60 @@ const {createModel} = require('./utils'); const linkReplacer = require('@tryghost/link-replacer'); const sinon = require('sinon'); const logging = require('@tryghost/logging'); +const {HtmlValidate} = require('html-validate'); + +function validateHtml(html) { + const htmlvalidate = new HtmlValidate({ + extends: [ + 'html-validate:document', + 'html-validate:standard' + ], + rules: { + // We need deprecated attrs for legacy tables in older email clients + 'no-deprecated-attr': 'off', + + // Don't care that the firstLexical Test
'; + let postUrl = 'http://example.com'; + let customSettings = {}; let emailRenderer = new EmailRenderer({ audienceFeedbackService: { buildLink: (_uuid, _postId, score) => { @@ -360,6 +416,9 @@ describe('Email renderer', function () { }, settingsCache: { get: (key) => { + if (customSettings[key]) { + return customSettings[key]; + } if (key === 'accent_color') { return '#ffffff'; } @@ -375,7 +434,7 @@ describe('Email renderer', function () { } }, getPostUrl: () => { - return 'http://example.com'; + return postUrl; }, renderers: { lexical: { @@ -430,6 +489,8 @@ describe('Email renderer', function () { }), loaded: ['posts_meta'] }; + postUrl = 'http://example.com'; + customSettings = {}; }); it('returns feedback buttons and unsubcribe links', async function () { @@ -788,6 +849,60 @@ describe('Email renderer', function () { responsePaid.html.should.containEql('finishing part only for members'); responsePaid.html.should.not.containEql('Become a paid member of Test Blog to get access to all'); }); + + it('should output valid HTML and escape HTML characters in mobiledoc', async function () { + const post = createModel({ + ...basePost, + title: 'This is\' a blog po"st test <3', + excerpt: 'This is a blog post test <3