fix: widen secure_parent_dir to skip entire install tree
Replace hardcoded /opt/hermes check with dynamic install-tree detection using Path(__file__).resolve().parent. This catches ALL install paths (Docker /opt/hermes, apt /usr/local/lib/hermes-agent, git clone, custom) instead of just the Docker image path. Also covers subdirectories of the install tree, not just the top-level dir. Add regression test test_install_tree_skipped to verify both the install root and subdirectories are excluded from chmod. Add contributor email mapping for bradmarshall987. Follow-up to PR #93050 by @bradmarshall987.
This commit is contained in:
@@ -0,0 +1 @@
|
||||
bradmarshall987
|
||||
+9
-8
@@ -350,6 +350,11 @@ _HERMES_NODE_TARGET_MAJOR = int(os.environ.get("HERMES_NODE_TARGET_MAJOR", "22")
|
||||
_managed_node_heal_attempted = False
|
||||
_NODE_BOOTSTRAP_SCRIPT = Path(__file__).resolve().parent / "scripts" / "lib" / "node-bootstrap.sh"
|
||||
|
||||
# Install tree root (this file lives at <install_root>/hermes_constants.py).
|
||||
# Used by secure_parent_dir() to skip chmod on the install dir — chmodding it
|
||||
# 0700 breaks hermes-user traversal in Docker (UID 10000). See #25821, #93050.
|
||||
_INSTALL_ROOT = Path(__file__).resolve().parent
|
||||
|
||||
|
||||
def node_tool_runnable(path: str | None) -> bool:
|
||||
"""Return True only when *path* is a Node/npm/npx binary that actually runs.
|
||||
@@ -1023,14 +1028,10 @@ def secure_parent_dir(path: Path) -> None:
|
||||
# Refuse root and its direct children (/usr, /home, /var, /tmp, …).
|
||||
if parent == Path("/") or len(parent.parts) < 3:
|
||||
return
|
||||
# Refuse /opt/hermes. The install dir lives on the image layer;
|
||||
# chmodding it to 0700 breaks hermes-user traversal and produces
|
||||
# spurious "Permission denied" on every new exec until manual
|
||||
# `chmod 0755 /opt/hermes`. Reproducer: any auth write to a file
|
||||
# directly under /opt/hermes (e.g. /opt/hermes/auth.json when
|
||||
# HERMES_HOME resolves there) triggers the 0700 chmod and locks
|
||||
# out UID 10000. See issue #25821 follow-up.
|
||||
if str(parent) == "/opt/hermes":
|
||||
# Refuse the install tree root. chmodding it 0700 breaks hermes-user
|
||||
# traversal in Docker (UID 10000) and any other install where the
|
||||
# runtime user doesn't own the install dir. See #25821, #93050.
|
||||
if parent == _INSTALL_ROOT or _INSTALL_ROOT in parent.parents:
|
||||
return
|
||||
try:
|
||||
os.chmod(parent, 0o700)
|
||||
|
||||
@@ -560,8 +560,29 @@ class TestSecureParentDir:
|
||||
secure_parent_dir(Path("/foo"))
|
||||
assert called_with == []
|
||||
|
||||
def test_install_tree_skipped(self, monkeypatch):
|
||||
"""Parent dir equal to (or inside) the install tree must NOT be chmod'd.
|
||||
|
||||
Regression test for #93050: secure_parent_dir() chmod'd /opt/hermes to
|
||||
0700 because it has 3 path parts and passed the ``< 3`` guard, locking
|
||||
out UID 10000 (hermes user) from traversing the install dir.
|
||||
"""
|
||||
install_root = Path(hermes_constants.__file__).resolve().parent
|
||||
|
||||
# Directly under the install root (e.g. /opt/hermes/auth.json)
|
||||
target = install_root / "auth.json"
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
secure_parent_dir(target)
|
||||
assert called_with == [], "must not chmod the install root"
|
||||
|
||||
# Inside a subdirectory of the install root
|
||||
sub = install_root / "subdir"
|
||||
target2 = sub / "auth.json"
|
||||
called_with2 = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with2.append((str(p), m)))
|
||||
secure_parent_dir(target2)
|
||||
assert called_with2 == [], "must not chmod dirs inside the install tree"
|
||||
|
||||
@pytest.mark.require_symlinks
|
||||
def test_symlink_resolved(self, tmp_path, monkeypatch):
|
||||
|
||||
Reference in New Issue
Block a user