fix: port Windows hook safety fixes (#1719)

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);
   });
 }
 

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');
 

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' } });

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);
     }