diff --git a/apps/sim/lib/copilot/tool-executor/executor.test.ts b/apps/sim/lib/copilot/tool-executor/executor.test.ts index dff1fc1c42f..772cccc5ca7 100644 --- a/apps/sim/lib/copilot/tool-executor/executor.test.ts +++ b/apps/sim/lib/copilot/tool-executor/executor.test.ts @@ -242,7 +242,12 @@ describe('copilot tool executor fallback', () => { const runWorkflowHandler = vi.fn().mockResolvedValue({ success: true, output: { ran: true } }) registerHandler('run_workflow', runWorkflowHandler) - const context = { userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'ws-1' } + const context = { + userId: 'user-1', + workflowId: 'workflow-1', + workspaceId: 'ws-1', + userPermission: 'write', + } const result = await executeTool('run_workflow', { workflow_input: {} }, context) expect(runWorkflowHandler).toHaveBeenCalledWith({ workflow_input: {} }, context) @@ -250,6 +255,35 @@ describe('copilot tool executor fallback', () => { expect(result).toEqual({ success: true, output: { ran: true } }) }) + /** + * `run_workflow` carries no catalog permission — the browser path authorizes it through the + * workflow APIs against the caller's own session. The headless fallback has no session, so + * without a bar of its own a deliberately capped run (an unattributed inbox message) would + * still execute workflows under the principal it was capped away from. + */ + it.each([['read'], [undefined]] as const)( + 'refuses the headless client fallback for a %s permission', + async (userPermission) => { + isKnownTool.mockReturnValue(true) + isSimExecuted.mockReturnValue(false) + isClientExecuted.mockReturnValue(true) + + const runWorkflowHandler = vi.fn().mockResolvedValue({ success: true }) + registerHandler('run_workflow', runWorkflowHandler) + + const result = await executeTool( + 'run_workflow', + { workflow_input: {} }, + { userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'ws-1', userPermission } + ) + + expect(result.success).toBe(false) + expect(result.error).toContain('requires write access') + expect(runWorkflowHandler).not.toHaveBeenCalled() + expect(executeAppTool).not.toHaveBeenCalled() + } + ) + it('falls back to app tool executor for client-routed tools with no registered handler', async () => { isKnownTool.mockReturnValue(true) isSimExecuted.mockReturnValue(false) diff --git a/apps/sim/lib/copilot/tool-executor/executor.ts b/apps/sim/lib/copilot/tool-executor/executor.ts index e934803dcfe..e4f7ba3a3ee 100644 --- a/apps/sim/lib/copilot/tool-executor/executor.ts +++ b/apps/sim/lib/copilot/tool-executor/executor.ts @@ -38,7 +38,26 @@ export async function executeTool( params: Record, context: ToolExecutionContext ): Promise { - const requiredPermission = getToolEntry(toolId)?.requiredPermission + // Client-routed tools (e.g. run_workflow) are normally executed in the browser and never + // reach this point in interactive mode. In headless mode (Mothership block, no browser) there + // is no client to delegate to, so fall back to the registered server-side handler when one + // exists — otherwise the call would route to executeAppTool and throw "Tool not found". + const usesHeadlessClientFallback = isClientExecuted(toolId) && hasHandler(toolId) + + /** + * Client-routed tools carry no catalog `requiredPermission` because the browser runs them + * through the workflow APIs, which authorize the caller's own session. The headless fallback + * has no session to authorize against and runs under the request's principal instead, so it + * has to supply a bar of its own. + * + * Without one, a run whose permission was deliberately capped still reaches `run_workflow`, + * and `runWorkflowFromCopilot` executes with `enforceCredentialAccess` — resolving the + * principal's workspace and personal secrets. That is the hole an unattributed inbox message + * leaves open: `resolveInboxExecutionActor` refuses it a secret actor, but the run still + * carries the workspace owner as principal. + */ + const requiredPermission = + getToolEntry(toolId)?.requiredPermission ?? (usesHeadlessClientFallback ? 'write' : undefined) if ( requiredPermission && !permissionSatisfies( @@ -54,13 +73,8 @@ export async function executeTool( const normalizedParams = normalizeToolParams(toolId, params, context) - // Client-routed tools (e.g. run_workflow) are normally executed in the browser and never - // reach this point in interactive mode. In headless mode (Mothership block, no browser) there - // is no client to delegate to, so fall back to the registered server-side handler when one - // exists — otherwise the call would route to executeAppTool and throw "Tool not found". const canUseRegisteredHandler = - isKnownTool(toolId) && - (isSimExecuted(toolId) || (isClientExecuted(toolId) && hasHandler(toolId))) + isKnownTool(toolId) && (isSimExecuted(toolId) || usesHeadlessClientFallback) if (!canUseRegisteredHandler) { const appParams = buildAppToolParams(normalizedParams, context) const options = { diff --git a/apps/sim/lib/mothership/inbox/executor.test.ts b/apps/sim/lib/mothership/inbox/executor.test.ts index f8231124a92..eaaf105c0a9 100644 --- a/apps/sim/lib/mothership/inbox/executor.test.ts +++ b/apps/sim/lib/mothership/inbox/executor.test.ts @@ -119,7 +119,7 @@ const WORKSPACE = { inboxMountedSecrets: ['INBOX_KEY'], } -describe('Inbox raw-secret actor', () => { +describe('Inbox execution actor', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() @@ -150,6 +150,8 @@ describe('Inbox raw-secret actor', () => { expect.objectContaining({ userId: 'member-1', secretActorUserId: 'member-1', + /** Their own, so an emailed request reaches exactly what they could in the app. */ + userPermission: 'write', secretMountPolicy: { secretScope: 'selected', mountedSecrets: ['INBOX_KEY'], @@ -158,10 +160,33 @@ describe('Inbox raw-secret actor', () => { ) }) - it('keeps owner execution fallback but removes raw-secret authority for an external sender', async () => { + it('does not lend a read-only member write authority', async () => { + queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK]) + queueTableRows(schemaMock.workspace, [WORKSPACE]) + queueTableRows(schemaMock.user, [{ id: 'member-1' }]) + mockCheckWorkspaceAccess.mockResolvedValue({ permission: 'read' }) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await executeInboxTask('task-1') + + expect(mockRunHeadlessCopilotLifecycle).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ userId: 'member-1', userPermission: 'read' }) + ) + }) + + /** + * The owner identity is there for billing and workspace reads, not to lend an unknown + * sender the owner's authority. Without the read ceiling the write-gated workflow tools + * would let an allowlisted external correspondent build and run a workflow as the owner, + * which resolves the owner's workspace and personal secrets — the same reach the null + * secret actor already refuses for a direct mount. + */ + it('caps an external sender at read even when the owner is an admin', async () => { queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK]) queueTableRows(schemaMock.workspace, [WORKSPACE]) queueTableRows(schemaMock.user, []) + mockCheckWorkspaceAccess.mockResolvedValue({ permission: 'admin' }) await executeInboxTask('task-1') @@ -170,6 +195,7 @@ describe('Inbox raw-secret actor', () => { expect.objectContaining({ userId: 'owner-1', secretActorUserId: null, + userPermission: 'read', secretMountPolicy: { secretScope: 'selected', mountedSecrets: ['INBOX_KEY'], @@ -178,4 +204,16 @@ describe('Inbox raw-secret actor', () => { ) expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() }) + + it('leaves an external sender with no permission at none rather than promoting to read', async () => { + queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK]) + queueTableRows(schemaMock.workspace, [WORKSPACE]) + queueTableRows(schemaMock.user, []) + mockCheckWorkspaceAccess.mockResolvedValue({ permission: null }) + + await executeInboxTask('task-1') + + const [, options] = mockRunHeadlessCopilotLifecycle.mock.calls[0] + expect(options.userPermission).toBeUndefined() + }) }) diff --git a/apps/sim/lib/mothership/inbox/executor.ts b/apps/sim/lib/mothership/inbox/executor.ts index 02eb799d218..35d21df8cb0 100644 --- a/apps/sim/lib/mothership/inbox/executor.ts +++ b/apps/sim/lib/mothership/inbox/executor.ts @@ -27,7 +27,11 @@ import type { AgentMailAttachment } from '@/lib/mothership/inbox/types' import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key' import { uploadFile } from '@/lib/uploads/core/storage-service' import { createFileContent, type MessageContent } from '@/lib/uploads/utils/file-utils' -import { checkWorkspaceAccess, getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' +import { + checkWorkspaceAccess, + getUserEntityPermissions, + type PermissionType, +} from '@/lib/workspaces/permissions/utils' import { getWorkspaceBilledAccountUserId } from '@/lib/workspaces/utils' const logger = createLogger('InboxExecutor') @@ -216,7 +220,7 @@ export async function executeInboxTask(taskId: string): Promise { } const workspaceAccess = await checkWorkspaceAccess(ws.id, userId) - const userPermission = workspaceAccess.permission + const userPermission = inboxToolPermission(actor, workspaceAccess.permission) const secretMountPolicy = normalizeSecretMountPolicy({ secretScope: ws.inboxSecretScope, mountedSecrets: ws.inboxMountedSecrets, @@ -343,12 +347,49 @@ export async function executeInboxTask(taskId: string): Promise { * Resolve the execution and raw-secret actors independently. Workspace members * execute and mount secrets as themselves. External senders retain the existing * owner execution fallback but receive no raw-secret actor. + * + * The owner fallback exists because billing attribution and workspace reads need + * a real user, not because an unknown sender should act as the owner. A null + * `secretActorUserId` is therefore the run's "no caller" signal, and callers must + * treat it as one everywhere authority is derived — see + * {@link inboxToolPermission}. */ interface InboxExecutionActor { executionUserId: string + /** Null when no workspace member owns this message. */ secretActorUserId: string | null } +/** + * How far an inbox run's tools may reach. + * + * An attributed message uses the sender's own workspace permission, which makes an + * emailed request equivalent to that member performing it in the app — a read-only + * member still cannot run or edit anything. + * + * An unattributed message resolves to the workspace owner so the run has a real + * user for billing and workspace reads, and the owner is typically an admin. Left + * alone, that hands an allowlisted external correspondent the owner's write + * authority: `create_workflow` and `edit_workflow` gate on + * `requiredPermission: 'write'`, and `run_workflow` is gated by the headless + * client-fallback bar in `executeTool` — it carries no catalog permission of its + * own. A workflow built or run through any of them executes with + * `enforceCredentialAccess`, resolving the owner's workspace *and personal* + * secrets. That is the same reach `secretActorUserId: null` already refuses for a + * direct mount, so refusing it here keeps one answer rather than two. + * + * Read is the ceiling rather than no permission at all because answering an + * external correspondent from workspace context is the point of the inbox; only + * mutation and execution are withheld. + */ +function inboxToolPermission( + actor: InboxExecutionActor, + workspacePermission: PermissionType | null +): PermissionType | null { + if (actor.secretActorUserId !== null) return workspacePermission + return workspacePermission === null ? null : 'read' +} + async function resolveInboxExecutionActor( senderEmail: string, ws: { id: string; ownerId: string }