Move to dompurify 3.4.13, and validate attachment URLs ourselves - #1229
Move to dompurify 3.4.13, and validate attachment URLs ourselves#1229jeremy wants to merge 3 commits into
Conversation
The floor was ^3.3.0 and the lockfile sat on 3.3.0, which nineteen published advisories reach. 3.4.13 is the first release none of them do. Bumping broke three browser tests, all data: URIs vanishing from a saved attachment, and the cause is one of the advisories rather than a regression. Until 3.3.2, an attribute admitted by a *functional* ADD_ATTR skipped URI validation entirely (GHSA-cjmm-f4jc-qw8r). Lexxy builds ADD_ATTR from the public allowedElements API, so `url` took that path: the data: URLs the tests cover worked because nothing was checking, and `url="javascript:alert(1)"` survived for the same reason. Confirmed against 3.3.0 — it keeps the javascript: URL. An attachment's `url` becomes an <img src> (action_text_attachment_node reads it into this.src). DOMPurify already permits data: on img[src], since img is in its DATA_URI_TAGS, but it can't know a custom element's `url` feeds that same sink, so it applies the plain URI check and drops it. ADD_DATA_URI_TAGS doesn't help — that check only covers src/href/xlink:href. So `url` is marked URI-safe, which hands the decision to a hook, and the hook allows exactly what DOMPurify allows on an img src: its scheme list plus data:. The hook only ever removes an attribute, never force-keeps one, so scoping stays with the ADD_ATTR predicate and a `url` on a tag that never declared it is dropped as before. Net effect is stricter than what shipped: data: URIs still round-trip, and javascript: no longer does. 143 vitest, 630 browser tests, the Action Text round-trip, and both builds. The three leak/attachment flakes were checked against main and predate this. Tests sit next to the Stimulus hook tests in test/javascript/unit, since the url hook is a property of the sanitizer config rather than of the per-editor helper that arrives later in this series.
There was a problem hiding this comment.
Pull request overview
Upgrades DOMPurify to 3.4.13 and adds explicit attachment URL validation.
Changes:
- Bumps the DOMPurify dependency and lockfile.
- Preserves
data:attachment URLs while rejecting executable schemes. - Adds unit coverage for URL filtering.
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 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
package.json |
Updates the DOMPurify dependency floor. |
yarn.lock |
Locks DOMPurify 3.4.13. |
src/config/dom_purify.js |
Adds attachment URL filtering. |
test/javascript/unit/dom_purify_attachment_url.test.js |
Tests allowed and executable URLs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Browser verificationAgainst the integration branch, in Chromium:
The unit tests are mutation-checked: revert the hook and the One gap the browser check turned up — pre-existing, not caused by this PR
The attachment node reads Not exploitable on its own — browsers do not execute a bc3 notebc3 pins bc3 gets 3.4.0. That's above the 3.3.2 fix for GHSA-cjmm-f4jc-qw8r, so behaviour matches — but bc3's resolution has to move as part of the bump, and yarn warns rather than failing, so it is easy to miss. CI: green. The one earlier red leg was a Selenium flake ( |
|
Handoff note for the whole campaign, including QA results and what still needs a human decision: #1234 |
…bute name Review catch, and a real one. ADD_URI_SAFE_ATTR is attribute-name-wide: marking `url` URI-safe takes it out of DOMPurify's URI checking on every tag, not just ours. The hook then applied the attachment's permissive scheme list — the one that exists because an attachment's url becomes an <img src> — to `url` wherever it appeared. Extensions declare arbitrary attributes on arbitrary tags through the public allowedElements API, and home/docs/extensions.md's own worked example is an <iframe>. So an extension declaring `url` inherited the exception: <x-widget url="data:text/html,<script>alert(1)</script>"> survived intact, with the data: URI preserved, on an element Lexxy knows nothing about. Whether that is script execution depends on what the extension's own renderer does with it — an iframe src or a navigation makes it so — and nothing about a third party's element justifies the exception in the first place. The hook now reads currentNode: the attachment element keeps data:, and every other tag gets DOMPurify's standard policy, which is what `url` would have had if we had never touched it. Still removal-only, so scoping stays with the ADD_ATTR predicate. Two corrections to what the comment claimed while here. It said the hook allows "exactly what DOMPurify allows on an img src"; it did not. It dropped `matrix:`, which IS_ALLOWED_URI permits — now restored, so the non-attachment path is that list verbatim. And it accepts `DATA:` where DOMPurify tests for a literal lowercase `data:` at offset zero; that one is deliberate, since browsers resolve schemes case-insensitively and the browser is what decides whether the URL loads. Both stated in the comment rather than left to be rediscovered. Adds the Action Text round trip AGENTS.md asks for on a sanitization change. action_text_load_test loads a data: URL but stops there; this covers editor → save → render → re-edit, which is where a disagreement between Lexxy and Loofah would surface. Mutation-checked: narrow the attachment list and the data: case fails; make isAttachmentTag unconditional and the extension case fails. The executable-scheme half of that test pins a gap rather than tolerating it. The refused url still reaches img[src] in the editor DOM, because the attachment node assigns this.src without going through the sanitizer, and the browser declines to load it. Pre-existing, strictly narrower than before this branch, and not exploitable — but the console assertion is exact, so any other error still fails and whoever closes it finds the reason written down.
The data: exception follows Lexxy.global's attachmentTagName by design — the comment on the `content` bypass spells out why there's no fallback to the literal `action-text-attachment` — but nothing tested the renamed case, and that is bc3's real path: it configures the element as bc-attachment. Two silent failures were unguarded. Hardcode the literal and bc3's avatars (data: urls on bc-attachment) get stripped, while a stray action-text-attachment keeps a data: exception it should no longer have. Both are now covered, and mutation-checked against exactly that hardcoding.
|
Follow-up from an adversarial self-review of my own fix: added coverage for the renamed-attachment-tag case, which is bc3's real path and was the one part of the scoping I'd left unpinned.
Both are now covered and mutation-checked against exactly that hardcoding. While there, one honest note on the fix's scope that the review threads didn't surface: reproducing DOMPurify's |
Consumer action: none required, but note the behaviour change below — an
attachment
urlwith an executable scheme is now dropped.The bump
The floor was
^3.3.0and the lockfile sat on 3.3.0, which nineteen publishedadvisories reach. 3.4.13 is the first release none of them do.
Why it isn't only a bump
Bumping broke three browser tests, all
data:URIs vanishing from a savedattachment — and the cause is one of the advisories rather than a regression.
Until 3.3.2, an attribute admitted by a functional
ADD_ATTRskipped URIvalidation entirely (GHSA-cjmm-f4jc-qw8r).
Lexxy builds
ADD_ATTRfrom the publicallowedElementsAPI, sourltook thatpath: the
data:URLs those tests cover worked because nothing was checking —and
url="javascript:alert(1)"survived for exactly the same reason. Confirmedagainst 3.3.0: it keeps the
javascript:URL.An attachment's
urlbecomes an<img src>(action_text_attachment_nodereadsit into
this.src). DOMPurify already permitsdata:onimg[src], sinceimgis in its
DATA_URI_TAGS, but it cannot know a custom element'surlfeeds thatsame sink, so it applies the plain URI check and drops it.
ADD_DATA_URI_TAGSdoesn't help — that check only covers
src/href/xlink:href.So
urlis marked URI-safe, which hands the decision to a hook, and the hookallows exactly what DOMPurify allows on an
img src: its scheme list plusdata:. The hook only ever removes an attribute, never force-keeps one, soscoping stays with the
ADD_ATTRpredicate — aurlon a tag that neverdeclared it is dropped as before.
Net effect is stricter than what shipped:
data:URIs still round-trip, andjavascript:no longer does.Verification
The url tests are mutation-checked: with the hook removed, the
data:case fails.The
javascript:case passes on 3.4.13 either way — it documents the restoredvalidation rather than the hook, and says so.
Part of a series re-filing #1227 at reviewable scope, after #1227 was reverted
from
mainin 8c64aa4. Merge order: #1228 → this → #images → #instance →#trusted-types → #1226. Nothing here is released.
Note for whoever does the bc3 bump afterwards: bc3 pins
dompurify: 3.4.0in bothdependenciesandresolutions, andresolutionsoverrides this floor — so thatpin has to move too, or bc3 gets 3.4.0 regardless of what lexxy asks for.
Draft: needs human review and a soak period before merging.