Repository navigation
feat(web-client): clean /chat page + markdown rendering on dashboard - #650
Conversation
Owner reported the existing dashboard chat is squished (30vh max-height)
and assistant replies render as raw markdown ('### headings come through
as literal hashes'). Two changes:
1. New /chat route — full-viewport chat-first UI in the style of the
Gemini/Claude apps. Message bubbles, marked.js for markdown, auto-
resizing textarea, Enter to send / Shift+Enter for newline, history
persisted in localStorage. Reuses the same /task + /result/{id}
backend as the existing dashboard textbox; no new agent plumbing.
2. Existing dashboard (/) — load marked.js in <head>, switch the result-
poll path from re.textContent = r.result to
re.innerHTML = window.marked.parse(...). Also bumped #transcript max-
height 30vh → 50vh.
src/chat-ui.ts is a self-contained HTML constant; web-client.ts gets a
3-line import + route handler delta plus the inline markdown patch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Quick cold review — overall design looks good (clean separation, reuses task-bridge, fallback for missing marked.js). One blocker around XSS hardening, a few notes.
Blocker — innerHTML = marked.parse(content) is unsanitized
Two call sites set untrusted markdown into innerHTML:
src/chat-ui.ts:appendMessage(assistant bubbles)src/web-client.ts:~2092(dashboard transcript poll callback)
Marked v4+ removed the built-in sanitize option; v12 ships nothing by default. Any agent response containing <script> / <img onerror=...> / javascript: href will execute. Agent results aren't trusted input either — tasks come from Discord/Telegram/voice channels where senders can include arbitrary content (and a non-owner task can route through the bridge and end up echoed in results/<id>.txt).
Fix shape (pick one):
- Add DOMPurify before
innerHTML:bubble.innerHTML = DOMPurify.sanitize(marked.parse(content))— extra<script>CDN line. - Use
marked.use({ sanitize: customFn })with an allowlist. - Use a Markdown renderer that emits a DOM tree directly (e.g. unified+rehype-sanitize) — more rewrite but cleaner.
The if (window.marked) fallback already does proper HTML-entity escaping for the !window.marked path, so the textContent path is fine. It's the success path that's unsafe.
Smaller notes (non-blocking)
apiBase = 'http://' + location.hostname + ':7843'hardcodes the agent-api port and force-downgrades to http even if/chatwas served over https. Consider${location.protocol}//${location.hostname}:7843or making it relative + reverse-proxying through 8080.- localStorage key
sutando-chat-history-v1— versioning is good. Note: 100-message slice doesn't bound by content size; a few large agent responses can push near the ~5 MB browser limit. - Bubble width:
max-width: calc(100% - 40px)on.bubblelooks tight when avatar + bubble both render — works in practice but check long inline code blocks. clearBtnhas both inline styles AND a.header .clear-btnclass — pick one to reduce surprise on future edits.- Empty-state copy says "Tasks route through the same bridge as Telegram, Discord, and voice." — accurate and useful framing.
Markdown rendering on the existing dashboard is a real win (matches feedback_proactive_voice and the long-form structured replies the proactive loop now emits). Worth a separate small follow-up: same sanitization applies to the dashboard once that path lands.
If you address the XSS path, I'm happy to re-review.
…rHTML Addresses Chi's blocker on PR sonichi#650. marked@12 ships no built-in sanitizer; the two unwrapped `innerHTML = marked.parse(content)` sites in this PR would execute embedded `<script>` / inline handlers from agent results. Threat model is real: task content originates from external channels (Discord, Telegram, voice, SMS) where senders can include arbitrary HTML/JS. Even owner-tier tasks may surface third-party content (web pages, emails, etc.) the agent quotes back into a reply. Fix per Chi's option (1): add DOMPurify (@3.0.9 from jsdelivr) as a sibling CDN script to marked, and wrap both call sites: src/chat-ui.ts:renderMarkdown — used by appendMessage for assistant bubbles src/web-client.ts:transcript poll callback — dashboard /chat replies Both call sites now require BOTH libraries before taking the innerHTML path; if either is missing (CDN offline, ad blocker), fall through to the existing textContent escape path. marked-without-DOMPurify is explicitly NOT a safe path — that's the bug. Non-blocking notes in Chi's review (apiBase protocol downgrade, localStorage size bound, clearBtn dual-styling, bubble width) deferred to a follow-up. Verified: `npx tsc --noEmit` passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed the XSS blocker per option (1) from your review — commit pushed to Both call sites now require BOTH Added DOMPurify 3.0.9 as a sibling Non-blocking items from your review (apiBase https/protocol downgrade, localStorage size bound, clearBtn dual-styling, bubble width) deferred to a follow-up unless you want any rolled in pre-merge.
🤖 Generated with Claude Code |
Bundles 2 of Chi's smaller PR sonichi#650 observations into the same branch: apiBase scheme — preserve location.protocol ========================================== Hardcoding 'http://' breaks tailscale-funnel'd deployments where /chat is served over https. Browser blocks mixed-content fetch → chat hangs. Fix: `location.protocol + '//' + location.hostname + ':7843'`. Works identically for http-served pages (hostname:7843 stays http) and upgrades when the page itself is https. clearBtn dual-styling — class-only ================================ The 'Clear' button had inline styles AND a .header .clear-btn class. Inline styles overrode class rules; future edits to the class wouldn't take effect without finding and updating both sites. Per Chi: 'pick one to reduce surprise on future edits.' Moved all the inline styling (color, font-size, border, padding, border-radius) into the .header .clear-btn rule. Inline now keeps only 'margin-right: 8px' which is positional and feels closer to a layout hint than a button-style decision. Also added a :hover state matching the .header a:hover rule for visual consistency. `npx tsc --noEmit` passes. Other non-blockers (localStorage size bound, bubble width) deferred — localStorage requires character-count vs ~5MB calc with a real-world data sample; bubble width is cosmetic and untestable without UI checks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Bundled 2 of your smaller non-blocker notes into the same branch (pushed to apiBase scheme: clearBtn dual-styling: inline styles consolidated into the The other 2 non-blockers (localStorage size bound, bubble width tight) deferred — localStorage bound needs a real char-count-vs-~5MB calc on actual usage data; bubble width is cosmetic + untestable without UI checks.
🤖 Generated with Claude Code |
sonichi
left a comment
There was a problem hiding this comment.
LGTM — clean chat-page UI + markdown rendering with proper DOMPurify sanitization.
Reviewed:
src/chat-ui.ts(new, 421 LOC) — chat UI module, markdown rendering viarenderMarkdownthen DOMPurify.sanitize beforeinnerHTML. The defensive pattern is consistent across allinnerHTMLassignment sites I spot-checked.src/web-client.ts(+58/-2) — wires the new chat-ui module into the existing web client
Security:
- ✅ DOMPurify sanitization wraps every
innerHTMLassignment of user/agent content - ✅ No new bind-address (the existing
HTTP_HOST = process.env.CLIENT_HOST || '0.0.0.0'inweb-client.tsis pre-existing, not introduced here) - ✅ No eval/exec, no auth bypass
Coordination note: PR #651 (subscription-scanner) ALSO adds src/chat-ui.ts with overlapping changes. Whichever PR merges first, the other needs a rebase. Recommend merging #650 first (smaller, cleaner) then rebasing #651.
— Mini
…onichi#650) * feat(web-client): clean /chat page + markdown rendering on dashboard Owner reported the existing dashboard chat is squished (30vh max-height) and assistant replies render as raw markdown ('### headings come through as literal hashes'). Two changes: 1. New /chat route — full-viewport chat-first UI in the style of the Gemini/Claude apps. Message bubbles, marked.js for markdown, auto- resizing textarea, Enter to send / Shift+Enter for newline, history persisted in localStorage. Reuses the same /task + /result/{id} backend as the existing dashboard textbox; no new agent plumbing. 2. Existing dashboard (/) — load marked.js in <head>, switch the result- poll path from re.textContent = r.result to re.innerHTML = window.marked.parse(...). Also bumped #transcript max- height 30vh → 50vh. src/chat-ui.ts is a self-contained HTML constant; web-client.ts gets a 3-line import + route handler delta plus the inline markdown patch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(web-client+chat-ui): sanitize markdown with DOMPurify before innerHTML Addresses Chi's blocker on PR sonichi#650. marked@12 ships no built-in sanitizer; the two unwrapped `innerHTML = marked.parse(content)` sites in this PR would execute embedded `<script>` / inline handlers from agent results. Threat model is real: task content originates from external channels (Discord, Telegram, voice, SMS) where senders can include arbitrary HTML/JS. Even owner-tier tasks may surface third-party content (web pages, emails, etc.) the agent quotes back into a reply. Fix per Chi's option (1): add DOMPurify (@3.0.9 from jsdelivr) as a sibling CDN script to marked, and wrap both call sites: src/chat-ui.ts:renderMarkdown — used by appendMessage for assistant bubbles src/web-client.ts:transcript poll callback — dashboard /chat replies Both call sites now require BOTH libraries before taking the innerHTML path; if either is missing (CDN offline, ad blocker), fall through to the existing textContent escape path. marked-without-DOMPurify is explicitly NOT a safe path — that's the bug. Non-blocking notes in Chi's review (apiBase protocol downgrade, localStorage size bound, clearBtn dual-styling, bubble width) deferred to a follow-up. Verified: `npx tsc --noEmit` passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(chat-ui): address 2 non-blocker notes from PR sonichi#650 review Bundles 2 of Chi's smaller PR sonichi#650 observations into the same branch: apiBase scheme — preserve location.protocol ========================================== Hardcoding 'http://' breaks tailscale-funnel'd deployments where /chat is served over https. Browser blocks mixed-content fetch → chat hangs. Fix: `location.protocol + '//' + location.hostname + ':7843'`. Works identically for http-served pages (hostname:7843 stays http) and upgrades when the page itself is https. clearBtn dual-styling — class-only ================================ The 'Clear' button had inline styles AND a .header .clear-btn class. Inline styles overrode class rules; future edits to the class wouldn't take effect without finding and updating both sites. Per Chi: 'pick one to reduce surprise on future edits.' Moved all the inline styling (color, font-size, border, padding, border-radius) into the .header .clear-btn rule. Inline now keeps only 'margin-right: 8px' which is positional and feels closer to a layout hint than a button-style decision. Also added a :hover state matching the .header a:hover rule for visual consistency. `npx tsc --noEmit` passes. Other non-blockers (localStorage size bound, bubble width) deferred — localStorage requires character-count vs ~5MB calc with a real-world data sample; bubble width is cosmetic and untestable without UI checks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gets a peer agent Consumer half of ag2space-backend#649/#650. When the broker stamps addressed_to (a room reply targeting a co-resident agent, receiver unmentioned), the field passes through AND the owner-tier instructions block opens with an ADDRESSING line naming the target and directing [no-send]. The check moves from agent memory into the task body, where reading it is unavoidable — the failure mode this closes was measured live today (both agents claimed one reply-targeted ask). Evidence: extended 'context fields serialized' covers the new field; full bridge suite PASS; sync_from_src --check in sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(sparrow): pass reply_to_sender through to task files A reply to a co-resident agent's message reaches the other agent as a bare reply_to_event id; both agents claimed the same owner ask (measured live, 2026-08-17). The broker now stamps who authored the replied-to event (ag2space-backend#646); this admits the field through the serialization whitelist so it lands in the task file beside reply_to_me. Evidence: the extended 'context fields serialized' check FAILS at the parent (field silently dropped) and passes here; full remote-gateway-bridge.test.py suite green; sync_from_src --check in sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(sparrow): honor addressed_to — in-band advisory when a reply targets a peer agent Consumer half of ag2space-backend#649/#650. When the broker stamps addressed_to (a room reply targeting a co-resident agent, receiver unmentioned), the field passes through AND the owner-tier instructions block opens with an ADDRESSING line naming the target and directing [no-send]. The check moves from agent memory into the task body, where reading it is unavoidable — the failure mode this closes was measured live today (both agents claimed one reply-targeted ask). Evidence: extended 'context fields serialized' covers the new field; full bridge suite PASS; sync_from_src --check in sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(protocol): reply_to_sender + addressed_to join the header vocabulary and defang set Integration review on backend#650 found the gap: the new keys were serialized by this branch but absent from KNOWN_HEADER_KEYS, so (1) consumers read them as body text — the demotion signal was ignored — and (2) a forged body line 'addressed_to: @x' survived confine_user_content verbatim (reproduced before fixing). Listing them promotes trusted-bridge headers AND defangs forged body copies, per the list's own lockstep contract. Evidence: forged-line repro at parent (both keys leak verbatim); after — guard defangs both, task-body-guard 82/82 (incl. 2 new forged-key checks), local-task-protocol read + write suites PASS, bridge suite PASS, vendored sync ✓. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(guards): reply_to_sender + addressed_to join the TS and phone mirrors The Python side promoted both keys as trusted headers while the enforced task-bridge.ts and conversation-server.ts mirrors still let forged body lines with the same names survive undefanged — the exact lockstep gap injection-guard-sweep exists to catch (it failed 2/49 on the prior head). Also condenses three over-cap comment blocks to the two-line bar. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
Owner pain points (reported via web chat):
max-height: 30vh, so anything longer than ~5 lines gets squished into a small scrolling region.fetch(/result/{id})and the existing code doesre.textContent = r.result, which renders### headingsand*lists*as literal text. With the proactive loop now sending long structured replies (skill suggestions, research summaries), this hurts.What's in this PR
New
/chatroutesrc/chat-ui.ts— a self-contained HTML constant (CHAT_HTML) served by web-client.ts at/chat. Full-page chat-first UI in the style of the Gemini/Claude desktop apps:/ssechannelMarkdown rendering on the existing dashboard
<head>re.textContent = r.result→re.innerHTML = window.marked.parse(r.result, {breaks: true, gfm: true})with atextContentfallback if marked failed to load#transcriptmax-height30vh→50vh.t-assistant(h1-h4, p, ul/ol, code, pre, table, etc.)Backend reuse
Both pages use the same
POST /task→ poll/result/{task_id}flow. No new agent endpoints./chatdoes not touch the WebSocket / voice path.Diff stats
src/chat-ui.ts, 399 lines, mostly HTML+CSS)web-client.ts(import, route handler before fall-through, markdown patch in poll callback)Testing
npx tsc --noEmitpassescurl /chatreturns 200 with the new HTML;curl /confirms marked.js script tag presenttextContent)🤖 Generated with Claude Code