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

Author: TheodoreSpeaks

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

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

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

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."
+    )
+  })
+})

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.`
+}

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<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.`