Skip to content

Commit ebf29c0

Browse files
authored
fix(mothership): require approval before browser tools act on a page (#9581)
Browser tools that act on a page (navigate, click, type, submit, upload, ...) run as the user in a browser carrying their signed-in sessions, but the approval gate read only the catalog flag, which no browser tool declares, so none of them ever prompted. toolRequiresApproval now gates every browser tool that BROWSER_TOOL_OBSERVES_ONLY does not mark observe-only, so a browser tool added later is gated by default, and both the dispatch gate and the in-band lane read the same answer. "Don't ask again" answers for browser tools are stored under one shared key, so a single answer covers the browser instead of one prompt per kind of action.
1 parent 70be4b7 commit ebf29c0

7 files changed

Lines changed: 96 additions & 11 deletions

File tree

‎apps/sim/app/api/copilot/tool-permission/route.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
TOOL_PERMISSION_DECISION,
1919
type ToolPermissionDecision,
2020
} from '@/lib/mothership/persistence/tool-permission'
21+
import { toolApprovalKey } from '@/lib/mothership/persistence/tool-permission/approval-key'
2122
import {
2223
addAutoAllowedTool,
2324
addChatAutoAllowedTool,
@@ -113,15 +114,15 @@ async function applyDecision(
113114
toolName: claimed.toolName,
114115
})
115116
} else if (decision === TOOL_PERMISSION_DECISION.always_allow) {
116-
await addAutoAllowedTool(userId, claimed.toolName).catch((err) => {
117+
await addAutoAllowedTool(userId, toolApprovalKey(claimed.toolName)).catch((err) => {
117118
logger.error('Failed to persist always-allow preference', {
118119
toolCallId,
119120
toolName: claimed.toolName,
120121
error: getErrorMessage(err),
121122
})
122123
})
123124
} else if (decision === TOOL_PERMISSION_DECISION.allow_chat && run.chatId) {
124-
await addChatAutoAllowedTool(run.chatId, claimed.toolName).catch((err) => {
125+
await addChatAutoAllowedTool(run.chatId, toolApprovalKey(claimed.toolName)).catch((err) => {
125126
logger.error('Failed to persist chat-scoped allow preference', {
126127
toolCallId,
127128
chatId: run.chatId,

‎apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/agent-group/tool-permission-card.tsx‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use client'
22

33
import { useCallback, useEffect, useState } from 'react'
4+
import { isBrowserToolName } from '@sim/browser-protocol'
45
import {
56
ChevronDown,
67
Chip,
@@ -102,6 +103,8 @@ export function ToolPermissionCard({
102103
// Only the first outstanding card offers the bulk action, so N cards do not
103104
// each render their own "Allow all".
104105
const showBulkActions = outstandingIds.length > 1 && outstandingIds[0] === toolCallId
106+
// The server remembers one answer for every browser action, so say so.
107+
const isBrowserTool = isBrowserToolName(toolName)
105108

106109
const submit = useCallback(
107110
async (decision: Decision, ids: string[]) => {
@@ -198,10 +201,10 @@ export function ToolPermissionCard({
198201
</DropdownMenuTrigger>
199202
<DropdownMenuContent align='start'>
200203
<DropdownMenuItem onSelect={() => void submit('allow_chat', [toolCallId])}>
201-
For this chat
204+
{isBrowserTool ? 'Browser actions in this chat' : 'For this chat'}
202205
</DropdownMenuItem>
203206
<DropdownMenuItem onSelect={() => void submit('always_allow', [toolCallId])}>
204-
For every chat
207+
{isBrowserTool ? 'Browser actions in every chat' : 'For every chat'}
205208
</DropdownMenuItem>
206209
</DropdownMenuContent>
207210
</DropdownMenu>
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import { isBrowserToolName } from '@sim/browser-protocol'
2+
3+
/**
4+
* What a "don't ask again" answer is remembered under. Browser tools share one
5+
* entry, so allowing the browser to act covers every action instead of asking
6+
* again for each kind of click. The shared entry is not a tool id, so it can
7+
* never collide with one.
8+
*/
9+
export function toolApprovalKey(toolName: string): string {
10+
return isBrowserToolName(toolName) ? 'browser_*' : toolName
11+
}

‎apps/sim/lib/mothership/request/tools/permission.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,22 @@ describe('runGatedToolExecution', () => {
395395
expect(context.toolPermissions.autoAllowed.has('terminal')).toBe(true)
396396
})
397397

398+
it('lets one allow-for-this-chat cover every browser action, not just the one asked about', async () => {
399+
const context = makeContext()
400+
const toolCall = makeToolCall()
401+
toolCall.name = 'browser_click'
402+
toolCall.params = { elementId: 4 }
403+
waitForToolPermissionDecision.mockResolvedValue({
404+
toolCallId: 'call-1',
405+
decision: 'allow_chat',
406+
})
407+
408+
await gate(context, toolCall, () => Promise.resolve({ status: 'success' }), [])
409+
410+
expect(toolCallNeedsApproval('browser_type', context, {}, false, { text: 'hi' })).toBe(false)
411+
expect(toolCallNeedsApproval('terminal', context, {}, false, { operation: 'run' })).toBe(true)
412+
})
413+
398414
it('accepts the normal chat-level decision for code with a secret reference', async () => {
399415
const context = makeContext()
400416
const toolCall = makeToolCall()

‎apps/sim/lib/mothership/request/tools/permission.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
decisionSuppressesFuturePrompts,
1919
waitForToolPermissionDecision,
2020
} from '@/lib/mothership/persistence/tool-permission'
21+
import { toolApprovalKey } from '@/lib/mothership/persistence/tool-permission/approval-key'
2122
import { withCopilotSpan } from '@/lib/mothership/request/otel'
2223
import { markToolResultSeen } from '@/lib/mothership/request/sse-utils'
2324
import { setTerminalToolCallState } from '@/lib/mothership/request/tool-call-state'
@@ -117,7 +118,7 @@ export function toolCallNeedsApproval(
117118
*/
118119
if (!context.toolPermissions.autoAllowPermitted) return true
119120

120-
return !context.toolPermissions.autoAllowed.has(toolName)
121+
return !context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName))
121122
}
122123

123124
function skipOutput(toolName: string) {
@@ -332,7 +333,7 @@ export function runGatedToolExecution(
332333
* endpoint reads the capability on its own request and refuses the
333334
* durable write.
334335
*/
335-
context.toolPermissions.autoAllowed.add(toolName)
336+
context.toolPermissions.autoAllowed.add(toolApprovalKey(toolName))
336337
}
337338

338339
if (!decisionAllowsExecution(decision.decision)) {

‎apps/sim/lib/mothership/tool-executor/router.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { BROWSER_TOOL_OBSERVES_ONLY, isBrowserToolName } from '@sim/browser-protocol'
12
import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags'
23
import { TOOL_CATALOG, type ToolCatalogEntry } from '@/lib/mothership/generated/tool-catalog-v1'
34

@@ -21,8 +22,17 @@ export function isKnownTool(toolId: string): boolean {
2122
return isToolInCatalog(toolId)
2223
}
2324

24-
/** Declared in the mothership tool catalog; Go carries the flag but never enforces it. */
25+
/**
26+
* Whether a call must wait for the user's approval.
27+
*
28+
* Declared in the mothership tool catalog, which Go carries but never enforces,
29+
* except for browser tools: Sim owns their split between acting on a page and
30+
* only observing it. One that acts does so as the user, in a browser signed in
31+
* to their accounts, so it is gated like a terminal command. Reading the split
32+
* from `BROWSER_TOOL_OBSERVES_ONLY` gates a browser tool added later by default.
33+
*/
2534
export function toolRequiresApproval(toolId: string): boolean {
35+
if (isBrowserToolName(toolId)) return !BROWSER_TOOL_OBSERVES_ONLY[toolId]
2636
return getToolEntry(toolId)?.requiresApproval === true
2737
}
2838

‎apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts‎

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* durable Stop write, and results arrive through the confirm route.
77
*/
88
import { authMock, authMockFns } from '@sim/testing/mocks/auth.mock'
9-
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
9+
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'
1010

1111
const { redisUrl, inheritedEnv } = await vi.hoisted(async () => {
1212
const { readTestRedisUrl } = await import('@sim/db/testing/test-infrastructure')
@@ -36,6 +36,7 @@ import {
3636
closeStreamToolAdmission,
3737
requestRunStop,
3838
} from '@/lib/mothership/async-runs/repository'
39+
import { getAutoAllowedTools } from '@/lib/mothership/persistence/tool-permission/auto-allow'
3940
import { prePersistClientExecutableToolCall } from '@/lib/mothership/request/handlers'
4041
import { waitForClientToolCompletion } from '@/lib/mothership/request/tools/client'
4142
import { TraceCollector } from '@/lib/mothership/request/trace'
@@ -72,7 +73,7 @@ const reportsResult = (toolCallId: string, status: 'success' | 'error' = 'succes
7273
data: { ok: true },
7374
})
7475

75-
const userAnswers = (toolCallId: string, decision: 'allow' | 'skip') =>
76+
const userAnswers = (toolCallId: string, decision: 'allow' | 'allow_chat' | 'skip') =>
7677
post(toolPermissionPOST, '/api/copilot/tool-permission', {
7778
decisions: [{ toolCallId, decision }],
7879
})
@@ -160,7 +161,11 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit
160161
streamComplete: false,
161162
wasAborted: false,
162163
errors: [],
163-
toolPermissions: { enabled: true, autoAllowed: new Set(), autoAllowPermitted: true },
164+
toolPermissions: {
165+
enabled: true,
166+
autoAllowed: await getAutoAllowedTools(userId, chatId),
167+
autoAllowPermitted: true,
168+
},
164169
}
165170
await prePersistClientExecutableToolCall(
166171
{
@@ -224,6 +229,11 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit
224229
})
225230
})
226231

232+
/** Turns read the chat's stored allow list, so one test's "don't ask again" must not leak. */
233+
afterEach(async () => {
234+
await db.update(copilotChats).set({ autoAllowedTools: [] }).where(eq(copilotChats.id, chatId))
235+
})
236+
227237
afterAll(async () => {
228238
await db.delete(copilotChats).where(eq(copilotChats.id, chatId))
229239
await deleteWorkspaceFixture(db, eq(workspace.id, workspaceId))
@@ -262,6 +272,37 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit
262272
expect(await storedCall(toolCallId)).toMatchObject({ status: 'pending', claimedBy: null })
263273
})
264274

275+
it('refuses to act in the signed-in browser until the user approves it', async () => {
276+
const { runId } = await startRun()
277+
const toolCallId = await agentCalls(runId, 'browser_click', { elementId: 4 })
278+
279+
expect((await desktopClaims(toolCallId)).status).toBe(403)
280+
expect(await storedCall(toolCallId)).toMatchObject({ status: 'pending', claimedBy: null })
281+
282+
expect((await userAnswers(toolCallId, 'allow')).status).toBe(200)
283+
expect((await desktopClaims(toolCallId)).status).toBe(200)
284+
})
285+
286+
it('remembers allowing the browser for this chat across every browser action, and only those', async () => {
287+
const { runId } = await startRun()
288+
const clickId = await agentCalls(runId, 'browser_click', { elementId: 4 })
289+
expect((await userAnswers(clickId, 'allow_chat')).status).toBe(200)
290+
291+
const nextTurn = await startRun()
292+
const typeId = await agentCalls(nextTurn.runId, 'browser_type', { elementId: 4, text: 'hi' })
293+
expect((await desktopClaims(typeId)).status).toBe(200)
294+
295+
const commandId = await agentCalls(nextTurn.runId, 'terminal', {
296+
operation: 'run',
297+
args: { command: 'ls' },
298+
})
299+
const commandClaim = await desktopClaims(commandId)
300+
expect(commandClaim.status).toBe(403)
301+
expect(await commandClaim.json()).toMatchObject({
302+
error: 'The user has not approved this tool call',
303+
})
304+
})
305+
265306
it('refuses a call the user declined before its gate was recorded on the row', async () => {
266307
const { runId } = await startRun()
267308
const toolCallId = generateId()
@@ -371,7 +412,9 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit
371412

372413
it('acknowledges a desktop result that lands after Stop without reviving the call', async () => {
373414
const { runId, streamId } = await startRun()
374-
const toolCallId = await desktopIsRunning(runId, 'browser_click')
415+
const toolCallId = await agentCalls(runId, 'browser_click', { elementId: 4 })
416+
expect((await userAnswers(toolCallId, 'allow')).status).toBe(200)
417+
expect((await desktopClaims(toolCallId)).status).toBe(200)
375418
await requestRunStop({ userId, workspaceId, streamId, chatId })
376419
expect(await storedCall(toolCallId)).toMatchObject({
377420
status: 'cancelled',

0 commit comments

Comments
 (0)