Skip to content

Commit bf3c6e1

Browse files
committed
fix(copilot): enforce delegated workspace scope in query_logs and set_environment_variables
A model-supplied workspaceId (or a workflowId in another workspace) could steer both tools to any workspace the acting principal can reach, bypassing the asserted-vs-context workspace comparison the rest of the Copilot tool surface enforces. Both now resolve through requireCopilotWorkspace — moved to a shared module — so an asserted workspace may only re-state the chat's execution workspace, and the default-workspace fallback is removed so a missing scope fails closed.
1 parent ab35ff4 commit bf3c6e1

9 files changed

Lines changed: 128 additions & 60 deletions

File tree

apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,9 @@ vi.mock('@/lib/copilot/tools/server/files/file-folder-application', () => ({
8686
...(fileId ? { resourceScope: { fileId } } : {}),
8787
})),
8888
ensureCopilotFileFolderPath: mocks.ensureCopilotFileFolderPath,
89+
}))
90+
91+
vi.mock('@/lib/copilot/tools/server/workspace-scope', () => ({
8992
requireCopilotWorkspace: vi.fn((context) => context.workspaceId),
9093
}))
9194

apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
} from '@/lib/copilot/application/execute-workflow-use-case'
1212
import { messageForCopilotTableError } from '@/lib/copilot/auth/table-delegation'
1313
import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
14-
import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/files/file-folder-application'
14+
import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/workspace-scope'
1515
import { decodeVfsPathSegments, encodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils'
1616
import { asOrchestrationError } from '@/lib/core/orchestration/types'
1717
import { PlatformEvents } from '@/lib/core/telemetry'

apps/sim/lib/copilot/tools/server/files/file-folder-application.ts

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,8 @@
11
import { executeCopilotFileUseCase } from '@/lib/copilot/application/execute-file-use-case'
22
import type { CopilotFileDelegationContext } from '@/lib/copilot/auth/file-delegation'
3-
import { OrchestrationError } from '@/lib/core/orchestration/types'
43
import { findWorkspaceFileFolderIdByPath } from '@/lib/uploads/contexts/workspace/workspace-file-folder-manager'
54
import { createWorkspaceFileFolderOperation } from '@/lib/workspace-files/application/workspace-file-folders'
65

7-
/**
8-
* Returns the execution workspace only. Model-provided workspace ids may assert the same value,
9-
* but can never select a different workspace or trigger a default-workspace fallback.
10-
*/
11-
export function requireCopilotWorkspace(
12-
context: CopilotFileDelegationContext,
13-
assertedWorkspaceId?: string
14-
): string {
15-
if (!context.workspaceId) throw new Error('Copilot execution workspace is required')
16-
if (assertedWorkspaceId && assertedWorkspaceId !== context.workspaceId) {
17-
throw new OrchestrationError(
18-
'validation',
19-
'Workspace ID does not match the Copilot execution workspace'
20-
)
21-
}
22-
return context.workspaceId
23-
}
24-
256
/** Creates missing parent folders through the shared folder application operation. */
267
export async function ensureCopilotFileFolderPath(
278
context: CopilotFileDelegationContext,

apps/sim/lib/copilot/tools/server/files/file-folders.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,8 @@ import {
99
type BaseServerTool,
1010
type ServerToolContext,
1111
} from '@/lib/copilot/tools/server/base-tool'
12-
import {
13-
ensureCopilotFileFolderPath,
14-
requireCopilotWorkspace,
15-
} from '@/lib/copilot/tools/server/files/file-folder-application'
12+
import { ensureCopilotFileFolderPath } from '@/lib/copilot/tools/server/files/file-folder-application'
13+
import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/workspace-scope'
1614
import { decodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils'
1715
import { asOrchestrationError, OrchestrationError } from '@/lib/core/orchestration/types'
1816
import {

apps/sim/lib/copilot/tools/server/user/set-environment-variables.test.ts

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,11 @@ afterAll(resetEnvironmentUtilsMock)
1515
const {
1616
ensureWorkflowAccessMock,
1717
ensureWorkspaceAccessMock,
18-
getDefaultWorkspaceIdMock,
1918
listCredentialsMock,
2019
performUpdateCredentialMock,
2120
} = vi.hoisted(() => ({
2221
ensureWorkflowAccessMock: vi.fn(),
2322
ensureWorkspaceAccessMock: vi.fn(),
24-
getDefaultWorkspaceIdMock: vi.fn(),
2523
listCredentialsMock: vi.fn(),
2624
performUpdateCredentialMock: vi.fn(),
2725
}))
@@ -37,7 +35,6 @@ vi.mock('@/lib/credentials/orchestration', () => ({
3735
vi.mock('@/lib/copilot/tools/handlers/access', () => ({
3836
ensureWorkflowAccess: ensureWorkflowAccessMock,
3937
ensureWorkspaceAccess: ensureWorkspaceAccessMock,
40-
getDefaultWorkspaceId: getDefaultWorkspaceIdMock,
4138
}))
4239

4340
import { setEnvironmentVariablesServerTool } from './set-environment-variables'
@@ -49,7 +46,6 @@ describe('setEnvironmentVariablesServerTool', () => {
4946
workflow: { id: 'wf-1', workspaceId: 'ws-from-workflow' },
5047
})
5148
ensureWorkspaceAccessMock.mockResolvedValue(undefined)
52-
getDefaultWorkspaceIdMock.mockResolvedValue('ws-default')
5349
upsertPersonalEnvVarsMock.mockResolvedValue({ added: ['API_KEY'], updated: [] })
5450
upsertWorkspaceEnvVarsMock.mockResolvedValue(['API_KEY'])
5551
listCredentialsMock.mockResolvedValue({
@@ -97,22 +93,59 @@ describe('setEnvironmentVariablesServerTool', () => {
9793
expect(result.scope).toBe('personal')
9894
})
9995

100-
it('falls back to the default workspace when none is in context', async () => {
101-
await setEnvironmentVariablesServerTool.execute(
102-
{
103-
variables: [{ name: 'API_KEY', value: 'secret' }],
104-
},
105-
{
106-
userId: 'user-1',
107-
}
96+
it('fails closed when the context carries no workspace', async () => {
97+
await expect(
98+
setEnvironmentVariablesServerTool.execute(
99+
{ variables: [{ name: 'API_KEY', value: 'secret' }] },
100+
{ userId: 'user-1' }
101+
)
102+
).rejects.toThrow('Copilot execution workspace is required')
103+
104+
expect(upsertWorkspaceEnvVarsMock).not.toHaveBeenCalled()
105+
})
106+
107+
it('accepts a workspaceId that re-asserts the execution workspace', async () => {
108+
const result = await setEnvironmentVariablesServerTool.execute(
109+
{ workspaceId: 'ws-1', variables: [{ name: 'API_KEY', value: 'secret' }] },
110+
{ userId: 'user-1', workspaceId: 'ws-1' }
108111
)
109112

110-
expect(getDefaultWorkspaceIdMock).toHaveBeenCalledWith('user-1')
111-
expect(upsertWorkspaceEnvVarsMock).toHaveBeenCalledWith(
112-
'ws-default',
113-
{ API_KEY: 'secret' },
114-
'user-1'
113+
expect(upsertWorkspaceEnvVarsMock).toHaveBeenCalledWith('ws-1', { API_KEY: 'secret' }, 'user-1')
114+
expect(result.workspaceId).toBe('ws-1')
115+
})
116+
117+
it('rejects a workspaceId that names a different workspace', async () => {
118+
await expect(
119+
setEnvironmentVariablesServerTool.execute(
120+
{ workspaceId: 'ws-other', variables: [{ name: 'API_KEY', value: 'secret' }] },
121+
{ userId: 'user-1', workspaceId: 'ws-1' }
122+
)
123+
).rejects.toThrow('Workspace ID does not match the Copilot execution workspace')
124+
125+
expect(upsertWorkspaceEnvVarsMock).not.toHaveBeenCalled()
126+
})
127+
128+
it('resolves the workspace from a workflow in the execution workspace', async () => {
129+
ensureWorkflowAccessMock.mockResolvedValue({ workflow: { id: 'wf-1', workspaceId: 'ws-1' } })
130+
131+
await setEnvironmentVariablesServerTool.execute(
132+
{ workflowId: 'wf-1', variables: [{ name: 'API_KEY', value: 'secret' }] },
133+
{ userId: 'user-1', workspaceId: 'ws-1' }
115134
)
135+
136+
expect(ensureWorkflowAccessMock).toHaveBeenCalledWith('wf-1', 'user-1', 'write')
137+
expect(upsertWorkspaceEnvVarsMock).toHaveBeenCalledWith('ws-1', { API_KEY: 'secret' }, 'user-1')
138+
})
139+
140+
it('rejects a workflowId whose workspace differs from the execution workspace', async () => {
141+
await expect(
142+
setEnvironmentVariablesServerTool.execute(
143+
{ workflowId: 'wf-1', variables: [{ name: 'API_KEY', value: 'secret' }] },
144+
{ userId: 'user-1', workspaceId: 'ws-1' }
145+
)
146+
).rejects.toThrow('Workspace ID does not match the Copilot execution workspace')
147+
148+
expect(upsertWorkspaceEnvVarsMock).not.toHaveBeenCalled()
116149
})
117150

118151
it('describes a workspace secret through the credential update handler, never rewriting its value', async () => {

apps/sim/lib/copilot/tools/server/user/set-environment-variables.ts

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,9 @@
11
import { createLogger } from '@sim/logger'
22
import { z } from 'zod'
33
import { SetEnvironmentVariables } from '@/lib/copilot/generated/tool-catalog-v1'
4-
import {
5-
ensureWorkflowAccess,
6-
ensureWorkspaceAccess,
7-
getDefaultWorkspaceId,
8-
} from '@/lib/copilot/tools/handlers/access'
4+
import { ensureWorkflowAccess, ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access'
95
import type { BaseServerTool, ServerToolContext } from '@/lib/copilot/tools/server/base-tool'
6+
import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/workspace-scope'
107
import { OrchestrationError } from '@/lib/core/orchestration/types'
118
import { performUpdateCredential } from '@/lib/credentials/orchestration'
129
import { listVisibleWorkspaceCredentials } from '@/lib/credentials/queries'
@@ -153,6 +150,12 @@ async function describeSecrets(params: {
153150
return { described, failures }
154151
}
155152

153+
/**
154+
* Workspace secrets always land in the chat's delegated workspace. Model-supplied
155+
* `workspaceId`/`workflowId` may only re-assert that workspace — never select a
156+
* different one the acting user happens to access, and never fall back to a
157+
* default workspace when the scope is missing.
158+
*/
156159
async function resolveWorkspaceId(
157160
params: SetEnvironmentVariablesParams,
158161
context: ServerToolContext | undefined,
@@ -166,16 +169,12 @@ async function resolveWorkspaceId(
166169
`Workflow ${params.workflowId} is not associated with a workspace`
167170
)
168171
}
169-
return workflow.workspaceId
170-
}
171-
172-
const workspaceId = params.workspaceId ?? context?.workspaceId
173-
if (workspaceId) {
174-
await ensureWorkspaceAccess(workspaceId, userId, 'write')
175-
return workspaceId
172+
return requireCopilotWorkspace(context ?? {}, workflow.workspaceId)
176173
}
177174

178-
return getDefaultWorkspaceId(userId)
175+
const workspaceId = requireCopilotWorkspace(context ?? {}, params.workspaceId)
176+
await ensureWorkspaceAccess(workspaceId, userId, 'write')
177+
return workspaceId
179178
}
180179

181180
export const setEnvironmentVariablesServerTool: BaseServerTool<

apps/sim/lib/copilot/tools/server/workflow/query-logs.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,36 @@ describe('queryLogsServerTool', () => {
215215
expect(result.error).toContain('missing')
216216
})
217217

218+
it('accepts a workspaceId that re-asserts the execution workspace', async () => {
219+
listLogsMock.mockResolvedValue({ data: [], nextCursor: null, total: 0 })
220+
221+
await queryLogsServerTool.execute({ view: 'list', workspaceId: 'ws-1' } as any, ctx)
222+
223+
expect(listLogsMock).toHaveBeenCalledTimes(1)
224+
expect(listLogsMock.mock.calls[0][0].workspaceId).toBe('ws-1')
225+
})
226+
227+
it('rejects a workspaceId that names a different workspace', async () => {
228+
await expect(
229+
queryLogsServerTool.execute({ view: 'list', workspaceId: 'ws-other' } as any, ctx)
230+
).rejects.toThrow('Workspace ID does not match the Copilot execution workspace')
231+
232+
expect(listLogsMock).not.toHaveBeenCalled()
233+
})
234+
235+
it('fails closed when the context carries no workspace', async () => {
236+
await expect(
237+
queryLogsServerTool.execute(
238+
{ view: 'list', workspaceId: 'ws-1' } as any,
239+
{
240+
userId: 'user-1',
241+
} as any
242+
)
243+
).rejects.toThrow('Copilot execution workspace is required')
244+
245+
expect(listLogsMock).not.toHaveBeenCalled()
246+
})
247+
218248
it('throws when unauthenticated', async () => {
219249
await expect(
220250
queryLogsServerTool.execute({ view: 'overview', executionId: 'exec-1' } as any, {} as any)

apps/sim/lib/copilot/tools/server/workflow/query-logs.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { z } from 'zod'
33
import { QueryLogs } from '@/lib/copilot/generated/tool-catalog-v1'
44
import type { BaseServerTool, ServerToolContext } from '@/lib/copilot/tools/server/base-tool'
5-
import { OrchestrationError } from '@/lib/core/orchestration/types'
5+
import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/workspace-scope'
66
import {
77
collectLargeValueExecutionIds,
88
collectLargeValueKeys,
@@ -118,12 +118,13 @@ const queryLogsArgsSchema = z.preprocess((value) => {
118118

119119
type QueryLogsArgs = z.infer<typeof queryLogsArgsSchema>
120120

121+
/**
122+
* Logs are always read from the chat's delegated workspace. A model-supplied
123+
* `workspaceId` may only re-assert that workspace — it can never select a
124+
* different one, even one the acting user could otherwise access.
125+
*/
121126
function resolveWorkspaceId(args: QueryLogsArgs, context?: ServerToolContext): string {
122-
const workspaceId = args.workspaceId ?? context?.workspaceId
123-
if (!workspaceId) {
124-
throw new OrchestrationError('validation', 'workspaceId is required')
125-
}
126-
return workspaceId
127+
return requireCopilotWorkspace(context ?? {}, args.workspaceId)
127128
}
128129

129130
function buildLogViewContext(
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import { OrchestrationError } from '@/lib/core/orchestration/types'
2+
3+
interface CopilotWorkspaceScopeContext {
4+
workspaceId?: string
5+
}
6+
7+
/**
8+
* Returns the execution workspace only. Model-provided workspace ids may assert the same value,
9+
* but can never select a different workspace or trigger a default-workspace fallback.
10+
*/
11+
export function requireCopilotWorkspace(
12+
context: CopilotWorkspaceScopeContext,
13+
assertedWorkspaceId?: string
14+
): string {
15+
if (!context.workspaceId) throw new Error('Copilot execution workspace is required')
16+
if (assertedWorkspaceId && assertedWorkspaceId !== context.workspaceId) {
17+
throw new OrchestrationError(
18+
'validation',
19+
'Workspace ID does not match the Copilot execution workspace'
20+
)
21+
}
22+
return context.workspaceId
23+
}

0 commit comments

Comments
 (0)