From d199d49e5211ed3b2920a48e1269811df4a9fab1 Mon Sep 17 00:00:00 2001
From: xianren <lqsxianren@qq.com>
Date: Thu, 26 Mar 2026 16:27:07 +0800
Subject: [PATCH] fix(security): remove shell=True to prevent command injection
 on Windows

Replaces shell=True subprocess calls with explicit argument lists to
prevent command injection vulnerabilities, particularly on Windows
where shell metacharacters can be exploited.

In _get_npx_command(), replaced subprocess.run() with shutil.which()
to locate the npx executable without spawning a shell process. This is
both safer and more efficient.

In dev(), removed the conditional shell=True for Windows since
_get_npx_command() already resolves the correct executable name
(e.g. npx.cmd), making shell dispatch unnecessary.

Fixes #1257

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 src/mcp/cli/cli.py      | 18 +++++++++---------
 tests/cli/test_utils.py | 22 +++++++++-------------
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/src/mcp/cli/cli.py b/src/mcp/cli/cli.py
index 62334a4a2..dd3cb57a1 100644
--- a/src/mcp/cli/cli.py
+++ b/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,16 +40,14 @@
 )
 
 
-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
+        # Use shutil.which() to safely locate the executable on Windows
+        # without invoking a shell, preventing command injection risks.
         for cmd in ["npx.cmd", "npx.exe", "npx"]:
-            try:
-                subprocess.run([cmd, "--version"], check=True, capture_output=True, shell=True)
+            if shutil.which(cmd) is not None:
                 return cmd
-            except subprocess.CalledProcessError:
-                continue
         return None
     return "npx"  # On Unix-like systems, just use npx
 
@@ -271,12 +270,13 @@ def dev(
             )
             sys.exit(1)
 
-        # Run the MCP Inspector command with shell=True on Windows
-        shell = sys.platform == "win32"
+        # Run the MCP Inspector command without shell=True to prevent
+        # command injection via shell metacharacters (see #1257).
+        # _get_npx_command() already resolves the correct executable
+        # (e.g. npx.cmd on Windows), so shell dispatch is unnecessary.
         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)
diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py
index 44f4ab4d3..c8601af9d 100644
--- a/tests/cli/test_utils.py
+++ b/tests/cli/test_utils.py
@@ -1,3 +1,4 @@
+import shutil
 import subprocess
 import sys
 from pathlib import Path
@@ -76,26 +77,21 @@ 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."""
+    """Should return one of the npx candidates on Windows via shutil.which()."""
     candidates = ["npx.cmd", "npx.exe", "npx"]
 
-    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])
+    def fake_which(cmd: str) -> str | None:
+        if cmd in candidates:
+            return f"/fake/path/{cmd}"
+        return None  # pragma: no cover
 
     monkeypatch.setattr(sys, "platform", "win32")
-    monkeypatch.setattr(subprocess, "run", fake_run)
+    monkeypatch.setattr(shutil, "which", fake_which)
     assert _get_npx_command() in candidates
 
 
 def test_get_npx_returns_none_when_npx_missing(monkeypatch: pytest.MonkeyPatch):
-    """Should give None if every candidate fails."""
+    """Should give None if every candidate is absent from PATH."""
     monkeypatch.setattr(sys, "platform", "win32", raising=False)
-
-    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 cmd: None)
     assert _get_npx_command() is None