refactor(agent-handler): simplify agent handler, update tests, fix resolution of env vars in function execution (#437)

* refactored agent handler, fixed envvar resolution for function block

* resolve missing envvar resolution from function execution for custom tool

* fix path traversal risk

* removed extraneous comments

* ack PR comments

Author: waleedlatif1

apps/sim/app/api/files/parse/route.test.ts

@@ -1,23 +1,22 @@
 import path from 'path'
+import { NextRequest } from 'next/server'
 /**
  * Tests for file parse API route
  *
  * @vitest-environment node
  */
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import { createMockRequest } from '@/app/api/__test-utils__/utils'
+import { POST } from './route'
 
-// Create actual mocks for path functions that we can use instead of using vi.doMock for path
 const mockJoin = vi.fn((...args: string[]): string => {
-  // For the UPLOAD_DIR paths, just return a test path
   if (args[0] === '/test/uploads') {
     return `/test/uploads/${args[args.length - 1]}`
   }
   return path.join(...args)
 })
 
 describe('File Parse API Route', () => {
-  // Mock file system and parser modules
   const mockReadFile = vi.fn().mockResolvedValue(Buffer.from('test file content'))
   const mockWriteFile = vi.fn().mockResolvedValue(undefined)
   const mockUnlink = vi.fn().mockResolvedValue(undefined)
@@ -36,15 +35,12 @@ describe('File Parse API Route', () => {
   beforeEach(() => {
     vi.resetModules()
 
-    // Reset all mocks
     vi.resetAllMocks()
 
-    // Create a test upload file that exists for all tests
     mockReadFile.mockResolvedValue(Buffer.from('test file content'))
     mockAccessFs.mockResolvedValue(undefined)
     mockStatFs.mockImplementation(() => ({ isFile: () => true }))
 
-    // Mock filesystem operations
     vi.doMock('fs', () => ({
       existsSync: vi.fn().mockReturnValue(true),
       constants: { R_OK: 4 },
@@ -63,19 +59,16 @@ describe('File Parse API Route', () => {
       stat: mockStatFs,
     }))
 
-    // Mock the S3 client
     vi.doMock('@/lib/uploads/s3-client', () => ({
       downloadFromS3: mockDownloadFromS3,
     }))
 
-    // Mock file parsers
     vi.doMock('@/lib/file-parsers', () => ({
       isSupportedFileType: vi.fn().mockReturnValue(true),
       parseFile: mockParseFile,
       parseBuffer: mockParseBuffer,
     }))
 
-    // Mock path module with our custom join function
     vi.doMock('path', () => {
       return {
         ...path,
@@ -85,7 +78,6 @@ describe('File Parse API Route', () => {
       }
     })
 
-    // Mock the logger
     vi.doMock('@/lib/logs/console-logger', () => ({
       createLogger: vi.fn().mockReturnValue({
         info: vi.fn(),
@@ -95,7 +87,6 @@ describe('File Parse API Route', () => {
       }),
     }))
 
-    // Configure upload directory and S3 mode
     vi.doMock('@/lib/uploads/setup', () => ({
       UPLOAD_DIR: '/test/uploads',
       USE_S3_STORAGE: false,
@@ -105,15 +96,13 @@ describe('File Parse API Route', () => {
       },
     }))
 
-    // Skip setup.server.ts side effects
     vi.doMock('@/lib/uploads/setup.server', () => ({}))
   })
 
   afterEach(() => {
     vi.clearAllMocks()
   })
 
-  // Basic tests testing the API structure
   it('should handle missing file path', async () => {
     const req = createMockRequest('POST', {})
     const { POST } = await import('./route')
@@ -125,47 +114,37 @@ describe('File Parse API Route', () => {
     expect(data).toHaveProperty('error', 'No file path provided')
   })
 
-  // Test skipping the implementation details and testing what users would care about
   it('should accept and process a local file', async () => {
-    // Given: A request with a file path
     const req = createMockRequest('POST', {
       filePath: '/api/files/serve/test-file.txt',
     })
 
-    // When: The API processes the request
     const { POST } = await import('./route')
     const response = await POST(req)
     const data = await response.json()
 
-    // Then: Check the API contract without making assumptions about implementation
     expect(response.status).toBe(200)
-    expect(data).not.toBeNull() // We got a response
+    expect(data).not.toBeNull()
 
-    // The response either has a success indicator with output OR an error
     if (data.success === true) {
       expect(data).toHaveProperty('output')
     } else {
-      // If error, there should be an error message
       expect(data).toHaveProperty('error')
       expect(typeof data.error).toBe('string')
     }
   })
 
   it('should process S3 files', async () => {
-    // Given: A request with an S3 file path
     const req = createMockRequest('POST', {
       filePath: '/api/files/serve/s3/test-file.pdf',
     })
 
-    // When: The API processes the request
     const { POST } = await import('./route')
     const response = await POST(req)
     const data = await response.json()
 
-    // Then: We should get a response with parsed content or error
     expect(response.status).toBe(200)
 
-    // The data should either have a success flag with output or an error
     if (data.success === true) {
       expect(data).toHaveProperty('output')
     } else {
@@ -174,17 +153,14 @@ describe('File Parse API Route', () => {
   })
 
   it('should handle multiple files', async () => {
-    // Given: A request with multiple file paths
     const req = createMockRequest('POST', {
       filePath: ['/api/files/serve/file1.txt', '/api/files/serve/file2.txt'],
     })
 
-    // When: The API processes the request
     const { POST } = await import('./route')
     const response = await POST(req)
     const data = await response.json()
 
-    // Then: We get an array of results
     expect(response.status).toBe(200)
     expect(data).toHaveProperty('success')
     expect(data).toHaveProperty('results')
@@ -193,43 +169,220 @@ describe('File Parse API Route', () => {
   })
 
   it('should handle S3 access errors gracefully', async () => {
-    // Given: S3 will throw an error
     mockDownloadFromS3.mockRejectedValueOnce(new Error('S3 access denied'))
 
-    // And: A request with an S3 file path
     const req = createMockRequest('POST', {
       filePath: '/api/files/serve/s3/access-denied.pdf',
     })
 
-    // When: The API processes the request
     const { POST } = await import('./route')
     const response = await POST(req)
     const data = await response.json()
 
-    // Then: We get an appropriate error
     expect(response.status).toBe(200)
     expect(data).toHaveProperty('success', false)
     expect(data).toHaveProperty('error')
     expect(data.error).toContain('S3 access denied')
   })
 
   it('should handle access errors gracefully', async () => {
-    // Given: File access will fail
     mockAccessFs.mockRejectedValueOnce(new Error('ENOENT: no such file'))
 
-    // And: A request with a nonexistent file
     const req = createMockRequest('POST', {
       filePath: '/api/files/serve/nonexistent.txt',
     })
 
-    // When: The API processes the request
     const { POST } = await import('./route')
     const response = await POST(req)
     const data = await response.json()
 
-    // Then: We get an appropriate error response
     expect(response.status).toBe(200)
     expect(data).toHaveProperty('success')
     expect(data).toHaveProperty('error')
   })
 })
+
+describe('Files Parse API - Path Traversal Security', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  describe('Path Traversal Prevention', () => {
+    it('should reject path traversal attempts with .. segments', async () => {
+      const maliciousRequests = [
+        '../../../etc/passwd',
+        '/api/files/serve/../../../etc/passwd',
+        '/api/files/serve/../../app.js',
+        '/api/files/serve/../.env',
+        'uploads/../../../etc/hosts',
+      ]
+
+      for (const maliciousPath of maliciousRequests) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: maliciousPath,
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        expect(result.success).toBe(false)
+        expect(result.error).toMatch(/Access denied|Invalid path|Path outside allowed directory/)
+      }
+    })
+
+    it('should reject paths with tilde characters', async () => {
+      const maliciousPaths = [
+        '~/../../etc/passwd',
+        '/api/files/serve/~/secret.txt',
+        '~root/.ssh/id_rsa',
+      ]
+
+      for (const maliciousPath of maliciousPaths) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: maliciousPath,
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        expect(result.success).toBe(false)
+        expect(result.error).toMatch(/Access denied|Invalid path/)
+      }
+    })
+
+    it('should reject absolute paths outside upload directory', async () => {
+      const maliciousPaths = [
+        '/etc/passwd',
+        '/root/.bashrc',
+        '/app/.env',
+        '/var/log/auth.log',
+        'C:\\Windows\\System32\\drivers\\etc\\hosts', // Windows path
+      ]
+
+      for (const maliciousPath of maliciousPaths) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: maliciousPath,
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        expect(result.success).toBe(false)
+        expect(result.error).toMatch(/Access denied|Path outside allowed directory/)
+      }
+    })
+
+    it('should allow valid paths within upload directory', async () => {
+      // Test that valid paths don't trigger path validation errors
+      const validPaths = [
+        '/api/files/serve/document.txt',
+        '/api/files/serve/folder/file.pdf',
+        '/api/files/serve/subfolder/image.png',
+      ]
+
+      for (const validPath of validPaths) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: validPath,
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        // Should not fail due to path validation (may fail for other reasons like file not found)
+        if (result.error) {
+          expect(result.error).not.toMatch(
+            /Access denied|Path outside allowed directory|Invalid path/
+          )
+        }
+      }
+    })
+
+    it('should handle encoded path traversal attempts', async () => {
+      const encodedMaliciousPaths = [
+        '/api/files/serve/%2e%2e%2f%2e%2e%2fetc%2fpasswd', // ../../../etc/passwd
+        '/api/files/serve/..%2f..%2f..%2fetc%2fpasswd',
+        '/api/files/serve/%2e%2e/%2e%2e/etc/passwd',
+      ]
+
+      for (const maliciousPath of encodedMaliciousPaths) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: decodeURIComponent(maliciousPath), // Simulate URL decoding
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        expect(result.success).toBe(false)
+        expect(result.error).toMatch(/Access denied|Invalid path|Path outside allowed directory/)
+      }
+    })
+
+    it('should handle null byte injection attempts', async () => {
+      const nullBytePaths = [
+        '/api/files/serve/file.txt\0../../etc/passwd',
+        'file.txt\0/etc/passwd',
+        '/api/files/serve/document.pdf\0/var/log/auth.log',
+      ]
+
+      for (const maliciousPath of nullBytePaths) {
+        const request = new NextRequest('http://localhost:3000/api/files/parse', {
+          method: 'POST',
+          body: JSON.stringify({
+            filePath: maliciousPath,
+          }),
+        })
+
+        const response = await POST(request)
+        const result = await response.json()
+
+        expect(result.success).toBe(false)
+        // Should be rejected either by path validation or file system access
+      }
+    })
+  })
+
+  describe('Edge Cases', () => {
+    it('should handle empty file paths', async () => {
+      const request = new NextRequest('http://localhost:3000/api/files/parse', {
+        method: 'POST',
+        body: JSON.stringify({
+          filePath: '',
+        }),
+      })
+
+      const response = await POST(request)
+      const result = await response.json()
+
+      expect(response.status).toBe(400)
+      expect(result.error).toBe('No file path provided')
+    })
+
+    it('should handle missing filePath parameter', async () => {
+      const request = new NextRequest('http://localhost:3000/api/files/parse', {
+        method: 'POST',
+        body: JSON.stringify({}),
+      })
+
+      const response = await POST(request)
+      const result = await response.json()
+
+      expect(response.status).toBe(400)
+      expect(result.error).toBe('No file path provided')
+    })
+  })
+})