fix: render markdown in chat assistant messages (#518)

[Chris0Jeky]

Summary

• Adds marked + DOMPurify dependencies for markdown-to-HTML rendering
• Assistant and System messages in Automation Chat now render formatted markdown (headings, lists, code blocks, bold/italic, etc.)
• User messages remain as plaintext {{ }} interpolation — they never touch v-html

Fix

• renderMarkdown(content) calls marked.parse() (sync string in v17) then DOMPurify.sanitize() before binding with v-html
• isAssistantOrSystemMessage() uses the existing normalizeChatRole() helper to detect non-user roles — handles both numeric and string role values from the backend
• Scoped CSS added for .td-message-content--markdown to style paragraphs, headings, lists, code blocks, blockquotes, and links in both light/dark themes via CSS custom properties

Security Note

• DOMPurify sanitization is applied before v-html on every render — no unsanitized content can reach the DOM
• User input path (message.role === 'User') uses {{ message.content }} (Vue's auto-escaped text interpolation) — completely separate from v-html
• No other v-html introduction in this diff

Closes

Closes #518

Risk

Low — v-html only used for assistant/system messages after DOMPurify sanitization; user message path is unchanged.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Self-Review (Adversarial Pass)

Security checks:

• DOMPurify applied before v-html: yes — renderMarkdown calls DOMPurify.sanitize(marked.parse(content) as string) and the return value is what v-html binds. There is no path where unsanitized content reaches v-html.
• User message path avoids v-html: yes — user messages use {{ message.content }} (Vue's auto-escaped text interpolation). The v-else branch never touches v-html.
• No other v-html introduction without sanitization: yes — this diff adds exactly one v-html binding, and it is gated by isAssistantOrSystemMessage() and goes through renderMarkdown().

Correctness checks:

• marked.parse() returns sync string: yes — verified with node -e against installed v17.0.5. The as string cast is correct; the async overload is not used here.
• Empty/null content handled: yes — renderMarkdown returns '' early when content is falsy, so no crash on empty messages.
• Dark/light theme markdown styles work: yes — all CSS uses var(--td-*) design tokens (surface, border, text, color-primary) that already respond to the theme. Code blocks use var(--td-surface-tertiary) and var(--td-border-default). No hardcoded colors were added.

Additional observations:

• isAssistantOrSystemMessage delegates to the existing normalizeChatRole() utility which handles both numeric (backend enum integer) and string role values — no parallel role-detection logic introduced.
• white-space: pre-wrap is removed for the markdown variant (.td-message-content--markdown) to avoid double-rendering of whitespace already structured by the HTML parser. Plain user messages retain pre-wrap as before.
• No risk of rendered <script> tags: DOMPurify strips them by default.

[gemini-code-assist]

Code Review

This pull request implements markdown rendering for assistant and system messages in the automation chat using the marked and dompurify libraries. The changes include a new rendering utility, template updates to support HTML output, and detailed CSS for markdown components. The review feedback identifies an opportunity to improve robustness by adding error handling to the markdown parsing logic.

[gemini-code-assist]

For improved robustness, it's a good practice to wrap the markdown parsing and sanitization logic in a try...catch block. This will prevent a single malformed message from the assistant from breaking the entire chat message display. If an error occurs, we can log it and display a safe, user-friendly error message.

function renderMarkdown(content: string): string {
  if (!content) {
    return ''
  }
  try {
    return DOMPurify.sanitize(marked.parse(content) as string)
  } catch (e) {
    console.error('Failed to render markdown content:', e)
    return '<p><em>Error rendering message content.</em></p>'
  }
}

[Chris0Jeky]

Adversarial Security Review (Independent Pass)

Verdict: REQUEST CHANGES

Two issues require attention before merge: one MAJOR (architectural risk worth fixing now) and two MINORs. No outright XSS blocker found, but the MAJOR is close enough to one that it should be corrected.

---

Findings

MAJOR — normalizeChatRole falls back to 'Assistant' for any unknown/invalid role

File: frontend/taskdeck-web/src/utils/chat.ts, isAssistantOrSystemMessage()

normalizeChatRole has this fallback:

return chatRoleByIndex[value] ?? 'Assistant'   // numeric out-of-range
return found ?? 'Assistant'                    // unknown string

So a message with role: 99, role: "hacker", or any future role string not in the known list will normalize to 'Assistant' and route into v-html + renderMarkdown(). If the backend ever stores or relays a malformed role value (e.g., from a compromised LLM response that injects into message metadata), a user-authored message body would hit v-html.

In practice DOMPurify is still the last line of defence, so this is not a live XSS today — but it's a logic inversion: unknown roles should fail-closed to the safe {{ }} path, not fail-open to v-html. The fix is a one-liner:

// utils/chat.ts  — change the fallback in normalizeChatRole from 'Assistant' to 'User'
return chatRoleByIndex[value] ?? 'User'
return found ?? 'User'

Or, alternatively, gate isAssistantOrSystemMessage with an explicit allowlist check without relying on the shared helper's fallback:

function isAssistantOrSystemMessage(message: ChatMessage): boolean {
  const role = normalizeChatRole(message.role)
  return role === 'Assistant' || role === 'System'
}

This is already correctly written — the problem is purely in the upstream helper's default. Change the fallback default to 'User' (the safe path), since any message of unknown origin should be treated as user-generated.

MINOR — marked.parse() type is string | Promise<string>; the as string cast hides a correctness risk

File: AutomationChatView.vue, line 27:

return DOMPurify.sanitize(marked.parse(content) as string)

The installed marked@17.0.5 type declaration at line 638 of marked.d.ts reads:

(src: string, options?: MarkedOptions | null): string | Promise<string>;

Without explicit { async: false } in the options object, TypeScript cannot prove the return is synchronous — which is why the as string cast was necessary to silence the compiler. In the default configuration (no walkTokens extension), marked.parse() is synchronous at runtime, but this is a fragile cast that:

1. Silences a real type warning without fixing the cause.
2. Would silently break if a marked.use() extension with an async walkTokens is ever added — the v-html binding would render [object Promise] instead of sanitized HTML.

Recommended fix — pin the sync overload explicitly:

import { marked, type MarkedOptions } from 'marked'

const SYNC_OPTIONS: MarkedOptions & { async: false } = { async: false }

function renderMarkdown(content: string): string {
  if (!content) return ''
  return DOMPurify.sanitize(marked.parse(content, SYNC_OPTIONS))
}

This removes the as string cast, the return type becomes string, and TypeScript enforces the contract going forward.

MINOR — Rendered <a> links open in the same tab; no rel="noopener noreferrer" on DOMPurify output

The CSS adds color and underline for links but the DOMPurify default configuration allows <a href="..."> without restricting target. More importantly, DOMPurify by default strips target entirely, which means links open in the same tab — navigating the user away from the app. It does not add rel="noopener noreferrer" either.

This is not an XSS vector (DOMPurify correctly blocks javascript: hrefs), but it is a UX/security-hygiene issue: assistant-generated links should open in a new tab with noopener noreferrer. Fix with a DOMPurify hook:

DOMPurify.addHook('afterSanitizeAttributes', (node) => {
  if (node.tagName === 'A') {
    node.setAttribute('target', '_blank')
    node.setAttribute('rel', 'noopener noreferrer')
  }
})

Or configure this once at module scope rather than per-call.

---

Security checks

Check	Result
DOMPurify applied to HTML output (not markdown input)	pass — DOMPurify.sanitize(marked.parse(content) as string) correctly sanitizes the rendered HTML, not the raw markdown
User message content never reaches v-html	conditional pass — direct role: 'User' path uses {{ }} correctly, but normalizeChatRole fallback default routes unknown roles to v-html (see MAJOR above)
marked.parse() returns sync string (not Promise) at runtime	pass at runtime — marked@17.0.5 without async extensions is synchronous; however the as string cast suppresses a valid type warning (see MINOR)
No CSP violation introduced	pass — the backend CSP is script-src 'self' with no unsafe-eval; v-html injecting static HTML does not require script execution and DOMPurify strips event handlers and <script> tags

Correctness checks

Check	Result
Role detection correctly gates v-html	fail — normalizeChatRole fallback is 'Assistant' not 'User'; unknown roles silently enter the v-html path
null/undefined content handled	pass — if (!content) return '' guards both null and empty string
Markdown CSS present for dark/light themes	pass — all selectors use var(--td-*) design tokens; both light and dark themes covered via the same custom property system

Build

Check	Result
TypeScript clean	pass — vue-tsc --noEmit exits 0 with no errors (though the as string cast is what makes this pass; see MINOR above)

---

Summary

The core XSS chain (user input → v-html) is correctly blocked: user messages use {{ }}, and DOMPurify runs after marked.parse(). The MAJOR issue is that the role-gating logic has a fail-open fallback that could in theory route a message with an unexpected role through v-html. Fix: change normalizeChatRole's default fallback from 'Assistant' to 'User'. The two MINORs (sync cast, link target) are worth addressing but are not blockers for initial merge if the MAJOR is fixed.

[Chris0Jeky]

Follow-up: MAJOR and MINOR fixes pushed

Commit e01cc9cf addresses the two highest-priority findings from the review above.

Fix 1 — MAJOR: fail-closed role fallback (utils/chat.ts)

normalizeChatRole now defaults to 'User' instead of 'Assistant' for any unrecognised role value (unknown string or out-of-range numeric). Unknown-role messages now route to the safe {{ }} text-interpolation path rather than the v-html path.

Fix 2 — MINOR: remove as string cast (AutomationChatView.vue)

marked.parse(content) replaced with marked.parse(content, { async: false }), which uses the synchronous overload directly. The return type is now string without a suppressing cast, so TypeScript will catch it if an async extension is ever added.

Verification

• vue-tsc --noEmit: clean
• vitest --run: 120 test files, 1102 tests — all pass

Still open (MINOR, non-blocking)

The link target/rel issue noted in the review (rendered <a> tags open in the same tab with no noopener noreferrer) remains. Recommend a follow-up issue or a DOMPurify afterSanitizeAttributes hook to add target="_blank" rel="noopener noreferrer" to all links in assistant messages.

[Chris0Jeky]

Second Adversarial Review (Post-Fix Pass)

Verdict: APPROVE

Both fixes from the first review are correctly applied (commit e01cc9cf). The PR is ready to merge.

---

Previous fixes verified

1. normalizeChatRole fallback → 'User': verified correct — both the numeric out-of-range branch (chatRoleByIndex[value] ?? 'User') and the unknown-string branch (found ?? 'User') now fail-closed to the safe {{ }} path. Any message with a role not in ['User', 'Assistant', 'System'] is treated as user content and never reaches v-html.

2. marked.parse sync guarantee: verified — marked.parse(content, { async: false }) is now used. Runtime check node -e "const {marked} = require('marked'); console.log(typeof marked.parse('**hi**', {async:false}))" returns string. The as string cast is gone; TypeScript enforces the sync overload directly. No [object Promise] risk.

---

Security checks

Check	Result
All v-html bindings traced — user content isolated	pass — exactly one v-html in all of src/, gated by isAssistantOrSystemMessage() and routed through renderMarkdown()
DOMPurify applied to HTML output before v-html	pass — DOMPurify.sanitize(marked.parse(content, { async: false })) runs before binding
null/undefined content handled	pass — if (!content) return '' early-return guards falsy content
No new XSS surface introduced	pass — DOMPurify strips <script>, event handlers, and javascript: hrefs by default; user messages use {{ }} auto-escaping
Unknown/future roles route to safe path	pass — fail-closed fallback confirmed; `ChatRoleValue = ChatRole

Build

Check	Result
TypeScript clean (vue-tsc -b)	pass — exits 0, no errors
Tests pass (vitest --run)	pass — 120 test files, 1102 tests, all green

---

Remaining open item (non-blocking, tracked from first review)

Rendered <a> tags in assistant markdown open in the same tab and lack rel="noopener noreferrer". DOMPurify strips target by default — no XSS, but assistant-generated links navigate users away from the app. A follow-up issue with a DOMPurify afterSanitizeAttributes hook is the right fix; this does not block merge.

---

Additional findings

None found. The diff is minimal, the logic is correct, and the security boundary is clean.