Terminal tool handling of inaccessible or unenterable working directory

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #66306 fix(terminal): fall back when the configured cwd is unenterable, not just missing (#65583)

  merged to main

  files: tests/tools/test_local_cwd_permission_fallback.py, tools/environments/local.py

  Show the diff

  • tests/tools/test_local_cwd_permission_fallback.py

    @@ -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
    +
    +
    [redacted:email]
    +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()

  • tools/environments/local.py

    @@ -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: