Bump DOMPurify to ^3.4.0 to restore URI validation on attachment url - #1238
Bump DOMPurify to ^3.4.0 to restore URI validation on attachment url#1238rosa wants to merge 2 commits into
Conversation
DOMPurify 3.3.0 is affected by GHSA-cjmm-f4jc-qw8r: when ADD_ATTR is passed as a predicate function — which buildConfig does for per-tag attribute allowlisting — _isValidAttribute short-circuits before the IS_ALLOWED_URI check, so a javascript:/data: value in an allowed URI attribute survives sanitize(). The action-text-attachment `url` attribute is read into an <img src>, so a stored javascript: url there is a real sanitizer bypass. It is inert on our own render paths (img src does not execute javascript:), but any consumer that routes the attachment url to a navigable sink would be exposed. Bumping to the patched line (3.4.x) makes predicate-allowed attributes run URI validation again: javascript:/data: are stripped from `url`, while legitimate absolute and relative blob URLs pass unchanged. `caption` and `filename` stay in ADD_URI_SAFE_ATTR (they are display strings, not URL sinks) and are unaffected; the `content` attribute (serialized HTML, SAFE_FOR_XML: false) is preserved. Adds a unit test covering all of these. Reported via HackerOne by @PetitPois. Advisory: GHSA-cjmm-f4jc-qw8r. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates DOMPurify to restore URI validation for attachment URLs and adds sanitizer regression coverage.
Changes:
- Upgrades DOMPurify to 3.4.13.
- Tests unsafe and legitimate attachment URLs.
- Verifies serialized attachment content remains preserved.
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 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
package.json |
Raises the DOMPurify dependency range. |
yarn.lock |
Locks DOMPurify 3.4.13. |
test/javascript/unit/config/sanitization.test.js |
Adds attachment sanitization regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Per the repo's Action Text Persistence requirement (AGENTS.md), a sanitization rule change needs a Capybara system test covering the full editor -> save -> render -> re-edit round-trip, exercising DOMPurify (client) together with Loofah (server) and the rendered view. Injects a safe and a javascript: attachment url, saves, and asserts the dangerous scheme is absent from both the rendered show page and the re-opened editor while the legitimate url survives. Verified this fails on DOMPurify 3.3.0 (the javascript: url reaches the rendered page) and passes on 3.4.13. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/system/attachment_url_sanitization_test.rb:20
- Using
example.commakes this system test perform an external DNS/TLS/image request, and the expected 404 then requires disabling all console assertions. That can delay or flake CI while also hiding unrelated browser errors. Use the dummy app's local/icon.png; with the patched sanitizer the unsafe URL does not reach an image, so the blanket console allowance is unnecessary.
SAFE_URL = "https://example.com/safe.png".freeze
test/javascript/unit/config/sanitization.test.js:52
- This only proves that a
contentattribute still exists; it would pass if DOMPurify emptied or changed the serialized HTML. Since this case is intended to guard preservation of the attribute value, parse the sanitized element and compare the decoded value exactly.
expect(output).toMatch(/content=/)
What
Bumps DOMPurify
^3.3.0→^3.4.0(resolves to 3.4.13) and adds a sanitizer regression test.Why
DOMPurify 3.3.0 is affected by GHSA-cjmm-f4jc-qw8r ("ADD_ATTR predicate skips URI validation", patched in 3.3.2 / 3.4.0 / 3.4.2).
buildConfigpassesADD_ATTRas a predicate function for per-tag attribute allowlisting:In 3.3.0,
_isValidAttributeshort-circuits at the predicate branch and returnstruebefore theIS_ALLOWED_URIcheck, so ajavascript:/data:text/htmlvalue in an allowlisted URI attribute survivessanitize(). In practice this means a<action-text-attachment url="javascript:…">keeps its dangerousurl, which is read into an<img src>(action_text_attachment_node.js).On our own render paths this is inert (a
javascript:URL in<img src>doesn't execute), but it's a genuine sanitizer bypass and would be exposed for any consumer that routes the attachmenturlto a navigable sink. Fixing the flagged dependency is the right resolution rather than papering over the one sink.The fix
Bumping to the patched line makes predicate-allowed attributes run URI validation again:
javascript:/data:text/htmlare stripped fromurl.https://…) and relative (/rails/active_storage/…) blob URLs pass unchanged.caption/filenameremain inADD_URI_SAFE_ATTR— they're display strings, not URL sinks, so they're intentionally not URI-validated and are unaffected.contentattribute (serialized HTML,SAFE_FOR_XML: false) is preserved.Verification
test/javascript/unit/config/sanitization.test.jscovers all five cases above.yarn vitest runsuite green (132 tests).yarn buildcompiles clean with 3.4.13.The browser suite (
yarn test:browser) isn't run here — CI should exercise it, since the bump changes URI-validation behavior for predicate-allowlisted attributes on custom elements (attachment paste/import/export paths worth a look).Credit
Reported via HackerOne by @PetitPois. Advisory: GHSA-cjmm-f4jc-qw8r.
🤖 Generated with Claude Code