fix(cron): validate script existence and fix profile-aware path in error messages
_validate_cron_script_path only checked that a no_agent/monitor script path was relative and contained within scripts_dir, never that the file actually existed — so a misplaced script registered fine and only failed at every fire with a generic "Script not found" from scheduler_script.py, dropping once-only jobs from jobs.json on failure. The error messages also hardcoded "~/.hermes/scripts/" even though resolution goes through get_hermes_home(), which is per-profile — actively misleading users running non-default profiles into placing scripts in the wrong directory. Now the validator resolves the file and returns a "Script file not found" error naming the actual resolved scripts_dir, surfacing the mistake at creation time instead of at every scheduled fire.
This commit is contained in:
@@ -451,6 +451,7 @@ class TestCronjobToolScript:
|
||||
monkeypatch.setenv("HERMES_INTERACTIVE", "1")
|
||||
from tools.cronjob_tools import cronjob
|
||||
|
||||
(cron_env / "scripts" / "some_script.py").write_text("print('hi')\n")
|
||||
create_result = json.loads(cronjob(
|
||||
action="create",
|
||||
schedule="every 1h",
|
||||
@@ -471,6 +472,7 @@ class TestCronjobToolScript:
|
||||
monkeypatch.setenv("HERMES_INTERACTIVE", "1")
|
||||
from tools.cronjob_tools import cronjob
|
||||
|
||||
(cron_env / "scripts" / "data_collector.py").write_text("print('hi')\n")
|
||||
cronjob(
|
||||
action="create",
|
||||
schedule="every 1h",
|
||||
|
||||
36
tests/tools/test_cronjob_job_args.py
Normal file
36
tests/tools/test_cronjob_job_args.py
Normal file
@@ -0,0 +1,36 @@
|
||||
"""Tests for tools/cronjob_job_args.py::_validate_cron_script_path (issue #105761).
|
||||
|
||||
Regression coverage: the validator used to accept a script path that pointed
|
||||
at a file that doesn't exist, only failing later at every cron fire with a
|
||||
generic "Script not found" error from cron/scheduler_script.py. It also
|
||||
hardcoded "~/.hermes/scripts/" in its messages even though resolution goes
|
||||
through get_hermes_home(), which is per-profile.
|
||||
"""
|
||||
|
||||
from hermes_constants import get_hermes_home
|
||||
from tools.cronjob_job_args import _validate_cron_script_path
|
||||
|
||||
|
||||
class TestValidateCronScriptPath:
|
||||
def test_missing_script_file_is_rejected_at_creation_time(self):
|
||||
error = _validate_cron_script_path("does_not_exist.sh")
|
||||
assert error is not None
|
||||
assert "not found" in error.lower()
|
||||
|
||||
def test_existing_script_file_passes(self):
|
||||
scripts_dir = get_hermes_home() / "scripts"
|
||||
scripts_dir.mkdir(parents=True, exist_ok=True)
|
||||
(scripts_dir / "real.sh").write_text("#!/bin/sh\necho hi\n")
|
||||
assert _validate_cron_script_path("real.sh") is None
|
||||
|
||||
def test_missing_file_error_names_the_resolved_scripts_dir(self):
|
||||
# The resolved dir must appear literally so the message stays correct
|
||||
# under profiles, where get_hermes_home() is not the global ~/.hermes.
|
||||
scripts_dir = get_hermes_home() / "scripts"
|
||||
error = _validate_cron_script_path("missing.py")
|
||||
assert str(scripts_dir) in error
|
||||
|
||||
def test_absolute_path_error_names_the_resolved_scripts_dir_not_global_literal(self):
|
||||
scripts_dir = get_hermes_home() / "scripts"
|
||||
error = _validate_cron_script_path("/etc/passwd")
|
||||
assert str(scripts_dir) in error
|
||||
@@ -296,17 +296,22 @@ def _validate_cron_script_path(script: Optional[str]) -> Optional[str]:
|
||||
|
||||
from hermes_constants import get_hermes_home
|
||||
raw = script.strip()
|
||||
scripts_dir = get_hermes_home() / "scripts"
|
||||
if raw.startswith(("/", "~")) or (len(raw) >= 2 and raw[1] == ":"):
|
||||
return (
|
||||
f"Script path must be relative to ~/.hermes/scripts/. "
|
||||
f"Script path must be relative to {scripts_dir}/. "
|
||||
f"Got absolute or home-relative path: {raw!r}. "
|
||||
f"Place scripts in ~/.hermes/scripts/ and use just the filename.")
|
||||
f"Place scripts in {scripts_dir}/ and use just the filename.")
|
||||
|
||||
from tools.path_security import validate_within_dir
|
||||
scripts_dir = get_hermes_home() / "scripts"
|
||||
scripts_dir.mkdir(parents=True, exist_ok=True)
|
||||
if validate_within_dir(scripts_dir / raw, scripts_dir):
|
||||
resolved_script = scripts_dir / raw
|
||||
if validate_within_dir(resolved_script, scripts_dir):
|
||||
return f"Script path escapes the scripts directory via traversal: {raw!r}"
|
||||
if not resolved_script.is_file():
|
||||
return (
|
||||
f"Script file not found: {resolved_script}. "
|
||||
f"Create it in {scripts_dir}/ first.")
|
||||
return None
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user