From f9927f01687f40eb0b77b9c98308a9dc1b05898c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 15 Mar 2026 18:40:42 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=91=20fix:=20Sanitize=20Markdown=20Art?= =?UTF-8?q?ifacts=20(#12249)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🛡️ fix: Sanitize markdown artifact rendering to prevent stored XSS Replace marked-react with react-markdown + remark-gfm for artifact markdown preview. react-markdown's skipHtml strips raw HTML tags, and a urlTransform guard blocks javascript: and data: protocol links. * fix: Update useArtifactProps test to expect react-markdown dependencies * fix: Harden markdown artifact sanitization - Convert isSafeUrl from denylist to allowlist (http, https, mailto, tel plus relative/anchor URLs); unknown protocols are now fail-closed - Add remark-breaks to restore single-newline-to-
behavior that was silently dropped when replacing marked-react - Export isSafeUrl from the host module and add 16 direct unit tests covering allowed protocols, blocked schemes (javascript, data, blob, vbscript, file, custom), edge cases (empty, whitespace, mixed case) - Hoist remarkPlugins to a module-level constant to avoid per-render array allocation in the generated Sandpack component - Fix import order in generated template (shortest to longest per AGENTS.md) and remove pre-existing trailing whitespace * fix: Return null for blocked URLs, add sync-guard comments and test - urlTransform returns null (not '') for blocked URLs so react-markdown omits the href/src attribute entirely instead of producing - Hoist urlTransform to module-level constant alongside remarkPlugins - Add JSDoc sync-guard comments tying the exported isSafeUrl to its template-string mirror, so future maintainers know to update both - Add synchronization test asserting the embedded isSafeUrl contains the same allowlist set, URL parsing, and relative-path checks as the export --- .../__tests__/useArtifactProps.test.ts | 6 +- client/src/utils/__tests__/markdown.test.ts | 96 +++++++++++++++++-- client/src/utils/artifacts.ts | 4 +- client/src/utils/markdown.ts | 55 ++++++++++- 4 files changed, 148 insertions(+), 13 deletions(-) diff --git a/client/src/hooks/Artifacts/__tests__/useArtifactProps.test.ts b/client/src/hooks/Artifacts/__tests__/useArtifactProps.test.ts index f9f29e0c56..e46a285c50 100644 --- a/client/src/hooks/Artifacts/__tests__/useArtifactProps.test.ts +++ b/client/src/hooks/Artifacts/__tests__/useArtifactProps.test.ts @@ -112,7 +112,7 @@ describe('useArtifactProps', () => { expect(result.current.files['content.md']).toBe('# No content provided'); }); - it('should provide marked-react dependency', () => { + it('should provide react-markdown dependency', () => { const artifact = createArtifact({ type: 'text/markdown', content: '# Test', @@ -120,7 +120,9 @@ describe('useArtifactProps', () => { const { result } = renderHook(() => useArtifactProps({ artifact })); - expect(result.current.sharedProps.customSetup?.dependencies).toHaveProperty('marked-react'); + expect(result.current.sharedProps.customSetup?.dependencies).toHaveProperty('react-markdown'); + expect(result.current.sharedProps.customSetup?.dependencies).toHaveProperty('remark-gfm'); + expect(result.current.sharedProps.customSetup?.dependencies).toHaveProperty('remark-breaks'); }); it('should update files when content changes', () => { diff --git a/client/src/utils/__tests__/markdown.test.ts b/client/src/utils/__tests__/markdown.test.ts index fcc0f169e6..9734e0e18a 100644 --- a/client/src/utils/__tests__/markdown.test.ts +++ b/client/src/utils/__tests__/markdown.test.ts @@ -1,4 +1,72 @@ -import { getMarkdownFiles } from '../markdown'; +import { isSafeUrl, getMarkdownFiles } from '../markdown'; + +describe('isSafeUrl', () => { + it('allows https URLs', () => { + expect(isSafeUrl('https://example.com')).toBe(true); + }); + + it('allows http URLs', () => { + expect(isSafeUrl('http://example.com/path')).toBe(true); + }); + + it('allows mailto links', () => { + expect(isSafeUrl('mailto:user@example.com')).toBe(true); + }); + + it('allows tel links', () => { + expect(isSafeUrl('tel:+1234567890')).toBe(true); + }); + + it('allows relative paths', () => { + expect(isSafeUrl('/path/to/page')).toBe(true); + expect(isSafeUrl('./relative')).toBe(true); + expect(isSafeUrl('../parent')).toBe(true); + }); + + it('allows anchor links', () => { + expect(isSafeUrl('#section')).toBe(true); + }); + + it('blocks javascript: protocol', () => { + expect(isSafeUrl('javascript:alert(1)')).toBe(false); + }); + + it('blocks javascript: with leading whitespace', () => { + expect(isSafeUrl(' javascript:alert(1)')).toBe(false); + }); + + it('blocks javascript: with mixed case', () => { + expect(isSafeUrl('JavaScript:alert(1)')).toBe(false); + }); + + it('blocks data: protocol', () => { + expect(isSafeUrl('data:text/html,x')).toBe(false); + }); + + it('blocks blob: protocol', () => { + expect(isSafeUrl('blob:http://example.com/uuid')).toBe(false); + }); + + it('blocks vbscript: protocol', () => { + expect(isSafeUrl('vbscript:MsgBox("xss")')).toBe(false); + }); + + it('blocks file: protocol', () => { + expect(isSafeUrl('file:///etc/passwd')).toBe(false); + }); + + it('blocks empty strings', () => { + expect(isSafeUrl('')).toBe(false); + }); + + it('blocks whitespace-only strings', () => { + expect(isSafeUrl(' ')).toBe(false); + }); + + it('blocks unknown/custom protocols', () => { + expect(isSafeUrl('custom:payload')).toBe(false); + }); +}); describe('markdown artifacts', () => { describe('getMarkdownFiles', () => { @@ -41,7 +109,7 @@ describe('markdown artifacts', () => { const markdown = '# Test'; const files = getMarkdownFiles(markdown); - expect(files['/components/ui/MarkdownRenderer.tsx']).toContain('import Markdown from'); + expect(files['/components/ui/MarkdownRenderer.tsx']).toContain('import ReactMarkdown from'); expect(files['/components/ui/MarkdownRenderer.tsx']).toContain('MarkdownRendererProps'); expect(files['/components/ui/MarkdownRenderer.tsx']).toContain( 'export default MarkdownRenderer', @@ -162,13 +230,29 @@ describe('markdown artifacts', () => { }); describe('markdown component structure', () => { - it('should generate a MarkdownRenderer component that uses marked-react', () => { + it('should generate a MarkdownRenderer component with safe markdown rendering', () => { const files = getMarkdownFiles('# Test'); const rendererCode = files['/components/ui/MarkdownRenderer.tsx']; - // Verify the component imports and uses Markdown from marked-react - expect(rendererCode).toContain("import Markdown from 'marked-react'"); - expect(rendererCode).toContain('{content}'); + expect(rendererCode).toContain("import ReactMarkdown from 'react-markdown'"); + expect(rendererCode).toContain("import remarkBreaks from 'remark-breaks'"); + expect(rendererCode).toContain('skipHtml={true}'); + expect(rendererCode).toContain('SAFE_PROTOCOLS'); + expect(rendererCode).toContain('isSafeUrl'); + expect(rendererCode).toContain('urlTransform={urlTransform}'); + expect(rendererCode).toContain('remarkPlugins={remarkPlugins}'); + expect(rendererCode).toContain('isSafeUrl(url) ? url : null'); + }); + + it('should embed isSafeUrl logic matching the exported version', () => { + const files = getMarkdownFiles('# Test'); + const rendererCode = files['/components/ui/MarkdownRenderer.tsx']; + + expect(rendererCode).toContain("new Set(['http:', 'https:', 'mailto:', 'tel:'])"); + expect(rendererCode).toContain('new URL(trimmed).protocol'); + expect(rendererCode).toContain("trimmed.startsWith('/')"); + expect(rendererCode).toContain("trimmed.startsWith('#')"); + expect(rendererCode).toContain("trimmed.startsWith('.')"); }); it('should pass markdown content to the Markdown component', () => { diff --git a/client/src/utils/artifacts.ts b/client/src/utils/artifacts.ts index 13f3a23b47..e862d18a40 100644 --- a/client/src/utils/artifacts.ts +++ b/client/src/utils/artifacts.ts @@ -108,7 +108,9 @@ const mermaidDependencies = { }; const markdownDependencies = { - 'marked-react': '^2.0.0', + 'remark-gfm': '^4.0.0', + 'remark-breaks': '^4.0.0', + 'react-markdown': '^9.0.1', }; const dependenciesMap: Record< diff --git a/client/src/utils/markdown.ts b/client/src/utils/markdown.ts index 12556c1a24..24d5105863 100644 --- a/client/src/utils/markdown.ts +++ b/client/src/utils/markdown.ts @@ -1,23 +1,70 @@ import dedent from 'dedent'; -const markdownRenderer = dedent(`import React, { useEffect, useState } from 'react'; -import Markdown from 'marked-react'; +const SAFE_PROTOCOLS = new Set(['http:', 'https:', 'mailto:', 'tel:']); + +/** + * Allowlist-based URL validator for markdown artifact rendering. + * Mirrored verbatim in the markdownRenderer template string below — + * any logic change MUST be applied to both copies. + */ +export const isSafeUrl = (url: string): boolean => { + const trimmed = url.trim(); + if (!trimmed) { + return false; + } + if (trimmed.startsWith('/') || trimmed.startsWith('#') || trimmed.startsWith('.')) { + return true; + } + try { + return SAFE_PROTOCOLS.has(new URL(trimmed).protocol); + } catch { + return false; + } +}; + +const markdownRenderer = dedent(`import React from 'react'; +import remarkGfm from 'remark-gfm'; +import remarkBreaks from 'remark-breaks'; +import ReactMarkdown from 'react-markdown'; interface MarkdownRendererProps { content: string; } +/** Mirror of the exported isSafeUrl in markdown.ts — keep in sync. */ +const SAFE_PROTOCOLS = new Set(['http:', 'https:', 'mailto:', 'tel:']); + +const isSafeUrl = (url: string): boolean => { + const trimmed = url.trim(); + if (!trimmed) return false; + if (trimmed.startsWith('/') || trimmed.startsWith('#') || trimmed.startsWith('.')) return true; + try { + return SAFE_PROTOCOLS.has(new URL(trimmed).protocol); + } catch { + return false; + } +}; + +const remarkPlugins = [remarkGfm, remarkBreaks]; +const urlTransform = (url: string) => (isSafeUrl(url) ? url : null); + const MarkdownRenderer: React.FC = ({ content }) => { return (
- {content} + + {content} +
); };