99 lines
5.2 KiB
Markdown
99 lines
5.2 KiB
Markdown
# XSS Sweep: Escape Server-Controlled Data at `innerHTML` Boundaries
|
|
|
|
Priority: **Critical** · Effort: **Low** · Risk: **Low**
|
|
|
|
## Problem
|
|
|
|
The client injects server-controlled strings into the DOM via `innerHTML` **without escaping** in several high-traffic render paths. Because the server broadcasts channel names, listener usernames, toast messages, and track titles over WebSocket to *every* connected client, a single malicious payload is a **stored/reflected XSS propagated peer-to-peer**.
|
|
|
|
There is also **no Content-Security-Policy** header set on the document or by the server, so once injected, script runs with full page privilege (same-origin as the session cookie).
|
|
|
|
## Affected Locations
|
|
|
|
| File:Line | Sink | Source |
|
|
|-----------|------|--------|
|
|
| `public/channelSync.js:174-183` | channel list `<span class="channel-name">${ch.name}</span>` and `<input value="${ch.name.replace(/"/g,'"')}">` | channel name (server) |
|
|
| `public/channelSync.js:161-163` | `listenersHtml` (listener usernames) | `ch.listeners` (server) |
|
|
| `public/utils.js:113` | toast history `... ${item.message}` | WS `toast` message (server) |
|
|
| `public/utils.js:149,159` | track title marquee | `track.title` (file metadata / yt-dlp) |
|
|
| `public/queue.js:342` | now-playing bar `${title}` | `track.title` (server) |
|
|
| `public/upload.js:307` | slow-queue list `${group.name}` | playlist name (server) |
|
|
| `public/upload.js:330` | slow-queue list `${item.title}` | `/api/fetch` response (server) |
|
|
|
|
Note: `trackComponent.js:59` and `playlists.js:44,57,58` **do** escape correctly today. The codebase is internally inconsistent — those are the model to follow.
|
|
|
|
## Root Cause
|
|
|
|
- No single shared escaping utility. `escapeHtml` is defined **twice** (`public/playlists.js:481-486` and `public/trackComponent.js:74-79`) as local copies.
|
|
- No lint rule or review guard preventing raw `${serverData}` inside template literals feeding `innerHTML`.
|
|
- No CSP as defense-in-depth.
|
|
|
|
## Implementation Plan
|
|
|
|
### Step 1 — Create one shared `escapeHtml` in `public/utils.js`
|
|
|
|
Move/deduplicate the existing helper into `utils.js` and expose it on `M`:
|
|
|
|
```js
|
|
M.escapeHtml = function (str) {
|
|
if (str == null) return "";
|
|
return String(str)
|
|
.replace(/&/g, "&")
|
|
.replace(/</g, "<")
|
|
.replace(/>/g, ">")
|
|
.replace(/"/g, """)
|
|
.replace(/'/g, "'");
|
|
};
|
|
```
|
|
|
|
Delete the two local copies in `playlists.js:481` and `trackComponent.js:74`; replace callers with `M.escapeHtml(...)`.
|
|
|
|
### Step 2 — Escape every server-sourced value at each sink above
|
|
|
|
For each location, wrap the interpolated value in `M.escapeHtml(...)`. For attribute contexts (e.g. `value="${...}"`), escaping with the helper above is sufficient since it includes `"`.
|
|
|
|
Worked example for `channelSync.js:174-183`:
|
|
|
|
```js
|
|
div.innerHTML = `
|
|
<div class="channel-header">
|
|
<span class="channel-name">${M.escapeHtml(ch.name)}</span>
|
|
<input ... value="${M.escapeHtml(ch.name)}" ...>
|
|
...
|
|
<div class="channel-listeners">${listenersHtml}</div>
|
|
</div>
|
|
`;
|
|
```
|
|
|
|
And `listenersHtml` itself (built at `channelSync.js:161-163`) must escape each username before joining.
|
|
|
|
### Step 3 — Add a defense-in-depth CSP header
|
|
|
|
In the static file route handler (`routes/static.ts`) — or centrally where index.html is served — add:
|
|
|
|
```
|
|
Content-Security-Policy: default-src 'self'; script-src 'self'; connect-src 'self' ws: wss:; media-src 'self' blob:; img-src 'self' data: blob:; style-src 'self' 'unsafe-inline'
|
|
```
|
|
|
|
(`'unsafe-inline'` for styles only — needed until stylesheets are consolidated. No `'unsafe-inline'` for scripts.)
|
|
|
|
Confirm this does not break the blob-URL audio playback (`media-src 'self' blob:`) or WebSocket (`connect-src ... ws: wss:`).
|
|
|
|
### Step 4 — (Optional, recommended) Centralize user-visible rendering
|
|
|
|
Longer-term, every track row / title render should go through `trackComponent.js`'s pure renderer which already escapes. Route now-playing-bar and marquee titles through the same path so escaping can't be forgotten again.
|
|
|
|
## Validation
|
|
|
|
- **Manual payload test**: create a channel named `<img src=x onerror="alert(document.cookie)">`, connect a second client, confirm no alert fires and the name renders literally.
|
|
- **Username payload**: set a username (or guest) containing `<script>` and verify the channel-list `listenersHtml` shows it escaped.
|
|
- **Toast payload**: trigger a server toast containing markup (e.g. via yt-dlp add with a crafted title) and confirm toast history (`utils.js:113`) shows literal text.
|
|
- **Track-title payload**: upload an audio file whose metadata title is `<svg/onload=alert(1)>`; confirm queue and now-playing bar render it literally.
|
|
- **CSP**: open DevTools → Network → confirm the CSP header is present on the document response and the console shows no CSP violations during normal use (play, cache, switch channel).
|
|
|
|
## Risk / Rollback
|
|
|
|
- Escaping is additive and behavior-preserving for well-formed data. Only risk is over-escaping if a value is already HTML-safe by construction — review each call site to confirm the raw string is plain text (it is, in all listed cases).
|
|
- A too-strict CSP could break blob audio or an inline event handler. If so, loosen the specific directive per the violation report rather than reverting.
|
|
- Rollback = revert the commit; no schema or data migration involved.
|