From cceb24ed8f9b4f1ed0c460a433a6845c38286aa7 Mon Sep 17 00:00:00 2001
From: "Claude (Codeman maintainer)" <noreply@anthropic.com>
Date: Sun, 14 Jun 2026 22:21:13 +0200
Subject: [PATCH] fix(sanitizer): enforce the curated allowlist + make the test
 run order-independently
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review fixes on top of the DOMPurify mXSS hardening:

- Remove `USE_PROFILES: { html: true }` from the sanitize-html.js config. DOMPurify
  treats USE_PROFILES and ALLOWED_TAGS/ALLOWED_ATTR as mutually exclusive — with a
  profile set it resets the allow-lists to the full HTML profile and silently ignores
  the curated lists, so the tight markdown-only allowlist was dead config (still
  XSS-safe via FORBID + core, but far broader than intended: <button>/<input>/
  <details>/<audio>/<select>/<label> all survived). Dropping USE_PROFILES puts the
  curated ALLOWED_TAGS/ALLOWED_ATTR back in force; FORBID_TAGS/FORBID_ATTR stay as
  defense-in-depth and DOMPurify keeps its default safe-URI handling.

- Rewrite test/markdown-sanitizer.test.ts to run in the default node environment with
  an in-test jsdom window instead of a per-file jsdom environment. That environment
  externalizes node:fs/node:path under vite, so the suite failed to load in isolation
  ("No such built-in module: node:") and only survived the full CI run because an
  earlier node-env test happened to pre-cache node:fs — order-dependent and fragile.
  The rewrite is order-robust and adds an "allowlist is actually enforced" block
  (non-markdown tags must be dropped) that fails if USE_PROFILES is reintroduced.

Verified: 25/25 tests pass standalone under config/vitest.ci.config.ts; tsc, lint,
format:check, check:frontend-syntax, check:public-assets, and npm run build all clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 src/web/public/sanitize-html.js | 10 +++-
 test/markdown-sanitizer.test.ts | 81 +++++++++++++++++++++++++--------
 2 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/src/web/public/sanitize-html.js b/src/web/public/sanitize-html.js
index 2f32670e..78f7aba7 100644
--- a/src/web/public/sanitize-html.js
+++ b/src/web/public/sanitize-html.js
@@ -123,8 +123,14 @@
       // foreign-namespace roots and style so config drift can't silently re-admit them.
       FORBID_TAGS: ['style', 'svg', 'math', 'script', 'iframe', 'object', 'embed', 'form'],
       FORBID_ATTR: ['style'],
-      // No data: URIs except images; block the rest. SVG/MathML namespaces fully disabled.
-      USE_PROFILES: { html: true },
+      // NOTE: do NOT set USE_PROFILES here. DOMPurify treats USE_PROFILES and
+      // ALLOWED_TAGS/ALLOWED_ATTR as mutually exclusive — when a profile is set it
+      // RESETS the allow-lists to the full profile and silently ignores the curated
+      // lists above, widening the tag set far beyond what markdown emits. Relying on
+      // the explicit ALLOWED_TAGS/ALLOWED_ATTR keeps the tight allowlist in force;
+      // FORBID_TAGS/FORBID_ATTR remain as defense-in-depth. DOMPurify still applies
+      // its default safe-URI handling (blocks javascript:/vbscript:, allows
+      // http/https/mailto/tel + data: only on image tags).
       ALLOW_DATA_ATTR: false,
       ADD_ATTR: [],
       RETURN_DOM: false,
diff --git a/test/markdown-sanitizer.test.ts b/test/markdown-sanitizer.test.ts
index 98a517b3..b9213c97 100644
--- a/test/markdown-sanitizer.test.ts
+++ b/test/markdown-sanitizer.test.ts
@@ -1,4 +1,3 @@
-// @vitest-environment jsdom
 /**
  * COD-56 — markdown HTML sanitizer (mXSS hardening).
  *
@@ -8,46 +7,70 @@
  * DENYLIST and is mXSS-prone — it never stripped `svg`/`math`/`style`, so foreign-namespace and
  * CSS vectors survived.
  *
- * This suite drives the EXACT shipping artifacts under jsdom:
+ * This suite drives the EXACT shipping artifacts:
  *   - src/web/public/vendor/dompurify.min.js   (the vendored sanitizer)
  *   - src/web/public/sanitize-html.js          (our allowlist config wired to DOMPurify)
  *
+ * It runs in the DEFAULT node environment (it deliberately does NOT declare a per-file jsdom
+ * environment) and constructs a jsdom window here, then binds the vendored DOMPurify to it. A
+ * per-file jsdom environment externalizes node:fs/node:path under vite, which made this suite fail
+ * to load when
+ * run in isolation (it only survived the full CI run because an earlier node-env test happened to
+ * pre-cache node:fs). Building the window in-test keeps fs/path native and the suite order-robust.
+ *
  * It feeds a corpus of mXSS payloads (svg/math/style/namespace-confusion/event-handler) and
- * asserts the output carries NO script-executing constructs, and that legitimate
- * markdown-rendered HTML survives unchanged.
+ * asserts the output carries NO script-executing constructs, that the curated allowlist is
+ * actually enforced (non-markdown tags dropped), and that legitimate markdown-rendered HTML
+ * survives unchanged. A faithful re-implementation of the OLD denylist is included and asserted to
+ * LET payloads through — the gap this fix closes.
  *
- * RED demonstration: a faithful re-implementation of the OLD denylist sanitizer is included and
- * asserted to LET payloads through — the gap this fix closes.
- *
- * No port / server needed; pure DOM logic in jsdom.
+ * No port / server needed.
  */
 
 import { describe, it, expect, beforeAll } from 'vitest';
 import { readFileSync } from 'node:fs';
 import { join } from 'node:path';
+import { JSDOM } from 'jsdom';
 
 const publicDir = join(process.cwd(), 'src/web/public');
 
-/** Build the SHIPPING sanitizer the way the browser does: vendored DOMPurify + sanitize-html.js,
- *  both evaluated inside the (jsdom) window so DOMPurify binds to a real DOM. */
+// One jsdom window shared by the shipping sanitizer (bound to its DOMPurify) and the old-denylist
+// reference impl (which needs a DOM `document`).
+const dom = new JSDOM('<!DOCTYPE html><html><body></body></html>');
+const jsdomWindow = dom.window as unknown as Window & typeof globalThis;
+const jsdomDocument = jsdomWindow.document;
+
+/** Build the SHIPPING sanitizer the way the browser does: vendored DOMPurify (bound to our jsdom
+ *  window) + the EXACT CONFIG from sanitize-html.js — so the same allow/forbid lists are exercised
+ *  under vitest without a real browser. */
 function loadShippingSanitizer(): (html: string) => string {
-  const win = window as unknown as Record<string, unknown>;
-  // Evaluate the vendored UMD in the jsdom global context (mirrors <script src=vendor/...>).
   const dompurifySrc = readFileSync(join(publicDir, 'vendor/dompurify.min.js'), 'utf8');
-  // sanitize-html.js is a classic script that reads global DOMPurify and sets sanitizeMarkdownHtml.
   const sanitizeSrc = readFileSync(join(publicDir, 'sanitize-html.js'), 'utf8');
-  // jsdom's window is the global in this vitest environment; eval the classic scripts into it
-  // so DOMPurify and sanitizeMarkdownHtml bind to the jsdom window exactly as in the browser.
-  (0, eval)(dompurifySrc);
-  (0, eval)(sanitizeSrc);
-  const fn = win.sanitizeMarkdownHtml as ((html: string) => string) | undefined;
+
+  // dompurify.min.js is a UMD — evaluate it as CommonJS to obtain the factory (createDOMPurify),
+  // then bind it to our jsdom window so DOMPurify sanitizes against a real DOM.
+  const dpModule: { exports: unknown } = { exports: {} };
+  // eslint-disable-next-line @typescript-eslint/no-implied-eval, no-new-func
+  new Function('module', 'exports', dompurifySrc)(dpModule, dpModule.exports);
+  const factory = dpModule.exports as (win: unknown) => { sanitize: (h: string, c?: unknown) => string };
+  const DOMPurify = factory(jsdomWindow);
+
+  // sanitize-html.js exposes createMarkdownSanitizer via its CommonJS export.
+  const sanModule: { exports: { createMarkdownSanitizer?: (dp: unknown) => (html: string) => string } } = {
+    exports: {},
+  };
+  // eslint-disable-next-line @typescript-eslint/no-implied-eval, no-new-func
+  new Function('module', 'exports', sanitizeSrc)(sanModule, sanModule.exports);
+  const create = sanModule.exports.createMarkdownSanitizer;
+  if (typeof create !== 'function') throw new Error('createMarkdownSanitizer not exported');
+  const fn = create(DOMPurify);
   if (typeof fn !== 'function') throw new Error('sanitizeMarkdownHtml not wired');
   return fn;
 }
 
 /** Faithful copy of the OLD denylist _sanitizeHtml (app.js pre-COD-56) — used only to prove RED. */
 function oldDenylistSanitize(html: string): string {
-  const tpl = document.createElement('template');
+  const tpl = jsdomDocument.createElement('template');
   tpl.innerHTML = html;
   const frag = tpl.content;
   for (const el of frag.querySelectorAll('script, iframe, object, embed, form, base, meta, link, style')) {
@@ -66,7 +89,7 @@ function oldDenylistSanitize(html: string): string {
       }
     }
   }
-  const div = document.createElement('div');
+  const div = jsdomDocument.createElement('div');
   div.appendChild(frag);
   return div.innerHTML;
 }
@@ -126,6 +149,24 @@ describe('COD-56 markdown sanitizer (DOMPurify allowlist)', () => {
     }
   });
 
+  describe('curated allowlist is actually enforced (USE_PROFILES must not override it)', () => {
+    // These tags are in DOMPurify's default html profile but NOT in the curated ALLOWED_TAGS.
+    // If USE_PROFILES were set, the profile would override the allowlist and these would survive.
+    const NON_MARKDOWN_TAGS: { name: string; html: string; tag: string }[] = [
+      { name: 'button', html: '<button>click</button>', tag: '<button' },
+      { name: 'input', html: '<input value="x">', tag: '<input' },
+      { name: 'details', html: '<details open>d</details>', tag: '<details' },
+      { name: 'audio', html: '<audio controls></audio>', tag: '<audio' },
+      { name: 'select/option', html: '<select><option>o</option></select>', tag: '<select' },
+      { name: 'label', html: '<label>l</label>', tag: '<label' },
+    ];
+    for (const { name, html, tag } of NON_MARKDOWN_TAGS) {
+      it(`drops non-markdown tag: ${name}`, () => {
+        expect(sanitize(html).toLowerCase()).not.toContain(tag);
+      });
+    }
+  });
+
   describe('legitimate markdown-rendered HTML survives', () => {
     it('keeps bold, links, lists, code, headings, tables, safe images', () => {
       const md =