diff --git a/src/mcp/server.ts b/src/mcp/server.ts index ea7bb9e..7772dd8 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -70,6 +70,7 @@ export class N8NDocumentationMCPServer { private previousTool: string | null = null; private previousToolTimestamp: number = Date.now(); private earlyLogger: EarlyErrorLogger | null = null; + private disabledToolsCache: Set | null = null; constructor(instanceContext?: InstanceContext, earlyLogger?: EarlyErrorLogger) { this.instanceContext = instanceContext; @@ -327,24 +328,46 @@ export class N8NDocumentationMCPServer { * Parse and cache disabled tools from DISABLED_TOOLS environment variable. * Returns a Set of tool names that should be filtered from registration. * + * Cached after first call since environment variables don't change at runtime. + * Includes safety limits: max 10KB env var length, max 200 tools. + * * @returns Set of disabled tool names */ private getDisabledTools(): Set { - const disabledToolsEnv = process.env.DISABLED_TOOLS || ''; - if (!disabledToolsEnv) { - return new Set(); + // Return cached value if available + if (this.disabledToolsCache !== null) { + return this.disabledToolsCache; } - const tools = disabledToolsEnv + let disabledToolsEnv = process.env.DISABLED_TOOLS || ''; + if (!disabledToolsEnv) { + this.disabledToolsCache = new Set(); + return this.disabledToolsCache; + } + + // Safety limit: prevent abuse with very long environment variables + if (disabledToolsEnv.length > 10000) { + logger.warn(`DISABLED_TOOLS environment variable too long (${disabledToolsEnv.length} chars), truncating to 10000`); + disabledToolsEnv = disabledToolsEnv.substring(0, 10000); + } + + let tools = disabledToolsEnv .split(',') .map(t => t.trim()) .filter(Boolean); + // Safety limit: prevent abuse with too many tools + if (tools.length > 200) { + logger.warn(`DISABLED_TOOLS contains ${tools.length} tools, limiting to first 200`); + tools = tools.slice(0, 200); + } + if (tools.length > 0) { logger.info(`Disabled tools configured: ${tools.join(', ')}`); } - return new Set(tools); + this.disabledToolsCache = new Set(tools); + return this.disabledToolsCache; } private setupHandlers(): void { @@ -445,7 +468,7 @@ export class N8NDocumentationMCPServer { // Log filtered tools count if any tools are disabled if (disabledTools.size > 0) { const totalAvailableTools = n8nDocumentationToolsFinal.length + (shouldIncludeManagementTools ? n8nManagementTools.length : 0); - logger.info(`Filtered ${disabledTools.size} disabled tools, ${tools.length}/${totalAvailableTools} tools available`); + logger.debug(`Filtered ${disabledTools.size} disabled tools, ${tools.length}/${totalAvailableTools} tools available`); } // Check if client is n8n (from initialization) @@ -498,7 +521,7 @@ export class N8NDocumentationMCPServer { text: JSON.stringify({ error: 'TOOL_DISABLED', message: `Tool '${name}' is not available in this deployment. It has been disabled via DISABLED_TOOLS environment variable.`, - disabledTools: Array.from(disabledTools) + tool: name }, null, 2) }] }; @@ -906,7 +929,9 @@ export class N8NDocumentationMCPServer { // Ensure args is an object and validate it args = args || {}; - // Safety check for disabled tools (defense in depth) + // Defense in depth: This should never be reached since CallToolRequestSchema + // handler already checks disabled tools (line 514-528), but we guard here + // in case of future refactoring or direct executeTool() calls const disabledTools = this.getDisabledTools(); if (disabledTools.has(name)) { throw new Error(`Tool '${name}' is disabled via DISABLED_TOOLS environment variable`); diff --git a/tests/unit/mcp/disabled-tools-additional.test.ts b/tests/unit/mcp/disabled-tools-additional.test.ts index 6b66bdd..044a9af 100644 --- a/tests/unit/mcp/disabled-tools-additional.test.ts +++ b/tests/unit/mcp/disabled-tools-additional.test.ts @@ -7,13 +7,26 @@ vi.mock('../../../src/database/node-repository'); vi.mock('../../../src/templates/template-service'); vi.mock('../../../src/utils/logger'); +/** + * Test wrapper class that exposes private methods for unit testing. + * This pattern is preferred over modifying production code visibility + * or using reflection-based testing utilities. + */ class TestableN8NMCPServer extends N8NDocumentationMCPServer { - // Expose the private getDisabledTools method for testing + /** + * Expose getDisabledTools() for testing environment variable parsing. + * @returns Set of disabled tool names from DISABLED_TOOLS env var + */ public testGetDisabledTools(): Set { return (this as any).getDisabledTools(); } - // Expose the private executeTool method for testing + /** + * Expose executeTool() for testing the defense-in-depth guard. + * @param name - Tool name to execute + * @param args - Tool arguments + * @returns Tool execution result + */ public async testExecuteTool(name: string, args: any): Promise { return (this as any).executeTool(name, args); } @@ -198,7 +211,7 @@ describe('Disabled Tools Additional Coverage (Issue #410)', () => { expect(duration).toBeLessThan(50); // Should be very fast }); - it('should handle 1000 disabled tools efficiently', () => { + it('should handle 1000 disabled tools efficiently and enforce 200 tool limit', () => { const manyTools = Array.from({ length: 1000 }, (_, i) => `tool_${i}`); process.env.DISABLED_TOOLS = manyTools.join(','); @@ -207,7 +220,8 @@ describe('Disabled Tools Additional Coverage (Issue #410)', () => { const disabledTools = server.testGetDisabledTools(); const duration = Date.now() - start; - expect(disabledTools.size).toBe(1000); + // Safety limit: max 200 tools enforced + expect(disabledTools.size).toBe(200); expect(duration).toBeLessThan(100); // Should still be fast }); diff --git a/tests/unit/mcp/disabled-tools.test.ts b/tests/unit/mcp/disabled-tools.test.ts index 0b8b780..b9c8f32 100644 --- a/tests/unit/mcp/disabled-tools.test.ts +++ b/tests/unit/mcp/disabled-tools.test.ts @@ -9,13 +9,26 @@ vi.mock('../../../src/database/node-repository'); vi.mock('../../../src/templates/template-service'); vi.mock('../../../src/utils/logger'); +/** + * Test wrapper class that exposes private methods for unit testing. + * This pattern is preferred over modifying production code visibility + * or using reflection-based testing utilities. + */ class TestableN8NMCPServer extends N8NDocumentationMCPServer { - // Expose the private getDisabledTools method for testing + /** + * Expose getDisabledTools() for testing environment variable parsing. + * @returns Set of disabled tool names from DISABLED_TOOLS env var + */ public testGetDisabledTools(): Set { return (this as any).getDisabledTools(); } - // Expose the private executeTool method for testing + /** + * Expose executeTool() for testing the defense-in-depth guard. + * @param name - Tool name to execute + * @param args - Tool arguments + * @returns Tool execution result + */ public async testExecuteTool(name: string, args: any): Promise { return (this as any).executeTool(name, args); }