diff --git a/apps/sim/app/api/copilot/tool-permission/route.ts b/apps/sim/app/api/copilot/tool-permission/route.ts index 11957bb8f69..86691d296cd 100644 --- a/apps/sim/app/api/copilot/tool-permission/route.ts +++ b/apps/sim/app/api/copilot/tool-permission/route.ts @@ -18,6 +18,7 @@ import { TOOL_PERMISSION_DECISION, type ToolPermissionDecision, } from '@/lib/mothership/persistence/tool-permission' +import { toolApprovalKey } from '@/lib/mothership/persistence/tool-permission/approval-key' import { addAutoAllowedTool, addChatAutoAllowedTool, @@ -113,7 +114,7 @@ async function applyDecision( toolName: claimed.toolName, }) } else if (decision === TOOL_PERMISSION_DECISION.always_allow) { - await addAutoAllowedTool(userId, claimed.toolName).catch((err) => { + await addAutoAllowedTool(userId, toolApprovalKey(claimed.toolName)).catch((err) => { logger.error('Failed to persist always-allow preference', { toolCallId, toolName: claimed.toolName, @@ -121,7 +122,7 @@ async function applyDecision( }) }) } else if (decision === TOOL_PERMISSION_DECISION.allow_chat && run.chatId) { - await addChatAutoAllowedTool(run.chatId, claimed.toolName).catch((err) => { + await addChatAutoAllowedTool(run.chatId, toolApprovalKey(claimed.toolName)).catch((err) => { logger.error('Failed to persist chat-scoped allow preference', { toolCallId, chatId: run.chatId, diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/agent-group/tool-permission-card.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/agent-group/tool-permission-card.tsx index d212fb5584e..fb5afcfa5cc 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/agent-group/tool-permission-card.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/agent-group/tool-permission-card.tsx @@ -1,6 +1,7 @@ 'use client' import { useCallback, useEffect, useState } from 'react' +import { isBrowserToolName } from '@sim/browser-protocol' import { ChevronDown, Chip, @@ -102,6 +103,8 @@ export function ToolPermissionCard({ // Only the first outstanding card offers the bulk action, so N cards do not // each render their own "Allow all". const showBulkActions = outstandingIds.length > 1 && outstandingIds[0] === toolCallId + // The server remembers one answer for every browser action, so say so. + const isBrowserTool = isBrowserToolName(toolName) const submit = useCallback( async (decision: Decision, ids: string[]) => { @@ -198,10 +201,10 @@ export function ToolPermissionCard({ void submit('allow_chat', [toolCallId])}> - For this chat + {isBrowserTool ? 'Browser actions in this chat' : 'For this chat'} void submit('always_allow', [toolCallId])}> - For every chat + {isBrowserTool ? 'Browser actions in every chat' : 'For every chat'} diff --git a/apps/sim/lib/mothership/persistence/tool-permission/approval-key.ts b/apps/sim/lib/mothership/persistence/tool-permission/approval-key.ts new file mode 100644 index 00000000000..eeb33c28864 --- /dev/null +++ b/apps/sim/lib/mothership/persistence/tool-permission/approval-key.ts @@ -0,0 +1,11 @@ +import { isBrowserToolName } from '@sim/browser-protocol' + +/** + * What a "don't ask again" answer is remembered under. Browser tools share one + * entry, so allowing the browser to act covers every action instead of asking + * again for each kind of click. The shared entry is not a tool id, so it can + * never collide with one. + */ +export function toolApprovalKey(toolName: string): string { + return isBrowserToolName(toolName) ? 'browser_*' : toolName +} diff --git a/apps/sim/lib/mothership/request/tools/permission.test.ts b/apps/sim/lib/mothership/request/tools/permission.test.ts index fbf5b3a2be5..a47bc10a90d 100644 --- a/apps/sim/lib/mothership/request/tools/permission.test.ts +++ b/apps/sim/lib/mothership/request/tools/permission.test.ts @@ -395,6 +395,22 @@ describe('runGatedToolExecution', () => { expect(context.toolPermissions.autoAllowed.has('terminal')).toBe(true) }) + it('lets one allow-for-this-chat cover every browser action, not just the one asked about', async () => { + const context = makeContext() + const toolCall = makeToolCall() + toolCall.name = 'browser_click' + toolCall.params = { elementId: 4 } + waitForToolPermissionDecision.mockResolvedValue({ + toolCallId: 'call-1', + decision: 'allow_chat', + }) + + await gate(context, toolCall, () => Promise.resolve({ status: 'success' }), []) + + expect(toolCallNeedsApproval('browser_type', context, {}, false, { text: 'hi' })).toBe(false) + expect(toolCallNeedsApproval('terminal', context, {}, false, { operation: 'run' })).toBe(true) + }) + it('accepts the normal chat-level decision for code with a secret reference', async () => { const context = makeContext() const toolCall = makeToolCall() diff --git a/apps/sim/lib/mothership/request/tools/permission.ts b/apps/sim/lib/mothership/request/tools/permission.ts index 50b5cf0c416..44e4b231f65 100644 --- a/apps/sim/lib/mothership/request/tools/permission.ts +++ b/apps/sim/lib/mothership/request/tools/permission.ts @@ -18,6 +18,7 @@ import { decisionSuppressesFuturePrompts, waitForToolPermissionDecision, } from '@/lib/mothership/persistence/tool-permission' +import { toolApprovalKey } from '@/lib/mothership/persistence/tool-permission/approval-key' import { withCopilotSpan } from '@/lib/mothership/request/otel' import { markToolResultSeen } from '@/lib/mothership/request/sse-utils' import { setTerminalToolCallState } from '@/lib/mothership/request/tool-call-state' @@ -117,7 +118,7 @@ export function toolCallNeedsApproval( */ if (!context.toolPermissions.autoAllowPermitted) return true - return !context.toolPermissions.autoAllowed.has(toolName) + return !context.toolPermissions.autoAllowed.has(toolApprovalKey(toolName)) } function skipOutput(toolName: string) { @@ -332,7 +333,7 @@ export function runGatedToolExecution( * endpoint reads the capability on its own request and refuses the * durable write. */ - context.toolPermissions.autoAllowed.add(toolName) + context.toolPermissions.autoAllowed.add(toolApprovalKey(toolName)) } if (!decisionAllowsExecution(decision.decision)) { diff --git a/apps/sim/lib/mothership/tool-executor/router.ts b/apps/sim/lib/mothership/tool-executor/router.ts index 3ff31c1af35..b90e4f3ee75 100644 --- a/apps/sim/lib/mothership/tool-executor/router.ts +++ b/apps/sim/lib/mothership/tool-executor/router.ts @@ -1,3 +1,4 @@ +import { BROWSER_TOOL_OBSERVES_ONLY, isBrowserToolName } from '@sim/browser-protocol' import { isCopilotToolPermissionsEnabled } from '@/lib/core/config/env-flags' import { TOOL_CATALOG, type ToolCatalogEntry } from '@/lib/mothership/generated/tool-catalog-v1' @@ -21,8 +22,17 @@ export function isKnownTool(toolId: string): boolean { return isToolInCatalog(toolId) } -/** Declared in the mothership tool catalog; Go carries the flag but never enforces it. */ +/** + * Whether a call must wait for the user's approval. + * + * Declared in the mothership tool catalog, which Go carries but never enforces, + * except for browser tools: Sim owns their split between acting on a page and + * only observing it. One that acts does so as the user, in a browser signed in + * to their accounts, so it is gated like a terminal command. Reading the split + * from `BROWSER_TOOL_OBSERVES_ONLY` gates a browser tool added later by default. + */ export function toolRequiresApproval(toolId: string): boolean { + if (isBrowserToolName(toolId)) return !BROWSER_TOOL_OBSERVES_ONLY[toolId] return getToolEntry(toolId)?.requiresApproval === true } diff --git a/apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts b/apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts index 62eeb65d908..72f75ce2e1b 100644 --- a/apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts +++ b/apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts @@ -6,7 +6,7 @@ * durable Stop write, and results arrive through the confirm route. */ import { authMock, authMockFns } from '@sim/testing/mocks/auth.mock' -import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest' const { redisUrl, inheritedEnv } = await vi.hoisted(async () => { const { readTestRedisUrl } = await import('@sim/db/testing/test-infrastructure') @@ -36,6 +36,7 @@ import { closeStreamToolAdmission, requestRunStop, } from '@/lib/mothership/async-runs/repository' +import { getAutoAllowedTools } from '@/lib/mothership/persistence/tool-permission/auto-allow' import { prePersistClientExecutableToolCall } from '@/lib/mothership/request/handlers' import { waitForClientToolCompletion } from '@/lib/mothership/request/tools/client' import { TraceCollector } from '@/lib/mothership/request/trace' @@ -72,7 +73,7 @@ const reportsResult = (toolCallId: string, status: 'success' | 'error' = 'succes data: { ok: true }, }) -const userAnswers = (toolCallId: string, decision: 'allow' | 'skip') => +const userAnswers = (toolCallId: string, decision: 'allow' | 'allow_chat' | 'skip') => post(toolPermissionPOST, '/api/copilot/tool-permission', { decisions: [{ toolCallId, decision }], }) @@ -160,7 +161,11 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit streamComplete: false, wasAborted: false, errors: [], - toolPermissions: { enabled: true, autoAllowed: new Set(), autoAllowPermitted: true }, + toolPermissions: { + enabled: true, + autoAllowed: await getAutoAllowedTools(userId, chatId), + autoAllowPermitted: true, + }, } await prePersistClientExecutableToolCall( { @@ -224,6 +229,11 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit }) }) + /** Turns read the chat's stored allow list, so one test's "don't ask again" must not leak. */ + afterEach(async () => { + await db.update(copilotChats).set({ autoAllowedTools: [] }).where(eq(copilotChats.id, chatId)) + }) + afterAll(async () => { await db.delete(copilotChats).where(eq(copilotChats.id, chatId)) await deleteWorkspaceFixture(db, eq(workspace.id, workspaceId)) @@ -262,6 +272,37 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit expect(await storedCall(toolCallId)).toMatchObject({ status: 'pending', claimedBy: null }) }) + it('refuses to act in the signed-in browser until the user approves it', async () => { + const { runId } = await startRun() + const toolCallId = await agentCalls(runId, 'browser_click', { elementId: 4 }) + + expect((await desktopClaims(toolCallId)).status).toBe(403) + expect(await storedCall(toolCallId)).toMatchObject({ status: 'pending', claimedBy: null }) + + expect((await userAnswers(toolCallId, 'allow')).status).toBe(200) + expect((await desktopClaims(toolCallId)).status).toBe(200) + }) + + it('remembers allowing the browser for this chat across every browser action, and only those', async () => { + const { runId } = await startRun() + const clickId = await agentCalls(runId, 'browser_click', { elementId: 4 }) + expect((await userAnswers(clickId, 'allow_chat')).status).toBe(200) + + const nextTurn = await startRun() + const typeId = await agentCalls(nextTurn.runId, 'browser_type', { elementId: 4, text: 'hi' }) + expect((await desktopClaims(typeId)).status).toBe(200) + + const commandId = await agentCalls(nextTurn.runId, 'terminal', { + operation: 'run', + args: { command: 'ls' }, + }) + const commandClaim = await desktopClaims(commandId) + expect(commandClaim.status).toBe(403) + expect(await commandClaim.json()).toMatchObject({ + error: 'The user has not approved this tool call', + }) + }) + it('refuses a call the user declined before its gate was recorded on the row', async () => { const { runId } = await startRun() const toolCallId = generateId() @@ -371,7 +412,9 @@ describe.runIf(Boolean(redisUrl))('desktop tool calls the server no longer admit it('acknowledges a desktop result that lands after Stop without reviving the call', async () => { const { runId, streamId } = await startRun() - const toolCallId = await desktopIsRunning(runId, 'browser_click') + const toolCallId = await agentCalls(runId, 'browser_click', { elementId: 4 }) + expect((await userAnswers(toolCallId, 'allow')).status).toBe(200) + expect((await desktopClaims(toolCallId)).status).toBe(200) await requestRunStop({ userId, workspaceId, streamId, chatId }) expect(await storedCall(toolCallId)).toMatchObject({ status: 'cancelled',