diff --git a/apps/sim/app/api/mcp/servers/[id]/route.ts b/apps/sim/app/api/mcp/servers/[id]/route.ts index 5a8a4073c5c..e13a844f911 100644 --- a/apps/sim/app/api/mcp/servers/[id]/route.ts +++ b/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, diff --git a/apps/sim/lib/mcp/application/operations.test.ts b/apps/sim/lib/mcp/application/operations.test.ts index c2bcc29e046..e03ab42f73c 100644 --- a/apps/sim/lib/mcp/application/operations.test.ts +++ b/apps/sim/lib/mcp/application/operations.test.ts @@ -132,6 +132,7 @@ const EXPECTED_CAPABILITIES: Record = 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', diff --git a/apps/sim/lib/mcp/application/operations.ts b/apps/sim/lib/mcp/application/operations.ts index 16cfb9c3f0f..1f38e05b576 100644 --- a/apps/sim/lib/mcp/application/operations.ts +++ b/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', diff --git a/apps/sim/lib/mcp/application/use-cases.test.ts b/apps/sim/lib/mcp/application/use-cases.test.ts index a4ba25d0fbc..c1d1a4f0bb9 100644 --- a/apps/sim/lib/mcp/application/use-cases.test.ts +++ b/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,46 @@ 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) + }) + + 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 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 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 () => { mocks.loadContext.mockResolvedValue({ ...workspace, workspaceOrganizationId: 'org-1' }) const args = { diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index 58de636ee78..68732ed1ecf 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/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,6 +390,18 @@ 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, }) @@ -393,6 +409,7 @@ async function updateMcpServer(args: { 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, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts index cd0370c8453..21636d5ebb2 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts +++ b/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' }, }) @@ -181,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', @@ -225,6 +238,157 @@ 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' }) + }) + + it('refuses a non-admin setting a URL on a server that has none', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: null, + 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' }) + }) + + it('refuses a non-admin save when the URL changed after it was checked', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + headers: {}, + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([]) + + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + allowDestinationChange: false, + url: 'https://example.com/mcp?token=rotated', + }) + + expect(result).toMatchObject({ success: false, errorCode: 'conflict' }) + }) + + 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).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 () => { + 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' }) + }) + + 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 () => { mockGenerateMcpServerId.mockReturnValue('server-1') dbChainMockFns.limit.mockResolvedValueOnce([]) @@ -286,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', @@ -335,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', @@ -380,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', diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index aa893f3fde4..b3d24fed276 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -18,7 +18,7 @@ import { import { detectMcpAuthType, oauthCredsChanged, revokeMcpOauthTokens } from '@/lib/mcp/oauth' import { mcpService } from '@/lib/mcp/service' import type { McpAuthType } from '@/lib/mcp/types' -import { generateMcpServerId } from '@/lib/mcp/utils' +import { generateMcpServerId, isSameMcpServerDestination } from '@/lib/mcp/utils' import { captureServerEvent } from '@/lib/posthog/server' const logger = createLogger('McpServerOrchestration') @@ -62,6 +62,11 @@ export interface PerformUpdateMcpServerParams extends ActorMetadata { workspaceId: string userId: string serverId: string + /** + * Whether the caller may point the server at a different host or path. + * Deployed workflows pin a server by id, so this is reserved for admins. + */ + allowDestinationChange: boolean name?: string description?: string | null transport?: McpServerTransport @@ -178,6 +183,22 @@ export async function createMcpServer( const urlChanged = existingServer ? existingServer.url !== params.url : true + /** + * Server ids are a 32-bit hash of the URL, so a different destination can + * collide with an existing row. Deployed workflows pin that id, so an + * upsert must never repoint it at another host. + */ + 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', + errorCode: 'conflict', + } + } + if (existingServer?.managedConnectorId) { return { success: false, @@ -249,11 +270,10 @@ export async function createMcpServer( if (shouldClearOauth) await revokeMcpOauthTokens(serverId, params.workspaceId) + const checkedUrl = existingServer.url + if (!checkedUrl) throw new Error(`MCP server ${serverId} has no URL to re-register against`) 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 = { name: params.name, description: params.description, @@ -301,8 +321,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() @@ -417,6 +454,19 @@ export async function updateMcpServer( if (!currentServer) return { success: false, error: 'Server not found', errorCode: 'not_found' } + const guardedUrl = params.allowDestinationChange ? undefined : params.url + if ( + guardedUrl !== undefined && + (!currentServer.url || !isSameMcpServerDestination(currentServer.url, guardedUrl)) + ) { + return { + success: false, + error: 'Only workspace admins can point an MCP server at a different URL', + errorCode: 'forbidden', + } + } + const checkedUrl = guardedUrl !== undefined ? currentServer.url : null + if ( params.oauthClientId && currentServer.authType !== 'oauth' && @@ -474,7 +524,8 @@ export async function updateMcpServer( and( eq(mcpServers.id, params.serverId), eq(mcpServers.workspaceId, params.workspaceId), - isNull(mcpServers.deletedAt) + isNull(mcpServers.deletedAt), + checkedUrl ? eq(mcpServers.url, checkedUrl) : undefined ) ) .returning() @@ -487,7 +538,15 @@ export async function updateMcpServer( return updated }) - if (!server) return { success: false, error: 'Server not found', errorCode: 'not_found' } + if (!server) { + return checkedUrl + ? { + success: false, + error: 'The MCP server URL changed while saving; reload and try again', + errorCode: 'conflict', + } + : { success: false, error: 'Server not found', errorCode: 'not_found' } + } const shouldClearCache = urlChanged || diff --git a/apps/sim/lib/mcp/utils.test.ts b/apps/sim/lib/mcp/utils.test.ts index c916fd19bcd..251ab3b7063 100644 --- a/apps/sim/lib/mcp/utils.test.ts +++ b/apps/sim/lib/mcp/utils.test.ts @@ -10,9 +10,10 @@ import { generateManagedMcpConnectionId, generateMcpServerId, isManagedMcpConnectionId, + isSameMcpServerDestination, parseMcpToolId, parseMcpToolTarget, -} from './utils' +} from '@/lib/mcp/utils' describe('generateMcpServerId', () => { const workspaceId = 'ws-test-123' @@ -49,6 +50,21 @@ describe('generateMcpServerId', () => { }) }) +describe('isSameMcpServerDestination', () => { + it('ignores only the query string and fragment', () => { + const url = 'https://mcp.example.com/mcp' + expect(isSameMcpServerDestination(url, `${url}?token=abc#x`)).toBe(true) + expect(isSameMcpServerDestination(url, 'https://MCP.example.com/mcp')).toBe(true) + }) + + it('treats a different host, path case, or trailing slash as a new destination', () => { + const url = 'https://mcp.example.com/mcp' + expect(isSameMcpServerDestination(url, 'https://other.example.com/mcp')).toBe(false) + expect(isSameMcpServerDestination(url, 'https://mcp.example.com/MCP')).toBe(false) + expect(isSameMcpServerDestination(url, `${url}/`)).toBe(false) + }) +}) + describe('categorizeError', () => { it.concurrent('returns 401 for McpOauthAuthorizationRequiredError via instanceof', () => { const error = new McpOauthAuthorizationRequiredError('mcp-a', 'A') diff --git a/apps/sim/lib/mcp/utils.ts b/apps/sim/lib/mcp/utils.ts index 4cd2dad2ad3..4745a83a269 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -243,6 +243,21 @@ export function generateMcpServerId(workspaceId: string, url: string): string { return `mcp-${hash}` } +/** + * Whether two URLs name the same MCP server destination: the same origin and + * exact path. Only the query string and fragment may differ — paths can be + * case-sensitive, so this is stricter than the id hash. + */ +export function isSameMcpServerDestination(a: string, b: string): boolean { + try { + const parsedA = new URL(a) + const parsedB = new URL(b) + return parsedA.origin === parsedB.origin && parsedA.pathname === parsedB.pathname + } catch { + return a === b + } +} + /** * Normalize URL for consistent hashing. * - Converts to lowercase diff --git a/scripts/check-unused-exports.baseline.json b/scripts/check-unused-exports.baseline.json index 013b0444c84..37af086f027 100644 --- a/scripts/check-unused-exports.baseline.json +++ b/scripts/check-unused-exports.baseline.json @@ -4823,7 +4823,6 @@ "apps/sim/lib/managed-agents/session-client.ts#SessionStopReason", "apps/sim/lib/managed-agents/session-client.ts#SessionUsage", "apps/sim/lib/mcp/application/operations.ts#McpServerOperation", - "apps/sim/lib/mcp/application/use-cases.ts#reconfigureMcpServerUseCase", "apps/sim/lib/mcp/application/use-cases.ts#registerMcpServerUseCase", "apps/sim/lib/mcp/constants.ts#MAX_MCP_PARAMETER_SCHEMA_BYTES=MAX_MCP_SERVER_PARAMETER_SCHEMAS_BYTES", "apps/sim/lib/mcp/constants.ts#MAX_MCP_SERVER_TOOLS_METADATA_BYTES=MAX_MCP_TOOLS_LIST_RESPONSE_BYTES",