fix(acp): isolate per-session approval callback via ContextVar#13471
fix(acp): isolate per-session approval callback via ContextVar#13471teknium1 wants to merge 1 commit into
Conversation
Concurrent ACP sessions in one Hermes process previously shared tools.terminal_tool._approval_callback as a module-global, so session B overwriting the slot could route session A's dangerous-command prompt through B's callback (and vice versa). Within a single OS user this was UX confusion rather than a cross-principal boundary break, but the shared state is genuine concurrency sloppiness worth fixing. Store the callback (and the sibling sudo password callback) in ContextVars. Each asyncio task gets its own copy, so per-session set_approval_callback calls no longer stomp on each other. ACP's prompt handler now wraps loop.run_in_executor in contextvars.copy_context().run so the per-session callback survives the hop into the worker thread — asyncio does not propagate contextvars across the executor boundary on its own, and this was verified empirically. Regression tests reproduce the original primitive (two overlapping sessions, each asserts it observes its own callback) and document the run_in_executor contextvar contract the ACP fix relies on. Reported by @xeloxa in GHSA-qg5c-hvr5-hjgr.
|
Likely duplicate of #13525 (merged). |
|
@alt-glitch @teknium1 Hi there! I noticed that you've added the 'security' and 'P2' tags here. Previously, my advisories were closed as 'non-security' and no CVE was assigned, so I was just wondering if this is the normal process now? If this is indeed considered a security issue, could I kindly ask you to make my advisories public? I completely understand if you have concerns about the project, but I would really appreciate it if you could review this. Thank you! :) |
|
Redundant — the same GHSA-qg5c-hvr5-hjgr concurrent-callback isolation issue was already fixed on main via |
Summary
Store the ACP approval callback in a
ContextVarso concurrent sessions in one Hermes process each see their own, instead of sharing a module-global that later-arriving sessions overwrite.Changes
tools/terminal_tool.py:_approval_callback/_sudo_password_callbackare nowContextVars;set_approval_callback/set_sudo_password_callbackstill work unchanged for CLI callers (single context holds a single callback).acp_adapter/server.py: drop the save/restore-global dance; wraploop.run_in_executor(_executor, _run_agent)withcontextvars.copy_context().run(...)so the per-session callback propagates into the worker thread.asynciodoes not copy contextvars into executors on its own — empirically verified.tests/acp/test_concurrent_approval_isolation.py: regression test that reproduces the original primitive (two overlapping "sessions," each asserts it observes its own callback) plus a guard on therun_in_executorcontextvar contract the fix relies on.Context
Reported in GHSA-qg5c-hvr5-hjgr by @xeloxa. Within a single OS user this was UX confusion (a dangerous-command prompt could land in the wrong editor tab) rather than a cross-principal boundary break, but the shared-state primitive is real concurrency sloppiness worth fixing. Same pattern we applied to gateway session vars in
e8034e2f.Validation
tests/acp/test_concurrent_approval_isolation.pytests/acp/ tests/cli/test_cli_approval_ui.py tests/tools/test_approval.pytests/tools/ -k "terminal or approval"