Skip to content

fix(packages): escape HTML special chars in serializeAttributes to prevent XSS#1670

Merged
luwes merged 5 commits into
videojs:mainfrom
Jerricho93:fix/1562-escape-serialized-attribute-values
Jun 18, 2026
Merged

fix(packages): escape HTML special chars in serializeAttributes to prevent XSS#1670
luwes merged 5 commits into
videojs:mainfrom
Jerricho93:fix/1562-escape-serialized-attribute-values

Conversation

@Jerricho93

@Jerricho93 Jerricho93 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

serializeAttributes interpolated attribute values directly into HTML strings without encoding. Because the result is assigned to shadowRoot.innerHTML at construction time, and Shadow DOM does not sandbox script execution, any attribute value containing " (e.g. a user-controlled poster, src, or crossorigin) could break out of the attribute and inject event handlers or <script> siblings into the shadow root.

Changes per sink:

  • utils: add private escapeAttributeValue that encodes &, <, >, " (ampersand first to prevent double-encoding); apply in serializeAttributes
  • html/BackgroundVideo: remove the local duplicate serializeAttributes; import the shared util and pick against the existing attribute allowlist
  • html/define/background: remove dead _attrs parameter (was passed to a template that never interpolated it)
  • icons/scripts/build: inline esc() in the generated renderIcon function body to close the codegen sink
  • site/build-ejected-skins: extend the partial local escapeAttributeValue to also cover < and >

Testing

  • New packages/utils/src/dom/tests/attributes.test.ts: 9 cases covering all four chars, boolean branch, ampersand-first ordering regression, and namedNodeMapToObject raw-value preservation
  • New packages/html/src/media/background-video/tests/background-video.test.ts: quote breakout, angle-bracket injection, non-allowlisted attribute filtering, boolean attrs, safe value round-trip
  • Extended packages/core/.../custom-media-element.test.ts: describe('XSS prevention') with 4 cases
  • New apps/e2e/tests/xss-prevention.spec.ts: 3 Playwright tests exercising the construction-time path via container.innerHTML

Validation

pnpm -F @videojs/utils test src/dom/tests/attributes.test.ts
pnpm -F @videojs/core test src/dom/media/custom-media-element
pnpm -F @videojs/html test src/media/background-video
pnpm test:e2e:vite
pnpm typecheck
pnpm lint

Known issue (follow-up)

CustomMediaElement has a pre-existing attribute routing inconsistency: getAttrsFromProps uses .toLowerCase() to build observedAttributes (e.g. crossorigin, controlslist, disableremoteplayback), while #define uses kebabCase to key mediaHostAttrToProp (e.g. cross-origin, controls-list, disable-remote-playback). Because these never match, multi-word attributes are never added to the disallowed set and always flow through serializeAttributes rather than attributeChangedCallback. This means post-construction setAttribute calls for those attributes do not sync to the media host setter. This fix makes serializeAttributes safe, closing the injection vector — but the routing inconsistency itself should be tracked and corrected separately.

Closes #1562


Note

High Risk
Security fix on HTML string assembly used at custom-element construction; behavior change for attribute values containing &<>" but intended to block script/event injection in shadow templates.

Overview
Closes an XSS path where unescaped attribute values were interpolated into HTML assigned to shadowRoot.innerHTML (and similar codegen sinks), allowing quote/angle-bracket breakout into event handlers or extra nodes inside shadow roots.

serializeAttributes in @videojs/utils/dom now runs values through escapeHtml (&, <, >, ", with ampersand first). BackgroundVideo drops its local serializer and uses the shared helper with an existing video-attribute allowlist via pick. background-video-skin stops passing unused attrs into a static template.

The same escaping pattern is applied to generated renderIcon output (icons build) and ejected skin HTML (build-ejected-skins), including </> on icon names and attrs.

Coverage adds unit tests for serializeAttributes / escapeHtml, XSS cases on CustomMediaElement and BackgroundVideo, and Playwright e2e on hls-video construction via innerHTML.

Reviewed by Cursor Bugbot for commit 101bd4a. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Jun 9, 2026

Copy link
Copy Markdown

@Jerricho93 is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

@netlify

netlify Bot commented Jun 9, 2026

Copy link
Copy Markdown

👷 Deploy request for vjs10-site pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 101bd4a

@Jerricho93 Jerricho93 marked this pull request as ready for review June 9, 2026 18:52
Comment thread packages/utils/src/dom/attributes.ts Outdated
Comment thread packages/utils/src/dom/attributes.ts Outdated
Comment thread packages/html/src/define/background/skin.ts

@luwes luwes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like some small changes but looks good overall!

Jerricho93 and others added 3 commits June 10, 2026 16:28
…event XSS

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n in e2e XSS tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Jerricho93 Jerricho93 force-pushed the fix/1562-escape-serialized-attribute-values branch from 530412d to 449c141 Compare June 10, 2026 19:29

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 449c141. Configure here.

Comment thread packages/html/src/media/background-video/index.ts Outdated
…ideo

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Jerricho93 Jerricho93 requested a review from luwes June 10, 2026 20:02
@vercel

vercel Bot commented Jun 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
v10-sandbox Ready Ready Preview, Comment Jun 10, 2026 8:03pm

Request Review

@luwes luwes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luwes luwes merged commit accf4bf into videojs:main Jun 18, 2026
19 of 21 checks passed
@luwes luwes mentioned this pull request Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Bug: XSS via Unescaped Attribute Values in CustomMediaElement Shadow Root

2 participants