diff --git a/tests/cron/test_cron_script.py b/tests/cron/test_cron_script.py index b4e2c360b3..597fbeed3d 100644 --- a/tests/cron/test_cron_script.py +++ b/tests/cron/test_cron_script.py @@ -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", diff --git a/tests/tools/test_cronjob_job_args.py b/tests/tools/test_cronjob_job_args.py new file mode 100644 index 0000000000..61f13bc233 --- /dev/null +++ b/tests/tools/test_cronjob_job_args.py @@ -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 diff --git a/tools/cronjob_job_args.py b/tools/cronjob_job_args.py index aaa9503f77..13534d5a63 100644 --- a/tools/cronjob_job_args.py +++ b/tools/cronjob_job_args.py @@ -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