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)