From 4d28f1485dd00bcfe3df6de6404aef9f08001174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Theodor=20N=2E=20Eng=C3=B8y?= Date: Sun, 8 Feb 2026 03:15:59 +0100 Subject: [PATCH 1/2] cli: avoid shell=True for npx/browser helpers --- .../clients/url_elicitation_client.py | 11 ++--- src/mcp/cli/cli.py | 43 +++++++++++-------- tests/cli/test_utils.py | 33 +++++++------- 3 files changed, 46 insertions(+), 41 deletions(-) diff --git a/examples/snippets/clients/url_elicitation_client.py b/examples/snippets/clients/url_elicitation_client.py index 9888c588e..282aec461 100644 --- a/examples/snippets/clients/url_elicitation_client.py +++ b/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}") diff --git a/src/mcp/cli/cli.py b/src/mcp/cli/cli.py index 858ab7db2..8bdf2f50b 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,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: diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py index 44f4ab4d3..5671c1f01 100644 --- a/tests/cli/test_utils.py +++ b/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 From 61d0c52145880d6ab58ec6a224bfbaa980517d18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Theodor=20N=2E=20Eng=C3=B8y?= Date: Sun, 8 Feb 2026 03:28:32 +0100 Subject: [PATCH 2/2] tests: type annotate fake shutil.which for pyright --- tests/cli/test_utils.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py index 5671c1f01..39ab11ec4 100644 --- a/tests/cli/test_utils.py +++ b/tests/cli/test_utils.py @@ -78,7 +78,11 @@ def test_get_npx_windows(monkeypatch: pytest.MonkeyPatch): import shutil monkeypatch.setattr(sys, "platform", "win32") - monkeypatch.setattr(shutil, "which", lambda name: "C:\\bin\\npx.exe" if name == "npx.exe" else None) + + def fake_which(name: str) -> str | None: + return "C:\\bin\\npx.exe" if name == "npx.exe" else None + + monkeypatch.setattr(shutil, "which", fake_which) assert _get_npx_command() == ["C:\\bin\\npx.exe"] @@ -88,7 +92,11 @@ def test_get_npx_windows_cmd_wrapper(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr(sys, "platform", "win32") monkeypatch.setenv("COMSPEC", "cmd.exe") - monkeypatch.setattr(shutil, "which", lambda name: "C:\\bin\\npx.cmd" if name == "npx.cmd" else None) + + def fake_which(name: str) -> str | None: + return "C:\\bin\\npx.cmd" if name == "npx.cmd" else None + + monkeypatch.setattr(shutil, "which", fake_which) assert _get_npx_command() == ["cmd.exe", "/c", "C:\\bin\\npx.cmd"] @@ -98,5 +106,8 @@ def test_get_npx_returns_none_when_npx_missing(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr(sys, "platform", "win32", raising=False) import shutil - monkeypatch.setattr(shutil, "which", lambda name: None) + def fake_which(name: str) -> str | None: + return None + + monkeypatch.setattr(shutil, "which", fake_which) assert _get_npx_command() is None