fix(tools): close boundary review gaps

Author: icecrasher321

apps/sim/lib/execution/payloads/materialization.server.test.ts

@@ -85,6 +85,46 @@ describe('readUserFileContent', () => {
     expect(mockVerifyFileAccess).not.toHaveBeenCalled()
   })
 
+  it.each(['profile-pictures', 'og-images', 'workspace-logos'] as const)(
+    'authorizes actorless reads from the trusted public %s context',
+    async (context) => {
+      const publicFile: UserFile = {
+        id: 'public-file',
+        name: 'public.png',
+        url: '',
+        size: 6,
+        type: 'image/png',
+        key: `${context}/public.png`,
+        context,
+      }
+      mockDownloadServableFileFromStorage.mockResolvedValueOnce({ buffer: Buffer.from('public') })
+
+      await expect(readUserFileContent(publicFile, { encoding: 'text' })).resolves.toBe('public')
+
+      expect(mockVerifyFileAccess).not.toHaveBeenCalled()
+      expect(mockResolveWorkspaceFile).not.toHaveBeenCalled()
+    }
+  )
+
+  it('does not let an actorless caller relabel a private key as public', async () => {
+    const relabeledFile: UserFile = {
+      id: 'private-file',
+      name: 'private.txt',
+      url: '',
+      size: 7,
+      type: 'text/plain',
+      key: 'workspace/workspace-1/private.txt',
+      context: 'og-images',
+    }
+
+    await expect(readUserFileContent(relabeledFile, { encoding: 'text' })).rejects.toThrow(
+      'File context does not match its storage key.'
+    )
+
+    expect(mockDownloadServableFileFromStorage).not.toHaveBeenCalled()
+    expect(mockVerifyFileAccess).not.toHaveBeenCalled()
+  })
+
   it('authorizes workspace files with the preserved actorless deployment principal', async () => {
     const principal = {
       kind: 'delegated' as const,

apps/sim/lib/execution/payloads/materialization.server.ts

@@ -23,6 +23,7 @@ import {
   bufferToBase64,
   inferContextFromKey,
   isGeneratedDocumentSourceType,
+  isPublicStorageContext,
 } from '@/lib/uploads/utils/file-utils'
 import { downloadServableFileFromStorage } from '@/lib/uploads/utils/file-utils.server'
 import { rebindWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal'
@@ -274,6 +275,10 @@ export async function assertUserFileContentAccess(
     return
   }
 
+  if (isPublicStorageContext(context)) {
+    return
+  }
+
   if (context === 'workspace' && options.principal && options.workspaceId) {
     const principal =
       options.principal.kind === 'delegated'

apps/sim/lib/internal/file/operations.test.ts

@@ -216,6 +216,34 @@ function workspaceFile(id: string, ownerUserId = 'user-1') {
   }
 }
 
+function actorlessDeploymentPrincipal(workspaceId = 'workspace-1') {
+  return {
+    kind: 'delegated' as const,
+    serviceId: 'executor' as const,
+    workspaceId,
+    delegationId: 'delegation-1',
+    audience: 'sim:workspace-files',
+    issuedAt: new Date(Date.now() - 1_000),
+    expiresAt: new Date(Date.now() + 60_000),
+    delegationContext: {
+      kind: 'workflow_execution' as const,
+      workflowId: 'workflow-1',
+      executionId: 'execution-1',
+      principal: {
+        kind: 'system' as const,
+        serviceId: 'schedule' as const,
+        workspaceId,
+        workflowId: 'workflow-1',
+      },
+      currentWorkflow: {
+        workflowId: 'workflow-1',
+        mode: 'deployment' as const,
+        deploymentVersionId: 'deployment-1',
+      },
+    },
+  }
+}
+
 describe('file manage operations', () => {
   beforeEach(() => {
     vi.clearAllMocks()
@@ -746,31 +774,7 @@ describe('file manage operations', () => {
 
   it('decompresses a canonical workspace archive for an actorless deployed execution', async () => {
     const archiveBuffer = Buffer.from('archive-bytes')
-    const principal = {
-      kind: 'delegated' as const,
-      serviceId: 'executor' as const,
-      workspaceId: 'workspace-1',
-      delegationId: 'delegation-1',
-      audience: 'sim:workspace-files',
-      issuedAt: new Date(Date.now() - 1_000),
-      expiresAt: new Date(Date.now() + 60_000),
-      delegationContext: {
-        kind: 'workflow_execution' as const,
-        workflowId: 'workflow-1',
-        executionId: 'execution-1',
-        principal: {
-          kind: 'system' as const,
-          serviceId: 'schedule' as const,
-          workspaceId: 'workspace-1',
-          workflowId: 'workflow-1',
-        },
-        currentWorkflow: {
-          workflowId: 'workflow-1',
-          mode: 'deployment' as const,
-          deploymentVersionId: 'deployment-1',
-        },
-      },
-    }
+    const principal = actorlessDeploymentPrincipal()
     mockDownloadFileFromStorage.mockResolvedValue(archiveBuffer)
     mockGetWorkspaceFile.mockResolvedValue({
       ...workspaceFile('archive'),
@@ -812,13 +816,39 @@ describe('file manage operations', () => {
 
     expect(response.status).toBe(200)
     expect(mockResolveEffectiveWorkspacePermission).not.toHaveBeenCalled()
-    expect(mockVerifyFileAccess).not.toHaveBeenCalled()
+    expect(mockGetWorkspaceFile).toHaveBeenCalledWith('workspace-1', 'archive', {
+      throwOnError: true,
+    })
     expect(mockDecompressArchiveBufferToWorkspaceFiles).toHaveBeenCalledWith(
       archiveBuffer,
       expect.objectContaining({ principal, workspaceId: 'workspace-1' })
     )
   })
 
+  it('rejects an actorless deployment principal bound to a different workspace', async () => {
+    const response = await executeFileManageOperation(
+      fileManageBodySchema.parse({
+        operation: 'decompress',
+        workspaceId: 'workspace-1',
+        fileId: 'archive',
+      }),
+      {
+        principal: actorlessDeploymentPrincipal('workspace-2'),
+        workspaceId: 'workspace-1',
+        attributedUserId: 'workspace-owner',
+        workflowId: 'workflow-1',
+        executionId: 'execution-1',
+        headers: new Headers(),
+        requestId: 'request-cross-workspace',
+      }
+    )
+
+    expect(response.status).toBe(403)
+    expect(mockGetWorkspaceFile).not.toHaveBeenCalled()
+    expect(mockDownloadFileFromStorage).not.toHaveBeenCalled()
+    expect(mockDecompressArchiveBufferToWorkspaceFiles).not.toHaveBeenCalled()
+  })
+
   it('omits source scope when canonical files have different owners', async () => {
     mockGetWorkspaceFile.mockImplementation(async (_workspaceId: string, fileId: string) =>
       workspaceFile(fileId, fileId === 'file-1' ? 'user-1' : 'user-2')

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

@@ -788,6 +788,11 @@ const PUBLIC_STORAGE_CONTEXTS = new Set<StorageContext>([
   'workspace-logos',
 ])
 
+/** Whether a trusted storage context is world-readable. */
+export function isPublicStorageContext(context: StorageContext): boolean {
+  return PUBLIC_STORAGE_CONTEXTS.has(context)
+}
+
 /**
  * Resolve the storage context for a stored file from its trusted key prefix.
  *
@@ -811,7 +816,7 @@ export function resolveTrustedFileContext(key: string, context?: string): Storag
   try {
     return inferContextFromKey(key)
   } catch (error) {
-    if (context && !PUBLIC_STORAGE_CONTEXTS.has(context as StorageContext)) {
+    if (context && !isPublicStorageContext(context as StorageContext)) {
       return context as StorageContext
     }
     throw error

scripts/check-tool-request-boundary.test.ts

@@ -51,8 +51,15 @@ describe('tool self-hop audit', () => {
   })
 
   it('rejects a compound same-origin URL constructor path', () => {
-    const audit = auditRequest(`
-      url: (params) => new URL('/api/tools/' + params.id, params.baseUrl).toString()
+    const audit = auditToolSelfHops(`
+      import { getBaseUrl } from '@/lib/core/utils/urls'
+      const tool = {
+        id: 'test_tool',
+        request: {
+          url: (params) => new URL('/api/tools/' + params.id, getBaseUrl()).toString(),
+          method: 'POST',
+        },
+      }
     `)
 
     expect(audit.violations[0]?.reason).toBe('same-origin-tool-request')
@@ -96,10 +103,81 @@ describe('tool self-hop audit', () => {
   })
 
   it('rejects a same-origin URL constructor', () => {
-    const audit = auditRequest(`
-      url: (params) => {
-        const url = new URL('/api/tools/test', params.baseUrl)
-        return url.toString()
+    const audit = auditToolSelfHops(`
+      import { getInternalApiBaseUrl } from '@/lib/core/utils/urls'
+      const tool = {
+        id: 'test_tool',
+        request: {
+          url: () => {
+            const url = new URL('/api/tools/test', getInternalApiBaseUrl())
+            return url.toString()
+          },
+          method: 'POST',
+        },
+      }
+    `)
+
+    expect(audit.violations[0]?.reason).toBe('same-origin-tool-request')
+  })
+
+  it('rejects a same-origin path passed through a local URL helper', () => {
+    const audit = auditToolSelfHops(`
+      import { getBaseUrl as getSimOrigin } from '@/lib/core/utils/urls'
+      function providerUrl(path, host) {
+        return new URL(path, host).toString()
+      }
+      const simOrigin = getSimOrigin()
+      const tool = {
+        id: 'test_tool',
+        request: {
+          url: () => providerUrl('/api/tools/test', simOrigin),
+          method: 'POST',
+        },
+      }
+    `)
+
+    expect(audit.violations[0]?.reason).toBe('same-origin-tool-request')
+  })
+
+  it('rejects a same-origin path forwarded through nested local helpers', () => {
+    const audit = auditToolSelfHops(`
+      import { getBaseUrl } from '@/lib/core/utils/urls'
+      function providerUrl(path, host) {
+        return new URL(path, host).toString()
+      }
+      function buildUrl(path) {
+        return providerUrl(path, getBaseUrl())
+      }
+      const tool = {
+        id: 'test_tool',
+        request: { url: () => buildUrl('/api/tools/test'), method: 'POST' },
+      }
+    `)
+
+    expect(audit.violations[0]?.reason).toBe('same-origin-tool-request')
+  })
+
+  it('rejects a same-origin path concatenated with the Sim origin', () => {
+    const audit = auditToolSelfHops(`
+      import { getBaseUrl } from '@/lib/core/utils/urls'
+      const tool = {
+        id: 'test_tool',
+        request: { url: () => getBaseUrl() + '/api/tools/test', method: 'POST' },
+      }
+    `)
+
+    expect(audit.violations[0]?.reason).toBe('same-origin-tool-request')
+  })
+
+  it('rejects a same-origin path passed through a known imported URL builder', () => {
+    const audit = auditToolSelfHops(`
+      import { buildAPIUrl as buildSimUrl } from '@/executor/utils/http'
+      const tool = {
+        id: 'test_tool',
+        request: {
+          url: () => buildSimUrl('/api/tools/test').toString(),
+          method: 'POST',
+        },
       }
     `)
 
@@ -141,4 +219,21 @@ describe('tool self-hop audit', () => {
 
     expect(audit.violations).toEqual([])
   })
+
+  it('allows an API-shaped provider path resolved against an external origin', () => {
+    const audit = auditToolSelfHops(`
+      function providerUrl(path, host) {
+        return new URL(path, host).toString()
+      }
+      const tool = {
+        id: 'test_tool',
+        request: {
+          url: () => providerUrl('/api/messages', 'https://provider.example.com'),
+          method: 'POST',
+        },
+      }
+    `)
+
+    expect(audit.violations).toEqual([])
+  })
 })