From bf7487b9754170a40efdc78837dd39e1e4d4d49c Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 13:01:17 -0700 Subject: [PATCH 1/4] fix(copilot): make tool write gates fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three handler-map management tools (manage_custom_tool, manage_mcp_tool, manage_skill) gated writes with `context.userPermission && perm !== 'write' && perm !== 'admin'`. userPermission is optional on the execution context, so an absent value skipped the check entirely and the write proceeded unguarded — while the server-tool router's equivalent gate is fail-closed. Both paths now share copilotToolCanWrite, built on the canonical permissionSatisfies (null/undefined never satisfies). manage_custom_tool additionally resolved its target as `params.workspaceId || context.workspaceId`, so a model-supplied workspace id won while the permission check was resolved for the context workspace — and upsertCustomTools performs no authz of its own. It now uses the server-set context only, matching its two siblings. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CiHhAk2R1NryaS3R8n2yFz --- .../handlers/management/manage-custom-tool.ts | 21 +++++---- .../handlers/management/manage-mcp-tool.ts | 10 ++--- .../tools/handlers/management/manage-skill.ts | 10 ++--- .../sim/lib/copilot/tools/permissions.test.ts | 45 +++++++++++++++++++ apps/sim/lib/copilot/tools/permissions.ts | 26 +++++++++++ apps/sim/lib/copilot/tools/server/router.ts | 7 +-- 6 files changed, 91 insertions(+), 28 deletions(-) create mode 100644 apps/sim/lib/copilot/tools/permissions.test.ts create mode 100644 apps/sim/lib/copilot/tools/permissions.ts diff --git a/apps/sim/lib/copilot/tools/handlers/management/manage-custom-tool.ts b/apps/sim/lib/copilot/tools/handlers/management/manage-custom-tool.ts index 1e22c7aabef..d1491ff9af9 100644 --- a/apps/sim/lib/copilot/tools/handlers/management/manage-custom-tool.ts +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-custom-tool.ts @@ -2,6 +2,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' +import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/tools/permissions' import { captureServerEvent } from '@/lib/posthog/server' import { deleteCustomTool, @@ -30,7 +31,6 @@ interface ManageCustomToolParams { schema?: ManageCustomToolSchema code?: string title?: string - workspaceId?: string } export async function executeManageCustomTool( @@ -39,22 +39,25 @@ export async function executeManageCustomTool( ): Promise { const params = rawParams as ManageCustomToolParams const operation = String(params.operation || '').toLowerCase() as ManageCustomToolOperation - const workspaceId = params.workspaceId || context.workspaceId + /** + * Server-set context only. A model-supplied `params.workspaceId` used to win + * here, while the permission gate above is resolved for the CONTEXT + * workspace — so a caller could name another workspace and have it + * authorized against their own. `upsertCustomTools` does no authz of its own + * (it only scopes queries by the id it is handed), so nothing downstream + * caught it. Matches manage_mcp_tool and manage_skill. + */ + const workspaceId = context.workspaceId if (!operation) { return { success: false, error: "Missing required 'operation' argument" } } const writeOps: string[] = ['add', 'edit', 'delete'] - if ( - writeOps.includes(operation) && - context.userPermission && - context.userPermission !== 'write' && - context.userPermission !== 'admin' - ) { + if (writeOps.includes(operation) && !copilotToolCanWrite(context.userPermission)) { return { success: false, - error: `Permission denied: '${operation}' on manage_custom_tool requires write access. You have '${context.userPermission}' permission.`, + error: copilotWriteDeniedMessage('manage_custom_tool', operation, context.userPermission), } } diff --git a/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts index a6b571c0441..c13ba02c76e 100644 --- a/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts @@ -4,6 +4,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { and, eq, isNull } from 'drizzle-orm' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' +import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/tools/permissions' import { performCreateMcpServer, performDeleteMcpServer, @@ -46,15 +47,10 @@ export async function executeManageMcpTool( } const writeOps: string[] = ['add', 'edit', 'delete'] - if ( - writeOps.includes(operation) && - context.userPermission && - context.userPermission !== 'write' && - context.userPermission !== 'admin' - ) { + if (writeOps.includes(operation) && !copilotToolCanWrite(context.userPermission)) { return { success: false, - error: `Permission denied: '${operation}' on manage_mcp_tool requires write access. You have '${context.userPermission}' permission.`, + error: copilotWriteDeniedMessage('manage_mcp_tool', operation, context.userPermission), } } diff --git a/apps/sim/lib/copilot/tools/handlers/management/manage-skill.ts b/apps/sim/lib/copilot/tools/handlers/management/manage-skill.ts index e3c1c7bfebc..95bd20be60a 100644 --- a/apps/sim/lib/copilot/tools/handlers/management/manage-skill.ts +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-skill.ts @@ -2,6 +2,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' +import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/tools/permissions' import { captureServerEvent } from '@/lib/posthog/server' import { getSkillActorContext } from '@/lib/skills/access' import { isBuiltinSkillId } from '@/lib/workflows/skills/builtin-skills' @@ -37,15 +38,10 @@ export async function executeManageSkill( // Workspace write gates only creation; edits and deletes are gated per skill // below (skill editor — explicit editor row or derived workspace admin). - if ( - operation === 'add' && - context.userPermission && - context.userPermission !== 'write' && - context.userPermission !== 'admin' - ) { + if (operation === 'add' && !copilotToolCanWrite(context.userPermission)) { return { success: false, - error: `Permission denied: 'add' on manage_skill requires write access. You have '${context.userPermission}' permission.`, + error: copilotWriteDeniedMessage('manage_skill', operation, context.userPermission), } } diff --git a/apps/sim/lib/copilot/tools/permissions.test.ts b/apps/sim/lib/copilot/tools/permissions.test.ts new file mode 100644 index 00000000000..7f242914b52 --- /dev/null +++ b/apps/sim/lib/copilot/tools/permissions.test.ts @@ -0,0 +1,45 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/tools/permissions' + +describe('copilotToolCanWrite', () => { + it('fails closed when the permission is absent', () => { + // The regression this exists for: the previous `perm && perm !== 'write' && + // perm !== 'admin'` ladders skipped the check entirely for these values, + // letting an ungated write through. userPermission is optional on the + // execution context, so every one of these is reachable. + expect(copilotToolCanWrite(undefined)).toBe(false) + expect(copilotToolCanWrite(null)).toBe(false) + expect(copilotToolCanWrite('')).toBe(false) + }) + + it('denies read-only and unrecognized permissions', () => { + expect(copilotToolCanWrite('read')).toBe(false) + expect(copilotToolCanWrite('nonsense')).toBe(false) + }) + + it('allows write and admin', () => { + expect(copilotToolCanWrite('write')).toBe(true) + expect(copilotToolCanWrite('admin')).toBe(true) + }) +}) + +describe('copilotWriteDeniedMessage', () => { + it('names the operation and the caller’s actual permission', () => { + expect(copilotWriteDeniedMessage('manage_custom_tool', 'delete', 'read')).toBe( + "Permission denied: 'delete' on manage_custom_tool requires write access. You have 'read' permission." + ) + }) + + it('reports "none" rather than an empty string when permission is absent', () => { + expect(copilotWriteDeniedMessage('manage_skill', 'add', undefined)).toContain("You have 'none'") + }) + + it('omits the operation label when there is no operation', () => { + expect(copilotWriteDeniedMessage('knowledge_base', undefined, 'read')).toBe( + "Permission denied: knowledge_base requires write access. You have 'read' permission." + ) + }) +}) diff --git a/apps/sim/lib/copilot/tools/permissions.ts b/apps/sim/lib/copilot/tools/permissions.ts new file mode 100644 index 00000000000..568e69e7560 --- /dev/null +++ b/apps/sim/lib/copilot/tools/permissions.ts @@ -0,0 +1,26 @@ +import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace' + +/** + * Whether a copilot tool call may perform a write, given the workspace + * permission resolved for the request. + * + * **Fails closed.** `ExecutionContext.userPermission` is optional, so an absent + * value must deny — the hand-written `perm && perm !== 'write' && perm !== + * 'admin'` ladders this replaces skipped the check entirely when the field was + * undefined, letting a write through unguarded. Shared by the server-tool + * router and the handler-map tools so the two copilot execution paths cannot + * disagree about what "write access" means. + */ +export function copilotToolCanWrite(userPermission: string | null | undefined): boolean { + return permissionSatisfies((userPermission ?? null) as PermissionType | null, 'write') +} + +/** Renders the denial message shared by both copilot execution paths. */ +export function copilotWriteDeniedMessage( + toolName: string, + operation: string | undefined, + userPermission: string | null | undefined +): string { + const actionLabel = operation ? `'${operation}' on ` : '' + return `Permission denied: ${actionLabel}${toolName} requires write access. You have '${userPermission || 'none'}' permission.` +} diff --git a/apps/sim/lib/copilot/tools/server/router.ts b/apps/sim/lib/copilot/tools/server/router.ts index 266c967bf57..8a99814333f 100644 --- a/apps/sim/lib/copilot/tools/server/router.ts +++ b/apps/sim/lib/copilot/tools/server/router.ts @@ -16,6 +16,7 @@ import { UserTable, WorkspaceFile, } from '@/lib/copilot/generated/tool-catalog-v1' +import { copilotToolCanWrite } from '@/lib/copilot/tools/permissions' import { assertServerToolNotAborted, type BaseServerTool, @@ -139,10 +140,6 @@ const WRITE_ACTIONS: Record = { [enrichmentRunServerTool.name]: ['*'], } -function isWritePermission(userPermission: string): boolean { - return userPermission === 'write' || userPermission === 'admin' -} - function isWriteAction(toolName: string, action: string | undefined): boolean { const writeActions = WRITE_ACTIONS[toolName] if (!writeActions) return false @@ -211,7 +208,7 @@ export async function routeExecution( if (WRITE_ACTIONS[toolName]) { const p = payload as Record const action = (p?.operation ?? p?.action) as string | undefined - if (isWriteAction(toolName, action) && !isWritePermission(context?.userPermission ?? '')) { + if (isWriteAction(toolName, action) && !copilotToolCanWrite(context?.userPermission)) { const actionLabel = action ? `'${action}' on ` : '' throw new Error( `Permission denied: ${actionLabel}${toolName} requires write access. You have '${context?.userPermission ?? 'none'}' permission.` From 22ee1fc2d4b85e7870709529a387128419d41f60 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 13:17:05 -0700 Subject: [PATCH 2/4] fix(copilot): gate materialize_file writes and enforce the deploy mutation lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tools/handlers/materialize-file.test.ts | 34 +++++++++++++++++++ .../tools/handlers/materialize-file.ts | 10 ++++++ .../sim/lib/copilot/tools/permissions.test.ts | 4 --- apps/sim/lib/copilot/tools/permissions.ts | 12 ++----- .../sim/lib/workflows/orchestration/deploy.ts | 8 +++++ 5 files changed, 55 insertions(+), 13 deletions(-) diff --git a/apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts b/apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts index ce1bf99bc2b..bdad3967c45 100644 --- a/apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts @@ -28,6 +28,10 @@ const { mockResolveStorageBillingContext: vi.fn(), })) +vi.mock('@/lib/copilot/tools/handlers/access', () => ({ + ensureWorkspaceAccess: vi.fn(), +})) + vi.mock('@/lib/copilot/tools/handlers/upload-file-reader', () => ({ findMothershipUploadRowByChatAndName: mockFindUpload, })) @@ -128,6 +132,36 @@ const mothershipRow = { updatedAt: new Date('2026-01-01'), } +describe('executeMaterializeFile - workspace write gate', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + it.each(['save', 'import', 'extract'])( + 'refuses %s without workspace write access and touches no upload', + async (operation) => { + const { ensureWorkspaceAccess } = await import('@/lib/copilot/tools/handlers/access') + vi.mocked(ensureWorkspaceAccess).mockRejectedValueOnce( + new Error('Write access required for this workspace') + ) + + const result = await executeMaterializeFile({ fileNames: ['a.json'], operation }, context) + + expect(result.success).toBe(false) + expect(result.error).toContain('Write access required') + expect(mockFindUpload).not.toHaveBeenCalled() + } + ) + + it('requires write, not merely read, access', async () => { + const { ensureWorkspaceAccess } = await import('@/lib/copilot/tools/handlers/access') + await executeMaterializeFile({ fileNames: ['a.json'], operation: 'save' }, context) + + expect(ensureWorkspaceAccess).toHaveBeenCalledWith(context.workspaceId, context.userId, 'write') + }) +}) + describe('executeMaterializeFile - unsupported operation', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/apps/sim/lib/copilot/tools/handlers/materialize-file.ts b/apps/sim/lib/copilot/tools/handlers/materialize-file.ts index ab6d9004757..3e2abda31a2 100644 --- a/apps/sim/lib/copilot/tools/handlers/materialize-file.ts +++ b/apps/sim/lib/copilot/tools/handlers/materialize-file.ts @@ -12,6 +12,7 @@ import { resolveStorageBillingContext, } from '@/lib/billing/storage' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' +import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access' import { findMothershipUploadRowByChatAndName } from '@/lib/copilot/tools/handlers/upload-file-reader' import { canonicalWorkspaceFilePath, encodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils' import { getServePathPrefix } from '@/lib/uploads' @@ -504,6 +505,15 @@ export async function executeMaterializeFile( 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.`, } } + + // Every operation writes: save/extract create files, import creates a workflow. + // The handler-map path has no central permission gate. + try { + await ensureWorkspaceAccess(context.workspaceId, context.userId, 'write') + } catch (error) { + return { success: false, error: getErrorMessage(error, 'Workspace write access required') } + } + const succeeded: string[] = [] const failed: Array<{ fileName: string; error: string }> = [] const resources: NonNullable = [] diff --git a/apps/sim/lib/copilot/tools/permissions.test.ts b/apps/sim/lib/copilot/tools/permissions.test.ts index 7f242914b52..a15ee1f791a 100644 --- a/apps/sim/lib/copilot/tools/permissions.test.ts +++ b/apps/sim/lib/copilot/tools/permissions.test.ts @@ -6,10 +6,6 @@ import { copilotToolCanWrite, copilotWriteDeniedMessage } from '@/lib/copilot/to describe('copilotToolCanWrite', () => { it('fails closed when the permission is absent', () => { - // The regression this exists for: the previous `perm && perm !== 'write' && - // perm !== 'admin'` ladders skipped the check entirely for these values, - // letting an ungated write through. userPermission is optional on the - // execution context, so every one of these is reachable. expect(copilotToolCanWrite(undefined)).toBe(false) expect(copilotToolCanWrite(null)).toBe(false) expect(copilotToolCanWrite('')).toBe(false) diff --git a/apps/sim/lib/copilot/tools/permissions.ts b/apps/sim/lib/copilot/tools/permissions.ts index 568e69e7560..52e5b55ed91 100644 --- a/apps/sim/lib/copilot/tools/permissions.ts +++ b/apps/sim/lib/copilot/tools/permissions.ts @@ -1,15 +1,9 @@ import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace' /** - * Whether a copilot tool call may perform a write, given the workspace - * permission resolved for the request. - * - * **Fails closed.** `ExecutionContext.userPermission` is optional, so an absent - * value must deny — the hand-written `perm && perm !== 'write' && perm !== - * 'admin'` ladders this replaces skipped the check entirely when the field was - * undefined, letting a write through unguarded. Shared by the server-tool - * router and the handler-map tools so the two copilot execution paths cannot - * disagree about what "write access" means. + * Whether a copilot tool call may write. Fails closed: `userPermission` is + * optional on the execution context, and absent must deny. Shared by the + * server-tool router and the handler-map tools. */ export function copilotToolCanWrite(userPermission: string | null | undefined): boolean { return permissionSatisfies((userPermission ?? null) as PermissionType | null, 'write') diff --git a/apps/sim/lib/workflows/orchestration/deploy.ts b/apps/sim/lib/workflows/orchestration/deploy.ts index 8a9f49f4a14..50e8577a99a 100644 --- a/apps/sim/lib/workflows/orchestration/deploy.ts +++ b/apps/sim/lib/workflows/orchestration/deploy.ts @@ -119,6 +119,10 @@ export async function performFullDeploy( const actorId = params.actorId ?? userId const requestId = params.requestId ?? generateRequestId() + // Backstop for every caller — routes may assert first to render their own 423, + // but the copilot deploy tools call this directly. + await assertWorkflowMutable(workflowId) + const [workflowRecord] = await db .select() .from(workflowTable) @@ -458,6 +462,8 @@ export async function performFullUndeploy( const actorId = params.actorId ?? userId const requestId = params.requestId ?? generateRequestId() + await assertWorkflowMutable(workflowId) + const [workflowRecord] = await db .select() .from(workflowTable) @@ -568,6 +574,8 @@ export async function performActivateVersion( const actorId = params.actorId ?? userId const requestId = params.requestId ?? generateRequestId() + await assertWorkflowMutable(workflowId) + const [versionRow] = await db .select({ id: workflowDeploymentVersion.id, From 92e1a3ef12bcb1f173ad84e856e4ba5d9fc2bdb7 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 13:32:15 -0700 Subject: [PATCH 3/4] fix(copilot): enforce secret-admin on env writes and gate KB indexing on usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more paths where the agent is a weaker route to the same write than the UI. upsertWorkspaceEnvVars was a weakened copy of the environment route's write: no per-key secret-admin check, no advisory lock, no audit row. Its only caller is the set_environment_variables copilot tool, which gates on workspace 'write' alone — so any write-level member could have the agent overwrite a workspace secret they do not administer, with nothing in the audit log and a lost-update race against the route's locked transaction. The gate now lives in the function so every caller inherits it, reusing getWorkspaceEnvKeyAdminAccess rather than restating the route's logic. knowledge_base add_file computed a billing attribution and then indexed without calling checkAttributedUsageLimits, which every upload route applies before accepting indexing work — an over-quota workspace could index without limit through the agent. The same file's query operation already gated correctly. Co-Authored-By: Claude Opus 5 --- .../server/knowledge/knowledge-base.test.ts | 59 +++++++++ .../tools/server/knowledge/knowledge-base.ts | 11 ++ apps/sim/lib/environment/utils.test.ts | 118 ++++++++++++++++++ apps/sim/lib/environment/utils.ts | 118 ++++++++++++++---- 4 files changed, 280 insertions(+), 26 deletions(-) create mode 100644 apps/sim/lib/environment/utils.test.ts diff --git a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts index 186e785d29c..bbc3956c7a9 100644 --- a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts +++ b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts @@ -82,7 +82,11 @@ vi.mock('@/app/api/knowledge/utils', () => ({ checkKnowledgeBaseWriteAccess: mockCheckKnowledgeBaseWriteAccess, })) +import { checkAttributedUsageLimits } from '@/lib/billing/core/billing-attribution' import { knowledgeBaseServerTool } from '@/lib/copilot/tools/server/knowledge/knowledge-base' +import { createSingleDocument } from '@/lib/knowledge/documents/service' +import { getKnowledgeBaseById } from '@/lib/knowledge/service' +import { resolveWorkspaceFileReference } from '@/lib/uploads/contexts/workspace/workspace-file-manager' const BILLING_ATTRIBUTION = { actorUserId: 'external-admin', @@ -173,3 +177,58 @@ describe('knowledge base connector Copilot operations', () => { } ) }) + +describe('knowledge base add_file usage gate', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockCheckKnowledgeBaseWriteAccess.mockResolvedValue({ + hasAccess: true, + knowledgeBase: { id: 'knowledge-base-1', workspaceId: 'workspace-paid', name: 'Paid KB' }, + }) + vi.mocked(getKnowledgeBaseById).mockResolvedValue({ + id: 'knowledge-base-1', + workspaceId: 'workspace-paid', + } as Awaited>) + }) + + function addFile() { + return knowledgeBaseServerTool.execute( + { + operation: 'add_file', + args: { knowledgeBaseId: 'knowledge-base-1', filePaths: ['files/report.pdf'] }, + }, + { + userId: 'external-admin', + workspaceId: 'workspace-paid', + billingAttribution: BILLING_ATTRIBUTION, + } + ) + } + + it('refuses to index when the payer is over its usage limit', async () => { + vi.mocked(checkAttributedUsageLimits).mockResolvedValue({ + isExceeded: true, + message: 'Usage limit exceeded.', + } as Awaited>) + + const result = await addFile() + + expect(result.success).toBe(false) + expect(result.message).toContain('Usage limit exceeded') + // The gate must precede any indexing work, matching the upload routes. + expect(resolveWorkspaceFileReference).not.toHaveBeenCalled() + expect(createSingleDocument).not.toHaveBeenCalled() + }) + + it('gates on the knowledge base workspace payer, not the caller', async () => { + vi.mocked(checkAttributedUsageLimits).mockResolvedValue({ + isExceeded: false, + } as Awaited>) + vi.mocked(resolveWorkspaceFileReference).mockResolvedValue(null) + + await addFile() + + expect(checkAttributedUsageLimits).toHaveBeenCalledWith(BILLING_ATTRIBUTION) + }) +}) diff --git a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts index bf19100c9ec..e86ef78fa2f 100644 --- a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts +++ b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts @@ -349,6 +349,17 @@ export const knowledgeBaseServerTool: BaseServerTool = [] const failedFiles: string[] = [] diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts new file mode 100644 index 00000000000..68a3ec9f5a9 --- /dev/null +++ b/apps/sim/lib/environment/utils.test.ts @@ -0,0 +1,118 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCreateWorkspaceEnvCredentials, + mockEncryptSecret, + mockGetUserEntityPermissions, + mockGetWorkspaceEnvKeyAdminAccess, + mockRecordAudit, +} = vi.hoisted(() => ({ + mockCreateWorkspaceEnvCredentials: vi.fn(), + mockEncryptSecret: vi.fn(), + mockGetUserEntityPermissions: vi.fn(), + mockGetWorkspaceEnvKeyAdminAccess: vi.fn(), + mockRecordAudit: vi.fn(), +})) + +// vitest.setup.ts mocks this module globally; this suite tests the real one. +vi.unmock('@/lib/environment/utils') + +vi.mock('@sim/audit', () => ({ + AuditAction: { ENVIRONMENT_UPDATED: 'environment.updated' }, + AuditResourceType: { ENVIRONMENT: 'environment' }, + recordAudit: mockRecordAudit, +})) +vi.mock('@/lib/core/security/encryption', () => ({ + decryptSecret: vi.fn(), + encryptSecret: mockEncryptSecret, +})) +vi.mock('@/lib/credentials/environment', () => ({ + createWorkspaceEnvCredentials: mockCreateWorkspaceEnvCredentials, + getAccessibleEnvCredentials: vi.fn(), + getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess, + syncPersonalEnvCredentialsForUser: vi.fn(), +})) +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + checkWorkspaceAccess: vi.fn(), + getUserEntityPermissions: mockGetUserEntityPermissions, +})) + +import { upsertWorkspaceEnvVars, WorkspaceEnvAccessError } from '@/lib/environment/utils' + +describe('upsertWorkspaceEnvVars', () => { + beforeEach(() => { + vi.clearAllMocks() + mockEncryptSecret.mockResolvedValue({ encrypted: 'cipher' }) + }) + + it('refuses to overwrite an existing secret the caller does not administer', async () => { + // Workspace `write` is what the copilot tool checks; the route additionally + // requires secret-admin on the specific key. Without this the agent was the + // weaker path to the same write. + mockGetUserEntityPermissions.mockResolvedValue('write') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(), + knownKeys: new Set(['STRIPE_KEY']), + }) + + await expect( + upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1') + ).rejects.toBeInstanceOf(WorkspaceEnvAccessError) + + expect(mockEncryptSecret).not.toHaveBeenCalled() + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + + it('refuses to add a new secret without workspace write', async () => { + mockGetUserEntityPermissions.mockResolvedValue('read') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(), + knownKeys: new Set(), + }) + + await expect( + upsertWorkspaceEnvVars('ws-1', { NEW_KEY: 'value' }, 'user-1') + ).rejects.toBeInstanceOf(WorkspaceEnvAccessError) + + expect(mockEncryptSecret).not.toHaveBeenCalled() + }) + + it('allows a key admin to rotate the key they administer', async () => { + mockGetUserEntityPermissions.mockResolvedValue('write') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(['STRIPE_KEY']), + knownKeys: new Set(['STRIPE_KEY']), + }) + + await expect( + upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1') + ).resolves.toEqual(['STRIPE_KEY']) + + expect(mockEncryptSecret).toHaveBeenCalledWith('rotated') + expect(mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ workspaceId: 'ws-1', actorId: 'user-1' }) + ) + }) + + it('treats a workspace admin as an admin of every key', async () => { + mockGetUserEntityPermissions.mockResolvedValue('admin') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(), + knownKeys: new Set(['STRIPE_KEY']), + }) + + await expect( + upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1') + ).resolves.toEqual(['STRIPE_KEY']) + }) + + it('records no audit and takes no lock for an empty update', async () => { + await expect(upsertWorkspaceEnvVars('ws-1', {}, 'user-1')).resolves.toEqual([]) + + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + expect(mockRecordAudit).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index f85989cecb0..cafa5b1ba39 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/apps/sim/lib/environment/utils.ts @@ -1,19 +1,34 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { db } from '@sim/db' import { environment, workspaceEnvironment } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' -import { eq, inArray } from 'drizzle-orm' +import { eq, inArray, sql } from 'drizzle-orm' import { LRUCache } from 'lru-cache' import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption' import { createWorkspaceEnvCredentials, getAccessibleEnvCredentials, + getWorkspaceEnvKeyAdminAccess, syncPersonalEnvCredentialsForUser, } from '@/lib/credentials/environment' -import { checkWorkspaceAccess, type WorkspaceAccess } from '@/lib/workspaces/permissions/utils' +import { + checkWorkspaceAccess, + getUserEntityPermissions, + type WorkspaceAccess, +} from '@/lib/workspaces/permissions/utils' const logger = createLogger('EnvironmentUtils') +const WORKSPACE_ENV_LOCK_TIMEOUT_MS = 5_000 + +/** Thrown when the acting user may not write one of the requested env keys. */ +export class WorkspaceEnvAccessError extends Error { + constructor(readonly keys: string[]) { + super('You must be an admin of these secrets to edit them') + this.name = 'WorkspaceEnvAccessError' + } +} const EFFECTIVE_DECRYPTED_ENV_CACHE_TTL_MS = 2_000 const EFFECTIVE_DECRYPTED_ENV_CACHE_MAX_ENTRIES = 1_000 @@ -317,43 +332,94 @@ export async function upsertWorkspaceEnvVars( newVars: Record, actingUserId: string ): Promise { - const updatedKeys: string[] = [] - if (Object.keys(newVars).length === 0) return updatedKeys + const updatedKeys = Object.keys(newVars) + if (updatedKeys.length === 0) return [] - const wsRows = await db - .select() - .from(workspaceEnvironment) - .where(eq(workspaceEnvironment.workspaceId, workspaceId)) - .limit(1) - const existingWsEncrypted = (wsRows[0]?.variables as Record) || {} + const permission = await getUserEntityPermissions(actingUserId, 'workspace', workspaceId) + const { adminKeys, knownKeys } = await getWorkspaceEnvKeyAdminAccess({ + workspaceId, + envKeys: updatedKeys, + userId: actingUserId, + }) + + // Overwriting an existing secret needs secret-admin on that specific key; + // workspace `write` alone only covers adding new ones. + const forbidden = updatedKeys.filter( + (key) => knownKeys.has(key) && permission !== 'admin' && !adminKeys.has(key) + ) + if (forbidden.length > 0) { + logger.warn('Workspace env update denied', { + workspaceId, + userId: actingUserId, + reason: 'not-secret-admin', + keys: forbidden, + }) + throw new WorkspaceEnvAccessError(forbidden) + } + const addingNew = updatedKeys.some((key) => !knownKeys.has(key)) + if (addingNew && permission !== 'admin' && permission !== 'write') { + logger.warn('Workspace env update denied', { + workspaceId, + userId: actingUserId, + reason: 'write-access-required', + keys: updatedKeys.filter((key) => !knownKeys.has(key)), + }) + throw new WorkspaceEnvAccessError(updatedKeys.filter((key) => !knownKeys.has(key))) + } const newlyEncrypted: Record = {} for (const [key, val] of Object.entries(newVars)) { const { encrypted } = await encryptSecret(val) newlyEncrypted[key] = encrypted - updatedKeys.push(key) } - const merged = { ...existingWsEncrypted, ...newlyEncrypted } + // Read-modify-write on a single jsonb column, so serialize against the + // route's identically-locked transaction or concurrent writers lose keys. + await db.transaction(async (tx) => { + await tx.execute( + sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)` + ) + await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${workspaceId}, 0))`) - await db - .insert(workspaceEnvironment) - .values({ - id: generateId(), - workspaceId, - variables: merged, - createdAt: new Date(), - updatedAt: new Date(), - }) - .onConflictDoUpdate({ - target: [workspaceEnvironment.workspaceId], - set: { variables: merged, updatedAt: new Date() }, - }) + const [existingRow] = await tx + .select() + .from(workspaceEnvironment) + .where(eq(workspaceEnvironment.workspaceId, workspaceId)) + .limit(1) + const merged = { + ...((existingRow?.variables as Record) || {}), + ...newlyEncrypted, + } + + await tx + .insert(workspaceEnvironment) + .values({ + id: generateId(), + workspaceId, + variables: merged, + createdAt: new Date(), + updatedAt: new Date(), + }) + .onConflictDoUpdate({ + target: [workspaceEnvironment.workspaceId], + set: { variables: merged, updatedAt: new Date() }, + }) + }) invalidateEffectiveDecryptedEnvCache({ workspaceId }) - const newKeys = Object.keys(newVars).filter((k) => !(k in existingWsEncrypted)) + const newKeys = updatedKeys.filter((key) => !knownKeys.has(key)) await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId }) + recordAudit({ + workspaceId, + actorId: actingUserId, + action: AuditAction.ENVIRONMENT_UPDATED, + resourceType: AuditResourceType.ENVIRONMENT, + resourceId: workspaceId, + description: `Updated ${updatedKeys.length} workspace environment variable(s)`, + metadata: { variableCount: updatedKeys.length, updatedKeys }, + }) + return updatedKeys } From b1103e38746a3b548fabc8fbff26b9f04e2c108f Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 15:23:37 -0700 Subject: [PATCH 4/4] fix(copilot): return lock denials and stop bootstrapping a legacy secret's ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the previous commits, both found by review. assertWorkflowMutable throws, and the three orchestration entry points awaited it outside any try/catch, so a locked workflow escaped as an exception instead of the { success: false } result the callers consume — performChatDeploy and the copilot deploy tools would have surfaced a generic 500 rather than a lock denial. performRevertToVersion already converted it; the three now do too, through a shared helper. upsertWorkspaceEnvVars derived newKeys for createWorkspaceEnvCredentials from the credential rows rather than the stored variables. A secret written before credential rows existed has no ACL, so overwriting it looked like adding a new key: it minted a credential and made the caller that secret's admin. The environment route derives newKeys from the locked jsonb read for exactly this reason, and now so does this. Co-Authored-By: Claude Opus 5 --- apps/sim/lib/environment/utils.test.ts | 56 +++++++++++++++++++ apps/sim/lib/environment/utils.ts | 15 +++-- .../workflows/orchestration/deploy.test.ts | 39 +++++++++++++ .../sim/lib/workflows/orchestration/deploy.ts | 24 +++++++- 4 files changed, 125 insertions(+), 9 deletions(-) diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts index 68a3ec9f5a9..6bb7ea944e4 100644 --- a/apps/sim/lib/environment/utils.test.ts +++ b/apps/sim/lib/environment/utils.test.ts @@ -9,12 +9,18 @@ const { mockGetUserEntityPermissions, mockGetWorkspaceEnvKeyAdminAccess, mockRecordAudit, + mockTx, } = vi.hoisted(() => ({ mockCreateWorkspaceEnvCredentials: vi.fn(), mockEncryptSecret: vi.fn(), mockGetUserEntityPermissions: vi.fn(), mockGetWorkspaceEnvKeyAdminAccess: vi.fn(), mockRecordAudit: vi.fn(), + mockTx: { + execute: vi.fn(), + select: vi.fn(), + insert: vi.fn(), + }, })) // vitest.setup.ts mocks this module globally; this suite tests the real one. @@ -35,6 +41,11 @@ vi.mock('@/lib/credentials/environment', () => ({ getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess, syncPersonalEnvCredentialsForUser: vi.fn(), })) +vi.mock('@sim/db', () => ({ + db: { + transaction: vi.fn(async (fn: (tx: unknown) => unknown) => fn(mockTx)), + }, +})) vi.mock('@/lib/workspaces/permissions/utils', () => ({ checkWorkspaceAccess: vi.fn(), getUserEntityPermissions: mockGetUserEntityPermissions, @@ -80,12 +91,23 @@ describe('upsertWorkspaceEnvVars', () => { expect(mockEncryptSecret).not.toHaveBeenCalled() }) + function stubStoredVariables(variables: Record) { + mockTx.execute.mockResolvedValue(undefined) + mockTx.select.mockReturnValue({ + from: () => ({ where: () => ({ limit: async () => [{ variables }] }) }), + }) + mockTx.insert.mockReturnValue({ + values: () => ({ onConflictDoUpdate: async () => undefined }), + }) + } + it('allows a key admin to rotate the key they administer', async () => { mockGetUserEntityPermissions.mockResolvedValue('write') mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ adminKeys: new Set(['STRIPE_KEY']), knownKeys: new Set(['STRIPE_KEY']), }) + stubStoredVariables({ STRIPE_KEY: 'old-cipher' }) await expect( upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1') @@ -103,6 +125,7 @@ describe('upsertWorkspaceEnvVars', () => { adminKeys: new Set(), knownKeys: new Set(['STRIPE_KEY']), }) + stubStoredVariables({ STRIPE_KEY: 'old-cipher' }) await expect( upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1') @@ -115,4 +138,37 @@ describe('upsertWorkspaceEnvVars', () => { expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() expect(mockRecordAudit).not.toHaveBeenCalled() }) + + it('does not mint a credential for a legacy secret already in the stored map', async () => { + // A secret written before credential rows existed has no ACL. Treating it as + // new would create one and make the caller its secret-admin — the route + // derives newKeys from the stored variables for exactly this reason. + mockGetUserEntityPermissions.mockResolvedValue('admin') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(), + knownKeys: new Set(), + }) + stubStoredVariables({ LEGACY_KEY: 'old-cipher' }) + + await upsertWorkspaceEnvVars('ws-1', { LEGACY_KEY: 'rotated' }, 'user-1') + + expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith( + expect.objectContaining({ newKeys: [] }) + ) + }) + + it('mints a credential for a genuinely new key', async () => { + mockGetUserEntityPermissions.mockResolvedValue('write') + mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({ + adminKeys: new Set(), + knownKeys: new Set(), + }) + stubStoredVariables({}) + + await upsertWorkspaceEnvVars('ws-1', { BRAND_NEW: 'value' }, 'user-1') + + expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith( + expect.objectContaining({ newKeys: ['BRAND_NEW'] }) + ) + }) }) diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index cafa5b1ba39..a848fb484ec 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/apps/sim/lib/environment/utils.ts @@ -375,7 +375,7 @@ export async function upsertWorkspaceEnvVars( // Read-modify-write on a single jsonb column, so serialize against the // route's identically-locked transaction or concurrent writers lose keys. - await db.transaction(async (tx) => { + const existingEncrypted = await db.transaction(async (tx) => { await tx.execute( sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)` ) @@ -386,10 +386,8 @@ export async function upsertWorkspaceEnvVars( .from(workspaceEnvironment) .where(eq(workspaceEnvironment.workspaceId, workspaceId)) .limit(1) - const merged = { - ...((existingRow?.variables as Record) || {}), - ...newlyEncrypted, - } + const existing = (existingRow?.variables as Record) || {} + const merged = { ...existing, ...newlyEncrypted } await tx .insert(workspaceEnvironment) @@ -404,10 +402,15 @@ export async function upsertWorkspaceEnvVars( target: [workspaceEnvironment.workspaceId], set: { variables: merged, updatedAt: new Date() }, }) + + return existing }) invalidateEffectiveDecryptedEnvCache({ workspaceId }) - const newKeys = updatedKeys.filter((key) => !knownKeys.has(key)) + // Derived from the stored variables, not from the credential rows: a legacy + // secret present in the jsonb map without a credential row is NOT new, and + // minting an ACL for it would make the caller its secret-admin. + const newKeys = updatedKeys.filter((key) => !(key in existingEncrypted)) await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId }) recordAudit({ diff --git a/apps/sim/lib/workflows/orchestration/deploy.test.ts b/apps/sim/lib/workflows/orchestration/deploy.test.ts index e428dc4b48b..e70aea0eba0 100644 --- a/apps/sim/lib/workflows/orchestration/deploy.test.ts +++ b/apps/sim/lib/workflows/orchestration/deploy.test.ts @@ -7,6 +7,7 @@ import { queueTableRows, resetDbChainMock, schemaMock, + workflowAuthzMockFns, } from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' @@ -97,9 +98,12 @@ vi.mock('@/lib/workflows/schedules', () => ({ validateWorkflowSchedules: mockValidateWorkflowSchedules, })) +// Resolves to the global @sim/platform-authz/workflow mock, so instanceof matches. +import { WorkflowLockedError } from '@sim/platform-authz/workflow' import { performActivateVersion, performFullDeploy, + performFullUndeploy, performRevertToVersion, } from '@/lib/workflows/orchestration/deploy' @@ -660,3 +664,38 @@ describe('performActivateVersion workspace event emission', () => { expect(mockEmitWorkflowDeployedEvent).not.toHaveBeenCalled() }) }) + +describe('mutation lock on the orchestration entry points', () => { + const mockAssertMutable = workflowAuthzMockFns.mockAssertWorkflowMutable + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockAssertMutable.mockRejectedValue(new WorkflowLockedError('Workflow is locked')) + }) + + it.each([ + ['performFullDeploy', () => performFullDeploy({ workflowId: 'wf-1', userId: 'user-1' })], + ['performFullUndeploy', () => performFullUndeploy({ workflowId: 'wf-1', userId: 'user-1' })], + [ + 'performActivateVersion', + () => performActivateVersion({ workflowId: 'wf-1', version: 2, userId: 'user-1' }), + ], + ])('%s returns a lock denial instead of throwing', async (_name, call) => { + // Callers like performChatDeploy and the copilot deploy tools consume the + // result object; a throw surfaces as a generic 500 instead of a denial. + const result = await call() + + expect(result.success).toBe(false) + expect(result.error).toContain('locked') + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + + it('proceeds past the gate when the workflow is mutable', async () => { + mockAssertMutable.mockResolvedValue(undefined) + + await performFullUndeploy({ workflowId: 'wf-1', userId: 'user-1' }) + + expect(mockAssertMutable).toHaveBeenCalledWith('wf-1') + }) +}) diff --git a/apps/sim/lib/workflows/orchestration/deploy.ts b/apps/sim/lib/workflows/orchestration/deploy.ts index 50e8577a99a..2abd4e04e67 100644 --- a/apps/sim/lib/workflows/orchestration/deploy.ts +++ b/apps/sim/lib/workflows/orchestration/deploy.ts @@ -96,6 +96,21 @@ export interface PerformFullDeployParams { actorId?: string } +/** + * Resolves a mutation-lock denial to a message instead of throwing, so the entry + * points below return their `{ success: false }` result shape rather than + * surfacing a 500 to callers that expect one — matching `performRevertToVersion`. + */ +async function workflowLockDenial(workflowId: string): Promise { + try { + await assertWorkflowMutable(workflowId) + return null + } catch (error) { + if (error instanceof WorkflowLockedError) return error.message + throw error + } +} + export interface PerformFullDeployResult { success: boolean deployedAt?: Date @@ -121,7 +136,8 @@ export async function performFullDeploy( // Backstop for every caller — routes may assert first to render their own 423, // but the copilot deploy tools call this directly. - await assertWorkflowMutable(workflowId) + const lockDenial = await workflowLockDenial(workflowId) + if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' } const [workflowRecord] = await db .select() @@ -462,7 +478,8 @@ export async function performFullUndeploy( const actorId = params.actorId ?? userId const requestId = params.requestId ?? generateRequestId() - await assertWorkflowMutable(workflowId) + const lockDenial = await workflowLockDenial(workflowId) + if (lockDenial) return { success: false, error: lockDenial } const [workflowRecord] = await db .select() @@ -574,7 +591,8 @@ export async function performActivateVersion( const actorId = params.actorId ?? userId const requestId = params.requestId ?? generateRequestId() - await assertWorkflowMutable(workflowId) + const lockDenial = await workflowLockDenial(workflowId) + if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' } const [versionRow] = await db .select({