diff --git a/apps/sim/lib/mcp/application/execute-tool.test.ts b/apps/sim/lib/mcp/application/execute-tool.test.ts index 4de6ad29275..d4870e2a3f8 100644 --- a/apps/sim/lib/mcp/application/execute-tool.test.ts +++ b/apps/sim/lib/mcp/application/execute-tool.test.ts @@ -36,7 +36,7 @@ vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock) vi.mock('@/lib/mcp/service', () => mcpServiceMock) vi.mock('@/lib/core/telemetry', () => telemetryMock) -import { executeMcpToolUseCase } from '@/lib/mcp/application/execute-tool' +import { executeMcpToolUseCase, validateToolArguments } from '@/lib/mcp/application/execute-tool' const mocks = { assertPermissionsAllowed: permissionCheckMockFns.mockAssertPermissionsAllowed, @@ -391,3 +391,33 @@ describe('executeMcpToolUseCase', () => { expect(mocks.executeTool).not.toHaveBeenCalled() }) }) + +describe('validateToolArguments', () => { + const tool = { + name: 'search', + serverId: 'mcp-server-1', + serverName: 'Search', + inputSchema: { + type: 'object' as const, + required: ['query'], + additionalProperties: false, + properties: { + query: { type: 'string' }, + limit: { type: 'integer' }, + }, + }, + } + + it('names the invalid argument so the caller can correct the call', () => { + expect(() => validateToolArguments(tool, { query: 'sim', limit: 'ten' })).toThrow( + 'Invalid MCP operation arguments: /limit must be integer' + ) + expect(() => validateToolArguments(tool, { query: 'sim', extra: true })).toThrow( + "Invalid MCP operation arguments: must NOT have additional properties 'extra'" + ) + }) + + it('accepts arguments the schema allows', () => { + expect(() => validateToolArguments(tool, { query: 'sim', limit: 5 })).not.toThrow() + }) +}) diff --git a/apps/sim/lib/mcp/application/execute-tool.ts b/apps/sim/lib/mcp/application/execute-tool.ts index 647917c7968..538fa445a8b 100644 --- a/apps/sim/lib/mcp/application/execute-tool.ts +++ b/apps/sim/lib/mcp/application/execute-tool.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' +import type { ErrorObject } from 'ajv' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { SIM_VIA_HEADER, serializeCallChain } from '@/lib/execution/call-chain' @@ -94,8 +95,25 @@ export function coerceToolArguments( /** Validates the complete discovered JSON Schema, including nested constraints and additional properties. */ export function validateToolArguments(tool: McpTool, args: Record): void { - if (!compileMcpToolSchema(tool.inputSchema)(args)) - throw new OrchestrationError('validation', 'Invalid MCP operation arguments') + const validate = compileMcpToolSchema(tool.inputSchema) + if (validate(args)) return + // Ajv stops at the first failure (no allErrors: unbounded work on untrusted input). + const first = validate.errors?.[0] + throw new OrchestrationError( + 'validation', + first + ? `Invalid MCP operation arguments: ${describeArgumentError(first)}` + : 'Invalid MCP operation arguments' + ) +} + +/** Names the failing argument so the caller can correct it; the schema and arguments are not secret. */ +function describeArgumentError(error: ErrorObject): string { + const extra = + error.keyword === 'additionalProperties' && typeof error.params.additionalProperty === 'string' + ? ` '${error.params.additionalProperty}'` + : '' + return [error.instancePath, `${error.message ?? 'is invalid'}${extra}`].filter(Boolean).join(' ') } export function transformToolResult(result: McpToolResult): ExecuteMcpToolResult { diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 3f843362398..5e577fa94ce 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -239,12 +239,11 @@ export class McpClient { logger.info(`[${this.config.name}] Registered tools/list_changed notification handler`) } - const serverVersion = this.client.getServerVersion() logger.info(`Successfully connected to MCP server: ${this.config.name}`, { ...diagnostics, durationMs: Date.now() - startedAt, outcome: 'connected' satisfies ConnectionOutcome, - protocolVersion: serverVersion, + protocolVersion: this.getNegotiatedVersion(), }) } catch (error) { this.isConnected = false @@ -543,9 +542,9 @@ export class McpClient { } } + /** The protocol version agreed during initialize, which the transport records. */ getNegotiatedVersion(): string | undefined { - const serverVersion = this.client.getServerVersion() - return typeof serverVersion === 'string' ? serverVersion : undefined + return this.transport.protocolVersion } getSessionId(): string | undefined { diff --git a/apps/sim/lib/mcp/tool-schema.ts b/apps/sim/lib/mcp/tool-schema.ts index 302f42e0992..43d65244f44 100644 --- a/apps/sim/lib/mcp/tool-schema.ts +++ b/apps/sim/lib/mcp/tool-schema.ts @@ -13,7 +13,6 @@ export function compileMcpToolSchema(schema: McpToolSchema): ValidateFunction { const validate = new Validator({ strict: false, validateFormats: false, - allErrors: false, }).compile(schema) if ('$async' in validate && validate.$async) throw new Error('Asynchronous schemas are not supported') diff --git a/apps/sim/tools/params.test.ts b/apps/sim/tools/params.test.ts index 443106f4aa6..e00166e6e40 100644 --- a/apps/sim/tools/params.test.ts +++ b/apps/sim/tools/params.test.ts @@ -1,3 +1,4 @@ +import Ajv from 'ajv' import { afterAll, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest' import type { ExecutorDelegationOrigin } from '@/executor/types' import { mergeToolParameters } from '@/tools/merge-params' @@ -186,11 +187,42 @@ describe('Tool Parameters Utils', () => { const { schema } = await createLLMToolSchema(legacyJsonTool, {}) expect(schema.properties.payload).toEqual({ - type: 'object', description: 'Legacy JSON array payload', }) }) + it('lets a json or any param take an array as well as an object', async () => { + const untypedTool = { + ...mockToolConfig, + id: 'untyped_tool', + params: { + orgIds: { + type: 'json', + required: true, + visibility: 'user-or-llm' as ParameterVisibility, + description: 'Organization IDs, e.g. [129410, 1034157]', + }, + value: { + type: 'any', + required: true, + visibility: 'user-or-llm' as ParameterVisibility, + }, + }, + } + + const { schema: llmSchema } = await createLLMToolSchema(untypedTool, {}) + const userSchema = createUserToolSchema(untypedTool, { surface: 'copilot' }) + + for (const schema of [llmSchema, userSchema]) { + const validate = new Ajv({ strict: false }).compile(schema) + expect(validate({ orgIds: [129410, 1034157], value: ['a'] })).toBe(true) + expect(validate({ orgIds: { id: 129410 }, value: { a: 1 } })).toBe(true) + expect(schema.properties.orgIds.description).toBe( + 'Organization IDs, e.g. [129410, 1034157]' + ) + } + }) + it.concurrent('should create schema excluding user-provided parameters', async () => { const userProvidedParams = { apiKey: 'user-provided-key', diff --git a/apps/sim/tools/params.ts b/apps/sim/tools/params.ts index 8ff780bcc11..dc71edd7135 100644 --- a/apps/sim/tools/params.ts +++ b/apps/sim/tools/params.ts @@ -35,7 +35,8 @@ type ToolParamDefinition = ToolConfig['params'][string] type ToolInputBlockConfig = Pick interface SchemaProperty { - type: string + /** Absent for a `json` or `any` param, which accepts any JSON value. */ + type?: string description?: string items?: ToolParameterItemSchema properties?: Record @@ -162,13 +163,13 @@ function buildParameterSchema( : buildFileReferenceParameterSchema(param) } - let schemaType = param.type - if (schemaType === 'json' || schemaType === 'any') { - schemaType = 'object' - } + // `json` and `any` params take arrays as often as objects, so they carry no + // `type`: an `object` claim would contradict their descriptions and make a + // validating runtime reject a correct array argument. + const isUntyped = param.type === 'json' || param.type === 'any' const propertySchema: SchemaProperty = { - type: schemaType, + ...(isUntyped ? {} : { type: param.type }), description: param.description || '', ...(param.minItems !== undefined ? { minItems: param.minItems } : {}), ...(param.maxItems !== undefined ? { maxItems: param.maxItems } : {}),