close
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion apps/sim/lib/copilot/tool-executor/executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,48 @@ 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)
expect(executeAppTool).not.toHaveBeenCalled()
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)
Expand Down
28 changes: 21 additions & 7 deletions apps/sim/lib/copilot/tool-executor/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,26 @@ export async function executeTool(
params: Record<string, unknown>,
context: ToolExecutionContext
): Promise<ToolExecutionResult> {
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(
Expand All @@ -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 = {
Expand Down
42 changes: 40 additions & 2 deletions apps/sim/lib/mothership/inbox/executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ const WORKSPACE = {
inboxMountedSecrets: ['INBOX_KEY'],
}

describe('Inbox raw-secret actor', () => {
describe('Inbox execution actor', () => {
beforeEach(() => {
vi.clearAllMocks()
resetDbChainMock()
Expand Down Expand Up @@ -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'],
Expand All @@ -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')

Expand All @@ -170,6 +195,7 @@ describe('Inbox raw-secret actor', () => {
expect.objectContaining({
userId: 'owner-1',
secretActorUserId: null,
userPermission: 'read',
secretMountPolicy: {
secretScope: 'selected',
mountedSecrets: ['INBOX_KEY'],
Expand All @@ -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()
})
})
45 changes: 43 additions & 2 deletions apps/sim/lib/mothership/inbox/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -216,7 +220,7 @@ export async function executeInboxTask(taskId: string): Promise<void> {
}

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,
Expand Down Expand Up @@ -343,12 +347,49 @@ export async function executeInboxTask(taskId: string): Promise<void> {
* 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'
}
Comment thread
icecrasher321 marked this conversation as resolved.

async function resolveInboxExecutionActor(
senderEmail: string,
ws: { id: string; ownerId: string }
Expand Down
Loading