From 99f54a39546591d21df5ca11149770a161c447d9 Mon Sep 17 00:00:00 2001
From: Paul Nothaft <paul@MacStudio-von-Paul.local>
Date: Sat, 5 Sep 2026 07:27:09 +0200
Subject: [PATCH] fix(security): strip control characters before scanning CSS
 for url()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review follow-up on the sanitizer dedup.

sanitizeCSS already carried the rule — "any pass that can join tokens has
to happen before validation, not after" — written above the URL scan to
explain why it runs after the HTML-comment strip. The control-character
strip is exactly such a pass and sat eleven lines below it.

So `u<CTRL>rl(https://tracker.example/p.gif)` was scanned as clean, and
the strip below then joined it into a live remote request with no
warning. Newlines are control characters here too, so `u\nrl(...)` did
it without an exotic byte. Verified against the real function before and
after: all five variants returned a live remote url() and now return
`none` plus the blocked-URL warning.

This PR is what exposed it. Dropping newsletterService's second
stripRemoteCssUrls pass was right — the duplicate hid a defect in the
shared sanitizer rather than fixing it — but it removed the belt that
was catching this for the newsletter path. Fixing the ordering fixes it
for every caller instead of restoring the second pass.

Refs #1264
---
 .../utils/cssSanitizer.remoteUrls.test.js     | 18 +++++++++++++
 backend/src/utils/cssSanitizer.js             | 26 ++++++++++++-------
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js
index 4153406f..8863b151 100644
--- a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js
+++ b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js
@@ -221,6 +221,24 @@ describe('sanitizeCSS', () => {
     expect(asParsed(sanitized)).not.toContain('evil.example');
   });
 
+  it.each([
+    ['a C0 control character', '\u0001'],
+    ['a NUL byte', '\u0000'],
+    ['a DEL byte', '\u007F'],
+    // Newlines are control characters for this strip, so the bypass did not
+    // need an exotic byte — ordinary-looking wrapped CSS was enough.
+    ['a newline', '\n'],
+  ])('blocks a url() that only becomes one after %s is removed', (_label, ch) => {
+    // Same token-joining hazard as the HTML-comment case above: the control
+    // strip used to run AFTER the URL scan, so `u\u0001rl(...)` was scanned
+    // as clean and then joined into a live remote request, with no warning.
+    const { sanitized, warnings } = sanitizeCSS(
+      `.a{background:u${ch}rl(https://evil.example/p.gif)}`
+    );
+    expect(asParsed(sanitized)).not.toContain('evil.example');
+    expect(warnings.join(' ')).toContain('external URL');
+  });
+
   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/utils/cssSanitizer.js b/backend/src/utils/cssSanitizer.js
index 578262de..e1634ce9 100644
--- a/backend/src/utils/cssSanitizer.js
+++ b/backend/src/utils/cssSanitizer.js
@@ -291,12 +291,22 @@ function sanitizeCSS(cssContent) {
   // Remove HTML comments that might be used for injection
   sanitized = sanitized.replace(/<!--[\s\S]*?-->/g, '');
 
-  // URLs are scanned AFTER the comment strip, not before. Removing
-  // `<!--x-->` from `u<!--x-->rl(https://evil.example/p.gif)` JOINS the
-  // remaining characters into a live `url(...)` — so a scan that ran first
-  // saw no token, reported the input clean, and the transformation below it
-  // then produced exactly the request the scan was there to prevent. Any
-  // pass that can join tokens has to happen before validation, not after.
+  // Remove control characters BEFORE the URL scan. This is the same
+  // token-joining hazard as the HTML-comment strip above: dropping the
+  // \u0001 from `u\u0001rl(https://evil.example/p.gif)` joins the remainder
+  // into a live `url(...)`, so a scan that ran first saw no token and
+  // reported the input clean. Newlines are control characters too, which
+  // made `u\nrl(...)` the same bypass in ordinary-looking CSS.
+  // eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS
+  sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, '');
+
+  // URLs are scanned AFTER the comment and control-character strips, not
+  // before. Removing `<!--x-->` from `u<!--x-->rl(https://evil.example/p.gif)`
+  // JOINS the remaining characters into a live `url(...)` — so a scan that
+  // ran first saw no token, reported the input clean, and the transformation
+  // below it then produced exactly the request the scan was there to
+  // prevent. Any pass that can join tokens has to happen before validation,
+  // not after.
   const urlPass = stripDisallowedUrls(sanitized);
   if (urlPass.blocked > 0) {
     warnings.push(
@@ -306,10 +316,6 @@ function sanitizeCSS(cssContent) {
     sanitized = urlPass.sanitized;
   }
 
-  // Remove control characters
-  // eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS
-  sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, '');
-
   // Remove any remaining script-like content
   sanitized = sanitized.replace(/<[^>]*>/g, '/* BLOCKED TAG */');