fix(update): refuse to mutate a venv containing foreign-owned files (#83529)
A venv ever touched by sudo pip / sudo hermes contains root-owned files (classically site-packages/*.dist-info/INSTALLER). A later normal-user 'hermes update' pulls code fine, then 'uv pip install -e .' dies with 'Permission denied (os error 13)' mid-mutation — venv/bin/hermes already deleted, CLI bricked. Add a bounded, pure-stat ownership preflight (_venv_foreign_owned_paths) that runs after the code pull and immediately before the dependency install. If foreign-owned paths are found it refuses up front, names the offending paths + owner uid, prints the exact recovery command (sudo chown -R $(id -un): <root>), and confirms the venv is untouched. Windows (no os.geteuid) and root skip entirely. Never raises, capped at ~2000 stat calls, no subprocess use (update tests mock subprocess.run). Same refuse-before-mutate philosophy as the contended-venv gate (#87331). Fixes #83529 Diagnosis and documented recovery by @eabase.
This commit is contained in:
Teknium
@@ -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
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user