From 1cf82746b72c2639547871e4e479d390760b7c1d Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 14:20:39 +0200 Subject: [PATCH] fix(security): close two CSS url() bypasses the sanitizer dedup exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by review against the correct base, and both are cases the second stripRemoteCssUrls pass had been catching before this PR removed it. Verified against the real functions before and after. An escaped quote outside a string. `\'` is an escaped identifier character, not a string opener, but the scanner stepped onto the apostrophe, entered string mode and copied the rest of the stylesheet unexamined — so `.hero{--marker:\';background:url(https://evil/p.gif)}` kept a live remote URL. Escapes are now consumed as a unit outside strings. An unterminated quote. Trusting one meant a single stray apostrophe disabled scanning for everything after it. An unclosed quote is a parse error, so the safe reading is to emit it as an ordinary character and keep scanning; a newline also ends a string, as it does in CSS. The entity mismatch behind the second case. sanitize-html writes `"` inside an attribute as `"`, so the scanner and the recipient's browser disagreed about where strings begin: in `style="font-family:"don't";background:url(...)"` the browser decodes first, reads the apostrophe as ordinary text inside a real string, and fetches the background — a tracking pixel by another name. Style attributes are now decoded before scanning and re-encoded after, which also stops the old code silently deleting quotes from the value. Also detaches the image handlers before releasing the canvas source. That one did NOT reproduce: measured in both Chromium and WebKit, neither fires `error` when the attribute is removed after a successful load. Applied anyway because the ordering is free and the failure it would cause is silent — canvasFailed set, the canvas swapped for an , and the image decoded a second time, the exact opposite of what the release is for. Refs #1264, #1287 --- .../newsletterService.sanitize.test.js | 29 ++++++++++++ .../utils/cssSanitizer.remoteUrls.test.js | 27 ++++++++++++ backend/src/services/newsletterService.js | 44 ++++++++++++++++++- backend/src/utils/cssSanitizer.js | 30 ++++++++++++- .../components/common/AuthenticatedImage.tsx | 8 ++++ 5 files changed, 135 insertions(+), 3 deletions(-) diff --git a/backend/__tests__/services/newsletterService.sanitize.test.js b/backend/__tests__/services/newsletterService.sanitize.test.js index c2e026be..59fb047d 100644 --- a/backend/__tests__/services/newsletterService.sanitize.test.js +++ b/backend/__tests__/services/newsletterService.sanitize.test.js @@ -92,6 +92,35 @@ describe('sanitizeCampaignBody', () => { expect(out).not.toContain('http://evil.example'); }); + it('blocks a tracking url() hidden behind " entities', () => { + // sanitize-html writes `"` inside an attribute as `"`, so the CSS + // scanner and the recipient's browser disagreed about where strings + // start: the browser decodes first and reads the apostrophe as ordinary + // text inside a real string, then fetches the background — while the + // scanner saw the apostrophe open a string and skipped past the url(). + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).not.toContain('evil.example'); + }); + + it('blocks a tracking url() hidden behind an escaped quote', () => { + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).not.toContain('evil.example'); + }); + + it('keeps a legitimate quoted font stack, re-encoded for the attribute', () => { + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).toContain('color:red'); + expect(out).toContain('Helvetica Neue'); + // Re-encoded, so the attribute stays well formed rather than being cut short. + expect(out).not.toMatch(/style="[^"]*"[^>]*"/); + }); + it('strips expression() out of an inline style', () => { const out = sanitizeCampaignBody('

hi

'); expect(out).not.toContain('expression('); diff --git a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js index 8863b151..ba30a667 100644 --- a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js +++ b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js @@ -239,6 +239,33 @@ describe('sanitizeCSS', () => { expect(warnings.join(' ')).toContain('external URL'); }); + it('blocks a url() hidden behind an escaped quote outside a string', () => { + // `\'` is an escaped identifier character, not a string opener. The + // scanner used to step onto the apostrophe, enter string mode, and copy + // the rest of the stylesheet — url() included — unexamined. + const { sanitized } = sanitizeCSS( + ".hero{--marker:\\';background:url(https://evil.example/p.gif)}" + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('does not let an unterminated quote hide everything after it', () => { + // An unclosed quote is a parse error. Trusting it meant one stray + // apostrophe disabled scanning for the remainder of the stylesheet, so + // the safe reading is to treat it as an ordinary character and continue. + const { sanitized } = sanitizeCSS( + "p{font-family:'don't;background:url(https://evil.example/p.gif)}" + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('still keeps legitimate quoted values and data: images intact', () => { + const font = sanitizeCSS('p{font-family:"Helvetica Neue",sans-serif;color:red}'); + expect(font.sanitized).toContain('"Helvetica Neue"'); + const data = sanitizeCSS(".a{background:url('data:image/png;base64,iVBORw0KGgo=')}"); + expect(data.sanitized).toContain('data:image/png'); + }); + it('still blocks the other forbidden patterns', () => { const { sanitized } = sanitizeCSS( '@import url("https://x/e.css"); .a{width:expression(alert(1));behavior:url(e.htc)}' diff --git a/backend/src/services/newsletterService.js b/backend/src/services/newsletterService.js index eaec1f76..8a29f79a 100644 --- a/backend/src/services/newsletterService.js +++ b/backend/src/services/newsletterService.js @@ -145,12 +145,52 @@ function sanitizeCampaignBody(html) { // rather than pattern-matching it, so the local pass this used to need is // gone. Keeping a second copy would mean two definitions of "disallowed" // drifting apart. + // + // Entities are decoded BEFORE the CSS is scanned, and re-encoded after. + // sanitize-html emits `"` inside an attribute as `"`, so the scanner + // and the recipient's browser otherwise disagree about where CSS strings + // begin: in `style="font-family:"don't";background:url(...)"` + // the browser decodes first and reads the apostrophe as ordinary text + // 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(css); - return sanitized ? `style="${sanitized.replace(/"/g, '')}"` : ''; + const { sanitized } = sanitizeCSS(decodeHtmlEntities(css)); + return sanitized ? `style="${encodeForAttribute(sanitized)}"` : ''; }); } +/** + * Decode the HTML entities sanitize-html emits inside attribute values, so + * CSS is scanned in the form the recipient's parser will see. One pass, so + * `&quot;` decodes to `"` and not to `"`. + */ +function decodeHtmlEntities(value) { + return String(value).replace( + /&(?:#(\d+)|#[xX]([0-9a-fA-F]+)|(quot|apos|amp|lt|gt));/g, + (whole, dec, hex, name) => { + if (dec !== undefined) { + const code = Number(dec); + return code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : whole; + } + if (hex !== undefined) { + const code = parseInt(hex, 16); + return code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : whole; + } + return { quot: '"', apos: '\'', amp: '&', lt: '<', gt: '>' }[name]; + } + ); +} + +/** Re-encode a sanitized value so it is safe inside a double-quoted attribute. */ +function encodeForAttribute(value) { + return String(value) + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(//g, '>'); +} + /** * Sanitize a campaign's optional `