diff --git a/src/server/mcp.test.ts b/src/server/mcp.test.ts index c4bc9c5a7..9bc40f316 100644 --- a/src/server/mcp.test.ts +++ b/src/server/mcp.test.ts @@ -227,6 +227,10 @@ describe('ResourceTemplate', () => { }); describe('tool()', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + /*** * Test: Zero-Argument Tool Registration */ @@ -1692,6 +1696,56 @@ describe('tool()', () => { expect(result.tools[0].name).toBe('test-without-meta'); expect(result.tools[0]._meta).toBeUndefined(); }); + + test('should validate tool names according to SEP specification', () => { + // Create a new server instance for this test + const testServer = new McpServer({ + name: 'test server', + version: '1.0' + }); + + // Spy on console.warn to verify warnings are logged + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); + + // Test valid tool names + testServer.registerTool( + 'valid-tool-name', + { + description: 'A valid tool name' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); + + // Test tool name with warnings (starts with dash) + testServer.registerTool( + '-warning-tool', + { + description: 'A tool name that generates warnings' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); + + // Test invalid tool name (contains spaces) + testServer.registerTool( + 'invalid tool name', + { + description: 'An invalid tool name' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); + + // Verify that warnings were issued (both for warnings and validation failures) + expect(warnSpy).toHaveBeenCalled(); + + // Verify specific warning content + const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(warningCalls.some(call => call.includes('Tool name starts or ends with a dash'))).toBe(true); + expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); + expect(warningCalls.some(call => call.includes('Tool name contains invalid characters'))).toBe(true); + + // Clean up spies + warnSpy.mockRestore(); + }); }); describe('resource()', () => { diff --git a/src/server/mcp.ts b/src/server/mcp.ts index 6593ea854..bee3b76ec 100644 --- a/src/server/mcp.ts +++ b/src/server/mcp.ts @@ -40,6 +40,7 @@ import { Completable, CompletableDef } from './completable.js'; import { UriTemplate, Variables } from '../shared/uriTemplate.js'; import { RequestHandlerExtra } from '../shared/protocol.js'; import { Transport } from '../shared/transport.js'; +import { validateAndWarnToolName } from '../shared/toolNameValidation.js'; /** * High-level MCP server that provides a simpler API for working with resources, tools, and prompts. @@ -664,6 +665,9 @@ export class McpServer { _meta: Record | undefined, callback: ToolCallback ): RegisteredTool { + // Validate tool name according to SEP specification + validateAndWarnToolName(name); + const registeredTool: RegisteredTool = { title, description, @@ -678,6 +682,9 @@ export class McpServer { remove: () => registeredTool.update({ name: null }), update: updates => { if (typeof updates.name !== 'undefined' && updates.name !== name) { + if (typeof updates.name === 'string') { + validateAndWarnToolName(updates.name); + } delete this._registeredTools[name]; if (updates.name) this._registeredTools[updates.name] = registeredTool; } diff --git a/src/shared/toolNameValidation.test.ts b/src/shared/toolNameValidation.test.ts new file mode 100644 index 000000000..64ba9d3ba --- /dev/null +++ b/src/shared/toolNameValidation.test.ts @@ -0,0 +1,127 @@ +import { validateToolName, validateAndWarnToolName, issueToolNameWarning } from './toolNameValidation.js'; + +// Spy on console.warn to capture output +let warnSpy: jest.SpyInstance; + +beforeEach(() => { + warnSpy = jest.spyOn(console, 'warn').mockImplementation(); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +describe('validateToolName', () => { + describe('valid tool names', () => { + test.each` + description | toolName + ${'simple alphanumeric names'} | ${'getUser'} + ${'names with underscores'} | ${'get_user_profile'} + ${'names with dashes'} | ${'user-profile-update'} + ${'names with dots'} | ${'admin.tools.list'} + ${'mixed character names'} | ${'DATA_EXPORT_v2.1'} + ${'single character names'} | ${'a'} + ${'128 character names'} | ${'a'.repeat(128)} + `('should accept $description', ({ toolName }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + }); + + describe('invalid tool names', () => { + test.each` + description | toolName | expectedWarning + ${'empty names'} | ${''} | ${'Tool name cannot be empty'} + ${'names longer than 128 characters'} | ${'a'.repeat(129)} | ${'Tool name exceeds maximum length of 128 characters (current: 129)'} + ${'names with spaces'} | ${'get user profile'} | ${'Tool name contains invalid characters: " "'} + ${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains invalid characters: ","'} + ${'names with forward slashes'} | ${'user/profile/update'} | ${'Tool name contains invalid characters: "/"'} + ${'names with other special chars'} | ${'user@domain.com'} | ${'Tool name contains invalid characters: "@"'} + ${'names with multiple invalid chars'} | ${'user name@domain,com'} | ${'Tool name contains invalid characters: " ", "@", ","'} + ${'names with unicode characters'} | ${'user-ñame'} | ${'Tool name contains invalid characters: "ñ"'} + `('should reject $description', ({ toolName, expectedWarning }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain(expectedWarning); + }); + }); + + describe('warnings for potentially problematic patterns', () => { + test.each` + description | toolName | expectedWarning | shouldBeValid + ${'names with spaces'} | ${'get user profile'} | ${'Tool name contains spaces, which may cause parsing issues'} | ${false} + ${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains commas, which may cause parsing issues'} | ${false} + ${'names starting with dash'} | ${'-get-user'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true} + ${'names ending with dash'} | ${'get-user-'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true} + ${'names starting with dot'} | ${'.get.user'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + ${'names ending with dot'} | ${'get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + ${'names with leading and trailing dots'} | ${'.get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + `('should warn about $description', ({ toolName, expectedWarning, shouldBeValid }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(shouldBeValid); + expect(result.warnings).toContain(expectedWarning); + }); + }); +}); + +describe('issueToolNameWarning', () => { + test('should output warnings to console.warn', () => { + const warnings = ['Warning 1', 'Warning 2']; + issueToolNameWarning('test-tool', warnings); + + expect(warnSpy).toHaveBeenCalledTimes(6); // Header + 2 warnings + 3 guidance lines + const calls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(calls[0]).toContain('Tool name validation warning for "test-tool"'); + expect(calls[1]).toContain('- Warning 1'); + expect(calls[2]).toContain('- Warning 2'); + expect(calls[3]).toContain('Tool registration will proceed, but this may cause compatibility issues.'); + expect(calls[4]).toContain('Consider updating the tool name'); + expect(calls[5]).toContain('See SEP: Specify Format for Tool Names'); + }); + + test('should handle empty warnings array', () => { + issueToolNameWarning('test-tool', []); + expect(warnSpy).toHaveBeenCalledTimes(0); + }); +}); + +describe('validateAndWarnToolName', () => { + test.each` + description | toolName | expectedResult | shouldWarn + ${'valid names with warnings'} | ${'-get-user-'} | ${true} | ${true} + ${'completely valid names'} | ${'get-user-profile'} | ${true} | ${false} + ${'invalid names with spaces'} | ${'get user profile'} | ${false} | ${true} + ${'empty names'} | ${''} | ${false} | ${true} + ${'names exceeding length limit'} | ${'a'.repeat(129)} | ${false} | ${true} + `('should handle $description', ({ toolName, expectedResult, shouldWarn }) => { + const result = validateAndWarnToolName(toolName); + expect(result).toBe(expectedResult); + + if (shouldWarn) { + expect(warnSpy).toHaveBeenCalled(); + } else { + expect(warnSpy).not.toHaveBeenCalled(); + } + }); + + test('should include space warning for invalid names with spaces', () => { + validateAndWarnToolName('get user profile'); + const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); + }); +}); + +describe('edge cases and robustness', () => { + test.each` + description | toolName | shouldBeValid | expectedWarning + ${'names with only dots'} | ${'...'} | ${true} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} + ${'names with only dashes'} | ${'---'} | ${true} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} + ${'names with only forward slashes'} | ${'///'} | ${false} | ${'Tool name contains invalid characters: "/"'} + ${'names with mixed valid/invalid chars'} | ${'user@name123'} | ${false} | ${'Tool name contains invalid characters: "@"'} + `('should handle $description', ({ toolName, shouldBeValid, expectedWarning }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(shouldBeValid); + expect(result.warnings).toContain(expectedWarning); + }); +}); diff --git a/src/shared/toolNameValidation.ts b/src/shared/toolNameValidation.ts new file mode 100644 index 000000000..fa96afde0 --- /dev/null +++ b/src/shared/toolNameValidation.ts @@ -0,0 +1,115 @@ +/** + * Tool name validation utilities according to SEP: Specify Format for Tool Names + * + * Tool names SHOULD be between 1 and 128 characters in length (inclusive). + * Tool names are case-sensitive. + * Allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z), digits + * (0-9), underscore (_), dash (-), and dot (.). + * Tool names SHOULD NOT contain spaces, commas, or other special characters. + */ + +/** + * Regular expression for valid tool names according to SEP-986 specification + */ +const TOOL_NAME_REGEX = /^[A-Za-z0-9._-]{1,128}$/; + +/** + * Validates a tool name according to the SEP specification + * @param name - The tool name to validate + * @returns An object containing validation result and any warnings + */ +export function validateToolName(name: string): { + isValid: boolean; + warnings: string[]; +} { + const warnings: string[] = []; + + // Check length + if (name.length === 0) { + return { + isValid: false, + warnings: ['Tool name cannot be empty'] + }; + } + + if (name.length > 128) { + return { + isValid: false, + warnings: [`Tool name exceeds maximum length of 128 characters (current: ${name.length})`] + }; + } + + // Check for specific problematic patterns (these are warnings, not validation failures) + if (name.includes(' ')) { + warnings.push('Tool name contains spaces, which may cause parsing issues'); + } + + if (name.includes(',')) { + warnings.push('Tool name contains commas, which may cause parsing issues'); + } + + // Check for potentially confusing patterns (leading/trailing dashes, dots, slashes) + if (name.startsWith('-') || name.endsWith('-')) { + warnings.push('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + } + + if (name.startsWith('.') || name.endsWith('.')) { + warnings.push('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + } + + // Check for invalid characters + if (!TOOL_NAME_REGEX.test(name)) { + const invalidChars = name + .split('') + .filter(char => !/[A-Za-z0-9._-]/.test(char)) + .filter((char, index, arr) => arr.indexOf(char) === index); // Remove duplicates + + warnings.push( + `Tool name contains invalid characters: ${invalidChars.map(c => `"${c}"`).join(', ')}`, + 'Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)' + ); + + return { + isValid: false, + warnings + }; + } + + return { + isValid: true, + warnings + }; +} + +/** + * Issues warnings for non-conforming tool names + * @param name - The tool name that triggered the warnings + * @param warnings - Array of warning messages + */ +export function issueToolNameWarning(name: string, warnings: string[]): void { + if (warnings.length > 0) { + console.warn(`Tool name validation warning for "${name}":`); + for (const warning of warnings) { + console.warn(` - ${warning}`); + } + console.warn('Tool registration will proceed, but this may cause compatibility issues.'); + console.warn('Consider updating the tool name to conform to the MCP tool naming standard.'); + console.warn( + 'See SEP: Specify Format for Tool Names (https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) for more details.' + ); + } +} + +/** + * Validates a tool name and issues warnings for non-conforming names + * @param name - The tool name to validate + * @returns true if the name is valid, false otherwise + */ +export function validateAndWarnToolName(name: string): boolean { + const result = validateToolName(name); + + // Always issue warnings for any validation issues (both invalid names and warnings) + issueToolNameWarning(name, result.warnings); + + return result.isValid; +}