mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-07 08:02:23 +00:00
fix(cron): reject id mutation + validate output paths under OUTPUT_DIR
Two defense-in-depth fixes on cron output path handling:
1. cron/jobs.py:update_job() rejects mutation of the immutable 'id' field
(raises ValueError). Dashboard PUT /api/cron/jobs/{id} converts this to
HTTP 400. Without this, an attacker who can reach the update endpoint
could rename a job's id to '../escape' and move its output directory
outside OUTPUT_DIR.
2. cron/jobs.py:_job_output_dir() validates job IDs before composing
paths: rejects '.', '..', '/', '\\', absolute paths, and Windows drive
prefixes. Used by save_job_output() and remove_job() so legacy unsafe
IDs (from before this guard) fail closed rather than half-applying a
shutil.rmtree or output write outside the sandbox.
Tests:
- update_job rejects {'id': '../escape'} without renaming
- remove_job(legacy '../escape' id) raises ValueError without deleting
files outside OUTPUT_DIR or removing the job from the store
- save_job_output rejects '..', './escape', 'nested/escape',
absolute paths
- dashboard PUT /api/cron/jobs/{id} with {'id': '../escape'} returns
400, job list unchanged
Salvaged from PR #29826 by @zapabob. Simplified implementation:
- Dropped a 23-line _validate_job_output_id() helper using Path.parts
semantics. The inline check (path separators + dot-components +
is_absolute) is shorter and behaviorally identical.
- Dropped the secondary OUTPUT_DIR.resolve()/relative_to() check —
redundant once we reject any path separator at the input boundary.
- Dropped the _docs/2026-05-21_cron-output-path-hardening_codex.md
planning artifact (we don't check planning docs into the repo).
Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
This commit is contained in:
parent
0c3e34e298
commit
2c3ca475c0
4 changed files with 113 additions and 4 deletions
|
|
@ -232,6 +232,23 @@ class TestJobCRUD:
|
|||
assert remove_job(job["id"]) is True
|
||||
assert get_job(job["id"]) is None
|
||||
|
||||
def test_remove_job_rejects_unsafe_legacy_id_before_output_cleanup(self, tmp_cron_dir):
|
||||
"""Legacy unsafe IDs left over from before the create-time guard
|
||||
must fail closed without half-applying the removal."""
|
||||
job = create_job(prompt="Legacy unsafe", schedule="every 1h")
|
||||
job["id"] = "../escape"
|
||||
save_jobs([job])
|
||||
outside = tmp_cron_dir / "escape"
|
||||
outside.mkdir()
|
||||
(outside / "keep.txt").write_text("keep", encoding="utf-8")
|
||||
|
||||
with pytest.raises(ValueError, match="output path"):
|
||||
remove_job("../escape")
|
||||
|
||||
# Job should still be in the store and the escape dir untouched.
|
||||
assert load_jobs()[0]["id"] == "../escape"
|
||||
assert (outside / "keep.txt").exists()
|
||||
|
||||
def test_remove_nonexistent_returns_false(self, tmp_cron_dir):
|
||||
assert remove_job("nonexistent") is False
|
||||
|
||||
|
|
@ -300,6 +317,17 @@ class TestUpdateJob:
|
|||
result = update_job("nonexistent_id", {"name": "X"})
|
||||
assert result is None
|
||||
|
||||
def test_update_rejects_id_change(self, tmp_cron_dir):
|
||||
"""Job IDs are filesystem path components — must be immutable."""
|
||||
job = create_job(prompt="Original", schedule="every 1h")
|
||||
|
||||
with pytest.raises(ValueError, match="id"):
|
||||
update_job(job["id"], {"id": "../escape"})
|
||||
|
||||
# Original job still resolvable, no rename happened.
|
||||
assert get_job(job["id"]) is not None
|
||||
assert get_job("../escape") is None
|
||||
|
||||
|
||||
class TestPauseResumeJob:
|
||||
def test_pause_sets_state(self, tmp_cron_dir):
|
||||
|
|
@ -953,3 +981,16 @@ class TestSaveJobOutput:
|
|||
assert output_file.exists()
|
||||
assert output_file.read_text() == "# Results\nEverything ok."
|
||||
assert "test123" in str(output_file)
|
||||
|
||||
@pytest.mark.parametrize("bad_job_id", ["../escape", "nested/escape", ".", "..", ""])
|
||||
def test_rejects_unsafe_job_id(self, tmp_cron_dir, bad_job_id):
|
||||
"""Path-escape attempts must fail closed and never create dirs."""
|
||||
with pytest.raises(ValueError, match="output path"):
|
||||
save_job_output(bad_job_id, "# Results")
|
||||
assert not (tmp_cron_dir / "escape").exists()
|
||||
|
||||
def test_rejects_absolute_job_id(self, tmp_cron_dir):
|
||||
"""Absolute paths as job IDs must fail closed."""
|
||||
with pytest.raises(ValueError, match="output path"):
|
||||
save_job_output(str(tmp_cron_dir / "outside"), "# Results")
|
||||
assert not (tmp_cron_dir / "outside").exists()
|
||||
|
|
|
|||
|
|
@ -131,6 +131,33 @@ async def test_cron_mutation_without_profile_finds_named_profile_job(isolated_pr
|
|||
assert worker_jobs[0]["enabled"] is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_cron_job_rejects_id_mutation(isolated_profiles):
|
||||
"""Dashboard surfaces a 400 (not a 500 or silent rename) when an
|
||||
id-mutation attempt is rejected by cron/jobs.update_job."""
|
||||
from hermes_cli import web_server
|
||||
|
||||
worker_job = web_server._call_cron_for_profile(
|
||||
"worker_alpha",
|
||||
"create_job",
|
||||
prompt="managed by named profile",
|
||||
schedule="every 1h",
|
||||
name="immutable-id-job",
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await web_server.update_cron_job(
|
||||
worker_job["id"],
|
||||
web_server.CronJobUpdate(updates={"id": "../escape"}),
|
||||
profile="worker_alpha",
|
||||
)
|
||||
|
||||
assert exc.value.status_code == 400
|
||||
assert "id" in exc.value.detail
|
||||
worker_jobs = await web_server.list_cron_jobs(profile="worker_alpha")
|
||||
assert [job["id"] for job in worker_jobs] == [worker_job["id"]]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cron_delete_with_profile_deletes_only_target_profile(isolated_profiles):
|
||||
from hermes_cli import web_server
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue