Skip to content

Commit 33feaa1

Browse files
fix(inbox): stop an unattributed sender inheriting owner write authority (#7006)
* fix(inbox): stop an unattributed sender inheriting owner write authority 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 66a9a18 commit 33feaa1

4 files changed

Lines changed: 139 additions & 12 deletions

File tree

apps/sim/lib/copilot/tool-executor/executor.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,14 +242,48 @@ describe('copilot tool executor fallback', () => {
242242
const runWorkflowHandler = vi.fn().mockResolvedValue({ success: true, output: { ran: true } })
243243
registerHandler('run_workflow', runWorkflowHandler)
244244

245-
const context = { userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'ws-1' }
245+
const context = {
246+
userId: 'user-1',
247+
workflowId: 'workflow-1',
248+
workspaceId: 'ws-1',
249+
userPermission: 'write',
250+
}
246251
const result = await executeTool('run_workflow', { workflow_input: {} }, context)
247252

248253
expect(runWorkflowHandler).toHaveBeenCalledWith({ workflow_input: {} }, context)
249254
expect(executeAppTool).not.toHaveBeenCalled()
250255
expect(result).toEqual({ success: true, output: { ran: true } })
251256
})
252257

258+
/**
259+
* `run_workflow` carries no catalog permission — the browser path authorizes it through the
260+
* workflow APIs against the caller's own session. The headless fallback has no session, so
261+
* without a bar of its own a deliberately capped run (an unattributed inbox message) would
262+
* still execute workflows under the principal it was capped away from.
263+
*/
264+
it.each([['read'], [undefined]] as const)(
265+
'refuses the headless client fallback for a %s permission',
266+
async (userPermission) => {
267+
isKnownTool.mockReturnValue(true)
268+
isSimExecuted.mockReturnValue(false)
269+
isClientExecuted.mockReturnValue(true)
270+
271+
const runWorkflowHandler = vi.fn().mockResolvedValue({ success: true })
272+
registerHandler('run_workflow', runWorkflowHandler)
273+
274+
const result = await executeTool(
275+
'run_workflow',
276+
{ workflow_input: {} },
277+
{ userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'ws-1', userPermission }
278+
)
279+
280+
expect(result.success).toBe(false)
281+
expect(result.error).toContain('requires write access')
282+
expect(runWorkflowHandler).not.toHaveBeenCalled()
283+
expect(executeAppTool).not.toHaveBeenCalled()
284+
}
285+
)
286+
253287
it('falls back to app tool executor for client-routed tools with no registered handler', async () => {
254288
isKnownTool.mockReturnValue(true)
255289
isSimExecuted.mockReturnValue(false)

apps/sim/lib/copilot/tool-executor/executor.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,26 @@ export async function executeTool(
3838
params: Record<string, unknown>,
3939
context: ToolExecutionContext
4040
): Promise<ToolExecutionResult> {
41-
const requiredPermission = getToolEntry(toolId)?.requiredPermission
41+
// Client-routed tools (e.g. run_workflow) are normally executed in the browser and never
42+
// reach this point in interactive mode. In headless mode (Mothership block, no browser) there
43+
// is no client to delegate to, so fall back to the registered server-side handler when one
44+
// exists — otherwise the call would route to executeAppTool and throw "Tool not found".
45+
const usesHeadlessClientFallback = isClientExecuted(toolId) && hasHandler(toolId)
46+
47+
/**
48+
* Client-routed tools carry no catalog `requiredPermission` because the browser runs them
49+
* through the workflow APIs, which authorize the caller's own session. The headless fallback
50+
* has no session to authorize against and runs under the request's principal instead, so it
51+
* has to supply a bar of its own.
52+
*
53+
* Without one, a run whose permission was deliberately capped still reaches `run_workflow`,
54+
* and `runWorkflowFromCopilot` executes with `enforceCredentialAccess` — resolving the
55+
* principal's workspace and personal secrets. That is the hole an unattributed inbox message
56+
* leaves open: `resolveInboxExecutionActor` refuses it a secret actor, but the run still
57+
* carries the workspace owner as principal.
58+
*/
59+
const requiredPermission =
60+
getToolEntry(toolId)?.requiredPermission ?? (usesHeadlessClientFallback ? 'write' : undefined)
4261
if (
4362
requiredPermission &&
4463
!permissionSatisfies(
@@ -54,13 +73,8 @@ export async function executeTool(
5473

5574
const normalizedParams = normalizeToolParams(toolId, params, context)
5675

57-
// Client-routed tools (e.g. run_workflow) are normally executed in the browser and never
58-
// reach this point in interactive mode. In headless mode (Mothership block, no browser) there
59-
// is no client to delegate to, so fall back to the registered server-side handler when one
60-
// exists — otherwise the call would route to executeAppTool and throw "Tool not found".
6176
const canUseRegisteredHandler =
62-
isKnownTool(toolId) &&
63-
(isSimExecuted(toolId) || (isClientExecuted(toolId) && hasHandler(toolId)))
77+
isKnownTool(toolId) && (isSimExecuted(toolId) || usesHeadlessClientFallback)
6478
if (!canUseRegisteredHandler) {
6579
const appParams = buildAppToolParams(normalizedParams, context)
6680
const options = {

apps/sim/lib/mothership/inbox/executor.test.ts

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ const WORKSPACE = {
119119
inboxMountedSecrets: ['INBOX_KEY'],
120120
}
121121

122-
describe('Inbox raw-secret actor', () => {
122+
describe('Inbox execution actor', () => {
123123
beforeEach(() => {
124124
vi.clearAllMocks()
125125
resetDbChainMock()
@@ -150,6 +150,8 @@ describe('Inbox raw-secret actor', () => {
150150
expect.objectContaining({
151151
userId: 'member-1',
152152
secretActorUserId: 'member-1',
153+
/** Their own, so an emailed request reaches exactly what they could in the app. */
154+
userPermission: 'write',
153155
secretMountPolicy: {
154156
secretScope: 'selected',
155157
mountedSecrets: ['INBOX_KEY'],
@@ -158,10 +160,33 @@ describe('Inbox raw-secret actor', () => {
158160
)
159161
})
160162

161-
it('keeps owner execution fallback but removes raw-secret authority for an external sender', async () => {
163+
it('does not lend a read-only member write authority', async () => {
164+
queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK])
165+
queueTableRows(schemaMock.workspace, [WORKSPACE])
166+
queueTableRows(schemaMock.user, [{ id: 'member-1' }])
167+
mockCheckWorkspaceAccess.mockResolvedValue({ permission: 'read' })
168+
mockGetUserEntityPermissions.mockResolvedValue('read')
169+
170+
await executeInboxTask('task-1')
171+
172+
expect(mockRunHeadlessCopilotLifecycle).toHaveBeenCalledWith(
173+
expect.any(Object),
174+
expect.objectContaining({ userId: 'member-1', userPermission: 'read' })
175+
)
176+
})
177+
178+
/**
179+
* The owner identity is there for billing and workspace reads, not to lend an unknown
180+
* sender the owner's authority. Without the read ceiling the write-gated workflow tools
181+
* would let an allowlisted external correspondent build and run a workflow as the owner,
182+
* which resolves the owner's workspace and personal secrets — the same reach the null
183+
* secret actor already refuses for a direct mount.
184+
*/
185+
it('caps an external sender at read even when the owner is an admin', async () => {
162186
queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK])
163187
queueTableRows(schemaMock.workspace, [WORKSPACE])
164188
queueTableRows(schemaMock.user, [])
189+
mockCheckWorkspaceAccess.mockResolvedValue({ permission: 'admin' })
165190

166191
await executeInboxTask('task-1')
167192

@@ -170,6 +195,7 @@ describe('Inbox raw-secret actor', () => {
170195
expect.objectContaining({
171196
userId: 'owner-1',
172197
secretActorUserId: null,
198+
userPermission: 'read',
173199
secretMountPolicy: {
174200
secretScope: 'selected',
175201
mountedSecrets: ['INBOX_KEY'],
@@ -178,4 +204,16 @@ describe('Inbox raw-secret actor', () => {
178204
)
179205
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
180206
})
207+
208+
it('leaves an external sender with no permission at none rather than promoting to read', async () => {
209+
queueTableRows(schemaMock.mothershipInboxTask, [INBOX_TASK])
210+
queueTableRows(schemaMock.workspace, [WORKSPACE])
211+
queueTableRows(schemaMock.user, [])
212+
mockCheckWorkspaceAccess.mockResolvedValue({ permission: null })
213+
214+
await executeInboxTask('task-1')
215+
216+
const [, options] = mockRunHeadlessCopilotLifecycle.mock.calls[0]
217+
expect(options.userPermission).toBeUndefined()
218+
})
181219
})

apps/sim/lib/mothership/inbox/executor.ts

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ import type { AgentMailAttachment } from '@/lib/mothership/inbox/types'
2727
import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key'
2828
import { uploadFile } from '@/lib/uploads/core/storage-service'
2929
import { createFileContent, type MessageContent } from '@/lib/uploads/utils/file-utils'
30-
import { checkWorkspaceAccess, getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
30+
import {
31+
checkWorkspaceAccess,
32+
getUserEntityPermissions,
33+
type PermissionType,
34+
} from '@/lib/workspaces/permissions/utils'
3135
import { getWorkspaceBilledAccountUserId } from '@/lib/workspaces/utils'
3236

3337
const logger = createLogger('InboxExecutor')
@@ -216,7 +220,7 @@ export async function executeInboxTask(taskId: string): Promise<void> {
216220
}
217221

218222
const workspaceAccess = await checkWorkspaceAccess(ws.id, userId)
219-
const userPermission = workspaceAccess.permission
223+
const userPermission = inboxToolPermission(actor, workspaceAccess.permission)
220224
const secretMountPolicy = normalizeSecretMountPolicy({
221225
secretScope: ws.inboxSecretScope,
222226
mountedSecrets: ws.inboxMountedSecrets,
@@ -343,12 +347,49 @@ export async function executeInboxTask(taskId: string): Promise<void> {
343347
* Resolve the execution and raw-secret actors independently. Workspace members
344348
* execute and mount secrets as themselves. External senders retain the existing
345349
* owner execution fallback but receive no raw-secret actor.
350+
*
351+
* The owner fallback exists because billing attribution and workspace reads need
352+
* a real user, not because an unknown sender should act as the owner. A null
353+
* `secretActorUserId` is therefore the run's "no caller" signal, and callers must
354+
* treat it as one everywhere authority is derived — see
355+
* {@link inboxToolPermission}.
346356
*/
347357
interface InboxExecutionActor {
348358
executionUserId: string
359+
/** Null when no workspace member owns this message. */
349360
secretActorUserId: string | null
350361
}
351362

363+
/**
364+
* How far an inbox run's tools may reach.
365+
*
366+
* An attributed message uses the sender's own workspace permission, which makes an
367+
* emailed request equivalent to that member performing it in the app — a read-only
368+
* member still cannot run or edit anything.
369+
*
370+
* An unattributed message resolves to the workspace owner so the run has a real
371+
* user for billing and workspace reads, and the owner is typically an admin. Left
372+
* alone, that hands an allowlisted external correspondent the owner's write
373+
* authority: `create_workflow` and `edit_workflow` gate on
374+
* `requiredPermission: 'write'`, and `run_workflow` is gated by the headless
375+
* client-fallback bar in `executeTool` — it carries no catalog permission of its
376+
* own. A workflow built or run through any of them executes with
377+
* `enforceCredentialAccess`, resolving the owner's workspace *and personal*
378+
* secrets. That is the same reach `secretActorUserId: null` already refuses for a
379+
* direct mount, so refusing it here keeps one answer rather than two.
380+
*
381+
* Read is the ceiling rather than no permission at all because answering an
382+
* external correspondent from workspace context is the point of the inbox; only
383+
* mutation and execution are withheld.
384+
*/
385+
function inboxToolPermission(
386+
actor: InboxExecutionActor,
387+
workspacePermission: PermissionType | null
388+
): PermissionType | null {
389+
if (actor.secretActorUserId !== null) return workspacePermission
390+
return workspacePermission === null ? null : 'read'
391+
}
392+
352393
async function resolveInboxExecutionActor(
353394
senderEmail: string,
354395
ws: { id: string; ownerId: string }

0 commit comments

Comments
 (0)