fix(copilot): close fail-open write gates in agent tools (#6132)

* fix(copilot): make tool write gates fail closed 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CiHhAk2R1NryaS3R8n2yFz * 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> * fix(copilot): enforce secret-admin on env writes and gate KB indexing on usage 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 <noreply@anthropic.com> * fix(copilot): return lock denials and stop bootstrapping a legacy secret's ACL 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 <noreply@anthropic.com> * style(env): collapse the merged test import onto one line Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm * fix(env): give the workspace env denial its own write-access message WorkspaceEnvAccessError reported "must be an admin of these secrets" for both denials, so a caller lacking workspace write to ADD a key was told to get secret-admin on a key that does not exist yet. Carry the reason and mirror the route's two messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-09-24 15:45:35 +08:00 · 2026-08-01 21:01:45 -04:00
parent 30820fcbca
commit 8348592d38
14 changed files with 527 additions and 54 deletions
@@ -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<ToolCallResult> {
  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),
    }
  }

@@ -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),
    }
  }

@@ -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),
    }
  }

@@ -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()
@@ -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<ToolCallResult['resources']> = []
@@ -0,0 +1,41 @@
+/**
+ * @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', () => {
+    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."
+    )
+  })
+})
@@ -0,0 +1,20 @@
+import { type PermissionType, permissionSatisfies } from '@sim/platform-authz/workspace'
+
+/**
+ * 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')
+}
+
+/** 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.`
+}
@@ -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<ReturnType<typeof getKnowledgeBaseById>>)
+  })
+
+  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<ReturnType<typeof checkAttributedUsageLimits>>)
+
+    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<ReturnType<typeof checkAttributedUsageLimits>>)
+    vi.mocked(resolveWorkspaceFileReference).mockResolvedValue(null)
+
+    await addFile()
+
+    expect(checkAttributedUsageLimits).toHaveBeenCalledWith(BILLING_ATTRIBUTION)
+  })
+})
@@ -349,6 +349,17 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg

          const kbWorkspaceId: string = targetKb.workspaceId
          const billingAttribution = requireKnowledgeBillingAttribution(context, kbWorkspaceId)
+
+          // Gate the payer before accepting indexing work, same as the upload routes.
+          const usage = await checkAttributedUsageLimits(billingAttribution)
+          if (usage.isExceeded) {
+            return {
+              success: false,
+              message:
+                usage.message || 'Usage limit exceeded. Please upgrade your plan to continue.',
+            }
+          }
+
          const added: Array<{ documentId: string; filename: string }> = []
          const failedFiles: string[] = []

@@ -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<string, string[]> = {
  [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<string, unknown>
    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.`
@@ -4,15 +4,173 @@
 import { dbChainMockFns, encryptionMock, encryptionMockFns, resetDbChainMock } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'

+const {
+  mockCreateWorkspaceEnvCredentials,
+  mockGetUserEntityPermissions,
+  mockGetWorkspaceEnvKeyAdminAccess,
+  mockRecordAudit,
+} = vi.hoisted(() => ({
+  mockCreateWorkspaceEnvCredentials: 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('@/lib/core/security/encryption', () => encryptionMock)
+vi.mock('@sim/audit', () => ({
+  AuditAction: { ENVIRONMENT_UPDATED: 'environment.updated' },
+  AuditResourceType: { ENVIRONMENT: 'environment' },
+  recordAudit: mockRecordAudit,
+}))
+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 {
  getEffectiveDecryptedEnv,
  getEffectiveEnvironmentSnapshot,
  invalidateEffectiveDecryptedEnvCache,
+  upsertWorkspaceEnvVars,
+  WorkspaceEnvAccessError,
 } from '@/lib/environment/utils'

+describe('upsertWorkspaceEnvVars', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+    encryptionMockFns.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<string>(),
+      knownKeys: new Set(['STRIPE_KEY']),
+    })
+
+    const error = await upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1').catch(
+      (e) => e
+    )
+
+    expect(error).toBeInstanceOf(WorkspaceEnvAccessError)
+    expect(error).toMatchObject({
+      reason: 'not-secret-admin',
+      message: 'You must be an admin of these secrets to edit them',
+    })
+    expect(encryptionMockFns.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<string>(),
+      knownKeys: new Set<string>(),
+    })
+
+    const error = await upsertWorkspaceEnvVars('ws-1', { NEW_KEY: 'value' }, 'user-1').catch(
+      (e) => e
+    )
+
+    expect(error).toBeInstanceOf(WorkspaceEnvAccessError)
+    // Distinct from the secret-admin denial: the route answers this case with a
+    // write-access message, and the agent surfaces whatever we throw verbatim.
+    expect(error).toMatchObject({
+      reason: 'write-access-required',
+      message: 'Write access is required to add new secrets',
+    })
+    expect(encryptionMockFns.mockEncryptSecret).not.toHaveBeenCalled()
+  })
+
+  function stubStoredVariables(variables: Record<string, string>) {
+    dbChainMockFns.limit.mockResolvedValue([{ variables }])
+  }
+
+  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')
+    ).resolves.toEqual(['STRIPE_KEY'])
+
+    expect(encryptionMockFns.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<string>(),
+      knownKeys: new Set(['STRIPE_KEY']),
+    })
+    stubStoredVariables({ STRIPE_KEY: 'old-cipher' })
+
+    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()
+  })
+
+  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<string>(),
+      knownKeys: new Set<string>(),
+    })
+    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<string>(),
+      knownKeys: new Set<string>(),
+    })
+    stubStoredVariables({})
+
+    await upsertWorkspaceEnvVars('ws-1', { BRAND_NEW: 'value' }, 'user-1')
+
+    expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith(
+      expect.objectContaining({ newKeys: ['BRAND_NEW'] })
+    )
+  })
+})
+
 describe('effective environment resolution cache', () => {
  beforeEach(() => {
    vi.clearAllMocks()
@@ -1,22 +1,48 @@
+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
 const EFFECTIVE_ENVIRONMENT_CACHE_TTL_MS = 2_000
 const EFFECTIVE_ENVIRONMENT_CACHE_MAX_ENTRIES = 1_000

+type WorkspaceEnvDenialReason = 'not-secret-admin' | 'write-access-required'
+
+/** Mirrors the messages the workspace environment route returns for the same denials. */
+const WORKSPACE_ENV_DENIAL_MESSAGES: Record<WorkspaceEnvDenialReason, string> = {
+  'not-secret-admin': 'You must be an admin of these secrets to edit them',
+  'write-access-required': 'Write access is required to add new secrets',
+}
+
+/** Thrown when the acting user may not write one of the requested env keys. */
+export class WorkspaceEnvAccessError extends Error {
+  constructor(
+    readonly reason: WorkspaceEnvDenialReason,
+    readonly keys: string[]
+  ) {
+    super(WORKSPACE_ENV_DENIAL_MESSAGES[reason])
+    this.name = 'WorkspaceEnvAccessError'
+  }
+}
+
 export interface EnvironmentResolutionSnapshot {
  personalEncrypted: Record<string, string>
  workspaceEncrypted: Record<string, string>
@@ -328,43 +354,100 @@ export async function upsertWorkspaceEnvVars(
  newVars: Record<string, string>,
  actingUserId: string
 ): Promise<string[]> {
-  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<string, string>) || {}
+  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('not-secret-admin', 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(
+      'write-access-required',
+      updatedKeys.filter((key) => !knownKeys.has(key))
+    )
+  }

  const newlyEncrypted: Record<string, string> = {}
  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.
+  const existingEncrypted = 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 existing = (existingRow?.variables as Record<string, string>) || {}
+    const merged = { ...existing, ...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() },
+      })
+
+    return existing
+  })

  invalidateEffectiveDecryptedEnvCache({ workspaceId })
-  const newKeys = Object.keys(newVars).filter((k) => !(k in existingWsEncrypted))
+  // 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({
+    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
 }

@@ -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')
+  })
+})
@@ -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<string | null> {
+  try {
+    await assertWorkflowMutable(workflowId)
+    return null
+  } catch (error) {
+    if (error instanceof WorkflowLockedError) return error.message
+    throw error
+  }
+}
+
 export interface PerformFullDeployResult {
  success: boolean
  deployedAt?: Date
@@ -119,6 +134,11 @@ 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.
+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' }
+
  const [workflowRecord] = await db
    .select()
    .from(workflowTable)
@@ -458,6 +478,9 @@ export async function performFullUndeploy(
  const actorId = params.actorId ?? userId
  const requestId = params.requestId ?? generateRequestId()

+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial }
+
  const [workflowRecord] = await db
    .select()
    .from(workflowTable)
@@ -568,6 +591,9 @@ export async function performActivateVersion(
  const actorId = params.actorId ?? userId
  const requestId = params.requestId ?? generateRequestId()

+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' }
+
  const [versionRow] = await db
    .select({
      id: workflowDeploymentVersion.id,