mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(terminal): fall back when the configured cwd is unenterable, not just missing (#66306)
A root-launched CLI session can leak /root into the terminal cwd state a non-root gateway/cron process later resolves (#65583). os.path.isdir('/root') is True for a non-root user — stat only needs search permission on / — so _resolve_safe_cwd returned it and subprocess.Popen(cwd='/root') died with PermissionError: [Errno 13], failing EVERY cron job's terminal/file/search tool on every command until restart. _resolve_safe_cwd now requires X_OK (new _cwd_usable helper) and climbs to the nearest enterable ancestor, logging a WARNING that names the leak class when an existing-but-denied cwd is skipped. Missing-cwd recovery (#17558) behavior unchanged. E2E-verified: LocalEnvironment constructed with an unenterable cwd now runs commands from the fallback directory instead of raising.
This commit is contained in:
parent
c49ed09336
commit
71252f0dcb
2 changed files with 119 additions and 9 deletions
83
tests/tools/test_local_cwd_permission_fallback.py
Normal file
83
tests/tools/test_local_cwd_permission_fallback.py
Normal file
|
|
@ -0,0 +1,83 @@
|
|||
"""Regression tests for inaccessible-cwd fallback (#65583).
|
||||
|
||||
``/root`` leaking into a non-root gateway/cron process's terminal cwd used to
|
||||
kill every command with ``PermissionError: [Errno 13] Permission denied:
|
||||
'/root'`` — ``os.path.isdir('/root')`` is True for a non-root user (stat only
|
||||
needs search permission on ``/``), so the old existence-only check in
|
||||
``_resolve_safe_cwd`` happily returned a directory ``subprocess.Popen`` could
|
||||
not enter. The fix checks X_OK and falls back to the nearest usable ancestor.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
|
||||
import pytest
|
||||
|
||||
from tools.environments.local import LocalEnvironment, _cwd_usable, _resolve_safe_cwd
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def denied_dir(tmp_path):
|
||||
"""A directory that exists but cannot be entered (simulates /root)."""
|
||||
d = tmp_path / "rootlike"
|
||||
d.mkdir()
|
||||
d.chmod(0)
|
||||
yield d
|
||||
d.chmod(0o755) # so pytest can clean up
|
||||
|
||||
|
||||
needs_posix_perms = pytest.mark.skipif(
|
||||
sys.platform == "win32" or os.geteuid() == 0,
|
||||
reason="chmod-based access denial needs POSIX + non-root",
|
||||
)
|
||||
|
||||
|
||||
@needs_posix_perms
|
||||
class TestInaccessibleCwdFallback:
|
||||
def test_cwd_usable_rejects_unenterable_directory(self, denied_dir):
|
||||
assert os.path.isdir(denied_dir) # the trap: stat succeeds
|
||||
assert _cwd_usable(str(denied_dir)) is False
|
||||
|
||||
def test_resolve_safe_cwd_falls_back_from_denied_dir(self, denied_dir, tmp_path):
|
||||
resolved = _resolve_safe_cwd(str(denied_dir))
|
||||
assert resolved != str(denied_dir)
|
||||
assert os.access(resolved, os.X_OK)
|
||||
# Nearest usable ancestor is the tmp_path parent, not a random tempdir.
|
||||
assert resolved == str(tmp_path)
|
||||
|
||||
def test_resolve_safe_cwd_climbs_past_denied_ancestor(self, denied_dir, tmp_path):
|
||||
missing_child = str(denied_dir / "sub" / "dir")
|
||||
resolved = _resolve_safe_cwd(missing_child)
|
||||
assert os.access(resolved, os.X_OK)
|
||||
assert resolved == str(tmp_path)
|
||||
|
||||
def test_local_environment_survives_denied_cwd(self, denied_dir):
|
||||
"""The #65583 shape: env constructed with an unenterable cwd must
|
||||
still execute commands instead of raising PermissionError."""
|
||||
env = LocalEnvironment(cwd=str(denied_dir), timeout=30)
|
||||
try:
|
||||
handle = env._run_bash("echo ALIVE")
|
||||
out = handle.stdout.read() if handle.stdout else b""
|
||||
rc = handle.wait(timeout=15)
|
||||
assert rc == 0
|
||||
assert b"ALIVE" in (out if isinstance(out, bytes) else out.encode())
|
||||
finally:
|
||||
try:
|
||||
env.cleanup()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
class TestUsableCwdBehaviorUnchanged:
|
||||
def test_existing_accessible_cwd_returned_verbatim(self, tmp_path):
|
||||
assert _resolve_safe_cwd(str(tmp_path)) == str(tmp_path)
|
||||
|
||||
def test_missing_cwd_still_climbs_to_existing_ancestor(self, tmp_path):
|
||||
missing = str(tmp_path / "gone" / "deeper")
|
||||
assert _resolve_safe_cwd(missing) == str(tmp_path)
|
||||
|
||||
def test_hopeless_path_falls_back_to_tempdir(self):
|
||||
# A path whose every component is missing outside any real tree.
|
||||
resolved = _resolve_safe_cwd("")
|
||||
assert resolved == tempfile.gettempdir()
|
||||
|
|
@ -136,11 +136,27 @@ def _quote_bash_path(path: str) -> str:
|
|||
return shlex.quote(_bash_safe_path(path))
|
||||
|
||||
|
||||
def _cwd_usable(path: str) -> bool:
|
||||
"""True when *path* is a directory this process can actually chdir into.
|
||||
|
||||
``os.path.isdir`` alone is not enough: stat() on ``/root`` succeeds for a
|
||||
non-root user (only ``/`` needs search permission), but
|
||||
``subprocess.Popen(cwd='/root')`` then dies with ``PermissionError:
|
||||
[Errno 13] Permission denied: '/root'``. Seen in the wild when a
|
||||
root-launched CLI session leaks ``/root`` into shared state that a
|
||||
non-root gateway/cron process later reads (#65583) — every cron job's
|
||||
terminal/file tool then fails on every command, forever. Checking
|
||||
X_OK up front lets the caller fall back instead.
|
||||
"""
|
||||
return os.path.isdir(path) and os.access(path, os.X_OK)
|
||||
|
||||
|
||||
def _resolve_safe_cwd(cwd: str) -> str:
|
||||
"""Return ``cwd`` if it exists as a directory, else the nearest existing
|
||||
ancestor. Falls back to ``tempfile.gettempdir()`` only if walking up the
|
||||
path can't find any existing directory (effectively never on a healthy
|
||||
filesystem, but cheap belt-and-braces).
|
||||
"""Return ``cwd`` if it exists as a directory this process can enter,
|
||||
else the nearest existing accessible ancestor. Falls back to
|
||||
``tempfile.gettempdir()`` only if walking up the path can't find any
|
||||
usable directory (effectively never on a healthy filesystem, but cheap
|
||||
belt-and-braces).
|
||||
|
||||
On Windows, also normalizes Git Bash / MSYS-style POSIX paths
|
||||
(``/c/Users/x``) to native Windows form before the isdir check so a
|
||||
|
|
@ -149,16 +165,27 @@ def _resolve_safe_cwd(cwd: str) -> str:
|
|||
|
||||
Used by ``_run_bash`` to recover when the configured cwd is gone — most
|
||||
commonly because a previous tool call deleted its own working directory
|
||||
(issue #17558). Without this guard, ``subprocess.Popen(..., cwd=...)``
|
||||
raises ``FileNotFoundError`` before bash starts, wedging every subsequent
|
||||
terminal call until the gateway restarts.
|
||||
(issue #17558) — or inaccessible to this user, e.g. ``/root`` leaking
|
||||
from a root-launched CLI session into a non-root gateway's cron jobs
|
||||
(issue #65583). Without this guard, ``subprocess.Popen(..., cwd=...)``
|
||||
raises ``FileNotFoundError``/``PermissionError`` before bash starts,
|
||||
wedging every subsequent terminal call until the gateway restarts.
|
||||
"""
|
||||
cwd = _msys_to_windows_path(cwd) if _IS_WINDOWS else cwd
|
||||
if cwd and os.path.isdir(cwd):
|
||||
if cwd and _cwd_usable(cwd):
|
||||
return cwd
|
||||
if cwd and os.path.isdir(cwd):
|
||||
logger.warning(
|
||||
"Configured terminal cwd %r exists but is not accessible to "
|
||||
"this user (uid=%s) — falling back to the nearest usable "
|
||||
"directory. If this is a gateway/cron process, check for "
|
||||
"root-owned paths leaking into terminal.cwd / TERMINAL_CWD "
|
||||
"(#65583).",
|
||||
cwd, getattr(os, "getuid", lambda: "?")(),
|
||||
)
|
||||
parent = os.path.dirname(cwd) if cwd else ""
|
||||
while parent:
|
||||
if os.path.isdir(parent):
|
||||
if _cwd_usable(parent):
|
||||
return parent
|
||||
next_parent = os.path.dirname(parent)
|
||||
if next_parent == parent:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue