Skip to content

Commit a752842

Browse files
committed
fix(security): bind copilot chat attachment keys to their owner
`POST /api/copilot/chat` accepted a client-supplied `fileAttachments[].key` and passed it straight into `trackChatUpload`, which wrote it into the `workspace_files` ownership binding with no permission check and no key-ownership validation. That binding is what `verifyFileAccess` and the Files feature resolve authorization from, so any workspace member — including read-only — could hand in another member's key and have their file re-parented to a private chat: removed from the Files listing, from folders and from download-by-id, and destroyable through the chat-delete FK cascade. The sibling register route already enforced these invariants; the copilot path did not. - `trackChatUpload` now rejects keys that do not address the target workspace, only re-links a chat-upload row the caller already owns (matched by row id, not by raw key), and only mints a new binding when the key has no prior record at all — including soft-deleted ones, which the partial active-key unique index would otherwise let it insert over. - Minting a new binding verifies the object exists in storage, matching `registerUploadedWorkspaceFile`. - `buildCopilotRequestPayload` gates attachment tracking on write/admin, the same grant the upload routes that issue these keys require.
1 parent 9064039 commit a752842

4 files changed

Lines changed: 364 additions & 31 deletions

File tree

apps/sim/lib/copilot/chat/payload.test.ts

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
import { workflowsUtilsMock } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66

7-
const { mockCreateUserToolSchema, mockGetHighestPrioritySubscription } = vi.hoisted(() => ({
8-
mockCreateUserToolSchema: vi.fn(() => ({ type: 'object', properties: {} })),
9-
mockGetHighestPrioritySubscription: vi.fn(),
10-
}))
7+
const { mockCreateUserToolSchema, mockGetHighestPrioritySubscription, mockTrackChatUpload } =
8+
vi.hoisted(() => ({
9+
mockCreateUserToolSchema: vi.fn(() => ({ type: 'object', properties: {} })),
10+
mockGetHighestPrioritySubscription: vi.fn(),
11+
mockTrackChatUpload: vi.fn(),
12+
}))
1113

1214
vi.mock('@/lib/billing/core/subscription', () => ({
1315
getHighestPrioritySubscription: mockGetHighestPrioritySubscription,
@@ -104,6 +106,10 @@ vi.mock('@/tools/params', () => ({
104106
createUserToolSchema: mockCreateUserToolSchema,
105107
}))
106108

109+
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
110+
trackChatUpload: mockTrackChatUpload,
111+
}))
112+
107113
import {
108114
buildCopilotRequestPayload,
109115
buildIntegrationToolSchemas,
@@ -209,6 +215,53 @@ describe('buildIntegrationToolSchemas', () => {
209215
describe('buildCopilotRequestPayload', () => {
210216
beforeEach(() => {
211217
vi.clearAllMocks()
218+
mockTrackChatUpload.mockResolvedValue({ displayName: 'payroll.xlsx' })
219+
})
220+
221+
describe('file attachment tracking', () => {
222+
const attachmentParams = {
223+
message: 'hi',
224+
userId: 'mallory',
225+
userMessageId: 'msg-1',
226+
mode: 'agent',
227+
model: 'claude-opus-4-8',
228+
workspaceId: 'ws-1',
229+
chatId: 'chat-1',
230+
fileAttachments: [
231+
{ id: 'a1', key: 'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx', size: 1 },
232+
],
233+
}
234+
235+
/**
236+
* Tracking writes `workspace_files` rows. A read-only member reaching the
237+
* chat endpoint must not gain that write through an attachment.
238+
*/
239+
it.each(['read', undefined])('does not track attachments for permission %s', async (perm) => {
240+
await buildCopilotRequestPayload(
241+
{ ...attachmentParams, userPermission: perm },
242+
{ selectedModel: 'claude-opus-4-8' }
243+
)
244+
245+
expect(mockTrackChatUpload).not.toHaveBeenCalled()
246+
})
247+
248+
it.each(['write', 'admin'])('tracks attachments for permission %s', async (perm) => {
249+
await buildCopilotRequestPayload(
250+
{ ...attachmentParams, userPermission: perm },
251+
{ selectedModel: 'claude-opus-4-8' }
252+
)
253+
254+
expect(mockTrackChatUpload).toHaveBeenCalledWith(
255+
'ws-1',
256+
'mallory',
257+
'chat-1',
258+
'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx',
259+
expect.anything(),
260+
expect.anything(),
261+
1,
262+
'msg-1'
263+
)
264+
})
212265
})
213266

214267
it('passes workspaceContext through to the Go request payload', async () => {

apps/sim/lib/copilot/chat/payload.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,23 @@ export async function buildCopilotRequestPayload(
333333
const effectiveMode = mode === 'agent' ? 'build' : mode
334334
const transportMode = effectiveMode === 'build' ? 'agent' : effectiveMode
335335

336-
// Track uploaded files in the DB and build context tags instead of base64 inlining
336+
// Track uploaded files in the DB and build context tags instead of base64 inlining.
337+
// Tracking writes `workspace_files` rows, so it needs the same write grant the
338+
// upload routes that issue these keys already require — reaching the chat
339+
// endpoint with `read` must not confer a file-write capability.
337340
const uploadContexts: Array<{ type: string; content: string; tag?: string; path?: string }> = []
341+
const canWriteWorkspaceFiles =
342+
params.userPermission === 'write' || params.userPermission === 'admin'
338343
if (chatId && params.workspaceId && fileAttachments && fileAttachments.length > 0) {
339-
for (const f of fileAttachments) {
344+
if (!canWriteWorkspaceFiles) {
345+
logger.warn('Dropping chat file attachments without workspace write access', {
346+
chatId,
347+
workspaceId: params.workspaceId,
348+
attachmentCount: fileAttachments.length,
349+
})
350+
}
351+
const trackableAttachments = canWriteWorkspaceFiles ? fileAttachments : []
352+
for (const f of trackableAttachments) {
340353
const filename = (f.filename ?? f.name ?? 'file') as string
341354
const mediaType = (f.media_type ?? f.mimeType ?? 'application/octet-stream') as string
342355
try {

0 commit comments

Comments
 (0)