diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f6ad6b03..adfb075de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Added APM-managed runtime binary resolution before PATH lookup; `find_runtime_binary()` now includes path-traversal security guards via `validate_path_segments` and `ensure_path_within`. (#605) +- Added codex >= v0.116 compatibility warning for GitHub Models in `setup-codex.sh` and `setup-codex.ps1`. (#605) +- Added `LAST_COMPAT_VERSION_MINOR` constant to both Codex setup scripts so the compatibility boundary is defined once. (#605) - `apm uninstall` now accepts the same marketplace notation as `apm install` (e.g. `my-plugin@official`) -- no more `owner/repo` lookup before removing a plugin you installed by name. Refs resolve via lockfile first (offline), then registry fallback, with a supply-chain guard that refuses any registry-returned canonical not already in the lockfile. ([#1323](https://github.com/microsoft/apm/issues/1323)) - Added `--target/-t` option to `apm update` command to specify agent target (#1297) - `apm pack --marketplace=FORMATS` filters which marketplace formats are built in a single run; accepts comma-separated names and sentinels `all`/`none`. (#1317) diff --git a/docs/src/content/docs/integrations/runtime-compatibility.md b/docs/src/content/docs/integrations/runtime-compatibility.md index 53b3c8b29..0b74b4aa5 100644 --- a/docs/src/content/docs/integrations/runtime-compatibility.md +++ b/docs/src/content/docs/integrations/runtime-compatibility.md @@ -77,6 +77,13 @@ scripts: APM automatically downloads, installs, and configures the Codex CLI with GitHub Models for free usage. +> **⚠️ Compatibility Note — Codex v0.116+ and GitHub Models:** Codex v0.116 and later default +> to the OpenAI Responses API (`wire_api=responses`), which GitHub Models does not expose +> (returns 404). If you install a Codex version ≥ v0.116, change `wire_api` in +> `~/.codex/config.toml` from `"responses"` to `"chat"` to restore GitHub Models +> compatibility. Alternatively, pin to the last known-compatible release: +> `apm runtime setup codex --version rust-v0.115.0` + ### Setup #### 1. Install via APM diff --git a/scripts/runtime/setup-codex.ps1 b/scripts/runtime/setup-codex.ps1 index 2b3f61a75..973259000 100644 --- a/scripts/runtime/setup-codex.ps1 +++ b/scripts/runtime/setup-codex.ps1 @@ -24,6 +24,8 @@ if (Test-Path $tokenHelperPath) { # Configuration $CodexRepo = "openai/codex" +# Last Codex minor version that works with GitHub Models without wire_api=chat (#605) +$LastCompatVersionMinor = 115 function Install-Codex { Write-Info "Setting up Codex runtime..." @@ -85,6 +87,7 @@ function Install-Codex { } Write-Info "Using Codex release: $latestTag" + $Version = $latestTag $downloadUrl = "https://github.com/$CodexRepo/releases/download/$latestTag/codex-$codexPlatform.exe.tar.gz" } else { $downloadUrl = "https://github.com/$CodexRepo/releases/download/$Version/codex-$codexPlatform.exe.tar.gz" @@ -168,6 +171,21 @@ wire_api = "responses" Write-Success "Codex configuration created at $codexConfig" Write-Info "Using Codex $Version." + + # Version compatibility check + if ($Version -match '^rust-v0\.(\d+)') { + $codexMinor = [int]$Matches[1] + if ($codexMinor -gt $LastCompatVersionMinor) { + Write-Host "" + Write-Warning "codex >= v0.116 requires wire_api=chat configuration for GitHub Models compatibility." + Write-Warning "The generated config uses wire_api=responses, which returns 404 with GitHub Models." + Write-Warning "To fix, update wire_api in ${codexConfig}:" + Write-Warning " wire_api = `"chat`"" + Write-Warning "Or install an older compatible version: apm runtime setup codex --version rust-v0.115.0" + Write-Host "" + } + } + Write-Info "Override with: apm runtime setup codex --version (e.g. 'latest')" } else { Write-Info "Vanilla mode: Skipping APM configuration" diff --git a/scripts/runtime/setup-codex.sh b/scripts/runtime/setup-codex.sh index ac87296fc..40951229e 100755 --- a/scripts/runtime/setup-codex.sh +++ b/scripts/runtime/setup-codex.sh @@ -27,6 +27,8 @@ CODEX_REPO="openai/codex" # Users can override with: apm runtime setup codex --version (e.g. 'latest') CODEX_VERSION="rust-v0.118.0" VANILLA_MODE=false +# Last Codex minor version that works with GitHub Models without wire_api=chat (#605) +LAST_COMPAT_VERSION_MINOR=115 # Parse command line arguments while [[ $# -gt 0 ]]; do @@ -127,6 +129,7 @@ setup_codex() { fi log_info "Using Codex release: $latest_tag" + CODEX_VERSION="$latest_tag" download_url="https://github.com/$CODEX_REPO/releases/download/$latest_tag/codex-$codex_platform.tar.gz" else download_url="https://github.com/$CODEX_REPO/releases/download/$CODEX_VERSION/codex-$codex_platform.tar.gz" @@ -211,6 +214,19 @@ EOF log_success "Codex configuration created at $codex_config" log_info "Using Codex $CODEX_VERSION." + + # Version compatibility check + codex_minor=$(echo "$CODEX_VERSION" | sed -n 's/^rust-v0\.\([0-9]*\).*/\1/p') + if [ -n "$codex_minor" ] && [ "$codex_minor" -gt "$LAST_COMPAT_VERSION_MINOR" ] 2>/dev/null; then + echo "" + log_warning "codex >= v0.116 requires wire_api=chat configuration for GitHub Models compatibility." + log_warning "The generated config uses wire_api=responses, which returns 404 with GitHub Models." + log_warning "To fix, update wire_api in $codex_config:" + log_warning " wire_api = \"chat\"" + log_warning "Or install an older compatible version: apm runtime setup codex --version rust-v0.115.0" + echo "" + fi + log_info "Override with: apm runtime setup codex --version (e.g. 'latest')" log_info "APM configured Codex with GitHub Models as default provider" log_info "Use 'apm install' to configure MCP servers for your projects" diff --git a/src/apm_cli/core/script_runner.py b/src/apm_cli/core/script_runner.py index ea86d2bcf..8a70f4bef 100644 --- a/src/apm_cli/core/script_runner.py +++ b/src/apm_cli/core/script_runner.py @@ -2,7 +2,6 @@ import os import re -import shutil import subprocess import sys import time @@ -12,6 +11,7 @@ import yaml # noqa: F401 from ..output.script_formatters import ScriptExecutionFormatter +from ..runtime.utils import find_runtime_binary from .token_manager import setup_runtime_environment @@ -571,11 +571,13 @@ def _execute_runtime_command( for line in env_lines: print(line) - # Execute using argument list (no shell interpretation) with updated environment - # On Windows, resolve the executable via shutil.which() so that shell - # wrappers like copilot.cmd / copilot.ps1 are found without shell=True. - if sys.platform == "win32" and actual_command_args: - resolved = shutil.which(actual_command_args[0]) + # Resolve the runtime binary to the APM-managed path (or fallback to PATH). + # This must happen here, not in _generate_runtime_command, so the command + # string stays parseable (bare names) by _detect_runtime / _transform_runtime_command. + # find_runtime_binary checks ~/.apm/runtimes/ first, then shutil.which(), + # which also covers Windows shell wrappers (.cmd / .ps1) via PATHEXT. + if actual_command_args: + resolved = find_runtime_binary(actual_command_args[0]) if resolved: actual_command_args[0] = resolved return subprocess.run(actual_command_args, check=True, env=env_vars) @@ -921,13 +923,11 @@ def _detect_installed_runtime(self) -> str: Raises: RuntimeError: If no compatible runtime is found """ - import shutil - - if shutil.which("copilot"): + if find_runtime_binary("copilot"): return "copilot" - elif shutil.which("codex"): + elif find_runtime_binary("codex"): return "codex" - elif shutil.which("gemini"): + elif find_runtime_binary("gemini"): return "gemini" else: raise RuntimeError( diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index d9ba7c7d4..a1a4da39a 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -21,6 +21,7 @@ from apm_cli.core.null_logger import NullCommandLogger from apm_cli.deps.lockfile import LockFile, get_lockfile_path +from apm_cli.runtime.utils import find_runtime_binary from apm_cli.utils.console import ( _get_console, # noqa: F401 -- module attribute; patched by tests and used via re-export _rich_error, @@ -836,7 +837,7 @@ def _filter_runtimes(detected_runtimes: list[str]) -> list[str]: except ImportError: available = [] for rt in mcp_compatible: - if shutil.which(rt): + if find_runtime_binary(rt): available.append(rt) return available @@ -847,7 +848,7 @@ def _filter_runtimes(detected_runtimes: list[str]) -> list[str]: mcp_compatible = [ rt for rt in detected_runtimes if rt in ClientFactory.supported_clients() ] - return [rt for rt in mcp_compatible if shutil.which(rt)] + return [rt for rt in mcp_compatible if find_runtime_binary(rt)] # ------------------------------------------------------------------ # Per-runtime installation diff --git a/src/apm_cli/integration/mcp_integrator_install.py b/src/apm_cli/integration/mcp_integrator_install.py index 322a5feef..4e17e06c7 100644 --- a/src/apm_cli/integration/mcp_integrator_install.py +++ b/src/apm_cli/integration/mcp_integrator_install.py @@ -7,11 +7,11 @@ from __future__ import annotations import builtins -import shutil from pathlib import Path from typing import TYPE_CHECKING from apm_cli.core.null_logger import NullCommandLogger +from apm_cli.runtime.utils import find_runtime_binary from apm_cli.utils.console import STATUS_SYMBOLS if TYPE_CHECKING: @@ -188,7 +188,7 @@ def run_mcp_install( # noqa: PLR0915 # adapter from writing to ~/.claude.json on hosts # where Claude Code was never installed. if (project_root_path / ".claude").is_dir() or ( - shutil.which("claude") is not None + find_runtime_binary("claude") is not None ): ClientFactory.create_client(runtime_name) installed_runtimes.append(runtime_name) @@ -199,7 +199,9 @@ def run_mcp_install( # noqa: PLR0915 except (ValueError, ImportError): continue except ImportError: - installed_runtimes = [rt for rt in ["copilot", "codex"] if shutil.which(rt) is not None] + installed_runtimes = [ + rt for rt in ["copilot", "codex"] if find_runtime_binary(rt) is not None + ] # VS Code: check binary on PATH or .vscode/ directory presence if _is_vscode_available(project_root=project_root_path): installed_runtimes.append("vscode") @@ -216,7 +218,9 @@ def run_mcp_install( # noqa: PLR0915 if (project_root_path / ".windsurf").is_dir(): installed_runtimes.append("windsurf") # Claude Code: directory-presence OR binary-on-PATH - if (project_root_path / ".claude").is_dir() or (shutil.which("claude") is not None): + if (project_root_path / ".claude").is_dir() or ( + find_runtime_binary("claude") is not None + ): installed_runtimes.append("claude") # Step 2: Get runtimes referenced in apm.yml scripts diff --git a/src/apm_cli/runtime/codex_runtime.py b/src/apm_cli/runtime/codex_runtime.py index 0005a8cb1..2eb1ffd41 100644 --- a/src/apm_cli/runtime/codex_runtime.py +++ b/src/apm_cli/runtime/codex_runtime.py @@ -1,10 +1,10 @@ """Codex runtime adapter for APM.""" -import shutil import subprocess from typing import Any, Dict, Optional # noqa: F401, UP035 from .base import RuntimeAdapter +from .utils import find_runtime_binary class CodexRuntime(RuntimeAdapter): @@ -39,8 +39,9 @@ def execute_prompt(self, prompt_content: str, **kwargs) -> str: try: # Use codex exec to execute the prompt with real-time streaming # Always skip git repo check when running from APM + codex_binary = find_runtime_binary("codex") or "codex" process = subprocess.Popen( - ["codex", "exec", "--skip-git-repo-check", prompt_content], + [codex_binary, "exec", "--skip-git-repo-check", prompt_content], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, # Merge stderr into stdout for streaming text=True, @@ -136,7 +137,7 @@ def is_available() -> bool: Returns: bool: True if runtime is available, False otherwise """ - return shutil.which("codex") is not None + return find_runtime_binary("codex") is not None @staticmethod def get_runtime_name() -> str: diff --git a/src/apm_cli/runtime/copilot_runtime.py b/src/apm_cli/runtime/copilot_runtime.py index afdd6635c..1bef7ae8b 100644 --- a/src/apm_cli/runtime/copilot_runtime.py +++ b/src/apm_cli/runtime/copilot_runtime.py @@ -2,12 +2,12 @@ import json import os # noqa: F401 -import shutil import subprocess from pathlib import Path from typing import Any, Dict, Optional # noqa: F401, UP035 from .base import RuntimeAdapter, _stream_subprocess_output +from .utils import find_runtime_binary class CopilotRuntime(RuntimeAdapter): @@ -149,7 +149,7 @@ def is_available() -> bool: Returns: bool: True if runtime is available, False otherwise """ - return shutil.which("copilot") is not None + return find_runtime_binary("copilot") is not None @staticmethod def get_runtime_name() -> str: diff --git a/src/apm_cli/runtime/utils.py b/src/apm_cli/runtime/utils.py new file mode 100644 index 000000000..c624949ee --- /dev/null +++ b/src/apm_cli/runtime/utils.py @@ -0,0 +1,66 @@ +"""Runtime binary resolution utilities.""" + +from __future__ import annotations + +import os +import shutil +import sys +from pathlib import Path + +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + validate_path_segments, +) + + +def find_runtime_binary(name: str) -> str | None: + """Return the resolved path to a runtime binary. + + Priority: + 1. ~/.apm/runtimes/ (APM-managed, executable) + 2. shutil.which(name) (system PATH fallback) + + On Windows the APM-managed binary may carry a ``.exe`` suffix, so both + ``name`` and ``name.exe`` are checked under ``~/.apm/runtimes/``. + + Raises + ------ + PathTraversalError + If *name* contains path-traversal sequences (e.g. ``..``, ``/``, + ``\\``) or is an absolute path. This is a security guard against + user-supplied input that could escape the ``~/.apm/runtimes/`` + directory. + """ + # Security: reject names containing path-traversal or separator + # characters before any filesystem path is constructed. + # Runtime names must be simple identifiers (e.g. "codex", "python"). + if "/" in name or "\\" in name: + raise PathTraversalError( + f"Invalid runtime name '{name}': must be a plain binary name " + "without path separators ('/' or '\\\\')" + ) + validate_path_segments(name, context="runtime name", reject_empty=True) + + apm_runtimes = Path.home() / ".apm" / "runtimes" + + def _safe_executable(candidate: Path) -> bool: + """Return True iff *candidate* is an executable file within *apm_runtimes*.""" + if not (candidate.is_file() and os.access(candidate, os.X_OK)): + return False + try: + ensure_path_within(candidate, apm_runtimes) + except PathTraversalError: + return False + return True + + if sys.platform == "win32": + apm_path_exe = apm_runtimes / f"{name}.exe" + if _safe_executable(apm_path_exe): + return str(apm_path_exe) + + apm_path = apm_runtimes / name + if _safe_executable(apm_path): + return str(apm_path) + + return shutil.which(name) diff --git a/tests/unit/runtime/__init__.py b/tests/unit/runtime/__init__.py new file mode 100644 index 000000000..46eb925ce --- /dev/null +++ b/tests/unit/runtime/__init__.py @@ -0,0 +1 @@ +"""Unit tests for runtime modules.""" diff --git a/tests/unit/runtime/test_runtime_utils.py b/tests/unit/runtime/test_runtime_utils.py new file mode 100644 index 000000000..3c637425f --- /dev/null +++ b/tests/unit/runtime/test_runtime_utils.py @@ -0,0 +1,285 @@ +"""Unit tests for runtime binary resolution utilities.""" + +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +from apm_cli.runtime.utils import find_runtime_binary + + +@pytest.fixture +def fake_home(tmp_path, monkeypatch): + """Override Path.home() to return a temporary directory.""" + monkeypatch.setattr(Path, "home", staticmethod(lambda: tmp_path)) + return tmp_path + + +class TestFindRuntimeBinary: + """Tests for find_runtime_binary utility function.""" + + def test_apm_managed_binary_takes_priority(self, fake_home): + """APM-managed binary should be returned when available and executable. + + When both ~/.apm/runtimes/ and a system PATH binary exist, + the APM-managed one should take priority. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary = runtime_dir / "codex" + apm_binary.touch() + apm_binary.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value="/usr/bin/codex"): + result = find_runtime_binary("codex") + + assert result == str(apm_binary) + + def test_falls_back_to_path_when_no_apm_binary(self, fake_home): + """System PATH should be consulted when no APM-managed binary exists.""" + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + # No binary created in ~/.apm/runtimes/ + + path_binary = "/usr/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + assert result == path_binary + + def test_returns_none_when_neither_exists(self, fake_home): + """None should be returned when binary doesn't exist anywhere. + + When neither ~/.apm/runtimes/ nor system PATH contains the binary, + the function should return None. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + # No binary in APM directory + + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + result = find_runtime_binary("codex") + + assert result is None + + def test_skips_non_executable_apm_binary(self, fake_home): + """Non-executable APM binary should be skipped in favor of PATH. + + When ~/.apm/runtimes/ exists but is not executable, the function + should fall back to checking system PATH. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary = runtime_dir / "codex" + apm_binary.touch() + # Not executable (mode 0o644) + apm_binary.chmod(0o644) + + path_binary = "/usr/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + assert result == path_binary + + @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test") + def test_windows_exe_suffix_priority(self, fake_home): + """On Windows, .exe suffix should be checked first. + + The function should check ~/.apm/runtimes/.exe before + ~/.apm/runtimes/ on Windows systems. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary_exe = runtime_dir / "codex.exe" + apm_binary_exe.touch() + apm_binary_exe.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value="/usr/bin/codex"): + result = find_runtime_binary("codex") + + assert result == str(apm_binary_exe) + + def test_windows_exe_suffix_falls_back_to_non_exe(self, fake_home): + """On Windows, if .exe doesn't exist, non-.exe version should be checked. + + When ~/.apm/runtimes/.exe doesn't exist but + ~/.apm/runtimes/ does, the non-.exe version should be used. + """ + # Use monkeypatch to override sys.platform + with patch("sys.platform", "win32"): + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary = runtime_dir / "codex" + apm_binary.touch() + apm_binary.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + result = find_runtime_binary("codex") + + assert result == str(apm_binary) + + def test_non_windows_ignores_exe_suffix(self, fake_home): + """On non-Windows systems, .exe suffix should be ignored. + + The function should only check ~/.apm/runtimes/ (without suffix) + on non-Windows systems. + """ + with patch("sys.platform", "linux"): + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + # Only .exe version exists + apm_binary_exe = runtime_dir / "codex.exe" + apm_binary_exe.touch() + apm_binary_exe.chmod(0o755) + + path_binary = "/usr/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + # Should fall back to PATH since .exe suffix is ignored on non-Windows + assert result == path_binary + + def test_multiple_binary_names(self, fake_home): + """Function should work correctly with different binary names. + + The priority order should work consistently across different binary names. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + + # Test with "python" + python_binary = runtime_dir / "python" + python_binary.touch() + python_binary.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + result = find_runtime_binary("python") + + assert result == str(python_binary) + + # Test with "node" + node_binary = runtime_dir / "node" + node_binary.touch() + node_binary.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + result = find_runtime_binary("node") + + assert result == str(node_binary) + + def test_apm_binary_without_execute_permission_falls_back(self, fake_home): + """An APM binary without execute permission should trigger fallback. + + os.access(path, os.X_OK) should be checked to ensure executability. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary = runtime_dir / "codex" + apm_binary.touch() + apm_binary.chmod(0o600) # Read/write but not execute + + path_binary = "/opt/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + assert result == path_binary + + def test_apm_runtimes_dir_creation(self, fake_home): + """Function should handle missing .apm/runtimes directory gracefully. + + If ~/.apm/runtimes doesn't exist, the function should not crash + and should return the system PATH result. + """ + # Don't create the directory - leave it as non-existent + path_binary = "/usr/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + assert result == path_binary + + def test_returns_string_path_not_path_object(self, fake_home): + """Function should return a string path, not a Path object. + + The return value should be a string for compatibility with subprocess calls. + """ + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + apm_binary = runtime_dir / "codex" + apm_binary.touch() + apm_binary.chmod(0o755) + + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + result = find_runtime_binary("codex") + + assert isinstance(result, str) + assert result == str(apm_binary) + + +class TestFindRuntimeBinaryPathSecurity: + """Tests for path-traversal security in find_runtime_binary.""" + + def test_rejects_dotdot_traversal(self, fake_home): + """Names with ``..`` segments must raise PathTraversalError.""" + from apm_cli.utils.path_security import PathTraversalError + + with pytest.raises(PathTraversalError): + find_runtime_binary("../../etc/passwd") + + def test_rejects_name_with_forward_slash(self, fake_home): + """Names containing '/' must raise PathTraversalError.""" + from apm_cli.utils.path_security import PathTraversalError + + with pytest.raises(PathTraversalError): + find_runtime_binary("some/path") + + def test_rejects_name_with_backslash(self, fake_home): + """Names containing '\\' must raise PathTraversalError.""" + from apm_cli.utils.path_security import PathTraversalError + + with pytest.raises(PathTraversalError): + find_runtime_binary("some\\path") + + def test_rejects_absolute_path_as_name(self, fake_home): + """Absolute paths passed as a name must raise PathTraversalError.""" + from apm_cli.utils.path_security import PathTraversalError + + with pytest.raises(PathTraversalError): + find_runtime_binary("/usr/bin/codex") + + def test_rejects_dotdot_url_encoded(self, fake_home): + """URL-encoded traversal sequences must also be rejected.""" + from apm_cli.utils.path_security import PathTraversalError + + with pytest.raises(PathTraversalError): + find_runtime_binary("%2e%2e/etc/passwd") + + def test_valid_simple_name_does_not_raise(self, fake_home): + """A plain binary name (no separators) must not raise any exception.""" + with patch("apm_cli.runtime.utils.shutil.which", return_value=None): + # Should not raise; result is None because binary is absent + result = find_runtime_binary("codex") + assert result is None + + def test_symlink_outside_runtimes_dir_is_rejected(self, fake_home): + """An APM binary that is a symlink pointing outside runtimes must be skipped.""" + import os + + runtime_dir = fake_home / ".apm" / "runtimes" + runtime_dir.mkdir(parents=True) + + # Create a real executable outside the runtimes dir + outside_binary = fake_home / "evil_codex" + outside_binary.write_text("#!/bin/sh\n") + outside_binary.chmod(0o755) + + # Symlink inside runtimes pointing outside + apm_link = runtime_dir / "codex" + os.symlink(outside_binary, apm_link) + + path_binary = "/usr/bin/codex" + with patch("apm_cli.runtime.utils.shutil.which", return_value=path_binary): + result = find_runtime_binary("codex") + + # The symlink escapes runtimes dir, so must fall back to PATH + assert result == path_binary diff --git a/tests/unit/test_codex_runtime.py b/tests/unit/test_codex_runtime.py index bf4697398..57ffb9c26 100644 --- a/tests/unit/test_codex_runtime.py +++ b/tests/unit/test_codex_runtime.py @@ -10,7 +10,7 @@ class TestCodexRuntime: """Test Codex runtime adapter.""" - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_init_success(self, mock_which): """Test successful initialization.""" mock_which.return_value = "/usr/local/bin/codex" @@ -20,7 +20,7 @@ def test_init_success(self, mock_which): assert runtime.model_name == "test-model" mock_which.assert_called_once_with("codex") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_init_not_available(self, mock_which): """Test initialization when Codex not available.""" mock_which.return_value = None @@ -29,7 +29,7 @@ def test_init_not_available(self, mock_which): CodexRuntime() @patch("apm_cli.runtime.codex_runtime.subprocess.Popen") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_execute_prompt_success(self, mock_which, mock_popen): """Test successful prompt execution.""" mock_which.return_value = "/usr/local/bin/codex" @@ -47,7 +47,7 @@ def test_execute_prompt_success(self, mock_which, mock_popen): mock_popen.assert_called_once() @patch("apm_cli.runtime.codex_runtime.subprocess.Popen") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_execute_prompt_failure(self, mock_which, mock_popen): """Test prompt execution failure.""" mock_which.return_value = "/usr/local/bin/codex" @@ -63,7 +63,7 @@ def test_execute_prompt_failure(self, mock_which, mock_popen): with pytest.raises(RuntimeError, match="Codex execution failed"): runtime.execute_prompt("Test prompt") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_list_available_models(self, mock_which): """Test listing available models.""" mock_which.return_value = "/usr/local/bin/codex" @@ -75,7 +75,7 @@ def test_list_available_models(self, mock_which): assert models["codex-default"]["provider"] == "codex" @patch("apm_cli.runtime.codex_runtime.subprocess.run") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_get_runtime_info(self, mock_which, mock_run): """Test getting runtime info.""" mock_which.return_value = "/usr/local/bin/codex" @@ -92,7 +92,7 @@ def test_get_runtime_info(self, mock_which, mock_run): assert info["version"] == "1.0.0" assert info["capabilities"]["mcp_servers"] == "native_support" - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_is_available_true(self, mock_which): """Test runtime availability check - available.""" mock_which.return_value = "/usr/local/bin/codex" @@ -100,7 +100,7 @@ def test_is_available_true(self, mock_which): assert CodexRuntime.is_available() is True mock_which.assert_called_once_with("codex") - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_is_available_false(self, mock_which): """Test runtime availability check - not available.""" mock_which.return_value = None @@ -112,7 +112,7 @@ def test_get_runtime_name(self): """Test runtime name getter.""" assert CodexRuntime.get_runtime_name() == "codex" - @patch("apm_cli.runtime.codex_runtime.shutil.which") + @patch("apm_cli.runtime.codex_runtime.find_runtime_binary") def test_str_representation(self, mock_which): """Test string representation.""" mock_which.return_value = "/usr/local/bin/codex" diff --git a/tests/unit/test_runtime_windows.py b/tests/unit/test_runtime_windows.py index 280ec43ad..bfcab7e1d 100644 --- a/tests/unit/test_runtime_windows.py +++ b/tests/unit/test_runtime_windows.py @@ -230,7 +230,7 @@ def test_execute_runtime_command_uses_shlex_on_windows(self): with ( patch("sys.platform", "win32"), - patch("apm_cli.core.script_runner.shutil.which", return_value=None), + patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None), patch("subprocess.run", return_value=MagicMock(returncode=0)) as mock_run, ): runner._execute_runtime_command("codex --quiet", "prompt content", env) @@ -245,7 +245,7 @@ def test_execute_runtime_command_preserves_quotes_on_windows(self): with ( patch("sys.platform", "win32"), - patch("apm_cli.core.script_runner.shutil.which", return_value=None), + patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None), patch("subprocess.run", return_value=MagicMock(returncode=0)) as mock_run, ): runner._execute_runtime_command('codex --model "gpt-4o mini"', "prompt content", env) diff --git a/tests/unit/test_script_runner.py b/tests/unit/test_script_runner.py index 405e07d39..f488fe775 100644 --- a/tests/unit/test_script_runner.py +++ b/tests/unit/test_script_runner.py @@ -161,10 +161,10 @@ def test_transform_runtime_command_copilot_with_codex_model_name(self): assert "codex exec" not in result @patch("subprocess.run") - @patch("apm_cli.core.script_runner.shutil.which", return_value=None) + @patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None) @patch("apm_cli.core.script_runner.setup_runtime_environment") def test_execute_runtime_command_with_env_vars( - self, mock_setup_env, mock_which, mock_subprocess + self, mock_setup_env, mock_find_runtime, mock_subprocess ): """Test runtime command execution with environment variables.""" mock_setup_env.return_value = {"EXISTING_VAR": "value"} @@ -191,10 +191,10 @@ def test_execute_runtime_command_with_env_vars( assert called_env["EXISTING_VAR"] == "value" # Existing env should be preserved @patch("subprocess.run") - @patch("apm_cli.core.script_runner.shutil.which", return_value=None) + @patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None) @patch("apm_cli.core.script_runner.setup_runtime_environment") def test_execute_runtime_command_multiple_env_vars( - self, mock_setup_env, mock_which, mock_subprocess + self, mock_setup_env, mock_find_runtime, mock_subprocess ): """Test runtime command execution with multiple environment variables.""" mock_setup_env.return_value = {} @@ -835,34 +835,30 @@ def test_discover_simple_name_finds_skill_md(self): os.chdir(original_dir) -class TestExecuteRuntimeCommandWindowsResolution: - """Test that _execute_runtime_command resolves executables on Windows.""" +class TestExecuteRuntimeCommandResolution: + """Test that _execute_runtime_command resolves executables on all platforms.""" def setup_method(self): self.runner = ScriptRunner() @patch("apm_cli.core.script_runner.subprocess.run") - @patch("apm_cli.core.script_runner.shutil.which", return_value=r"C:\npm\copilot.cmd") - @patch("apm_cli.core.script_runner.sys") - def test_resolves_executable_on_windows(self, mock_sys, mock_which, mock_run): - """On win32, the executable should be resolved via shutil.which.""" - mock_sys.platform = "win32" + @patch("apm_cli.core.script_runner.find_runtime_binary", return_value=r"C:\npm\copilot.cmd") + def test_resolves_executable(self, mock_find_runtime, mock_run): + """The executable should be resolved via find_runtime_binary on all platforms.""" mock_run.return_value = MagicMock(returncode=0) self.runner._execute_runtime_command( "copilot --log-level all", "prompt content", os.environ.copy() ) - mock_which.assert_called_once_with("copilot") + mock_find_runtime.assert_called_once_with("copilot") call_args = mock_run.call_args[0][0] assert call_args[0] == r"C:\npm\copilot.cmd" @patch("apm_cli.core.script_runner.subprocess.run") - @patch("apm_cli.core.script_runner.shutil.which", return_value=None) - @patch("apm_cli.core.script_runner.sys") - def test_keeps_original_when_which_returns_none(self, mock_sys, mock_which, mock_run): - """If shutil.which can't find it, keep the original name.""" - mock_sys.platform = "win32" + @patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None) + def test_keeps_original_when_find_returns_none(self, mock_find_runtime, mock_run): + """If find_runtime_binary can't find it, keep the original name.""" mock_run.return_value = MagicMock(returncode=0) self.runner._execute_runtime_command("copilot -p", "prompt content", os.environ.copy()) @@ -871,13 +867,11 @@ def test_keeps_original_when_which_returns_none(self, mock_sys, mock_which, mock assert call_args[0] == "copilot" @patch("apm_cli.core.script_runner.subprocess.run") - @patch("apm_cli.core.script_runner.shutil.which") - @patch("apm_cli.core.script_runner.sys") - def test_skips_resolution_on_non_windows(self, mock_sys, mock_which, mock_run): - """On non-Windows, shutil.which should not be called.""" - mock_sys.platform = "linux" + @patch("apm_cli.core.script_runner.find_runtime_binary", return_value=None) + def test_resolves_on_non_windows(self, mock_find_runtime, mock_run): + """find_runtime_binary should be called on all platforms, not just Windows.""" mock_run.return_value = MagicMock(returncode=0) self.runner._execute_runtime_command("copilot -p", "prompt content", os.environ.copy()) - mock_which.assert_not_called() + mock_find_runtime.assert_called_once_with("copilot")