From 641c49f8412e0d7ea0e9435ee7cd756b4af3e455 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:19:22 +0530 Subject: [PATCH] fix(tools): resolve ssh paths against the remote home, keep guards intact Follow-up to the ssh path fix salvaged from #121686. - Read the ssh anchor raw (session record, override, TERMINAL_CWD): the shared workspace-root helper expands ~ on the Hermes host, so TERMINAL_CWD='~/proj' still resolved into the container home. - Resolve ~ to the remote home the SSH environment detects at connect, bringing the environment up through the file tools' own creator (_get_file_ops, same cwd and cache) when none is live. A failed bring-up is remembered per container for 30s so one call's several resolutions don't each retry; a live environment is always used first. SSHEnvironment now records whether the home was detected, and a guessed /home/ (echo $HOME failed) is not used. Results are absolute and stable from the first call (read tracking and staleness checks key on them), and '..' normalizes to the real target: relative traversal like ../../../etc/x from ~ was refused on main and slipped past the sensitive-path guard on the PR head. - If the remote home cannot be detected, an ssh ~-path that climbs above ~ cannot be classified; the write guard refuses it. - ~user passes through for the remote shell instead of becoming ~/~user. - coerce_ssh_remote_cwd maps paths under the host subprocess home onto ~/, except when that home is the OS user's real home. - The outside-workspace warning compares in the remote namespace (it fired on every correct relative write when the anchor was ~). - The backend type is looked up once per resolution again (the PR head did three per local path). --- tools/environments/ssh.py | 2 + tools/file_tools_paths.py | 128 +++++++++++++++++++++---------- tools/file_tools_write_guards.py | 7 +- tools/terminal_tool_config.py | 32 ++++---- 4 files changed, 115 insertions(+), 54 deletions(-) diff --git a/tools/environments/ssh.py b/tools/environments/ssh.py index ff021a0837..a79dafb937 100644 --- a/tools/environments/ssh.py +++ b/tools/environments/ssh.py @@ -76,6 +76,7 @@ class SSHEnvironment(BaseEnvironment): if probe_only: self._sync_manager = None return + self._remote_home_detected = False self._remote_home = self._detect_remote_home() self._ensure_remote_dirs() self._sync_manager = FileSyncManager( @@ -153,6 +154,7 @@ class SSHEnvironment(BaseEnvironment): result = self._run_ssh("echo $HOME", timeout=10) if result.returncode == 0 and result.stdout.strip(): logger.debug("SSH: remote home = %s", result.stdout.strip()) + self._remote_home_detected = True return result.stdout.strip() return "/root" if self.user == "root" else f"/home/{self.user}" diff --git a/tools/file_tools_paths.py b/tools/file_tools_paths.py index 8c4a2cb657..7b36680608 100644 --- a/tools/file_tools_paths.py +++ b/tools/file_tools_paths.py @@ -9,6 +9,7 @@ edits to the agent process cwd, e.g. the main repo during a worktree session). import os import posixpath import sys +import time from pathlib import Path, PurePosixPath # ``TERMINAL_CWD`` values that mean "not configured" ("." from a stale config; @@ -17,6 +18,9 @@ _TERMINAL_CWD_SENTINELS = frozenset({"", ".", "./", "auto", "cwd"}) _CONTAINER_PATH_BACKENDS_FALLBACK = frozenset({"docker", "singularity", "modal", "daytona", "vercel_sandbox"}) # Backend name inferred from the live environment's class name (first match wins). _ENV_CLASS_NAME_HINTS = ("local", "ssh", "docker", "singularity", "modal", "daytona") +# Container task id -> monotonic time of the last failed SSH bring-up in path resolution. +_SSH_HOME_RETRY_AFTER = 30.0 +_ssh_home_failed_at: dict[str, float] = {} def _expand_tilde(path: str) -> str: @@ -164,40 +168,81 @@ def _anchor(text: str, base, container_paths: bool) -> Path | PurePosixPath: def _ssh_remote_anchor(task_id: str) -> str: - """Working directory on the SSH target, never the Hermes host's home. + """Working directory on the SSH target, read RAW: ``_authoritative_workspace_root`` + expands ``~`` on the Hermes host, which names a directory the remote does not have. - A recorded ``~`` stays ``~``. A recorded copy of the container subprocess - home (the host path ``get_subprocess_home()`` returns) is the same idea - written out by mistake, so it collapses back to ``~`` for the remote shell. + Same precedence (session record, registered override, ``$TERMINAL_CWD``); a + value that is neither ``~``-prefixed nor POSIX-absolute (a Windows or relative + host path) is skipped. ``coerce_ssh_remote_cwd`` maps the host subprocess home back to ``~``. """ + from agent.runtime_cwd import scope_terminal_cwd + from tools.terminal_tool import get_session_cwd, resolve_task_overrides from tools.terminal_tool_config import coerce_ssh_remote_cwd - raw = _authoritative_workspace_root(task_id) or "~" - return coerce_ssh_remote_cwd(raw, "ssh") or "~" + for raw in (get_session_cwd(task_id), resolve_task_overrides(task_id).get("cwd"), scope_terminal_cwd()): + text = str(raw or "").strip() + if text.lower() not in _TERMINAL_CWD_SENTINELS and (text.startswith("~") or posixpath.isabs(text)): + return coerce_ssh_remote_cwd(text, "ssh") + return "~" + + +def _ssh_remote_home(task_id: str) -> str | None: + """The SSH user's home as detected at connect (``echo $HOME``), else None. + + Brings the environment up through the file tools' own creator (same cwd + and cache as the call that follows) when none is live: they resolve before + touching the backend, and a first call keyed ``~/x`` while later ones key + ``/home/u/x`` splits read tracking and staleness checks for one file. A + failed bring-up is remembered briefly so one tool call's several + resolutions don't each wait out the SSH connect timeout; the tool's own + backend call reports the error. + """ + from tools.file_tools import _get_file_ops + from tools.terminal_tool import _resolve_container_task_id + from tools.terminal_tool_lifecycle import get_active_env + + key = _resolve_container_task_id(task_id) + env = get_active_env(task_id) + if env is None: + if time.monotonic() - _ssh_home_failed_at.get(key, float("-inf")) < _SSH_HOME_RETRY_AFTER: + return None + try: + env = _get_file_ops(task_id).env + except Exception: # noqa: BLE001 — connect failure + _ssh_home_failed_at[key] = time.monotonic() + return None + _ssh_home_failed_at.pop(key, None) + # A guessed /home/ (``echo $HOME`` failed) must not stand in for the real home. + home = getattr(env, "_remote_home", None) if getattr(env, "_remote_home_detected", False) else None + return home if isinstance(home, str) and posixpath.isabs(home) else None def _resolve_ssh_path(filepath: str, task_id: str) -> PurePosixPath: - """Resolve *filepath* in the SSH target's namespace. + """Resolve *filepath* in the SSH target's namespace, never via the Hermes host + (``Path.resolve()`` and ``get_subprocess_home()`` both name host directories). - Do not ``Path.resolve()`` or expand ``~`` with ``get_subprocess_home()``: - both name directories on the Hermes host (``/opt/data/home`` in Docker), - which the remote ``cd`` then rejects. + ``~`` becomes the remote home once the live environment has detected it, so the + result is absolute and the write guards see the real target. Before that it stays + ``~``-prefixed for the remote shell; ``~user`` always does. """ text = str(filepath or "").strip() + if not text.startswith("~") and not posixpath.isabs(text): + text = posixpath.join(_ssh_remote_anchor(task_id), text) if text == "~" or text.startswith("~/"): - return PurePosixPath(text) - if posixpath.isabs(text): + home = _ssh_remote_home(task_id) + text = home + text[1:] if home else text + if not text.startswith("~"): return _normalize_without_host_deref(text) - anchor = _ssh_remote_anchor(task_id) - if anchor == "~": - joined = f"~/{text}" - elif anchor.startswith("~/"): - joined = f"{anchor.rstrip('/')}/{text}" - else: - joined = posixpath.normpath(posixpath.join(anchor, text)) - if joined == "~" or joined.startswith("~/"): - return PurePosixPath(joined) - return _normalize_without_host_deref(joined) + head, _, tail = text.partition("/") + tail = posixpath.normpath(tail) if tail else "." + return PurePosixPath(head if tail == "." else f"{head}/{tail}") + + +def _ssh_path_escapes_home(resolved: str) -> bool: + """True for a still-``~``-relative SSH path that climbs above ``~``: its absolute + target is unknown until the remote home is, so no path guard can classify it.""" + tail = resolved.partition("/")[2] if resolved.startswith("~") else "" + return tail == ".." or tail.startswith("../") def _resolve_base_dir( @@ -205,17 +250,14 @@ def _resolve_base_dir( """Return the ABSOLUTE base directory for resolving relative paths: ``_authoritative_workspace_root``, else the process cwd as a last resort. - SSH is the exception: the anchor is a remote path (often ``~``), and the - process cwd is the Hermes host. + SSH resolves in the remote namespace (callers passing *container_paths* have + already ruled SSH out). """ - if _terminal_env_type_for_task(task_id) == "ssh": - anchor = _ssh_remote_anchor(task_id) - if anchor == "~" or anchor.startswith("~/"): - return PurePosixPath(anchor) - return _normalize_without_host_deref(anchor) - root = _authoritative_workspace_root(task_id) if container_paths is None: + if _terminal_env_type_for_task(task_id) == "ssh": + return _resolve_ssh_path(".", task_id) container_paths = _uses_container_paths(task_id) + root = _authoritative_workspace_root(task_id) # A backend's relative cwd is anchored to the process cwd once, here. return _anchor(_host_text(root or os.getcwd(), container_paths), os.getcwd, container_paths) @@ -231,20 +273,26 @@ def _resolve_path_for_task(filepath: str, task_id: str = "default") -> Path | Pu -def _path_resolution_warning(filepath: str, resolved: Path, task_id: str = "default") -> str | None: +def _path_resolution_warning(filepath: str, resolved: Path | PurePosixPath, task_id: str = "default") -> str | None: """Warn when a RELATIVE path resolved OUTSIDE the task's workspace root (the edit is about to land in a different checkout than the terminal's cwd). - ``None`` for absolute paths, an unknown root, or a path under the root.""" + ``None`` for absolute paths, an unknown root, or a path under the root. + SSH compares in the remote namespace, as ``_resolve_path_for_task`` resolved it.""" try: - if Path(_expand_tilde(filepath)).is_absolute(): - return None - workspace_root = _authoritative_workspace_root(task_id) - if not workspace_root: - return None - if _uses_container_paths(task_id): - root = _normalize_without_host_deref(Path(_expand_tilde(workspace_root))) + if _terminal_env_type_for_task(task_id) == "ssh": + if filepath.startswith("~") or posixpath.isabs(filepath): + return None + root = _resolve_ssh_path(".", task_id) else: - root = Path(_expand_tilde(workspace_root)).resolve() + if Path(_expand_tilde(filepath)).is_absolute(): + return None + workspace_root = _authoritative_workspace_root(task_id) + if not workspace_root: + return None + if _uses_container_paths(task_id): + root = _normalize_without_host_deref(Path(_expand_tilde(workspace_root))) + else: + root = Path(_expand_tilde(workspace_root)).resolve() if resolved.is_relative_to(root): return None return ( diff --git a/tools/file_tools_write_guards.py b/tools/file_tools_write_guards.py index 1ec2d31a26..3b85096f87 100644 --- a/tools/file_tools_write_guards.py +++ b/tools/file_tools_write_guards.py @@ -22,7 +22,8 @@ from tools.binary_extensions import ( is_pdf_path, is_sqlite_sidecar, ) -from tools.file_tools_paths import _expand_tilde, _resolve_path_for_task +from tools.file_tools_paths import ( + _expand_tilde, _resolve_path_for_task, _ssh_path_escapes_home, _terminal_env_type_for_task) from tools.file_tools_read_tracking import _has_full_write_baseline, _read_mtime_drifted # Prefixes matched after realpath. macOS: /private/var mirrors /var — block the @@ -153,6 +154,10 @@ def _check_sensitive_path(filepath: str, task_id: str = "default") -> str | None if nt_err: return nt_err candidates = (_resolved_or_raw(filepath, task_id), os.path.normpath(_expand_tilde(filepath))) + if _ssh_path_escapes_home(candidates[0]) and _terminal_env_type_for_task(task_id) == "ssh": + return ( + f"Refusing to write to {filepath}: it climbs above the SSH home and the remote " + "home could not be detected, so its target cannot be checked. Pass an absolute path.") if any(c.startswith(_SENSITIVE_PATH_PREFIXES) or c in _SENSITIVE_EXACT_PATHS for c in candidates): return ( f"Refusing to write to sensitive system path: {filepath}\n" diff --git a/tools/terminal_tool_config.py b/tools/terminal_tool_config.py index 58d12f2b6a..a4ce13537a 100644 --- a/tools/terminal_tool_config.py +++ b/tools/terminal_tool_config.py @@ -9,9 +9,10 @@ so ``tools.terminal_tool.`` keeps resolving (and monkeypatching) as before import logging import json import os +import posixpath import re from contextlib import contextmanager -from typing import Any +from typing import Any, overload # Log-record parity with the origin module. logger = logging.getLogger("tools.terminal_tool") @@ -95,30 +96,35 @@ def _get_plugin_env_provider(env_type: str): return _plugin_registry_lookup(env_type, "get_provider", None) +@overload +def coerce_ssh_remote_cwd(cwd: str, env_type: str | None) -> str: ... +@overload +def coerce_ssh_remote_cwd(cwd: None, env_type: str | None) -> None: ... def coerce_ssh_remote_cwd(cwd: str | None, env_type: str | None) -> str | None: """Cwd to send to an SSH backend. - ``~`` and ``~/...`` stay literal so the remote shell expands them to the + ``~``-prefixed paths stay literal so the remote shell expands them to the SSH user's home. The Hermes process's subprocess home (``/opt/data/home`` in the official Docker image) is a directory on the machine running - Hermes. Using it as the remote working directory makes ``cd`` exit 126 - and anchors relative writes on a path that does not exist on the target. - Other backends are unchanged. + Hermes: it and anything under it are rewritten onto the remote ``~``, since + ``cd`` into the host path exits 126 on the target. A subprocess home that is + the OS user's real home is left alone: a remote path may legitimately match + it. Other backends are unchanged. """ if not isinstance(cwd, str) or (env_type or "").strip().lower() != "ssh": return cwd text = cwd.strip() - if not text or text == "~" or text.startswith("~/"): + if not text or text.startswith("~"): return text or "~" - try: - from hermes_constants import get_subprocess_home + from hermes_constants import get_real_home, get_subprocess_home - home = get_subprocess_home() - except Exception: - home = None - if home and os.path.normpath(text) == os.path.normpath(home): + home = get_subprocess_home() + if not home or not posixpath.isabs(text) or posixpath.normpath(home) == posixpath.normpath(get_real_home()): + return text + rel = posixpath.relpath(posixpath.normpath(text), posixpath.normpath(home)) + if rel == ".": return "~" - return text + return text if rel == ".." or rel.startswith("../") else f"~/{rel}" def _is_unusable_container_cwd(cwd: str) -> bool: