fix(security): close kanban worker auto-approve gap for dangerous commands - #54
Open
hashbender wants to merge 1 commit into
Open
fix(security): close kanban worker auto-approve gap for dangerous commands#54hashbender wants to merge 1 commit into
hashbender wants to merge 1 commit into
Conversation
|
Review Complete Risk: 🔴 Critical (82/100) — no findings · 374 LOC across 7 files PR adds kanban approval mode support but the subprocess environment construction has four security/correctness gaps: HERMES_EXEC_ASK leaks from the gateway bypassing kanban deny guards, HERMES_CRON_SESSION leaks from the cron scheduler overriding kanban mode, credentials leak via unsanitized os.environ copy, and _is_gateway_approval_context() misses a HERMES_KANBAN_SESSION exclusion. Files Reviewed (7 files) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Kanban-dispatched worker subprocesses — the primary execution vehicle for long-horizon, unattended multi-agent work in Hermes — fall through every approval-context check in
tools/approval.pyand silently auto-approve any non-hardline dangerous command, with only alogger.warning()left behind.Why
Same bug class as NousResearch#30882 (P0, fixed in NousResearch#34497): an execution surface the approval system doesn't recognize defaults to trusted-by-omission instead of trusted-by-explicit-config. That fix's own description states the philosophy plainly: "The approval gate is a documented heuristic in SECURITY.md, not a security boundary; this restores its intended behavior — it doesn't add one."
Unlike NousResearch#30882 (a ContextVar-propagation regression in threads), this is a plain omission, not a regression — kanban workers are separate
subprocessprocesses (not threads sharing contextvars), and the approval-context question was simply never wired in when kanban dispatch was built.hermes_cli/kanban_db.py's worker-spawn env construction (_default_spawn) sets 15+ environment variables (HERMES_KANBAN_TASK,HERMES_KANBAN_WORKSPACE, etc.) but never any of the four flagstools/approval.pychecks to recognize a non-interactive context:HERMES_CRON_SESSION,HERMES_GATEWAY_SESSION,HERMES_SESSION_PLATFORM,HERMES_INTERACTIVE. All three approval-gate functions (check_dangerous_command,check_all_command_guards,check_execute_code_guard) therefore fall through to the bare non-interactive auto-approve branch.Change
Mirrors the proven
cron_modepattern exactly — not a new mechanism:HERMES_KANBAN_SESSIONenv flag set at worker spawn time (hermes_cli/kanban_db.py)approvals.kanban_modeconfig, defaultdeny(matchingcron_mode's default)_get_kanban_approval_mode()mirrors_get_cron_approval_mode()verbatimtools/approval.py, alongside the existing cron branch — not replacing itapprovals.kanban_mode: approveremains available for users who've deliberately decided to trust their kanban workers, same ascron_modekanban_modelives underapprovals(not thekanban:config block) — that block has a known, separately-filed duplicate-key bug (NousResearch#55779) that silently drops the first of two"kanban"keys in the sameDEFAULT_CONFIGdict literal.How to test
Verified against the real, unmodified functions (not mocked) before writing any fix code:
test_kanban_approval_mode.pypass; all pre-existing cron/CLI/gateway tests across the 4 touched test files stay byte-stable when run per-file (matchingscripts/run_tests.sh's per-file isolation).scripts/check-windows-footguns.pyclean on the diff (pure env-var/config logic).Platforms tested
macOS (logic-only change, no OS-specific code paths — pure env-var/config string matching).
Note
While testing I found an unrelated, pre-existing test-isolation issue (
test_cron_approval_mode.py+test_approval.pyleak shared module state when run together in onepytestprocess — confirmed on unmodifiedmaintoo, invisible to CI becausescripts/run_tests.shisolates each test file into its own subprocess). Filed separately, not part of this diff.Fixes NousResearch#55945
Mirror-of: NousResearch#55946
NousResearch#55946