harden(web-ui): escAttr/data-* sweep across remaining attribute interpolations (#123)

[mvalancy]

Follow-up from the #121/#122 re-review. #121 added escAttr() (escapes & < > " ') and applied it to the notification / dl-card / dl-panel-Cancel attribute sites. The review found the SAME class elsewhere in web-ui/index.html, still using text-context esc() (no quote escaping) or raw interpolation. All values are server/catalog-constrained ids today (not exploitable free-text), but the codebase now has the correct helper, so this is a consistency sweep completing the #121 hardening class.

Sites fixed (per-site fix)

Attribute context → escAttr() (a " in the value can no longer break out of the double-quoted attribute and inject an event handler):

• catalogCardHtml — data-id="${esc(it.id)}" → escAttr (the exact esc-in-attribute pattern harden(web-ui): notification inbox passes id via inline onclick — safe today only by server id-constraint, not esc(); use data-id + delegation #121 warns against); also folded in data-search on the same element / same it (was esc()), since leaving it would be inconsistent with the fixed data-id beside it.
• renderFullWikipediaHero — data-id="${full.requestId}" (was raw) → escAttr.
• renderContentDetail — data-id="${item.requestId}" (was raw) → escAttr.

Inline-JS string → data-* + constant delegated handler (escAttr alone is not enough here — a ' still breaks the JS string; converting removes the inline-JS interpolation entirely, mirroring dlMiniAction/triggerRequestFromEl):

• renderSafety mod-review buttons — onclick="reviewModItem('${esc(it.id)}',…)" → data-mod-id/data-mod-act + new reviewModFromEl(this).
• startCommunityService('${…}', this) ×3 (health repair _repairBtn, tool detail, community hub — one was esc()-in-inline-JS, two were raw) → data-svc-id + new startServiceFromEl(this). Covers the ticket's "any similar onXXX=\"fn('${...}')\"".

Text context → esc() (lower risk):

• renderProgressPanel — ${dl.type} → esc(dl.type).

Comment: tightened the _onNotifAction comment to cite escAttr (attribute-escape at render + read-via-getAttribute), matching its siblings.

No ' or " breakout remains at any converted site: attribute values are escAttr-escaped (quotes included) and ids that used to sit in inline JS are now read from data-* via getAttribute, never interpolated into a parser as code.

Intentionally left as-is (documented residual): ~40 other attribute interpolations in index.html are fed by hardcoded catalog constants baked into the HTML (TOOLS/MODEL_FAMILIES/CONTENT_LIBRARY literals — logos, alt, href, data-search, triggerDownload('…','${constant}')), not server/free-text; and the constant-fed inline handlers setNotifFilter/setModerationSensitivity take hardcoded enum values. Two server-derived esc()-in-attribute sites in the unrelated renderPackages function (data-kind/href from /api/packages, a constrained enum + server-generated archive path) were not flagged by the #121/#122 review and are out of this ticket's scope — noted here as a candidate for a future pass rather than widening this diff.

Tests

New tests/screenshots/specs/escattr-sweep.spec.ts (2 tests) drives the two highest-value converted sites with one adversarial id carrying every vector at once — " (attribute breakout) + '/');payload (inline-JS breakout) + onmouseover= + <img … onerror=>:

1. catalog Download data-id (attribute escape): no injected onmouseover, id round-trips verbatim in data-id, onclick is the constant triggerRequestFromEl(this), hover executes nothing, and clicking Download POSTs the verbatim id to /api/request.
2. mod-review Dismiss (inline-JS → delegation): onclick is the constant reviewModFromEl(this) and contains neither the id nor the payload, no injected onmouseover, data-mod-id/data-mod-act round-trip verbatim, hover executes nothing, and clicking Dismiss POSTs the verbatim {id, action} to /api/moderation/review.

Fail-without / pass-with verified: both tests fail against the temporarily-reverted pre-fix code (catalog: onmouseover was injected → Received: "window.__xss=1"; mod-review: the data-mod-act button doesn't exist) and pass after the fix.

Test counts (all offline, stubbed via page.route):

• New spec: 2 passed.
• Affected specs re-run green: health + notifications (incl. the harden(web-ui): notification inbox passes id via inline onclick — safe today only by server id-constraint, not esc(); use data-id + delegation #121 sibling breakout tests) + nav + ui-exercise (clicks every button on every route, exercising the new wrappers) + new spec = 60 passed.
• Full web-ui.spec.ts: 226 passed, 33 skipped, 1 failed — the one failure (ZIM content directory exists) is a pure fs.existsSync check for content/zim, which isn't materialized in a fresh worktree (runtime appliance symlink); unrelated to this diff and fails on clean dev here too.
• No bash validator inspects index.html or these render functions, so none are affected.

Knowledge

Appended a distinct anchor to .agents/knowledge/pipeline-insights.md: a hardening/security PR must prove the specific failure mode it claims to close (test the exact attack, not a harmless variant — and verify fail-without by reverting), durable docs must state accurate invariants (the review once blocked a PR that shipped a false "now safe" guarantee — esc() in a double-quoted attribute, #121), and prefer data-* + delegation over interpolating data into markup/inline-JS.

Residual risk

Low. All swept ids are server/catalog-constrained today, so this is defense-in-depth; the change is behavior-preserving (delegated handlers read the same values via getAttribute; dl/mod/community actions verified working). Web UI stays vanilla/zero-dep; both themes + a11y unaffected (no markup structure change beyond swapping inline handlers for data-* + constant handlers).

Fixes #123

🤖 Generated with Claude Code