fix(mcp): guard re-registration against concurrent URL changes

Author: TheodoreSpeaks

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

@@ -2,8 +2,7 @@ import { resetDbChainMock } from '@sim/testing'
 import type { NextRequest } from 'next/server'
 import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockPermission, mockPerformUpdateMcpServer } = vi.hoisted(() => ({
-  mockPermission: { current: 'admin' },
+const { mockPerformUpdateMcpServer } = vi.hoisted(() => ({
   mockPerformUpdateMcpServer: vi.fn(),
 }))
 
@@ -29,7 +28,7 @@ vi.mock('@/lib/mcp/middleware', () => ({
           userEmail: 'test@example.com',
           workspaceId: 'workspace-1',
           requestId: 'request-1',
-          permission: mockPermission.current,
+          permission: 'admin',
         },
         routeContext
       ),
@@ -55,7 +54,6 @@ function updateRequest() {
 describe('MCP server PATCH route', () => {
   beforeEach(() => {
     resetDbChainMock()
-    mockPermission.current = 'admin'
   })
 
   afterAll(() => {
@@ -89,22 +87,4 @@ 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/lib/mcp/application/use-cases.test.ts

@@ -123,37 +123,33 @@ describe('MCP server application use cases', () => {
         },
       })
     ).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 })
-    )
+    await expect(
+      reconfigureMcpServerUseCase.execute({
+        principal: { kind: 'session', userId: 'user-1' },
+        input: {
+          workspaceId: workspace.workspaceId,
+          serverId: server.id,
+          url: `${server.url}?token=rotated`,
+        },
+      })
+    ).resolves.toMatchObject({ server: { id: server.id } })
 
     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' })
-    )
+    await expect(
+      reconfigureMcpServerUseCase.execute({
+        principal: { kind: 'session', userId: 'user-1' },
+        input: {
+          workspaceId: workspace.workspaceId,
+          serverId: server.id,
+          url: 'https://new.example.com/mcp',
+        },
+      })
+    ).resolves.toMatchObject({ server: { id: server.id } })
   })
 
   it('resolves a selected organization server through canonical scope and current permissions', async () => {

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

@@ -193,6 +193,7 @@ describe('MCP server lifecycle orchestration', () => {
 
   it('resets to disconnected when a create/upsert flips an existing OAuth server to headers', async () => {
     mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'server-1' }])
     dbChainMockFns.limit.mockResolvedValueOnce([
       {
         id: 'server-1',
@@ -257,8 +258,6 @@ describe('MCP server lifecycle orchestration', () => {
     })
 
     expect(result).toMatchObject({ success: false, errorCode: 'forbidden' })
-    expect(dbChainMockFns.set).not.toHaveBeenCalled()
-    expect(mockRevokeOauthTokens).not.toHaveBeenCalled()
   })
 
   it('refuses a non-admin setting a URL on a server that has none', async () => {
@@ -281,7 +280,6 @@ describe('MCP server lifecycle orchestration', () => {
     })
 
     expect(result).toMatchObject({ success: false, errorCode: 'forbidden' })
-    expect(dbChainMockFns.set).not.toHaveBeenCalled()
   })
 
   it('refuses a non-admin save when the URL changed after it was checked', async () => {
@@ -336,10 +334,10 @@ describe('MCP server lifecycle orchestration', () => {
       url: 'https://new.example.com/mcp',
     })
 
-    expect(result.success).toBe(true)
-    expect(dbChainMockFns.set).toHaveBeenCalledWith(
-      expect.objectContaining({ url: 'https://new.example.com/mcp' })
-    )
+    expect(result).toMatchObject({
+      success: true,
+      server: { url: 'https://new.example.com/mcp' },
+    })
   })
 
   it('refuses a registration whose id collides with a server at a different host', async () => {
@@ -364,7 +362,31 @@ describe('MCP server lifecycle orchestration', () => {
     })
 
     expect(result).toMatchObject({ success: false, errorCode: 'conflict' })
-    expect(dbChainMockFns.set).not.toHaveBeenCalled()
+  })
+
+  it('refuses a re-registration when the URL changed after it was checked', async () => {
+    mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.limit.mockResolvedValueOnce([
+      {
+        id: 'server-1',
+        deletedAt: null,
+        url: 'https://example.com/mcp',
+        authType: 'headers',
+        oauthClientId: null,
+        oauthClientSecret: null,
+      },
+    ])
+    dbChainMockFns.returning.mockResolvedValueOnce([])
+
+    const result = await performCreateMcpServer({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      name: 'Example',
+      url: 'https://example.com/mcp',
+      authType: 'headers',
+    })
+
+    expect(result).toMatchObject({ success: false, errorCode: 'conflict' })
   })
 
   it('registers a new server as disconnected rather than stamping a connection it never made', async () => {
@@ -428,6 +450,7 @@ describe('MCP server lifecycle orchestration', () => {
 
   it('leaves a re-registered server disconnected until discovery re-runs', async () => {
     mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'server-1' }])
     dbChainMockFns.limit.mockResolvedValueOnce([
       {
         id: 'server-1',
@@ -477,6 +500,7 @@ describe('MCP server lifecycle orchestration', () => {
    */
   it('keeps an OAuth server connected through a re-registration that only renames it', async () => {
     mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'server-1' }])
     dbChainMockFns.limit.mockResolvedValueOnce([
       {
         id: 'server-1',
@@ -522,6 +546,7 @@ describe('MCP server lifecycle orchestration', () => {
 
   it('resets a re-registered server whose transport changes', async () => {
     mockGenerateMcpServerId.mockReturnValue('server-1')
+    dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'server-1' }])
     dbChainMockFns.limit.mockResolvedValueOnce([
       {
         id: 'server-1',

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

@@ -188,7 +188,10 @@ export async function createMcpServer(
      * collide with an existing row. Deployed workflows pin that id, so an
      * upsert must never repoint it at another host.
      */
-    if (existingServer?.url && !isSameMcpServerDestination(existingServer.url, params.url)) {
+    if (
+      existingServer &&
+      (!existingServer.url || !isSameMcpServerDestination(existingServer.url, params.url))
+    ) {
       return {
         success: false,
         error: 'An MCP server with a conflicting id already exists in this workspace',
@@ -267,11 +270,9 @@ export async function createMcpServer(
 
       if (shouldClearOauth) await revokeMcpOauthTokens(serverId, params.workspaceId)
 
+      const checkedUrl = existingServer.url
       let updatedFields: string[] = []
-      await db.transaction(async (tx) => {
-        if (shouldClearOauth) {
-          await tx.delete(mcpServerOauth).where(eq(mcpServerOauth.mcpServerId, serverId))
-        }
+      const rewritten = await db.transaction(async (tx) => {
         const updateValues: Partial<typeof mcpServers.$inferInsert> = {
           name: params.name,
           description: params.description,
@@ -319,8 +320,25 @@ export async function createMcpServer(
         updatedFields = Object.entries(updateValues)
           .filter(([key, value]) => key !== 'updatedAt' && value !== undefined)
           .map(([key]) => key)
-        await tx.update(mcpServers).set(updateValues).where(eq(mcpServers.id, serverId))
+        /** Matching the checked URL keeps a concurrent admin repoint from being written back. */
+        const [updated] = await tx
+          .update(mcpServers)
+          .set(updateValues)
+          .where(and(eq(mcpServers.id, serverId), eq(mcpServers.url, checkedUrl)))
+          .returning({ id: mcpServers.id })
+        if (!updated) return false
+        if (shouldClearOauth) {
+          await tx.delete(mcpServerOauth).where(eq(mcpServerOauth.mcpServerId, serverId))
+        }
+        return true
       })
+      if (!rewritten) {
+        return {
+          success: false,
+          error: 'The MCP server URL changed while saving; reload and try again',
+          errorCode: 'conflict',
+        }
+      }
 
       const [server] = await db
         .select()