Skip to content

fix(cron): route update_job() through the gateway-lifecycle guard - #94655

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/cron-update-job-lifecycle-guard
Open

pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/cron-update-job-lifecycle-guard

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

Summary

create_job() enforces cron.lifecycle_guard.check_gateway_lifecycle() against prompt+script before a job is scheduled, specifically to prevent an agent-driven self-restart/self-stop SIGTERM-respawn loop under launchd/systemd KeepAlive (#30719). The comment right above that call is explicit that this exists precisely so a bypass via the agent's own cronjob model tool (which calls create_job directly) is covered, not just hermes cron create.

update_job() — the only other write path for the same prompt/script fields, reachable from hermes cron edit, the agent's own cronjob(action="update", ...) tool, and the dashboard's PATCH /api/jobs/{job_id} REST endpoint (gateway/platforms/api_server.py::_handle_update_job) — never called the guard at all.

Empirically verified before writing the fix, against the real module:

create_job(prompt="evil", schedule="every 5m", no_agent=True, script=<path to a script containing "hermes gateway restart">)
-> GatewayLifecycleBlocked: Blocked: cron job contains a gateway lifecycle command...

# create a harmless job first, then:
update_job(job_id, {"script": <same dangerous script path>})
-> succeeds silently, no error at all

For a no_agent job, cron/scheduler.py::_run_job_script executes the script via a bare subprocess.Popen — no HERMES_GATEWAY flag, no terminal_tool mediation — so this is a complete, unguarded bypass once a benign job is updated to point at (or embed) a lifecycle command. For a prompt-based (agent) job it downgrades to the LLM's own choice to run it as a shell command, which terminal_tool's separate HERMES_GATEWAY=1 guard would still catch as a second layer.

Fix

update_job() re-runs check_gateway_lifecycle() against the merged (post-update) prompt+script whenever either field is part of the update — matching create_job()'s own "both fields scanned together" contract, which also closes the split-across-fields evasion the guard's own docstring calls out (a command spread across prompt and script so neither field alone looks dangerous). Updates that don't touch either field are unaffected, so a legacy record whose stored script became dangerous through some other means (a hand-edit, a pre-guard record) doesn't suddenly block an unrelated rename/reschedule.

Verification

  • Empirically verified the fix closes all three vectors: prompt-only update, script-only update, and an update to one field combined with an already-stored dangerous value in the other (merged-record scan).
  • New regression tests (tests/hermes_cli/test_gateway_restart_loop.py::TestUpdateJobBlocksLifecycleCommands, 6 tests), mirroring the existing TestCreateJobBlocksLifecycleCommands class exactly: prompt-only block, script-only block, the merged-record/split-fields case, benign updates still succeed, an unrelated-field update on a legacy-dangerous record doesn't rescan, and the end-to-end cronjob(action="update", ...) tool surfaces the block as an error with the #30719 hint (matching the create-path coverage).
  • Mutation-verified: reverting the fix reproduces exactly 4 failures (the blocking cases); the 2 benign-update controls pass either way.
  • Full tests/hermes_cli/test_gateway_restart_loop.py (303 tests) passes.
  • Broader sweep: tests/cron/ + tests/tools/test_cronjob_tools.py/test_cronjob_run_background.py/test_cronjob_run_immediate.py + tests/hermes_cli/test_cron.py (1054 tests, 1 pre-existing skip) all pass.
  • ruff check clean on both changed files.

Competitor / related-PR notes (checked fresh before opening)

  • fix(cron): block gateway lifecycle commands on job update #51980 (open since 2026-06-24, stale, fix(cron): block gateway lifecycle commands on job update) patches this exact bug at a narrower layer: it adds the guard check inside tools/cronjob_tools.py's cronjob(action="update", ...) handler only, never touching cron/jobs.py::update_job() itself. I confirmed via grep that update_job() has two other real callers besides that tool wrapper: cron/monitor.py/cron/scheduler.py (internal, non-attacker-controlled field updates, unaffected either way) and, critically, gateway/platforms/api_server.py::_handle_update_job — the dashboard's PATCH /api/jobs/{job_id} REST endpoint, which passes prompt through to update_job() with no lifecycle check of its own. Even if fix(cron): block gateway lifecycle commands on job update #51980 merges as-is, that dashboard path would remain exposed for prompt-based lifecycle commands. This PR fixes the shared chokepoint itself, so it covers hermes cron edit, the agent tool, the dashboard, and any future caller in one place. If fix(cron): block gateway lifecycle commands on job update #51980 also lands, its tool-layer check becomes a harmless, redundant double-check — not a conflict — though both PRs add a new test class at the same anchor point in tests/hermes_cli/test_gateway_restart_loop.py, so whichever merges second will need a small rebase there.
  • No other open or closed PR touches cron/jobs.py::update_job()'s relationship to the lifecycle guard.

create_job() enforces cron.lifecycle_guard.check_gateway_lifecycle()
against prompt+script before a job is scheduled, specifically to
prevent an agent-driven self-restart/self-stop SIGTERM-respawn loop
under launchd/systemd KeepAlive (NousResearch#30719). The comment right above that
call is explicit that this exists precisely so a bypass via the agent's
own `cronjob` model tool (which calls create_job directly) is covered,
not just `hermes cron create`.

update_job() — the only other write path for the same prompt/script
fields, reachable from `hermes cron edit`, the agent's own
`cronjob(action="update", ...)` tool, and the dashboard's
`PATCH /api/jobs/{job_id}` REST endpoint (gateway/platforms/
api_server.py::_handle_update_job) — never called the guard at all. A
job created with a harmless script could be updated afterward to embed
`hermes gateway restart` / `systemctl restart hermes-gateway` /
`launchctl kickstart ...` with zero guard in between. For a `no_agent`
job (cron/scheduler.py::_run_job_script executes the script via a bare
subprocess.Popen, with no HERMES_GATEWAY flag and no terminal_tool
mediation) this is a complete, unguarded bypass; for a prompt-based job
it downgrades to the LLM's own choice to run it as a shell command,
which terminal_tool's separate HERMES_GATEWAY=1 guard would still catch.

Empirically verified before writing the fix: create_job() correctly
raises GatewayLifecycleBlocked for a script containing
`hermes gateway restart`, but update_job() lets the exact same script
through onto an already-created, previously-benign job with no error at
all.

Fix: update_job() re-runs check_gateway_lifecycle() against the merged
(post-update) prompt+script whenever either field is part of the
update — matching create_job()'s own "both fields scanned together"
contract, which also closes the split-across-fields evasion the guard's
docstring calls out (a command spread across prompt and script so
neither field alone looks dangerous). Updates that don't touch either
field are unaffected, so a legacy record whose stored script became
dangerous through some other means (a hand-edit, a pre-guard record)
doesn't suddenly block an unrelated rename/reschedule.

New regression tests (tests/hermes_cli/test_gateway_restart_loop.py,
TestUpdateJobBlocksLifecycleCommands, 6 tests) mirror the existing
TestCreateJobBlocksLifecycleCommands class: prompt-only block,
script-only block, the merged-record/split-fields case, benign updates
still succeed, an unrelated-field update on a legacy-dangerous record
doesn't rescan, and the end-to-end cronjob(action="update", ...) tool
surfaces the block as an error with the NousResearch#30719 hint (matching the
create-path coverage exactly).

Mutation-verified: reverting the fix reproduces exactly 4 failures (the
blocking cases); the 2 benign-update controls pass either way.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Aug 25, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

This PR closes a real security gap: create_job() enforces a gateway-lifecycle guard that blocks cron jobs from embedding gateway-restart commands (preventing SIGTERM-respawn loops under launchd/systemd KeepAlive, #30719), but update_job() — the only other write path for prompt/script — never re-validated. A job could be created benign and then updated to embed a restart command. The fix scans the merged record (prompt + script together), correctly closing the split-across-fields evasion the guard's docstring calls out. The test suite is excellent — particularly test_update_job_unrelated_field_does_not_rescan which handles the legacy/hand-edited record edge case.

A few technical concerns:

  1. Lazy import inside function body (cron/jobs.py:2276): The from cron.lifecycle_guard import check_gateway_lifecycle import is done inside update_job() rather than at module top. If this is to avoid a circular import, that's understandable, but it means an import error would only surface at runtime when a user updates a job — not at startup. Consider adding a brief comment explaining why the import is deferred, or hoisting it to module level if there's no circular dependency.

  2. Script parameter handling: check_gateway_lifecycle receives updated.get("script") which could be None when only the prompt is being updated (line 2280). The prompt is coerced via _coerce_job_text(...).strip() but the script is passed raw. If check_gateway_lifecycle doesn't gracefully handle None for the script argument, this would raise an unexpected error on prompt-only updates. Worth verifying that the guard's signature explicitly accepts Optional[str].

  3. Split-command across simultaneous prompt+script update: The tests cover updating one field while the other is benign, and updating script while prompt is benign. But there's no test for the case where BOTH prompt and script are updated simultaneously, each carrying half of a split command. The merged-record scan should catch this, but an explicit test would confirm the {**job, **updates} merge correctly assembles the combined record before the guard runs.

Overall, this is a well-reasoned, well-tested fix for a genuine privilege-escalation path through the cron update API.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants