fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131)#34497
Merged
Conversation
Worker threads that dispatch Hermes tools started with an empty contextvars.Context and no thread-local approval/sudo callbacks. Add tools/thread_context.propagate_context_to_thread factoring that capture/install/clear lifecycle (mirrors the GHSA-qg5c-hvr5-hjgr pattern), and refactor agent/tool_executor onto it so the security-critical logic lives in one audited place. Update the contextvar-propagation source guard for the new call shape. Refs #33057
…+ guard entry Wrap both execute_code RPC threads (local UDS + remote file-RPC) with propagate_context_to_thread so gateway sessions no longer fall into check_dangerous_command's non-interactive auto-approve branch and the CLI approval prompt stays reachable. Add check_execute_code_guard: one-shot fail-closed approval of the whole script in gateway/ask/cron-deny before the child spawns (skips isolated backends; command-string built only past the early returns). Drop the broad HERMES_ env passthrough for an explicit operational allowlist plus DSN/WEBHOOK secret substrings, and update the POSIX-equivalence oracle. Refs #4146, #27303, #30882, #33057
When approvals.mode=manual with security.tirith_enabled off and no auxiliary.approval model, dangerous commands and execute_code scripts can only be gated by live in-chat approval; with routing fixed they now fail closed (block) rather than silently auto-run. Surface that at startup so operators knowingly enable tirith or auxiliary.approval for unattended gateways. Refs #30882
Cover context+callback propagation and teardown-clears, a source guard that both RPC threads stay wrapped, the check_execute_code_guard decision matrix (isolated backend, headless-local, cron-deny, gateway approve/deny/timeout/missing-notify, smart mode, session-yolo), the env-scrub allowlist/secret rules, and a behavioral test that execute_code() blocks before spawning on denial. Refs #4146, #27303, #30882, #33057
… scrub Follow-up mitigation for the #27303 env-scrub tightening. Dropping the broad HERMES_ prefix in favor of a 4-var operational allowlist is correct hardening, but a sandbox script that imports a repo module reading a non-allowlisted HERMES_* var at import time would otherwise see it silently unset. _scrub_child_env now emits a one-shot debug log naming the dropped non-secret HERMES_* vars and pointing at the env_passthrough opt-in escape hatch. Secret-shaped vars are never named in the log. Tests: dropped vars are logged + env_passthrough named; no log when nothing is dropped; secret vars excluded from the diagnostic.
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/tools/test_execute_code_approval_cluster.py:22: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues (1):
| Rule | Count |
|---|---|
call-non-callable |
1 |
First entries
tools/approval.py:1237: [call-non-callable] call-non-callable: Object of type `~None` is not callable
Unchanged: 4897 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
This was referenced May 29, 2026
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
3 participants
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.
Closes #4146 · Closes #27303 · Closes #33057 · Closes #30882 · supersedes #27304, #30893, #33246
Salvage of #34131 (@firefly / banditburai) onto current
main, plus a small diagnosability follow-up. Restores the dangerous-command approval guard across everyexecute_codeentry point and stops the sandbox child env from leaking config.Summary
execute_code's RPC worker threads were rawthreading.Threads — they started with an empty context, dropping the approval/session ContextVars (routing gateway sessions into the non-interactive auto-approve branch) and the thread-local approval/sudo callbacks (so the CLI prompt couldn't reach the user). Dangerous commands a sandbox script issued therefore ran without approval. The approval gate is a documented heuristic inSECURITY.md, not a security boundary; this restores its intended behavior — it doesn't add one.Fixes
terminal()skips the approval guardterminalstays allowlistedHERMES_*env passthrough leaks configHow
tools/thread_context.py— one auditedpropagate_context_to_thread()(ContextVar + thread-local approval/sudo capture→install→clear). Applied to both execute_code RPC threads;agent/tool_executor.pyrefactored onto it (one copy, not three).check_execute_code_guard— one-shot whole-script approval in gateway/ask/cron-deny before the child spawns; fail-closed on deny/timeout/missing-notify; skips isolated container backends. Gateway wait loop deduped into_await_gateway_decision.HERMES_prefix → explicit{HOME,PROFILE,CONFIG,ENV}allowlist +DSN/WEBHOOKsecret substrings. Skill/configenv_passthroughremains the explicit opt-in (checked first).approvals.mode=manualwith no risk assessor.Follow-up mitigation (this salvage)
Tightening the env scrub is correct, but a sandbox script importing a repo module that reads a non-allowlisted
HERMES_*var at import time would otherwise see it silently unset._scrub_child_envnow emits a one-shot debug log naming the dropped non-secretHERMES_*vars and pointing at theenv_passthroughopt-in. Secret-shaped vars are never named in the log.Behavior changes (no regression to common workflows)
approvals.mode=manualnow prompts before a sandbox script runs (consistent with howterminal()already behaves).off/yolo bypasses, same as terminal.cron_mode=deny(the default) now blocks execute_code, closing the hole in an existing policy that already blocks terminal commands.cron_mode: approverestores it.Validation
env_passthroughopt-in restores a droppedHERMES_*var post-scrub.Authorship: @firefly's 4 commits preserved via rebase-merge; the diagnosability follow-up is a separate maintainer commit.
Infographic