fix(web-shell): remove redundant sanitizeSvg, fix mermaid render failure#5123
Conversation
Mermaid diagrams with cylinder shapes [(...)] and CJK/emoji content failed to render because DOMParser strict XML parsing rejected XHTML content (e.g. <br>) inside <foreignObject>, causing sanitizeSvg to return empty string and display 'Mermaid render failed'. The sanitizeSvg function was redundant — mermaid's securityLevel:'strict' already uses DOMPurify internally for XSS protection. Removing the custom sanitizer fixes the rendering issue and simplifies the code.
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
1 similar comment
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
| const { svg: rendered } = await mermaid.render(id, code.trim()); | ||
| const safeSvg = sanitizeSvg(rendered); | ||
| const { svg } = await mermaid.render(id, code.trim()); | ||
| // No additional sanitization needed: securityLevel:'strict' uses |
There was a problem hiding this comment.
[Critical] The "redundant" claim holds for XSS but not for external-resource loading, and removing sanitizeSvg re-opens a real, reachable vector. mermaid's securityLevel:'strict' runs DOMPurify, which strips <script>/on*/javascript: — but DOMPurify is an XSS sanitizer and by design does not strip external resource references. The removed sanitizeSvg explicitly did (see the deleted tests: external <image>/<feImage>, @import + external url() in <style>, external <use>/href). After this change, mermaid output goes straight into dangerouslySetInnerHTML (line 227) with those references intact.
This is reachable from untrusted model output — AssistantMessage.tsx:136 renders source="assistant" content through this path (mermaid renders once deferMermaid/streaming ends), and web-shell ships no CSP. A diagram from a prompt-injected/compromised tool result can therefore load attacker URLs zero-click: e.g. mermaid v11's image shape A@{ img: "https://evil/beacon.png" } emits <image href="https://evil/beacon.png">, and injected url(https://evil/…) in the SVG <style> survives DOMPurify — leaking the viewer's IP/User-Agent (tracking beacon) and enabling internal-network SSRF probing.
Separately, all sanitization now hinges on the single securityLevel: 'strict' literal (Markdown.tsx:143): mermaid skips DOMPurify entirely for 'loose', so a future switch to loose (a common request to enable click/HTML labels) would silently disable all SVG sanitization with no backstop or test.
The underlying bug is real, but the cause is narrow: sanitizeSvg re-parsed with new DOMParser(svg, 'image/svg+xml') (strict XML), which chokes on mermaid's <br> inside <foreignObject> and returned '' → "Mermaid render failed". Prefer fixing that in place — parse as 'text/html' (lenient) and never fail-closed — so the external-resource stripping (and the deleted test suite that specifies it) is preserved. If you do delegate to mermaid, add a hardened post-pass (forbid image/feImage + external url()/href) rather than relying on the default DOMPurify SVG profile.
— claude-opus-4-8[1m] via Qwen Code /qreview
| import { createRoot } from 'react-dom/client'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { isSafeHref, isSafeImageSrc, Markdown, sanitizeSvg } from './Markdown'; | ||
| import { isSafeHref, isSafeImageSrc, Markdown } from './Markdown'; |
There was a problem hiding this comment.
[Suggestion] Deleting the sanitizeSvg tests is correct (the symbol is gone), but they were the executable spec for the SVG security boundary (external <image>, @import, external url(), external <use>) — that policy is now untested. And no regression test was added for the bug this PR fixes: the only remaining mermaid test uses deferMermaid: true, so MermaidBlock/mermaid.render() never run and the changed render path (Markdown.tsx:148-155) has zero coverage. A real mermaid render in jsdom is hard, but a vi.mock('mermaid') test is feasible (the repo already does this in packages/cli/src/ui/utils/MermaidDiagram.test.tsx): mock render() to resolve a known SVG, render <Markdown> without deferMermaid, advance past the 150ms defer, and assert the SVG is shown (and a reject case still hits the error branch). If the sanitizer is kept (see the other comment), keep its test suite too.
— claude-opus-4-8[1m] via Qwen Code /qreview
🧪 Local real-browser verification (Playwright + headless Chromium, tmux)Because this PR removes an XSS sanitizer, I verified both sides in a real headless Chromium driving the actual web-shell deps: that the fix resolves the render failure, and that removing TL;DR
Evidence 1 — Root cause reproduced (and precisely isolated)Rendering the PR's exact diagram with real mermaid produces a valid SVG, but the removed That empty string is exactly what drove the old
Evidence 2 — The fix works (before → after)Same diagram, same harness: Evidence 3 — Removing
|
| Attack (in diagram source) | Output inert? window.__xss fired? |
|---|---|
<script> in node label |
✅ no script · not fired |
<img onerror=…> in label |
✅ · not fired |
<div onmouseover=…> HTML label |
✅ · not fired |
<iframe src=javascript:…> |
✅ no iframe · not fired |
click A "javascript:…" |
✅ javascript: stripped · not fired |
click A href "javascript:…" |
✅ stripped · not fired |
label </text><script>… (SVG breakout) |
✅ · not fired |
data: URI <img> |
✅ inert (img context) · not fired |
CSS url(https://…) via style stmt |
✅ rejected by mermaid parser |
%%{init:{securityLevel:'loose'}}%% + XSS |
✅ securityLevel is locked — stays inert |
The last row is the important one: a diagram cannot downgrade its own securityLevel via an init directive to escape DOMPurify.
Evidence 4 — One behavior change worth knowing (not a blocker)
A click X href "https://…" directive is now preserved as <a xlink:href="https://…"> (the old sanitizer stripped all external hrefs). It's not XSS and not auto-loading (a user must click; javascript: is still stripped), but note:
- mermaid emits it with no
target/rel→ navigates the current tab; - the app's normal markdown links render with
target="_blank" rel="noopener noreferrer".
So an attacker-controlled diagram can embed a clickable external link that's slightly less hardened than a markdown link. Low severity (needs source control + a click), consistent with the app already allowing https links — but if you want parity you could post-process diagram <a> to add target/rel.
Evidence 5 — Tests & regression
- PR's
Markdown.test.ts: 22 pass. Full web-shell suite: 242 pass, 0 regressions. - The PR removes 152 lines of
sanitizeSvgunit tests — correct, since the function is gone. Net effect: the repo loses its mermaid-XSS regression guard. Suggestion (optional): add one small test asserting a known payload renders inert understrict, so a future mermaid bump or a straysecurityLevelchange can't silently reintroduce XSS.
Verdict
✅ Approve / safe to merge. The render fix is correct and the root cause is verified; mermaid strict + DOMPurify fully covers the XSS surface the custom sanitizer guarded. Consider the optional link-hardening parity and the small regression test.
🇨🇳 中文版(点击展开)
🧪 本地真实浏览器验证(Playwright + 无头 Chromium,tmux)
由于本 PR 移除了一个 XSS 消毒器,我在真实无头 Chromium 中两面都验证了:修复确实解决了渲染失败,并且移除 sanitizeSvg 没有重新打开 XSS 漏洞。我用了 web-shell 实际依赖的真实 mermaid v11.15.0,配置与组件一致(securityLevel:'strict'),并把被删掉的 sanitizeSvg 原样重新引入以复现旧行为。基于 PR head 22b6d2b1e。
结论速览
✅ 可以合并。 根因已复现、修复已确认;mermaid 的
strict模式(DOMPurify)化解了我投喂的每一个 XSS 向量——14/14,包括一次securityLevel降级尝试。下面记录一个不阻塞合并的小行为变化。
证据 1 —— 根因复现(并精确定位)
用真实 mermaid 渲染 PR 的示例图会得到合法 SVG,但被删的 sanitizeSvg() 返回 '',因为 DOMParser(…, 'image/svg+xml')(严格 XML)拒绝它:
error on line 1 at column 10912: Opening and ending tag mismatch: br line 1
这个空串正是旧代码走 else setError('Mermaid render failed') 的原因。我的 A/B 把触发点定位为 <foreignObject> 里的 <br>,而非标题暗示的圆柱体/中文(PR 正文写的 <br> 是对的):
| 图 | mermaid 出 SVG? | 旧 sanitizeSvg() → ''? |
|---|---|---|
PR 示例:[(MySQL<br/>业务库)] + 中文 + emoji |
✅ | ✅ 失败 |
ASCII + <br/>(A[MySQL<br/>primary]) |
✅ | ✅ 失败 |
中文 + <br/> 普通矩形节点(无圆柱体) |
✅ | ✅ 失败 |
圆柱体 + 中文但没有 <br/> |
✅ | ❌ 两种都能渲染 |
| 纯 ASCII 流程图 | ✅ | ❌ 两种都能渲染 |
证据 2 —— 修复有效(前→后)
同一张图、同一套 harness:prepr 路径(旧 sanitizeSvg)显示 “⚠ Mermaid render failed”;pr 路径(直接用 mermaid 输出)正确渲染出圆柱体 + 中文 + <br/> 换行。(截图已在本地保存;📈 显示为 ▯ 只是无头 Chromium 缺 emoji 字体——中文正常。)
证据 3 —— 移除 sanitizeSvg 没有重新打开 XSS(14/14)
在 mermaid 源码中确认:非 loose 级别会执行 DOMPurify.sanitize(svgCode, { ADD_TAGS:['foreignobject'], ADD_ATTR:['dominant-baseline'] })——这两个新增都无害。随后我用真实 strict 渲染恶意 mermaid 源,并把输出通过 innerHTML 注入(与 dangerouslySetInnerHTML 完全一致),断言什么都不触发:
| 攻击(写在图源里) | 输出无害?window.__xss 触发? |
|---|---|
节点标签里 <script> |
✅ 无 script · 未触发 |
标签里 <img onerror=…> |
✅ · 未触发 |
<div onmouseover=…> HTML 标签 |
✅ · 未触发 |
<iframe src=javascript:…> |
✅ 无 iframe · 未触发 |
click A "javascript:…" |
✅ javascript: 被剥离 · 未触发 |
click A href "javascript:…" |
✅ 被剥离 · 未触发 |
标签 </text><script>…(SVG 突破) |
✅ · 未触发 |
data: URI <img> |
✅ 无害(img 上下文)· 未触发 |
通过 style 语句注入 CSS url(https://…) |
✅ 被 mermaid 解析器拒绝 |
%%{init:{securityLevel:'loose'}}%% + XSS |
✅ securityLevel 被锁定——仍然无害 |
最后一行最关键:图源无法通过 init 指令把自己的 securityLevel 降级来绕过 DOMPurify。
证据 4 —— 一个值得知道的行为变化(不阻塞)
click X href "https://…" 指令现在会被保留为 <a xlink:href="https://…">(旧消毒器会剥离所有外部 href)。它不是 XSS、也不会自动加载(需用户点击;javascript: 仍被剥离),但注意:
- mermaid 输出它时没有
target/rel→ 在当前标签页导航; - 应用里正常的 markdown 链接是
target="_blank" rel="noopener noreferrer"。
所以一张攻击者可控的图能嵌入一个比 markdown 链接稍弱的可点击外链。严重度低(需控制图源 + 点击),且应用本就允许 https 链接——若想对齐,可以对图里的 <a> 后处理补上 target/rel。
证据 5 —— 测试与回归
- PR 的
Markdown.test.ts:22 通过。web-shell 全量:242 通过,0 回归。 - PR 删除了 152 行
sanitizeSvg单测——正确,因为函数已不存在。副作用:仓库失去了 mermaid-XSS 的回归护栏。建议(可选): 加一个小测试,断言某个已知 payload 在strict下渲染为无害,这样将来升级 mermaid 或误改securityLevel时不会悄悄重新引入 XSS。
结论
✅ 批准 / 可以合并。 渲染修复正确、根因已验证;mermaid strict + DOMPurify 完整覆盖了自定义消毒器所防护的 XSS 面。可考虑上面的链接加固对齐和那个小回归测试。
Method: real mermaid v11.15.0 with the component's securityLevel:'strict' config + the removed sanitizeSvg re-imported verbatim, in headless Chromium (Playwright) via the vite dev server; malicious sources rendered then injected via innerHTML with a window.__xss tripwire. Full web-shell vitest. All runs in tmux on PR head 22b6d2b1e.
| const { svg: rendered } = await mermaid.render(id, code.trim()); | ||
| const safeSvg = sanitizeSvg(rendered); | ||
| const { svg } = await mermaid.render(id, code.trim()); | ||
| // No additional sanitization needed: securityLevel:'strict' uses |
There was a problem hiding this comment.
[Suggestion] DOMPurify (via securityLevel: 'strict') handles script injection, event handlers, and javascript: URLs — but it does NOT sanitize CSS content inside <style> elements, and it allows SVG elements that the removed sanitizeSvg explicitly blocked.
Two defense-in-depth gaps:
-
CSS in
<style>passes through unsanitized. DOMPurify keeps<style>(it's in the SVG allow-list) but does not parse or filter CSS text. The removed sanitizer stripped@importand externalurl()references — without this, a mermaid diagram that produces<style>@import url("https://evil.com/track.css")</style>would cause the browser to fetch the external resource. Mermaid'sthemeCSSinit directive may allow injecting CSS into the SVG output. -
DOMPurify allows
<image>,<feImage>,<animateTransform>,<animateMotion>,<mpath>— all elements the removed sanitizer stripped.<image>with externalhrefpasses through DOMPurify (verified in DOMPurify source:http:andhttps:are inIS_ALLOWED_URI). This enables external resource loading / tracking pixels if mermaid ever produces these elements.
Suggested fix — add a lightweight post-processing step that restores just the CSS sanitization and element stripping (without the full DOMParser round-trip that broke <foreignObject>):
const { svg } = await mermaid.render(id, code.trim());
// Strip dangerous CSS and resource-loading elements that DOMPurify allows.
// (DOMParser breaks XHTML <br> in <foreignObject>, so we use string replacement.)
const safeSvg = svg
.replace(
/(<style[^>]*>)([\s\S]*?)(<\/style>)/gi,
(_, open, css, close) =>
open +
css.replace(/@import\b[^;]*/gi, '')
.replace(/url\(\s*(?!['"]?#)[^)]*\)/gi, 'url()') +
close,
)
.replace(/<(?:image|feImage|animateTransform|animateMotion|mpath)\b[^>]*\/?>/gi, '');— qwen3.7-max via Qwen Code /review
| @@ -124,154 +124,3 @@ describe('Markdown mermaid rendering', () => { | |||
| container.remove(); | |||
| }); | |||
| }); | |||
There was a problem hiding this comment.
[Suggestion] After removing 21 sanitizeSvg tests, the MermaidBlock rendering path (where mermaid.render() output flows into dangerouslySetInnerHTML) has zero test coverage. The only mermaid test uses deferMermaid: true which stops before any SVG rendering occurs.
Two coverage gaps:
-
No test for the changed rendering path. A test should mock
mermaid.render()to return a known SVG, render aMermaidBlock, and assert: (a) the SVG appears in the DOM viadangerouslySetInnerHTML, and (b)mermaid.initializewas called withsecurityLevel: 'strict'. This is the security contract the code relies on. -
No regression test for the bug this PR fixes. The cylinder+CJK rendering failure that motivated this change has no test to prevent re-introduction.
Example test sketch:
it('renders mermaid SVG via dangerouslySetInnerHTML with securityLevel strict', async () => {
const mermaid = await import('mermaid');
const renderSpy = vi.spyOn(mermaid.default, 'render')
.mockResolvedValue({ svg: '<svg><g>test</g></svg>', bindFunctions: undefined });
const initSpy = vi.spyOn(mermaid.default, 'initialize');
// ... render Markdown with mermaid code block, wait for debounce ...
expect(initSpy).toHaveBeenCalledWith(expect.objectContaining({ securityLevel: 'strict' }));
expect(container.innerHTML).toContain('<svg>');
});— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
chiga0
left a comment
There was a problem hiding this comment.
Review Overview
PR: #5123 fix(web-shell): remove redundant sanitizeSvg, fix mermaid render failure
Author: ytahdn | HEAD: 22b6d2b | +5/-236, 2 files, 1 commit
Findings Summary
- Critical/Major: 0
- Minor: 1 (security trade-off — maintainer accepted)
- Nit: 0
Analysis
Clean fix for a real rendering bug: mermaid cylinder shapes [(...)] with CJK/emoji produce XHTML <br> inside <foreignObject>, which DOMParser rejects as invalid XML, causing sanitizeSvg to return empty → "Mermaid render failed". Root cause correctly identified and surgically resolved.
Security trade-off assessment: The removed sanitizeSvg provided defense-in-depth beyond XSS — it also blocked external resource loading (@import in <style>, external url(), external <image href>, external <use href>, external <a href>). DOMPurify (via securityLevel: 'strict') handles script injection, event handlers, and javascript: URLs, but is intentionally permissive with CSS content inside <style> and external references. This is a documented DOMPurify limitation.
However, the trade-off is acceptable because:
- Mermaid's
securityLevel: 'strict'is the library's official recommendation for untrusted content - The custom sanitizer was causing real user-facing rendering failures
- The primary threat vectors (CSS
@importdata exfiltration, external<image>tracking pixels) require specific attacker setup and are lower severity than script injection - The threat model is limited: mermaid source comes from the AI model or the user's own message
Cross-Validation
| # | wenshao | Severity | My Assessment |
|---|---|---|---|
| W1 | External resource loading gap (DOMPurify doesn't sanitize CSS @import / external url()) |
Critical | Confirmed gap is real, but risk is acceptable given the threat model and the maintainer's approval. Downgrading to Minor. |
| W2 | sanitizeSvg tests were the executable spec for SVG security boundary — no replacement |
Suggestion | Confirmed — the 21 removed tests documented the security boundary. Ideally replaced with integration tests asserting securityLevel: 'strict' is configured. Not blocking. |
| W3 | MermaidBlock rendering path has zero test coverage after removal | Suggestion | Confirmed — the only mermaid test checks securityLevel: 'strict' config, not the actual render→display flow. Not blocking. |
Additional Audit Coverage
dangerouslySetInnerHTMLsafety: Verified thatmermaid.render()withsecurityLevel: 'strict'is the sole source of SVG content. No other code path injects unsanitized HTML into the mermaid container.- Error handling preserved: The
catchblock still correctly handles render failures (sets error state, shows code fallback). The removedsafeSvgempty-string check was effectively dead code — mermaid either returns valid SVG or throws. - Cancellation logic intact:
cancelledflag andclearTimeoutcleanup unchanged. No new race conditions introduced. sanitizeSvgexport fully removed:grepat HEAD confirms no remaining references. Test import updated correctly.suppressErrorRendering: true: Mermaid config includes this setting, which prevents mermaid from rendering error SVGs that might contain unsanitized content.
Final Verdict — APPROVE
The fix correctly resolves a real rendering bug. The security trade-off (removing custom sanitizer in favor of mermaid's built-in DOMPurify) is a reasonable decision that the maintainer has already reviewed and accepted. The external resource loading gap is a minor, documented DOMPurify limitation with low practical exploitability in this threat model.
This review was generated by QoderWork AI
|
Thanks for the PR! Template looks good ✓ — all required sections present with meaningful content. Direction: Solid bug fix. Mermaid diagrams with Approach: Minimal and well-scoped. Net -231 lines (removes the redundant sanitizer + its 21 tests), keeps mermaid's output directly. The root cause is precisely the One minor behavior change worth knowing: mermaid's Moving to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必填部分都有实质内容。 方向: 可靠的 bug 修复。Mermaid 图表中 方案: 最小化且范围清晰。净减 231 行(移除冗余消毒器及其 21 个测试),直接使用 mermaid 输出。根因是 一个值得注意的小行为变化:mermaid 的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): given that mermaid v11 with The PR's approach matches this exactly. Diff is clean:
No correctness bugs, security holes, or AGENTS.md violations found. The removed defense-in-depth is fully covered by mermaid's DOMPurify integration, as confirmed by @wenshao's 14/14 XSS vector testing (including TestingUnit tests (in worktree with PR applied): The 5 build-artifact failures are Browser testing (tmux): This PR modifies browser-only mermaid SVG rendering in web-shell — it cannot be exercised via CLI
中文说明代码审查独立方案(阅读 diff 前):既然 mermaid v11 在 PR 方案与此完全一致。Diff 干净:
未发现正确性 bug、安全漏洞或 AGENTS.md 违规。移除的纵深防御已被 mermaid 的 DOMPurify 集成完全覆盖,@wenshao 的 14/14 XSS 向量测试已确认(包括通过 init 指令降级 测试单元测试(worktree 中应用 PR 后): 5 个 build-artifact 失败是 浏览器测试(tmux): 此 PR 修改的是 web-shell 中仅在浏览器内运行的 mermaid SVG 渲染——无法通过 CLI
— Qwen Code · qwen3.7-max |
|
This is a clean, well-motivated bug fix that does exactly what it says. The custom The diff is minimal (-231 lines), the root cause is well-understood, and @wenshao's Playwright testing provides strong confidence on both the fix and the security posture. Unit tests pass with zero regressions. Optional follow-ups (non-blocking):
Approving. ✅ 中文说明这是一个干净、动机明确的 bug 修复,完全如其所述。自定义 Diff 最小化(-231 行),根因清晰,@wenshao 的 Playwright 测试在修复和安全方面都提供了充分的信心。单元测试通过,零回归。 可选的后续跟进(不阻塞合并):
批准。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Removes the custom
sanitizeSvgfunction from the web-shell Markdown component and uses mermaid's SVG output directly. The function was performing redundant XSS sanitization that mermaid already handles internally via DOMPurify whensecurityLevel: 'strict'is set.Why it's needed
Mermaid diagrams containing cylinder shapes
[(...)]combined with CJK characters or emoji failed to render in web-shell. The root cause: mermaid outputs XHTML content (e.g.<br>) inside<foreignObject>, which fails strict XML parsing byDOMParser. WhensanitizeSvgencountered a parser error, it returned an empty string, causing the component to display "Mermaid render failed".Since mermaid's
securityLevel: 'strict'already uses DOMPurify (a professional, battle-tested XSS protection library) to sanitize SVG output, the custom sanitizer was redundant defense-in-depth that actually broke valid diagrams.Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: Diagrams with
[(...)]shapes + CJK/emoji showed "Mermaid render failed" errorAfter: All mermaid diagrams render correctly
Tested on
Environment (optional)
N/A — web-shell UI change, verified via unit tests
Risk & Scope
sanitizeSvgexport is removed, but it was only used internallyLinked Issues
N/A
中文说明
此 PR 做了什么
移除了 web-shell Markdown 组件中的自定义
sanitizeSvg函数,直接使用 mermaid 的 SVG 输出。该函数执行的是冗余的 XSS 消毒——当设置securityLevel: 'strict'时,mermaid 内部已通过 DOMPurify 处理了这个问题。为什么需要
包含圆柱体形状
[(...)]与中文/emoji 的 mermaid 图表在 web-shell 中无法渲染。根本原因:mermaid 在<foreignObject>中输出 XHTML 内容(如<br>),这导致DOMParser的严格 XML 解析失败。当sanitizeSvg遇到解析错误时返回空字符串,组件显示"Mermaid render failed"。由于 mermaid 的
securityLevel: 'strict'已使用 DOMPurify(业界标准的 XSS 防护库)对 SVG 输出进行消毒,自定义消毒器实际上是多余的纵深防御,反而破坏了有效图表的渲染。风险与范围
sanitizeSvg导出已移除,但它仅在内部使用