From 64c9e66b56e452d75a0b994b6924b81b7694e54a Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Fri, 24 Mar 2023 06:14:55 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20broken=20link=20tracking?= =?UTF-8?q?=20in=20newsletters=20(#16473)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/2805 When we render mobiledoc to HTML, it automatically escapes HTML entities in the process, so a button or directly pasted link with href="https://example.com?code=test" will be rendered as href="https://example.com?code=test" as the url is encoded in the rendered HTML. Our link tracking was using the encoded URL as the redirect URL in newsletters, causing certain links to break. This change updates the link tracking to decode the URL with `entities.decode(url)` so we store the correct redirect URL in our DB and ensure link tracking redirects to the correct url from newsletters. --------- Co-authored-by: Rishabh --- ghost/email-service/lib/email-renderer.js | 3 + ghost/email-service/package.json | 5 +- .../email-service/test/email-renderer.test.js | 216 ++++++++++++------ yarn.lock | 10 +- 4 files changed, 152 insertions(+), 82 deletions(-) diff --git a/ghost/email-service/lib/email-renderer.js b/ghost/email-service/lib/email-renderer.js index fde383874b..422b77519f 100644 --- a/ghost/email-service/lib/email-renderer.js +++ b/ghost/email-service/lib/email-renderer.js @@ -8,6 +8,7 @@ const {textColorForBackgroundColor, darkenToContrastThreshold} = require('@trygh const {DateTime} = require('luxon'); const htmlToPlaintext = require('@tryghost/html-to-plaintext'); const tpl = require('@tryghost/tpl'); +const entities = require('entities'); const messages = { subscriptionStatus: { @@ -292,6 +293,8 @@ class EmailRenderer { // Link tracking if (options.clickTrackingEnabled) { html = await this.#linkReplacer.replace(html, async (url) => { + // Decode any escaped entities in the url + url = new URL(entities.decode(url.toString())); // We ignore all links that contain %%{uuid}%% // because otherwise we would add tracking to links that need to be replaced first if (url.toString().indexOf('%%{uuid}%%') !== -1) { diff --git a/ghost/email-service/package.json b/ghost/email-service/package.json index 65530bb9a0..5c497041c9 100644 --- a/ghost/email-service/package.json +++ b/ghost/email-service/package.json @@ -19,10 +19,10 @@ ], "devDependencies": { "c8": "7.13.0", + "html-validate": "7.13.3", "mocha": "10.2.0", "should": "13.2.3", - "sinon": "15.0.2", - "html-validate": "7.13.3" + "sinon": "15.0.2" }, "dependencies": { "@tryghost/color-utils": "0.1.23", @@ -35,6 +35,7 @@ "@tryghost/validator": "^0.2.0", "bson-objectid": "2.0.4", "cheerio": "0.22.0", + "entities": "4.4.0", "handlebars": "4.7.7", "juice": "8.1.0", "moment-timezone": "0.5.23" diff --git a/ghost/email-service/test/email-renderer.test.js b/ghost/email-service/test/email-renderer.test.js index 0269b52cea..8b10b01568 100644 --- a/ghost/email-service/test/email-renderer.test.js +++ b/ghost/email-service/test/email-renderer.test.js @@ -858,82 +858,9 @@ describe('Email renderer', function () { let renderedPost = '

Lexical Test

'; let postUrl = 'http://example.com'; let customSettings = {}; - let emailRenderer = new EmailRenderer({ - audienceFeedbackService: { - buildLink: (_uuid, _postId, score) => { - return new URL('http://feedback-link.com/?score=' + encodeURIComponent(score) + '&uuid=' + encodeURIComponent(_uuid)); - } - }, - urlUtils: { - urlFor: (type) => { - if (type === 'image') { - return 'http://icon.example.com'; - } - return 'http://example.com/subdirectory'; - }, - isSiteUrl: (u) => { - return u.hostname === 'example.com'; - } - }, - settingsCache: { - get: (key) => { - if (customSettings[key]) { - return customSettings[key]; - } - if (key === 'accent_color') { - return '#ffffff'; - } - if (key === 'timezone') { - return 'Etc/UTC'; - } - if (key === 'title') { - return 'Test Blog'; - } - if (key === 'icon') { - return 'ICON'; - } - } - }, - getPostUrl: () => { - return postUrl; - }, - renderers: { - lexical: { - render: () => { - return renderedPost; - } - }, - mobiledoc: { - render: () => { - return '

Mobiledoc Test

'; - } - } - }, - linkReplacer, - memberAttributionService: { - addPostAttributionTracking: (u) => { - u.searchParams.append('post_tracking', 'added'); - return u; - } - }, - linkTracking: { - service: { - addTrackingToUrl: (u, _post, uuid) => { - return new URL('http://tracked-link.com/?m=' + encodeURIComponent(uuid) + '&url=' + encodeURIComponent(u.href)); - } - } - }, - outboundLinkTagger: { - addToUrl: (u, newsletter) => { - u.searchParams.append('source_tracking', newsletter?.get('name') ?? 'site'); - return u; - } - }, - labs: { - isSet: () => true - } - }); + let emailRenderer; let basePost; + let addTrackingToUrlStub; beforeEach(function () { basePost = { @@ -955,6 +882,83 @@ describe('Email renderer', function () { }; postUrl = 'http://example.com'; customSettings = {}; + addTrackingToUrlStub = sinon.stub(); + addTrackingToUrlStub.callsFake((u, _post, uuid) => { + return new URL('http://tracked-link.com/?m=' + encodeURIComponent(uuid) + '&url=' + encodeURIComponent(u.href)); + }); + emailRenderer = new EmailRenderer({ + audienceFeedbackService: { + buildLink: (_uuid, _postId, score) => { + return new URL('http://feedback-link.com/?score=' + encodeURIComponent(score) + '&uuid=' + encodeURIComponent(_uuid)); + } + }, + urlUtils: { + urlFor: (type) => { + if (type === 'image') { + return 'http://icon.example.com'; + } + return 'http://example.com/subdirectory'; + }, + isSiteUrl: (u) => { + return u.hostname === 'example.com'; + } + }, + settingsCache: { + get: (key) => { + if (customSettings[key]) { + return customSettings[key]; + } + if (key === 'accent_color') { + return '#ffffff'; + } + if (key === 'timezone') { + return 'Etc/UTC'; + } + if (key === 'title') { + return 'Test Blog'; + } + if (key === 'icon') { + return 'ICON'; + } + } + }, + getPostUrl: () => { + return postUrl; + }, + renderers: { + lexical: { + render: () => { + return renderedPost; + } + }, + mobiledoc: { + render: () => { + return '

Mobiledoc Test

'; + } + } + }, + linkReplacer, + memberAttributionService: { + addPostAttributionTracking: (u) => { + u.searchParams.append('post_tracking', 'added'); + return u; + } + }, + linkTracking: { + service: { + addTrackingToUrl: addTrackingToUrlStub + } + }, + outboundLinkTagger: { + addToUrl: (u, newsletter) => { + u.searchParams.append('source_tracking', newsletter?.get('name') ?? 'site'); + return u; + } + }, + labs: { + isSet: () => true + } + }); }); it('returns feedback buttons and unsubcribe links', async function () { @@ -1182,6 +1186,68 @@ describe('Email renderer', function () { clickTrackingEnabled: true }; + renderedPost = '

Lexical Test

HelloHello

'; + + let response = await emailRenderer.renderBody( + post, + newsletter, + segment, + options + ); + + // Check all links have domain tracked-link.com + const $ = cheerio.load(response.html); + const links = []; + for (const link of $('a').toArray()) { + const href = $(link).attr('href'); + links.push(href); + if (href.includes('unsubscribe_url')) { + href.should.eql('%%{unsubscribe_url}%%'); + } else if (href.includes('feedback-link.com')) { + href.should.containEql('%%{uuid}%%'); + } else { + href.should.containEql('tracked-link.com'); + href.should.containEql('m=%%{uuid}%%'); + } + } + + // Update the following array when you make changes to the email template, check if replacements are correct for each newly added link. + assert.deepEqual(links, [ + `http://tracked-link.com/?m=%%{uuid}%%&url=http%3A%2F%2Fexample.com%2F%3Fsource_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, + `http://tracked-link.com/?m=%%{uuid}%%&url=http%3A%2F%2Fexample.com%2F%3Fsource_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, + `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fexternal-domain.com%2F%3Fref%3D123%26source_tracking%3Dsite`, + `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fencoded-link.com%2F%3Fcode%3Dtest%26source_tracking%3Dsite`, + `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fexample.com%2F%3Fref%3D123%26source_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, + `%%{unsubscribe_url}%%`, + `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fghost.org%2F%3Fsource_tracking%3Dsite` + ]); + + // Check uuid in replacements + response.replacements.length.should.eql(2); + response.replacements[0].id.should.eql('uuid'); + response.replacements[0].token.should.eql(/%%\{uuid\}%%/g); + response.replacements[1].id.should.eql('unsubscribe_url'); + response.replacements[1].token.should.eql(/%%\{unsubscribe_url\}%%/g); + }); + + it('handles encoded links', async function () { + const post = createModel(basePost); + const newsletter = createModel({ + header_image: null, + name: 'Test Newsletter', + show_badge: true, + feedback_enabled: true, + show_post_title_section: true + }); + const segment = null; + const options = { + clickTrackingEnabled: true + }; + renderedPost = '

Lexical Test

Hello

'; let response = await emailRenderer.renderBody( diff --git a/yarn.lock b/yarn.lock index 91af9bf6f7..7b3b297dbc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -13838,6 +13838,11 @@ ensure-posix-path@^1.0.0, ensure-posix-path@^1.0.1, ensure-posix-path@^1.0.2, en resolved "https://registry.yarnpkg.com/ensure-posix-path/-/ensure-posix-path-1.1.1.tgz#3c62bdb19fa4681544289edb2b382adc029179ce" integrity sha512-VWU0/zXzVbeJNXvME/5EmLuEj2TauvoaTz6aFYK1Z92JCBlDlZ3Gu0tuGR42kpW1754ywTs+QB0g5TP0oj9Zaw== +entities@4.4.0, entities@^4.2.0, entities@^4.3.0, entities@^4.4.0, entities@~4.4.0: + version "4.4.0" + resolved "https://registry.yarnpkg.com/entities/-/entities-4.4.0.tgz#97bdaba170339446495e653cfd2db78962900174" + integrity sha512-oYp7156SP8LkeGD0GF85ad1X9Ai79WtRsZ2gxJqtBuzH+98YUV6jkHEKlZkMbcrjJjIVJNIDP/3WL9wQkoPbWA== + entities@^1.1.1, entities@~1.1.1: version "1.1.2" resolved "https://registry.yarnpkg.com/entities/-/entities-1.1.2.tgz#bdfa735299664dfafd34529ed4f8522a275fea56" @@ -13848,11 +13853,6 @@ entities@^2.0.0: resolved "https://registry.yarnpkg.com/entities/-/entities-2.2.0.tgz#098dc90ebb83d8dffa089d55256b351d34c4da55" integrity sha512-p92if5Nz619I0w+akJrLZH0MX0Pb5DX39XOwQTtXSdQQOaYH03S1uIQp4mhOZtAXrxq4ViO67YTiLBo2638o9A== -entities@^4.2.0, entities@^4.3.0, entities@^4.4.0, entities@~4.4.0: - version "4.4.0" - resolved "https://registry.yarnpkg.com/entities/-/entities-4.4.0.tgz#97bdaba170339446495e653cfd2db78962900174" - integrity sha512-oYp7156SP8LkeGD0GF85ad1X9Ai79WtRsZ2gxJqtBuzH+98YUV6jkHEKlZkMbcrjJjIVJNIDP/3WL9wQkoPbWA== - entities@~2.1.0: version "2.1.0" resolved "https://registry.yarnpkg.com/entities/-/entities-2.1.0.tgz#992d3129cf7df6870b96c57858c249a120f8b8b5"