fix(ui): credentials.html XSS sinks + harden XSS guards (follow-up to #781)

[jaidhyani]

Follow-up to #781 (admin UI stored-XSS class fix), which merged before this bot-review round was addressed. Same concern, same DOM-construction approach; these are the review items that landed after #781 merged.

What this adds

credentials.html — the real gap #781 missed

The bot correctly flagged that credentials.html had the exact stored-XSS class #781 eliminates, but #781's sweep missed it because the file built HTML by raw string concatenation with no escapeHtml at all — so a grep for escapeHtml( never surfaced it. Migrated both credential tables to DOM construction:

• Cached credentials: onclick="invalidateOne('" + cred.key_hash + "')" (inline-onclick concat) → addEventListener + dataset.keyHash; key_hash also flowed into a title="..." attribute and cell text unescaped → textContent. key_hash is SHA-256 hex (safe by character set today), but that's exactly the "internally derived so it's fine" assumption that rotted in feat: user-differentiation core (history per-user view, labels, session_summaries) #780/feat(ui): cursor pagination + lazy loading for admin dashboard #752 — migrated for real rather than carved out.
• Server credentials: credential name flowed into a data-name="..." attribute via a quote-unsafe escaper (escHtml), gated only by a server-side regex → DOM construction with dataset.name; the existing delegated click listener reads data-name unchanged. Removed the now-unused escHtml.
• Error sinks: the config / cached-credentials / server-credentials catch blocks injected e.message into innerHTML. e.message is unconstrained server-returned text (not character-set-limited like key_hash), so these are genuine sinks → textContent.

Regression-guard hardening

• Widened _INLINE_HANDLER_JS_STRING: it previously used [^"']*, which stopped at the first quote — so it only caught ${...} template-literal shapes and missed the string-concat form (onclick="f('" + x + "')") that credentials.html actually used, plus multi-arg shapes. Now catches both, with its exact coverage and accepted gaps documented in the comment rather than over-claimed (it flags the dangerous opening idiom; it does not parse JS or prove attacker-influence). Added a self-test for the concat shape and a credentials.html-specific guard.
• Added context-limit header comments to the hardened escapeHtml helpers (conversation_live.js, diff_viewer.html): safe for HTML text and quoted attributes only — NOT JS-string, URL/javascript:, or unquoted-attribute contexts (use DOM construction there). Prevents the next maintainer from reaching for it in an onclick/href sink.
• history_list.html empty-state: innerHTML='<div...>' string literal → replaceChildren (consistency nit; trivial).

Deferred (tracked, not in this PR)

• CSP backstop on the admin UI routes → Trello card created (https://trello.com/c/B8wobuI2). It's a separate refactor: every admin page uses inline <script> blocks and inline Alpine handlers, so a meaningful script-src 'self' CSP needs per-page nonces or extracting all inline JS to files.

Out of scope (unchanged from #781's reasoning)

policy_config.js / form_renderer.js inline onclick handlers interpolate safePath (via a quote-escaping esc()) and path — an internally-computed JSON-schema field path, not request/attacker data. Left as-is, consistent with #781.

./scripts/dev_checks.sh green (ruff, pyright 0 errors, full unit suite incl. the 13 XSS guards).

🤖 Generated with Claude Code

[claude]

PR Review — fix(ui): credentials.html XSS sinks + harden XSS guards

Solid follow-up to #781. The DOM-construction migration is correct, the comments explain why (not just what), and widening _INLINE_HANDLER_JS_STRING to catch the string-concat shape — the form that actually appeared in credentials.html and that the original escapeHtml-name grep missed — is a real improvement. The self-test (test_inline_handler_guard_catches_concat_shape) is a nice touch: it pins the regex's behaviour so the next widening can't silently regress past matches.

A few items worth considering:

1. config_dashboard.html has the same class of bug — not addressed by this sweep

The PR's reasoning for migrating credentials.html (raw concat with no escaper at all; e.message is unconstrained server-returned text) applies almost verbatim to src/luthien_proxy/static/config_dashboard.html:

• Line 454: identical e.message sink the PR just fixed in credentials.html:

  document.getElementById('content').innerHTML = `<div class=\"section\" style=\"color:#f87171\">Error: ${e.message}</div>`;

• Lines 326–330: defines its own esc() using the textContent/innerHTML trick that escapes <>& but not quotes — the same anti-pattern fix(ui): close stored-XSS class in admin UI static assets #781 hardened in conversation_live.js and diff_viewer.html.
• Lines 384, 389: esc(val), esc(name), esc(field.type) are interpolated into quoted value=\"…\", data-field=\"…\", data-type=\"…\" attributes via that quote-unsafe esc().

Field names and types are developer-defined in config_fields.py today, but that is exactly the "internally derived so it's fine" assumption the PR explicitly rejected for key_hash. The e.message line is unambiguously a sink by the PR's own criterion. Suggest either folding it into this PR (it's small and self-contained) or opening a Trello card so it doesn't quietly outlive the sweep — and ideally adding config_dashboard.html to DOM_MIGRATED_FILES so the regression guard covers it.

2. test_credentials_uses_dom_construction_for_key_hash — brittle assertion

assert \"onclick=\" not in source, \"credentials.html must not use inline onclick handlers.\"

This is a substring match against the entire file, including comments and string literals. It happens to pass today because the file's lone remaining mention of "onclick" (line 625) is onclick JS without =. But a future comment like // removed onclick=\"foo\" or a string '<button onclick=…>' in a (safe) test fixture would fail it. Consider scoping to actual attribute usage, e.g. re.search(r'\\bonclick\\s*=\\s*[\"\\']', source).

3. credentials.html line 733 — leftover innerHTML for static content

loading.innerHTML = '<span class=\"spinner\">&#x23F3;</span> Loading credentials...';

Fully static so it's safe, but inconsistent with the file's new DOM-construction style. Trivial — flagging for consistency, not correctness.

4. Regex coverage caveat is well-documented, but worth a TODO

The widened _INLINE_HANDLER_JS_STRING deliberately catches "first dynamic string after (" only. The comment states this clearly, but a future shape like onclick=\"f('const', '\" + x + \"')\" (constant first arg, dynamic second arg) slips past. Not a request to widen further now — the lazy quantifier tradeoffs are real — just suggesting a # TODO: next to the caveat so it isn't lost.

5. Quick adjacent scan — activity_monitor.js, policy_config.js

Both contain several el.innerHTML = \…`template literals.policy_config.jsis explicitly deferred (per the PR description:safePathis internally computed JSON-schema path), andactivity_monitor.js` appears to interpolate event metadata. Worth a quick once-over to confirm none of those interpolations cross the "attacker-influenced" line — or note them as deferred alongside the existing Trello card.

Coverage

The widened guard + the new test_credentials_uses_dom_construction_for_key_hash test pin both the file-level migration and the regex behaviour, which is the right level of coverage for static-asset XSS. No JS DOM runtime, so source-text assertions are the pragmatic ceiling — that limitation is already acknowledged in the module docstring.

Nice work — the rationale in the comments ("key_hash is SHA-256 hex today, but that assumption rots") and the up-front documentation of the regex's gaps both raise the bar for future XSS reviews in this repo.

[jaidhyani]

Addressed in 57518d73. Rather than patch the one new file, I ran a comprehensive sweep of every file under src/luthien_proxy/static/** so a fresh review should find nothing new. Per-file disposition:

File	Disposition
activity_monitor.js	Migrated. Real attacker-reachable sink — the activity stream is derived from live request traffic. createEventElement rendered the full event via JSON.stringify(event) into a <pre> with no escaping (JSON.stringify doesn't escape </>, so a payload field like </pre><script>… injects script), plus event_type as HTML text; rebuildFilterDropdown interpolated request-derived category/subtype into innerHTML incl. id=""/value="". All → DOM construction (textContent/dataset/property assignment).
config_dashboard.html	Migrated (review #1). Hardened the quote-unsafe esc() (used in value=""/title=""/data-field="") to escape all five chars; e.message→innerHTML catch sink → textContent. Field metadata is dev-defined today, but applied the rots-assumption bar anyway.
diff_viewer.html	Migrated (completed). The two error.message→innerHTML catch handlers I missed in pass 1 → textContent.
credentials.html	Migrated (prior commit) + spinner static-literal → replaceChildren (review #3).
history_list.html, request_logs.html, inference_providers.html	Migrated (PR #781 / prior commit).
conversation_live.js	Hardened escapeHtml (quotes); text-context uses safe.
policy_config.js, form_renderer.js	Safe, not migrated. Quote-correct esc() everywhere dynamic; the only inline-handler interpolations are safePath/path — internally-computed JSON-schema field paths, not request/attacker data. Deferred, consistent with prior reasoning + the CSP backstop. (review #5: confirmed policy_config.js stays deferred.)
nav.js	Safe. Full DOM construction already.
client_setup.html	Safe. Its one dynamic value ({{BASE_URL}}) is html_escape'd server-side in ui/routes.py; copyCode reads innerText.
index.html, conversation_live.html, policy_config.html, activity_monitor.html	Safe. Static shells, no dynamic JS / no innerHTML.

Test-guard items:

• Remove PolicyEngine and inline Redis setup #2 (brittle test): replaced the whole-file "onclick=" not in source substring match with a scoped _INLINE_HANDLER_ATTR = re.compile(r'\bon\w+\s*=\s*["\']') that only fires on real attribute usage (not comments/CSS/prose), with a self-test.
• gh actions to check correctness #4: added a TODO next to the _INLINE_HANDLER_JS_STRING caveat documenting the still-uncaught constant-first-arg / dynamic-second-arg shape.
• Added config_dashboard.html + activity_monitor.js to DOM_MIGRATED_FILES; generalized the escaper test to cover esc() as well as escapeHtml. 19 guard tests pass; full unit suite 2773 passed; dev_checks green.

Deferred (tracked): CSP backstop on the admin UI routes — separate refactor (needs nonces or inline-script extraction), Trello card https://trello.com/c/B8wobuI2. The deferred policy_config.js/form_renderer.js inline handlers become trivially removable once that CSP lands.

No JS runtime here, so these guards are source-text assertions, not live DOM tests — honest limitation.

---

Posted by Claude Code (Opus 4.8 (1M context))

[claude]

Code Review — PR #782

Thorough, well-scoped follow-up to #781. The DOM-construction migration is consistent across files, the regex hardening is honest about its gaps, and the new tests pin the specific shapes that bit this codebase. A few notes below; nothing blocking.

Code quality & correctness

• activity_monitor.js:227 — incidental bug fix worth calling out. Wrapping transactionId in String(...) before .substring(0, 12) prevents a TypeError if transaction_id/call_id ever arrives as a non-string from the stream. The old code would have thrown; the new code is defensively cast. Good catch; might be worth a one-liner in the PR description so reviewers see it as intentional rather than incidental.

• credentials.html:671 closure capture is clean. addEventListener('click', () => invalidateOne(cred.key_hash)) uses for...of with const cred, so there's no stale-index closure pitfall that var + classic for would have. Good.

• credentials.html:737 spinner uses the literal ⏳ (U+23F3) rather than ⏳. Correct — textContent doesn't interpret HTML entities, so the entity form would have rendered as the literal string ⏳. Just confirming this is intentional and depends on UTF-8 source encoding, which the file is.

• Delegated click listener at credentials.html:879-882 reads data-name via getAttribute('data-name') and the producer sets it via btn.dataset.name = name. These pair correctly (DOMStringMap normalizes dataset.name → data-name). Verified the contract is intact.

• escapeHtml / esc comment updates explicitly enumerate unsafe contexts (JS-string, URL/javascript:, unquoted attributes). This is the right kind of comment — it tells future maintainers what not to do, not just what the function does. Nice.

Regression-guard hardening

• Widened _INLINE_HANDLER_JS_STRING regex now catches both ${...} and string-concat shapes, with documented gaps (constant-first-arg / dynamic-second-arg slips past). The TODO is the right call — full coverage requires either a JS parser or a CSP forbidding inline handlers outright (already on the Trello card). The self-test (test_inline_handler_guard_catches_concat_shape) pins the new coverage and the non-match cases.

• _INLINE_HANDLER_ATTR regex is properly scoped with \bon\w+ so it doesn't fire on class=\"onclick-styled\" or comments. The self-test (test_inline_handler_attr_regex_is_scoped) is a small but valuable addition; regex tests for security guards are easy to skip and easy to regret.

Minor / non-blocking

• Changelog handling. The PR amends changelog.d/fix-admin-ui-xss-class.md (from fix(ui): close stored-XSS class in admin UI static assets #781) rather than creating fix-admin-ui-xss-followup.md. Per changelog.d/README.md the convention is <short-handle>.md. Since fix(ui): close stored-XSS class in admin UI static assets #781's release hasn't been cut yet (the fragment is still in changelog.d/), amending in place produces a single coherent entry, which is arguably the better user-facing result. Worth noting only because the next person reading the README convention may wonder. Either choice is defensible — flagging, not objecting.

• Duplicated escape helper. escapeHtml in conversation_live.js / diff_viewer.html and esc in config_dashboard.html now all implement the same five-character replace with the same caveats. There's no module system on these static pages, so DRY-ing them requires either a shared <script src> or build step — out of scope for this PR, but worth a Trello card alongside the CSP work since both touch "how do we share JS across admin pages".

• errSpan.textContent = 'Failed to load credentials: ' + e.message could be errSpan.append('Failed to load credentials: ', e.message) for parity with the rest of the DOM-construction style. Trivial; no behavior difference.

Security

• No new sinks introduced. All migrated paths route attacker-influenced data through textContent/dataset/addEventListener. The remaining hand-rolled escapers now cover quoted-attribute contexts and have explicit comments rejecting JS-string / URL contexts.
• e.message sinks (config_dashboard.html, credentials.html, diff_viewer.html) correctly identified as unconstrained server text and converted to textContent. This is the right call — server error strings are exactly the surface you don't want bypassing the escaper because they look "internal".
• CSP backstop is correctly deferred — making a meaningful script-src 'self' work requires removing inline scripts/Alpine handlers across every admin page. Tracked in Trello.

Test coverage

Strong. New parametrized tests cover the three additional DOM-migrated files (credentials.html, config_dashboard.html, activity_monitor.js), the third escaper (config_dashboard.html's esc), and the two regex shapes. The guard list is the right shape: source-text assertions pinning the specific anti-patterns that have recurred, not aspirational "no XSS" assertions.

One small suggestion: test_credentials_uses_dom_construction_for_key_hash asserts not _INLINE_HANDLER_ATTR.search(source). That's the strongest possible guard for credentials.html (zero inline handlers) and is correct today, but it will fail loudly the next time someone adds any onclick to that file even if unrelated to key_hash. That's arguably a feature for this specific file given its history, but worth being deliberate about — consider adding a comment in the test explaining the assertion is intentionally file-wide, not key_hash-specific, so a future contributor understands why it's structured this way.

---

Overall: ship it. The PR is doing exactly what a security follow-up should — close the gap the original sweep missed, harden the detection that should have caught it, and document the residual gaps rather than over-claim.