From f103d3d29f75e34657fcc6e2d4c7ed7cc6eddf77 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 12:12:25 -0700 Subject: [PATCH] improvement(execute): enforce workspace permissions on function file exports Check the acting user's workspace access before a function execution uses a request-supplied workspaceId, and gate workspace file writes in the shared VFS writer so every caller is covered by default. Access is resolved once per request and threaded through the export path so the added check does not re-query per output file. --- .../app/api/function/execute/route.test.ts | 120 ++++++++++++++++++ apps/sim/app/api/function/execute/route.ts | 103 ++++++++++++--- apps/sim/lib/copilot/request/tools/files.ts | 6 + .../lib/copilot/vfs/resource-writer.test.ts | 100 +++++++++++++++ apps/sim/lib/copilot/vfs/resource-writer.ts | 29 ++++- 5 files changed, 339 insertions(+), 19 deletions(-) diff --git a/apps/sim/app/api/function/execute/route.test.ts b/apps/sim/app/api/function/execute/route.test.ts index 91c9fb403f2..d39498ad2c9 100644 --- a/apps/sim/app/api/function/execute/route.test.ts +++ b/apps/sim/app/api/function/execute/route.test.ts @@ -28,6 +28,17 @@ import { SandboxOutputLimitError, } from '@/lib/execution/remote-sandbox/output-limits' +function grantedAccess(workspaceId: string) { + return { + exists: true, + hasAccess: true, + canWrite: true, + canAdmin: false, + workspace: { id: workspaceId }, + permission: 'admin', + } +} + const { mockExecuteInSandbox, mockExecuteInIsolatedVM, @@ -41,6 +52,8 @@ const { mockUploadFile, mockValidateWorkspaceFileWriteTarget, mockWriteWorkspaceFileByPath, + mockCheckWorkspaceAccess, + mockResolveWorkspaceAccess, } = vi.hoisted(() => ({ mockExecuteInSandbox: vi.fn(), mockExecuteInIsolatedVM: vi.fn(), @@ -59,6 +72,13 @@ const { mockUploadFile: vi.fn(), mockValidateWorkspaceFileWriteTarget: vi.fn(), mockWriteWorkspaceFileByPath: vi.fn(), + mockCheckWorkspaceAccess: vi.fn(), + mockResolveWorkspaceAccess: vi.fn(), +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + checkWorkspaceAccess: mockCheckWorkspaceAccess, + resolveWorkspaceAccess: mockResolveWorkspaceAccess, })) vi.mock('@/lib/core/security/encryption', () => ({ @@ -153,6 +173,9 @@ describe('Function Execute API Route', () => { authType: 'internal_jwt', }) + mockCheckWorkspaceAccess.mockImplementation(async (id: string) => grantedAccess(id)) + mockResolveWorkspaceAccess.mockImplementation(async (id: string) => grantedAccess(id)) + mockExecuteInIsolatedVM.mockResolvedValue({ result: 'test', stdout: '' }) mockUploadFile.mockImplementation(async ({ customKey }) => ({ key: customKey })) clearLargeValueCacheForTests() @@ -218,6 +241,103 @@ describe('Function Execute API Route', () => { expect(data).toHaveProperty('error', 'Unauthorized') }) + it('rejects a body-supplied workspaceId the acting user is not a member of', async () => { + mockCheckWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: false, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-victim' }, + permission: null, + }) + + const req = createMockRequest('POST', { + code: 'return "test"', + workspaceId: 'workspace-victim', + }) + + const response = await POST(req) + const data = await response.json() + + expect(response.status).toBe(403) + expect(data).toHaveProperty('error', 'Workspace access denied') + expect(mockCheckWorkspaceAccess).toHaveBeenCalledWith('workspace-victim', 'user-123') + expect(mockExecuteInIsolatedVM).not.toHaveBeenCalled() + }) + + it('rejects a sandbox output export into a workspace the acting user cannot write to', async () => { + envFlagsMock.isRemoteSandboxEnabled = true + mockExecuteInSandbox.mockResolvedValueOnce({ + result: 'done', + stdout: 'ok', + sandboxId: 'sandbox-123', + exportedFiles: { '/tmp/out.txt': 'owned by attacker' }, + }) + const readOnly = { + exists: true, + hasAccess: true, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-victim' }, + permission: 'read', + } + mockCheckWorkspaceAccess.mockResolvedValue(readOnly) + mockResolveWorkspaceAccess.mockResolvedValue(readOnly) + + const req = createMockRequest('POST', { + code: 'print("done")', + language: 'python', + workspaceId: 'workspace-victim', + outputs: { + files: [{ path: 'files/README.md', mode: 'overwrite', sandboxPath: '/tmp/out.txt' }], + }, + }) + + const response = await POST(req) + const data = await response.json() + + expect(response.status).toBe(403) + expect(data).toHaveProperty('error', 'Workspace access denied') + expect(mockValidateWorkspaceFileWriteTarget).not.toHaveBeenCalled() + expect(mockWriteWorkspaceFileByPath).not.toHaveBeenCalled() + }) + + it('rejects an export whose workspace is derived from a body-supplied workflowId', async () => { + envFlagsMock.isRemoteSandboxEnabled = true + mockExecuteInSandbox.mockResolvedValueOnce({ + result: 'done', + stdout: 'ok', + sandboxId: 'sandbox-123', + exportedFiles: { '/tmp/out.txt': 'owned by attacker' }, + }) + workflowsUtilsMock.getWorkflowById.mockResolvedValueOnce({ + id: 'workflow-victim', + workspaceId: 'workspace-victim', + }) + mockResolveWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: false, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-victim' }, + permission: null, + }) + + const req = createMockRequest('POST', { + code: 'print("done")', + language: 'python', + workflowId: 'workflow-victim', + outputs: { + files: [{ path: 'files/README.md', mode: 'overwrite', sandboxPath: '/tmp/out.txt' }], + }, + }) + + const response = await POST(req) + + expect(response.status).toBe(403) + expect(mockWriteWorkspaceFileByPath).not.toHaveBeenCalled() + }) + it('runs import-free JavaScript in isolated-vm without a remote provider', async () => { const req = createMockRequest('POST', { code: 'return "test"', diff --git a/apps/sim/app/api/function/execute/route.ts b/apps/sim/app/api/function/execute/route.ts index 6dbcacffebb..57b5c2594c5 100644 --- a/apps/sim/app/api/function/execute/route.ts +++ b/apps/sim/app/api/function/execute/route.ts @@ -88,6 +88,11 @@ import { type WorkspaceFileSecretProvenance, } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance' import { getWorkflowById } from '@/lib/workflows/utils' +import { + checkWorkspaceAccess, + resolveWorkspaceAccess, + type WorkspaceAccess, +} from '@/lib/workspaces/permissions/utils' import { escapeRegExp, normalizeName, REFERENCE } from '@/executor/constants' import { type OutputSchema, resolveBlockReference } from '@/executor/utils/block-reference' import { @@ -1383,11 +1388,41 @@ function exportUnchangedNote(sandboxPath?: string): string { ) } +function exportFailure( + error: string, + status: number, + stdout: string, + executionTime: number +): NextResponse { + return NextResponse.json( + { success: false, error, output: { result: null, stdout: cleanStdout(stdout), executionTime } }, + { status } + ) +} + +/** + * Both `workspaceId` and `workflowId` arrive in the request body, so the workspace an export + * resolves to is caller-controlled either way. Returns null when the acting user cannot write to + * it, gating the secret-provenance scan and overwrite probe that run before the write itself. + */ +async function authorizeExportWorkspace( + workspaceId: string, + authUserId: string, + provided?: WorkspaceAccess +): Promise { + const access = await resolveWorkspaceAccess(workspaceId, authUserId, provided) + if (access.exists && access.canWrite) return access + + logger.warn('Sandbox file export denied for workspace', { workspaceId, userId: authUserId }) + return null +} + async function maybeExportSandboxFileToWorkspace(args: { routeContext: FunctionRouteExecutionContext authUserId: string workflowId?: string workspaceId?: string + workspaceAccess?: WorkspaceAccess outputPath?: string outputFormat?: string outputMimeType?: string @@ -1403,6 +1438,7 @@ async function maybeExportSandboxFileToWorkspace(args: { authUserId, workflowId, workspaceId, + workspaceAccess, outputPath, outputFormat, outputMimeType, @@ -1432,16 +1468,17 @@ async function maybeExportSandboxFileToWorkspace(args: { workspaceId || (workflowId ? (await getWorkflowById(workflowId))?.workspaceId : undefined) if (!resolvedWorkspaceId) { - return NextResponse.json( - { - success: false, - error: 'Workspace context required to save sandbox file to workspace', - output: { result: null, stdout: cleanStdout(stdout), executionTime }, - }, - { status: 400 } + return exportFailure( + 'Workspace context required to save sandbox file to workspace', + 400, + stdout, + executionTime ) } + const access = await authorizeExportWorkspace(resolvedWorkspaceId, authUserId, workspaceAccess) + if (!access) return exportFailure('Workspace access denied', 403, stdout, executionTime) + if (exportedFileContent === undefined) { return NextResponse.json( { @@ -1496,6 +1533,7 @@ async function maybeExportSandboxFileToWorkspace(args: { const written = await writeWorkspaceFileByPath({ workspaceId: resolvedWorkspaceId, userId: authUserId, + workspaceAccess: access, target: { path: targetPath, mode, @@ -1557,6 +1595,7 @@ async function maybeExportSandboxFilesToWorkspace(args: { authUserId: string workflowId?: string workspaceId?: string + workspaceAccess?: WorkspaceAccess outputFiles: OutputFileDeclaration[] exportedFiles?: Record exportedFileContent?: string @@ -1587,6 +1626,7 @@ async function maybeExportSandboxFilesToWorkspace(args: { authUserId: args.authUserId, workflowId: args.workflowId, workspaceId: args.workspaceId, + workspaceAccess: args.workspaceAccess, outputPath: file.formatPath ?? file.path, outputFormat: file.format, outputMimeType: file.mimeType, @@ -1604,20 +1644,23 @@ async function maybeExportSandboxFilesToWorkspace(args: { args.workspaceId || (args.workflowId ? (await getWorkflowById(args.workflowId))?.workspaceId : undefined) if (!resolvedWorkspaceId) { - return NextResponse.json( - { - success: false, - error: 'Workspace context required to save sandbox files to workspace', - output: { - result: null, - stdout: cleanStdout(args.stdout), - executionTime: args.executionTime, - }, - }, - { status: 400 } + return exportFailure( + 'Workspace context required to save sandbox files to workspace', + 400, + args.stdout, + args.executionTime ) } + const access = await authorizeExportWorkspace( + resolvedWorkspaceId, + args.authUserId, + args.workspaceAccess + ) + if (!access) { + return exportFailure('Workspace access denied', 403, args.stdout, args.executionTime) + } + const preparedFiles = [] let totalOutputBytes = 0 for (const file of sandboxFiles) { @@ -1690,6 +1733,7 @@ async function maybeExportSandboxFilesToWorkspace(args: { validateWorkspaceFileWriteTarget({ workspaceId: resolvedWorkspaceId, userId: args.authUserId, + workspaceAccess: access, target: prepared.target, }) ) @@ -1744,6 +1788,7 @@ async function maybeExportSandboxFilesToWorkspace(args: { const written = await writeWorkspaceFileByPath({ workspaceId: resolvedWorkspaceId, userId: args.authUserId, + workspaceAccess: access, target: prepared.target, buffer, inferredMimeType: prepared.resolvedMimeType, @@ -1938,6 +1983,25 @@ export const POST = withRouteHandler(async (req: NextRequest) => { isCustomTool = false, _sandboxFiles, } = body + + // The internal JWT carries no workspace scope, so a body-supplied workspaceId would + // otherwise be the sole authorization input for sandbox selection and file exports. + // Denial is returned rather than thrown: this handler's catch-all would turn a thrown + // WorkspaceAccessDeniedError into a 500 before withRouteHandler could map it. + const workspaceAccess = workspaceId + ? await checkWorkspaceAccess(workspaceId, auth.userId) + : undefined + if (workspaceAccess && !workspaceAccess.hasAccess) { + logger.warn(`[${requestId}] Function execution denied for workspace`, { + workspaceId, + userId: auth.userId, + }) + return NextResponse.json( + { success: false, error: 'Workspace access denied' }, + { status: 403 } + ) + } + if (selectedSandboxId && !isRemoteSandboxEnabled) { return NextResponse.json( { success: false, error: 'The Function code sandbox is not configured' }, @@ -2174,6 +2238,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { authUserId: auth.userId, workflowId, workspaceId, + workspaceAccess, outputFiles, exportedFiles, exportedFileContent, @@ -2357,6 +2422,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { authUserId: auth.userId, workflowId, workspaceId, + workspaceAccess, outputFiles, exportedFiles, exportedFileContent, @@ -2447,6 +2513,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { authUserId: auth.userId, workflowId, workspaceId, + workspaceAccess, outputFiles, exportedFiles, exportedFileContent, diff --git a/apps/sim/lib/copilot/request/tools/files.ts b/apps/sim/lib/copilot/request/tools/files.ts index 7f4b9ca55ec..d5891082020 100644 --- a/apps/sim/lib/copilot/request/tools/files.ts +++ b/apps/sim/lib/copilot/request/tools/files.ts @@ -16,6 +16,7 @@ import { type WorkspaceFileSecretProvenance, type WorkspaceFileSecretProvenanceRepresentation, } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance' +import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils' import type { ResolvedSecretMatcher } from '@/executor/utils/resolved-secret-matcher' import { createResolvedSecretMatcher, @@ -433,6 +434,10 @@ export async function maybeWriteOutputToFile( } const writtenFiles = [] + // Resolved once so the writer's authorization check does not re-query per output file. + const workspaceAccess = preparedFiles.length + ? await checkWorkspaceAccess(workspaceId, userId) + : undefined for (const { outputFile, format, contentType, buffer, secretProvenance } of preparedFiles) { if (context.abortSignal?.aborted) { throw new Error('Request aborted before tool mutation could be applied') @@ -441,6 +446,7 @@ export async function maybeWriteOutputToFile( const written = await writeWorkspaceFileByPath({ workspaceId, userId, + workspaceAccess, target: { path: outputFile.path, mode: outputFile.mode ?? 'create', diff --git a/apps/sim/lib/copilot/vfs/resource-writer.test.ts b/apps/sim/lib/copilot/vfs/resource-writer.test.ts index 9142bcde515..5e81993fb1e 100644 --- a/apps/sim/lib/copilot/vfs/resource-writer.test.ts +++ b/apps/sim/lib/copilot/vfs/resource-writer.test.ts @@ -5,8 +5,19 @@ const mocks = vi.hoisted(() => { readonly code = 'FILE_EXISTS' as const } + /** + * Stands in for the production error, which extends `HttpError`. Only the message is + * asserted here; the real class is what carries `statusCode = 403` to `withRouteHandler`. + */ + class WorkspaceAccessDeniedError extends Error { + constructor(readonly workspaceId: string) { + super(`Workspace access denied: ${workspaceId}`) + } + } + return { FileConflictError, + WorkspaceAccessDeniedError, ensureWorkspaceFileFolderPath: vi.fn(), findWorkspaceFileFolderIdByPath: vi.fn(), normalizeWorkspaceFileItemName: vi.fn((name: string) => name.trim()), @@ -14,9 +25,15 @@ const mocks = vi.hoisted(() => { resolveWorkspaceFileReference: vi.fn(), updateWorkspaceFileContent: vi.fn(), uploadWorkspaceFile: vi.fn(), + resolveWorkspaceAccess: vi.fn(), } }) +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + resolveWorkspaceAccess: mocks.resolveWorkspaceAccess, + WorkspaceAccessDeniedError: mocks.WorkspaceAccessDeniedError, +})) + vi.mock('@/lib/uploads/contexts/workspace/workspace-file-folder-manager', () => ({ ensureWorkspaceFileFolderPath: mocks.ensureWorkspaceFileFolderPath, findWorkspaceFileFolderIdByPath: mocks.findWorkspaceFileFolderIdByPath, @@ -37,6 +54,89 @@ describe('resource writer', () => { beforeEach(() => { vi.clearAllMocks() mocks.ensureWorkspaceFileFolderPath.mockResolvedValue('folder-id') + mocks.resolveWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: true, + canWrite: true, + canAdmin: false, + workspace: { id: 'workspace-1' }, + permission: 'admin', + }) + }) + + it('refuses to write into a workspace the acting user is not a member of', async () => { + mocks.resolveWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: false, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-victim' }, + permission: null, + }) + + await expect( + writeWorkspaceFileByPath({ + workspaceId: 'workspace-victim', + userId: 'attacker', + target: { path: 'files/README.md', mode: 'overwrite' }, + buffer: Buffer.from('owned'), + inferredMimeType: 'text/markdown', + }) + ).rejects.toThrow('Workspace access denied: workspace-victim') + + expect(mocks.resolveWorkspaceAccess).toHaveBeenCalledWith( + 'workspace-victim', + 'attacker', + undefined + ) + expect(mocks.resolveWorkspaceFileReference).not.toHaveBeenCalled() + expect(mocks.updateWorkspaceFileContent).not.toHaveBeenCalled() + expect(mocks.uploadWorkspaceFile).not.toHaveBeenCalled() + }) + + it('refuses to write for a read-only workspace member', async () => { + mocks.resolveWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: true, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-1' }, + permission: 'read', + }) + + await expect( + writeWorkspaceFileByPath({ + workspaceId: 'workspace-1', + userId: 'reader', + target: { path: 'files/notes.md', mode: 'create' }, + buffer: Buffer.from('hello'), + inferredMimeType: 'text/markdown', + }) + ).rejects.toThrow('Workspace access denied: workspace-1') + + expect(mocks.uploadWorkspaceFile).not.toHaveBeenCalled() + }) + + it('refuses to validate a write target in a workspace the user cannot write to', async () => { + mocks.resolveWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: false, + canWrite: false, + canAdmin: false, + workspace: { id: 'workspace-victim' }, + permission: null, + }) + + await expect( + validateWorkspaceFileWriteTarget({ + workspaceId: 'workspace-victim', + userId: 'attacker', + target: { path: 'files/README.md', mode: 'overwrite' }, + }) + ).rejects.toThrow('Workspace access denied: workspace-victim') + + expect(mocks.resolveWorkspaceFileReference).not.toHaveBeenCalled() + expect(mocks.findWorkspaceFileFolderIdByPath).not.toHaveBeenCalled() }) it('auto-creates missing parent folders for plain workspace file creates', async () => { diff --git a/apps/sim/lib/copilot/vfs/resource-writer.ts b/apps/sim/lib/copilot/vfs/resource-writer.ts index 9dfa24141f2..3e1f74902fc 100644 --- a/apps/sim/lib/copilot/vfs/resource-writer.ts +++ b/apps/sim/lib/copilot/vfs/resource-writer.ts @@ -15,6 +15,11 @@ import { EXACT_EMPTY_WORKSPACE_FILE_SECRET_PROVENANCE, type WorkspaceFileSecretProvenance, } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance' +import { + resolveWorkspaceAccess, + type WorkspaceAccess, + WorkspaceAccessDeniedError, +} from '@/lib/workspaces/permissions/utils' export type WorkspaceFileWriteMode = 'create' | 'overwrite' @@ -136,11 +141,30 @@ function vfsPathForRecord(record: WorkspaceFileRecord): string { return canonicalWorkspaceFilePath({ folderPath: record.folderPath, name: record.name }) } +/** + * Authorization is a property of the writer rather than something each caller has to remember, + * because `workspaceId` reaches here from request bodies. Callers holding a resolved access pass + * it through `workspaceAccess`; {@link resolveWorkspaceAccess} guards the reuse. + */ +async function assertWorkspaceFileWriteAccess(args: { + workspaceId: string + userId: string + workspaceAccess?: WorkspaceAccess +}): Promise { + const access = await resolveWorkspaceAccess(args.workspaceId, args.userId, args.workspaceAccess) + if (!access.exists || !access.canWrite) { + throw new WorkspaceAccessDeniedError(args.workspaceId) + } +} + export async function validateWorkspaceFileWriteTarget(args: { workspaceId: string - userId?: string + userId: string + workspaceAccess?: WorkspaceAccess target: WorkspaceFileWriteTarget }): Promise { + await assertWorkspaceFileWriteAccess(args) + if (args.target.mode === 'overwrite') { const existing = await resolveWorkspaceFileReference(args.workspaceId, args.target.path) if (!existing) { @@ -165,6 +189,7 @@ export async function validateWorkspaceFileWriteTarget(args: { export async function writeWorkspaceFileByPath(args: { workspaceId: string userId: string + workspaceAccess?: WorkspaceAccess target: WorkspaceFileWriteTarget buffer: Buffer inferredMimeType: string @@ -177,6 +202,8 @@ export async function writeWorkspaceFileByPath(args: { /** Private provenance for the exact bytes being written. */ secretProvenance?: WorkspaceFileSecretProvenance }): Promise { + await assertWorkspaceFileWriteAccess(args) + const contentType = args.target.mimeType || args.inferredMimeType if (args.target.mode === 'overwrite') { const existing = await resolveWorkspaceFileReference(args.workspaceId, args.target.path)