Give Lexxy its own DOMPurify instance, and an allowlist per editor

src/config/dom_purify.js

@@ -1,7 +1,24 @@
-import DOMPurify from "dompurify"
+import createDOMPurify from "dompurify"
 import { getCSSFromStyleObject, getStyleObjectFromCSS } from "@lexical/selection"
 import Lexxy from "./lexxy"
 
+// Lexxy's own DOMPurify instance, deliberately not the shared default export.
+//
+// dompurify's default export is a singleton, and both its config and its hooks
+// are global to every consumer in the bundle. That makes configuring it from an
+// editor's connectedCallback actively dangerous for the host app: DOMPurify
+// treats a persistent config as final, so once setConfig() has run, every
+// later `sanitize(html, config)` anywhere in the app silently ignores its own
+// config argument. An app sanitizing untrusted HTML with, say,
+// `{ ALLOW_DATA_ATTR: false }` would keep passing that option and stop getting
+// it the moment a Lexxy editor connected — with no error and no visible change
+// at the call site.
+//
+// Calling the default export with a window returns a fresh, independent
+// instance. This one carries the hooks and config below; nothing we do here can
+// reach the app's instance, and nothing it does can reach ours.
+const DOMPurify = createDOMPurify(window)
+
 // alt is inert on every element it can appear on, so it sits in the blanket
 // list. srcset is deliberately absent — it carries URLs, so it belongs to a
 // consumer that declares it.

src/elements/editor.js

@@ -361,7 +361,7 @@ export class LexicalEditorElement extends HTMLElement {
 
   #readSanitizedEditorValue() {
     return this.editor?.read(() => {
-      return sanitize($generateHtmlFromNodes(this.editor, null))
+      return sanitize($generateHtmlFromNodes(this.editor, null), this.editor)
     }) ?? null
   }
 
@@ -755,7 +755,7 @@ export class LexicalEditorElement extends HTMLElement {
   }
 
   #configureSanitizer(editor) {
-    setSanitizerConfig(this.#getAllowedElements(editor))
+    setSanitizerConfig(editor, this.#getAllowedElements(editor))
   }
 
   #getAllowedElements(editor) {

src/helpers/sanitization_helper.js

@@ -1,10 +1,37 @@
 import { DOMPurify, buildConfig } from "../config/dom_purify"
 
-export function setSanitizerConfig(allowedTags) {
-  DOMPurify.clearConfig()
-  DOMPurify.setConfig(buildConfig(allowedTags))
+// Sanitizer config is per editor, and passed to each sanitize() call.
+//
+// Neither of those is incidental. This used to call DOMPurify.setConfig() on the
+// shared singleton, which had two distinct consequences:
+//
+// 1. A persistent config is final — DOMPurify ignores the per-call config once
+//    one is set — so it silently disarmed the sanitizing of any host app that
+//    also imports dompurify. See config/dom_purify for why we now own our
+//    instance; keeping the config per-call means there is no global sanitizer
+//    state left even on that instance.
+//
+// 2. One config for the whole module meant the last editor to connect decided
+//    how every other editor on the page sanitized. That is not cosmetic: an
+//    editor's `value` is sanitized on read, so a rich editor sharing a page with
+//    a plain one would silently drop its own headings, lists and links from the
+//    value it submits. Keying on the editor is what fixes that.
+//
+// Registered against the Lexical editor, which is the identity both call sites
+// have to hand: the element has it as `this.editor`, and nodes receive it as the
+// second argument to createDOM().
+const configs = new WeakMap()
+
+// Only reached if sanitize() is called for an editor that never registered one.
+// Falling back to the most recent config keeps the old behaviour rather than
+// silently widening the allowlist to DOMPurify's permissive defaults.
+let fallbackConfig = {}
+
+export function setSanitizerConfig(editor, allowedTags) {
+  fallbackConfig = buildConfig(allowedTags)
+  configs.set(editor, fallbackConfig)
 }
 
-export function sanitize(html) {
-  return DOMPurify.sanitize(html)
+export function sanitize(html, editor) {
+  return DOMPurify.sanitize(html, configs.get(editor) ?? fallbackConfig)
 }

src/nodes/custom_action_text_attachment_node.js

@@ -72,11 +72,13 @@ export class CustomActionTextAttachmentNode extends DecoratorNode {
     this.plainText = plainText ?? extractPlainTextFromHtml(innerHtml)
   }
 
-  createDOM() {
+  createDOM(_config, editor) {
     const figure = createElement(this.tagName, { "content-type": this.contentType, "data-lexxy-decorator": true, draggable: true })
     figure.dataset.lexicalNodeKey = this.__key
 
-    figure.insertAdjacentHTML("beforeend", sanitize(this.innerHtml))
+    // The editor is passed through so this content is sanitized with its own
+    // allowlist rather than whichever editor connected most recently.
+    figure.insertAdjacentHTML("beforeend", sanitize(this.innerHtml, editor))
 
     const deleteButton = createElement("lexxy-node-delete-button")
     figure.appendChild(deleteButton)

test/browser/fixtures/sanitizer-isolation.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+  <meta name="viewport" content="width=device-width,initial-scale=1">
+  <title>Lexxy Test — sanitizer isolation</title>
+  <link rel="stylesheet" href="/styles.css">
+</head>
+<body>
+  <form>
+    <div class="body">
+      <lexxy-editor id="rich" class="lexxy-content" placeholder="Rich"></lexxy-editor>
+    </div>
+
+    <!-- Connects second, so under the old module-level config it was this
+         editor's allowlist that governed the rich one above. -->
+    <div class="body">
+      <lexxy-editor id="plain" preset="plain" class="lexxy-content" placeholder="Plain"></lexxy-editor>
+    </div>
+
+    <div class="events"></div>
+  </form>
+
+  <script type="module" src="/sanitizer-isolation.js"></script>
+</body>
+</html>

test/browser/fixtures/sanitizer-isolation.js

@@ -0,0 +1,6 @@
+import { configure } from "lexxy"
+import "./events_logger.js"
+
+// A plain-text preset alongside the default rich one. The two resolve different
+// importable tags, which is what makes their sanitizer allowlists differ.
+configure({ plain: { richText: false } })

test/browser/tests/editor/sanitizer_isolation.test.js

@@ -0,0 +1,56 @@
+import { test } from "../../test_helper.js"
+import { EditorHandle } from "../../helpers/editor_handle.js"
+import { expect } from "@playwright/test"
+
+// Each editor sanitizes with its own allowlist.
+//
+// The config used to be one module-level value installed with
+// DOMPurify.setConfig(), so the last editor to connect decided how every editor
+// on the page sanitized. An editor's `value` is sanitized on read, so a rich
+// editor sharing a page with a plain one silently dropped its own headings and
+// lists from the value it submitted — with nothing different on screen, because
+// the editor DOM was never the thing being rewritten.
+//
+// The unit test for this runs in JSDOM. This one exists because the failure
+// involves the custom-element lifecycle, cached value reads and the bundled
+// build, none of which JSDOM proves anything about.
+test.describe("sanitizer isolation between editors", () => {
+  const RICH = "<h1>Title</h1><ul><li>one</li></ul><p>body</p>"
+
+  test("a rich editor keeps its formatting after a plain editor connects", async ({ page }) => {
+    const rich = new EditorHandle(page, "#rich")
+    const plain = new EditorHandle(page, "#plain")
+
+    await page.goto("/sanitizer-isolation.html")
+    await rich.waitForConnected()
+    await plain.waitForConnected()
+
+    await rich.setValue(RICH)
+
+    // Editing clears the cached value, so the next read re-sanitizes. That read
+    // is what used to pick up the plain editor's allowlist.
+    await rich.click()
+    await rich.send("!")
+
+    const value = await rich.value()
+    expect(value).toContain("<h1>")
+    expect(value).toContain("<ul>")
+  })
+
+  test("the plain editor still sanitizes with its own narrower allowlist", async ({ page }) => {
+    const rich = new EditorHandle(page, "#rich")
+    const plain = new EditorHandle(page, "#plain")
+
+    await page.goto("/sanitizer-isolation.html")
+    await rich.waitForConnected()
+    await plain.waitForConnected()
+
+    await plain.setValue(RICH)
+    await plain.click()
+    await plain.send("!")
+
+    // Control: proves the two editors really do resolve different allowlists, so
+    // the assertion above isn't passing because both are simply permissive.
+    expect(await plain.value()).not.toContain("<h1>")
+  })
+})

test/javascript/unit/editor/sanitizer_isolation.test.js

@@ -0,0 +1,79 @@
+import { afterEach, expect, test } from "vitest"
+import { $getRoot } from "lexical"
+import Lexxy from "src/config/lexxy"
+import { createTestEditor, destroyTestEditor, setContent, tick } from "../helpers/editor_helper"
+
+// Each editor sanitizes with its own allowlist.
+//
+// The sanitizer config used to be one module-level value installed with
+// DOMPurify.setConfig(), so the last editor to connect decided how every other
+// editor on the page sanitized. Because an editor's `value` is sanitized on
+// read, a rich editor sharing a page with a plain one would silently drop its
+// own headings and lists from the value it submitted — data loss on save, with
+// nothing visible in the editor to suggest it.
+
+const RICH = "<h1>Title</h1><ul><li>one</li></ul><p>body</p>"
+
+let rich, plain
+
+afterEach(async () => {
+  await destroyTestEditor(rich)
+  await destroyTestEditor(plain)
+  rich = plain = undefined
+})
+
+async function typeInto(editorElement) {
+  editorElement.editor.update(() => {
+    $getRoot().getLastDescendant()?.select().insertText("!")
+  }, { discrete: true })
+  await tick()
+}
+
+test("a rich editor keeps its formatting after a plain editor connects", async () => {
+  Lexxy.configure({ plain: { richText: false } })
+
+  rich = await createTestEditor({ value: RICH })
+  plain = await createTestEditor({ attributes: { preset: "plain" } })
+  await tick()
+
+  // Editing clears the cached value, so the next read re-sanitizes. That read is
+  // what used to pick up the plain editor's allowlist.
+  await typeInto(rich)
+
+  expect(rich.value).toContain("<h1>")
+  expect(rich.value).toContain("<ul>")
+})
+
+test("the plain editor still sanitizes with its own narrower allowlist", async () => {
+  Lexxy.configure({ plain: { richText: false } })
+
+  rich = await createTestEditor({ value: RICH })
+  plain = await createTestEditor({ attributes: { preset: "plain" }, value: RICH })
+  await tick()
+
+  await typeInto(plain)
+
+  // Control: proves the two editors really do resolve different allowlists, so
+  // the assertion above isn't passing because both are simply permissive.
+  expect(plain.value).not.toContain("<h1>")
+})
+
+test("attachment content is sanitized with its own editor's allowlist", async () => {
+  Lexxy.configure({ plain: { richText: false } })
+
+  const attachment = '<action-text-attachment content-type="application/vnd.test.thing" ' +
+    'content="&lt;blockquote&gt;quoted&lt;/blockquote&gt;"></action-text-attachment>'
+
+  rich = await createTestEditor()
+  plain = await createTestEditor({ attributes: { preset: "plain" } })
+  await tick()
+
+  // The content has to be set *after* the plain editor connects. A decorator
+  // node builds its DOM once, so loading the attachment up front would render
+  // it before there was any competing config — passing whether or not the bug
+  // is present.
+  await setContent(rich, `<p>${attachment}</p>`)
+
+  // blockquote is in the rich editor's allowlist but not the plain one's.
+  expect(rich.querySelector("action-text-attachment").innerHTML).toContain("<blockquote>")
+})

test/javascript/unit/helpers/sanitization_helper.test.js

@@ -0,0 +1,99 @@
+import { beforeEach, expect, test } from "vitest"
+import DOMPurify from "dompurify"
+import { sanitize, setSanitizerConfig } from "src/helpers/sanitization_helper"
+
+// Lexxy must not configure the DOMPurify singleton the host app also imports.
+//
+// DOMPurify treats a persistent config as final: after setConfig(), every later
+// sanitize(html, config) ignores its config argument. Lexxy configures its
+// sanitizer when an editor connects, so if that ran against the shared instance
+// it would silently disarm the app's own sanitizing — options the app is still
+// passing, and still relying on, would stop applying with no error anywhere.
+//
+// These assert isolation in both directions against the real singleton. The
+// config is keyed by editor, but nothing here depends on it being a real
+// Lexical editor — any identity will do.
+
+const DIRTY = '<span data-controller="evil" class="keep">hi</span>'
+const editor = { name: "stand-in for a Lexical editor" }
+
+beforeEach(() => {
+  DOMPurify.clearConfig()
+  setSanitizerConfig(editor, [ "span", "p", "strong" ])
+})
+
+test("configuring Lexxy's sanitizer leaves the app's per-call config working", () => {
+  // The app's call still gets exactly what it asked for.
+  expect(DOMPurify.sanitize(DIRTY, { ALLOW_DATA_ATTR: false }))
+    .toBe('<span class="keep">hi</span>')
+
+  // Control: the option is what's doing the work, so this test can't pass
+  // because something else happened to strip data-*.
+  expect(DOMPurify.sanitize(DIRTY, {})).toContain("data-controller")
+})
+
+test("the app's persistent config does not reach into Lexxy's sanitizing", () => {
+  DOMPurify.setConfig({ ALLOWED_TAGS: [ "b" ], ALLOWED_ATTR: [] })
+
+  expect(sanitize('<span class="keep">hi</span>', editor)).toBe('<span class="keep">hi</span>')
+})
+
+test("Lexxy's hooks are not installed on the shared instance", () => {
+  // The style filter hook in config/dom_purify would rewrite this to color
+  // only, dropping the disallowed property.
+  expect(DOMPurify.sanitize('<p style="color: red; position: fixed">x</p>', { ALLOWED_ATTR: [ "style" ] }))
+    .toContain("position")
+})
+
+test("sanitize applies the configured allowlist", () => {
+  expect(sanitize("<span>keep</span><script>evil()</script>", editor)).toBe("<span>keep</span>")
+})
+
+test("each editor keeps its own allowlist", () => {
+  const other = { name: "a second editor" }
+  setSanitizerConfig(other, [ "strong" ])
+
+  expect(sanitize("<span>a</span><strong>b</strong>", editor)).toBe("<span>a</span><strong>b</strong>")
+  expect(sanitize("<span>a</span><strong>b</strong>", other)).toBe("a<strong>b</strong>")
+})
+
+// The regression this change exists for, at attribute rather than tag level.
+//
+// "each editor keeps its own allowlist" above only varies *tags*, so a
+// per-editor rule about an *attribute* could still be shared or misapplied
+// without failing anything. `content` is the attribute that matters: it carries
+// the attachment's serialized markup, so losing it destroys the attachment on
+// the round trip rather than merely trimming it.
+//
+// Both cases register the *other* editor last on purpose. Under the module-level
+// config this replaced, the last editor to connect decided for every editor on
+// the page, and each direction below catches one half of that: the first loses an
+// attachment that should have survived, the second keeps `content` on an element
+// whose own config denied it.
+const ATTACHMENT = '<action-text-attachment sgid="x" content-type="text/html" content="&lt;span&gt;hi&lt;/span&gt;"></action-text-attachment>'
+const ALLOWS_CONTENT = [ { tag: "action-text-attachment", attributes: [ "content", "content-type", "sgid" ] } ]
+const DENIES_CONTENT = [ { tag: "action-text-attachment", attributes: [ "content-type", "sgid" ] } ]
+
+test("an editor allowing attachment content keeps it when another editor denies it", () => {
+  const allows = { name: "attachments on" }
+  const denies = { name: "attachments off" }
+
+  setSanitizerConfig(allows, ALLOWS_CONTENT)
+  setSanitizerConfig(denies, DENIES_CONTENT) // registered last
+
+  expect(sanitize(ATTACHMENT, allows)).toContain("content=")
+})
+
+test("an editor denying attachment content strips it when another editor allows it", () => {
+  const denies = { name: "attachments off" }
+  const allows = { name: "attachments on" }
+
+  setSanitizerConfig(denies, DENIES_CONTENT)
+  setSanitizerConfig(allows, ALLOWS_CONTENT) // registered last
+
+  const sanitized = sanitize(ATTACHMENT, denies)
+
+  expect(sanitized).not.toContain("content=\"")
+  // Only the attribute is refused — the element itself is still allowed here.
+  expect(sanitized).toContain("action-text-attachment")
+})