Skip to content

Commit 22ee1fc

Browse files
fix(copilot): gate materialize_file writes and enforce the deploy mutation lock
materialize_file's save/extract/import all create workspace resources, but the handler-map path has no central permission check, so a read-only member could create files and workflows through the agent. Gate on write access after param validation. assertWorkflowMutable moves into performFullDeploy / performFullUndeploy / performActivateVersion, where performRevertToVersion already had it. The check previously lived only in the deploy routes, so the copilot deploy tools — which call the orchestration functions directly — could deploy, undeploy, and activate versions of a locked workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent bf7487b commit 22ee1fc

5 files changed

Lines changed: 55 additions & 13 deletions

File tree

apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ const {
2828
mockResolveStorageBillingContext: vi.fn(),
2929
}))
3030

31+
vi.mock('@/lib/copilot/tools/handlers/access', () => ({
32+
ensureWorkspaceAccess: vi.fn(),
33+
}))
34+
3135
vi.mock('@/lib/copilot/tools/handlers/upload-file-reader', () => ({
3236
findMothershipUploadRowByChatAndName: mockFindUpload,
3337
}))
@@ -128,6 +132,36 @@ const mothershipRow = {
128132
updatedAt: new Date('2026-01-01'),
129133
}
130134

135+
describe('executeMaterializeFile - workspace write gate', () => {
136+
beforeEach(() => {
137+
vi.clearAllMocks()
138+
resetDbChainMock()
139+
})
140+
141+
it.each(['save', 'import', 'extract'])(
142+
'refuses %s without workspace write access and touches no upload',
143+
async (operation) => {
144+
const { ensureWorkspaceAccess } = await import('@/lib/copilot/tools/handlers/access')
145+
vi.mocked(ensureWorkspaceAccess).mockRejectedValueOnce(
146+
new Error('Write access required for this workspace')
147+
)
148+
149+
const result = await executeMaterializeFile({ fileNames: ['a.json'], operation }, context)
150+
151+
expect(result.success).toBe(false)
152+
expect(result.error).toContain('Write access required')
153+
expect(mockFindUpload).not.toHaveBeenCalled()
154+
}
155+
)
156+
157+
it('requires write, not merely read, access', async () => {
158+
const { ensureWorkspaceAccess } = await import('@/lib/copilot/tools/handlers/access')
159+
await executeMaterializeFile({ fileNames: ['a.json'], operation: 'save' }, context)
160+
161+
expect(ensureWorkspaceAccess).toHaveBeenCalledWith(context.workspaceId, context.userId, 'write')
162+
})
163+
})
164+
131165
describe('executeMaterializeFile - unsupported operation', () => {
132166
beforeEach(() => {
133167
vi.clearAllMocks()

apps/sim/lib/copilot/tools/handlers/materialize-file.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
resolveStorageBillingContext,
1313
} from '@/lib/billing/storage'
1414
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
15+
import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access'
1516
import { findMothershipUploadRowByChatAndName } from '@/lib/copilot/tools/handlers/upload-file-reader'
1617
import { canonicalWorkspaceFilePath, encodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils'
1718
import { getServePathPrefix } from '@/lib/uploads'
@@ -504,6 +505,15 @@ export async function executeMaterializeFile(
504505
error: `Unsupported materialize_file operation "${operation}". Use "save", "import", or "extract". For CSV/TSV/JSON → use the table subagent; for documents → use the knowledge subagent.`,
505506
}
506507
}
508+
509+
// Every operation writes: save/extract create files, import creates a workflow.
510+
// The handler-map path has no central permission gate.
511+
try {
512+
await ensureWorkspaceAccess(context.workspaceId, context.userId, 'write')
513+
} catch (error) {
514+
return { success: false, error: getErrorMessage(error, 'Workspace write access required') }
515+
}
516+
507517
const succeeded: string[] = []
508518
const failed: Array<{ fileName: string; error: string }> = []
509519
const resources: NonNullable<ToolCallResult['resources']> = []

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,6 @@ import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/to
66

77
describe('copilotToolCanWrite', () => {
88
it('fails closed when the permission is absent', () => {
9-
// The regression this exists for: the previous `perm && perm !== 'write' &&
10-
// perm !== 'admin'` ladders skipped the check entirely for these values,
11-
// letting an ungated write through. userPermission is optional on the
12-
// execution context, so every one of these is reachable.
139
expect(copilotToolCanWrite(undefined)).toBe(false)
1410
expect(copilotToolCanWrite(null)).toBe(false)
1511
expect(copilotToolCanWrite('')).toBe(false)

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,9 @@
11
import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace'
22

33
/**
4-
* Whether a copilot tool call may perform a write, given the workspace
5-
* permission resolved for the request.
6-
*
7-
* **Fails closed.** `ExecutionContext.userPermission` is optional, so an absent
8-
* value must deny — the hand-written `perm && perm !== 'write' && perm !==
9-
* 'admin'` ladders this replaces skipped the check entirely when the field was
10-
* undefined, letting a write through unguarded. Shared by the server-tool
11-
* router and the handler-map tools so the two copilot execution paths cannot
12-
* disagree about what "write access" means.
4+
* Whether a copilot tool call may write. Fails closed: `userPermission` is
5+
* optional on the execution context, and absent must deny. Shared by the
6+
* server-tool router and the handler-map tools.
137
*/
148
export function copilotToolCanWrite(userPermission: string | null | undefined): boolean {
159
return permissionSatisfies((userPermission ?? null) as PermissionType | null, 'write')

apps/sim/lib/workflows/orchestration/deploy.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ export async function performFullDeploy(
119119
const actorId = params.actorId ?? userId
120120
const requestId = params.requestId ?? generateRequestId()
121121

122+
// Backstop for every caller — routes may assert first to render their own 423,
123+
// but the copilot deploy tools call this directly.
124+
await assertWorkflowMutable(workflowId)
125+
122126
const [workflowRecord] = await db
123127
.select()
124128
.from(workflowTable)
@@ -458,6 +462,8 @@ export async function performFullUndeploy(
458462
const actorId = params.actorId ?? userId
459463
const requestId = params.requestId ?? generateRequestId()
460464

465+
await assertWorkflowMutable(workflowId)
466+
461467
const [workflowRecord] = await db
462468
.select()
463469
.from(workflowTable)
@@ -568,6 +574,8 @@ export async function performActivateVersion(
568574
const actorId = params.actorId ?? userId
569575
const requestId = params.requestId ?? generateRequestId()
570576

577+
await assertWorkflowMutable(workflowId)
578+
571579
const [versionRow] = await db
572580
.select({
573581
id: workflowDeploymentVersion.id,

0 commit comments

Comments
 (0)