Escape or avoid HTML interpolation for all external and API-sourced text #1

New issue

Closed

opened 2026-08-23 22:37:27 +02:00 by kristofferopsahl · 1 comment

kristofferopsahl commented

2026-08-23 22:37:27 +02:00

Collaborator

Finding

The frontend renders extensive API/external data through template strings assigned to innerHTML, but only a few call sites use esc(). Player names, team names, error details, override notes, fixture opponent names, accuracy details, and bookmaker-derived strings can therefore become markup.

Representative evidence:

frontend/app.js:877-883, 956-957, 1266-1268, 1358-1367, 3665, 3951, 4090, 4117, 4216, 4278-4279, 4346
frontend/ticker.js:90-96, 124-125, 140
frontend/app.js:49-56 defines a correct HTML escaper, but rg finds only three calls while dozens of external strings are interpolated into HTML.
tests/web/test_escaping.py tests the helper itself, not the sinks that need it.

This is a DOM XSS risk whenever upstream data, persisted notes, or server error text contains markup. The official FPL feed and bookmaker feeds are external trust boundaries even if their current values are benign.

Suggested direction

Prefer textContent/DOM construction for text-only values.
Where HTML templates remain, escape every non-constant value at the point of interpolation, including attribute contexts.
Add sink-level tests using hostile player/team/note/error strings and assert no executable node/event handler is created.
Consider a small rendering primitive that makes raw text the default and explicit trusted markup exceptional.

Acceptance criteria

External/API-controlled strings cannot create elements, attributes, or event handlers in any rendered surface.
Tests cover representative table, modal, error, override, fixture, and ticker sinks.
Existing UI output remains functionally unchanged for ordinary data.

## Finding The frontend renders extensive API/external data through template strings assigned to `innerHTML`, but only a few call sites use `esc()`. Player names, team names, error details, override notes, fixture opponent names, accuracy details, and bookmaker-derived strings can therefore become markup. Representative evidence: - `frontend/app.js:877-883`, `956-957`, `1266-1268`, `1358-1367`, `3665`, `3951`, `4090`, `4117`, `4216`, `4278-4279`, `4346` - `frontend/ticker.js:90-96`, `124-125`, `140` - `frontend/app.js:49-56` defines a correct HTML escaper, but `rg` finds only three calls while dozens of external strings are interpolated into HTML. - `tests/web/test_escaping.py` tests the helper itself, not the sinks that need it. This is a DOM XSS risk whenever upstream data, persisted notes, or server error text contains markup. The official FPL feed and bookmaker feeds are external trust boundaries even if their current values are benign. ## Suggested direction - Prefer `textContent`/DOM construction for text-only values. - Where HTML templates remain, escape every non-constant value at the point of interpolation, including attribute contexts. - Add sink-level tests using hostile player/team/note/error strings and assert no executable node/event handler is created. - Consider a small rendering primitive that makes raw text the default and explicit trusted markup exceptional. ## Acceptance criteria - External/API-controlled strings cannot create elements, attributes, or event handlers in any rendered surface. - Tests cover representative table, modal, error, override, fixture, and ticker sinks. - Existing UI output remains functionally unchanged for ordinary data.

kristofferopsahl added the

security

priority: high

labels

2026-08-23 22:37:27 +02:00