fix(ui): close stored-XSS class in admin UI static assets

[jaidhyani]

Root cause

The admin UI is vanilla JS served as static HTML (no bundler, no framework — Alpine.js is vendored for nav only). Several render paths built HTML by string interpolation and relied on hand-rolled escapeHtml helpers shaped like:

function escapeHtml(str) { const div = document.createElement('div'); div.textContent = str; return div.innerHTML; }

That escapes <, >, & but not ' or ". So any attacker-influenced value interpolated into an inline onclick="...('${escapeHtml(x)}')" JS string, or into a quoted HTML attribute, can break out and execute. The interpolated values are request/model-derived: session_id, call_id, transaction_id, tool_call_id, model/endpoint names, provider names.

This is a recurring class, not a one-off — #780 and #752 each tripped the same pattern, plus a pre-existing session_id sink in history_list.html.

Standard solution chosen — and why

DOM construction (createElement / textContent / setAttribute / addEventListener + dataset) for the genuinely attacker-controlled JS-string and event-handler sinks. This is the browser-native, zero-dependency safe answer: quotes and markup become inert by construction because values are assigned as data/text, never parsed as HTML or JS. It's also already the established pattern in this repo — nav.js's createBadgeElement builds its nodes exactly this way.

For the two large template-literal render files that aren't worth a full rewrite in a bug-fix PR (conversation_live.js, diff_viewer.html), I hardened the existing escapeHtml to escape all five HTML-significant characters, closing their attribute-interpolation sinks.

What I rejected:

• A new bespoke escaper as the strategy — the maintainer explicitly flagged "we shouldn't have to roll our own," and an escaper is the fragile approach (context-sensitive: an HTML-text escaper is wrong in attribute/JS-string contexts). DOM construction removes the context-sensitivity entirely.
• A small auto-escaping templating/sanitizer library (e.g. DOMPurify, lit-html) — the templating volume is small and localized to a handful of render functions. Adding a JS dependency to a repo with no package.json / no bundler / no JS toolchain violates the repo's KISS / no-unnecessary-deps norms. The cost isn't justified.

The dependency tradeoff: DOM construction is more verbose than template literals, but it's stdlib, it matches existing code, and it's the only option that makes the sink safe regardless of context.

Files / sinks fixed

DOM-construction migration (attacker-controlled JS-string / event-handler sinks):

• history_list.html — viewSession('${escapeHtml(session.session_id)}') onclick + preview/meta rendering; broken escapeHtml removed (now unused).
• diff_viewer.html — selectCall('${call.call_id}') onclick (was unescaped) + recent-calls card text.
• request_logs.html — showTransaction('${log.transaction_id}') onclick (was unescaped) + log rows (endpoint/model/direction) + transaction/session detail meta.
• inference_providers.html — editProvider('...') / deleteProvider('...') inline onclicks → addEventListener + dataset; resolves the file's own TODO(post-merge) about this exact idiom; its (correct) escapeHtml removed (now unused).

Escaper hardening (attribute-interpolation sinks in large template-literal files):

• conversation_live.js — escapeHtml now escapes "/'; closes the data-tool-call-id="${escapeHtml(...)}" attribute breakout.
• diff_viewer.html — escapeHtml hardened for defense-in-depth (its remaining uses are HTML-text, already safe).

Tests:

• tests/luthien_proxy/unit_tests/ui/test_static_xss_guards.py — source-level regression guards: no inline-handler JS-string interpolation in the migrated files, retained escapers escape both quote characters, and each specific sink is wired via addEventListener. (The repo has no JS runtime, so these are source-text assertions — see "Not runtime-verified" below.)

Out of scope (deliberately)

policy_config.js / form_renderer.js also use inline onclick="window.moveSubPolicy('${safePath}', ...)". I left these: safePath goes through the file's esc() which does escape quotes, and path is an internally-computed JSON-schema field path (derived from the policy config schema, not from request traffic) — not attacker-influenced. Migrating them to addEventListener for consistency is a reasonable follow-up but isn't part of this XSS-class fix.

Relationship to #780 / #752

• feat: user-differentiation core (history per-user view, labels, session_summaries) #780 (user differentiation) is in flight touching history_list.html and adding a local quote-hardening to that file's escapeHtml. This PR is built on current main, not feat: user-differentiation core (history per-user view, labels, session_summaries) #780, and addresses the root cause class-wide — it may supersede feat: user-differentiation core (history per-user view, labels, session_summaries) #780's local hardening. Whichever merges second should reconcile history_list.html (this PR removes that file's escapeHtml entirely in favor of DOM construction, so feat: user-differentiation core (history per-user view, labels, session_summaries) #780's local hardening of it becomes moot).
• feat(ui): cursor pagination + lazy loading for admin dashboard #752 (cursor pagination) flagged an XSS in fragments/sessions.html. That fragment file does not exist on current main (neither does feat: user-differentiation core (history per-user view, labels, session_summaries) #780's user_id UI sink) — so it's not in this PR's tree. The pattern it represents is exactly what this PR eliminates; when feat(ui): cursor pagination + lazy loading for admin dashboard #752 lands, its sessions.html should use DOM construction for session_id rather than escapeHtml-into-onclick.

CSP backstop (noted, not implemented)

A Content-Security-Policy header on the admin UI routes (src/luthien_proxy/ui/routes.py) would be a strong defense-in-depth backstop. It's not implemented here because every admin page uses inline <script> blocks and inline event handlers (Alpine's x-data/@change, the policy-config inline onclicks). A meaningful script-src 'self' CSP would break all of them and requires either per-page nonces or moving all inline JS to external files — a substantial, separate refactor that doesn't belong in a focused bug-fix PR. Recommended as a follow-up.

One PR = One Concern / COE

Strictly the XSS-class fix; no feature changes. This is a bug fix and warrants a COE-style note (root cause: context-insensitive hand-rolled escaper + string-interpolated HTML; the recurrence across #780/#752 is the signal that the pattern, not any single sink, is the defect).

🤖 Generated with Claude Code

[claude]

Review

Clean, well-scoped XSS-class fix. The PR description does most of my work for me — the rationale for DOM construction over a bespoke escaper or a new dependency is convincing, the inline comments at each migrated sink explain why (not just what), and the regression guards are a good way to prevent re-introduction in a repo with no JS runtime. A few observations:

credentials.html has the same pattern and isn't addressed

src/luthien_proxy/static/credentials.html:632 is the exact stored-XSS class this PR is eliminating:

html += '<td><button class="danger" onclick="invalidateOne(\'' + cred.key_hash + '\')">Invalidate</button></td>';

And :628 interpolates cred.key_hash into a title="..." attribute and as text — also unescaped. :642 interpolates e.message into innerHTML.

The values happen to be safe today — key_hash is hashlib.sha256(...).hexdigest() (credential_manager.py:79-81), so it's constrained to [0-9a-f], and e.message comes from fetch/JSON errors. But that's defense by character-set, the exact failure mode that bit #780 / #752 when "internally derived" turned out not to be.

The PR description explicitly carves out policy_config.js / form_renderer.js as out-of-scope with rationale (internally-computed path, escaped via esc()). credentials.html isn't mentioned in either bucket. Suggest one of:

• Add it to the DOM-migration sweep (it's three sinks).
• Acknowledge it explicitly with the same "non-attacker-controlled by SHA-256 character set" reasoning as the policy_config carve-out, so a future reader knows it was considered.

Regression-guard regex is narrower than the docstring implies

_INLINE_HANDLER_JS_STRING (test_static_xss_guards.py:74-77) is:

on\w+\s*=\s*["'][^"']*\(\s*\\?['"]\$\{

The [^"']* between = and ( means the regex only catches the first argument-list opening, with no intervening quotes. It catches the patterns this PR removed (onclick="foo('${x}')") but would slip past:

• Multi-arg: onclick="foo(123, '${x}')" — the ' before ${ is the second argument, but the regex can't get past the first ( and the comma run because [^"']* ends at the first quote.
• Backslash-escaped variant inside double-quoted attribute followed by additional args: onclick="foo(\"a\", \"${x}\")".

Not blocking — none of the migrated files have these shapes today — but worth either widening the regex (e.g., allow [^"']*?(?:\([^"']*)? to skip through prior args) or noting the limitation in the docstring so a future maintainer doesn't trust it for more than it catches.

Inline comment justifying timeEl.innerHTML = timeStr is correct

history_list.html:610-613 — verified: formatTimeRange at :681 composes its result from a static days/months array and Date numeric methods. No request data flows into timeStr. The "limited static markup is safe" claim holds. Nice to leave the breadcrumb.

Minor: hardcoded-HTML innerHTML sites left in migrated files

history_list.html:542 (container.innerHTML = '<div class="empty-state">No sessions found</div>') and inference_providers.html empty-state path now use the new DOM-construction style — but :542 still uses innerHTML with a string literal. Functionally safe (no interpolation), but minor inconsistency with the new pattern in the same file. Trivially: replaceChildren + createElement.

Defense-in-depth note on the hardened escapeHtml helpers

The new five-char escaper in conversation_live.js and diff_viewer.html is correct for HTML-text and quoted attribute contexts. It is still not safe for unquoted attributes, JS-string contexts, or URL contexts. The current call sites are all quoted-attribute / HTML-text, so this is fine today. If you want a future-proof guard, an assertion or comment at the function header — "use only in HTML text or quoted attribute contexts" — would prevent the next maintainer from reaching for it in an href="javascript:..." or unquoted data-foo=${...} context.

CSP follow-up

Strongly agree it's a separate refactor — the inline <script> blocks plus Alpine x-data/@change mean a meaningful script-src 'self' needs nonces or a real extraction pass. Worth a Trello ticket so it doesn't drop.

Summary

Approve with the credentials.html ask — that's the one item I think genuinely belongs in this PR (or in an explicit out-of-scope note), because it's the same class and the PR's stated framing is "this class, not these instances." Everything else is nits / follow-ups.