Skip to content

fix: sanitize notification HTML, restrict stream link scheme, fix replay/demo leaks (#542, #541, #550)#494

Open
Flegma wants to merge 1 commit into
mainfrom
audit/web-security-correctness
Open

fix: sanitize notification HTML, restrict stream link scheme, fix replay/demo leaks (#542, #541, #550)#494
Flegma wants to merge 1 commit into
mainfrom
audit/web-security-correctness

Conversation

@Flegma

@Flegma Flegma commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Web security + leak fixes from the 2026-07 audit. Closes #542, #541, #550.

Changes

NotificationMessage HTML sanitization (#542)
NotificationMessage.vue set container.innerHTML = props.html and rebuilt every node via h(tag, attrs, ...), copying all attributes verbatim with only anchor hrefs filtered. It was the only HTML sink in the app not passing through DOMPurify (the news components both sanitize). Now props.html is DOMPurify.sanitized before parsing, and the SSR fallback renders as escaped text instead of raw innerHTML. Strips scripts, event-handler attributes, and javascript:/data: URIs so a notification embedding unescaped user text can't reconstruct an XSS sink.

Stream link scheme restriction (#541)
z.string().url() accepts javascript:/data: URIs. A stored stream link flowed into an iframe src (StreamEmbed.mountGenericIframe) and window.open (MatchLiveStreams.openStream) on public match pages.

  • parseStreamLink now rejects non-http(s) (and malformed) links, so they never become an embed.
  • mountGenericIframe scheme-guards defensively and adds a sandbox attribute to the arbitrary third-party embed (keeps the player working, blocks top-window navigation).
  • openStream scheme-guards window.open and passes noopener,noreferrer.
  • The add/edit Zod schema now .refine()s the link to http(s).

Resource leaks on unmount (#550)

  • Replay3DLite.vue: the pointerup/pointermove listeners were added to window as anonymous functions, so cleanup() could not remove them and their closures pinned the entire Three.js scene (camera/controls/renderer) on every remount. Named them and added removeEventListener in cleanup.
  • useDemoPlayback.ts: the shared 1Hz poll timer, the demo-session:state socket handler, and the document visibilitychange listener were never torn down on unmount (dev attach mode never routes through stop()). Added ref-counted onScopeDispose cleanup that stops them when the last consumer unmounts, preserving the intentional shared-poll design.

Testing

  • yarn build (nuxt build --standalone): green.

Not included (deliberately deferred)

#535 (MatchOptions toggles double-fire on Space): on closer analysis for the fix, both the row @click and the switch's @update:model-value pass the same target value to handleChange (a set, not a relative toggle), so the double-fire is likely idempotent rather than a net no-op. The reported "net no-op" outcome needs confirming in a browser before changing the shared toggle-row pattern (used across MatchOptions and TournamentStageForm). Noted on #535; not fixed here to avoid a speculative UX change.

…replay + demo leaks

- NotificationMessage: sanitize props.html with DOMPurify before parsing and
  rebuilding nodes, and render the SSR fallback as text instead of raw
  innerHTML. This was the only HTML sink in the app skipping DOMPurify, so a
  notification message embedding unescaped user text (team/player name) could
  reconstruct a javascript:/iframe/onerror XSS sink (#542).
- StreamEmbed / MatchLiveStreams: reject non-http(s) stream links (z.url()
  accepts javascript:/data:). parseStreamLink and mountGenericIframe now
  scheme-guard before building the iframe (plus a sandbox attr on the generic
  embed), and openStream scheme-guards window.open and adds noopener (#541).
- Replay3DLite: the pointerup/pointermove window listeners were anonymous, so
  cleanup could not remove them and their closures pinned the whole Three.js
  scene per remount. Named and removed on unmount (#550).
- useDemoPlayback: the shared 1Hz poll timer, socket handler, and document
  visibilitychange listener were never torn down on unmount (notably dev
  attach mode, which skips stop()). Added ref-counted onScopeDispose cleanup
  that fires when the last consumer unmounts (#550).
@Flegma

Flegma commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Independent code review run on the diff: clean, no changes needed. The reviewer verified the useDemoPlayback ref-count is balanced across all call sites, the module-scoped visibilityHandler is correct, the DOMPurify config preserves the attributes the node rebuild needs, and (notably) the app runs ssr: false so the NotificationMessage SSR fallback never executes in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant