cli: avoid shell=True for npx/browser helpers

Author: Theodor N. Engøy

examples/snippets/clients/url_elicitation_client.py

@@ -24,8 +24,6 @@
 
 import asyncio
 import json
-import subprocess
-import sys
 import webbrowser
 from typing import Any
 from urllib.parse import urlparse
@@ -124,12 +122,9 @@ def extract_domain(url: str) -> str:
 def open_browser(url: str) -> None:
     """Open URL in the default browser."""
     try:
-        if sys.platform == "darwin":
-            subprocess.run(["open", url], check=False)
-        elif sys.platform == "win32":
-            subprocess.run(["start", url], shell=True, check=False)
-        else:
-            webbrowser.open(url)
+        # Use the stdlib helper which handles macOS/Windows/Linux without
+        # invoking a shell directly.
+        webbrowser.open(url, new=2)
     except Exception as e:
         print(f"Failed to open browser: {e}")
         print(f"Please manually open: {url}")

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,18 +40,29 @@
 )
 
 
-def _get_npx_command():
-    """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"]:
-            try:
-                subprocess.run([cmd, "--version"], check=True, capture_output=True, shell=True)
-                return cmd
-            except subprocess.CalledProcessError:
-                continue
-        return None
-    return "npx"  # On Unix-like systems, just use npx
+def _get_npx_command() -> list[str] | None:
+    """Get the correct npx command for the current platform.
+
+    Returns a subprocess-compatible command prefix. This avoids shell=True, and
+    on Windows it can wrap a .cmd/.bat shim via cmd.exe (COMSPEC) safely.
+    """
+    if sys.platform != "win32":
+        return ["npx"]  # On Unix-like systems, just use npx
+
+    # Prefer an executable if present, but fall back to the .cmd shim.
+    for cmd in ["npx.exe", "npx.cmd", "npx"]:
+        resolved = shutil.which(cmd)
+        if not resolved:
+            continue
+
+        # .cmd/.bat shims must be invoked via cmd.exe without using shell=True.
+        if resolved.lower().endswith((".cmd", ".bat")):
+            comspec = os.environ.get("COMSPEC") or "cmd.exe"
+            return [comspec, "/c", resolved]
+
+        return [resolved]
+
+    return None
 
 
 def _parse_env_var(env_var: str) -> tuple[str, str]:  # pragma: no cover
@@ -271,13 +283,10 @@ 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,
+            npx_cmd + ["@modelcontextprotocol/inspector"] + uv_cmd,
             check=True,
-            shell=shell,
-            env=dict(os.environ.items()),  # Convert to list of tuples for env update
+            env=os.environ.copy(),
         )
         sys.exit(process.returncode)
     except subprocess.CalledProcessError as e:

tests/cli/test_utils.py

@@ -1,7 +1,5 @@
-import subprocess
 import sys
 from pathlib import Path
-from typing import Any
 
 import pytest
 
@@ -72,30 +70,33 @@ def test_build_uv_command_adds_editable_and_packages():
 def test_get_npx_unix_like(monkeypatch: pytest.MonkeyPatch):
     """Should return "npx" on unix-like systems."""
     monkeypatch.setattr(sys, "platform", "linux")
-    assert _get_npx_command() == "npx"
+    assert _get_npx_command() == ["npx"]
 
 
 def test_get_npx_windows(monkeypatch: pytest.MonkeyPatch):
-    """Should return one of the npx candidates on Windows."""
-    candidates = ["npx.cmd", "npx.exe", "npx"]
+    """Should return a subprocess-friendly command prefix on Windows."""
+    import shutil
 
-    def fake_run(cmd: list[str], **kw: Any) -> subprocess.CompletedProcess[bytes]:
-        if cmd[0] 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", lambda name: "C:\\bin\\npx.exe" if name == "npx.exe" else None)
+    assert _get_npx_command() == ["C:\\bin\\npx.exe"]
+
+
+def test_get_npx_windows_cmd_wrapper(monkeypatch: pytest.MonkeyPatch):
+    """Should wrap .cmd/.bat shims via COMSPEC on Windows."""
+    import shutil
 
     monkeypatch.setattr(sys, "platform", "win32")
-    monkeypatch.setattr(subprocess, "run", fake_run)
-    assert _get_npx_command() in candidates
+    monkeypatch.setenv("COMSPEC", "cmd.exe")
+    monkeypatch.setattr(shutil, "which", lambda name: "C:\\bin\\npx.cmd" if name == "npx.cmd" else None)
+
+    assert _get_npx_command() == ["cmd.exe", "/c", "C:\\bin\\npx.cmd"]
 
 
 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)
+    import shutil
 
-    def always_fail(*args: Any, **kwargs: Any) -> subprocess.CompletedProcess[bytes]:
-        raise subprocess.CalledProcessError(1, args[0])
-
-    monkeypatch.setattr(subprocess, "run", always_fail)
+    monkeypatch.setattr(shutil, "which", lambda name: None)
     assert _get_npx_command() is None