Skip to content

Commit 21ce1f1

Browse files
committed
fix(mothership): require approval before browser tools act on a page
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 3b6be5c commit 21ce1f1

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
@@ -386,6 +386,22 @@ describe('runGatedToolExecution', () => {
386386
expect(context.toolPermissions.autoAllowed.has('terminal')).toBe(true)
387387
})
388388

389+
it('lets one allow-for-this-chat cover every browser action, not just the one asked about', async () => {
390+
const context = makeContext()
391+
const toolCall = makeToolCall()
392+
toolCall.name = 'browser_click'
393+
toolCall.params = { elementId: 4 }
394+
waitForToolPermissionDecision.mockResolvedValue({
395+
toolCallId: 'call-1',
396+
decision: 'allow_chat',
397+
})
398+
399+
await gate(context, toolCall, () => Promise.resolve({ status: 'success' }), [])
400+
401+
expect(toolCallNeedsApproval('browser_type', context, {}, false, { text: 'hi' })).toBe(false)
402+
expect(toolCallNeedsApproval('terminal', context, {}, false, { operation: 'run' })).toBe(true)
403+
})
404+
389405
it('accepts the normal chat-level decision for code with a secret reference', async () => {
390406
const context = makeContext()
391407
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'
@@ -101,7 +102,7 @@ export function toolCallNeedsApproval(
101102
*/
102103
if (!context.toolPermissions.autoAllowPermitted) return true
103104

104-
return !context.toolPermissions.autoAllowed.has(toolName)
105+
return !context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName))
105106
}
106107

107108
function skipOutput(toolName: string) {
@@ -316,7 +317,7 @@ export function runGatedToolExecution(
316317
* endpoint reads the capability on its own request and refuses the
317318
* durable write.
318319
*/
319-
context.toolPermissions.autoAllowed.add(toolName)
320+
context.toolPermissions.autoAllowed.add(toolApprovalKey(toolName))
320321
}
321322

322323
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
{
@@ -223,6 +228,11 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit
223228
})
224229
})
225230

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

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

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

0 commit comments

Comments
 (0)