refactor: move test-only PolicyContext.for_testing into a test fixture

[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 refactor — moves the test-only PolicyContext.for_testing() factory out of src/ and into a tests/ fixture. Production class shrinks by 45 lines of dead-in-prod code, and the fixture file is a near-identical paste so behavior is preserved.

Positives

• Real correctness win, not just hygiene: for_testing() was wiring NullEventEmitter, so any accidental call from production code would have silently bypassed event auditing. Removing that footgun is worth doing.
• Function (make_policy_context) instead of classmethod is the right call once it's no longer tied to the class — no reason to keep the cls machinery.
• All call sites updated in one pass; no stragglers (grep -rn "for_testing" src/ tests/ is clean except for the leftovers listed below).
• Changelog fragment present and accurate.

Findings

1. PR description is from a different PR (blocking before merge).
The description body is the XSS / escapeHtml fix writeup — talks about history_list.html, diff_viewer.html, DOM construction, CSP backstop, etc. None of that is in this diff (the diff is purely the PolicyContext.for_testing → fixture move). If this is squash-merged, the merge commit body will be wrong and confusing in git log. Please rewrite the description before marking ready / merging.

2. Leftover for_testing naming in tests (nit, but easy).
The rename is incomplete in a few spots — the new function is make_policy_context but these still read as if it weren't:

• tests/luthien_proxy/unit_tests/policy_core/test_policy_context.py:143 — class TestPolicyContextForTesting (docstring updated, class name not)
• tests/luthien_proxy/unit_tests/policy_core/test_policy_context.py:146-147 — test_for_testing_creates_valid_context / \"for_testing() creates a usable PolicyContext.\"
• tests/luthien_proxy/unit_tests/policy_core/test_policy_context.py:303 — test_for_testing_forwards_policy_cache_factory
• tests/luthien_proxy/unit_tests/credentials/test_policy_context_credentials.py:80 — class TestPolicyContextForTesting
• tests/luthien_proxy/unit_tests/credentials/test_policy_context_credentials.py:83-84 — test_for_testing_accepts_credential_params / \"for_testing() accepts credential parameters.\"

A pure mechanical rename (for_testing → make_policy_context in those identifiers/docstrings) closes the loop.

3. Redundant string quotes on type hints (style nit).
tests/luthien_proxy/fixtures/policy_context.py has from __future__ import annotations at the top, so all annotations are already strings at evaluation time. The quotes on \"Credential | None\", \"CredentialManager | None\", \"InferenceProviderRegistry | None\", \"PolicyCacheFactory | None\" aren't needed and aren't applied consistently (e.g. RawHttpRequest | None is unquoted in the same signature). Either unquote all four for consistency or drop the __future__ import and keep the quotes — current state is mixed.

Non-issues I checked

• Signature parity with the old for_testing() — identical (same params, same defaults, same kwargs).
• tests.luthien_proxy.fixtures.* import ordering — matches existing pattern (see test_anthropic_stream_validator.py, test_credential_manager_resolve.py).
• TYPE_CHECKING import block — correct for breaking import cycles with credential_manager / inference.registry / utils.policy_cache.
• Test coverage — same tests, just rebound to the new symbol. No coverage regression.

Recommendation: fix the PR description (blocker), and ideally the for_testing identifier leftovers (nit). LGTM otherwise.