Skip to content

Commit df6159d

Browse files
committed
refactor(copilot): align manage_sandbox handler with the manage_* family
- Gate writes with copilotToolCanAdmin(context.userPermission), the admin sibling of the copilotToolCanWrite helper manage_custom_tool and manage_skill already use, instead of a second DB permission read. The tool now declares RequiredPermission "write", so the executor has already resolved the caller's permission by the time the handler runs. - Trim comments that restated the code, per the family's near-zero density.
1 parent 42d8d6f commit df6159d

7 files changed

Lines changed: 29 additions & 44 deletions

File tree

apps/sim/app/api/workspaces/[id]/sandboxes/authorize.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ export interface SandboxMutationActor {
1717
}
1818

1919
/**
20-
* Maps a refused write onto the status code the editor expects. Shared by both
21-
* route files so the create path and the edit/delete path cannot describe the
22-
* same failure differently.
23-
*
2420
* `invalid_dependencies` carries a line number per rejected row, which the
2521
* generic validation error does not — the editor marks those inline.
2622
*/

apps/sim/lib/copilot/generated/tool-catalog-v1.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3646,6 +3646,7 @@ export const ManageSandbox: ToolCatalogEntry = {
36463646
},
36473647
required: ['operation'],
36483648
},
3649+
requiredPermission: 'write',
36493650
}
36503651

36513652
export const ManageScheduledTask: ToolCatalogEntry = {

apps/sim/lib/copilot/tools/handlers/management/manage-sandbox.test.ts

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
66
import type { ExecutionContext } from '@/lib/copilot/request/types'
77

88
const {
9-
getUserEntityPermissionsMock,
109
hasWorkspaceSandboxAccessMock,
1110
enforceWorkspaceRateLimitMock,
1211
createWorkspaceSandboxMock,
1312
updateWorkspaceSandboxMock,
1413
deleteWorkspaceSandboxMock,
1514
listWorkspaceSandboxesMock,
1615
} = vi.hoisted(() => ({
17-
getUserEntityPermissionsMock: vi.fn(),
1816
hasWorkspaceSandboxAccessMock: vi.fn(),
1917
enforceWorkspaceRateLimitMock: vi.fn(),
2018
createWorkspaceSandboxMock: vi.fn(),
@@ -23,10 +21,6 @@ const {
2321
listWorkspaceSandboxesMock: vi.fn(),
2422
}))
2523

26-
vi.mock('@/lib/workspaces/permissions/utils', () => ({
27-
getUserEntityPermissions: getUserEntityPermissionsMock,
28-
}))
29-
3024
vi.mock('@/lib/billing/core/subscription', () => ({
3125
hasWorkspaceSandboxAccess: hasWorkspaceSandboxAccessMock,
3226
}))
@@ -48,7 +42,12 @@ vi.mock('@/lib/execution/remote-sandbox/workspace-sandboxes', () => ({
4842

4943
import { executeManageSandbox } from '@/lib/copilot/tools/handlers/management/manage-sandbox'
5044

51-
const context = { userId: 'user-1', workflowId: 'wf-1', workspaceId: 'ws-1' } as ExecutionContext
45+
const context = {
46+
userId: 'user-1',
47+
workflowId: 'wf-1',
48+
workspaceId: 'ws-1',
49+
userPermission: 'admin',
50+
} as ExecutionContext
5251

5352
const sandbox = {
5453
id: 'sb-1',
@@ -67,7 +66,6 @@ const sandbox = {
6766
describe('manage_sandbox', () => {
6867
beforeEach(() => {
6968
vi.clearAllMocks()
70-
getUserEntityPermissionsMock.mockResolvedValue('admin')
7169
hasWorkspaceSandboxAccessMock.mockResolvedValue(true)
7270
enforceWorkspaceRateLimitMock.mockResolvedValue(null)
7371
listWorkspaceSandboxesMock.mockResolvedValue([sandbox])
@@ -84,14 +82,14 @@ describe('manage_sandbox', () => {
8482

8583
it('ignores a model-supplied workspaceId and uses the server context', async () => {
8684
await executeManageSandbox({ operation: 'list', workspaceId: 'other-ws' }, context)
87-
expect(getUserEntityPermissionsMock).toHaveBeenCalledWith('user-1', 'workspace', 'ws-1')
8885
expect(listWorkspaceSandboxesMock).toHaveBeenCalledWith('ws-1')
8986
})
9087

91-
it('lists with only read access, and does not spend the mutation budget', async () => {
92-
getUserEntityPermissionsMock.mockResolvedValue('read')
93-
94-
const result = await executeManageSandbox({ operation: 'list' }, context)
88+
it('lists without spending the mutation budget or the plan check', async () => {
89+
const result = await executeManageSandbox({ operation: 'list' }, {
90+
...context,
91+
userPermission: 'write',
92+
} as ExecutionContext)
9593
expect(result.success).toBe(true)
9694
expect(result.output).toMatchObject({ count: 1, strategy: 'prebuilt' })
9795
const [listed] = (result.output as { sandboxes: Record<string, unknown>[] }).sandboxes
@@ -102,11 +100,9 @@ describe('manage_sandbox', () => {
102100
})
103101

104102
it.each(['add', 'edit', 'delete'])('requires workspace admin to %s', async (operation) => {
105-
getUserEntityPermissionsMock.mockResolvedValue('write')
106-
107103
const result = await executeManageSandbox(
108104
{ operation, name: 'x', language: 'python', sandboxId: 'sb-1' },
109-
context
105+
{ ...context, userPermission: 'write' } as ExecutionContext
110106
)
111107

112108
expect(result.success).toBe(false)

apps/sim/lib/copilot/tools/handlers/management/manage-sandbox.ts

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { getErrorMessage, toError } from '@sim/utils/errors'
33
import { hasWorkspaceSandboxAccess } from '@/lib/billing/core/subscription'
44
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
5+
import { copilotToolCanAdmin } from '@/lib/copilot/tools/permissions'
56
import { enforceWorkspaceRateLimit } from '@/lib/core/rate-limiter/route-helpers'
67
import {
78
isSandboxLanguage,
@@ -19,7 +20,6 @@ import {
1920
type SandboxWriteFailure,
2021
updateWorkspaceSandbox,
2122
} from '@/lib/execution/remote-sandbox/workspace-sandboxes'
22-
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
2323

2424
const logger = createLogger('CopilotToolExecutor')
2525

@@ -69,7 +69,7 @@ function isStringArray(value: unknown): value is string[] {
6969
/**
7070
* Reproduces the REST routes' gate — workspace admin, plan entitlement, then the
7171
* shared mutation budget — so chat cannot create a sandbox the same user could
72-
* not create in Settings > Sandboxes. `list` needs only read, matching GET.
72+
* not create in Settings > Sandboxes.
7373
*/
7474
export async function executeManageSandbox(
7575
rawParams: Record<string, unknown>,
@@ -91,13 +91,8 @@ export async function executeManageSandbox(
9191
const isWrite = WRITE_OPERATIONS.includes(operation)
9292

9393
try {
94-
const permission = await getUserEntityPermissions(context.userId, 'workspace', workspaceId)
95-
if (!permission) {
96-
return { success: false, error: 'You do not have access to this workspace' }
97-
}
98-
9994
if (isWrite) {
100-
if (permission !== 'admin') {
95+
if (!copilotToolCanAdmin(context.userPermission)) {
10196
return { success: false, error: SANDBOX_ADMIN_REQUIRED }
10297
}
10398
if (!(await hasWorkspaceSandboxAccess(workspaceId))) {

apps/sim/lib/copilot/tools/permissions.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,14 @@ export function copilotToolCanWrite(userPermission: string | null | undefined):
99
return permissionSatisfies((userPermission ?? null) as PermissionType | null, 'write')
1010
}
1111

12+
/**
13+
* Whether a copilot tool call may perform an admin-only action. Same fail-closed
14+
* contract as {@link copilotToolCanWrite}.
15+
*/
16+
export function copilotToolCanAdmin(userPermission: string | null | undefined): boolean {
17+
return permissionSatisfies((userPermission ?? null) as PermissionType | null, 'admin')
18+
}
19+
1220
/** Renders the denial message shared by both copilot execution paths. */
1321
export function copilotWriteDeniedMessage(
1422
toolName: string,

apps/sim/lib/execution/remote-sandbox/workspace-sandboxes.test.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,12 +80,6 @@ describe('workspace sandbox operations', () => {
8080
)
8181
})
8282

83-
/**
84-
* The regression this guards: `nextName = name ?? existing.name` treated a
85-
* whitespace-only name as "supplied", so it trimmed to empty, skipped the
86-
* conflict pre-check (falsy), and wrote an unnamed sandbox the UI cannot
87-
* create and the user cannot select.
88-
*/
8983
it('refuses a whitespace-only name on edit instead of writing it', async () => {
9084
queueTableRows(workspaceSandbox, [existingRow])
9185

apps/sim/lib/execution/remote-sandbox/workspace-sandboxes.ts

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,6 @@ function isSandboxNameConflictError(error: unknown): boolean {
228228
return message.includes(WORKSPACE_SANDBOX_NAME_INDEX) || message.includes('23505')
229229
}
230230

231-
/** Why a write was refused, rendered by each caller for its own surface. */
232231
export type SandboxWriteFailure =
233232
| { code: 'invalid_name'; message: string }
234233
| { code: 'name_conflict'; name: string }
@@ -267,17 +266,15 @@ async function readBackOrFail(workspaceId: string, sandboxId: string): Promise<S
267266

268267
export interface CreateWorkspaceSandboxParams {
269268
workspaceId: string
270-
/** Attributed as `createdBy`; the caller has already authorized this actor. */
271269
userId: string
272270
name: string
273271
language: SandboxLanguage
274-
/** Raw submitted lines — comments and blanks are stripped during validation. */
275272
dependencies: readonly string[]
276273
}
277274

278275
/**
279-
* Creates a sandbox and enqueues its build. Authorization, entitlement, and rate
280-
* limiting are the caller's job — the route and the copilot tool differ there.
276+
* Authorization, entitlement, and rate limiting are the caller's job — the route
277+
* and the copilot tool differ there.
281278
*/
282279
export async function createWorkspaceSandbox(
283280
params: CreateWorkspaceSandboxParams
@@ -328,8 +325,6 @@ export interface UpdateWorkspaceSandboxParams {
328325
}
329326

330327
/**
331-
* Applies a partial edit and re-enqueues the build.
332-
*
333328
* The build is scheduled unconditionally, because the registry decides what a
334329
* save costs: a `ready` or in-flight row is left alone, so renaming or re-saving
335330
* an unchanged spec enqueues nothing, while a failed one gets the immediate
@@ -413,9 +408,9 @@ export async function updateWorkspaceSandbox(
413408
}
414409

415410
/**
416-
* Deletes a sandbox and releases its build. A block may still reference it;
417-
* that execution fails closed naming the missing sandbox, rather than silently
418-
* falling back to an image without its dependencies.
411+
* A block may still reference the deleted sandbox; that execution fails closed
412+
* naming the missing sandbox, rather than silently falling back to an image
413+
* without its dependencies.
419414
*/
420415
export async function deleteWorkspaceSandbox(
421416
workspaceId: string,

0 commit comments

Comments
 (0)