mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(files): don't reject external URLs containing '..' in file parse validation (#4821)
* fix(files): don't reject external URLs containing '..' in file parse validation The file block's file_fetch operation rejected any external URL whose path contained '..' (e.g. Slack files-pri slugs with a literal '...') with 'Access denied: path traversal detected'. Traversal checks only apply to local paths — external http(s) URLs are fetched with SSRF protection downstream and are never resolved against the filesystem, so they now short-circuit as valid. Internal /api/files/serve/ URLs keep full traversal protection. * test(files): fix external-URL assertion to handle undefined error * test(files): assert success explicitly in external-URL traversal test * fix(files): keep traversal protection for https URLs matching internal serve paths
This commit is contained in:
@@ -796,6 +796,68 @@ describe('Files Parse API - Path Traversal Security', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('should not treat .. inside external URLs as path traversal', async () => {
|
||||
inputValidationMockFns.mockValidateUrlWithDNS.mockResolvedValue({
|
||||
isValid: true,
|
||||
resolvedIP: '203.0.113.10',
|
||||
})
|
||||
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValue(
|
||||
new Response('slack file content', {
|
||||
status: 200,
|
||||
headers: { 'content-type': 'text/plain' },
|
||||
})
|
||||
)
|
||||
permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
|
||||
|
||||
// Slack truncates long titles with a literal ellipsis, so the slug contains `..`
|
||||
const slackUrl =
|
||||
'https://files.slack.com/files-pri/T08-F0B/_other__no_invitation_messages_get_sent_-_sim_on_railway...txt'
|
||||
|
||||
const request = new NextRequest('http://localhost:3000/api/files/parse', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ filePath: slackUrl, workspaceId: 'workspace-id' }),
|
||||
})
|
||||
|
||||
const response = await POST(request)
|
||||
const result = await response.json()
|
||||
|
||||
expect(result.success).toBe(true)
|
||||
// The URL reaching the pinned fetch proves it passed validation and routed
|
||||
// to external-URL handling rather than being rejected as a local path.
|
||||
expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).toHaveBeenCalledWith(
|
||||
slackUrl,
|
||||
'203.0.113.10',
|
||||
expect.any(Object)
|
||||
)
|
||||
})
|
||||
|
||||
it('should still reject traversal in https URLs that look like internal serve URLs', async () => {
|
||||
inputValidationMockFns.mockValidateUrlWithDNS.mockResolvedValue({
|
||||
isValid: true,
|
||||
resolvedIP: '203.0.113.10',
|
||||
})
|
||||
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValue(
|
||||
new Response('should never be fetched', { status: 200 })
|
||||
)
|
||||
|
||||
// Absolute https URL containing `/api/files/serve/` matches isInternalFileUrl and would
|
||||
// route to handleCloudFile — so it must keep traversal protection, not be waved through
|
||||
// as an external URL.
|
||||
const request = new NextRequest('http://localhost:3000/api/files/parse', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({
|
||||
filePath: 'https://attacker.com/api/files/serve/../../../etc/passwd',
|
||||
}),
|
||||
})
|
||||
|
||||
const response = await POST(request)
|
||||
const result = await response.json()
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.error).toMatch(/Access denied: path traversal detected/)
|
||||
expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should handle encoded path traversal attempts', async () => {
|
||||
const encodedMaliciousPaths = [
|
||||
'/api/files/serve/%2e%2e%2f%2e%2e%2fetc%2fpasswd', // ../../../etc/passwd
|
||||
|
||||
@@ -419,13 +419,33 @@ function assertParsedContentWithinLimit(content: string, maxBytes?: number): str
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate file path for security - prevents null byte injection and path traversal attacks
|
||||
* Validate file path for security - prevents null byte injection and path traversal attacks.
|
||||
*
|
||||
* External URLs (`http`/`https`) are fetched over HTTP — with SSRF protection applied
|
||||
* downstream in `fetchExternalUrlToWorkspace` (DNS resolution + private/reserved IP blocking)
|
||||
* — and are never resolved against the filesystem, so `..`/`~` are legal URL content and must
|
||||
* not be rejected. Providers such as Slack routinely emit slugs containing a literal `...`.
|
||||
*
|
||||
* Internal file URLs (`/api/files/serve/...`) ARE resolved to storage keys and filesystem
|
||||
* paths via `extractStorageKey`, so they keep full traversal protection. The external
|
||||
* short-circuit explicitly excludes them: `parseFileSingle` routes anything matching
|
||||
* `isInternalFileUrl` to `handleCloudFile` (even an absolute `https://host/api/files/serve/...`),
|
||||
* so such inputs must stay subject to the `..`/`~` checks rather than being waved through as
|
||||
* external URLs. Only the leading-`/` "outside allowed directory" check is relaxed for them,
|
||||
* since that prefix is expected.
|
||||
*/
|
||||
function validateFilePath(filePath: string): { isValid: boolean; error?: string } {
|
||||
if (filePath.includes('\0')) {
|
||||
return { isValid: false, error: 'Invalid path: null byte detected' }
|
||||
}
|
||||
|
||||
if (
|
||||
(filePath.startsWith('http://') || filePath.startsWith('https://')) &&
|
||||
!isInternalFileUrl(filePath)
|
||||
) {
|
||||
return { isValid: true }
|
||||
}
|
||||
|
||||
if (filePath.includes('..')) {
|
||||
return { isValid: false, error: 'Access denied: path traversal detected' }
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user