Skip to content

Commit 468fc3f

Browse files
committed
fix(plan): enforce benchmark and native action admission
1 parent 67ec4cc commit 468fc3f

11 files changed

Lines changed: 108 additions & 23 deletions

File tree

‎apps/sim/app/api/desktop/tool/authorize/route.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,20 @@ describe('desktop tool authorization', () => {
106106
expect(response.status).toBe(403)
107107
})
108108

109+
it.each([false, true])(
110+
'refuses native computer actions through generic desktop authorization with claim %s',
111+
async (claim) => {
112+
getAsyncToolCall.mockResolvedValueOnce({
113+
toolCallId: 'computer-tool',
114+
runId: 'run-1',
115+
status: 'pending',
116+
toolName: 'computer',
117+
args: { action: 'list_apps' },
118+
})
119+
expect((await POST(request('computer-tool', claim))).status).toBe(403)
120+
}
121+
)
122+
109123
it('rejects a replayed browser action after its pending row was claimed', async () => {
110124
getAsyncToolCall.mockResolvedValueOnce({
111125
toolCallId: 'browser-tool',

‎apps/sim/app/api/desktop/tool/authorize/route.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
import {
2121
chatViewDesktopLeaseOwnerToken,
2222
getDesktopToolClaimOwner,
23-
isDesktopToolCall,
23+
isBackgroundDesktopToolCall,
2424
isLocalReadToolCall,
2525
} from '@/lib/mothership/tools/desktop-tools'
2626

@@ -83,7 +83,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
8383
)
8484

8585
const args = isRecordLike(toolCall.args) ? (toolCall.args as Record<string, unknown>) : {}
86-
if (!isDesktopToolCall(toolCall.toolName, args)) {
86+
if (!isBackgroundDesktopToolCall(toolCall.toolName, args)) {
8787
return NextResponse.json(
8888
{ error: 'Tool call is not authorized for desktop execution' },
8989
{ status: 403 }

‎apps/sim/lib/benchmarks/application/stage-lease.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,25 @@ describe('benchmark stage lifetime', () => {
2525
})
2626
afterEach(() => vi.useRealTimers())
2727

28+
it.each([0, -1])(
29+
'does not start work when its initial lease expired %s milliseconds ago',
30+
async (offset) => {
31+
let started = false
32+
await expect(
33+
withBenchmarkStageLease(
34+
{ ...attempt, leaseExpiresAt: new Date(Date.now() + offset) },
35+
undefined,
36+
async () => {
37+
started = true
38+
return 'stale result'
39+
}
40+
)
41+
).rejects.toMatchObject({ code: 'conflict' })
42+
expect(started).toBe(false)
43+
expect(vi.getTimerCount()).toBe(0)
44+
}
45+
)
46+
2847
it('keeps a healthy inspection alive beyond ten minutes and stops its heartbeat on completion', async () => {
2948
const result = withBenchmarkStageLease(attempt, undefined, async (signal) => {
3049
await vi.advanceTimersByTimeAsync(60 * 60_000)

‎apps/sim/lib/benchmarks/application/stage-lease.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,12 @@ export async function withBenchmarkStageLease<T>(
1818
ownership.abort(
1919
new OrchestrationError('conflict', 'This step lost its run ownership. Refresh and retry it.')
2020
)
21-
let expiry = setTimeout(loseOwnership, Math.max(0, attempt.leaseExpiresAt.getTime() - Date.now()))
21+
const remaining = attempt.leaseExpiresAt.getTime() - Date.now()
22+
if (remaining <= 0) {
23+
loseOwnership()
24+
signal.throwIfAborted()
25+
}
26+
let expiry = setTimeout(loseOwnership, remaining)
2227
expiry.unref?.()
2328
let onAbort: (() => void) | undefined
2429
const aborted = new Promise<never>((_resolve, reject) => {

‎apps/sim/lib/computer-use/repository.integration.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ describe.skipIf(!connection)('computer use one-shot admission in PostgreSQL', ()
5454
`CREATE TABLE copilot_runs (id uuid PRIMARY KEY, chat_id uuid NOT NULL, user_id text NOT NULL, status text NOT NULL, tool_admission_closed_at timestamp)`
5555
)
5656
await connection.unsafe(
57-
`CREATE TABLE copilot_async_tool_calls (tool_call_id text PRIMARY KEY, run_id uuid NOT NULL, tool_name text NOT NULL, args jsonb NOT NULL, status text NOT NULL, claimed_by text, claimed_at timestamp, created_at timestamp DEFAULT now(), updated_at timestamp DEFAULT now())`
57+
`CREATE TABLE copilot_async_tool_calls (tool_call_id text PRIMARY KEY, run_id uuid NOT NULL, tool_name text NOT NULL, args jsonb NOT NULL, status text NOT NULL, claimed_by text, claimed_at timestamp, permission_requested_at timestamp, permission_decision text, created_at timestamp DEFAULT now(), updated_at timestamp DEFAULT now())`
5858
)
5959
database.current = drizzle(connection)
6060
})
@@ -80,6 +80,27 @@ describe.skipIf(!connection)('computer use one-shot admission in PostgreSQL', ()
8080
await requireConnection()`UPDATE copilot_runs SET tool_admission_closed_at = now()`
8181
expect(await claimComputerUseTool(input)).toBeNull()
8282
})
83+
it.each([
84+
{ requested: true, decision: null },
85+
{ requested: true, decision: 'skip' },
86+
{ requested: false, decision: 'skip' },
87+
])(
88+
'refuses an unapproved action with gate $requested and decision $decision',
89+
async ({ requested, decision }) => {
90+
await requireConnection()`UPDATE copilot_async_tool_calls SET permission_requested_at = CASE WHEN ${requested} THEN now() ELSE NULL END, permission_decision = ${decision}`
91+
expect(await claimComputerUseTool(input)).toBeNull()
92+
const [row] =
93+
await requireConnection()`SELECT status, claimed_by FROM copilot_async_tool_calls`
94+
expect(row).toEqual({ status: 'pending', claimed_by: null })
95+
}
96+
)
97+
it.each(['allow', 'allow_chat', 'always_allow'])(
98+
'claims an action approved with %s',
99+
async (decision) => {
100+
await requireConnection()`UPDATE copilot_async_tool_calls SET permission_requested_at = now(), permission_decision = ${decision}`
101+
expect(await claimComputerUseTool(input)).toEqual({ args: { action: 'list_apps' } })
102+
}
103+
)
83104
it('refuses old calls and leaves them unclaimed', async () => {
84105
await requireConnection()`UPDATE copilot_async_tool_calls SET created_at = now() - interval '3 minutes'`
85106
expect(await claimComputerUseTool(input)).toBeNull()

‎apps/sim/lib/computer-use/repository.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { copilotAsyncToolCalls, copilotChats, copilotRuns } from '@sim/db/schema
33
import { ComputerUseSchema } from '@sim/desktop-bridge/computer-use'
44
import { omit, toRecord } from '@sim/utils/object'
55
import { and, eq, isNull, notInArray, sql } from 'drizzle-orm'
6+
import { executableToolPermission } from '@/lib/mothership/async-runs/executable-tool-permission'
67
import { DESKTOP_TOOL_CLAIM_OWNER } from '@/lib/mothership/async-runs/lifecycle'
78

89
interface ComputerUseClaim {
@@ -55,7 +56,8 @@ export async function claimComputerUseTool(input: ComputerUseClaim) {
5556
.where(
5657
and(
5758
eq(copilotAsyncToolCalls.toolCallId, input.toolCallId),
58-
eq(copilotAsyncToolCalls.status, 'pending')
59+
eq(copilotAsyncToolCalls.status, 'pending'),
60+
executableToolPermission()
5961
)
6062
)
6163
.returning({ args: copilotAsyncToolCalls.args })

‎apps/sim/lib/mothership/agent-cli/read-only.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,19 @@ describe('benchmark reference workspace inspection', () => {
4949
}
5050
)
5151

52+
it.each(['/api/v2/secrets', '/api/v2/secrets?cursor=next', '/api/v2/secrets/name'])(
53+
'refuses credential values from %s before contacting the source',
54+
async (path) => {
55+
let dispatched = false
56+
const transport = readOnlyCliTransport(async () => {
57+
dispatched = true
58+
return Response.json({ data: 'sensitive fixture' })
59+
})
60+
expect((await transport(`https://sim.test${path}`)).status).toBe(403)
61+
expect(dispatched).toBe(false)
62+
}
63+
)
64+
5265
it('retains paginated reads and table queries without allowing lookalike mutation paths', async () => {
5366
const transport = readOnlyCliTransport(async () => Response.json({ data: 'authorized result' }))
5467
for (const path of ['/api/v2/workflows?cursor=next', '/api/v2/tables/table']) {

‎apps/sim/lib/mothership/agent-cli/read-only.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@ export function readOnlyCliTransport(transport: typeof fetch): typeof fetch {
3333
const request = new Request(input, init)
3434
const path = new URL(request.url).pathname
3535
if (
36-
request.method !== 'GET' &&
37-
!(request.method === 'POST' && /^\/api\/v2\/tables\/[^/]+\/query(?:\/count)?$/.test(path))
36+
/^\/api\/v2\/secrets(?:\/|$)/.test(path) ||
37+
(request.method !== 'GET' &&
38+
!(request.method === 'POST' && /^\/api\/v2\/tables\/[^/]+\/query(?:\/count)?$/.test(path)))
3839
) {
3940
return Response.json(
4041
{
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { copilotAsyncToolCalls } from '@sim/db/schema'
2+
import { and, inArray, isNull, or } from 'drizzle-orm'
3+
import { EXECUTABLE_TOOL_PERMISSION_DECISIONS } from '@/lib/mothership/async-runs/lifecycle'
4+
5+
/** Check permission in the claim update so a concurrent decision cannot be bypassed. */
6+
export function executableToolPermission() {
7+
return or(
8+
and(
9+
isNull(copilotAsyncToolCalls.permissionRequestedAt),
10+
isNull(copilotAsyncToolCalls.permissionDecision)
11+
),
12+
inArray(copilotAsyncToolCalls.permissionDecision, [...EXECUTABLE_TOOL_PERMISSION_DECISIONS])
13+
)
14+
}

‎apps/sim/lib/mothership/async-runs/repository.ts‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import { type ResourceOwner, resourceScopeFromOwner } from '@/lib/core/resource-
3030
import { acquireAdvisoryXactLock } from '@/lib/db/advisory-locks'
3131
import type { SessionProcessIdentity } from '@/lib/execution/remote-sandbox/session-process'
3232
import { AsyncToolCallOwnershipError } from '@/lib/mothership/async-runs/errors'
33+
import { executableToolPermission } from '@/lib/mothership/async-runs/executable-tool-permission'
3334
import {
3435
INTERRUPTED_SIM_TOOL_MESSAGE,
3536
SIM_TOOL_EXECUTION_LEASE_SECONDS,
@@ -836,15 +837,7 @@ export async function claimDesktopToolCall(
836837
isNull(copilotAsyncToolCalls.pickupDeadlineAt),
837838
sql`${copilotAsyncToolCalls.pickupDeadlineAt} > clock_timestamp()`
838839
),
839-
or(
840-
and(
841-
isNull(copilotAsyncToolCalls.permissionRequestedAt),
842-
isNull(copilotAsyncToolCalls.permissionDecision)
843-
),
844-
inArray(copilotAsyncToolCalls.permissionDecision, [
845-
...EXECUTABLE_TOOL_PERMISSION_DECISIONS,
846-
])
847-
)
840+
executableToolPermission()
848841
)
849842
)
850843
.returning({ id: copilotAsyncToolCalls.id })

0 commit comments

Comments
 (0)