Fix XSS via unsanitized action-text-attachment content attribute by jorgemanrubia · Pull Request #903 · basecamp/lexxy

jorgemanrubia · 2026-03-20T17:27:19Z

Summary

Security fix: CustomActionTextAttachmentNode.createDOM() inserted innerHtml extracted from the content attribute directly into the DOM via insertAdjacentHTML without sanitization. Because DOMPurify treats content as a harmless string attribute during paste, malicious HTML (e.g. <img onerror=alert(...)>, <script>, <meta http-equiv=refresh>) survived the initial paste sanitization and was injected into the live DOM when the node rendered.
Sanitize innerHtml with DOMPurify.sanitize() before inserting it into the DOM. Uses DOMPurify's defaults (not the editor's restrictive tag allowlist) so legitimate attachment content (mentions with <span>, cards, etc.) is preserved while XSS vectors are stripped.
Adds Playwright regression tests covering XSS via onerror, <script> injection, <meta http-equiv=refresh> injection, and verifying legitimate mention content still renders correctly.

Copilot

Pull request overview

This PR addresses an XSS vector where CustomActionTextAttachmentNode renders HTML from the content attribute into the live DOM without sanitization, and adds Playwright regression coverage around paste-driven injection attempts.

Changes:

Sanitize custom attachment innerHtml before inserting it into the DOM.
Add a dedicated Playwright test suite covering multiple XSS payload variants and a “legitimate mention” case.
Introduce a new sanitizeAttachmentContent() helper using DOMPurify without the editor’s restrictive allowlist.

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 3 changed files in this pull request and generated 5 comments.

File	Description
`src/nodes/custom_action_text_attachment_node.js`	Sanitizes attachment `innerHtml` before inserting into the DOM.
`src/helpers/sanitization_helper.js`	Adds `sanitizeAttachmentContent()` helper (DOMPurify default allowlist path).
`test/browser/tests/paste/xss_sanitization.test.js`	Adds Playwright regression tests for XSS attempts via `content` attribute.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-20T17:32:29Z

+    await editor.paste("", { html: scriptPayload })
+    await editor.flush()
+    await page.waitForTimeout(1000)
+
+    expect(dialogTriggered).toBe(false)
+


These fixed waitForTimeout(1000) delays will add multiple seconds to the Playwright suite and can be a source of flakiness. Prefer waiting on a specific condition (e.g., page.waitForEvent('dialog', { timeout: ... }) and asserting it times out, or using locator-based expectations) with a smaller timeout budget.

Copilot · 2026-03-20T17:32:30Z

+    // The XSS alert should not have fired
+    expect(dialogTriggered).toBe(false)
+
+    // The malicious img with onerror should not be present in the DOM
+    await assertEditorContent(editor, async (content) => {
+      await expect(content.locator("img[onerror]")).toHaveCount(0)
+    })
+  })


The regression tests validate the live DOM is safe, but they don’t currently assert that the editor’s serialized output (e.g. editor.value() / exported HTML) has had the XSS payload removed from the attachment content attribute. Adding an assertion that the exported HTML/JSON no longer contains onerror, <script>, or <meta http-equiv=refresh> would help prevent a stored-payload regression.

Copilot · 2026-03-20T17:32:30Z

  createDOM() {
    const figure = createElement(this.tagName, { "content-type": this.contentType, "data-lexxy-decorator": true })

-    figure.insertAdjacentHTML("beforeend", this.innerHtml)
+    figure.insertAdjacentHTML("beforeend", sanitizeAttachmentContent(this.innerHtml))



Sanitizing only at render-time means the node still retains the original (potentially malicious) innerHtml, and exportDOM() / exportJSON() will persist it back into the content attribute unchanged. That leaves a stored payload in saved HTML/JSON that could be re-used by other consumers (or by older versions) and defeats the goal of removing the XSS payload from the document. Consider sanitizing innerHtml when constructing/importing the node (e.g., in importDOM conversion and/or the constructor) so both rendering and exports use the sanitized value (or sanitize on export).

Copilot · 2026-03-20T17:32:30Z

+// Uses DOMPurify defaults to strip XSS vectors (scripts, event handlers)
+// while preserving the richer tag set that server-rendered attachment
+// content legitimately uses (e.g. <span>, <div>, <img>).


sanitizeAttachmentContent() is documented as using “DOMPurify defaults”, but this module imports buildConfig from src/config/dom_purify.js, which globally registers DOMPurify hooks (style filtering, class stripping on strong/em). Those hooks will still run here, so the behavior is not pure defaults. Either adjust the comment to reflect that only the tag/attr allowlist is default, or create a separate DOMPurify instance for attachment content if the intent is truly unmodified defaults.

Suggested change

// Uses DOMPurify defaults to strip XSS vectors (scripts, event handlers)

// while preserving the richer tag set that server-rendered attachment

// content legitimately uses (e.g. <span>, <div>, <img>).

// Uses the shared DOMPurify instance configured in ../config/dom_purify

// (including its global hooks like style filtering) but relies on the

// default tag/attribute allowlist (no additional per-call config) to

// strip XSS vectors while preserving the richer tag set that server-

// rendered attachment content legitimately uses (e.g. <span>, <div>, <img>).

Copilot · 2026-03-20T17:32:30Z

+    await editor.paste("", { html: xssPayload })
+    await editor.flush()
+    await page.waitForTimeout(1000)
+
+    // The XSS alert should not have fired
+    expect(dialogTriggered).toBe(false)


These fixed waitForTimeout(1000) delays will add multiple seconds to the Playwright suite and can be a source of flakiness. Prefer waiting on a specific condition (e.g., page.waitForEvent('dialog', { timeout: ... }) and asserting it times out, or using locator-based expectations) with a smaller timeout budget.

CustomActionTextAttachmentNode.createDOM() inserted the innerHtml extracted from the content attribute directly into the DOM via insertAdjacentHTML without sanitization. Because DOMPurify treats content as a harmless string attribute during paste, malicious HTML (e.g. <img onerror=alert(...)>, <script>, <meta http-equiv=refresh>) survived the initial paste sanitization and was injected into the live DOM when the node rendered. Sanitize innerHtml with DOMPurify.sanitize() (using its defaults to strip XSS vectors while preserving the richer tag set that server-rendered attachment content legitimately uses) before inserting it into the DOM in createDOM().

Copilot AI review requested due to automatic review settings March 20, 2026 17:27

jorgemanrubia self-assigned this Mar 20, 2026

Copilot started reviewing on behalf of jorgemanrubia March 20, 2026 17:27 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

jorgemanrubia force-pushed the jorge/fix-xss-attachment-content-sanitization branch from 8d9664a to a3985b3 Compare April 7, 2026 11:52

jorgemanrubia merged commit 2ef7d8b into main Apr 7, 2026
11 checks passed

jorgemanrubia deleted the jorge/fix-xss-attachment-content-sanitization branch April 7, 2026 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix XSS via unsanitized action-text-attachment content attribute#903

Fix XSS via unsanitized action-text-attachment content attribute#903
jorgemanrubia merged 1 commit intomainfrom
jorge/fix-xss-attachment-content-sanitization

jorgemanrubia commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-// Uses DOMPurify defaults to strip XSS vectors (scripts, event handlers)
-// while preserving the richer tag set that server-rendered attachment
-// content legitimately uses (e.g. <span>, <div>, <img>).
+// Uses the shared DOMPurify instance configured in ../config/dom_purify
+// (including its global hooks like style filtering) but relies on the
+// default tag/attribute allowlist (no additional per-call config) to
+// strip XSS vectors while preserving the richer tag set that server-
+// rendered attachment content legitimately uses (e.g. <span>, <div>, <img>).

Conversation

jorgemanrubia commented Mar 20, 2026

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants