From 17c049ab8d3522956383c6aab710d4132723ff62 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 18:45:05 -0700 Subject: [PATCH 1/2] fix(inbox): stop an unattributed sender inheriting owner write authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveInboxExecutionActor refuses to name a raw-secret actor when the sender matches no workspace member, then hands the run ws.ownerId for everything else. That identity also supplies userPermission, which is what executeTool gates on, so the owner's admin satisfied every requiredPermission check. In headless mode the client-routed workflow tools fall back to their registered server handlers (see the comment in tool-executor/executor.ts), so create_workflow, edit_workflow and run_workflow — all requiredPermission 'write' — were reachable. runWorkflowFromCopilot then executes with enforceCredentialAccess and the owner as actor, which resolves the owner's workspace and personal secrets. An allowlisted external correspondent could therefore reach, through a workflow it had the agent build and run, exactly what the null secret actor refuses for a direct mount. Cap the run's tool permission at read when no member owns the message. An attributed message is unchanged and still uses the sender's own permission, so a read-only member emailing the inbox still cannot run or edit anything. Read rather than none because answering an external correspondent from workspace context is the point of the inbox; only mutation and execution are withheld. The owner identity itself stays: billing attribution and workspace reads need a real user. This separates that need from the authority that came with it. Co-Authored-By: Claude Opus 5 (1M context) --- .../sim/lib/mothership/inbox/executor.test.ts | 42 +++++++++++++++++- apps/sim/lib/mothership/inbox/executor.ts | 43 ++++++++++++++++++- 2 files changed, 81 insertions(+), 4 deletions(-) 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..61bd03bee11 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,47 @@ 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`, `edit_workflow` and `run_workflow` all gate on + * `requiredPermission: 'write'`, and a workflow built and run through 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 } From 6a0ce48f987d51e2dacf208baa912e16e0d766fc Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 19:05:08 -0700 Subject: [PATCH 2/2] fix(copilot): bar the headless client-tool fallback below write 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 in executeTool has no session and runs under the request's principal instead, with nothing standing in for that check. So the read cap from the previous commit did not reach run_workflow, run_workflow_until_block, run_block or run_from_block: all four are route 'client' with no requiredPermission, unlike create_workflow and edit_workflow. An unattributed inbox sender could therefore still run an existing workflow, which executes with enforceCredentialAccess under the workspace owner and resolves the owner's workspace and personal secrets. Derive the requirement at the gate instead: a client-routed tool taking the headless fallback requires write. Interactive callers never reach this branch, so the browser path is unaffected. The catalog itself is generated from the copilot contracts repo and cannot carry this rule, which only applies to the fallback. Also corrects the inboxToolPermission doc, which claimed run_workflow gates on requiredPermission 'write'. It does not; it is gated here. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/tool-executor/executor.test.ts | 36 ++++++++++++++++++- .../sim/lib/copilot/tool-executor/executor.ts | 28 +++++++++++---- apps/sim/lib/mothership/inbox/executor.ts | 8 +++-- 3 files changed, 61 insertions(+), 11 deletions(-) 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.ts b/apps/sim/lib/mothership/inbox/executor.ts index 61bd03bee11..35d21df8cb0 100644 --- a/apps/sim/lib/mothership/inbox/executor.ts +++ b/apps/sim/lib/mothership/inbox/executor.ts @@ -370,9 +370,11 @@ interface InboxExecutionActor { * 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`, `edit_workflow` and `run_workflow` all gate on - * `requiredPermission: 'write'`, and a workflow built and run through them executes - * with `enforceCredentialAccess`, resolving the owner's workspace *and personal* + * 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. *