Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (deferred full review — large diff)
This is a substantial new feature (1,264 lines) introducing cross-platform approval delegation. The design is well-documented in the module docstring. A few observations for the author:
Looks Good
- Thread-safe state management using
threading.Lock— appropriate for a gateway service - TTL-based delegation entries (600s) prevent indefinite memory growth
reload_delegation_config()supports hot-reload after config edits- Clear
admin_chat_keyformat for routing
Suggestions
- The global
_delegation_mapis in-process only. In a multi-worker gateway setup, approvals delegated to one worker would not be visible to another. Consider noting this limitation in the module docstring if horizontal scaling is expected. resolve_delegationusesmax(sessions.values(), key=...)on the most recent entry — this could raiseValueErrorifsessionsbecomes empty after pruning. The pruning happens inside the lock, so it should be safe, but an explicit guard would be defensive.
Reviewed by Hermes Agent
|
1416ee3 to
8d36d69
Compare
c4b684e to
3843bb8
Compare
|
@teknium1 @tonydwb Ready for re-review 🙏
|
7f58570 to
c2a66bc
Compare
c2a66bc to
e4a01c6
Compare
|
Hi @teknium1 @tonydwb |
5cdc0a8 to
e789122
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for replacing the earlier monkey-patch approach with an opt-in native flow. The delegation premise remains valid on current main: gateway/run.py:18632-18653 still sends approvals to the originating chat.
Problems
gateway/slash_commands.py:4355resolves by platform/chat only. It does not verifyevent.source.user_idagainst the configured admin, so an otherwise gateway-authorized member of a configured admin group can resolve a delegated request with typed/approveor/deny.gateway/run.py:18857passesadmin_user_id, but most changed adapters only accept ignored**kwargs. Telegram's existing pending state stores onlysession_key(plugins/platforms/telegram/adapter.py:4631-4633) and authorizes callbacks through the general gateway policy (:5361-5373), not the delegation admin identity.gateway/slash_commands.py:4430-4435schedules work onasyncio.get_running_loop()and blocks on.result().safe_schedule_threadsafe()callsasyncio.run_coroutine_threadsafe()(agent/async_utils.py:62-63), so this blocks the loop needed to send the notification.
Suggested changes
- Carry and enforce configured admin user ID plus chat ID for typed and button approval paths on every supported platform.
- Await the originating-user notification directly, and add runner/slash integration tests for identity rejection and notification delivery.
Automated hermes-sweeper review.
| source = event.source | ||
| _src_plat = source.platform.value if hasattr(source.platform, "value") else str(source.platform) | ||
| _src_chat_id = str(source.chat_id or "") | ||
| _delegation = resolve_delegation(_src_plat, _src_chat_id) |
There was a problem hiding this comment.
This lookup is keyed only by platform and chat. Before resolving, verify event.source.user_id against the configured admin for this delegation; otherwise any gateway-authorized participant in a configured admin group can type /approve and resolve the original user's command.
| command=cmd, | ||
| session_key=_approval_session_key, | ||
| description=f"[Delegation] {_user_name} ({_src_plat}): {desc} | {cmd[:50]}", | ||
| admin_user_id=_admin.get("user_id"), |
There was a problem hiding this comment.
admin_user_id is ignored by the newly changed Telegram/Discord/Feishu/Matrix/Teams/QQ/WhatsApp sender signatures (**kwargs). Store and enforce it in each callback state, rather than relying on the broader platform authorization policy, or the configured separation-of-duties identity is not enforced.
| log_message="Delegation user notify error", | ||
| ) | ||
| if _notify_fut is not None: | ||
| _notify_fut.result(timeout=15) |
There was a problem hiding this comment.
This async handler schedules onto its own running loop and then blocks that loop with .result(timeout=15). The scheduled send cannot run until the wait returns. await _user_adapter.send(...) here, or use non-blocking task scheduling with failure logging.
d13f2cf to
509fc5c
Compare
ad14b31 to
c113cbc
Compare
c113cbc to
579a936
Compare
579a936 to
29a70bb
Compare
29a70bb to
2b11d48
Compare
|
Hi @teknium1, could you please approve the workflow runs for this PR when you have a moment? CI is currently blocked on first-time-contributor workflow approval ( Thanks! |
8cb5f8b to
e98d07a
Compare
bd125e8 to
61d64b0
Compare
5fa7bd0 to
a3a4801
Compare
9cb9f4c to
b5ba30b
Compare
… (+7 platforms) Addresses three review findings from @teknium1 on PR NousResearch#47863: 1. /approve verifies event.source.user_id via is_admin_user() before resolving delegated approvals — any gateway-authorized user in the admin's chat could previously resolve them. 2. admin_user_id was passed by gateway/run.py but silently swallowed by **kwargs in 6 platform adapters (Telegram, Discord, Feishu, Slack, Teams, Matrix). Now each adapter declares it as a formal parameter, stores it in the callback state, and enforces it on every button click / reaction — the separation-of-duties model requires the exact configured admin, not just any chat member. (WeCom already had this; unchanged.) 3. async handler _try_handle_delegated_approval blocked its own event loop with .result(timeout=15). Replaced with await asyncio.wrap_future() so the scheduled send can run. Changes: +126 / -12 across 7 files 14/14 tests pass
0c5c1ad to
3fbaa1b
Compare
|
Rebased onto latest Done: branch rebased onto Meanwhile upstream
Attack-surface pivot forced by the template refactor (replaces the earlier signature-probe note): with Verification: 434 passed, 4 skipped locally — the full Net diff vs |
9e40272 to
4ada018
Compare
Config-driven delegation of dangerous-command approvals from non-admin users to designated admins across platforms (e.g. WeChat user -> Feishu admin), with 4-button cards on all capable platforms, typed /approve text fallback, and per-platform admin-identity + chat-origin validation on every button click. Grafted onto upstream's exec-approval template method (ad305be) and the plain-language message pass (23036e2): - ExecApprovalPrompt carries admin_user_id through the base template. - Delegation buttons route only to adapters that declare _enforces_delegation_admin_identity (fail-closed; the template method made the old signature probe unable to tell enforcing adapters from render-only ones). - Approval button/card labels resolve once in the base under an opt-in locale flag; the English default tracks upstream's shared EA_HEADER_TEXT wording so card header and body never diverge. - Telegram keeps validate-before-consume (auth via the shared _UNAUTHORIZED notice, admin-gate pop-after-validate) instead of _claim_callback_state's pop-on-claim, preserving finding NousResearch#6. Addresses review findings NousResearch#1-NousResearch#8 (see PR description).
4ada018 to
617c7cc
Compare
Summary
Native, config-driven approval delegation — routes non-admin users' dangerous command approvals to designated admins across platforms (e.g. WeChat/WeCom → Feishu). Replaces PR #37771's monkey-patch approach with direct hook integration in core modules.
Why a new PR (not iteration of #37771)
PR #37771 used monkey-patching to intercept gateway approval flows — 3 runtime patches on
run.pyinternals that break on refactors and are hard to review. This implementation achieves the same feature via native hooks:_approval_notify_sync,_handle_approve_command,_handle_deny_commandapprovals.delegate_to(list)approvals.delegation.enabled+approvals.delegation.admins(list)t()for all card elements (header, reason, buttons) across all platforms_try_handle_delegated_approval()shared methoddelegation.py+__init__.pymonkey-patches__init__.py— pure state, no side effectsEnterprise Security Context
In enterprise environments, the default approval mechanism allows self-approval — the same user who triggers a dangerous command can approve it. This violates separation of duties and makes Hermes unsuitable for regulated deployments.
Key Changes
gateway/approval_delegation/__init__.pygateway/run_turn_runner.py_approval_notify_sync: button-based (send_exec_approval) + text fallback.register_delegation()only called after successful send. Button routing additionally requires the adapter to declare the admin identity gate (_enforces_delegation_admin_identitymarker; see finding #8) — adapters without it get the text/approvepath instead.gateway/platforms/base.pyExecApprovalPrompt.admin_user_idfield +send_exec_approvalforwarding; explicit enforcement marker_enforces_delegation_admin_identity/supports_delegation_admin_gate(); locale-aware approval labels (_EA_I18N_ACTION_LABELS) resolved once in the base — English rendering is byte-identical to upstreamgateway/slash_commands.py/approve/denyhandling with cross-platform result notification. Delegation check runs BEFORE the local pending-approval gate (admins resolve other users' sessions).plugins/platforms/feishu/adapter.pyoperator.user_idnotopen_id)plugins/platforms/wecom/adapter.pysend_exec_approval()with template_card buttons, re-adapted to the streaming architecture (group chats reply passively via cached req_id; DMs useAPP_CMD_SEND) + dual securityplugins/platforms/slack/adapter.py_approval_resolvedfor double-click +_approval_adminfor admin identity). Multi-workspace compatible.plugins/platforms/telegram/adapter.py_approval_statedict withadmin_user_id/chat_idenforcementplugins/platforms/discord/adapter.py_component_check_authadmin enforcementplugins/platforms/matrix/adapter.pyadmin_user_idenforcementplugins/platforms/teams/adapter.pyadmin_user_idenforcementtests/gateway/test_telegram_approval_buttons.py_approval_state(assertions unchanged)hermes_cli/config_defaults.pyapprovals.delegationlocales/*.yaml(16 files)gateway.approval_delegation.*translation keys (22 keys)tests/gateway/approval_delegation/tests/gateway/platforms/test_wecom_approval.pyReviewer Feedback (addressed)
Three security findings from @teknium1's review have been resolved across all 7 button-capable platforms:
/approve//denydidn't verifyuser_id— any chat member could resolveis_admin_user()check in_try_handle_delegated_approval()admin_user_idswallowed by**kwargs— button callbacks used general gateway auth, not delegation admin identityadmin_user_ida formal parameter across all adapters; stored in callback state; enforced on every button clickasynchandler blocked event loop withfuture.result(timeout=15)await asyncio.wrap_future()Adversarial Review Findings (addressed)
Independent adversarial review (multiple rounds, incl. after the WeCom button restoration and the latest rebase) discovered five additional hardening issues:
admin_user_id: all platforms silently skipped admin identity enforcement whenadmin_user_idwas empty stringlogger.warning()on all button-capable platforms whenadmin_user_idis empty. Discord already fail-closed (emptyadmin_user_idsset = deny).register_delegation()called beforesend_exec_approval()— failed sends left orphan delegations blocking sessions for 600s TTLregister_delegation()inside theif _sent:block. Delegation only created after confirmed delivery to admin. Failed sends cleanly skip to next admin or fallback._approval_stateBEFORE admin identity validation — any chat member (attacker) clicking the button first consumed the state, so the real admin's later click got "already been resolved", permanently blocking the approval.get()for validation,.pop()only after all checks pass. Unauthorized clicks return early with state preserved, matching the Feishu/WeCom/Teams pattern (verified: attacker click → state retained → admin can still approve).stored=Noneto fail-closed (unknown/guessed task_id → no action), matching the Feishu/Telegram/Teams pattern. Validated with adversarial tests.send_exec_approvala base template method (ad305be) and added native button rendering on qqbot / WhatsApp / relay after this PR's original security review. The template acceptsadmin_user_idfor every adapter, so a signature probe can no longer tell the enforcing renderers from the ones that silently ignore the field (WhatsApp: any allowlist member could approve another user's session; qqbot: session-owner check makes admin clicks dead UX)_enforces_delegation_admin_identity = True+supports_delegation_admin_gate()). The delegation router only sends buttons to marked adapters; everything else — including upstream-native qqbot/WhatsApp/relay and any future template renderer — falls back to typed/approve, which always runsis_admin_user. Fail-closed by default (base marker =False). 7 regression tests pin the invariant across all adapters.Per-platform
admin_user_idenforcement_approval_state: Dict[int, dict]caller_idvs storedadmin_user_id+chat_idmatch_approval_stateadmin_user_idchecked in_component_check_auth_approval_stateoperator.user_idvs adminuser_id(notopen_id) +chat_idmatch_approval_resolved: Dict[Any, bool]+_approval_admin: Dict[Any, tuple]channel_id+admin_user_idin separate dict. Multi-key lookup for workspace-scoped markers. WARNING on missing/empty admin info.admin_user_idcheck on button clickadmin_user_idcheck on reaction_approval_tasks: Dict[str, tuple]_handle_template_card_event: chat_id + user_id match, fail-closed (preserves task on reject)Config
Features
/approve/denyfor platforms without button supportadmin_user_idproducesWARNINGlog for visibility.Platform Support
_is_interactive_operator_authorizedexpected_chat_id_is_callback_user_authorizedchat_id/chat_typematch_component_check_auth+ roleschannel_idmatch_is_interactive_user_authorizedexpected_chat_idsender_idin_handle_template_card_eventexpected_chat_idButton Layout (all platforms)
All button-capable platforms render the same 4 options:
Button clicks flow through platform-specific callbacks, then synthesise
/approve [session|always]or/denytext commands through the standard slash-command pipeline.Security: Forwarded Card Prevention
All button-capable platforms implement dual validation to prevent forwarded-card abuse:
user_id/open_id)chat_id/channel_id)If an admin forwards an approval card to another chat, any button click from that chat is rejected with audit logging — the original admin retains the ability to approve.
operator.user_idcheckopen_chat_idmatchfrom_user.idcheckchat_id/chat_typematchuser.idcheckchannel.idmatch_component_check_auth(user + roles + admin_user_id)channel_idmatchsender.useridcheckchatidmatchTesting
Unit Tests
Full regression after the latest rebase (onto 979576d, template-method graft): 434 passed, 4
skipped in a single
tests/gateway -k "approv or delegat or exec_approval or wecom"sweep —including the two suites above, upstream's own
test_exec_approval_template.py, theapproval-boundary / approve-deny / send-timeout-ambiguity / prompt-redaction / decline-fallback
suites, and the feishu/slack/telegram/discord/teams/matrix/qqbot/whatsapp approval-button +
callback-auth suites. No upstream regressions.
E2E Verified
Registered: admin=feishu:... → session=agent:main:weixin+Redirected approval to admin→ Feishu admin button click resolves → command executes; same-session follow-up skips re-approval (session tier)locale.language: zh)Breaking Changes
None. Existing approval behavior is unchanged when
delegation.enabledisfalse(default).Related Work
Three other open PRs touch adjacent parts of the approval pipeline. No overlap in implementation, but listed here for reviewer context:
approvals.escalate_to: platform:chat_id— statically route approval prompts to a single operator target; text-only, fail-closed on unavailable target_gateway_queuesby(session_key, user_id)to prevent cross-user/approvehijack in shared gateway threadsis_admin_user()+ chat-origin validation at the delegation boundary. Both can merge independently.load_config_readonly()on approval guard path (perf)Closes
Closes #37771