fix(security): authorize MCP subagent IDs, oauth workspace, credential admin demotion (#4551)

waleedlatif1 · web-flow · commit d895e0efee40 · 2026-05-11T10:08:31.000-07:00
* fix(security): authorize MCP subagent IDs, oauth workspace, credential admin demotion

- handleSubagentToolCall and handleDirectToolCall now authorize user-supplied
  workflowId/workspaceId via authorizeWorkflowByWorkspacePermission /
  ensureWorkspaceAccess before forwarding downstream; resolvedWorkspaceId is
  derived from the authorized workflow record instead of trusted from the body
- executeOAuthGetAuthLink verifies caller membership (write level) on the
  target workspaceId before generating the OAuth link or writing
  pendingCredentialDraft, closing the cross-workspace credential injection path
- POST /api/credentials/[id]/members wraps role updates in a transaction that
  counts active admins and rejects demotion of the last admin (mirrors the
  existing DELETE guard in the same file)
- GET /api/credentials/[id]/members returns uniform 404 for both missing and
  inaccessible credentials to remove the existence oracle

* fix(security): address PR review — active-status guard, FOR UPDATE locks, workspaceId propagation

- credentials/members POST: add `current.status === 'active'` check to the
  last-admin demotion guard so re-inviting a revoked admin as a non-admin role
  no longer incorrectly hits the "Cannot demote the last admin" path
- credentials/members POST+DELETE: add `.for('update')` to the active-admin
  count SELECT inside both transactions to serialize concurrent demotions and
  eliminate the admin-count TOCTOU race under Postgres READ COMMITTED
- credentials/members POST: also lock the member row itself with `.for('update')`
  so the role+status read and the subsequent UPDATE are atomic
- mcp/copilot handleDirectToolCall: thread the DB-verified workspaceId from the
  authorization result into prepareExecutionContext instead of relying on
  user-supplied args
- oauth handler: fix error message to mention both workspaceId and userId when
  either is missing from the execution context
diff --git a/apps/sim/app/api/credentials/[id]/members/route.ts b/apps/sim/app/api/credentials/[id]/members/route.ts
@@ -58,7 +58,7 @@ export const GET = withRouteHandler(async (_request: NextRequest, context: Route
       .limit(1)
 
     if (!cred) {
-      return NextResponse.json({ members: [] }, { status: 200 })
+      return NextResponse.json({ error: 'Not found' }, { status: 404 })
     }
 
     const callerPerm = await getUserEntityPermissions(
@@ -67,7 +67,7 @@ export const GET = withRouteHandler(async (_request: NextRequest, context: Route
       cred.workspaceId
     )
     if (callerPerm === null) {
-      return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
+      return NextResponse.json({ error: 'Not found' }, { status: 404 })
     }
 
     const members = await db
@@ -120,10 +120,36 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Route
       .limit(1)
 
     if (existing) {
-      await db
-        .update(credentialMember)
-        .set({ role, status: 'active', updatedAt: now })
-        .where(eq(credentialMember.id, existing.id))
+      const ok = await db.transaction(async (tx) => {
+        const [current] = await tx
+          .select({ role: credentialMember.role, status: credentialMember.status })
+          .from(credentialMember)
+          .where(eq(credentialMember.id, existing.id))
+          .limit(1)
+          .for('update')
+        if (current?.role === 'admin' && current?.status === 'active' && role !== 'admin') {
+          const activeAdmins = await tx
+            .select({ id: credentialMember.id })
+            .from(credentialMember)
+            .where(
+              and(
+                eq(credentialMember.credentialId, credentialId),
+                eq(credentialMember.role, 'admin'),
+                eq(credentialMember.status, 'active')
+              )
+            )
+            .for('update')
+          if (activeAdmins.length <= 1) return false
+        }
+        await tx
+          .update(credentialMember)
+          .set({ role, status: 'active', updatedAt: now })
+          .where(eq(credentialMember.id, existing.id))
+        return true
+      })
+      if (!ok) {
+        return NextResponse.json({ error: 'Cannot demote the last admin' }, { status: 400 })
+      }
       return NextResponse.json({ success: true })
     }
 
@@ -195,6 +221,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Rou
               eq(credentialMember.status, 'active')
             )
           )
+          .for('update')
 
         if (activeAdmins.length <= 1) {
           return false
diff --git a/apps/sim/app/api/mcp/copilot/route.ts b/apps/sim/app/api/mcp/copilot/route.ts
@@ -27,6 +27,7 @@ import { createRequestId } from '@/lib/copilot/request/http'
 import { runHeadlessCopilotLifecycle } from '@/lib/copilot/request/lifecycle/headless'
 import { orchestrateSubagentStream } from '@/lib/copilot/request/subagent'
 import { ensureHandlersRegistered, executeTool } from '@/lib/copilot/tool-executor'
+import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access'
 import { prepareExecutionContext } from '@/lib/copilot/tools/handlers/context'
 import { DIRECT_TOOL_DEFS, SUBAGENT_TOOL_DEFS } from '@/lib/copilot/tools/mcp/definitions'
 import { env } from '@/lib/core/config/env'
@@ -445,10 +446,36 @@ async function handleDirectToolCall(
   userId: string
 ): Promise<CallToolResult> {
   try {
+    const rawWorkflowId = (args.workflowId as string) || ''
+    let resolvedWorkspaceId: string | undefined
+    if (rawWorkflowId) {
+      const authorization = await authorizeWorkflowByWorkspacePermission({
+        workflowId: rawWorkflowId,
+        userId,
+        action: 'read',
+      })
+      if (!authorization.allowed) {
+        return {
+          content: [
+            {
+              type: 'text',
+              text: JSON.stringify(
+                { success: false, error: 'Workflow not found or access denied' },
+                null,
+                2
+              ),
+            },
+          ],
+          isError: true,
+        }
+      }
+      resolvedWorkspaceId = authorization.workflow?.workspaceId || undefined
+    }
     const execContext = await prepareExecutionContext(
       userId,
-      (args.workflowId as string) || '',
-      (args.chatId as string) || undefined
+      rawWorkflowId,
+      (args.chatId as string) || undefined,
+      { workspaceId: resolvedWorkspaceId }
     )
 
     const toolCall = {
@@ -642,21 +669,55 @@ async function handleSubagentToolCall(
       context.plan = args.plan
     }
 
+    // Authorize user-supplied workflowId / workspaceId before forwarding downstream
+    const rawWorkflowId = args.workflowId as string | undefined
+    const rawWorkspaceId = args.workspaceId as string | undefined
+    let resolvedWorkflowId: string | undefined
+    let resolvedWorkspaceId: string | undefined
+
+    if (rawWorkflowId) {
+      const authorization = await authorizeWorkflowByWorkspacePermission({
+        workflowId: rawWorkflowId,
+        userId,
+        action: 'read',
+      })
+      if (!authorization.allowed) {
+        return {
+          content: [
+            {
+              type: 'text',
+              text: JSON.stringify(
+                { success: false, error: 'Workflow not found or access denied' },
+                null,
+                2
+              ),
+            },
+          ],
+          isError: true,
+        }
+      }
+      resolvedWorkflowId = rawWorkflowId
+      resolvedWorkspaceId = authorization.workflow?.workspaceId || undefined
+    } else if (rawWorkspaceId) {
+      await ensureWorkspaceAccess(rawWorkspaceId, userId, 'read')
+      resolvedWorkspaceId = rawWorkspaceId
+    }
+
     const result = await orchestrateSubagentStream(
       toolDef.agentId,
       {
         message: requestText,
-        workflowId: args.workflowId,
-        workspaceId: args.workspaceId,
+        workflowId: resolvedWorkflowId,
+        workspaceId: resolvedWorkspaceId,
         context,
         model: DEFAULT_COPILOT_MODEL,
         headless: true,
         source: 'mcp',
       },
       {
         userId,
-        workflowId: args.workflowId as string | undefined,
-        workspaceId: args.workspaceId as string | undefined,
+        workflowId: resolvedWorkflowId,
+        workspaceId: resolvedWorkspaceId,
         simRequestId,
         abortSignal,
       }
diff --git a/apps/sim/lib/copilot/tools/handlers/oauth.ts b/apps/sim/lib/copilot/tools/handlers/oauth.ts
@@ -4,6 +4,7 @@ import { toError } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
 import { and, eq, lt } from 'drizzle-orm'
 import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
+import { ensureWorkspaceAccess } from '@/lib/copilot/tools/handlers/access'
 import { getBaseUrl } from '@/lib/core/utils/urls'
 import { getAllOAuthServices } from '@/lib/oauth/utils'
 
@@ -14,6 +15,10 @@ export async function executeOAuthGetAuthLink(
   const providerName = String(rawParams.providerName || rawParams.provider_name || '')
   const baseUrl = getBaseUrl()
   try {
+    if (!context.workspaceId || !context.userId) {
+      throw new Error('workspaceId and userId are required to generate an OAuth link')
+    }
+    await ensureWorkspaceAccess(context.workspaceId, context.userId, 'write')
     const result = await generateOAuthLink(
       context.userId,
       context.workspaceId,
