diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index c47d3880..76da6b5d 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -320,12 +320,29 @@ function probeCommandServer(serverName, config) { 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. + 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'] + stdio: ['pipe', 'ignore', 'pipe'], + shell: needsShell }); } catch (error) { finish({ @@ -373,19 +390,32 @@ function probeCommandServer(serverName, config) { } try { - child.kill('SIGTERM'); + 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?.(); + } } catch { // ignore } - setTimeout(() => { - try { - child.kill('SIGKILL'); - } catch { - // ignore - } - }, 200).unref?.(); - finish({ ok: true, statusCode: null, diff --git a/tests/hooks/mcp-health-check.test.js b/tests/hooks/mcp-health-check.test.js index 118b8706..958221c1 100644 --- a/tests/hooks/mcp-health-check.test.js +++ b/tests/hooks/mcp-health-check.test.js @@ -952,6 +952,46 @@ async function runTests() { } })) passed++; else failed++; + 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'); + const statePath = path.join(tempDir, 'mcp-health.json'); + const serverScript = path.join(tempDir, 'shell-server.js'); + + try { + // 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. + writeConfig(configPath, { + mcpServers: { + shelltest: { + command: 'node', + args: [serverScript] + } + } + }); + + const input = { tool_name: 'mcp__shelltest__ping', 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: '100' + }); + + assert.strictEqual(result.code, 0, `Expected non-absolute command to resolve via shell, got ${result.code}`); + + const state = readState(statePath); + assert.strictEqual(state.servers.shelltest.status, 'healthy', 'Expected shell-resolved server to be marked healthy'); + } finally { + cleanupTempDir(tempDir); + } + })) passed++; else failed++; + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); }