Strip Stimulus behavior attributes in DOMPurify sanitization - #1225
Open
jeremy wants to merge 4 commits into
Open
Strip Stimulus behavior attributes in DOMPurify sanitization#1225jeremy wants to merge 4 commits into
jeremy wants to merge 4 commits into
Conversation
Stored/pasted attachment content can carry data-controller and data-action, which let arbitrary HTML wire up live Stimulus controllers in the viewer's session once hydrated. The sanitizer config allowed all data-* attributes through (the default), so these two never got stripped even though the server-side whitelist already refuses them on write. Add both to FORBID_ATTR, which DOMPurify 3.x honors ahead of ALLOWED_ATTR/ADD_ATTR/ALLOW_DATA_ATTR regardless of tag or content type. This is deliberately narrow: a blanket ALLOW_DATA_ATTR:false would also strip data-language, data-highlight-language, and data-trix-*, which stored content legitimately depends on. Both the hydrate path (CustomActionTextAttachmentNode#createDOM -> insertAdjacentHTML(sanitize(...))) and the export path (sanitize($generateHtmlFromNodes(...)) in editor.js) share the one DOMPurify config built by buildConfig(), so this closes both routes at once. Covers leg A (stored-XSS delivery) of HackerOne #3925075.
Contributor
There was a problem hiding this comment.
Pull request overview
Restricts sanitized editor HTML from activating Stimulus behavior.
Changes:
- Forbids
data-controlleranddata-action. - Adds hydration, round-trip, compatibility, and mention tests.
- Review found an
ADD_ATTRextension bypass requiring correction.
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 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/config/dom_purify.js |
Adds forbidden Stimulus attributes. |
test/browser/tests/paste/xss_sanitization.test.js |
Adds sanitization regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DOMPurify 3.x evaluates the functional ADD_ATTR (built from the public allowedElements extension API) ahead of FORBID_ATTR, so an extension that declared data-controller/data-action on a tag would reinstate it. Add an uponSanitizeAttribute hook that drops these two attributes unconditionally, making the class-level prohibition config-independent. FORBID_ATTR stays as the declarative fast path. Adds a vitest unit test covering the extension-declared case and the data-language no-over-strip guard.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/config/dom_purify.js:74
- This comment contradicts the verified behavior documented just above: a functional
ADD_ATTRcan overrideFORBID_ATTR, which is why the hook is required. Reword this so future security changes do not incorrectly rely onFORBID_ATTRalone.
// wire up arbitrary controllers/actions in the viewer's session. FORBID_ATTR wins over
// ALLOWED_ATTR/ADD_ATTR/ALLOW_DATA_ATTR in DOMPurify, so this holds even though other
// data-* attributes (data-language, data-trix-*, etc.) are otherwise allowed through.
test/browser/tests/paste/xss_sanitization.test.js:156
- The second-hydration check can pass vacuously if export drops or sanitizes the attachment's
content=payload, because the only precondition asserted here is the SGID. Assert that both malicious attribute names remain inserializedbefore feeding it back; then the later zero-count assertions actually demonstrate rehydration sanitization rather than payload disappearance.
const serialized = await editor.value()
expect(serialized).toContain("sgid=\"test-sgid-alice\"")
src/config/dom_purify.js:75
- This changes a sanitization rule, but the added Vitest/Playwright coverage never crosses Rails save/render/re-edit.
AGENTS.md:53-55explicitly requires a Capybara system test for sanitization changes, andtest/system/mention_round_trip_test.rbprovides the attachment round-trip pattern. Add a system test that saves the payload, verifies the rendered DOM has no Stimulus attributes, then reopens the editor and verifies hydration remains clean.
FORBID_ATTR: [ "data-controller", "data-action" ]
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c9cb1dd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d-trip Confirm the class-level prohibition holds across a real edit -> save -> render -> re-edit cycle in the dummy app, not only the client setValue path: an attachment's content= that smuggles data-controller/data-action hydrates into the editor with those attributes stripped, while a mention and a data-language code block survive the whole round-trip. The payload is delivered with setValue because Action Text re-renders a resolvable mention from its partial on save and a content-carrying non-attachable attachment fails to render, so neither seeds the vector faithfully. The first hydration is the guarded leg and goes red if the sanitizer hook / FORBID_ATTR is reverted.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/config/dom_purify.js:74
- This explanation contradicts both the hook above and the new regression test: a functional
ADD_ATTRcan bypassFORBID_ATTRin the installed DOMPurify version. Please document thatFORBID_ATTRcovers ordinary allowlisting while the hook enforces the extension-backed case; otherwise a future maintainer could remove the hook based on this statement.
// wire up arbitrary controllers/actions in the viewer's session. FORBID_ATTR wins over
// ALLOWED_ATTR/ADD_ATTR/ALLOW_DATA_ATTR in DOMPurify, so this holds even though other
// data-* attributes (data-language, data-trix-*, etc.) are otherwise allowed through.
test/system/stimulus_sanitization_test.rb:27
- The Rails test guidelines require fixtures for test setup and specifically recommend
posts(:empty)as the starting point (STYLE.md:323). Reusing that fixture avoids introducing an inline record when this test replaces the editor value immediately anyway.
post = Post.create!(title: "Stimulus sanitization round trip", body: "<p>start</p>")
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a8c5a522
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow the repo's round-trip testing convention (AGENTS.md) — begin from the shared posts(:empty) fixture rather than a standalone Post. setValue replaces the editor content, so the starting shell is immaterial to what the test proves.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/config/dom_purify.js:74
- This comment contradicts both the hook rationale above and the new regression test: a functional
ADD_ATTRcan bypassFORBID_ATTR. Documenting the opposite obscures why the hook is security-critical and could lead to its removal. Clarify thatFORBID_ATTRcovers the normal allowlists while the hook handles extension-provided attributes.
// Stimulus behavior attributes must never survive sanitization: they let stored content
// wire up arbitrary controllers/actions in the viewer's session. FORBID_ATTR wins over
// ALLOWED_ATTR/ADD_ATTR/ALLOW_DATA_ATTR in DOMPurify, so this holds even though other
// data-* attributes (data-language, data-trix-*, etc.) are otherwise allowed through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
data-controlleranddata-actiontoFORBID_ATTRin the DOMPurify config built bybuildConfig()(src/config/dom_purify.js).Why
Stored/pasted attachment content (e.g. an
action-text-attachment/bc-attachment'scontent=attribute) can carry arbitrary HTML. That HTML is sanitized with DOMPurify before being written into the live DOM, but the sanitizer config never restricteddata-*attributes (ALLOW_DATA_ATTRdefaults totrue), sodata-controller/data-actionpassed straight through. Once hydrated into the document, those attributes let stored content wire up arbitrary Stimulus controllers/actions in the viewer's session — a stored-XSS-adjacent privilege escalation via the app's own controller registry. The server-side whitelist already refuses these two attributes on write, so this was a client/server sanitizer-contract mismatch: the client needed to enforce the same rule on read/hydrate.FORBID_ATTRin DOMPurify 3.x wins overALLOWED_ATTR/ADD_ATTR/ALLOW_DATA_ATTR, so this closes the gap without a blanketALLOW_DATA_ATTR: false— which would also stripdata-language,data-highlight-language, anddata-trix-*, all of which stored content legitimately depends on (code blocks, Trix-authored content). It's also independent of the existinguponSanitizeAttribute(style allowlist) anduponSanitizeElement(strong/em class strip) hooks, so it doesn't interact with either.Both delivery routes share this one config, so one change closes both:
CustomActionTextAttachmentNode#createDOM→insertAdjacentHTML(sanitize(this.innerHtml))(src/nodes/custom_action_text_attachment_node.js)sanitize($generateHtmlFromNodes(this.editor, null))(src/elements/editor.js)Closes leg A (stored-XSS delivery) of H1 #3925075 at the class level (surgical Stimulus-attr strip; preserves data-language/data-trix-*).
Verification
test/browser/tests/paste/xss_sanitization.test.js:bc-attachmentcontent carryingdata-controller/data-actionhydrates with zero matching attributes in the live DOM, and stays clean on a second hydration of the round-tripped stored value.data-languagesurvives sanitization unstripped (no over-strip).npm run test:browser -- xss_sanitizationgreen on bothchromiumandwebkit.npm run test:browser(chromium + firefox + webkit): 1893 passed, 5 skipped, 2 flaky (pre-existing, unrelated to this change — attachment-delete-before-load timing and a table-toolbar focus test).