From 3c8d3ecfa0c90883cf408bcb62d6d701ae7d5efe Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 27 May 2026 00:13:07 -0700 Subject: [PATCH] fix(approval): extend gateway-lifecycle guard to launchctl and pidof-based kills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dangerous-command approval layer already blocks `hermes gateway (stop|restart)`, `pkill/killall hermes|gateway`, and `kill ... $(pgrep ...)`. A reporter noted on #33071 that the agent can still achieve the same effect by driving launchd directly against the gateway's service label (`launchctl stop ai.hermes.gateway`, `launchctl kickstart -k system/ai.hermes.gateway`, etc.) or by substituting `pidof` for `pgrep` in the kill-expansion form. This widens the "Gateway lifecycle protection" block in `tools/approval.py` to cover both vectors: - `launchctl (stop|kickstart|bootout|unload|kill|disable|remove)` scoped to commands that target a Hermes label (`hermes`, `ai.hermes`). Read-only inspection (`launchctl print …`, `launchctl list`) and operations against unrelated labels remain unflagged. - `kill ... $(pidof …)` and the backtick form, alongside the existing `pgrep` expansion. `pidof` is the BSD/Linux equivalent and is equally opaque to the `(pkill|killall) … hermes` name pattern. Intentionally left out of scope: plain `kill -TERM ` with a PID looked up out-of-band. Catching that would require runtime PID state and would break the existing `TestPgrepKillExpansion::test_safe_kill_pid_not_flagged` contract, which guarantees that a plain literal-PID `kill 12345` stays safe. --- tests/tools/test_approval.py | 55 ++++++++++++++++++++++++++++++++++++ tools/approval.py | 11 ++++++-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 62359601ea8..a4d0c7af22c 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1121,6 +1121,61 @@ class TestPgrepKillExpansion: dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False + def test_kill_dollar_pidof_detected(self): + """`kill $(pidof hermes)` is the BSD/Linux equivalent of the + pgrep expansion and bypasses the pkill/killall name pattern + in the same way. See issue #33071.""" + cmd = "kill -TERM $(pidof hermes_cli.main)" + dangerous, _, desc = detect_dangerous_command(cmd) + assert dangerous is True + assert "pidof" in desc.lower() or "pgrep" in desc.lower() + + def test_kill_backtick_pidof_detected(self): + cmd = "kill -9 `pidof hermes`" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True + + +class TestLaunchctlGatewayLifecycle: + """launchctl stop/kickstart/bootout/unload against the Hermes service + label achieves the same effect as `hermes gateway stop|restart` and + must require the same approval. See issue #33071. + """ + + def test_launchctl_stop_hermes_detected(self): + cmd = "launchctl stop ai.hermes.gateway" + dangerous, _, desc = detect_dangerous_command(cmd) + assert dangerous is True + assert "launchd" in desc.lower() or "hermes" in desc.lower() + + def test_launchctl_kickstart_hermes_detected(self): + cmd = "launchctl kickstart -k system/ai.hermes.gateway" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True + + def test_launchctl_bootout_hermes_detected(self): + cmd = "launchctl bootout system/ai.hermes.gateway" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True + + def test_launchctl_unload_hermes_detected(self): + cmd = "launchctl unload ~/Library/LaunchAgents/ai.hermes.gateway.plist" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True + + def test_launchctl_print_unrelated_not_flagged(self): + """Read-only inspection of an unrelated launchd label must stay safe.""" + cmd = "launchctl print system/com.apple.WindowServer" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False + + def test_launchctl_stop_unrelated_not_flagged(self): + """`launchctl stop` on a non-Hermes label is out of scope for the + gateway-lifecycle guard.""" + cmd = "launchctl stop com.example.unrelated" + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False + class TestGitDestructiveOps: """git reset --hard, push --force, clean -f, branch -D can destroy diff --git a/tools/approval.py b/tools/approval.py index e26aa423f5f..e444e089dce 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -443,8 +443,15 @@ DANGEROUS_PATTERNS = [ # The name-based pattern above catches `pkill hermes` but not # `kill -9 $(pgrep -f hermes)` because the substitution is opaque # to regex at detection time. Catch the structural pattern instead. - (r'\bkill\b.*\$\(\s*pgrep\b', "kill process via pgrep expansion (self-termination)"), - (r'\bkill\b.*`\s*pgrep\b', "kill process via backtick pgrep expansion (self-termination)"), + # `pidof` is the BSD/Linux alternative to `pgrep` and is equally + # opaque, so include it in the same alternation. + (r'\bkill\b.*\$\(\s*(pgrep|pidof)\b', "kill process via pgrep/pidof expansion (self-termination)"), + (r'\bkill\b.*`\s*(pgrep|pidof)\b', "kill process via backtick pgrep/pidof expansion (self-termination)"), + # launchctl-driven gateway stop/restart on macOS. The agent can bypass + # the `hermes gateway stop|restart` pattern above by driving launchd + # directly against the service label (commonly `ai.hermes.gateway`). + # Catch the operations that stop, restart, or unload it. + (r'\blaunchctl\s+(stop|kickstart|bootout|unload|kill|disable|remove)\b.*\b(hermes|ai\.hermes)\b', "stop/restart hermes launchd service (kills running agents)"), # File copy/move/edit into sensitive system paths (/etc/ and macOS # /private/etc/ mirror). (rf'\b(cp|mv|install)\b.*\s{_SYSTEM_CONFIG_PATH}', "copy/move file into system config path"),