diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index 76da6b5d..3e763d05 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -306,126 +306,175 @@ function probeCommandServer(serverName, config) { ...(config.env && typeof config.env === 'object' && !Array.isArray(config.env) ? config.env : {}) }; - let stderr = ''; let done = false; - let timer = null; function finish(result) { if (done) return; done = true; - if (timer) { - clearTimeout(timer); - timer = null; - } resolve(result); } - // On Windows, commands like 'npx' are actually 'npx.cmd' batch files that - // require shell expansion to resolve. However, absolute paths (e.g. - // 'C:\Program Files\nodejs\node.exe') must NOT use shell mode because - // cmd.exe misparses paths containing spaces. Only enable shell for - // non-absolute commands that need PATH resolution. - // - // Security: validate the command for shell metacharacters before enabling - // shell mode. cmd.exe treats &, |, <, >, ^, %, !, (, ), ;, and whitespace - // as operators/separators. A crafted command value from an MCP config file - // could otherwise inject arbitrary shell commands. + // On Windows, commands like 'npx' are commonly exposed as npx.cmd. + // Probe bare PATH commands through platform-extension fallbacks, but keep + // absolute/relative path commands as a single candidate so their existing + // ENOENT failure semantics stay intact. + const commandIsString = typeof command === 'string' && command.length > 0; + const isPathLike = commandIsString && ( + path.isAbsolute(command) + || command.includes('/') + || command.includes('\\') + ); + const candidates = process.platform === 'win32' + && commandIsString + && !path.extname(command) + && !isPathLike + ? [command, `${command}.cmd`, `${command}.exe`, `${command}.bat`] + : [command]; + + // cmd.exe treats these as operators, grouping syntax, expansion markers, + // separators, or argument boundaries. Do not route such command strings + // through shell mode. const UNSAFE_SHELL_CHARS = /[&|<>^%!()\s;]/; - const needsShell = - process.platform === 'win32' && - typeof command === 'string' && - !path.isAbsolute(command) && - !UNSAFE_SHELL_CHARS.test(command); - let child; - try { - child = spawn(command, args, { - env: mergedEnv, - cwd: process.cwd(), - stdio: ['pipe', 'ignore', 'pipe'], - shell: needsShell - }); - } catch (error) { - finish({ - ok: false, - statusCode: null, - reason: error.message - }); - return; - } - child.stderr.on('data', chunk => { - if (stderr.length < 4000) { - const remaining = 4000 - stderr.length; - stderr += String(chunk).slice(0, remaining); + function attempt(idx) { + const tryCommand = candidates[idx]; + const isLast = idx + 1 >= candidates.length; + let stderr = ''; + let attemptDone = false; + let timer = null; + + function retryNext() { + if (attemptDone) return; + attemptDone = true; + if (timer) { + clearTimeout(timer); + timer = null; + } + attempt(idx + 1); } - }); - child.on('error', error => { - finish({ - ok: false, - statusCode: null, - reason: error.message - }); - }); + function attemptFinish(result) { + if (attemptDone) return; + attemptDone = true; + if (timer) { + clearTimeout(timer); + timer = null; + } + finish(result); + } - child.on('exit', (code, signal) => { - finish({ - ok: false, - statusCode: code, - reason: stderr.trim() || `process exited before handshake (${signal || code || 'unknown'})` - }); - }); + // Node 18.20+/20.12+ refuse to spawn .cmd/.bat directly on Windows + // after the CVE-2024-27980 mitigation. Only those extension candidates + // go through cmd.exe, after the command string is shell-character clean. + const useShell = process.platform === 'win32' + && typeof tryCommand === 'string' + && /\.(cmd|bat)$/i.test(tryCommand) + && !UNSAFE_SHELL_CHARS.test(tryCommand); - timer = setTimeout(() => { - // A fast-crashing stdio server can finish before the timer callback runs - // on a loaded machine. Check the process state again before classifying it - // as healthy on timeout. - if (child.exitCode !== null || child.signalCode !== null) { - finish({ + let child; + try { + child = spawn(tryCommand, args, { + env: mergedEnv, + cwd: process.cwd(), + stdio: ['pipe', 'ignore', 'pipe'], + shell: useShell + }); + } catch (error) { + if ((error.code === 'ENOENT' || error.code === 'EINVAL') && !isLast) { + retryNext(); + return; + } + attemptFinish({ ok: false, - statusCode: child.exitCode, - reason: stderr.trim() || `process exited before handshake (${child.signalCode || child.exitCode || 'unknown'})` + statusCode: null, + reason: error.message }); return; } - try { - if (needsShell && child.pid && process.platform === 'win32') { - // When spawned via shell on Windows, child is cmd.exe. kill() only - // terminates the shell and leaves the real server process orphaned. - // taskkill /T kills the entire process tree rooted at cmd.exe. - const killResult = spawnSync('taskkill', ['/PID', String(child.pid), '/T', '/F'], { stdio: 'ignore' }); - if (killResult.error || (typeof killResult.status === 'number' && killResult.status !== 0)) { - // taskkill not on PATH, permission denied, or already exited. - // Best-effort fallback: signal the cmd.exe shell directly. The - // child tree may still leak if it already detached, but this at - // least kills the shell we spawned. - try { child.kill('SIGKILL'); } catch { /* ignore */ } - } - } else { - child.kill('SIGTERM'); - setTimeout(() => { - try { - child.kill('SIGKILL'); - } catch { - // ignore - } - }, 200).unref?.(); + child.stderr.on('data', chunk => { + if (stderr.length < 4000) { + const remaining = 4000 - stderr.length; + stderr += String(chunk).slice(0, remaining); } - } catch { - // ignore - } - - finish({ - ok: true, - statusCode: null, - reason: `${serverName} accepted a new stdio process` }); - }, timeoutMs); - if (typeof timer.unref === 'function') { - timer.unref(); + child.on('error', error => { + if ((error.code === 'ENOENT' || error.code === 'EINVAL') && !isLast) { + retryNext(); + return; + } + attemptFinish({ + ok: false, + statusCode: null, + reason: error.message + }); + }); + + child.on('exit', (code, signal) => { + attemptFinish({ + ok: false, + statusCode: code, + reason: stderr.trim() || `process exited before handshake (${signal || code || 'unknown'})` + }); + }); + + timer = setTimeout(() => { + // A fast-crashing stdio server can finish before the timer callback runs + // on a loaded machine. Check the process state again before classifying it + // as healthy on timeout. + if (child.exitCode !== null || child.signalCode !== null) { + attemptFinish({ + ok: false, + statusCode: child.exitCode, + reason: stderr.trim() || `process exited before handshake (${child.signalCode || child.exitCode || 'unknown'})` + }); + return; + } + + try { + if (useShell && child.pid && process.platform === 'win32') { + // When spawned via shell on Windows, child is cmd.exe. kill() only + // terminates the shell and leaves the real server process orphaned. + // taskkill /T kills the entire process tree rooted at cmd.exe. + const killResult = spawnSync('taskkill', ['/PID', String(child.pid), '/T', '/F'], { + stdio: 'ignore', + windowsHide: true + }); + if (killResult.error || (typeof killResult.status === 'number' && killResult.status !== 0)) { + // taskkill not on PATH, permission denied, or already exited. + // Best-effort fallback: signal the cmd.exe shell directly. The + // child tree may still leak if it already detached, but this at + // least kills the shell we spawned. + try { child.kill('SIGKILL'); } catch { /* ignore */ } + } + } else { + child.kill('SIGTERM'); + setTimeout(() => { + try { + child.kill('SIGKILL'); + } catch { + // ignore + } + }, 200).unref?.(); + } + } catch { + // ignore + } + + attemptFinish({ + ok: true, + statusCode: null, + reason: `${serverName} accepted a new stdio process` + }); + }, timeoutMs); + + if (typeof timer.unref === 'function') { + timer.unref(); + } } + + attempt(0); }); } diff --git a/scripts/hooks/post-edit-format.js b/scripts/hooks/post-edit-format.js index d6486866..26a79f93 100644 --- a/scripts/hooks/post-edit-format.js +++ b/scripts/hooks/post-edit-format.js @@ -21,7 +21,7 @@ const { execFileSync, spawnSync } = require('child_process'); const path = require('path'); // Shell metacharacters that cmd.exe interprets as command separators/operators -const UNSAFE_PATH_CHARS = /[&|<>^%!]/; +const UNSAFE_PATH_CHARS = /[&|<>^%!;`()$]/; const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter'); diff --git a/tests/hooks/hooks.test.js b/tests/hooks/hooks.test.js index 0142a876..9f2767d7 100644 --- a/tests/hooks/hooks.test.js +++ b/tests/hooks/hooks.test.js @@ -2732,6 +2732,68 @@ async function runTests() { passed++; else failed++; + if ( + await asyncTest('blocks Windows shell metacharacters before shell:true formatter execution', async () => { + const hookPath = path.join(scriptsDir, 'post-edit-format.js'); + const resolverPath = path.join(scriptsDir, '..', 'lib', 'resolve-formatter.js'); + const childProcess = require('child_process'); + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + const originalSpawnSync = childProcess.spawnSync; + const originalExecFileSync = childProcess.execFileSync; + const resolvedResolverPath = require.resolve(resolverPath); + const resolvedHookPath = require.resolve(hookPath); + const originalResolverCache = require.cache[resolvedResolverPath]; + const originalHookCache = require.cache[resolvedHookPath]; + const blockedPaths = ['semicolon;test.js', 'backtick`test.js', 'subshell$(test).js', 'group(test).js']; + + try { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + + let spawnCalls = []; + childProcess.spawnSync = (...args) => { + spawnCalls.push(args); + return { status: 0, stderr: Buffer.from('') }; + }; + childProcess.execFileSync = () => { + throw new Error('execFileSync should not run for Windows .cmd formatter shims'); + }; + + require.cache[resolvedResolverPath] = { + id: resolvedResolverPath, + filename: resolvedResolverPath, + loaded: true, + exports: { + findProjectRoot: () => process.cwd(), + detectFormatter: () => 'prettier', + resolveFormatterBin: () => ({ bin: 'formatter.cmd', prefix: [] }) + } + }; + delete require.cache[resolvedHookPath]; + + const { run } = require(hookPath); + + for (const filePath of blockedPaths) { + spawnCalls = []; + const stdinJson = JSON.stringify({ tool_input: { file_path: filePath } }); + assert.strictEqual(run(stdinJson), stdinJson, 'Should pass through original stdin JSON'); + assert.strictEqual(spawnCalls.length, 0, `Should reject ${filePath} before spawnSync`); + } + } finally { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform); + } + childProcess.spawnSync = originalSpawnSync; + childProcess.execFileSync = originalExecFileSync; + if (originalResolverCache) require.cache[resolvedResolverPath] = originalResolverCache; + else delete require.cache[resolvedResolverPath]; + if (originalHookCache) require.cache[resolvedHookPath] = originalHookCache; + else delete require.cache[resolvedHookPath]; + } + }) + ) + passed++; + else failed++; + if ( await asyncTest('matches .tsx extension for formatting', async () => { const stdinJson = JSON.stringify({ tool_input: { file_path: '/nonexistent/component.tsx' } }); diff --git a/tests/hooks/mcp-health-check.test.js b/tests/hooks/mcp-health-check.test.js index 958221c1..02145ade 100644 --- a/tests/hooks/mcp-health-check.test.js +++ b/tests/hooks/mcp-health-check.test.js @@ -952,6 +952,65 @@ async function runTests() { } })) passed++; else failed++; + // Windows-only: child_process.spawn cannot resolve .cmd/.bat shims for + // bare PATH commands without an extension, and Node 18.20+/20.12+ refuse + // to spawn .cmd targets without `shell: true` (CVE-2024-27980). The probe + // must retry bare command names with platform extensions and route .cmd/.bat + // through the shell, otherwise tools like `npx` are misclassified as + // unhealthy on first use. Path-like commands keep single-candidate ENOENT + // semantics. + if (process.platform === 'win32') { + if (await asyncTest('windows: probes bare PATH commands via .cmd fallback', async () => { + const tempDir = createTempDir(); + const binDir = path.join(tempDir, 'bin'); + const configPath = path.join(tempDir, 'claude.json'); + const statePath = path.join(tempDir, 'mcp-health.json'); + + fs.mkdirSync(binDir, { recursive: true }); + fs.writeFileSync( + path.join(binDir, 'winfallback.cmd'), + ['@echo off', 'node -e "setInterval(()=>{},1000)"', ''].join('\r\n') + ); + + try { + writeConfig(configPath, { + mcpServers: { + winfallback: { + command: 'winfallback', + args: [] + } + } + }); + + const input = { tool_name: 'mcp__winfallback__list', tool_input: {} }; + const result = runHook(input, { + CLAUDE_HOOK_EVENT_NAME: 'PreToolUse', + ECC_MCP_CONFIG_PATH: configPath, + ECC_MCP_HEALTH_STATE_PATH: statePath, + ECC_MCP_HEALTH_TIMEOUT_MS: '500', + PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}` + }); + + assert.strictEqual( + result.code, + 0, + `Expected bare command to be probed via .cmd fallback: ${hookFailureDetails(result, statePath)}` + ); + + const state = readState(statePath); + assert.strictEqual( + state.servers.winfallback.status, + 'healthy', + 'Expected bare command to be marked healthy via .cmd fallback' + ); + } finally { + cleanupTempDir(tempDir); + } + })) passed++; else failed++; + } else { + console.log(' - skipped: windows: probes bare PATH commands via .cmd fallback (non-Windows)'); + } + if (await asyncTest('probes command servers using non-absolute commands (e.g. npx) via PATH resolution', async () => { const tempDir = createTempDir(); const configPath = path.join(tempDir, 'claude.json'); @@ -962,10 +1021,8 @@ async function runTests() { // Create a server script that stays alive fs.writeFileSync(serverScript, "setInterval(() => {}, 1000);\n"); - // Use 'node' (non-absolute) as the command to exercise PATH-based resolution. - // On Windows, shell execution is especially relevant for commands exposed via - // batch wrappers such as 'npx.cmd'; using 'node' here simulates that class of - // non-absolute command without depending on npx being available in the environment. + // Use 'node' (non-absolute) as the command to exercise PATH-based + // resolution without depending on npx being available in the environment. writeConfig(configPath, { mcpServers: { shelltest: { @@ -983,10 +1040,10 @@ async function runTests() { ECC_MCP_HEALTH_TIMEOUT_MS: '100' }); - assert.strictEqual(result.code, 0, `Expected non-absolute command to resolve via shell, got ${result.code}`); + assert.strictEqual(result.code, 0, `Expected non-absolute command to resolve via PATH, got ${result.code}`); const state = readState(statePath); - assert.strictEqual(state.servers.shelltest.status, 'healthy', 'Expected shell-resolved server to be marked healthy'); + assert.strictEqual(state.servers.shelltest.status, 'healthy', 'Expected PATH-resolved server to be marked healthy'); } finally { cleanupTempDir(tempDir); }