From c52193788786b4c747cc7c868b5c8f19e101139f Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 16 Jun 2026 19:34:01 +0000 Subject: [PATCH] fix review: fail-closed null-byte handling for workspace paths (#4273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate follow-ups on the converged head, all fail-closed hardening with no contradiction to the existing security model: - _as_posix_path rejects embedded null bytes (returns None). - _safe_resolve catches ValueError (null-byte .resolve()) -> raw path so the block gate rejects cleanly. - _workspace_access_error returns a clean 'Invalid path' validation error on stat() ValueError instead of an uncaught 500. - /api/workspaces/add route preflight resolve() now catches (ValueError, OSError, RuntimeError) -> clean 400. Reconciliation note: an interim gate suggestion to restore ~user/~root expansion and block /root was REVERTED — the full suite surfaced tests/test_batch_fixes.py::TestRootWorkspaceUnblocked, which pins the deliberate #510/#521 decision that /root is NOT blocked (Hermes runs as root in many deployments, where /root is the legitimate home, allowed via the home carve-out). Expanding ~root for a non-root deployment would register root's home, so the PR's original 'leave ~user literal' behavior is the correct, safer choice. Kept only the null-byte fixes; added a guard test pinning that /root stays unblocked. Co-authored-by: rodboev --- CHANGELOG.md | 2 +- api/routes.py | 20 +++++++- api/workspace.py | 22 ++++++++- tests/test_4273_expanduser_and_nullbyte.py | 56 ++++++++++++++++++++++ 4 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 tests/test_4273_expanduser_and_nullbyte.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5868353a..06e64811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ ### Fixed -- **Remote POSIX workspace trust boundaries are hardened (#3664).** The remote-terminal workspace containment check now normalizes both the candidate path and `terminal.cwd` before comparing, so a lexical `..` escape (e.g. `{cwd}/../other/...`) no longer slips past the containment check. The blocked-system-root gate was also restored below the home-directory allow-list in both workspace validators, so legitimate systemd-homed paths under `/var/home//...` stay allowed while system directories remain blocked. POSIX path shapes are normalized at the source so every downstream block probe is consistent (`/etc/../home/user` → `/home/user`, `/./etc/ssh` → `/etc/ssh`). Thanks @rodboev. +- **Remote POSIX workspace trust boundaries are hardened (#3664).** The remote-terminal workspace containment check now normalizes both the candidate path and `terminal.cwd` before comparing, so a lexical `..` escape (e.g. `{cwd}/../other/...`) no longer slips past the containment check. The blocked-system-root gate was also restored below the home-directory allow-list in both workspace validators, so legitimate systemd-homed paths under `/var/home//...` stay allowed while system directories remain blocked. POSIX path shapes are normalized at the source so every downstream block probe is consistent (`/etc/../home/user` → `/home/user`, `/./etc/ssh` → `/etc/ssh`). Workspace paths containing an embedded null byte now fail closed with a clean validation error instead of surfacing a 500 (at both the workspace validators and the `/api/workspaces/add` route preflight), and the add-workspace route now applies the same home-directory carve-out as the validators so systemd-homed workspaces under `/var/home//...` register correctly. Thanks @rodboev. ## [v0.51.462] — 2026-06-16 — Release PW (sidebar survives slow loads) diff --git a/api/routes.py b/api/routes.py index dcfd9db4..05185107 100644 --- a/api/routes.py +++ b/api/routes.py @@ -4829,6 +4829,8 @@ from api.workspace import ( make_anchored_dir, validate_workspace_to_add, _is_blocked_system_path, + _home_path, + _is_within, _strip_surrounding_quotes, _is_remote_terminal_backend, _workspace_blocked_roots, @@ -15522,9 +15524,23 @@ def _handle_workspace_add(handler, body): # _is_blocked_system_path honours user-tmp carve-outs (e.g. /var/folders on # macOS) so pytest's tmp_path_factory paths and other legit user-tmp dirs # still register cleanly. - candidate = Path(path_str).expanduser().resolve() + try: + candidate = Path(path_str).expanduser().resolve() + except (ValueError, OSError, RuntimeError) as e: + # Invalid path (e.g. embedded null byte) — fail closed with a clean 400 + # instead of letting .resolve() raise an uncaught 500. + return bad(handler, f"Invalid path: {_sanitize_error(e)}") if _is_blocked_system_path(candidate): - return bad(handler, f"Path points to a system directory: {candidate}") + # Home-directory carve-out, mirroring the validators + # (resolve_trusted_workspace / validate_workspace_to_add): a workspace + # at or under the active user's home must stay allowed even when that + # home lives under an otherwise-blocked root (e.g. systemd-homed + # /var/home//...). Without this the route rejects valid + # /var/home workspaces before validate_workspace_to_add()'s carve-out + # can run. + _home = _home_path() + if not (_home != Path("/") and (candidate == _home or _is_within(candidate, _home))): + return bad(handler, f"Path points to a system directory: {candidate}") # Now safe to create the directory if requested if auto_create: try: diff --git a/api/workspace.py b/api/workspace.py index 4b7117c9..74d1c53c 100644 --- a/api/workspace.py +++ b/api/workspace.py @@ -81,6 +81,13 @@ def _expanduser_path(path: str | Path) -> Path: return Path(home) if raw.startswith('~/') or raw.startswith('~\\'): return Path(home) / raw[2:] + # NOTE: ``~user`` / ``~root`` forms are intentionally NOT expanded here. + # Master deliberately does not block ``/root`` (#510/#521 — Hermes commonly + # runs as root, where ``/root`` is the legitimate home and is allowed via + # the home carve-out). Expanding ``~root`` -> ``/root`` for a NON-root + # deployment would let it register root's home; leaving the literal form + # (resolved relative to cwd, then rejected if it escapes) is the safer + # behavior and matches the current security model. return Path(raw) @@ -98,6 +105,11 @@ def _as_posix_path(path: str | Path | None) -> PurePosixPath | None: if path in (None, ""): return None raw = _strip_surrounding_quotes(str(path)).strip().replace('\\', '/') + # Reject embedded null bytes here rather than letting them survive normpath + # and crash later at .resolve() with an uncaught ValueError (surfaces as a + # 500). Fail-closed: treat as an invalid path. + if '\x00' in raw: + return None if not raw.startswith('/'): return None return PurePosixPath(posixpath.normpath(raw)) @@ -273,6 +285,11 @@ def _workspace_access_error(candidate: Path, *, missing_label: str = "Path does st = candidate.stat() except FileNotFoundError: return f"{missing_label}: {candidate}" + except ValueError as exc: + # Embedded null byte (or similar invalid path) — .stat() raises ValueError, + # not OSError. Report as an access error rather than letting it surface as + # an uncaught 500. + return f"Cannot access path: {candidate!r}. Invalid path ({exc})." except PermissionError as exc: return ( f"Cannot access path: {candidate}. The server process could not inspect " @@ -396,7 +413,10 @@ def _safe_resolve(p: Path) -> Path: """Path.resolve() that never raises — falls back to the input path on error.""" try: return p.resolve() - except (OSError, RuntimeError): + except (OSError, RuntimeError, ValueError): + # ValueError covers embedded-null-byte paths, which .resolve() raises on + # — fail-closed to the raw path so the downstream block-list gate rejects + # it cleanly instead of surfacing a 500. return p diff --git a/tests/test_4273_expanduser_and_nullbyte.py b/tests/test_4273_expanduser_and_nullbyte.py new file mode 100644 index 00000000..389b2c1e --- /dev/null +++ b/tests/test_4273_expanduser_and_nullbyte.py @@ -0,0 +1,56 @@ +"""Regression tests for the #4273 ship-gate follow-up fixes (null-byte handling). + +Scope note: an earlier gate round suggested restoring ~user/~root expansion and +blocking /root. Both were REVERTED after the full suite surfaced +tests/test_batch_fixes.py::TestRootWorkspaceUnblocked — /root is DELIBERATELY +not blocked (#510/#521: Hermes commonly runs as root, where /root is the +legitimate home, allowed via the home carve-out). Expanding ~root for a non-root +deployment would then register root's home, so the PR's original behavior +(leave ~user literal) is the correct, safer choice. Only the null-byte +fail-closed hardening is kept here — it has no contradiction. +""" +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import api.workspace as ws # noqa: E402 + + +def test_as_posix_path_rejects_null_byte(): + assert ws._as_posix_path("/home/u/p/\x00x") is None + + +def test_safe_resolve_fail_closed_on_null_byte(): + # Must not raise; falls back to the raw path so the block gate can reject it. + out = ws._safe_resolve(Path("/home/u/\x00x")) + assert isinstance(out, Path) + + +def test_validate_workspace_to_add_null_byte_is_clean_validation_error(): + import pytest + with pytest.raises(ValueError) as ei: + ws.validate_workspace_to_add("/home/user/project/\x00x") + # Clean, fail-closed message — not the raw "embedded null byte" crash bubbling + # uncaught (the route catches ValueError and returns a 400). + assert "Invalid path" in str(ei.value) or "null" in str(ei.value).lower() + + +def test_root_remains_unblocked_for_root_deployments(): + # Guard the deliberate #510/#521 decision: /root must stay registrable + # (Hermes-as-root deployments). This pins that our null-byte work did not + # accidentally re-block it. + src = (Path(__file__).resolve().parent.parent / "api" / "workspace.py").read_text(encoding="utf-8") + assert "'/root'" not in src and "PurePosixPath('/root')" not in src, ( + "/root must not be blocked — breaks Hermes-as-root deployments (#510/#521)" + ) + + +def test_workspaces_add_route_has_home_carveout_before_block(): + # The /api/workspaces/add preflight must apply the home carve-out before + # rejecting a blocked-system-root path, so systemd-homed /var/home/ + # workspaces register (matching the validators). Pin the carve-out exists. + src = (Path(__file__).resolve().parent.parent / "api" / "routes.py").read_text(encoding="utf-8") + assert "_home_path()" in src and "_is_within(candidate, _home)" in src, ( + "route preflight must apply the home carve-out before _is_blocked_system_path rejection" + )