Avoid shell invocation for Windows npx

Author: yyzquwu

src/mcp/cli/cli.py

@@ -3,6 +3,7 @@
 import importlib.metadata
 import importlib.util
 import os
+import shutil
 import subprocess
 import sys
 from pathlib import Path
@@ -39,15 +40,18 @@
 )
 
 
-def _get_npx_command():
+def _get_npx_command() -> str | None:
     """Get the correct npx command for the current platform."""
     if sys.platform == "win32":
         # Try both npx.cmd and npx.exe on Windows
         for cmd in ["npx.cmd", "npx.exe", "npx"]:
+            npx_path = shutil.which(cmd)
+            if not npx_path:
+                continue
             try:
-                subprocess.run([cmd, "--version"], check=True, capture_output=True, shell=True)
-                return cmd
-            except subprocess.CalledProcessError:
+                subprocess.run([npx_path, "--version"], check=True, capture_output=True)
+                return npx_path
+            except (OSError, subprocess.CalledProcessError):
                 continue
         return None
     return "npx"  # On Unix-like systems, just use npx
@@ -271,12 +275,9 @@ def dev(
             )
             sys.exit(1)
 
-        # Run the MCP Inspector command with shell=True on Windows
-        shell = sys.platform == "win32"
         process = subprocess.run(
             [npx_cmd, "@modelcontextprotocol/inspector"] + uv_cmd,
             check=True,
-            shell=shell,
             env=dict(os.environ.items()),  # Convert to list of tuples for env update
         )
         sys.exit(process.returncode)

tests/cli/test_utils.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+from mcp.cli import cli as cli_module
 from mcp.cli.cli import _build_uv_command, _get_npx_command, _parse_file_path  # type: ignore[reportPrivateUsage]
 
 
@@ -78,24 +79,69 @@ def test_get_npx_unix_like(monkeypatch: pytest.MonkeyPatch):
 def test_get_npx_windows(monkeypatch: pytest.MonkeyPatch):
     """Should return one of the npx candidates on Windows."""
     candidates = ["npx.cmd", "npx.exe", "npx"]
+    located = {cmd: f"C:\\Node\\{cmd}" for cmd in candidates}
 
     def fake_run(cmd: list[str], **kw: Any) -> subprocess.CompletedProcess[bytes]:
-        if cmd[0] in candidates:
+        assert kw.get("shell") is not True
+        assert cmd[0] in located.values()
+        if Path(cmd[0]).name in candidates:
             return subprocess.CompletedProcess(cmd, 0)
         else:  # pragma: no cover
             raise subprocess.CalledProcessError(1, cmd[0])
 
     monkeypatch.setattr(sys, "platform", "win32")
+    monkeypatch.setattr("shutil.which", located.get)
     monkeypatch.setattr(subprocess, "run", fake_run)
-    assert _get_npx_command() in candidates
+    assert _get_npx_command() in located.values()
+
+
+def test_get_npx_windows_skips_failed_candidates(monkeypatch: pytest.MonkeyPatch):
+    """Should keep checking candidates if one exists but fails."""
+    located = {"npx.cmd": "C:\\Node\\npx.cmd", "npx.exe": "C:\\Node\\npx.exe"}
+
+    def fake_run(cmd: list[str], **kw: Any) -> subprocess.CompletedProcess[bytes]:
+        assert kw.get("shell") is not True
+        if cmd[0].endswith("npx.cmd"):
+            raise subprocess.CalledProcessError(1, cmd)
+        return subprocess.CompletedProcess(cmd, 0)
+
+    monkeypatch.setattr(sys, "platform", "win32")
+    monkeypatch.setattr("shutil.which", located.get)
+    monkeypatch.setattr(subprocess, "run", fake_run)
+
+    assert _get_npx_command() == located["npx.exe"]
 
 
 def test_get_npx_returns_none_when_npx_missing(monkeypatch: pytest.MonkeyPatch):
     """Should give None if every candidate fails."""
     monkeypatch.setattr(sys, "platform", "win32", raising=False)
+    monkeypatch.setattr("shutil.which", lambda cmd: None)
+    assert _get_npx_command() is None
 
-    def always_fail(*args: Any, **kwargs: Any) -> subprocess.CompletedProcess[bytes]:
-        raise subprocess.CalledProcessError(1, args[0])
 
-    monkeypatch.setattr(subprocess, "run", always_fail)
-    assert _get_npx_command() is None
+def test_dev_runs_inspector_without_shell(monkeypatch: pytest.MonkeyPatch):
+    """mcp dev should not route file paths or args through a platform shell."""
+    calls: list[tuple[list[str], dict[str, Any]]] = []
+
+    class Server:
+        dependencies = ["server-dep"]
+
+    def fake_run(cmd: list[str], **kw: Any) -> subprocess.CompletedProcess[str]:
+        calls.append((cmd, kw))
+        return subprocess.CompletedProcess(cmd, 0)
+
+    monkeypatch.setattr(cli_module, "_parse_file_path", lambda file_spec: (Path("server&calc.py"), None))
+    monkeypatch.setattr(cli_module, "_import_server", lambda file, server_object: Server())
+    monkeypatch.setattr(cli_module, "_build_uv_command", lambda file_spec, with_editable, with_packages: ["uv", "run"])
+    monkeypatch.setattr(cli_module, "_get_npx_command", lambda: "C:\\Node\\npx.cmd")
+    monkeypatch.setattr(subprocess, "run", fake_run)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli_module.dev("server&calc.py", with_packages=["cli-dep"])
+
+    assert exc_info.value.code == 0
+    assert len(calls) == 1
+    cmd, kwargs = calls[0]
+    assert cmd == ["C:\\Node\\npx.cmd", "@modelcontextprotocol/inspector", "uv", "run"]
+    assert kwargs["check"] is True
+    assert "shell" not in kwargs