diff --git a/backend/__tests__/services/newsletterService.sanitize.test.js b/backend/__tests__/services/newsletterService.sanitize.test.js index 59fb047d..948dc0fa 100644 --- a/backend/__tests__/services/newsletterService.sanitize.test.js +++ b/backend/__tests__/services/newsletterService.sanitize.test.js @@ -12,7 +12,9 @@ jest.mock('../../src/database/db', () => ({ db: jest.fn(), logActivity: jest.fn( const { sanitizeCampaignBody, sanitizeCampaignCss, MAX_BODY_BYTES, + sanitizeInlineStylesAfterSubstitution, } = require('../../src/services/newsletterService'); +const { safeTemplateReplace } = require('../../src/services/emailProcessor'); describe('sanitizeCampaignBody', () => { it('returns empty string for empty input', () => { @@ -188,3 +190,46 @@ describe('sanitizeCampaignCss', () => { expect(css).not.toContain(''); }); }); + +describe('inline CSS is re-checked after template substitution', () => { + // The stored body is sanitized, but safeTemplateReplace rewrites it + // afterwards — so the string that was validated is not the string that is + // sent. A conditional inside a style attribute can delete the quoting that + // made a url() inert, which no amount of lexer correctness can catch. + const payload = + `

hi

`; + + it('neutralises a url() that substitution would activate', () => { + const stored = sanitizeCampaignBody(payload); + // Correctly left alone at write time: the url() really is inside a CSS + // string while the conditionals are still in place. + expect(stored).toContain('evil.example'); + + const substituted = safeTemplateReplace(stored, { company_name: '' }, { escapeHtml: true }); + // Expansion removed the quotes, so without the recheck this ships live. + expect(substituted).toMatch(/background:url\(https:\/\/evil\.example/); + + const rendered = sanitizeInlineStylesAfterSubstitution(substituted); + expect(rendered).not.toMatch(/url\(\s*['"]?https:\/\/evil\.example/); + expect(rendered).toContain('background:none'); + }); + + it('leaves a body without style attributes untouched', () => { + const html = '

Hello {{first_name}}

'; + expect(sanitizeInlineStylesAfterSubstitution(html)).toBe(html); + }); + + it('keeps legitimate inline styles through the recheck', () => { + const html = '

hi

'; + const out = sanitizeInlineStylesAfterSubstitution(html); + expect(out).toContain('color:red'); + expect(out).toContain('font-size:14px'); + }); + + it('is safe on empty and nullish input', () => { + expect(sanitizeInlineStylesAfterSubstitution('')).toBe(''); + expect(sanitizeInlineStylesAfterSubstitution(null)).toBeNull(); + }); +}); diff --git a/backend/src/services/newsletterService.js b/backend/src/services/newsletterService.js index 8a29f79a..9c991ef2 100644 --- a/backend/src/services/newsletterService.js +++ b/backend/src/services/newsletterService.js @@ -154,10 +154,37 @@ function sanitizeCampaignBody(html) { // inside a real string, then makes the url() request — while the scanner // saw the apostrophe open a string and skipped everything after it. The // scanner has to be shown what the browser will actually parse. - .replace(/style="([^"]*)"/gi, (match, css) => { - const { sanitized } = sanitizeCSS(decodeHtmlEntities(css)); - return sanitized ? `style="${encodeForAttribute(sanitized)}"` : ''; - }); + .replace(STYLE_ATTRIBUTE, sanitizeStyleAttribute); +} + +/** Shared by the write-time sanitize and the post-substitution recheck. */ +const STYLE_ATTRIBUTE = /style="([^"]*)"/gi; + +function sanitizeStyleAttribute(match, css) { + const { sanitized } = sanitizeCSS(decodeHtmlEntities(css)); + return sanitized ? `style="${encodeForAttribute(sanitized)}"` : ''; +} + +/** + * Re-check inline CSS AFTER template substitution. + * + * Sanitizing runs on the stored body, but `safeTemplateReplace` rewrites it + * afterwards — so the string that was validated is not the string that gets + * sent. A conditional inside a style attribute can delete the very characters + * that made a URL inert: + * + * style="--x:x{{#if company_name}}'{{/if}};background:url(https://evil…)" + * + * At sanitize time the url() sits inside a CSS string and is correctly left + * alone; once the conditional is expanded the quotes are gone and the + * background is live. No amount of lexer correctness fixes that, because the + * text being lexed is not the text being delivered — the check has to run + * again on the final output. Substitution cannot introduce a `"` (values are + * HTML-escaped), so the attribute regex still matches what it should. + */ +function sanitizeInlineStylesAfterSubstitution(html) { + if (!html) return html; + return String(html).replace(STYLE_ATTRIBUTE, sanitizeStyleAttribute); } /** @@ -330,7 +357,12 @@ async function renderForRecipient(campaign, customer, options = {}) { // Substitution happens AFTER sanitizing, with escaping on: a customer's own // company name is untrusted text and must not be able to inject markup by // riding in through a variable the sanitizer never saw. - const body = safeTemplateReplace(safeBody, variables, { escapeHtml: true }); + // Re-checked after substitution, not only before it: expansion can remove + // the quoting that made a url() inert at sanitize time. See + // sanitizeInlineStylesAfterSubstitution. + const body = sanitizeInlineStylesAfterSubstitution( + safeTemplateReplace(safeBody, variables, { escapeHtml: true }) + ); const subject = safeTemplateReplace(campaign.subject || '', variables); const { css } = sanitizeCampaignCss(campaign.body_css); @@ -942,6 +974,7 @@ async function sendTest(campaignId, toEmail, adminId) { module.exports = { sanitizeCampaignBody, + sanitizeInlineStylesAfterSubstitution, sanitizeCampaignCss, unsubscribeToken, verifyUnsubscribeToken,