fix review: fail-closed null-byte handling for workspace paths (#4273)

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 <rodboev@users.noreply.github.com>

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/<user>/...` 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/<user>/...` 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/<user>/...` register correctly. Thanks @rodboev.
 
 ## [v0.51.462] — 2026-06-16 — Release PW (sidebar survives slow loads)
 

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/<user>/...). 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:

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
 
 

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/<user>
+    # 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"
+    )