From 64505ac25cb578eecce4d7b41059cba1e818500a Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 21 May 2026 16:23:12 +0100 Subject: [PATCH] Tighten DOMPurify config in LLM markdown renderer Two hardening adjustments to renderMarkdown in aiChatUtils.ts: - Pin ALLOWED_URI_REGEXP to ^(?:https?:|mailto:). The previous config left href scheme filtering to DOMPurify's library defaults, which strips javascript:/data: in current versions but is version- dependent. Making the allowlist explicit means the safety property survives a future DOMPurify upgrade that loosens the internal default. - Drop 'class' from ALLOWED_ATTR. The LLM has no legitimate reason to apply arbitrary CSS classes, and allowing them on sanitized markup is a UI-redress surface: an attacker who can influence the chat output (prompt injection, compromised model endpoint) could inject something like
to overlay the page or hide content. Also expanded the marked.setOptions comment to document that the sanitizer is the real XSS gate but marked options are kept tight as defence-in-depth: a future DOMPurify-config loosening shouldn't mean script tags from the LLM hit the DOM. Regression tests added in aiChatUtils.test.ts: - javascript: hrefs stripped - data: hrefs stripped - class attribute stripped from
- https links preserved with target=_blank rel=noopener noreferrer --- .../AI/__tests__/aiChatUtils.test.ts | 36 +++++++++++++++++++ .../src/components/AI/aiChatUtils.ts | 29 +++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/frontend-modern/src/components/AI/__tests__/aiChatUtils.test.ts b/frontend-modern/src/components/AI/__tests__/aiChatUtils.test.ts index e96e89ae66..28b26d2f87 100644 --- a/frontend-modern/src/components/AI/__tests__/aiChatUtils.test.ts +++ b/frontend-modern/src/components/AI/__tests__/aiChatUtils.test.ts @@ -105,6 +105,42 @@ describe('aiChatUtils', () => { spy.mockRestore(); }); + + // Regression: drop `javascript:` href from LLM-supplied markdown links. + // Relies on the explicit ALLOWED_URI_REGEXP pin so the behaviour survives + // future DOMPurify upgrades that might relax internal scheme filtering. + it('strips javascript: hrefs from links', () => { + const output = utils.renderMarkdown('[click](javascript:alert(1))'); + expect(output).not.toContain('javascript:'); + expect(output).not.toContain('alert(1)'); + }); + + // Regression: data: URIs are not in the allowlist either; including SVG + // data URIs which some browsers will execute inline script from. + it('strips data: hrefs from links', () => { + const output = utils.renderMarkdown('[svg](data:image/svg+xml,)'); + expect(output).not.toContain('data:'); + }); + + // Regression: the LLM has no legitimate reason to apply arbitrary CSS + // classes — used to be ALLOWED_ATTR; allowing them opens a UI-redress + // surface (overlay attacks, hidden text, etc.). + it('strips class attribute from LLM-supplied markup', () => { + const output = utils.renderMarkdown( + '
overlay
', + ); + expect(output).not.toContain('class="fixed'); + expect(output).not.toContain('inset-0'); + }); + + // Regression: real http/https links still render and pick up the safe + // target/rel attributes from the afterSanitizeAttributes hook. + it('preserves https links and applies target/rel', () => { + const output = utils.renderMarkdown('[ok](https://example.com)'); + expect(output).toContain('href="https://example.com"'); + expect(output).toContain('target="_blank"'); + expect(output).toContain('rel="noopener noreferrer"'); + }); }); describe('Mention ID format contracts', () => { diff --git a/frontend-modern/src/components/AI/aiChatUtils.ts b/frontend-modern/src/components/AI/aiChatUtils.ts index da6bc964d1..e4c356c51e 100644 --- a/frontend-modern/src/components/AI/aiChatUtils.ts +++ b/frontend-modern/src/components/AI/aiChatUtils.ts @@ -18,7 +18,15 @@ export function groupModelsByProvider(models: ModelInfo[]): Map gfm: true, // GitHub Flavored Markdown @@ -59,7 +67,21 @@ export const renderMarkdown = (content: unknown): string => { try { configureDOMPurify(); const rawHtml = marked.parse(normalized) as string; - // Sanitize to prevent XSS from malicious LLM output or injected content + // Sanitize to prevent XSS from malicious LLM output or injected content. + // + // Hardening notes: + // - ALLOWED_URI_REGEXP pins href values to http/https/mailto. Without + // this we rely on DOMPurify's internal default URI scheme filter, + // which strips javascript:/data: in current versions but is library- + // version dependent; an explicit allowlist regex makes the intent + // survive future updates. + // - 'class' was previously in ALLOWED_ATTR. The LLM has no legitimate + // reason to apply utility classes, and allowing arbitrary classes + // opens a UI-redressing surface (e.g. injecting "fixed inset-0 z-50 + // bg-black" to overlay the page). Drop it. + // - 'target' and 'rel' are not in ALLOWED_ATTR because the + // afterSanitizeAttributes hook above sets them on every ; they + // don't need to be parsed in from the LLM-supplied HTML. return DOMPurify.sanitize(rawHtml, { // Allow common formatting tags but block scripts, iframes, etc. ALLOWED_TAGS: [ @@ -93,7 +115,8 @@ export const renderMarkdown = (content: unknown): string => { 'span', 'div', ], - ALLOWED_ATTR: ['href', 'target', 'rel', 'class'], + ALLOWED_ATTR: ['href'], + ALLOWED_URI_REGEXP: /^(?:https?:|mailto:)/i, // Force all links to open in new tab and prevent opener attacks ADD_ATTR: ['target', 'rel'], });