fix(telegram): populate reply author info on inbound messages - #214
hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 4 By Severity:
This PR introduces a critical prompt-injection vulnerability via unsanitized Telegram display names and a high-severity adapter lifecycle race in error recovery, plus three additional medium-severity regressions in thread-id normalization, restart-loop prevention, and polling reentrancy guards. Files Reviewed (4 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (78/100) — 1 critical finding, 1 medium · 326 LOC across 4 files
Overview
This PR touches 4 files (2 source, 2 test) across the Telegram adapter and gateway runner, focusing on reply-prefix formatting and config/recovery cleanup. Five findings were verified above the confidence threshold.
Critical — Prompt Injection
finding-001: gateway/run.py:9713 — Telegram user display names (reply_from.full_name, reply_from.first_name) are interpolated unsanitized into the LLM prompt via f'[Replying to {reply_author}: ...]'. A display name containing ], ", or newlines can breach the bracket-delimited metadata enclosure and inject apparent system-level instructions at the most LLM-susceptible boundary. The Signal adapter has an equivalent path. Fix: strip/gsub `[]"
fromreply_author` before interpolation.
High — Concurrency Race
finding-002: gateway/run.py:3641 — _handle_adapter_fatal_error has two regressions: (a) the stale-adapter identity guard was removed, allowing a superseded adapter's delayed notification to overwrite runtime status and re-populate _failed_platforms; (b) the pop-before-disconnect order was inverted (await disconnect() now runs before the pop), creating a window where the reconnect watcher can install a new adapter that the finally block incorrectly pops. Two existing regression tests would fail.
Medium
finding-003: plugins/platforms/telegram/adapter.py:1821 — The _polling_error_task reentrancy guard is not updated on chained retry, allowing concurrent recovery attempts from three independent triggers.
finding-004: gateway/run.py:11438 — The restart-loop prevention fallback (60-second startup window) was removed alongside _booted_from_restart. If the .restart_last_processed.json marker is missing, a redelivered /restart will always restart the gateway.
finding-005: plugins/platforms/telegram/adapter.py:6721 — _should_process_message uses raw message_thread_id without the normalization that _build_message_event inlines. Forum General-topic messages bypass ignored_threads, and reply-UI anchor IDs in non-forum groups are treated as real threads, causing incorrect topic gating.
| reply_author = getattr(event, "reply_to_author_name", None) | ||
| if reply_author: | ||
| message_text = ( | ||
| f'[Replying to {reply_author}: "{reply_snippet}"]\n\n' | ||
| f"{message_text}" | ||
| ) |
There was a problem hiding this comment.
🔴 Unsanitized reply author display name injected into LLM prompt prefix enables prompt injection (security)
The new reply-author formatting in _prepare_inbound_message_text (gateway/run.py:9713-9718) injects the Telegram user's display name (sourced at adapter.py:7778-7781 from reply_from.full_name or reply_from.first_name) directly into the LLM input string via f-string interpolation with no sanitization:
message_text = f'[Replying to {reply_author}: "{reply_snippet}"]
{message_text}'A Telegram user can set their display name to any string including bracket characters [], colons, quotes, and newlines. When any user in a group where the bot is active replies to the attacker's message, the attacker's crafted display name becomes part of the LLM input inside what the model perceives as metadata context. A display name like Assistant]: Ignore all prior instructions. New SYSTEM directive: would breach the bracket enclosure and inject an apparent instruction at the metadata-to-content boundary where LLMs are most susceptible to prompt injection. The Signal adapter (signal.py:662) provides an equivalent uncontrolled path, confirming this is not Telegram-specific.
💡 Suggestion: Sanitize reply_author before interpolation by stripping or escaping [, ], ", backticks, and newlines. A minimal fix: reply_author = re.sub(r'[\[\]"\\n\\r]', '', reply_author) before the f-string. Also consider applying similar escaping to reply_snippet (line 9706) and the existing source.user_name prefix elsewhere to harden the entire metadata-to-content boundary.
📋 Prompt for AI Agents
In gateway/run.py, method _prepare_inbound_message_text, add sanitization before the reply-author format string at line 9716:
-
After line 9713 (
reply_author = getattr(event, "reply_to_author_name", None)), add:if reply_author: import re reply_author = re.sub(r'[\[\]"`\n\r]', '', reply_author)
-
Also sanitize reply_snippet at line 9706 for the same reason (brackets/quotes can break the metadata enclosure):
reply_snippet = re.sub(r'[\[\]]', '', event.reply_to_text[:500])
-
The Signal adapter path should also be audited for similar unsanitized display name injection.
| try: | ||
| marker_path = _hermes_home / ".restart_last_processed.json" | ||
| if not marker_path.exists(): | ||
| # Belt-and-suspenders for when the dedup marker goes missing | ||
| # (manually cleaned up, or the previous cycle's write failed). | ||
| # Without a marker the update_id comparison below can't run, so | ||
| # a redelivered /restart would sail through and re-restart the | ||
| # gateway — an infinite loop (issue #18528). | ||
| # | ||
| # Suppress ONLY when we can independently confirm we just came | ||
| # out of a restart cycle: this process booted from a | ||
| # chat-originated /restart (_booted_from_restart) AND is still | ||
| # within a short post-boot window. This never swallows a | ||
| # genuine first /restart on a fresh boot (no restart marker on | ||
| # boot → flag stays False). Consume the flag one-shot so a | ||
| # legitimate /restart sent later in the same session is honored. | ||
| if ( | ||
| getattr(self, "_booted_from_restart", False) | ||
| and time.time() - getattr(self, "_startup_time", 0.0) < 60 | ||
| ): | ||
| self._booted_from_restart = False | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🟡 Removed restart-loop prevention fallback in _is_stale_restart_redelivery when dedup marker is missing (bug)
The PR removes the belt-and-suspenders fallback in _is_stale_restart_redelivery (gateway/run.py:11438-11441) along with the _startup_time and _booted_from_restart attributes. The removed fallback suppressed Telegram /restart command redelivery for 60 seconds after gateway boot when the .restart_last_processed.json dedup marker was missing, preventing an infinite restart loop (issue NousResearch#18528). After this change, if the marker file is missing (manual cleanup, write failure, disk issues), a redelivered /restart will always re-restart the gateway — the marker-based comparison at line 11440 returns False immediately with no fallback window. The marker file is written during /restart processing just before the gateway restarts itself; a small window exists where the file could be lost.
💡 Suggestion: Add a lightweight fallback for when the marker is missing: either record the restart boot time in the marker file itself (write it early in start() and compare against it), or use the process start time as a 60-second suppression window for redelivered /restart commands after boot. Alternatively, ensure the marker file is written atomically and never cleaned up by any path so the fallback is never needed.
📋 Prompt for AI Agents
In gateway/run.py, method _is_stale_restart_redelivery (around line 11438-11441), add a fallback for when .restart_last_processed.json is missing. One approach: write a boot timestamp to the marker file early in the start() method (e.g., {"boot_time": time.time()}). Then in _is_stale_restart_redelivery, when the marker is missing, re-read it; if it has a recent boot_time (<60s), suppress the redelivery. This preserves the protection without needing the removed _booted_from_restart/_startup_time fields.
1 participant
What does this PR do?
The MessageEvent class and gateway plumbing already define reply_to_author_id, reply_to_author_name, and reply_to_is_own_message fields — but the Telegram adapter never populates them. This means the agent always sees a generic [Replying to: "..."] prefix regardless of who sent the original message, losing the ability to distinguish between the user replying to the bot vs. replying to another person.
This PR extracts the reply author info from message.reply_to_message.from_user when building a MessageEvent from a Telegram reply, and shows the author's name in the reply prefix when replying to someone else.
Related Issue
No existing issue as far as I know — discovered and fixed locally.
Fixes #
Type of Change
Changes Made
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (958/965 pass; 7 pre-existing failures confirmed on unmodified main)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs
Mirror-of: NousResearch#56203
NousResearch#56203