Give Lexxy its own DOMPurify instance, and an allowlist per editor - #1231
Give Lexxy its own DOMPurify instance, and an allowlist per editor#1231jeremy wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Creates an isolated DOMPurify instance and assigns sanitizer allowlists per Lexical editor.
Changes:
- Prevents Lexxy from modifying the host application’s DOMPurify singleton.
- Stores sanitizer configuration per editor.
- Adds unit and browser isolation tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/config/dom_purify.js |
Creates Lexxy’s private DOMPurify instance. |
src/helpers/sanitization_helper.js |
Stores and applies per-editor configurations. |
src/elements/editor.js |
Supplies editor identity during sanitization. |
src/nodes/custom_action_text_attachment_node.js |
Uses the owning editor’s allowlist. |
test/javascript/unit/helpers/sanitization_helper.test.js |
Tests singleton and configuration isolation. |
test/javascript/unit/editor/sanitizer_isolation.test.js |
Tests isolation between real editor instances. |
test/browser/tests/editor/sanitizer_isolation.test.js |
Exercises isolation in the bundled browser build. |
test/browser/fixtures/sanitizer-isolation.js |
Defines the narrower preset. |
test/browser/fixtures/sanitizer-isolation.html |
Provides the two-editor fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bc3 QA: this fixes a live bug on a shipping path, and I can show itThe disarm this PR describes is not hypothetical for bc3. import DOMPurify from "dompurify"
const sanitizerConfig = {
ALLOWED_TAGS: [ "bc-mention", "span", "img" ],
ALLOWED_ATTR: [ "alt", "class", "gid", "height", "src", "title", "width" ],
CUSTOM_ELEMENT_HANDLING: { tagNameCheck: /^bc-mention$/, ... }
}
export const sanitizeAttachmentContent = (html) => DOMPurify.sanitize(html, sanitizerConfig)Its own comment says where it is used: the chat pending-line controller and the Trix paste serializer — "so both surfaces apply the same allowlist and rejection policy." Under 0.9.29 they don't. Once a Lexxy editor connects,
Row 2 is bc3 still passing I have not assessed how far that widening can be pushed on those two paths — that needs someone who knows what bc3 does with the output downstream — but bc3 clearly intends Rest of the bc3 QAAgainst
Browser check of the named regressionThe regression this PR must not lose — two editors, differing attachment configs — verified in Chromium against the integration branch:
CI: green (Rails-main and Rails-8.1 legs both pass; the one earlier red leg was a Selenium flake in |
|
Handoff note for the whole campaign, including QA results and what still needs a human decision: #1234 |
930c084 to
22d0874
Compare
Two faults, one mechanism: Lexxy was calling setConfig() on the DOMPurify
singleton.
That singleton is shared with the host app, and DOMPurify treats a persistent
config as final — once setConfig() has run, every later `sanitize(html, config)`
anywhere in the bundle silently ignores its own config argument. An app
sanitizing untrusted HTML with `{ ALLOW_DATA_ATTR: false }` would keep passing
that option and stop getting it the moment a Lexxy editor connected, with no
error and nothing visible at the call site. Calling dompurify's default export
with a window returns a fresh instance; ours carries our hooks and config, and
neither side can reach the other.
One module-level config also meant the last editor to connect decided how every
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 silently
dropped its own headings, lists and links from the value it submitted. The
config is now keyed by the Lexical editor — the identity both call sites already
have, as `this.editor` on the element and as createDOM()'s second argument — and
passed to each sanitize() call rather than set persistently, so no global
sanitizer state is left even on our own instance.
createDOM(_config, editor) is not a new signature. It is a Lexical override, and
Lexical has always called it with both arguments; neither `sanitize` nor
`setSanitizerConfig` is exported from src/index.js, so there is no external API
change here.
Covered at both levels, because neither catches the other: unit tests assert
isolation in both directions against the real singleton, and a Playwright
fixture puts two differently configured editors on one page — the reproduction
that fails on main.
Review catch, and a fair one: the PR names "two editors with differing attachment configs, the one denying content strips it and the one allowing it keeps it" as the regression that must keep passing, and then only tested tags. A per-editor rule about an *attribute* could have been shared or misapplied without failing anything here. `content` is the attribute that matters, because it carries the attachment's serialized markup — losing it destroys the attachment on the round trip rather than trimming it. Both directions are covered, each registering the other editor last, which is how the module-level config this replaced actually failed. Mutation-checked against that old behaviour: stop keying the config by editor and both new cases fail.
22d0874 to
43fc302
Compare
Consumer action: none here, but the PR that follows this one adds a CSP
requirement that exists because of this change. Read #trusted-types with it.
Two faults, one mechanism: Lexxy was calling
setConfig()on the DOMPurifysingleton.
It disarmed the host app's sanitizer
That singleton is shared with the host app, and DOMPurify treats a persistent
config as final — once
setConfig()has run, every latersanitize(html, config)anywhere in the bundle silently ignores its own configargument. An app sanitizing untrusted HTML with
{ ALLOW_DATA_ATTR: false }would keep passing that option and stop getting it the moment a Lexxy editor
connected. No error, nothing visible at the call site.
Calling dompurify's default export with a window returns a fresh instance. Ours
carries our hooks and config; neither side can reach the other.
It let the last editor to connect decide for all of them
One module-level config meant exactly that. Not cosmetic: an editor's
valueissanitized on read, so a rich editor sharing a page with a plain one silently
dropped its own headings, lists and links from the value it submitted. That
is data loss, and only on a page with more than one editor.
The config is now keyed by the Lexical editor — the identity both call sites
already have, as
this.editoron the element and ascreateDOM()'s secondargument — and passed to each
sanitize()call rather than set persistently, sono global sanitizer state is left even on our own instance.
Not an API change
createDOM(_config, editor)is a Lexical override, and Lexical has always calledit with both arguments. Neither
sanitizenorsetSanitizerConfigis exportedfrom
src/index.js. Nothing here is reachable from outside the package.Verification
Both levels, because neither catches the other. Unit tests assert isolation in
both directions against the real singleton; a Playwright fixture puts two
differently configured editors on one page — the reproduction that fails on
main.The regression that must keep passing: two editors with differing attachment
configs, where the one denying
contentstrips it and the one allowing it keepsit.
Part of a series re-filing #1227 at reviewable scope, after #1227 was reverted
from
mainin 8c64aa4. Merge order: #1228 → #1229 → #1230 → this →#trusted-types → #1226. Nothing here is released.
Draft: needs human review and a soak period before merging.