fix(security): close two CSS url() bypasses the sanitizer dedup exposed

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
<img>, and the image decoded a second time, the exact opposite of what
the release is for.

Refs #1264, #1287

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(
+      `<p style="font-family:&quot;don't&quot;;background:url(https://evil.example/p.gif)">hi</p>`
+    );
+    expect(out).not.toContain('evil.example');
+  });
+
+  it('blocks a tracking url() hidden behind an escaped quote', () => {
+    const out = sanitizeCampaignBody(
+      `<p style="--m:\\';background:url(https://evil.example/p.gif)">hi</p>`
+    );
+    expect(out).not.toContain('evil.example');
+  });
+
+  it('keeps a legitimate quoted font stack, re-encoded for the attribute', () => {
+    const out = sanitizeCampaignBody(
+      `<p style="color:red;font-family:&quot;Helvetica Neue&quot;,sans-serif">hi</p>`
+    );
+    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('<p style="width:expression(alert(1))">hi</p>');
     expect(out).not.toContain('expression(');

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)}'

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
+ * `"` 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, '&amp;')
+    .replace(/"/g, '&quot;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;');
+}
+
 /**
  * Sanitize a campaign's optional `<style>` block. Delegates to the shared
  * cssSanitizer, which already blocks `@import`, `expression(`, `behavior:`,

backend/src/utils/cssSanitizer.js

@@ -140,6 +140,18 @@ function stripDisallowedUrls(css) {
       continue;
     }
 
+    // --- escape OUTSIDE a string -----------------------------------------
+    // `\'` is an escaped identifier character, not the start of a string.
+    // Without this the scanner stepped onto the apostrophe, entered string
+    // mode, and copied the rest of the stylesheet unscanned — so
+    // `.hero{--marker:\';background:url(https://evil.example/p.gif)}` kept a
+    // live remote URL. Consume the escape and its escaped character together.
+    if (input[i] === '\\' && i + 1 < input.length) {
+      out += input.slice(i, i + 2);
+      i += 2;
+      continue;
+    }
+
     // --- string ----------------------------------------------------------
     // Escape-aware: `\"` inside a double-quoted string does NOT close it.
     // Decoding escapes up front (an earlier attempt) turned that into a real
@@ -147,11 +159,27 @@ function stripDisallowedUrls(css) {
     if (input[i] === '"' || input[i] === '\'') {
       const quote = input[i];
       let j = i + 1;
+      let closed = false;
       while (j < input.length) {
         if (input[j] === '\\') { j += 2; continue; }
-        if (input[j] === quote) { j += 1; break; }
+        // A newline ends a string in CSS (it produces a bad-string token), so
+        // an unclosed quote must not run past the end of its own line.
+        if (input[j] === '\n' || input[j] === '\r' || input[j] === '\f') break;
+        if (input[j] === quote) { j += 1; closed = true; break; }
         j += 1;
       }
+      // An UNTERMINATED quote is a parse error, and trusting it is how a
+      // stray apostrophe hid everything after it: `font-family:&quot;don't`
+      // opened a string that swallowed the url() following it, while the
+      // recipient's browser — which decodes the entity first — saw the
+      // apostrophe safely inside a real string and made the request. Failing
+      // closed here means emitting the quote as an ordinary character and
+      // carrying on scanning, so a later url() is still examined.
+      if (!closed) {
+        out += input[i];
+        i += 1;
+        continue;
+      }
       out += input.slice(i, Math.min(j, input.length));
       i = Math.min(j, input.length);
       continue;

frontend/src/components/common/AuthenticatedImage.tsx

@@ -283,6 +283,14 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
       // (#1287). Nothing redraws from `imageRef` afterwards: drawToCanvas
       // has this one caller.
       if (drawn) {
+        // Handlers off BEFORE the src goes. Measured in Chromium and WebKit:
+        // neither fires `error` when the attribute is removed after a
+        // successful load, so this is not fixing an observed bug — but if any
+        // engine ever did, `onerror` would set canvasFailed, swap the canvas
+        // for a plain <img>, and decode the image a second time, which is the
+        // exact opposite of what this release is for. The ordering is free.
+        img.onload = null;
+        img.onerror = null;
         imageRef.current = null;
         img.removeAttribute('src');
       }