diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index dc5da42d47..712d37ec66 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -7237,6 +7237,121 @@ def _rebuild_desktop_after_update( return True +def _path_uid(path) -> Optional[int]: + """Owner uid of ``path`` via ``os.stat`` — ``None`` when unreadable. + + Separate seam so tests can simulate root-owned files without chown + (which needs root). Never raises. + """ + try: + return os.stat(path, follow_symlinks=False).st_uid + except OSError: + return None + + +def _venv_foreign_owned_paths(venv_root, limit: int = 5) -> list: + """Bounded scan for venv entries not owned by the current user (#83529). + + A venv that was ever touched by ``sudo pip`` / ``sudo hermes`` contains + root-owned files (classically ``*.dist-info/INSTALLER``). A later normal + ``hermes update`` then dies mid-mutation inside ``uv pip install -e .`` + ("Permission denied (os error 13)") with ``venv/bin/hermes`` already + deleted — the CLI is bricked. Same philosophy as the contended-venv gate + (#87331): a venv we cannot safely mutate is never mutated at all. + + Checks a deliberately BOUNDED set (no full recursion — must never be + slow): the venv root, each direct entry of ``venv/bin``, the top-level + entries of the first ``lib/python*/site-packages`` found, and the direct + children of each ``*.dist-info`` there. Caps stat calls at ~2000 and + returned paths at ``limit``. POSIX-only: returns ``[]`` on Windows + (no ``os.geteuid``) and when running as root. Swallows every per-entry + ``OSError`` and returns ``[]`` on any structural surprise — this helper + must NEVER raise and must never add noticeable latency to update. + + Returns a list of ``(path_str, uid)`` tuples, at most ``limit`` long. + """ + try: + if not hasattr(os, "geteuid"): + return [] # windows-footgun: ok — POSIX ownership concept only + euid = os.geteuid() # windows-footgun: ok — guarded by hasattr above + if euid == 0: + return [] # root can rewrite anything; nothing to refuse + + venv_root = Path(venv_root) + budget = 2000 # max stat() calls — hard bound on preflight cost + foreign: list = [] + + def _check(p) -> bool: + """stat one path; True while scan should continue.""" + nonlocal budget + if budget <= 0 or len(foreign) >= limit: + return False + budget -= 1 + uid = _path_uid(p) + if uid is not None and uid != euid: + foreign.append((str(p), uid)) + return budget > 0 and len(foreign) < limit + + def _scan_dir(d, recurse_dist_info: bool = False) -> None: + try: + entries = list(os.scandir(d)) + except OSError: + return + for entry in entries: + if not _check(entry.path): + return + if recurse_dist_info and entry.name.endswith(".dist-info"): + try: + children = list(os.scandir(entry.path)) + except OSError: + continue + for child in children: + if not _check(child.path): + return + + if not _check(venv_root): + return foreign[:limit] + _scan_dir(venv_root / "bin") + + # First lib/python*/site-packages (POSIX venv layout). + site_packages = next( + iter(sorted(venv_root.glob("lib/python*/site-packages"))), None + ) + if site_packages is not None: + _scan_dir(site_packages, recurse_dist_info=True) + + return foreign[:limit] + except Exception: + # Preflight is advisory: any structural surprise means "no verdict", + # never a crashed or blocked update. + return [] + + +def _refuse_update_if_venv_foreign_owned(project_root) -> None: + """Refuse-before-mutate ownership gate for the dependency install (#83529). + + Runs after the code pull (pulling code is safe) and immediately before + the first venv mutation. If the venv contains files owned by another + uid, the ``uv pip install -e .`` below would die mid-mutation and brick + the install — so refuse up front, with the exact recovery command, + while the venv is still fully intact. No subprocess calls here: update + tests mock ``subprocess.run`` with sequenced side effects. + """ + foreign = _venv_foreign_owned_paths(Path(project_root) / "venv") + if not foreign: + return + print("\n✗ Update stopped: this install's venv contains files owned by another user.") + print(" Updating now would fail midway (Permission denied) and leave Hermes broken.") + print(" This usually happens after running hermes or pip with sudo. Offending paths:") + for p, uid in foreign: + print(f" - {p} (owner uid {uid})") + print("\n Fix ownership, then re-run the update:") + print(f" sudo chown -R $(id -un): {project_root}") + print(" hermes update") + print("\n Nothing in the venv was modified.") + sys.exit(1) + + def _cmd_update_impl(args, gateway_mode: bool): """Body of ``cmd_update`` — kept separate so the wrapper can always restore stdio even on ``sys.exit``.""" @@ -8330,6 +8445,11 @@ def _cmd_update_impl(args, gateway_mode: bool): # breaks on this machine, keep base deps and reinstall the remaining extras # individually so update does not silently strip working capabilities. # + # Ownership preflight (#83529): refuse before the first venv mutation + # if the venv contains foreign-owned files (sudo-pip residue) — the + # install below would die mid-mutation and brick the CLI. + _refuse_update_if_venv_foreign_owned(_m().PROJECT_ROOT) + # # Self-lock deferral (relocated preflight — #86735): if THIS process # holds a native extension the sync must rewrite, defer NOW — after # the code swap, so only the dependency install is pending and the diff --git a/tests/hermes_cli/test_update_venv_ownership_preflight.py b/tests/hermes_cli/test_update_venv_ownership_preflight.py new file mode 100644 index 0000000000..56d9c67fcd --- /dev/null +++ b/tests/hermes_cli/test_update_venv_ownership_preflight.py @@ -0,0 +1,137 @@ +"""Venv ownership preflight for ``hermes update`` (#83529). + +A venv touched by ``sudo pip`` / ``sudo hermes`` contains root-owned files +(e.g. ``site-packages/hermes_agent-*.dist-info/INSTALLER``). A later normal +``hermes update`` then dies mid-mutation inside ``uv pip install -e .`` +("Permission denied (os error 13)") with ``venv/bin/hermes`` already deleted, +bricking the CLI. The preflight refuses BEFORE the first venv mutation and +prints the exact chown recovery command. + +The helper must be pure ``os.stat``/``os.scandir`` — no subprocess calls — +because update-path tests mock ``subprocess.run`` with sequenced side effects. +""" + +import os + +import pytest + +from hermes_cli import update_cmd + + +def _make_fake_venv(tmp_path): + """Minimal POSIX venv layout with a dist-info directory.""" + venv = tmp_path / "venv" + (venv / "bin").mkdir(parents=True) + (venv / "bin" / "hermes").write_text("#!stub\n") + (venv / "bin" / "python").write_text("#!stub\n") + sp = venv / "lib" / "python3.12" / "site-packages" + dist_info = sp / "hermes_agent-1.0.0.dist-info" + dist_info.mkdir(parents=True) + (dist_info / "INSTALLER").write_text("pip\n") + (dist_info / "RECORD").write_text("\n") + (sp / "some_pkg").mkdir() + return venv + + +def test_all_owned_returns_empty(tmp_path): + venv = _make_fake_venv(tmp_path) + assert update_cmd._venv_foreign_owned_paths(venv) == [] + + +def test_all_owned_preflight_proceeds(tmp_path, monkeypatch, capsys): + """Gate is a no-op (no exit, no output) when everything is user-owned.""" + _make_fake_venv(tmp_path) + update_cmd._refuse_update_if_venv_foreign_owned(tmp_path) + assert capsys.readouterr().out == "" + + +def test_foreign_owned_dist_info_child_detected(tmp_path, monkeypatch): + venv = _make_fake_venv(tmp_path) + installer = str( + venv / "lib" / "python3.12" / "site-packages" + / "hermes_agent-1.0.0.dist-info" / "INSTALLER" + ) + real_uid = update_cmd._path_uid + + def fake_uid(path): + if str(path) == installer: + return 0 # simulate root-owned sudo-pip residue + return real_uid(path) + + monkeypatch.setattr(update_cmd, "_path_uid", fake_uid) + foreign = update_cmd._venv_foreign_owned_paths(venv) + assert foreign == [(installer, 0)] + + +def test_foreign_owned_refuses_with_chown_hint(tmp_path, monkeypatch, capsys): + venv = _make_fake_venv(tmp_path) + hermes_bin = str(venv / "bin" / "hermes") + real_uid = update_cmd._path_uid + monkeypatch.setattr( + update_cmd, + "_path_uid", + lambda p: 0 if str(p) == hermes_bin else real_uid(p), + ) + with pytest.raises(SystemExit) as exc: + update_cmd._refuse_update_if_venv_foreign_owned(tmp_path) + assert exc.value.code == 1 + out = capsys.readouterr().out + assert hermes_bin in out + assert "owner uid 0" in out + assert f"sudo chown -R $(id -un): {tmp_path}" in out + assert "Nothing in the venv was modified." in out + + +def test_limit_caps_reported_paths(tmp_path, monkeypatch): + venv = _make_fake_venv(tmp_path) + bin_dir = venv / "bin" + for i in range(10): + (bin_dir / f"tool{i}").write_text("x") + monkeypatch.setattr( + update_cmd, + "_path_uid", + lambda p: 0 if str(p).startswith(str(bin_dir) + os.sep) else 12345, + ) + monkeypatch.setattr(update_cmd.os, "geteuid", lambda: 12345, raising=False) + foreign = update_cmd._venv_foreign_owned_paths(venv, limit=3) + assert len(foreign) == 3 + + +def test_no_geteuid_returns_empty(tmp_path, monkeypatch): + """Windows (no os.geteuid) skips the preflight entirely.""" + venv = _make_fake_venv(tmp_path) + + class _NoGeteuidOS: + def __getattr__(self, name): + if name == "geteuid": + raise AttributeError(name) + return getattr(os, name) + + monkeypatch.setattr(update_cmd, "os", _NoGeteuidOS()) + assert update_cmd._venv_foreign_owned_paths(venv) == [] + + +def test_running_as_root_returns_empty(tmp_path, monkeypatch): + venv = _make_fake_venv(tmp_path) + monkeypatch.setattr(update_cmd.os, "geteuid", lambda: 0, raising=False) + # Even with foreign uids everywhere, root skips the gate. + monkeypatch.setattr(update_cmd, "_path_uid", lambda p: 4242) + assert update_cmd._venv_foreign_owned_paths(venv) == [] + + +def test_never_raises_on_scandir_permission_error(tmp_path, monkeypatch): + venv = _make_fake_venv(tmp_path) + + def boom(path): + raise PermissionError(13, "Permission denied", str(path)) + + monkeypatch.setattr(update_cmd.os, "scandir", boom) + assert update_cmd._venv_foreign_owned_paths(venv) == [] + + +def test_never_raises_on_missing_venv(tmp_path): + assert update_cmd._venv_foreign_owned_paths(tmp_path / "no-venv") == [] + + +def test_path_uid_none_on_oserror(tmp_path): + assert update_cmd._path_uid(tmp_path / "does-not-exist") is None