diff --git a/src/main/services/DxtService.ts b/src/main/services/DxtService.ts index 59521efc63..59e97b2aa1 100644 --- a/src/main/services/DxtService.ts +++ b/src/main/services/DxtService.ts @@ -8,6 +8,27 @@ import { v4 as uuidv4 } from 'uuid' const logger = loggerService.withContext('DxtService') +/** + * Ensure a target path is within the base directory to prevent path traversal attacks. + * This is the correct approach: validate the final resolved path rather than sanitizing input. + * + * @param basePath - The base directory that the target must be within + * @param targetPath - The target path to validate + * @returns The resolved target path if valid + * @throws Error if the target path escapes the base directory + */ +export function ensurePathWithin(basePath: string, targetPath: string): string { + const resolvedBase = path.resolve(basePath) + const resolvedTarget = path.resolve(path.normalize(targetPath)) + + // Must be direct child of base directory, no subdirectories allowed + if (path.dirname(resolvedTarget) !== resolvedBase) { + throw new Error('Path traversal detected: target path must be direct child of base directory') + } + + return resolvedTarget +} + // Type definitions export interface DxtManifest { dxt_version: string @@ -68,6 +89,76 @@ export interface DxtUploadResult { error?: string } +/** + * Validate and sanitize a command to prevent path traversal attacks. + * Commands should be either: + * 1. Simple command names (e.g., "node", "python", "npx") - looked up in PATH + * 2. Absolute paths (e.g., "/usr/bin/node", "C:\\Program Files\\node\\node.exe") + * 3. Relative paths starting with ./ or .\ (relative to extractDir) + * + * Rejects commands containing path traversal sequences (..) + * + * @param command - The command to validate + * @returns The validated command + * @throws Error if command contains path traversal or is invalid + */ +export function validateCommand(command: string): string { + if (!command || typeof command !== 'string') { + throw new Error('Invalid command: command must be a non-empty string') + } + + const trimmed = command.trim() + if (!trimmed) { + throw new Error('Invalid command: command cannot be empty') + } + + // Check for path traversal sequences + // This catches: .., ../, ..\, /../, \..\, etc. + if (/(?:^|[/\\])\.\.(?:[/\\]|$)/.test(trimmed) || trimmed === '..') { + throw new Error(`Invalid command: path traversal detected in "${command}"`) + } + + // Check for null bytes + if (trimmed.includes('\0')) { + throw new Error('Invalid command: null byte detected') + } + + return trimmed +} + +/** + * Validate command arguments to prevent injection attacks. + * Rejects arguments containing path traversal sequences. + * + * @param args - The arguments array to validate + * @returns The validated arguments array + * @throws Error if any argument contains path traversal + */ +export function validateArgs(args: string[]): string[] { + if (!Array.isArray(args)) { + throw new Error('Invalid args: must be an array') + } + + return args.map((arg, index) => { + if (typeof arg !== 'string') { + throw new Error(`Invalid args: argument at index ${index} must be a string`) + } + + // Check for null bytes + if (arg.includes('\0')) { + throw new Error(`Invalid args: null byte detected in argument at index ${index}`) + } + + // Check for path traversal in arguments that look like paths + // Only validate if the arg contains path separators (indicating it's meant to be a path) + if ((arg.includes('/') || arg.includes('\\')) && /(?:^|[/\\])\.\.(?:[/\\]|$)/.test(arg)) { + throw new Error(`Invalid args: path traversal detected in argument at index ${index}`) + } + + return arg + }) +} + export function performVariableSubstitution( value: string, extractDir: string, @@ -134,12 +225,16 @@ export function applyPlatformOverrides(mcpConfig: any, extractDir: string, userC // Apply variable substitution to all string values if (resolvedConfig.command) { resolvedConfig.command = performVariableSubstitution(resolvedConfig.command, extractDir, userConfig) + // Validate command after substitution to prevent path traversal attacks + resolvedConfig.command = validateCommand(resolvedConfig.command) } if (resolvedConfig.args) { resolvedConfig.args = resolvedConfig.args.map((arg: string) => performVariableSubstitution(arg, extractDir, userConfig) ) + // Validate args after substitution to prevent path traversal attacks + resolvedConfig.args = validateArgs(resolvedConfig.args) } if (resolvedConfig.env) { @@ -271,10 +366,8 @@ class DxtService { } // Use server name as the final extract directory for automatic version management - // Sanitize the name to prevent creating subdirectories - const sanitizedName = manifest.name.replace(/\//g, '-') - const serverDirName = `server-${sanitizedName}` - const finalExtractDir = path.join(this.mcpDir, serverDirName) + const serverDirName = `server-${manifest.name}` + const finalExtractDir = ensurePathWithin(this.mcpDir, path.join(this.mcpDir, serverDirName)) // Clean up any existing version of this server if (fs.existsSync(finalExtractDir)) { @@ -354,27 +447,15 @@ class DxtService { public cleanupDxtServer(serverName: string): boolean { try { - // Handle server names that might contain slashes (e.g., "anthropic/sequential-thinking") - // by replacing slashes with the same separator used during installation - const sanitizedName = serverName.replace(/\//g, '-') - const serverDirName = `server-${sanitizedName}` - const serverDir = path.join(this.mcpDir, serverDirName) + const serverDirName = `server-${serverName}` + const serverDir = ensurePathWithin(this.mcpDir, path.join(this.mcpDir, serverDirName)) - // First try the sanitized path if (fs.existsSync(serverDir)) { logger.debug(`Removing DXT server directory: ${serverDir}`) fs.rmSync(serverDir, { recursive: true, force: true }) return true } - // Fallback: try with original name in case it was stored differently - const originalServerDir = path.join(this.mcpDir, `server-${serverName}`) - if (fs.existsSync(originalServerDir)) { - logger.debug(`Removing DXT server directory: ${originalServerDir}`) - fs.rmSync(originalServerDir, { recursive: true, force: true }) - return true - } - logger.warn(`Server directory not found: ${serverDir}`) return false } catch (error) { diff --git a/src/main/services/__tests__/DxtService.test.ts b/src/main/services/__tests__/DxtService.test.ts new file mode 100644 index 0000000000..90873152fa --- /dev/null +++ b/src/main/services/__tests__/DxtService.test.ts @@ -0,0 +1,202 @@ +import path from 'path' +import { describe, expect, it } from 'vitest' + +import { ensurePathWithin, validateArgs, validateCommand } from '../DxtService' + +describe('ensurePathWithin', () => { + const baseDir = '/home/user/mcp' + + describe('valid paths', () => { + it('should accept direct child paths', () => { + expect(ensurePathWithin(baseDir, '/home/user/mcp/server-test')).toBe('/home/user/mcp/server-test') + expect(ensurePathWithin(baseDir, '/home/user/mcp/my-server')).toBe('/home/user/mcp/my-server') + }) + + it('should accept paths with unicode characters', () => { + expect(ensurePathWithin(baseDir, '/home/user/mcp/服务器')).toBe('/home/user/mcp/服务器') + expect(ensurePathWithin(baseDir, '/home/user/mcp/サーバー')).toBe('/home/user/mcp/サーバー') + }) + }) + + describe('path traversal prevention', () => { + it('should reject paths that escape base directory', () => { + expect(() => ensurePathWithin(baseDir, '/home/user/mcp/../../../etc')).toThrow('Path traversal detected') + expect(() => ensurePathWithin(baseDir, '/etc/passwd')).toThrow('Path traversal detected') + expect(() => ensurePathWithin(baseDir, '/home/user')).toThrow('Path traversal detected') + }) + + it('should reject subdirectories', () => { + expect(() => ensurePathWithin(baseDir, '/home/user/mcp/sub/dir')).toThrow('Path traversal detected') + expect(() => ensurePathWithin(baseDir, '/home/user/mcp/a/b/c')).toThrow('Path traversal detected') + }) + + it('should reject Windows-style path traversal', () => { + const winBase = 'C:\\Users\\user\\mcp' + expect(() => ensurePathWithin(winBase, 'C:\\Users\\user\\mcp\\..\\..\\Windows\\System32')).toThrow( + 'Path traversal detected' + ) + }) + + it('should reject null byte attacks', () => { + const maliciousPath = path.join(baseDir, 'server\x00/../../../etc/passwd') + expect(() => ensurePathWithin(baseDir, maliciousPath)).toThrow('Path traversal detected') + }) + + it('should handle encoded traversal attempts', () => { + expect(() => ensurePathWithin(baseDir, '/home/user/mcp/../escape')).toThrow('Path traversal detected') + }) + }) + + describe('edge cases', () => { + it('should reject base directory itself', () => { + expect(() => ensurePathWithin(baseDir, '/home/user/mcp')).toThrow('Path traversal detected') + }) + + it('should handle relative path construction', () => { + const target = path.join(baseDir, 'server-name') + expect(ensurePathWithin(baseDir, target)).toBe('/home/user/mcp/server-name') + }) + }) +}) + +describe('validateCommand', () => { + describe('valid commands', () => { + it('should accept simple command names', () => { + expect(validateCommand('node')).toBe('node') + expect(validateCommand('python')).toBe('python') + expect(validateCommand('npx')).toBe('npx') + expect(validateCommand('uvx')).toBe('uvx') + }) + + it('should accept absolute paths', () => { + expect(validateCommand('/usr/bin/node')).toBe('/usr/bin/node') + expect(validateCommand('/usr/local/bin/python3')).toBe('/usr/local/bin/python3') + expect(validateCommand('C:\\Program Files\\nodejs\\node.exe')).toBe('C:\\Program Files\\nodejs\\node.exe') + }) + + it('should accept relative paths starting with ./', () => { + expect(validateCommand('./node_modules/.bin/tsc')).toBe('./node_modules/.bin/tsc') + expect(validateCommand('.\\scripts\\run.bat')).toBe('.\\scripts\\run.bat') + }) + + it('should trim whitespace', () => { + expect(validateCommand(' node ')).toBe('node') + expect(validateCommand('\tpython\n')).toBe('python') + }) + }) + + describe('path traversal prevention', () => { + it('should reject commands with path traversal (Unix style)', () => { + expect(() => validateCommand('../../../bin/sh')).toThrow('path traversal detected') + expect(() => validateCommand('../../etc/passwd')).toThrow('path traversal detected') + expect(() => validateCommand('/usr/../../../bin/sh')).toThrow('path traversal detected') + }) + + it('should reject commands with path traversal (Windows style)', () => { + expect(() => validateCommand('..\\..\\..\\Windows\\System32\\cmd.exe')).toThrow('path traversal detected') + expect(() => validateCommand('..\\..\\Windows\\System32\\calc.exe')).toThrow('path traversal detected') + expect(() => validateCommand('C:\\..\\..\\Windows\\System32\\cmd.exe')).toThrow('path traversal detected') + }) + + it('should reject just ".."', () => { + expect(() => validateCommand('..')).toThrow('path traversal detected') + }) + + it('should reject mixed style path traversal', () => { + expect(() => validateCommand('../..\\mixed/..\\attack')).toThrow('path traversal detected') + }) + }) + + describe('null byte injection', () => { + it('should reject commands with null bytes', () => { + expect(() => validateCommand('node\x00.exe')).toThrow('null byte detected') + expect(() => validateCommand('python\0')).toThrow('null byte detected') + }) + }) + + describe('edge cases', () => { + it('should reject empty strings', () => { + expect(() => validateCommand('')).toThrow('command must be a non-empty string') + expect(() => validateCommand(' ')).toThrow('command cannot be empty') + }) + + it('should reject non-string input', () => { + // @ts-expect-error - testing runtime behavior + expect(() => validateCommand(null)).toThrow('command must be a non-empty string') + // @ts-expect-error - testing runtime behavior + expect(() => validateCommand(undefined)).toThrow('command must be a non-empty string') + // @ts-expect-error - testing runtime behavior + expect(() => validateCommand(123)).toThrow('command must be a non-empty string') + }) + }) + + describe('real-world attack scenarios', () => { + it('should prevent Windows system32 command injection', () => { + expect(() => validateCommand('../../../../Windows/System32/cmd.exe')).toThrow('path traversal detected') + expect(() => validateCommand('..\\..\\..\\..\\Windows\\System32\\powershell.exe')).toThrow( + 'path traversal detected' + ) + }) + + it('should prevent Unix bin injection', () => { + expect(() => validateCommand('../../../../bin/bash')).toThrow('path traversal detected') + expect(() => validateCommand('../../../usr/bin/curl')).toThrow('path traversal detected') + }) + }) +}) + +describe('validateArgs', () => { + describe('valid arguments', () => { + it('should accept normal arguments', () => { + expect(validateArgs(['--version'])).toEqual(['--version']) + expect(validateArgs(['-y', '@anthropic/mcp-server'])).toEqual(['-y', '@anthropic/mcp-server']) + expect(validateArgs(['install', 'package-name'])).toEqual(['install', 'package-name']) + }) + + it('should accept arguments with safe paths', () => { + expect(validateArgs(['./src/index.ts'])).toEqual(['./src/index.ts']) + expect(validateArgs(['/absolute/path/file.js'])).toEqual(['/absolute/path/file.js']) + }) + + it('should accept empty array', () => { + expect(validateArgs([])).toEqual([]) + }) + }) + + describe('path traversal prevention', () => { + it('should reject arguments with path traversal', () => { + expect(() => validateArgs(['../../../etc/passwd'])).toThrow('path traversal detected') + expect(() => validateArgs(['--config', '../../secrets.json'])).toThrow('path traversal detected') + expect(() => validateArgs(['..\\..\\Windows\\System32\\config'])).toThrow('path traversal detected') + }) + + it('should only check path-like arguments', () => { + // Arguments without path separators should pass even with dots + expect(validateArgs(['..version'])).toEqual(['..version']) + expect(validateArgs(['test..name'])).toEqual(['test..name']) + }) + }) + + describe('null byte injection', () => { + it('should reject arguments with null bytes', () => { + expect(() => validateArgs(['file\x00.txt'])).toThrow('null byte detected') + expect(() => validateArgs(['--config', 'path\0name'])).toThrow('null byte detected') + }) + }) + + describe('edge cases', () => { + it('should reject non-array input', () => { + // @ts-expect-error - testing runtime behavior + expect(() => validateArgs('not an array')).toThrow('must be an array') + // @ts-expect-error - testing runtime behavior + expect(() => validateArgs(null)).toThrow('must be an array') + }) + + it('should reject non-string elements', () => { + // @ts-expect-error - testing runtime behavior + expect(() => validateArgs([123])).toThrow('must be a string') + // @ts-expect-error - testing runtime behavior + expect(() => validateArgs(['valid', null])).toThrow('must be a string') + }) + }) +})