Skip to content

Commit 12d3b05

Browse files
committed
fix(copilot): gate post-tool output writes behind write permission
The Copilot/Mothership executor runs three post-tool output-redirection sinks (maybeWriteOutputToFile, maybeWriteOutputToTable, maybeWriteReadCsvToTable) that persist a tool's result into the workspace. They were gated only on identity (workspaceId + userId), not on permission. Because function_execute/user_table/read are read-allowed for execution (absent from WRITE_ACTIONS in tools/server/router.ts), a read-only collaborator could drive the agent to durably create/overwrite workspace files and insert/overwrite table rows via output declarations — a function-level authorization bypass (CWE-862) that the dedicated write tools correctly reject. Add a shared denyOutputWriteWithoutWritePermission guard built on the canonical permissionSatisfies predicate and apply it to all three sinks, once a write is actually intended, so read-only principals get the same Permission denied outcome as the dedicated mutation tools.
1 parent 3c6c6b1 commit 12d3b05

5 files changed

Lines changed: 136 additions & 1 deletion

File tree

apps/sim/lib/copilot/request/tools/files.test.ts

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,33 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { describe, expect, it } from 'vitest'
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockWriteWorkspaceFileByPath } = vi.hoisted(() => ({
7+
mockWriteWorkspaceFileByPath: vi.fn(),
8+
}))
9+
10+
vi.mock('@/lib/copilot/vfs/resource-writer', () => ({
11+
writeWorkspaceFileByPath: mockWriteWorkspaceFileByPath,
12+
}))
13+
14+
vi.mock('@/lib/copilot/request/otel', () => ({
15+
withCopilotSpan: (
16+
_name: string,
17+
_attrs: Record<string, unknown> | undefined,
18+
fn: (span: unknown) => Promise<unknown>
19+
) => fn({ setAttribute: vi.fn(), setAttributes: vi.fn(), addEvent: vi.fn() }),
20+
}))
21+
22+
import { FunctionExecute } from '@/lib/copilot/generated/tool-catalog-v1'
523
import {
624
extractTabularData,
25+
maybeWriteOutputToFile,
726
normalizeOutputWorkspaceFileName,
827
serializeOutputForFile,
928
unwrapFunctionExecuteOutput,
1029
} from '@/lib/copilot/request/tools/files'
30+
import type { ExecutionContext } from '@/lib/copilot/request/types'
1131

1232
describe('unwrapFunctionExecuteOutput', () => {
1333
it('unwraps the function_execute envelope { result, stdout }', () => {
@@ -87,6 +107,53 @@ describe('normalizeOutputWorkspaceFileName', () => {
87107
})
88108
})
89109

110+
describe('maybeWriteOutputToFile', () => {
111+
function buildContext(overrides: Partial<ExecutionContext> = {}): ExecutionContext {
112+
return {
113+
userId: 'user-1',
114+
workflowId: 'wf-1',
115+
workspaceId: 'workspace-1',
116+
userPermission: 'write',
117+
...overrides,
118+
}
119+
}
120+
121+
beforeEach(() => {
122+
vi.clearAllMocks()
123+
mockWriteWorkspaceFileByPath.mockResolvedValue({
124+
id: 'file-1',
125+
name: 'report.csv',
126+
vfsPath: 'files/report.csv',
127+
mode: 'overwrite',
128+
})
129+
})
130+
131+
it('denies a read-only principal without writing the file', async () => {
132+
const result = await maybeWriteOutputToFile(
133+
FunctionExecute.id,
134+
{ outputs: { files: [{ path: 'files/report.csv', mode: 'overwrite' }] } },
135+
{ success: true, output: { result: 'name,age\nAlice,30', stdout: '' } },
136+
buildContext({ userPermission: 'read' })
137+
)
138+
139+
expect(result.success).toBe(false)
140+
expect(result.error).toContain('requires write access')
141+
expect(mockWriteWorkspaceFileByPath).not.toHaveBeenCalled()
142+
})
143+
144+
it('writes the output file for a write principal', async () => {
145+
const result = await maybeWriteOutputToFile(
146+
FunctionExecute.id,
147+
{ outputs: { files: [{ path: 'files/report.csv', mode: 'overwrite' }] } },
148+
{ success: true, output: { result: 'name,age\nAlice,30', stdout: '' } },
149+
buildContext()
150+
)
151+
152+
expect(result.success).toBe(true)
153+
expect(mockWriteWorkspaceFileByPath).toHaveBeenCalledTimes(1)
154+
})
155+
})
156+
90157
describe('extractTabularData', () => {
91158
it('extracts rows directly from an array input', () => {
92159
expect(extractTabularData([{ a: 1 }, { a: 2 }])).toEqual([{ a: 1 }, { a: 2 }])

apps/sim/lib/copilot/request/tools/files.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1'
66
import { TraceEvent } from '@/lib/copilot/generated/trace-events-v1'
77
import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1'
88
import { withCopilotSpan } from '@/lib/copilot/request/otel'
9+
import { denyOutputWriteWithoutWritePermission } from '@/lib/copilot/request/tools/permissions'
910
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
1011
import { decodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils'
1112
import { writeWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer'
@@ -210,6 +211,9 @@ export async function maybeWriteOutputToFile(
210211
const outputFiles = getOutputFileDeclarations(params).filter((file) => !file.sandboxPath)
211212
if (outputFiles.length === 0) return result
212213

214+
const denied = denyOutputWriteWithoutWritePermission(context)
215+
if (denied) return denied
216+
213217
const outputObject =
214218
result.output && typeof result.output === 'object' && !Array.isArray(result.output)
215219
? (result.output as Record<string, unknown>)
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace'
2+
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
3+
4+
/**
5+
* Guards a post-tool output-redirection sink against read-only principals.
6+
*
7+
* `function_execute`, `user_table`, and `read` are read-allowed for execution
8+
* (they don't mutate the workspace themselves), so the router's `WRITE_ACTIONS`
9+
* gate in `tools/server/router.ts` lets read-only collaborators run them. But
10+
* their output-redirection declarations (`outputs.files`, `outputTable`)
11+
* durably persist to the workspace — creating/overwriting files and table rows.
12+
* Those writes must satisfy the same write gate as the dedicated mutation tools.
13+
*
14+
* Returns a denial `ToolCallResult` when the caller lacks write access (so the
15+
* agent surfaces the same `Permission denied` outcome it gets from `create_file`
16+
* / `user_table` writes), or `null` when the write may proceed.
17+
*/
18+
export function denyOutputWriteWithoutWritePermission(
19+
context: ExecutionContext
20+
): ToolCallResult | null {
21+
if (permissionSatisfies(context.userPermission as PermissionType | undefined, 'write')) {
22+
return null
23+
}
24+
return {
25+
success: false,
26+
error: `Permission denied: writing tool output to the workspace requires write access. You have '${context.userPermission ?? 'none'}' permission.`,
27+
}
28+
}

apps/sim/lib/copilot/request/tools/tables.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ function buildContext(overrides: Partial<ExecutionContext> = {}): ExecutionConte
6161
userId: 'user-1',
6262
workflowId: 'wf-1',
6363
workspaceId: 'workspace-1',
64+
userPermission: 'write',
6465
...overrides,
6566
}
6667
}
@@ -86,6 +87,20 @@ describe('maybeWriteOutputToTable', () => {
8687
expect(mockReplaceTableRows).not.toHaveBeenCalled()
8788
})
8889

90+
it('denies a read-only principal without touching the table', async () => {
91+
const result = await maybeWriteOutputToTable(
92+
FunctionExecute.id,
93+
{ outputTable: 'tbl_1' },
94+
{ success: true, output: { result: [{ name: 'Alice' }] } },
95+
buildContext({ userPermission: 'read' })
96+
)
97+
98+
expect(result.success).toBe(false)
99+
expect(result.error).toContain('requires write access')
100+
expect(mockGetTableById).not.toHaveBeenCalled()
101+
expect(mockReplaceTableRows).not.toHaveBeenCalled()
102+
})
103+
89104
it('replaces rows through the service with name keys remapped to column ids', async () => {
90105
const result = await maybeWriteOutputToTable(
91106
FunctionExecute.id,
@@ -179,6 +194,20 @@ describe('maybeWriteReadCsvToTable', () => {
179194
expect(mockReplaceTableRows).not.toHaveBeenCalled()
180195
})
181196

197+
it('denies a read-only principal without touching the table', async () => {
198+
const result = await maybeWriteReadCsvToTable(
199+
ReadTool.id,
200+
{ outputTable: 'tbl_1', path: 'files/people.csv' },
201+
{ success: true, output: { content: 'name,age\nAlice,30' } },
202+
buildContext({ userPermission: 'read' })
203+
)
204+
205+
expect(result.success).toBe(false)
206+
expect(result.error).toContain('requires write access')
207+
expect(mockGetTableById).not.toHaveBeenCalled()
208+
expect(mockReplaceTableRows).not.toHaveBeenCalled()
209+
})
210+
182211
it('imports CSV content through the service with id-keyed rows', async () => {
183212
const result = await maybeWriteReadCsvToTable(
184213
ReadTool.id,

apps/sim/lib/copilot/request/tools/tables.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1'
88
import { TraceEvent } from '@/lib/copilot/generated/trace-events-v1'
99
import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1'
1010
import { withCopilotSpan } from '@/lib/copilot/request/otel'
11+
import { denyOutputWriteWithoutWritePermission } from '@/lib/copilot/request/tools/permissions'
1112
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
1213
import type { RowData, TableDefinition } from '@/lib/table'
1314
import { buildIdByName, rowDataNameToId } from '@/lib/table/column-keys'
@@ -63,6 +64,9 @@ export async function maybeWriteOutputToTable(
6364
const outputTable = params?.outputTable as string | undefined
6465
if (!outputTable) return result
6566

67+
const denied = denyOutputWriteWithoutWritePermission(context)
68+
if (denied) return denied
69+
6670
return withCopilotSpan(
6771
TraceSpan.CopilotToolsWriteOutputTable,
6872
{
@@ -178,6 +182,9 @@ export async function maybeWriteReadCsvToTable(
178182
const outputTable = params?.outputTable as string | undefined
179183
if (!outputTable) return result
180184

185+
const denied = denyOutputWriteWithoutWritePermission(context)
186+
if (denied) return denied
187+
181188
return withCopilotSpan(
182189
TraceSpan.CopilotToolsWriteCsvToTable,
183190
{

0 commit comments

Comments
 (0)