From 88eaf0da67aacf1c0ff3cd636925769f653194ba Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 31 Jul 2026 01:54:19 -0700 Subject: [PATCH 1/2] fix(exec): route git/ffmpeg/ffprobe spawns through _safe_exec (BE-5358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every remaining bare-name subprocess spawn in the repo passed `["git", ...]` or `["ffmpeg", ...]`, so Windows' `CreateProcess` searched the current working directory before `$PATH` and would execute a planted `git.exe`/`ffmpeg.exe`. Several of these spawn *after* an `os.chdir()` into a user-supplied repo or custom-node directory, so the attacker-controlled directory is the CWD by construction. Adds `_safe_exec.resolve_required_binary` — the required-binary companion to `resolve_binary`, a thin wrapper over it so the bare-name / fully-qualified / CWD-plant gates cannot drift — and converts every git, ffmpeg and ffprobe spawn to a trusted absolute path. `BinaryNotFoundError` subclasses both `RuntimeError` and `FileNotFoundError` so the call sites that already degraded on a missing binary keep degrading identically instead of starting to crash. `comfy preview`'s duplicate `shutil.which` presence check is folded into the resolution, so both binaries are looked up once and the paths reused. --- comfy_cli/_safe_exec.py | 60 +++- comfy_cli/cmdline.py | 6 +- comfy_cli/command/install.py | 26 +- comfy_cli/command/outdated.py | 6 +- comfy_cli/command/preview.py | 47 ++- comfy_cli/file_utils.py | 6 +- comfy_cli/git_utils.py | 30 +- comfy_cli/output/preview.py | 18 +- comfy_cli/registry/config_parser.py | 4 +- tests/comfy_cli/command/github/test_pr.py | 8 +- tests/comfy_cli/command/test_preview.py | 106 +++++- .../comfy_cli/command/test_update_version.py | 25 +- tests/comfy_cli/output/test_inline_preview.py | 107 ++++++ tests/comfy_cli/test_safe_exec.py | 51 +++ tests/comfy_cli/test_safe_exec_call_sites.py | 310 ++++++++++++++++++ 15 files changed, 765 insertions(+), 45 deletions(-) create mode 100644 tests/comfy_cli/output/test_inline_preview.py create mode 100644 tests/comfy_cli/test_safe_exec_call_sites.py diff --git a/comfy_cli/_safe_exec.py b/comfy_cli/_safe_exec.py index 5826a918..9a04214b 100644 --- a/comfy_cli/_safe_exec.py +++ b/comfy_cli/_safe_exec.py @@ -12,9 +12,15 @@ the import cycle that would come from ``cuda_detect`` importing ``hardware`` (``hardware`` already imports ``cuda_detect``). -Contract: **never raises.** Every failure — a missing binary, a broken ``$PATH`` -lookup, an unresolvable path — degrades to ``None`` so callers can keep their -existing degrade-to-``None`` behaviour without new error handling. +Two entry points share one set of gates: + +* :func:`resolve_binary` — for *optional* probes. **Never raises**; every failure + (a missing binary, a broken ``$PATH`` lookup, an unresolvable path) degrades to + ``None`` so callers keep their existing degrade-to-``None`` behaviour. +* :func:`resolve_required_binary` — for binaries a command cannot run without + (``git``, ``ffmpeg``). Raises :class:`BinaryNotFoundError` with an actionable + message instead of returning ``None``. It is a thin wrapper over + :func:`resolve_binary`, so the CWD/qualification gates cannot drift apart. """ from __future__ import annotations @@ -29,6 +35,24 @@ _PATH_SEPARATORS = ("/", "\\") +class BinaryNotFoundError(RuntimeError, FileNotFoundError): + """A binary a command cannot run without could not be trusted-resolved. + + Raised by :func:`resolve_required_binary` — either the binary is absent from + ``$PATH``, or the only match was CWD-anchored and therefore refused. + + It deliberately subclasses **both** ``RuntimeError`` and + ``FileNotFoundError``. ``FileNotFoundError`` is the exception ``subprocess`` + already raises today when a bare-name spawn finds no such binary, so every + call site that already degrades on a missing binary (``except + (subprocess.SubprocessError, FileNotFoundError)`` in + :mod:`comfy_cli.file_utils`, ``except OSError`` in + :mod:`comfy_cli.command.outdated`, …) keeps degrading *identically* rather + than starting to crash. ``RuntimeError`` is carried for callers that want to + name the failure explicitly without reaching for an OS-error class. + """ + + def _is_bare_name(name: str) -> bool: """Return ``True`` if ``name`` is a plain binary name with no path part. @@ -147,3 +171,33 @@ def resolve_binary(name: str) -> str | None: except Exception: logger.debug("resolving binary %r failed", name, exc_info=True) return None + + +def resolve_required_binary(name: str) -> str: + """Resolve a *required* system binary to a trusted absolute path. + + The required-binary companion to :func:`resolve_binary`: identical gates + (bare-name check, fully-qualified check, CWD-plant check), but a failure + raises :class:`BinaryNotFoundError` instead of returning ``None``. Use it for + binaries whose absence is fatal to the command — ``git``, ``ffmpeg``, + ``ffprobe`` — where "skip this probe" is not a meaningful degradation. + + Resolving deliberately happens at *call* time, not import time, so the answer + reflects the ``$PATH`` and CWD in force when the process is actually spawned. + Callers that ``os.chdir`` into a user-supplied directory should resolve + **before** the ``chdir``: the absolute path returned here already defeats + ``CreateProcess``'s current-directory search at spawn time, and resolving + first means a plant in the target directory cannot even shadow a legitimate + binary into a hard failure. + + :raises BinaryNotFoundError: the binary is not on ``$PATH``, or the only match + resolved into the current working directory and was refused. + """ + path = resolve_binary(name) + if path is None: + raise BinaryNotFoundError( + f"{name!r} was not found on PATH, or the only match resolved into the current directory " + f"(which is not trusted). Install {name} and make sure it is on PATH outside the directory " + f"you are running from." + ) + return path diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index 9a3594f9..4a428348 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -11,6 +11,7 @@ from comfy_cli import cancellation, constants, env_checker, logging, tracking, ui, utils from comfy_cli import where as where_module +from comfy_cli._safe_exec import resolve_required_binary from comfy_cli.auth import command as auth_command from comfy_cli.cloud import command as cloud_command from comfy_cli.command import ( @@ -736,8 +737,11 @@ def update( if version is not None: _switch_comfy_version(comfy_path, version, stash=not no_stash) else: + # Resolved before the chdir so a ``git`` planted in ``comfy_path`` + # can neither be picked up nor shadow the real one. + git_bin = resolve_required_binary("git") os.chdir(comfy_path) - subprocess.run(["git", "pull"], check=True) + subprocess.run([git_bin, "pull"], check=True) python = resolve_workspace_python(comfy_path) # A uv-managed venv may have no pip — bootstrap it first so the install # below doesn't crash with `No module named pip` (no-op if pip exists). diff --git a/comfy_cli/command/install.py b/comfy_cli/command/install.py index 59283b36..577b91d7 100755 --- a/comfy_cli/command/install.py +++ b/comfy_cli/command/install.py @@ -16,6 +16,7 @@ from rich.prompt import Confirm from comfy_cli import constants, ui +from comfy_cli._safe_exec import resolve_required_binary from comfy_cli.command.custom_nodes.command import update_node_id_cache from comfy_cli.command.github.pr_info import PRInfo from comfy_cli.constants import GPU_OPTION @@ -234,8 +235,11 @@ def execute( # checkout specified commit if commit is not None: + # Resolved before the chdir: ``repo_dir`` is user-supplied, so a ``git`` + # planted there must not be found — nor shadow the real one. + git_bin = resolve_required_binary("git") os.chdir(repo_dir) - subprocess.run(["git", "checkout", commit], check=True) + subprocess.run([git_bin, "checkout", commit], check=True) python = ensure_workspace_python(repo_dir) rprint(f"Using Python: [bold]{python}[/bold]") @@ -483,12 +487,13 @@ def clone_comfyui(url: str, repo_dir: str): """ Clone the ComfyUI repository from the specified URL. """ + git_bin = resolve_required_binary("git") if "@" in url: # clone specific branch url, branch = url.rsplit("@", 1) - subprocess.run(["git", "clone", "-b", branch, url, repo_dir], check=True) + subprocess.run([git_bin, "clone", "-b", branch, url, repo_dir], check=True) else: - subprocess.run(["git", "clone", url, repo_dir], check=True) + subprocess.run([git_bin, "clone", url, repo_dir], check=True) def _resolve_latest_tag_from_local(repo_dir: str) -> tuple[str | None, bool]: @@ -516,8 +521,11 @@ def _resolve_latest_tag_from_local(repo_dir: str) -> tuple[str | None, bool]: """ fetch_ok = False try: + # ``BinaryNotFoundError`` subclasses ``FileNotFoundError``, so an absent + # (or CWD-planted, hence refused) ``git`` lands in the same tolerant + # handlers that already cover a missing binary here. completed = subprocess.run( - ["git", "-C", repo_dir, "fetch", "--tags", "--quiet"], + [resolve_required_binary("git"), "-C", repo_dir, "fetch", "--tags", "--quiet"], capture_output=True, text=True, timeout=30, @@ -529,7 +537,7 @@ def _resolve_latest_tag_from_local(repo_dir: str) -> tuple[str | None, bool]: try: result = subprocess.run( - ["git", "-C", repo_dir, "tag", "--list"], + [resolve_required_binary("git"), "-C", repo_dir, "tag", "--list"], capture_output=True, text=True, check=True, @@ -674,8 +682,14 @@ def _git_capture(repo_dir: str, *args: str, timeout: int = 30) -> subprocess.Com Uses ``git -C`` rather than ``os.chdir`` so a failure part-way through a switch can't leave the process in someone else's directory. """ - argv = ["git", "-C", repo_dir, *args] + # Only ever used to label a failure result; the spawned argv is built below + # from the trusted-resolved path. + argv: list[str] = ["git", "-C", repo_dir, *args] try: + # Resolving inside the ``try`` keeps the never-raises contract: an absent + # (or CWD-planted, hence refused) ``git`` raises ``BinaryNotFoundError``, + # a ``FileNotFoundError``, and comes back as the usual rc=1 result. + argv = [resolve_required_binary("git"), "-C", repo_dir, *args] return subprocess.run(argv, capture_output=True, text=True, check=False, timeout=timeout) except (subprocess.SubprocessError, FileNotFoundError, OSError) as exc: return subprocess.CompletedProcess(args=argv, returncode=1, stdout="", stderr=str(exc)) diff --git a/comfy_cli/command/outdated.py b/comfy_cli/command/outdated.py index 2dcadd04..b19742f7 100644 --- a/comfy_cli/command/outdated.py +++ b/comfy_cli/command/outdated.py @@ -34,6 +34,7 @@ from pathlib import Path from typing import Any +from comfy_cli._safe_exec import resolve_required_binary from comfy_cli.command.pack_scan import iter_pack_dirs as _iter_pack_dirs from comfy_cli.command.pack_scan import read_pyproject as _read_pyproject from comfy_cli.registry import RegistryAPI @@ -191,8 +192,11 @@ def _git_output(args: list[str], cwd: str) -> str | None: ``_GIT_HARDENING`` / ``_GIT_SAFE_ENV``. """ try: + # ``resolve_required_binary`` raises ``BinaryNotFoundError`` (a + # ``FileNotFoundError``), which the ``OSError`` arm below already turns + # into the same ``None`` a missing ``git`` produced before. out = subprocess.run( - ["git", *_GIT_HARDENING, *args], + [resolve_required_binary("git"), *_GIT_HARDENING, *args], cwd=cwd, capture_output=True, text=True, diff --git a/comfy_cli/command/preview.py b/comfy_cli/command/preview.py index 90680ac5..6f534297 100644 --- a/comfy_cli/command/preview.py +++ b/comfy_cli/command/preview.py @@ -16,7 +16,6 @@ from __future__ import annotations import json -import shutil import subprocess from pathlib import Path from typing import Annotated, Any @@ -24,6 +23,7 @@ import typer from comfy_cli import tracking +from comfy_cli._safe_exec import BinaryNotFoundError, resolve_required_binary from comfy_cli.output import get_renderer, rprint _IMAGE_FORMAT_HINTS = ("_pipe", "image2", "png", "jpeg", "mjpeg", "gif", "webp", "bmp", "apng") @@ -93,10 +93,23 @@ def classify_streams(probe: dict) -> dict: def build_preview_cmd( - kind: str, input_path: str, out_path: str, *, grid: tuple[int, int], width: int, duration: float | None + kind: str, + input_path: str, + out_path: str, + *, + grid: tuple[int, int], + width: int, + duration: float | None, + ffmpeg_bin: str, ) -> list[str]: - """Build the ffmpeg argv for a preview of ``kind``. I/O-free.""" - base = ["ffmpeg", "-v", "error", "-y", "-i", input_path] + """Build the ffmpeg argv for a preview of ``kind``. I/O-free. + + ``ffmpeg_bin`` is the trusted absolute path from + :func:`comfy_cli._safe_exec.resolve_required_binary`; it is a *required* + keyword rather than one defaulting to ``"ffmpeg"`` so no caller can + reintroduce the bare-name spawn this argument exists to prevent. + """ + base = [ffmpeg_bin, "-v", "error", "-y", "-i", input_path] if kind == "video": cols, rows = grid n = max(1, cols * rows) @@ -110,9 +123,9 @@ def build_preview_cmd( return base + ["-frames:v", "1", "-vf", f"scale='min({width},iw)':-1", out_path] -def _ffprobe(path: Path) -> dict: +def _ffprobe(path: Path, ffprobe_bin: str) -> dict: proc = subprocess.run( - ["ffprobe", "-v", "error", "-print_format", "json", "-show_streams", "-show_format", str(path)], + [ffprobe_bin, "-v", "error", "-print_format", "json", "-show_streams", "-show_format", str(path)], capture_output=True, text=True, ) @@ -137,16 +150,24 @@ def preview_cmd( if not file.is_file(): renderer.error(code="preview_input_not_found", message=f"File not found: {file}", hint="check the path") raise typer.Exit(code=1) - if not (shutil.which("ffmpeg") and shutil.which("ffprobe")): + # One resolution pass doubles as the presence check: both binaries are looked + # up once, to trusted absolute paths that are reused for every spawn below. + # A CWD-anchored match is refused, so running ``comfy preview`` from a + # directory holding a planted ``ffmpeg.exe`` reports it as unavailable rather + # than executing it. + try: + ffmpeg_bin = resolve_required_binary("ffmpeg") + ffprobe_bin = resolve_required_binary("ffprobe") + except BinaryNotFoundError: renderer.error( code="ffmpeg_unavailable", message="ffmpeg/ffprobe not found on PATH — `comfy preview` needs them.", hint="install ffmpeg (e.g. `brew install ffmpeg` / `apt install ffmpeg`)", ) - raise typer.Exit(code=1) + raise typer.Exit(code=1) from None try: - info = classify_streams(_ffprobe(file)) + info = classify_streams(_ffprobe(file, ffprobe_bin)) except (RuntimeError, json.JSONDecodeError) as e: renderer.error(code="preview_failed", message=f"Could not probe {file}: {e}") raise typer.Exit(code=1) from e @@ -166,7 +187,13 @@ def preview_cmd( out_path = out or (file.parent / f"{file.stem}.preview.png") cmd = build_preview_cmd( - info["kind"], str(file), str(out_path), grid=(cols, rows), width=width, duration=info["duration"] + info["kind"], + str(file), + str(out_path), + grid=(cols, rows), + width=width, + duration=info["duration"], + ffmpeg_bin=ffmpeg_bin, ) proc = subprocess.run(cmd, capture_output=True, text=True) if proc.returncode != 0 or not out_path.is_file(): diff --git a/comfy_cli/file_utils.py b/comfy_cli/file_utils.py index d3f087c2..f01e2e50 100644 --- a/comfy_cli/file_utils.py +++ b/comfy_cli/file_utils.py @@ -12,6 +12,7 @@ from pathspec import PathSpec from comfy_cli import constants, ui +from comfy_cli._safe_exec import resolve_required_binary class DownloadException(Exception): @@ -445,8 +446,11 @@ def _load_comfyignore_spec(ignore_filename: str = ".comfyignore") -> PathSpec | def list_git_tracked_files(base_path: str | os.PathLike = ".") -> list[str]: try: + # ``BinaryNotFoundError`` subclasses ``FileNotFoundError``, so an absent — + # or CWD-planted, hence refused — ``git`` degrades to ``[]`` here exactly + # as a missing ``git`` already did. result = subprocess.check_output( - ["git", "-C", os.fspath(base_path), "ls-files"], + [resolve_required_binary("git"), "-C", os.fspath(base_path), "ls-files"], text=True, ) except (subprocess.SubprocessError, FileNotFoundError): diff --git a/comfy_cli/git_utils.py b/comfy_cli/git_utils.py index d186c172..a85ca83e 100644 --- a/comfy_cli/git_utils.py +++ b/comfy_cli/git_utils.py @@ -5,6 +5,7 @@ from rich.panel import Panel from rich.text import Text +from comfy_cli._safe_exec import resolve_required_binary from comfy_cli.command.github.pr_info import PRInfo console = Console() @@ -38,7 +39,14 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: :param repo_path: Path to the Git repository :param tag: The tag to checkout :return: True if the checkout succeeds, False if any git command failed. + :raises BinaryNotFoundError: ``git`` is absent from ``$PATH`` (previously a + bare ``FileNotFoundError`` from ``subprocess``, which was equally + uncaught here — the message is the only thing that changed). """ + # Resolved BEFORE the ``os.chdir`` below so the lookup can't see a ``git`` + # planted in the caller-supplied ``repo_path``, and so the absolute path we + # spawn defeats Windows' current-directory search once we are inside it. + git_bin = resolve_required_binary("git") original_dir = os.getcwd() try: # Change to the repository directory @@ -48,7 +56,7 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: # Skip the network fetch when the tag is already present locally. tag_present_locally = ( subprocess.run( - ["git", "rev-parse", "--verify", f"refs/tags/{tag}"], + [git_bin, "rev-parse", "--verify", f"refs/tags/{tag}"], capture_output=True, text=True, check=False, @@ -56,10 +64,10 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: == 0 ) if not tag_present_locally: - subprocess.run(["git", "fetch", "--tags"], check=True, capture_output=True, text=True) + subprocess.run([git_bin, "fetch", "--tags"], check=True, capture_output=True, text=True) # Checkout the specified tag - subprocess.run(["git", "checkout", tag], check=True, capture_output=True, text=True) + subprocess.run([git_bin, "checkout", tag], check=True, capture_output=True, text=True) console.print(f"[bold green]Successfully checked out tag: [cyan]{tag}[/cyan][/bold green]") @@ -92,6 +100,8 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: def checkout_pr(repo_path: str, pr_info: PRInfo) -> bool: + # See ``git_checkout_tag``: resolve before the ``os.chdir`` into ``repo_path``. + git_bin = resolve_required_binary("git") original_dir = os.getcwd() try: @@ -100,18 +110,18 @@ def checkout_pr(repo_path: str, pr_info: PRInfo) -> bool: if pr_info.is_fork: remote_name = f"pr-{pr_info.number}-{pr_info.user}" - result = subprocess.run(["git", "remote", "get-url", remote_name], capture_output=True, text=True) + result = subprocess.run([git_bin, "remote", "get-url", remote_name], capture_output=True, text=True) if result.returncode != 0: subprocess.run( - ["git", "remote", "add", remote_name, pr_info.head_repo_url], + [git_bin, "remote", "add", remote_name, pr_info.head_repo_url], check=True, capture_output=True, text=True, ) subprocess.run( - ["git", "fetch", remote_name, pr_info.head_branch], check=True, capture_output=True, text=True + [git_bin, "fetch", remote_name, pr_info.head_branch], check=True, capture_output=True, text=True ) # fix: "feature/add-support" -> "pr-123-feature-add-support" @@ -119,20 +129,22 @@ def checkout_pr(repo_path: str, pr_info: PRInfo) -> bool: local_branch = f"pr-{pr_info.number}-{sanitized_branch}" subprocess.run( - ["git", "checkout", "-B", local_branch, f"{remote_name}/{pr_info.head_branch}"], + [git_bin, "checkout", "-B", local_branch, f"{remote_name}/{pr_info.head_branch}"], check=True, capture_output=True, text=True, ) else: - subprocess.run(["git", "fetch", "origin", pr_info.head_branch], check=True, capture_output=True, text=True) + subprocess.run( + [git_bin, "fetch", "origin", pr_info.head_branch], check=True, capture_output=True, text=True + ) sanitized_branch = sanitize_for_local_branch(pr_info.head_branch) local_branch = f"pr-{pr_info.number}-{sanitized_branch}" subprocess.run( - ["git", "checkout", "-B", local_branch, f"origin/{pr_info.head_branch}"], + [git_bin, "checkout", "-B", local_branch, f"origin/{pr_info.head_branch}"], check=True, capture_output=True, text=True, diff --git a/comfy_cli/output/preview.py b/comfy_cli/output/preview.py index 6f6a5e72..d231e909 100644 --- a/comfy_cli/output/preview.py +++ b/comfy_cli/output/preview.py @@ -21,6 +21,8 @@ import warnings from pathlib import Path +from comfy_cli._safe_exec import resolve_binary + # Suppress term-image's "not running in a terminal" warning — we handle # the TTY check ourselves before calling into term-image. warnings.filterwarnings("ignore", message=".*not running within a terminal.*", category=UserWarning) @@ -64,6 +66,13 @@ def _show_video(path: Path) -> None: def _show_video_thumbnail(path: Path) -> None: """Extract the first frame via ffmpeg and display it.""" + # ``resolve_binary`` — not the required variant — because this previewer's + # documented contract is to skip silently: a missing ffmpeg and one refused + # for resolving into the CWD degrade the same way, to no thumbnail. + ffmpeg_bin = resolve_binary("ffmpeg") + if ffmpeg_bin is None: + return + tmp_path: str | None = None try: with tempfile.NamedTemporaryFile(suffix=".jpg", delete=False) as tmp: @@ -71,7 +80,7 @@ def _show_video_thumbnail(path: Path) -> None: result = subprocess.run( # noqa: S603 [ - "ffmpeg", + ffmpeg_bin, "-i", str(path), "-vf", @@ -101,10 +110,15 @@ def _show_video_thumbnail(path: Path) -> None: def _show_video_info(path: Path) -> None: """Show a Rich panel with video metadata via ffprobe.""" + # See ``_show_video_thumbnail``: skip-silently is the contract here too. + ffprobe_bin = resolve_binary("ffprobe") + if ffprobe_bin is None: + return + try: result = subprocess.run( # noqa: S603 [ - "ffprobe", + ffprobe_bin, "-v", "quiet", "-print_format", diff --git a/comfy_cli/registry/config_parser.py b/comfy_cli/registry/config_parser.py index f67e6789..bacf36cd 100644 --- a/comfy_cli/registry/config_parser.py +++ b/comfy_cli/registry/config_parser.py @@ -10,6 +10,7 @@ from tomlkit.items import Comment, Trivia from comfy_cli import ui +from comfy_cli._safe_exec import resolve_required_binary from comfy_cli.registry.types import ( ComfyConfig, License, @@ -242,7 +243,8 @@ def initialize_project_config(): # Get the current git remote URL try: - git_remote_url = subprocess.check_output(["git", "remote", "get-url", "origin"]).decode().strip() + git_bin = resolve_required_binary("git") + git_remote_url = subprocess.check_output([git_bin, "remote", "get-url", "origin"]).decode().strip() git_remote_url = _strip_url_credentials(git_remote_url) except subprocess.CalledProcessError as e: raise Exception("Could not retrieve Git remote URL. Are you in a Git repository?") from e diff --git a/tests/comfy_cli/command/github/test_pr.py b/tests/comfy_cli/command/github/test_pr.py index 64370be6..b924831f 100644 --- a/tests/comfy_cli/command/github/test_pr.py +++ b/tests/comfy_cli/command/github/test_pr.py @@ -1,3 +1,4 @@ +import os import subprocess import sys from unittest.mock import Mock, patch @@ -258,7 +259,12 @@ def test_checkout_pr_fork_success(self, mock_getcwd, mock_chdir, mock_subprocess assert mock_subprocess.call_count == 4 calls = mock_subprocess.call_args_list - assert "git" in calls[0][0][0] + # git is spawned by its resolved absolute path, never the bare name + # Windows' CreateProcess would look up in the CWD first (BE-5358). + for call in calls: + argv0 = call[0][0][0] + assert os.path.isabs(argv0), argv0 + assert os.path.basename(argv0).removesuffix(".exe") == "git", argv0 assert "remote" in calls[1][0][0] assert "fetch" in calls[2][0][0] assert "checkout" in calls[3][0][0] diff --git a/tests/comfy_cli/command/test_preview.py b/tests/comfy_cli/command/test_preview.py index 303711aa..aec022b3 100644 --- a/tests/comfy_cli/command/test_preview.py +++ b/tests/comfy_cli/command/test_preview.py @@ -2,7 +2,11 @@ from __future__ import annotations +import os import shutil +import subprocess +from pathlib import Path +from unittest.mock import patch import pytest from typer.testing import CliRunner @@ -49,23 +53,33 @@ def test_classify_audio(): # --- pure: build the ffmpeg command ---------------------------------------- +_FFMPEG = os.path.join(os.sep, "usr", "bin", "ffmpeg") + + def test_build_cmd_video_is_contact_sheet(): - cmd = build_preview_cmd("video", "in.mp4", "out.png", grid=(4, 3), width=300, duration=6.0) + cmd = build_preview_cmd("video", "in.mp4", "out.png", grid=(4, 3), width=300, duration=6.0, ffmpeg_bin=_FFMPEG) s = " ".join(cmd) - assert cmd[0] == "ffmpeg" + assert cmd[0] == _FFMPEG assert "tile=4x3" in s and "in.mp4" in s and s.endswith("out.png") def test_build_cmd_audio_is_waveform(): - cmd = build_preview_cmd("audio", "a.flac", "out.png", grid=(4, 3), width=600, duration=30.0) + cmd = build_preview_cmd("audio", "a.flac", "out.png", grid=(4, 3), width=600, duration=30.0, ffmpeg_bin=_FFMPEG) assert "showwavespic" in " ".join(cmd) def test_build_cmd_image_is_scaled(): - cmd = build_preview_cmd("image", "i.png", "out.png", grid=(4, 3), width=512, duration=None) + cmd = build_preview_cmd("image", "i.png", "out.png", grid=(4, 3), width=512, duration=None, ffmpeg_bin=_FFMPEG) assert "scale" in " ".join(cmd) +def test_build_cmd_requires_an_explicit_ffmpeg_path(): + """``ffmpeg_bin`` is keyword-only and has no default, so a caller cannot + silently fall back to the bare name this whole change removes.""" + with pytest.raises(TypeError): + build_preview_cmd("image", "i.png", "out.png", grid=(4, 3), width=512, duration=None) + + # --- integration: actually run it (needs ffmpeg) --------------------------- @@ -74,7 +88,6 @@ def test_preview_image_end_to_end(tmp_path, monkeypatch): """End-to-end: a real image in → a preview PNG written next to it, exit 0.""" monkeypatch.setattr("comfy_cli.tracking.prompt_tracking_consent", lambda *a, **kw: None) monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **kw: None) - import subprocess from comfy_cli.cmdline import app as cli_app @@ -109,3 +122,86 @@ def test_preview_missing_file_errors(tmp_path, monkeypatch): monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **kw: None) with pytest.raises(typer.Exit): preview_cmd(tmp_path / "nope.png") + + +# --- CWD binary-planting guard --------------------------------------------- + + +def _which_cwd_first(system_dir: Path): + """A ``shutil.which`` that searches the CWD before ``$PATH`` — Windows' order.""" + + def which(name, *_args, **_kwargs): + for candidate in (Path(os.getcwd()) / name, system_dir / name): + if candidate.exists(): + return str(candidate) + return None + + return which + + +_PROBE_JSON = ( + '{"streams": [{"codec_type": "video", "width": 8, "height": 8, "nb_frames": "1"}],' + ' "format": {"format_name": "png_pipe"}}' +) + + +def test_preview_spawns_resolved_absolute_paths(tmp_path, monkeypatch): + """Both ffprobe and ffmpeg go out as trusted absolute paths, resolved once.""" + from comfy_cli import _safe_exec + from comfy_cli.command import preview as preview_mod + + monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **kw: None) + system_bin = tmp_path / "usr_bin" + system_bin.mkdir() + for name in ("ffmpeg", "ffprobe"): + (system_bin / name).write_text("") + src = tmp_path / "src.png" + src.write_bytes(b"\x89PNG") + out = tmp_path / "out.png" + monkeypatch.chdir(tmp_path) + + calls: list[list[str]] = [] + + def fake_run(argv, **_kwargs): + calls.append(list(argv)) + out.write_bytes(b"\x89PNG") + return subprocess.CompletedProcess(args=list(argv), returncode=0, stdout=_PROBE_JSON, stderr="") + + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(preview_mod.subprocess, "run", fake_run), + ): + preview_mod.preview_cmd(src, out=out) + + assert [c[0] for c in calls] == [str(system_bin / "ffprobe"), str(system_bin / "ffmpeg")] + + +def test_preview_reports_unavailable_for_a_cwd_planted_ffmpeg(tmp_path, monkeypatch): + """A planted ``ffmpeg`` in the CWD is refused, not executed: the command exits + with the same ``ffmpeg_unavailable`` error it already used when ffmpeg was + simply not installed.""" + import typer + + from comfy_cli import _safe_exec + from comfy_cli.command import preview as preview_mod + + monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **kw: None) + system_bin = tmp_path / "usr_bin" + system_bin.mkdir() + (system_bin / "ffprobe").write_text("") # ffprobe is legitimately installed + attacker_cwd = tmp_path / "attacker" + attacker_cwd.mkdir() + (attacker_cwd / "ffmpeg").write_text("") # ...but ffmpeg is only the plant + monkeypatch.chdir(attacker_cwd) + src = attacker_cwd / "src.png" + src.write_bytes(b"\x89PNG") + + calls: list[list[str]] = [] + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(preview_mod.subprocess, "run", lambda argv, **kw: calls.append(list(argv))), + ): + with pytest.raises(typer.Exit): + preview_mod.preview_cmd(src) + + assert calls == [], "the planted ffmpeg must never be spawned" diff --git a/tests/comfy_cli/command/test_update_version.py b/tests/comfy_cli/command/test_update_version.py index bced554b..981fc00d 100644 --- a/tests/comfy_cli/command/test_update_version.py +++ b/tests/comfy_cli/command/test_update_version.py @@ -5,10 +5,15 @@ ``install.switch_comfyui_version`` directly; the CLI-level cases drive ``cmdline.update`` so the flag wiring, the dependency reinstall, and the exit codes are covered too. + +``git`` is spawned by its trusted absolute path (``_safe_exec``), so ``FakeGit`` +dispatches on the argv's *basename* and the resolver is pinned to +``_FAKE_GIT_BIN`` to keep these tests hermetic. """ from __future__ import annotations +import os import subprocess from unittest.mock import patch @@ -16,9 +21,16 @@ import typer from typer.testing import CliRunner -from comfy_cli import cmdline +from comfy_cli import _safe_exec, cmdline from comfy_cli.command import install as install_inner +_FAKE_GIT_BIN = os.path.join(os.sep, "usr", "bin", "git") + + +def _is_git(argv: list[str]) -> bool: + """``argv[0]`` is now an absolute path, not the bare name.""" + return bool(argv) and os.path.basename(argv[0]).removesuffix(".exe") == "git" + class FakeGit: """A stand-in for ``subprocess.run`` that answers the git calls we make. @@ -66,7 +78,7 @@ def ran(self, *prefix: str) -> bool: @property def git_calls(self) -> list[list[str]]: - return [c for c in self.calls if c and c[0] == "git"] + return [c for c in self.calls if _is_git(c)] def call_matching(self, *prefix: str) -> list[str] | None: for call in self.git_calls: @@ -77,7 +89,7 @@ def call_matching(self, *prefix: str) -> list[str] | None: # -- the dispatcher -------------------------------------------------- def __call__(self, argv, **kwargs): argv = list(argv) - if not argv or argv[0] != "git": + if not _is_git(argv): self.other_calls.append((argv, kwargs)) return self._done(argv, self.pip_returncode) @@ -130,7 +142,10 @@ def _done(argv, returncode, stdout="", stderr=""): @pytest.fixture def fake_git(): git = FakeGit(tags=("v0.2.7", "v0.2.9", "v0.3.0", "v0.3.1")) - with patch("comfy_cli.command.install.subprocess.run", side_effect=git): + with ( + patch.object(_safe_exec.shutil, "which", return_value=_FAKE_GIT_BIN), + patch("comfy_cli.command.install.subprocess.run", side_effect=git), + ): yield git @@ -336,7 +351,7 @@ def test_plain_update_still_pulls(self, fake_git, tmp_path): """The bare ``comfy update comfy`` path must be untouched by --version.""" _run_update(tmp_path) - assert fake_git.git_calls[0] == ["git", "pull"] + assert fake_git.git_calls[0] == [_FAKE_GIT_BIN, "pull"] assert fake_git.other_calls[0][0] == [ "/resolved/python", "-m", diff --git a/tests/comfy_cli/output/test_inline_preview.py b/tests/comfy_cli/output/test_inline_preview.py new file mode 100644 index 00000000..b3953b5f --- /dev/null +++ b/tests/comfy_cli/output/test_inline_preview.py @@ -0,0 +1,107 @@ +"""Tests for the best-effort inline previewer's ffmpeg/ffprobe spawns. + +Unlike ``comfy preview`` (which fails loudly when ffmpeg is missing), this module +is documented to skip silently. It therefore uses ``resolve_binary`` — the probe +entry point — so an unresolvable *or* CWD-planted binary degrades to "no +preview", never to a spawn and never to an exception. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path +from unittest.mock import patch + +from comfy_cli import _safe_exec +from comfy_cli.output import preview as inline_preview + + +def _which_cwd_first(system_dir: Path): + """A ``shutil.which`` that searches the CWD before ``$PATH`` — Windows' order.""" + + def which(name, *_args, **_kwargs): + for candidate in (Path(os.getcwd()) / name, system_dir / name): + if candidate.exists(): + return str(candidate) + return None + + return which + + +def _system_bin(tmp_path: Path, *names: str) -> Path: + directory = tmp_path / "usr_bin" + directory.mkdir(exist_ok=True) + for name in names: + (directory / name).write_text("") + return directory + + +def test_thumbnail_spawns_resolved_ffmpeg(tmp_path, monkeypatch): + system_bin = _system_bin(tmp_path, "ffmpeg") + monkeypatch.chdir(tmp_path) + calls: list[list[str]] = [] + + def fake_run(argv, **_kwargs): + calls.append(list(argv)) + return subprocess.CompletedProcess(args=list(argv), returncode=1, stdout=b"", stderr=b"") + + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(inline_preview.subprocess, "run", fake_run), + ): + inline_preview._show_video_thumbnail(tmp_path / "clip.mp4") + + assert [c[0] for c in calls] == [str(system_bin / "ffmpeg")] + + +def test_thumbnail_skips_silently_for_a_cwd_planted_ffmpeg(tmp_path, monkeypatch): + system_bin = _system_bin(tmp_path) # nothing legitimate on PATH + attacker_cwd = tmp_path / "attacker" + attacker_cwd.mkdir() + (attacker_cwd / "ffmpeg").write_text("") + monkeypatch.chdir(attacker_cwd) + + calls: list[list[str]] = [] + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(inline_preview.subprocess, "run", lambda argv, **kw: calls.append(list(argv))), + ): + inline_preview._show_video_thumbnail(attacker_cwd / "clip.mp4") + + assert calls == [] + + +def test_video_info_spawns_resolved_ffprobe(tmp_path, monkeypatch): + system_bin = _system_bin(tmp_path, "ffprobe") + monkeypatch.chdir(tmp_path) + calls: list[list[str]] = [] + + def fake_run(argv, **_kwargs): + calls.append(list(argv)) + return subprocess.CompletedProcess(args=list(argv), returncode=1, stdout="", stderr="") + + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(inline_preview.subprocess, "run", fake_run), + ): + inline_preview._show_video_info(tmp_path / "clip.mp4") + + assert [c[0] for c in calls] == [str(system_bin / "ffprobe")] + + +def test_video_info_skips_silently_for_a_cwd_planted_ffprobe(tmp_path, monkeypatch): + system_bin = _system_bin(tmp_path) + attacker_cwd = tmp_path / "attacker" + attacker_cwd.mkdir() + (attacker_cwd / "ffprobe").write_text("") + monkeypatch.chdir(attacker_cwd) + + calls: list[list[str]] = [] + with ( + patch.object(_safe_exec.shutil, "which", _which_cwd_first(system_bin)), + patch.object(inline_preview.subprocess, "run", lambda argv, **kw: calls.append(list(argv))), + ): + inline_preview._show_video_info(attacker_cwd / "clip.mp4") + + assert calls == [] diff --git a/tests/comfy_cli/test_safe_exec.py b/tests/comfy_cli/test_safe_exec.py index a1eb5772..c2633b6c 100644 --- a/tests/comfy_cli/test_safe_exec.py +++ b/tests/comfy_cli/test_safe_exec.py @@ -206,3 +206,54 @@ def test_resolve_binary_skips_drive_less_windows_match(self): patch.object(_safe_exec.shutil, "which", return_value=r"\tools\nvidia-smi.exe"), ): assert _safe_exec.resolve_binary("nvidia-smi") is None + + +class TestResolveRequiredBinary: + """The required-binary companion: same gates, but a failure is loud. + + ``git``/``ffmpeg`` failing is fatal to the command that wanted them, so + "return ``None`` and let the caller skip" — right for a probe — would just + push a ``TypeError`` one frame down.""" + + def test_returns_the_resolved_path(self, tmp_path): + system_dir = tmp_path / "System32" + system_dir.mkdir() + legit = system_dir / "git" + legit.write_text("") + with ( + patch.object(_safe_exec.os, "getcwd", return_value=str(tmp_path)), + patch.object(_safe_exec.shutil, "which", return_value=str(legit)), + ): + assert _safe_exec.resolve_required_binary("git") == str(legit) + + def test_raises_when_the_binary_is_absent(self): + with patch.object(_safe_exec.shutil, "which", return_value=None): + with pytest.raises(_safe_exec.BinaryNotFoundError, match="git"): + _safe_exec.resolve_required_binary("git") + + def test_raises_for_a_binary_planted_in_the_cwd(self, tmp_path): + planted = tmp_path / "git" + planted.write_text("") + with ( + patch.object(_safe_exec.os, "getcwd", return_value=str(tmp_path)), + patch.object(_safe_exec.shutil, "which", return_value=str(planted)), + ): + with pytest.raises(_safe_exec.BinaryNotFoundError, match="current directory"): + _safe_exec.resolve_required_binary("git") + + def test_raises_for_a_non_bare_name(self): + """The gates are shared with ``resolve_binary`` — a name carrying a path + component is refused here too, rather than looked up.""" + with pytest.raises(_safe_exec.BinaryNotFoundError): + _safe_exec.resolve_required_binary("./git") + + @pytest.mark.parametrize("caught_as", [FileNotFoundError, OSError, RuntimeError]) + def test_is_catchable_by_the_existing_degradation_handlers(self, caught_as): + """Call sites that already tolerated a missing binary catch + ``FileNotFoundError``/``OSError``; the ``RuntimeError`` base is there for + callers that want to name the failure without an OS-error class. Both + must hold, or converting a tolerant site would silently turn a graceful + degradation into a crash.""" + with patch.object(_safe_exec.shutil, "which", return_value=None): + with pytest.raises(caught_as): + _safe_exec.resolve_required_binary("git") diff --git a/tests/comfy_cli/test_safe_exec_call_sites.py b/tests/comfy_cli/test_safe_exec_call_sites.py new file mode 100644 index 00000000..c9f62fda --- /dev/null +++ b/tests/comfy_cli/test_safe_exec_call_sites.py @@ -0,0 +1,310 @@ +"""Regression tests for the ``git`` / ``ffmpeg`` / ``ffprobe`` spawn sites. + +Companion to :mod:`tests.comfy_cli.test_safe_exec`, which unit-tests the resolver +itself. Here the contract under test is per *call site*: + +* the argv actually handed to :mod:`subprocess` starts with the **resolved + absolute path**, never the bare name Windows' ``CreateProcess`` would look up + in the current working directory; +* a binary planted **directly in the CWD** is never executed — the site either + fails loudly (required binaries) or degrades exactly as it already did when the + binary was simply absent (tolerant sites and the best-effort previewer). + +The sites that ``os.chdir`` into a user-supplied directory before spawning get an +explicit test each, because there the attacker-controlled directory *is* the CWD +by construction rather than by the user happening to be standing in it. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +# ``comfy_cli.command`` must be imported before ``comfy_cli.git_utils``: the two +# form a pre-existing import cycle (``git_utils`` → ``command.github.pr_info`` → +# ``command/__init__`` → ``command.install`` → ``git_utils``) that only resolves +# when the ``command`` package is the entry point, which is how the real CLI +# always reaches it. ``# isort: split`` pins that order against the formatter. +from comfy_cli.command import install as install_cmd +from comfy_cli.command import outdated as outdated_cmd +from comfy_cli.command.github.pr_info import PRInfo + +# isort: split +from comfy_cli import _safe_exec, file_utils, git_utils + +# --- harness --------------------------------------------------------------- + + +def _plant(directory: Path, name: str) -> Path: + """Create a stand-in executable called ``name`` inside ``directory``.""" + directory.mkdir(parents=True, exist_ok=True) + binary = directory / name + binary.write_text("") + return binary + + +def _cwd_first_which(system_dir: Path): + """A ``shutil.which`` that searches the CWD *before* ``$PATH``. + + That is Windows' lookup order (and POSIX's too when ``$PATH`` holds ``.`` or + an empty entry), and it is what makes CWD planting exploitable. Emulating it + here lets these tests reproduce the Windows vector on any host: a plant in + the CWD wins the lookup, and the resolver's job is to refuse it. + """ + + def which(name, *_args, **_kwargs): + for candidate in (Path(os.getcwd()) / name, system_dir / name): + if candidate.exists(): + return str(candidate) + return None + + return which + + +class _Recorder: + """Stand-in for ``subprocess.run`` that records argv and never spawns.""" + + def __init__(self, returncode: int = 0, stdout: str = "", stderr: str = ""): + self.calls: list[list[str]] = [] + self._result = (returncode, stdout, stderr) + + def __call__(self, argv, *_args, **_kwargs): + self.calls.append(list(argv)) + returncode, stdout, stderr = self._result + return subprocess.CompletedProcess(args=list(argv), returncode=returncode, stdout=stdout, stderr=stderr) + + @property + def argv0s(self) -> list[str]: + return [call[0] for call in self.calls] + + +@pytest.fixture +def system_bin(tmp_path): + """A directory standing in for a normal, trusted absolute ``$PATH`` entry.""" + directory = tmp_path / "usr_bin" + directory.mkdir() + return directory + + +@pytest.fixture +def neutral_cwd(tmp_path, monkeypatch): + """A CWD with nothing planted in it.""" + directory = tmp_path / "neutral_cwd" + directory.mkdir() + monkeypatch.chdir(directory) + return directory + + +# --- git_utils: the two ``os.chdir``-then-spawn sites ----------------------- + + +class TestGitUtilsResolvesBeforeChdir: + """``git_checkout_tag`` / ``checkout_pr`` ``os.chdir`` into a caller-supplied + repo directory and then spawn ``git``. Both resolve ``git`` *before* that + chdir, so a ``git`` planted in the target repo can neither be executed nor + shadow the legitimate binary into a hard failure.""" + + def test_checkout_tag_spawns_resolved_path_not_the_repo_plant(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + repo = tmp_path / "repo" + planted = _plant(repo, "git") + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(git_utils.subprocess, "run", recorder), + ): + assert git_utils.git_checkout_tag(str(repo), "v1.2.3") is True + + assert recorder.calls, "expected git to be invoked" + assert set(recorder.argv0s) == {str(legit)} + assert str(planted) not in recorder.argv0s + assert "git" not in recorder.argv0s # never the bare name + + def test_checkout_pr_spawns_resolved_path_not_the_repo_plant(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + repo = tmp_path / "repo" + planted = _plant(repo, "git") + pr_info = PRInfo( + number=7, + head_repo_url="https://github.com/comfy/comfy.git", + head_branch="feature/x", + base_repo_url="https://github.com/comfy/comfy.git", + base_branch="main", + title="t", + user="u", + mergeable=True, + ) + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(git_utils.subprocess, "run", recorder), + ): + assert git_utils.checkout_pr(str(repo), pr_info) is True + + assert recorder.calls, "expected git to be invoked" + assert set(recorder.argv0s) == {str(legit)} + assert str(planted) not in recorder.argv0s + + def test_checkout_tag_refuses_a_git_planted_in_the_cwd(self, tmp_path, system_bin, monkeypatch): + """The remaining shape: the *process* CWD is the attacker's directory.""" + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + planted = _plant(attacker_cwd, "git") + monkeypatch.chdir(attacker_cwd) + repo = tmp_path / "repo" + repo.mkdir() + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(git_utils.subprocess, "run", recorder), + pytest.raises(_safe_exec.BinaryNotFoundError), + ): + git_utils.git_checkout_tag(str(repo), "v1.2.3") + + assert recorder.calls == [], f"planted {planted} must never be executed" + + def test_checkout_tag_restores_cwd_when_git_is_unresolvable(self, tmp_path, system_bin, neutral_cwd): + """Resolution happens before the chdir, so the failure cannot strand the + process inside the repo directory.""" + repo = tmp_path / "repo" + repo.mkdir() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + pytest.raises(_safe_exec.BinaryNotFoundError), + ): + git_utils.git_checkout_tag(str(repo), "v1.2.3") + assert Path(os.getcwd()).resolve() == neutral_cwd.resolve() + + +# --- tolerant git sites: degrade exactly as a missing git already did ------- + + +class TestTolerantGitSitesDegrade: + """``BinaryNotFoundError`` subclasses ``FileNotFoundError``, so the sites that + already swallowed a missing ``git`` keep their existing degradation instead of + starting to raise.""" + + def test_list_git_tracked_files_uses_resolved_path(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + recorded: list[list[str]] = [] + + def fake_check_output(argv, **_kwargs): + recorded.append(list(argv)) + return "a.py\nb.py\n" + + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(file_utils.subprocess, "check_output", fake_check_output), + ): + assert file_utils.list_git_tracked_files(str(tmp_path)) == ["a.py", "b.py"] + + assert recorded[0][0] == str(legit) + + def test_list_git_tracked_files_returns_empty_for_cwd_planted_git(self, tmp_path, system_bin, monkeypatch): + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + _plant(attacker_cwd, "git") + monkeypatch.chdir(attacker_cwd) + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(file_utils.subprocess, "check_output", recorder), + ): + assert file_utils.list_git_tracked_files(str(tmp_path)) == [] + assert recorder.calls == [] + + def test_outdated_git_output_uses_resolved_path(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + recorder = _Recorder(stdout="true\n") + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(outdated_cmd.subprocess, "run", recorder), + ): + assert outdated_cmd._git_output(["rev-parse", "--is-inside-work-tree"], str(tmp_path)) == "true" + assert recorder.argv0s == [str(legit)] + + def test_outdated_git_output_returns_none_for_cwd_planted_git(self, tmp_path, system_bin, monkeypatch): + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + _plant(attacker_cwd, "git") + monkeypatch.chdir(attacker_cwd) + + recorder = _Recorder(stdout="true\n") + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(outdated_cmd.subprocess, "run", recorder), + ): + assert outdated_cmd._git_output(["rev-parse"], str(tmp_path)) is None + assert recorder.calls == [] + + def test_install_git_capture_uses_resolved_path(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + recorder = _Recorder(stdout="abc1234\n") + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(install_cmd.subprocess, "run", recorder), + ): + result = install_cmd._git_capture(str(tmp_path), "rev-parse", "--short", "HEAD") + assert result.returncode == 0 + assert recorder.argv0s == [str(legit)] + + def test_install_git_capture_fails_soft_for_cwd_planted_git(self, tmp_path, system_bin, monkeypatch): + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + _plant(attacker_cwd, "git") + monkeypatch.chdir(attacker_cwd) + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(install_cmd.subprocess, "run", recorder), + ): + result = install_cmd._git_capture(str(tmp_path), "rev-parse") + assert result.returncode == 1 + assert recorder.calls == [] + + def test_resolve_latest_tag_from_local_uses_resolved_path(self, tmp_path, system_bin, neutral_cwd): + legit = _plant(system_bin, "git") + recorder = _Recorder(stdout="v0.1.0\nv0.2.0\n") + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(install_cmd.subprocess, "run", recorder), + ): + tag, fetch_ok = install_cmd._resolve_latest_tag_from_local(str(tmp_path)) + assert (tag, fetch_ok) == ("v0.2.0", True) + assert set(recorder.argv0s) == {str(legit)} + + +class TestCloneComfyuiUsesResolvedGit: + def test_clone_uses_resolved_path(self, system_bin, neutral_cwd, tmp_path): + legit = _plant(system_bin, "git") + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(install_cmd.subprocess, "run", recorder), + ): + install_cmd.clone_comfyui("https://github.com/comfy/comfy.git", str(tmp_path / "dest")) + assert recorder.argv0s == [str(legit)] + + def test_clone_refuses_a_git_planted_in_the_cwd(self, tmp_path, system_bin, monkeypatch): + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + _plant(attacker_cwd, "git") + monkeypatch.chdir(attacker_cwd) + + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + patch.object(install_cmd.subprocess, "run", recorder), + pytest.raises(_safe_exec.BinaryNotFoundError), + ): + install_cmd.clone_comfyui("https://github.com/comfy/comfy.git", str(tmp_path / "dest")) + assert recorder.calls == [] From abf558f5a553b5e84624ad213f73aec0ad1f7d15 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 31 Jul 2026 05:59:12 -0700 Subject: [PATCH 2/2] fix(exec): distinguish "binary absent" from "binary refused" (BE-5358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Cursor review panel on #643. resolve_required_binary collapsed two very different events into one "was not found on PATH" message: git/ffmpeg genuinely not installed, and git/ffmpeg found but refused because the only match was CWD-anchored. That told users with a working binary to go install it, and hid the interesting part — something named git was sitting in their CWD. _safe_exec now reports *why* it refused (BinaryRefusal / BinaryNotFoundError .reason / .candidate) and callers branch on it: - file_utils.list_git_tracked_files: a *refused* git no longer degrades to [], which zip_files reads as "not a git repo" and answers by walking the whole directory — packaging untracked/gitignored files (.env, keys, venvs) into the archive `comfy node publish` uploads. An absent git still degrades as before. `comfy node pack` reports the refusal and aborts. - command/preview: refusal renders the new ffmpeg_untrusted code instead of "install ffmpeg"; both ffprobe/ffmpeg spawns now have timeouts, so a corrupt or crafted media file can't hang `comfy preview` while buffering. - git_utils.git_checkout_tag / checkout_pr: both are documented to return False on failure and both callers rely on it, so a refusal comes back as False rather than escaping as a traceback past their handlers. - install.switch_comfyui_version: checks git up front as version_switch_git_unavailable — otherwise _git_capture's rc=1 made the first `rev-parse --verify ` report "version not found: no tag vX". - registry/config_parser: handler widened to OSError, so a refusal doesn't escape after create_comfynode_config() has already written pyproject.toml. Also option-injection hardening on the git argv: `git clone -- `, and reject_option_like_ref for `git checkout `, where a `--` separator cannot help (git scans options before the first positional). git tag/branch both refuse to create a name starting with `-`, and a SHA never does, so nothing legitimate is rejected. Tests: the fake-binary fixtures pinned drive-less \usr\bin\, which _is_fully_qualified rejects on Windows — the exact platform this hardening targets — so every test using them would have failed there. Pinned a drive-qualified path under os.name == "nt". --- comfy_cli/_safe_exec.py | 173 ++++++++++++++++-- comfy_cli/command/custom_nodes/command.py | 9 +- comfy_cli/command/install.py | 36 +++- comfy_cli/command/preview.py | 55 ++++-- comfy_cli/error_codes.py | 13 ++ comfy_cli/file_utils.py | 34 +++- comfy_cli/git_utils.py | 138 +++++++++----- comfy_cli/registry/config_parser.py | 8 +- tests/comfy_cli/command/test_preview.py | 6 +- .../comfy_cli/command/test_update_version.py | 6 +- tests/comfy_cli/test_safe_exec.py | 43 ++++- tests/comfy_cli/test_safe_exec_call_sites.py | 89 ++++++++- 12 files changed, 512 insertions(+), 98 deletions(-) diff --git a/comfy_cli/_safe_exec.py b/comfy_cli/_safe_exec.py index 9a04214b..d3c260d4 100644 --- a/comfy_cli/_safe_exec.py +++ b/comfy_cli/_safe_exec.py @@ -20,26 +20,77 @@ * :func:`resolve_required_binary` — for binaries a command cannot run without (``git``, ``ffmpeg``). Raises :class:`BinaryNotFoundError` with an actionable message instead of returning ``None``. It is a thin wrapper over - :func:`resolve_binary`, so the CWD/qualification gates cannot drift apart. + :func:`resolve_binary_detail`, so the CWD/qualification gates cannot drift + apart. + +Both are built on :func:`resolve_binary_detail`, which reports *why* a name was +refused (:class:`BinaryRefusal`). "Absent from ``$PATH``" and "present but +refused because it resolved into the CWD" are very different events — one is a +missing install, the other is a possible planted binary — and collapsing them +into one message tells a user with a perfectly good ``git`` to go install ``git``. +Callers that surface the failure should branch on +:attr:`BinaryNotFoundError.reason`. """ from __future__ import annotations +import enum import logging import ntpath import os import shutil +from typing import NamedTuple logger = logging.getLogger(__name__) _PATH_SEPARATORS = ("/", "\\") +class BinaryRefusal(enum.Enum): + """Why :func:`resolve_binary_detail` declined to hand back a path.""" + + ABSENT = "absent" + """``shutil.which`` found no match at all — the binary is not installed.""" + + NOT_BARE_NAME = "not_bare_name" + """The caller passed something carrying a path component; see + :func:`_is_bare_name`. A programming error, not a user-fixable condition.""" + + CWD_ANCHORED = "cwd_anchored" + """A match was found, but it is anchored in the current working directory + (a relative ``$PATH`` entry, or an absolute entry that *is* the CWD). The + binary may well be legitimate — running from a directory that is itself a + ``$PATH`` entry such as ``/usr/bin`` lands here — but it cannot be + distinguished from a plant, so it is refused.""" + + UNVERIFIABLE = "unverifiable" + """A match was found but could not be placed relative to the CWD (a deleted + or unreadable CWD, an unresolvable path). Refused fail-closed.""" + + +class BinaryResolution(NamedTuple): + """What :func:`resolve_binary_detail` concluded about one name.""" + + path: str | None + """The trusted absolute path, or ``None`` if the name was refused.""" + + reason: BinaryRefusal | None + """``None`` on success; otherwise why the name was refused.""" + + candidate: str | None + """The path ``shutil.which`` returned, even when it was then refused — so a + diagnostic can name the file it declined to run. ``None`` when the lookup + never got that far.""" + + class BinaryNotFoundError(RuntimeError, FileNotFoundError): """A binary a command cannot run without could not be trusted-resolved. Raised by :func:`resolve_required_binary` — either the binary is absent from ``$PATH``, or the only match was CWD-anchored and therefore refused. + :attr:`reason` says which, so callers can render an accurate diagnostic + instead of always claiming the binary is not installed; :attr:`candidate` + carries the path that was refused, when there was one. It deliberately subclasses **both** ``RuntimeError`` and ``FileNotFoundError``. ``FileNotFoundError`` is the exception ``subprocess`` @@ -52,6 +103,29 @@ class BinaryNotFoundError(RuntimeError, FileNotFoundError): name the failure explicitly without reaching for an OS-error class. """ + def __init__( + self, + message: str, + *, + binary: str, + reason: BinaryRefusal, + candidate: str | None = None, + ): + super().__init__(message) + self.binary = binary + self.reason = reason + self.candidate = candidate + + @property + def is_absent(self) -> bool: + """``True`` only when the binary is genuinely not installed. + + Everything else means a match *was* found and then refused — the + distinction callers need before telling a user to install something they + already have. + """ + return self.reason is BinaryRefusal.ABSENT + def _is_bare_name(name: str) -> bool: """Return ``True`` if ``name`` is a plain binary name with no path part. @@ -104,16 +178,32 @@ def is_planted_in_cwd(path: str) -> bool: the caller then skips the probe, which is the same degradation as the binary being absent. Failing open here would be the module's only error path that hands an unvetted string to :mod:`subprocess`. + + Callers that need to *tell those two apart* — "definitely in the CWD" vs "we + could not look" — should use :func:`_place_relative_to_cwd`, which keeps them + separate; this wrapper collapses both to the refusing answer. + """ + return _place_relative_to_cwd(path) is not False + + +def _place_relative_to_cwd(path: str) -> bool | None: + """Tri-state companion to :func:`is_planted_in_cwd`. + + ``True`` — ``path``'s immediate parent *is* the CWD. ``False`` — it + demonstrably is not. ``None`` — the comparison could not be made at all + (deleted CWD, unresolvable path). Both ``True`` and ``None`` are refusals, + but only ``True`` is evidence of a plant, and the two deserve different + messages. """ try: cwd = os.path.normcase(os.path.realpath(os.getcwd())) # ``os.path.dirname`` of a bare/relative ``which`` result is "", which # ``realpath`` correctly resolves against the CWD. parent = os.path.normcase(os.path.realpath(os.path.dirname(path))) - return parent == cwd except (OSError, ValueError): logger.debug("cannot place %r relative to the CWD; treating as planted", path, exc_info=True) - return True + return None + return parent == cwd def resolve_binary(name: str) -> str | None: @@ -154,23 +244,46 @@ def resolve_binary(name: str) -> str | None: ``C:\\Windows\\System32``): the probe is then skipped and the caller degrades exactly as it would if the binary were not installed. """ + return resolve_binary_detail(name).path + + +def resolve_binary_detail(name: str) -> BinaryResolution: + """The gate shared by :func:`resolve_binary` and :func:`resolve_required_binary`. + + Returns a :class:`BinaryResolution`: ``path`` set and ``reason`` ``None`` on + success, or ``path`` ``None`` and ``reason`` naming the + :class:`BinaryRefusal` that stopped it. See :func:`resolve_binary` for the + full rationale behind each gate; this function exists so the *required* + wrapper can say which gate fired rather than emitting one message that + guesses. + + Like :func:`resolve_binary`, it never raises: an unexpected error degrades to + :attr:`BinaryRefusal.UNVERIFIABLE`. + """ try: if not _is_bare_name(name): logger.debug("refusing to resolve %r: not a bare binary name", name) - return None + return BinaryResolution(None, BinaryRefusal.NOT_BARE_NAME, None) path = shutil.which(name) if path is None: - return None + return BinaryResolution(None, BinaryRefusal.ABSENT, None) if not _is_fully_qualified(path): + # ``which`` returns ``os.path.join(entry, name)``, so a relative + # result means the matching ``$PATH`` entry was relative — i.e. the + # match lives under the CWD. logger.debug("skipping %r: match is not fully qualified (%s)", name, path) - return None - if is_planted_in_cwd(path): + return BinaryResolution(None, BinaryRefusal.CWD_ANCHORED, path) + in_cwd = _place_relative_to_cwd(path) + if in_cwd is None: + logger.debug("skipping %r: cannot place match relative to the CWD (%s)", name, path) + return BinaryResolution(None, BinaryRefusal.UNVERIFIABLE, path) + if in_cwd: logger.debug("skipping %r: resolved into CWD (%s)", name, path) - return None - return path + return BinaryResolution(None, BinaryRefusal.CWD_ANCHORED, path) + return BinaryResolution(path, None, path) except Exception: logger.debug("resolving binary %r failed", name, exc_info=True) - return None + return BinaryResolution(None, BinaryRefusal.UNVERIFIABLE, None) def resolve_required_binary(name: str) -> str: @@ -190,14 +303,42 @@ def resolve_required_binary(name: str) -> str: first means a plant in the target directory cannot even shadow a legitimate binary into a hard failure. + The raised error names the *specific* gate that fired (see + :attr:`BinaryNotFoundError.reason`), because "not installed" and "installed + but refused" need opposite remedies — telling someone with a working ``git`` + to install ``git`` sends them the wrong way and hides the fact that a binary + was found sitting in their working directory. + :raises BinaryNotFoundError: the binary is not on ``$PATH``, or the only match resolved into the current working directory and was refused. """ - path = resolve_binary(name) - if path is None: + resolution = resolve_binary_detail(name) + if resolution.path is None: raise BinaryNotFoundError( - f"{name!r} was not found on PATH, or the only match resolved into the current directory " - f"(which is not trusted). Install {name} and make sure it is on PATH outside the directory " - f"you are running from." + _refusal_message(name, resolution), + binary=name, + reason=resolution.reason or BinaryRefusal.UNVERIFIABLE, + candidate=resolution.candidate, + ) + return resolution.path + + +def _refusal_message(name: str, resolution: BinaryResolution) -> str: + """An actionable one-liner for the gate that refused ``name``.""" + where = f" ({resolution.candidate})" if resolution.candidate else "" + if resolution.reason is BinaryRefusal.ABSENT: + return f"{name!r} was not found on PATH. Install {name} and make sure it is on PATH, then try again." + if resolution.reason is BinaryRefusal.CWD_ANCHORED: + return ( + f"refusing to run {name!r}: the only match on PATH{where} is in the directory you are " + f"running from, which is not trusted — a program planted there would run instead of the " + f"real {name}. Run from a different directory, or make sure {name} is installed on PATH " + f"outside your working directory." + ) + if resolution.reason is BinaryRefusal.UNVERIFIABLE: + return ( + f"refusing to run {name!r}: could not check whether the match on PATH{where} lives in the " + f"directory you are running from (the working directory may have been deleted). " + f"Run from a directory that still exists and try again." ) - return path + return f"refusing to resolve {name!r}: not a bare binary name." diff --git a/comfy_cli/command/custom_nodes/command.py b/comfy_cli/command/custom_nodes/command.py index aa5319f5..5f2f2578 100644 --- a/comfy_cli/command/custom_nodes/command.py +++ b/comfy_cli/command/custom_nodes/command.py @@ -12,6 +12,7 @@ from rich.console import Console from comfy_cli import constants, logging, tracking, ui, utils +from comfy_cli._safe_exec import BinaryNotFoundError from comfy_cli.command.custom_nodes.bisect_custom_nodes import bisect_app from comfy_cli.command.custom_nodes.cm_cli_util import execute_cm_cli, find_cm_cli from comfy_cli.config_manager import ConfigManager @@ -1342,7 +1343,13 @@ def pack(): if includes: typer.echo(f"Including additional directories: {', '.join(includes)}") - zip_files(zip_filename, includes=includes) + try: + zip_files(zip_filename, includes=includes) + except BinaryNotFoundError as e: + # git was found and refused; packing anyway would sweep in untracked and + # gitignored files. Abort with the reason rather than a traceback. + ui.display_error_message(str(e)) + raise typer.Exit(code=1) from None typer.echo(f"Created zip file: {NODE_ZIP_FILENAME}") logging.info("Node has been packed successfully.") diff --git a/comfy_cli/command/install.py b/comfy_cli/command/install.py index 577b91d7..d7865ebb 100755 --- a/comfy_cli/command/install.py +++ b/comfy_cli/command/install.py @@ -16,12 +16,12 @@ from rich.prompt import Confirm from comfy_cli import constants, ui -from comfy_cli._safe_exec import resolve_required_binary +from comfy_cli._safe_exec import BinaryNotFoundError, resolve_required_binary from comfy_cli.command.custom_nodes.command import update_node_id_cache from comfy_cli.command.github.pr_info import PRInfo from comfy_cli.constants import GPU_OPTION from comfy_cli.cuda_detect import DEFAULT_CUDA_TAG -from comfy_cli.git_utils import checkout_pr, git_checkout_tag +from comfy_cli.git_utils import checkout_pr, git_checkout_tag, reject_option_like_ref from comfy_cli.output import rprint from comfy_cli.resolve_python import ensure_workspace_python from comfy_cli.uv import DependencyCompiler, ensure_pip @@ -235,6 +235,16 @@ def execute( # checkout specified commit if commit is not None: + # A ``--`` separator cannot protect this argument: git scans for options + # *before* the first positional, so ``--upload-pack=…`` would be read as + # an option wherever the separator sits. Git refnames may not begin with + # ``-`` at all, so refusing such a value rejects only inputs that could + # never have worked. + try: + reject_option_like_ref(commit, "commit") + except ValueError as e: + rprint(f"[bold red]{e}[/bold red]") + raise typer.Exit(code=1) from None # Resolved before the chdir: ``repo_dir`` is user-supplied, so a ``git`` # planted there must not be found — nor shadow the real one. git_bin = resolve_required_binary("git") @@ -488,12 +498,16 @@ def clone_comfyui(url: str, repo_dir: str): Clone the ComfyUI repository from the specified URL. """ git_bin = resolve_required_binary("git") + # ``--`` ends option parsing: git permutes options among positionals, so a + # ``url`` (or ``repo_dir``) beginning with ``--`` would otherwise be read as + # an option — ``--upload-pack=`` and ``--config==`` both turn a + # clone into arbitrary command execution. if "@" in url: # clone specific branch url, branch = url.rsplit("@", 1) - subprocess.run([git_bin, "clone", "-b", branch, url, repo_dir], check=True) + subprocess.run([git_bin, "clone", "-b", branch, "--", url, repo_dir], check=True) else: - subprocess.run([git_bin, "clone", url, repo_dir], check=True) + subprocess.run([git_bin, "clone", "--", url, repo_dir], check=True) def _resolve_latest_tag_from_local(repo_dir: str) -> tuple[str | None, bool]: @@ -811,6 +825,20 @@ def switch_comfyui_version( :raises VersionSwitchError: on any resolution, stash, or checkout failure. """ + # Checked up front and named for what it is. ``_git_capture`` deliberately + # never raises, so without this a git that could not be trusted-resolved + # would just look like rc=1 to the first caller below — and the first caller + # is a `rev-parse --verify `, whose non-zero result reports "version not + # found: no tag vX". That sends the user hunting for a version that exists. + try: + resolve_required_binary("git") + except BinaryNotFoundError as exc: + raise VersionSwitchError( + code="version_switch_git_unavailable", + message=str(exc), + hint="a version switch needs a usable git — install it, or run from a directory that has no git binary in it", + ) from exc + previous = _describe_head(comfy_path) # --- 1. Resolve + validate the target before touching anything ----------- diff --git a/comfy_cli/command/preview.py b/comfy_cli/command/preview.py index 6f534297..0994d7f3 100644 --- a/comfy_cli/command/preview.py +++ b/comfy_cli/command/preview.py @@ -28,6 +28,14 @@ _IMAGE_FORMAT_HINTS = ("_pipe", "image2", "png", "jpeg", "mjpeg", "gif", "webp", "bmp", "apng") +# Both spawns capture output, so an ffmpeg/ffprobe that never exits on a corrupt +# or crafted file would hang `comfy preview` forever while buffering. Bounded for +# the same reason the sibling previewer in ``comfy_cli.output.preview`` is, with +# a larger budget: that one grabs a single thumbnail frame, this one renders a +# whole contact sheet or waveform. +_PROBE_TIMEOUT_SECONDS = 30 +_RENDER_TIMEOUT_SECONDS = 120 + def _to_float(v: Any) -> float | None: try: @@ -124,11 +132,15 @@ def build_preview_cmd( def _ffprobe(path: Path, ffprobe_bin: str) -> dict: - proc = subprocess.run( - [ffprobe_bin, "-v", "error", "-print_format", "json", "-show_streams", "-show_format", str(path)], - capture_output=True, - text=True, - ) + try: + proc = subprocess.run( + [ffprobe_bin, "-v", "error", "-print_format", "json", "-show_streams", "-show_format", str(path)], + capture_output=True, + text=True, + timeout=_PROBE_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError(f"ffprobe timed out after {_PROBE_TIMEOUT_SECONDS}s") from exc if proc.returncode != 0: raise RuntimeError(proc.stderr.strip() or "ffprobe failed") return json.loads(proc.stdout or "{}") @@ -158,12 +170,23 @@ def preview_cmd( try: ffmpeg_bin = resolve_required_binary("ffmpeg") ffprobe_bin = resolve_required_binary("ffprobe") - except BinaryNotFoundError: - renderer.error( - code="ffmpeg_unavailable", - message="ffmpeg/ffprobe not found on PATH — `comfy preview` needs them.", - hint="install ffmpeg (e.g. `brew install ffmpeg` / `apt install ffmpeg`)", - ) + except BinaryNotFoundError as exc: + if exc.is_absent: + renderer.error( + code="ffmpeg_unavailable", + message="ffmpeg/ffprobe not found on PATH — `comfy preview` needs them.", + hint="install ffmpeg (e.g. `brew install ffmpeg` / `apt install ffmpeg`)", + ) + else: + # Installed, but the match was refused. Saying "install ffmpeg" here + # would send a user who already has it down the wrong path *and* bury + # the interesting part: something named ffmpeg is sitting in the + # directory they ran from. + renderer.error( + code="ffmpeg_untrusted", + message=str(exc), + hint="run `comfy preview` from a directory that does not contain an ffmpeg/ffprobe binary", + ) raise typer.Exit(code=1) from None try: @@ -195,7 +218,15 @@ def preview_cmd( duration=info["duration"], ffmpeg_bin=ffmpeg_bin, ) - proc = subprocess.run(cmd, capture_output=True, text=True) + try: + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=_RENDER_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + renderer.error( + code="preview_failed", + message=f"ffmpeg did not finish within {_RENDER_TIMEOUT_SECONDS}s.", + hint="check the file isn't corrupt; try a smaller --grid/--width", + ) + raise typer.Exit(code=1) from None if proc.returncode != 0 or not out_path.is_file(): renderer.error( code="preview_failed", diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 1cd00e37..3232ab47 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -522,6 +522,12 @@ class ErrorCode: "`comfy preview` needs ffmpeg + ffprobe on PATH and they weren't found.", "install ffmpeg (e.g. `brew install ffmpeg` / `apt install ffmpeg`)", ), + ErrorCode( + "ffmpeg_untrusted", + "ffmpeg/ffprobe were found, but only inside the directory you ran from, " + "so they were refused rather than executed — the shape of a planted binary.", + "run `comfy preview` from a directory that does not contain an ffmpeg/ffprobe binary", + ), ErrorCode( "preview_unsupported_media", "The file has no image/video/audio stream to preview.", @@ -737,6 +743,13 @@ class ErrorCode: "`comfy update comfy --version X` could not resolve X to a ComfyUI tag; the workspace was left untouched.", "run `git tag --list 'v*'` in your ComfyUI workspace to see every available version", ), + ErrorCode( + "version_switch_git_unavailable", + "`comfy update comfy --version X` could not use git at all — it is absent from PATH, or the " + "only match resolved into the directory you ran from and was refused. Reported separately so " + "it isn't misread as the requested version not existing.", + "a version switch needs a usable git — install it, or run from a directory that has no git binary in it", + ), ErrorCode( "version_switch_dirty_tree", "`comfy update comfy --version X --no-stash` found uncommitted changes and refused to switch.", diff --git a/comfy_cli/file_utils.py b/comfy_cli/file_utils.py index f01e2e50..3d80a5bf 100644 --- a/comfy_cli/file_utils.py +++ b/comfy_cli/file_utils.py @@ -12,7 +12,7 @@ from pathspec import PathSpec from comfy_cli import constants, ui -from comfy_cli._safe_exec import resolve_required_binary +from comfy_cli._safe_exec import BinaryNotFoundError, resolve_required_binary class DownloadException(Exception): @@ -445,12 +445,27 @@ def _load_comfyignore_spec(ignore_filename: str = ".comfyignore") -> PathSpec | def list_git_tracked_files(base_path: str | os.PathLike = ".") -> list[str]: + """Git-tracked files under ``base_path``, or ``[]`` when git can't tell us. + + ``[]`` means "no git answer" and callers (see :func:`zip_files`) treat it as + "not a git repository". An *absent* git has always produced that, so it still + does. A git that was found and then **refused** must not: the caller would + silently fall back to walking the whole directory, so the refusal is raised + rather than flattened into the same empty list. + """ + # Resolved outside the tolerant handler below so the two cases stay + # distinguishable — ``BinaryNotFoundError`` subclasses ``FileNotFoundError``, + # which that handler swallows. + try: + git_bin = resolve_required_binary("git") + except BinaryNotFoundError as exc: + if not exc.is_absent: + raise + return [] + try: - # ``BinaryNotFoundError`` subclasses ``FileNotFoundError``, so an absent — - # or CWD-planted, hence refused — ``git`` degrades to ``[]`` here exactly - # as a missing ``git`` already did. result = subprocess.check_output( - [resolve_required_binary("git"), "-C", os.fspath(base_path), "ls-files"], + [git_bin, "-C", os.fspath(base_path), "ls-files"], text=True, ) except (subprocess.SubprocessError, FileNotFoundError): @@ -471,7 +486,14 @@ def _is_force_included(rel_path: str, include_prefixes: list[str]) -> bool: def zip_files(zip_filename, includes=None): - """Zip git-tracked files respecting optional .comfyignore patterns.""" + """Zip git-tracked files respecting optional .comfyignore patterns. + + :raises BinaryNotFoundError: ``git`` was found but refused (see + :func:`comfy_cli._safe_exec.resolve_required_binary`). The walk-everything + fallback below is safe for "this isn't a git repo", but not for "we can't + trust git": it would package untracked and gitignored files — ``.env``, + keys, venvs — into an archive that ``comfy node publish`` uploads. + """ includes = includes or [] include_prefixes: list[str] = [_normalize_path(os.path.normpath(include.lstrip("/"))) for include in includes] diff --git a/comfy_cli/git_utils.py b/comfy_cli/git_utils.py index a85ca83e..bcc2bfc0 100644 --- a/comfy_cli/git_utils.py +++ b/comfy_cli/git_utils.py @@ -5,12 +5,54 @@ from rich.panel import Panel from rich.text import Text -from comfy_cli._safe_exec import resolve_required_binary +from comfy_cli._safe_exec import BinaryNotFoundError, resolve_required_binary from comfy_cli.command.github.pr_info import PRInfo console = Console() +def reject_option_like_ref(ref: str, what: str = "ref") -> None: + """Refuse a revision that git would parse as an option. + + Git scans for options *before* the first positional and permutes them, so a + value like ``--upload-pack=`` passed where a revision is expected is + read as an option no matter where a ``--`` separator sits — for ``git + clone`` the separator works, for ``git checkout `` it cannot. + + Nothing legitimate is lost: ``git tag`` and ``git branch`` both refuse to + *create* a name beginning with ``-`` ("'-foo' is not a valid tag name"), and + a commit SHA never starts with one. So a ``-``-leading value is always either + a typo or an injection attempt. + + :raises ValueError: ``ref`` begins with ``-``. + """ + if ref.startswith("-"): + raise ValueError(f"invalid {what} {ref!r}: git {what}s may not begin with '-'") + + +def _print_checkout_failure(tag: str, detail: str, stderr: str | None = None) -> None: + """Render the "Git Checkout Failed" panel for a tag checkout that didn't happen.""" + error_message = Text() + error_message.append("Git Checkout Error", style="bold red on white") + error_message.append("\n\nFailed to checkout tag: ", style="bold yellow") + error_message.append(f"[cyan]{tag}[/cyan]") + error_message.append("\n\nError details:", style="bold red") + error_message.append(f"\n{detail}", style="italic") + + if stderr: + error_message.append("\n\nError output:", style="bold red") + error_message.append(f"\n{stderr}", style="italic yellow") + + console.print( + Panel( + error_message, + title="[bold white on red]Git Checkout Failed[/bold white on red]", + border_style="red", + expand=False, + ) + ) + + def sanitize_for_local_branch(branch_name: str) -> str: if not branch_name: return "unknown" @@ -38,15 +80,21 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: :param repo_path: Path to the Git repository :param tag: The tag to checkout - :return: True if the checkout succeeds, False if any git command failed. - :raises BinaryNotFoundError: ``git`` is absent from ``$PATH`` (previously a - bare ``FileNotFoundError`` from ``subprocess``, which was equally - uncaught here — the message is the only thing that changed). + :return: True if the checkout succeeds, False if any git command failed — + including git being absent or refused. Both callers + (``checkout_stable_comfyui``, ``checkout_pr``) turn ``False`` into a + clean CLI error, so a resolution failure must not escape as a traceback + past them. """ # Resolved BEFORE the ``os.chdir`` below so the lookup can't see a ``git`` # planted in the caller-supplied ``repo_path``, and so the absolute path we # spawn defeats Windows' current-directory search once we are inside it. - git_bin = resolve_required_binary("git") + try: + reject_option_like_ref(tag, "tag") + git_bin = resolve_required_binary("git") + except (BinaryNotFoundError, ValueError) as e: + _print_checkout_failure(tag, str(e)) + return False original_dir = os.getcwd() try: # Change to the repository directory @@ -73,26 +121,7 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: return True except subprocess.CalledProcessError as e: - error_message = Text() - error_message.append("Git Checkout Error", style="bold red on white") - error_message.append("\n\nFailed to checkout tag: ", style="bold yellow") - error_message.append(f"[cyan]{tag}[/cyan]") - error_message.append("\n\nError details:", style="bold red") - error_message.append(f"\n{str(e)}", style="italic") - - if e.stderr: - error_message.append("\n\nError output:", style="bold red") - error_message.append(f"\n{e.stderr}", style="italic yellow") - - console.print( - Panel( - error_message, - title="[bold white on red]Git Checkout Failed[/bold white on red]", - border_style="red", - expand=False, - ) - ) - + _print_checkout_failure(tag, str(e), stderr=e.stderr) return False finally: # Ensure we always return to the original directory @@ -100,8 +129,18 @@ def git_checkout_tag(repo_path: str, tag: str) -> bool: def checkout_pr(repo_path: str, pr_info: PRInfo) -> bool: + """Check out ``pr_info``'s branch in ``repo_path``. + + :return: True on success, False on any git failure — including git being + absent or refused, which callers handle as a clean CLI error rather than + a traceback (same contract as :func:`git_checkout_tag`). + """ # See ``git_checkout_tag``: resolve before the ``os.chdir`` into ``repo_path``. - git_bin = resolve_required_binary("git") + try: + git_bin = resolve_required_binary("git") + except BinaryNotFoundError as e: + _print_pr_checkout_failure(pr_info, str(e)) + return False original_dir = os.getcwd() try: @@ -155,25 +194,34 @@ def checkout_pr(repo_path: str, pr_info: PRInfo) -> bool: return True except subprocess.CalledProcessError as e: - error_message = Text() - error_message.append("Git PR Checkout Error", style="bold red on white") - error_message.append(f"\n\nFailed to checkout PR #{pr_info.number}", style="bold yellow") - error_message.append(f"\nTitle: {pr_info.title}", style="italic") - error_message.append(f"\nBranch: {pr_info.head_branch}", style="italic") - - if e.stderr: - error_message.append("\n\nError output:", style="bold red") - error_message.append(f"\n{e.stderr}", style="italic yellow") - - console.print( - Panel( - error_message, - title="[bold white on red]PR Checkout Failed[/bold white on red]", - border_style="red", - expand=False, - ) - ) + _print_pr_checkout_failure(pr_info, stderr=e.stderr) return False finally: os.chdir(original_dir) + + +def _print_pr_checkout_failure(pr_info: PRInfo, detail: str | None = None, stderr: str | None = None) -> None: + """Render the "PR Checkout Failed" panel for a PR checkout that didn't happen.""" + error_message = Text() + error_message.append("Git PR Checkout Error", style="bold red on white") + error_message.append(f"\n\nFailed to checkout PR #{pr_info.number}", style="bold yellow") + error_message.append(f"\nTitle: {pr_info.title}", style="italic") + error_message.append(f"\nBranch: {pr_info.head_branch}", style="italic") + + if detail: + error_message.append("\n\nError details:", style="bold red") + error_message.append(f"\n{detail}", style="italic") + + if stderr: + error_message.append("\n\nError output:", style="bold red") + error_message.append(f"\n{stderr}", style="italic yellow") + + console.print( + Panel( + error_message, + title="[bold white on red]PR Checkout Failed[/bold white on red]", + border_style="red", + expand=False, + ) + ) diff --git a/comfy_cli/registry/config_parser.py b/comfy_cli/registry/config_parser.py index bacf36cd..1aa4fcb0 100644 --- a/comfy_cli/registry/config_parser.py +++ b/comfy_cli/registry/config_parser.py @@ -246,8 +246,12 @@ def initialize_project_config(): git_bin = resolve_required_binary("git") git_remote_url = subprocess.check_output([git_bin, "remote", "get-url", "origin"]).decode().strip() git_remote_url = _strip_url_credentials(git_remote_url) - except subprocess.CalledProcessError as e: - raise Exception("Could not retrieve Git remote URL. Are you in a Git repository?") from e + except (subprocess.CalledProcessError, OSError) as e: + # ``OSError`` also covers ``BinaryNotFoundError`` — git absent, or found + # and refused. Without it that escapes as a traceback *after* + # ``create_comfynode_config()`` above has already written pyproject.toml, + # leaving a half-initialized project behind. + raise Exception(f"Could not retrieve Git remote URL. Are you in a Git repository? ({e})") from e # Convert SSH URL to HTTPS if needed if git_remote_url.startswith("git@github.com:"): diff --git a/tests/comfy_cli/command/test_preview.py b/tests/comfy_cli/command/test_preview.py index aec022b3..b841c7cd 100644 --- a/tests/comfy_cli/command/test_preview.py +++ b/tests/comfy_cli/command/test_preview.py @@ -53,7 +53,11 @@ def test_classify_audio(): # --- pure: build the ffmpeg command ---------------------------------------- -_FFMPEG = os.path.join(os.sep, "usr", "bin", "ffmpeg") +# ``os.path.join(os.sep, ...)`` yields the drive-less ``\usr\bin\`` on Windows, +# which ``_safe_exec._is_fully_qualified`` rejects — pin a drive-qualified path there +# so these tests exercise the resolver instead of a ``BinaryNotFoundError`` on the very +# platform the hardening targets. +_FFMPEG = r"C:\tools\bin\ffmpeg" if os.name == "nt" else os.path.join(os.sep, "usr", "bin", "ffmpeg") def test_build_cmd_video_is_contact_sheet(): diff --git a/tests/comfy_cli/command/test_update_version.py b/tests/comfy_cli/command/test_update_version.py index 981fc00d..829a38e9 100644 --- a/tests/comfy_cli/command/test_update_version.py +++ b/tests/comfy_cli/command/test_update_version.py @@ -24,7 +24,11 @@ from comfy_cli import _safe_exec, cmdline from comfy_cli.command import install as install_inner -_FAKE_GIT_BIN = os.path.join(os.sep, "usr", "bin", "git") +# ``os.path.join(os.sep, ...)`` yields the drive-less ``\usr\bin\`` on Windows, +# which ``_safe_exec._is_fully_qualified`` rejects — pin a drive-qualified path there +# so these tests exercise the resolver instead of a ``BinaryNotFoundError`` on the very +# platform the hardening targets. +_FAKE_GIT_BIN = r"C:\tools\bin\git" if os.name == "nt" else os.path.join(os.sep, "usr", "bin", "git") def _is_git(argv: list[str]) -> bool: diff --git a/tests/comfy_cli/test_safe_exec.py b/tests/comfy_cli/test_safe_exec.py index c2633b6c..f8450557 100644 --- a/tests/comfy_cli/test_safe_exec.py +++ b/tests/comfy_cli/test_safe_exec.py @@ -228,8 +228,11 @@ def test_returns_the_resolved_path(self, tmp_path): def test_raises_when_the_binary_is_absent(self): with patch.object(_safe_exec.shutil, "which", return_value=None): - with pytest.raises(_safe_exec.BinaryNotFoundError, match="git"): + with pytest.raises(_safe_exec.BinaryNotFoundError, match="not found on PATH") as exc_info: _safe_exec.resolve_required_binary("git") + assert exc_info.value.reason is _safe_exec.BinaryRefusal.ABSENT + assert exc_info.value.is_absent is True + assert exc_info.value.binary == "git" def test_raises_for_a_binary_planted_in_the_cwd(self, tmp_path): planted = tmp_path / "git" @@ -238,8 +241,44 @@ def test_raises_for_a_binary_planted_in_the_cwd(self, tmp_path): patch.object(_safe_exec.os, "getcwd", return_value=str(tmp_path)), patch.object(_safe_exec.shutil, "which", return_value=str(planted)), ): - with pytest.raises(_safe_exec.BinaryNotFoundError, match="current directory"): + with pytest.raises(_safe_exec.BinaryNotFoundError, match="refusing to run") as exc_info: _safe_exec.resolve_required_binary("git") + assert exc_info.value.reason is _safe_exec.BinaryRefusal.CWD_ANCHORED + assert exc_info.value.candidate == str(planted) + + def test_a_refusal_is_not_reported_as_an_absent_binary(self, tmp_path): + """The distinction the diagnostics hang on: a refused binary is present. + + Telling a user with a working ``git`` to go install ``git`` sends them + the wrong way and hides the interesting part — something named ``git`` + was found sitting in the directory they ran from. + """ + planted = tmp_path / "git" + planted.write_text("") + with ( + patch.object(_safe_exec.os, "getcwd", return_value=str(tmp_path)), + patch.object(_safe_exec.shutil, "which", return_value=str(planted)), + ): + with pytest.raises(_safe_exec.BinaryNotFoundError) as exc_info: + _safe_exec.resolve_required_binary("git") + assert exc_info.value.is_absent is False + assert "install" not in str(exc_info.value).lower().split("make sure")[0] + assert str(planted) in str(exc_info.value) + + def test_an_unplaceable_match_is_reported_as_unverifiable(self, tmp_path): + """A deleted/unreadable CWD is refused too, but it is not evidence of a + plant, so it gets its own reason rather than borrowing the plant's.""" + legit = tmp_path / "System32" / "git" + legit.parent.mkdir() + legit.write_text("") + with ( + patch.object(_safe_exec.os, "getcwd", side_effect=OSError("cwd deleted")), + patch.object(_safe_exec.shutil, "which", return_value=str(legit)), + ): + with pytest.raises(_safe_exec.BinaryNotFoundError) as exc_info: + _safe_exec.resolve_required_binary("git") + assert exc_info.value.reason is _safe_exec.BinaryRefusal.UNVERIFIABLE + assert exc_info.value.is_absent is False def test_raises_for_a_non_bare_name(self): """The gates are shared with ``resolve_binary`` — a name carrying a path diff --git a/tests/comfy_cli/test_safe_exec_call_sites.py b/tests/comfy_cli/test_safe_exec_call_sites.py index c9f62fda..00f96377 100644 --- a/tests/comfy_cli/test_safe_exec_call_sites.py +++ b/tests/comfy_cli/test_safe_exec_call_sites.py @@ -10,6 +10,12 @@ fails loudly (required binaries) or degrades exactly as it already did when the binary was simply absent (tolerant sites and the best-effort previewer). +"Degrades as if absent" has one deliberate exception, covered below: +:func:`comfy_cli.file_utils.list_git_tracked_files`. There ``[]`` means "not a +git repository" and makes :func:`~comfy_cli.file_utils.zip_files` package the +whole directory, so a *refused* git raises rather than silently widening the +archive ``comfy node publish`` uploads. + The sites that ``os.chdir`` into a user-supplied directory before spawning get an explicit test each, because there the attacker-controlled directory *is* the CWD by construction rather than by the user happening to be standing in it. @@ -152,7 +158,12 @@ def test_checkout_pr_spawns_resolved_path_not_the_repo_plant(self, tmp_path, sys assert str(planted) not in recorder.argv0s def test_checkout_tag_refuses_a_git_planted_in_the_cwd(self, tmp_path, system_bin, monkeypatch): - """The remaining shape: the *process* CWD is the attacker's directory.""" + """The remaining shape: the *process* CWD is the attacker's directory. + + ``git_checkout_tag`` is documented to return ``False`` on failure and both + callers rely on that for a clean CLI error, so a refusal has to come back + as ``False`` rather than escaping as a traceback past them. + """ _plant(system_bin, "git") attacker_cwd = tmp_path / "attacker" planted = _plant(attacker_cwd, "git") @@ -164,9 +175,8 @@ def test_checkout_tag_refuses_a_git_planted_in_the_cwd(self, tmp_path, system_bi with ( patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), patch.object(git_utils.subprocess, "run", recorder), - pytest.raises(_safe_exec.BinaryNotFoundError), ): - git_utils.git_checkout_tag(str(repo), "v1.2.3") + assert git_utils.git_checkout_tag(str(repo), "v1.2.3") is False assert recorder.calls == [], f"planted {planted} must never be executed" @@ -175,12 +185,42 @@ def test_checkout_tag_restores_cwd_when_git_is_unresolvable(self, tmp_path, syst process inside the repo directory.""" repo = tmp_path / "repo" repo.mkdir() + with patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)): + assert git_utils.git_checkout_tag(str(repo), "v1.2.3") is False + assert Path(os.getcwd()).resolve() == neutral_cwd.resolve() + + def test_checkout_pr_returns_false_when_git_is_unresolvable(self, tmp_path, system_bin, neutral_cwd): + """Same contract for the PR checkout path.""" + repo = tmp_path / "repo" + repo.mkdir() + pr_info = PRInfo( + number=42, + head_repo_url="https://github.com/comfy/comfy.git", + head_branch="feature/x", + base_repo_url="https://github.com/comfy/comfy.git", + base_branch="main", + title="t", + user="u", + mergeable=True, + ) + with patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)): + assert git_utils.checkout_pr(str(repo), pr_info) is False + assert Path(os.getcwd()).resolve() == neutral_cwd.resolve() + + def test_checkout_tag_refuses_an_option_like_tag(self, tmp_path, system_bin, neutral_cwd): + """``git checkout `` has no end-of-options escape, so a tag that git + would parse as an option (``--upload-pack=``) is rejected outright.""" + _plant(system_bin, "git") + repo = tmp_path / "repo" + repo.mkdir() + + recorder = _Recorder() with ( patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), - pytest.raises(_safe_exec.BinaryNotFoundError), + patch.object(git_utils.subprocess, "run", recorder), ): - git_utils.git_checkout_tag(str(repo), "v1.2.3") - assert Path(os.getcwd()).resolve() == neutral_cwd.resolve() + assert git_utils.git_checkout_tag(str(repo), "--upload-pack=touch /tmp/pwned") is False + assert recorder.calls == [] # --- tolerant git sites: degrade exactly as a missing git already did ------- @@ -207,7 +247,25 @@ def fake_check_output(argv, **_kwargs): assert recorded[0][0] == str(legit) - def test_list_git_tracked_files_returns_empty_for_cwd_planted_git(self, tmp_path, system_bin, monkeypatch): + def test_list_git_tracked_files_returns_empty_when_git_is_absent(self, tmp_path, neutral_cwd): + """An *absent* git still degrades to ``[]`` — that has always meant "no + git answer", and ``zip_files`` reads it as "not a git repository".""" + recorder = _Recorder() + with ( + patch.object(_safe_exec.shutil, "which", lambda *_a, **_k: None), + patch.object(file_utils.subprocess, "check_output", recorder), + ): + assert file_utils.list_git_tracked_files(str(tmp_path)) == [] + assert recorder.calls == [] + + def test_list_git_tracked_files_raises_for_cwd_planted_git(self, tmp_path, system_bin, monkeypatch): + """A *refused* git must not be flattened into the same ``[]``. + + ``zip_files`` treats ``[]`` as "not a git repository" and falls back to + walking the whole directory — which would sweep untracked and gitignored + files (``.env``, keys, venvs) into the archive ``comfy node publish`` + uploads. Refusing loudly is the only safe answer here. + """ _plant(system_bin, "git") attacker_cwd = tmp_path / "attacker" _plant(attacker_cwd, "git") @@ -217,10 +275,25 @@ def test_list_git_tracked_files_returns_empty_for_cwd_planted_git(self, tmp_path with ( patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), patch.object(file_utils.subprocess, "check_output", recorder), + pytest.raises(_safe_exec.BinaryNotFoundError), ): - assert file_utils.list_git_tracked_files(str(tmp_path)) == [] + file_utils.list_git_tracked_files(str(tmp_path)) assert recorder.calls == [] + def test_zip_files_does_not_walk_everything_when_git_is_refused(self, tmp_path, system_bin, monkeypatch): + """End-to-end shape of the above: the publish archive is never widened.""" + _plant(system_bin, "git") + attacker_cwd = tmp_path / "attacker" + _plant(attacker_cwd, "git") + (attacker_cwd / ".env").write_text("SECRET=hunter2") + monkeypatch.chdir(attacker_cwd) + + with ( + patch.object(_safe_exec.shutil, "which", _cwd_first_which(system_bin)), + pytest.raises(_safe_exec.BinaryNotFoundError), + ): + file_utils.zip_files(str(tmp_path / "node.zip")) + def test_outdated_git_output_uses_resolved_path(self, tmp_path, system_bin, neutral_cwd): legit = _plant(system_bin, "git") recorder = _Recorder(stdout="true\n")