fix: close simplify-pass findings — scheduler sibling site + home-unresolvable totality
3-reviewer simplify pass (reuse/quality/efficiency) findings: - cron/scheduler.py _run_job_script: the ORIGINAL that lifecycle_guard._resolve_script_path documents mirroring had the exact same unguarded expanduser() — a NUL-bearing script value survives creation (the guard treats it as nothing-to-scan) and crashed the scheduler at fire time with ValueError instead of a clean job failure. Same ingestion contract applied; regression test added. - lifecycle_guard._resolve_script_path: get_hermes_home() -> Path.home() raises RuntimeError when neither HERMES_HOME nor HOME resolves (arbitrary-UID containers); the cron entry point called it bare. Caught -> None; totality test added. - terminal_tool: stale 'cat ...' docstring updated to the bounded head -c form. - lifecycle_guard: dead 'script_text and' condition dropped (guarded by 'if not script_text: continue' directly above). Efficiency reviewer: no material findings (measured — encode/expand costs negligible vs walk I/O, no timing regression vs base).
This commit is contained in:
parent
c135b88d2d
commit
863e313185
|
|
@ -410,7 +410,7 @@ def _contains_unsafe_gateway_action(
|
|||
# Relative references inside a script resolve against that script's
|
||||
# directory, not the original command's cwd.
|
||||
script_dir = _resolve_script_directory(str(resolved)) or cwd
|
||||
if script_text and _contains_unsafe_gateway_action(
|
||||
if _contains_unsafe_gateway_action(
|
||||
script_text,
|
||||
cwd=script_dir,
|
||||
depth=depth + 1,
|
||||
|
|
@ -488,7 +488,13 @@ def _resolve_script_path(script_path: str) -> Optional[Path]:
|
|||
return None
|
||||
if raw.is_absolute():
|
||||
return raw
|
||||
return get_hermes_home() / "scripts" / raw
|
||||
try:
|
||||
return get_hermes_home() / "scripts" / raw
|
||||
except (RuntimeError, OSError):
|
||||
# get_hermes_home() falls back to Path.home(), which raises when
|
||||
# neither HERMES_HOME nor HOME is resolvable (launchd/systemd
|
||||
# environments) — same ingestion contract: nothing to scan.
|
||||
return None
|
||||
|
||||
|
||||
def _read_script_for_scanning(script_path: str) -> str:
|
||||
|
|
|
|||
|
|
@ -2251,7 +2251,16 @@ def _run_job_script(
|
|||
scripts_dir.mkdir(parents=True, exist_ok=True)
|
||||
scripts_dir_resolved = scripts_dir.resolve()
|
||||
|
||||
raw = Path(script_path).expanduser()
|
||||
try:
|
||||
raw = Path(script_path).expanduser()
|
||||
except (ValueError, RuntimeError, OSError):
|
||||
# Same ingestion contract as cron.lifecycle_guard: a NUL-bearing
|
||||
# value (ValueError) or an unexpandable ``~`` (RuntimeError with no
|
||||
# resolvable HOME) can never name a real script. The creation-time
|
||||
# guard tolerates such values as "nothing to scan", so they can
|
||||
# reach fire time — fail the run with a report instead of crashing
|
||||
# the scheduler with an unhandled exception.
|
||||
return False, f"Blocked: script path is not a valid filesystem path: {script_path!r}"
|
||||
if raw.is_absolute():
|
||||
path = raw.resolve()
|
||||
else:
|
||||
|
|
|
|||
|
|
@ -118,3 +118,16 @@ def test_run_job_script_path_traversal_still_blocked(hermes_env):
|
|||
ok, output = _run_job_script("/etc/passwd")
|
||||
assert ok is False
|
||||
assert "Blocked" in output or "outside" in output
|
||||
|
||||
|
||||
def test_run_job_script_nul_path_fails_cleanly(hermes_env):
|
||||
"""Sibling of the lifecycle-guard ingestion fix: a NUL-bearing script
|
||||
value can survive to fire time (the creation-time guard treats it as
|
||||
"nothing to scan"), and ``Path.expanduser()`` raises ValueError — not
|
||||
OSError — on it. The scheduler must fail the run with a report, not
|
||||
crash with an unhandled exception."""
|
||||
from cron.scheduler import _run_job_script
|
||||
|
||||
ok, output = _run_job_script("~user\x00bad.sh")
|
||||
assert ok is False
|
||||
assert "Blocked" in output
|
||||
|
|
|
|||
|
|
@ -867,6 +867,30 @@ class TestLifecycleGuardModule:
|
|||
"echo hello"
|
||||
) is False
|
||||
|
||||
def test_cron_guard_total_when_home_unresolvable(self, monkeypatch):
|
||||
"""`get_hermes_home()` falls back to Path.home(), which raises
|
||||
RuntimeError when neither HERMES_HOME nor HOME resolves
|
||||
(arbitrary-UID containers, launchd). The cron entry point must
|
||||
treat a relative script value as unresolvable — nothing to scan —
|
||||
not crash."""
|
||||
from pathlib import Path
|
||||
|
||||
from cron.lifecycle_guard import check_gateway_lifecycle
|
||||
|
||||
monkeypatch.delenv("HERMES_HOME", raising=False)
|
||||
monkeypatch.delenv("HOME", raising=False)
|
||||
monkeypatch.setattr(
|
||||
Path,
|
||||
"home",
|
||||
classmethod(
|
||||
lambda cls: (_ for _ in ()).throw(
|
||||
RuntimeError("Could not determine home directory")
|
||||
)
|
||||
),
|
||||
)
|
||||
# Must not raise; relative script cannot resolve without a home.
|
||||
check_gateway_lifecycle("daily ops", "relative-script.sh")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defense 2 (chokepoint): cron.jobs.create_job blocks the AGENT model-tool path
|
||||
|
|
|
|||
|
|
@ -2533,7 +2533,7 @@ def terminal_tool(
|
|||
|
||||
For local backends the script path is on the host filesystem. For
|
||||
SSH/Modal/Daytona the same path is remote; the local read misses, so we
|
||||
fall back to ``env.execute('cat ...')``.
|
||||
fall back to a bounded ``env.execute('head -c ... < path')`` read.
|
||||
"""
|
||||
if env is None:
|
||||
return None
|
||||
|
|
|
|||
Loading…
Reference in New Issue