fix(mcp): restrict MCP server destination changes to admins

Author: TheodoreSpeaks

apps/sim/app/api/mcp/servers/[id]/route.test.ts

@@ -2,7 +2,8 @@ import { resetDbChainMock } from '@sim/testing'
 import type { NextRequest } from 'next/server'
 import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockPerformUpdateMcpServer } = vi.hoisted(() => ({
+const { mockPermission, mockPerformUpdateMcpServer } = vi.hoisted(() => ({
+  mockPermission: { current: 'admin' },
   mockPerformUpdateMcpServer: vi.fn(),
 }))
 
@@ -28,7 +29,7 @@ vi.mock('@/lib/mcp/middleware', () => ({
           userEmail: 'test@example.com',
           workspaceId: 'workspace-1',
           requestId: 'request-1',
-          permission: 'admin',
+          permission: mockPermission.current,
         },
         routeContext
       ),
@@ -54,6 +55,7 @@ function updateRequest() {
 describe('MCP server PATCH route', () => {
   beforeEach(() => {
     resetDbChainMock()
+    mockPermission.current = 'admin'
   })
 
   afterAll(() => {
@@ -87,4 +89,22 @@ describe('MCP server PATCH route', () => {
     expect(body.data.server.oauthClientSecret).toBeUndefined()
     expect(body.data.server.hasOauthClientSecret).toBe(true)
   })
+
+  it.each([
+    ['admin', true],
+    ['write', false],
+  ])('lets only an admin change the destination (%s)', async (permission, allowed) => {
+    mockPermission.current = permission
+    mockPerformUpdateMcpServer.mockResolvedValueOnce({
+      success: false,
+      error: 'Server not found',
+      errorCode: 'not_found',
+    })
+
+    await PATCH(updateRequest(), { params: Promise.resolve({ id: 'server-1' }) })
+
+    expect(mockPerformUpdateMcpServer).toHaveBeenCalledWith(
+      expect.objectContaining({ allowDestinationChange: allowed })
+    )
+  })
 })

apps/sim/app/api/mcp/servers/[id]/route.ts

@@ -30,7 +30,7 @@ export const PATCH = withRouteHandler(
   )(
     async (
       request: NextRequest,
-      { userId, userName, userEmail, workspaceId, requestId },
+      { userId, userName, userEmail, workspaceId, requestId, permission },
       { params }
     ) => {
       try {
@@ -59,6 +59,7 @@ export const PATCH = withRouteHandler(
           actorName: userName,
           actorEmail: userEmail,
           serverId,
+          allowDestinationChange: permission === 'admin',
           name: body.name,
           description: body.description,
           transport: body.transport,

apps/sim/lib/mcp/application/operations.test.ts

@@ -132,6 +132,7 @@ const EXPECTED_CAPABILITIES: Record<keyof typeof mcpServerOperations, string> =
   register: 'mcp_tools.use',
   update: 'mcp_tools.use',
   reconfigure: 'mcp_tools.use',
+  changeDestination: 'mcp_tools.use',
   delete: 'mcp_tools.use',
   discoverTools: 'mcp_tools.use',
   executeTool: 'mcp_tools.use',

apps/sim/lib/mcp/application/operations.ts

@@ -209,6 +209,15 @@ export const mcpServerOperations = {
     capability: 'mcp_tools.use',
     ...ALL_PRINCIPAL_POLICY,
   }),
+  /** Pointing a server at another host or path; deployed workflows pin servers by id. */
+  changeDestination: defineWorkspaceOperation({
+    id: 'mcp_servers.change_destination',
+    oauthScope: 'api:write',
+    minimumRole: 'admin',
+    workspaceApiKey: 'deny',
+    capability: 'mcp_tools.use',
+    ...HUMAN_PRINCIPAL_POLICY,
+  }),
   delete: defineWorkspaceOperation({
     id: 'mcp_servers.delete',
     oauthScope: 'api:write',

apps/sim/lib/mcp/application/use-cases.test.ts

@@ -12,12 +12,14 @@ import {
   workspaceUploadsMockFns,
 } from '@sim/testing/mocks/workspace-uploads.mock'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { InsufficientWorkspacePermissionsError } from '@/lib/core/application'
 
 const { events, hoisted } = vi.hoisted(() => ({
   events: [] as string[],
   hoisted: {
     idState: vi.fn(),
     create: vi.fn(),
+    update: vi.fn(),
     effects: vi.fn(),
     getServer: vi.fn(),
     listServers: vi.fn(),
@@ -31,7 +33,7 @@ vi.mock('@/lib/mcp/orchestration', () => ({
   applyMcpServerMutationEffects: hoisted.effects,
   createMcpServer: hoisted.create,
   deleteMcpServer: vi.fn(),
-  updateMcpServer: vi.fn(),
+  updateMcpServer: hoisted.update,
 }))
 vi.mock('@/lib/mcp/queries', () => ({
   getMcpServerIdState: hoisted.idState,
@@ -45,6 +47,7 @@ import {
   discoverMcpServerToolsUseCase,
   discoverMcpToolsUseCase,
   getMcpServerUseCase,
+  reconfigureMcpServerUseCase,
 } from '@/lib/mcp/application/use-cases'
 
 const mocks = {
@@ -109,6 +112,50 @@ describe('MCP server application use cases', () => {
     mocks.discoverServerTools.mockResolvedValue([])
   })
 
+  it('refuses a writer pointing a server at a different host before writing', async () => {
+    await expect(
+      reconfigureMcpServerUseCase.execute({
+        principal: { kind: 'session', userId: 'user-1' },
+        input: {
+          workspaceId: workspace.workspaceId,
+          serverId: server.id,
+          url: 'https://other-host.example.com/mcp',
+        },
+      })
+    ).rejects.toBeInstanceOf(InsufficientWorkspacePermissionsError)
+
+    expect(mocks.update).not.toHaveBeenCalled()
+  })
+
+  it('lets a writer change only the query string, and an admin change the host', async () => {
+    mocks.update.mockResolvedValue({ success: true, server, configurationChanged: true })
+
+    await reconfigureMcpServerUseCase.execute({
+      principal: { kind: 'session', userId: 'user-1' },
+      input: {
+        workspaceId: workspace.workspaceId,
+        serverId: server.id,
+        url: `${server.url}?token=rotated`,
+      },
+    })
+    expect(mocks.update).toHaveBeenLastCalledWith(
+      expect.objectContaining({ allowDestinationChange: false })
+    )
+
+    mocks.resolvePermission.mockResolvedValue('admin')
+    await reconfigureMcpServerUseCase.execute({
+      principal: { kind: 'session', userId: 'user-1' },
+      input: {
+        workspaceId: workspace.workspaceId,
+        serverId: server.id,
+        url: 'https://new.example.com/mcp',
+      },
+    })
+    expect(mocks.update).toHaveBeenLastCalledWith(
+      expect.objectContaining({ allowDestinationChange: true, url: 'https://new.example.com/mcp' })
+    )
+  })
+
   it('resolves a selected organization server through canonical scope and current permissions', async () => {
     mocks.loadContext.mockResolvedValue({ ...workspace, workspaceOrganizationId: 'org-1' })
     const args = {

apps/sim/lib/mcp/application/use-cases.ts

@@ -2,7 +2,11 @@ import { AuditAction, AuditResourceType } from '@sim/audit'
 import { resolvePrincipalAttribution } from '@sim/auth/principal'
 import { getPostgresErrorCode } from '@sim/utils/errors'
 import type { CursorKey, ListSortOrder } from '@/lib/api/list-query'
-import { defineAuthorizedWorkspaceUseCase, ForbiddenOperationError } from '@/lib/core/application'
+import {
+  authorizeWorkspaceOperation,
+  defineAuthorizedWorkspaceUseCase,
+  ForbiddenOperationError,
+} from '@/lib/core/application'
 import { OrchestrationError } from '@/lib/core/orchestration/types'
 import { sanitizeUrlForLog } from '@/lib/core/utils/logging'
 import {
@@ -37,7 +41,7 @@ import {
 import { mcpService } from '@/lib/mcp/service'
 import { compileMcpToolSchema } from '@/lib/mcp/tool-schema'
 import type { McpAuthType } from '@/lib/mcp/types'
-import { generateMcpServerId } from '@/lib/mcp/utils'
+import { generateMcpServerId, isSameMcpServerDestination } from '@/lib/mcp/utils'
 
 type McpServerTransport = McpServerRow['transport']
 type McpWriteSource = 'api' | 'settings' | 'tool_input'
@@ -386,13 +390,26 @@ async function updateMcpServer(args: {
       'This MCP server is managed from its Credential Group settings'
     )
   }
+  const changesDestination =
+    args.input.url !== undefined &&
+    !!args.context.server.url &&
+    !isSameMcpServerDestination(args.context.server.url, args.input.url)
+  if (changesDestination) {
+    await authorizeWorkspaceOperation(
+      args.principal,
+      mcpServerOperations.changeDestination,
+      args.context,
+      authorizationOptions
+    )
+  }
   const attribution = resolvePrincipalAttribution(args.principal, {
     workspaceBillingOwnerUserId: args.context.billedAccountUserId,
   })
   const result = await updateMcpServerRecord({
     workspaceId: args.context.workspaceId,
     userId: attribution.attributedUserId,
     serverId: args.context.server.id,
+    allowDestinationChange: changesDestination,
     name: args.input.name,
     description: args.input.description,
     transport: args.input.transport,

apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts

@@ -29,7 +29,16 @@ vi.mock('@/lib/mcp/domain-check', () => ({
 }))
 vi.mock('@/lib/mcp/oauth', () => mcpOauthMock)
 vi.mock('@/lib/mcp/service', () => mcpServiceMock)
-vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: mockGenerateMcpServerId }))
+vi.mock('@/lib/mcp/utils', () => ({
+  generateMcpServerId: mockGenerateMcpServerId,
+  isSameMcpServerDestination: (a: string, b: string) => {
+    const destination = (url: string) => {
+      const parsed = new URL(url)
+      return `${parsed.origin}${parsed.pathname}`
+    }
+    return destination(a) === destination(b)
+  },
+}))
 vi.mock('@/lib/posthog/server', () => posthogServerMock)
 
 import {
@@ -74,6 +83,7 @@ describe('MCP server lifecycle orchestration', () => {
       workspaceId: 'workspace-1',
       userId: 'user-1',
       serverId: 'server-1',
+      allowDestinationChange: false,
       oauthClientId: 'client-1',
       oauthClientIdProvided: true,
     })
@@ -116,6 +126,7 @@ describe('MCP server lifecycle orchestration', () => {
       workspaceId: 'workspace-1',
       userId: 'user-1',
       serverId: 'server-1',
+      allowDestinationChange: false,
       authType: 'headers',
     })
 
@@ -163,6 +174,7 @@ describe('MCP server lifecycle orchestration', () => {
       workspaceId: 'workspace-1',
       userId: 'user-1',
       serverId: 'server-1',
+      allowDestinationChange: false,
       headers: { authorization: 'Bearer rotated' },
     })
 
@@ -225,6 +237,90 @@ describe('MCP server lifecycle orchestration', () => {
     expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1')
   })
 
+  it('refuses a non-admin pointing an existing server at a different host', async () => {
+    dbChainMockFns.limit.mockResolvedValueOnce([
+      {
+        url: 'https://example.com/mcp',
+        authType: 'headers',
+        headers: {},
+        oauthClientId: null,
+        oauthClientSecret: null,
+      },
+    ])
+
+    const result = await performUpdateMcpServer({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      serverId: 'server-1',
+      allowDestinationChange: false,
+      url: 'https://other-host.example.com/mcp',
+    })
+
+    expect(result).toMatchObject({ success: false, errorCode: 'forbidden' })
+    expect(dbChainMockFns.set).not.toHaveBeenCalled()
+    expect(mockRevokeOauthTokens).not.toHaveBeenCalled()
+  })
+
+  it('lets an admin point an existing server at a different host', async () => {
+    dbChainMockFns.limit.mockResolvedValueOnce([
+      {
+        url: 'https://example.com/mcp',
+        authType: 'headers',
+        headers: {},
+        oauthClientId: null,
+        oauthClientSecret: null,
+      },
+    ])
+    dbChainMockFns.returning.mockResolvedValueOnce([
+      {
+        id: 'server-1',
+        workspaceId: 'workspace-1',
+        name: 'Example',
+        transport: 'streamable-http',
+        url: 'https://new.example.com/mcp',
+        authType: 'headers',
+      },
+    ])
+
+    const result = await performUpdateMcpServer({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      serverId: 'server-1',
+      allowDestinationChange: true,
+      url: 'https://new.example.com/mcp',
+    })
+
+    expect(result.success).toBe(true)
+    expect(dbChainMockFns.set).toHaveBeenCalledWith(
+      expect.objectContaining({ url: 'https://new.example.com/mcp' })
+    )
+  })
+
+  it('refuses a registration whose id collides with a server at a different host', async () => {
+    mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.limit.mockResolvedValueOnce([
+      {
+        id: 'server-1',
+        deletedAt: null,
+        url: 'https://example.com/mcp',
+        authType: 'headers',
+        oauthClientId: null,
+        oauthClientSecret: null,
+      },
+    ])
+
+    const result = await performCreateMcpServer({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      name: 'Example',
+      url: 'https://other-host.example.com/collide',
+      authType: 'headers',
+    })
+
+    expect(result).toMatchObject({ success: false, errorCode: 'conflict' })
+    expect(dbChainMockFns.set).not.toHaveBeenCalled()
+  })
+
   it('registers a new server as disconnected rather than stamping a connection it never made', async () => {
     mockGenerateMcpServerId.mockReturnValue('server-1')
     dbChainMockFns.limit.mockResolvedValueOnce([])