Skip to content

Commit 9f47c14

Browse files
committed
fix(uploads): match internal file marker in URL path only
isInternalFileUrl matched the /api/files/serve/ substring anywhere in the string, so a crafted URL could carry it in a query string or fragment and skip DNS/SSRF validation. Match it in the path component only. The raw path is checked without URL normalization on purpose: the files parse route relies on traversal sequences surviving this check (an absolute https://host/api/files/serve/../.. URL must classify as internal so the '..' check rejects it, rather than being normalized to /etc/... and waved through as external). Host is intentionally not gated — cross-tenant reads are prevented at the storage sink by verifyFileAccess, and host-allowlisting would break self-hosted/multi-domain deployments. Adds unit tests.
1 parent a4bba94 commit 9f47c14

2 files changed

Lines changed: 61 additions & 3 deletions

File tree

apps/sim/lib/uploads/utils/file-utils.test.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,42 @@
22
* @vitest-environment node
33
*/
44
import { describe, expect, it } from 'vitest'
5-
import { isAbortError, isNetworkError } from '@/lib/uploads/utils/file-utils'
5+
import { isAbortError, isInternalFileUrl, isNetworkError } from '@/lib/uploads/utils/file-utils'
6+
7+
describe('isInternalFileUrl', () => {
8+
it('classifies relative serve paths as internal', () => {
9+
expect(isInternalFileUrl('/api/files/serve/kb/123-file.pdf')).toBe(true)
10+
expect(isInternalFileUrl('/api/files/serve/workspace/ws-1/file.txt?context=workspace')).toBe(
11+
true
12+
)
13+
})
14+
15+
it('classifies absolute serve URLs as internal regardless of host', () => {
16+
expect(isInternalFileUrl('https://www.sim.ai/api/files/serve/kb/x.pdf')).toBe(true)
17+
expect(isInternalFileUrl('http://localhost:3000/api/files/serve/blob/kb/x')).toBe(true)
18+
// Host is not used to gate (self-hosted/multi-domain); the storage sink authorizes.
19+
expect(isInternalFileUrl('https://other-host/api/files/serve/workspace/v/x')).toBe(true)
20+
})
21+
22+
it('does not match the marker outside the path (query/fragment)', () => {
23+
expect(isInternalFileUrl('https://evil.com/x?next=/api/files/serve/secret')).toBe(false)
24+
expect(isInternalFileUrl('https://evil.com/page#/api/files/serve/secret')).toBe(false)
25+
expect(isInternalFileUrl('https://evil.com/redirect?u=/api/files/serve/kb/x')).toBe(false)
26+
})
27+
28+
it('preserves traversal sequences so they survive downstream rejection', () => {
29+
// Must stay internal (not normalized away) so the parse route applies its `..` check.
30+
expect(isInternalFileUrl('https://attacker.com/api/files/serve/../../../etc/passwd')).toBe(true)
31+
expect(isInternalFileUrl('/api/files/serve/../../app.js')).toBe(true)
32+
})
33+
34+
it('returns false for non-internal and non-string inputs', () => {
35+
expect(isInternalFileUrl('https://example.com/file.pdf')).toBe(false)
36+
expect(isInternalFileUrl('data:text/plain;base64,abc')).toBe(false)
37+
// @ts-expect-error verifying runtime guard
38+
expect(isInternalFileUrl(undefined)).toBe(false)
39+
})
40+
})
641

742
describe('isAbortError', () => {
843
it('returns true for AbortError-named errors', () => {

apps/sim/lib/uploads/utils/file-utils.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -534,10 +534,33 @@ export function extractStorageKey(filePath: string): string {
534534
}
535535

536536
/**
537-
* Check if a URL is an internal file serve URL
537+
* Whether a URL targets the internal file-serve endpoint (`/api/files/serve/`).
538+
*
539+
* The marker is matched only in the URL's path component, so it cannot be
540+
* smuggled through a query string or fragment (e.g.
541+
* `https://evil.com/x?next=/api/files/serve/...`) to skip DNS/SSRF validation.
542+
*
543+
* The raw path is inspected without URL normalization on purpose: callers such
544+
* as the files parse route rely on traversal sequences (`..`) surviving this
545+
* check so they are rejected downstream rather than collapsed away. A path-only
546+
* marker still classifies any host as internal (e.g.
547+
* `https://other-host/api/files/serve/<key>`); cross-tenant reads are prevented
548+
* at the storage sink by {@link verifyFileAccess}, not by host matching, which
549+
* would break self-hosted and multi-domain deployments.
538550
*/
539551
export function isInternalFileUrl(fileUrl: string): boolean {
540-
return fileUrl.includes('/api/files/serve/')
552+
if (typeof fileUrl !== 'string') {
553+
return false
554+
}
555+
556+
let path = fileUrl
557+
const scheme = /^[a-z][a-z0-9+.-]*:\/\/[^/?#]*/i.exec(path)
558+
if (scheme) {
559+
path = path.slice(scheme[0].length)
560+
}
561+
path = path.split(/[?#]/, 1)[0]
562+
563+
return path.startsWith('/api/files/serve/')
541564
}
542565

543566
/**

0 commit comments

Comments
 (0)