diff --git a/src/config/dom_purify.js b/src/config/dom_purify.js index 23d5f5f87..203d1f3f3 100644 --- a/src/config/dom_purify.js +++ b/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. diff --git a/src/elements/editor.js b/src/elements/editor.js index 6739dd4d0..710ba0c49 100644 --- a/src/elements/editor.js +++ b/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) { diff --git a/src/helpers/sanitization_helper.js b/src/helpers/sanitization_helper.js index a745c680a..55ee7ffec 100644 --- a/src/helpers/sanitization_helper.js +++ b/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) } diff --git a/src/nodes/custom_action_text_attachment_node.js b/src/nodes/custom_action_text_attachment_node.js index 6500ff656..f9a969903 100644 --- a/src/nodes/custom_action_text_attachment_node.js +++ b/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) diff --git a/test/browser/fixtures/sanitizer-isolation.html b/test/browser/fixtures/sanitizer-isolation.html new file mode 100644 index 000000000..049848e3e --- /dev/null +++ b/test/browser/fixtures/sanitizer-isolation.html @@ -0,0 +1,26 @@ + + + + + + Lexxy Test — sanitizer isolation + + + +
+
+ +
+ + +
+ +
+ +
+
+ + + + diff --git a/test/browser/fixtures/sanitizer-isolation.js b/test/browser/fixtures/sanitizer-isolation.js new file mode 100644 index 000000000..59ac9bad3 --- /dev/null +++ b/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 } }) diff --git a/test/browser/tests/editor/sanitizer_isolation.test.js b/test/browser/tests/editor/sanitizer_isolation.test.js new file mode 100644 index 000000000..b7eea8fa9 --- /dev/null +++ b/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 = "

Title

body

" + + 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("

") + expect(value).toContain("