Skip to content

fix(brain): collapsed-run time range rendered HTML as literal text#1215

Merged
vivekchand merged 1 commit into
mainfrom
fix/brain-collapsed-range-html-escape
May 14, 2026
Merged

fix(brain): collapsed-run time range rendered HTML as literal text#1215
vivekchand merged 1 commit into
mainfrom
fix/brain-collapsed-range-html-escape

Conversation

@vivekchand

@vivekchand vivekchand commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

Test plan

  • Headless chromium against Brain tab, post-fix, shows clean 13 May 22:54:19 → 13 May 19:54:45 instead of escaped <span>.
  • All 16 telegram CHANNEL.OUT events + 6 OpenClaw TUI events render correctly.

🤖 Generated with Claude Code

@vivekchand
fix(brain): collapsed-run time range rendered HTML as literal text
34998d8 
E2E playwright test against the Brain tab caught literal
`<span style="opacity:0.45;font-size:10px;margin-right:3px;">13 May</span>`
text appearing in the collapsed-outbound row's time range.

`formatBrainTime()` returns trusted HTML (wraps the date prefix in a
styled span), so wrapping `rangeLabel` in `escHtml()` turned that
markup into visible text. Drop the escape — the helper output is
trusted and the literal " → " separator carries no user input.

Caught during full E2E verification of #1212 + #1213.
This was referenced May 14, 2026
Merged
Merged
@vivekchand
vivekchand merged commit 7b6eb1d into main May 14, 2026
10 checks passed
@vivekchand
vivekchand deleted the fix/brain-collapsed-range-html-escape branch May 14, 2026 11:14
@vivekchand

vivekchand commented May 14, 2026

Copy link
Copy Markdown
Owner Author

UX review

  • 🟡 P1 — The fix is correct (drop escHtml around trusted formatBrainTime() output) but the wider pattern is the worry: we have two adjacent html += '<span ...>' + escHtml(X) + '</span>' lines on L2895 and L2902. One escapes (correctly — it's a free-text title attr), one DOESN'T (after this fix — markup-bearing). A reader skimming the diff has no way to know which is which without reading the helper. Add a 1-line comment marker on every escHtml(...) call site explaining "why escape" or "why trusted". Action: clawmetry/static/js/app.js:renderBrainStream — comment marker convention (/* TRUSTED-HTML */) on the new branch, plus a follow-up rename — formatBrainTime → formatBrainTimeHtml so the trusted-HTML return type is in the name.
  • 🟡 P1 — formatBrainTime() returns trusted HTML strings — but it ALSO is called in plenty of other places where HTML escaping COULD have been needed. If a future caller forgets that the return is HTML, they'll double-escape or worse. Rename to formatBrainTimeHtml to encode the contract in the symbol name. Action: clawmetry/static/js/app.js — rename formatBrainTime → formatBrainTimeHtml, audit all callers.
  • 🟢 nit — The collapsed run shows 13 May 22:54:19 → 13 May 19:54:45 — the END time is BEFORE the start time (PM/AM confusion or descending sort). Probably correct (most-recent first) but reads as a bug to a non-technical user. Worth confirming the sort order convention is consistent across the Brain tab. Action: visual QA pass on collapsed-run rendering with mixed-day events.

@vivekchand

vivekchand commented May 14, 2026

Copy link
Copy Markdown
Owner Author

Product review

  • 🔴 P0 — renderBrainStream is hand-built string concatenation of HTML. We've now had at least two XSS-adjacent fixes here in two weeks. First-principles: stop building HTML by string-concat in 2026. Either (a) migrate this renderer to document.createElement + textContent (zero string-concat, zero escape decisions), or (b) adopt a tiny tagged-template helper like html\...${userInput}`that does the escaping for us. Today's PR is a correct one-line fix; the LANDMINE that produced it is the whole approach. Action: file follow-up issue "MigraterenderBrainStream` off raw string-concat HTML" and stop accepting these as one-liners.
  • 🟡 P1 — Test plan says "[x] Headless chromium against Brain tab, post-fix, shows clean ..." but no E2E test landed in the PR. The next time someone wraps rangeLabel in escHtml because they think they're being defensive, we have no Playwright assertion to catch it. Add one. Action: tests/e2e/test_brain.spec.ts (or wherever brain E2E lives) — assert the collapsed-run text matches /^\\d+ \\w+ \\d+:\\d+:\\d+ → \\d+ \\w+/ (no literal <span).
  • 🟢 nit — Zero tier-strategy implications. Brain tab is OSS-tier and the fix is universal. Ship it.

This was referenced May 14, 2026
Closed
Merged
vivekchand added a commit that referenced this pull request May 16, 2026
@vivekchand @claude
fix(brain): use plain-text time in collapsed-run range label (#1388)
f6ba742 
formatBrainTime() returns HTML (a <span> wrapping the date prefix for
opacity styling), making rangeLabel an HTML string that must NOT be
passed through escHtml() — a fragile invariant that already caused
one regression (PR #1215 double-escaped it).

Add _formatBrainTimePlain() that returns "Today 14:23:45" as plain
text. Switch the two formatBrainTime() calls in the collapsed-run
rangeLabel path to use it; the range label can now be safely escaped
with escHtml() and the warning comment explaining why not to escape
is gone.

No change to existing formatBrainTime() callers — the styled brain-time
spans are unaffected. Minor visual: date prefix in collapsed-range badge
no longer has reduced opacity, which is acceptable in a summary badge.

Closes #1222

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vivekchand