feat: add Telegram channel support#2168
Conversation
esengine
left a comment
There was a problem hiding this comment.
Substantial feature and the access-control module is well-structured — userId normalization, log redaction (redactTelegramUserId), clear owner/allowlist/runtime/open modes, and it correctly fails closed once an owner or allowlist is configured (non-matching → unauthorized). grammY is a solid library choice. But this can't merge yet and there's one security default I want changed first.
1. Rebase — the PR is conflicting. It touches config.ts, App.tsx, slash/commands.ts, slash/dispatch.ts, EN.ts/zh-CN.ts and package-lock, all of which moved with recent merges. Please rebase onto current main and re-resolve; I'll review the real diff after that.
2. CI hasn't run (fork PR). An 1815-line feature that adds a network-facing bot needs green CI before merge — I'll approve the workflow.
3. Security: don't default to open/TOFU for a bot that drives a code agent (blocker). In decideTelegramAccess, when no ownerUserId, allowlist, or runtimeBoundUserId is set, the first sender is accepted (mode: "open", bindRuntime: true) and bound trust-on-first-use. The problem: this bot can edit files and run shell on the host, so 'whoever messages first wins' is a remote-code-execution exposure if the token leaks or the operator forgets to set an owner — and there's a race window between bot start and the first legitimate message. Please make it fail-closed: if neither owner nor allowlist is configured, refuse to start the channel (or reject every message) with a clear 'set telegram.ownerUserId or allowlist' error, rather than binding the first stranger. If you want to keep a convenience path, gate TOFU behind an explicit opt-in flag (e.g. telegram.trustFirstSender: true) and log a loud warning while it's unbound. describeTelegramAccess already reports 'open (unbound)', which confirms this state is reachable.
4. Things I'll want to check once rebased (note for the next pass, not necessarily changes): how an incoming Telegram message maps to agent actions — can a remote message invoke slash commands / tools / destructive operations directly, and is there any rate limiting or command filtering? Given the RCE surface, the message→action path is the most important thing to get right. And confirm the new grammy dependency is pinned and the lockfile addition is intentional.
Net: rebase, flip the unconfigured default to fail-closed, get CI green, and I'll do a full pass on the message-dispatch path.
7640bbc to
0883723
Compare
0883723 to
da3172f
Compare
|
Thanks for the detailed review. Updated in the latest push:
Local verification now passes:
CI is still waiting on the fork PR workflow approval from upstream. Once it runs, I’ll follow up on the message-dispatch path concerns in the next review pass. |
esengine
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased version — both blockers are resolved:
- Rebased onto main; conflicts in config.ts/App.tsx/slash wiring/i18n/lockfile re-resolved.
- Fail-closed default confirmed.
TelegramAccessModedroppedopenentirely, anddecideTelegramAccessnow ends in{ accept: false, reason: "unauthorized" }— so with noownerUserId/allowlist/runtimeBound, every sender is rejected, and the channel refuses to start with theaccessRequirederror ('Set telegram.ownerUserId or telegram.allowlist'). The TOFUruntimemode survives but only as an explicit opt-in and only accepts the bound id.tests/telegram-access.test.tscovers it. That's exactly the safe-by-default posture I wanted for a code-driving bot.
I also traced the dispatch path you deferred: an incoming message becomes telegramSubmitRef.current?.(text), i.e. a normal agent prompt, and the submittingRef/busy FIFO queue serializes turns. Since that's now gated behind fail-closed allowlisting, the RCE surface is contained to users the operator explicitly trusts — acceptable.
CI re-approved on my side (fork). Two non-blocking follow-ups for later, not gating merge:
- Rate limiting: the busy-queue serializes but doesn't bound queue growth; an authorized user spamming messages grows it unboundedly. A simple cap/debounce would harden it.
- A focused pass on
bot.tstoken handling + polling error paths would be worth it given it's network-facing, but nothing in the access/dispatch model blocks merge.
Once CI (esp. CodeQL, given the new dep + network code) is green, this is good to land. Nice turnaround.
esengine
left a comment
There was a problem hiding this comment.
CI is green now (CI + CodeQL both pass), and the two blockers were resolved in the rebased push — fail-closed default (no open mode; refuses to start without owner/allowlist) and the dispatch path is gated behind that. Approving and merging. The two non-blocking follow-ups still stand for a later PR: bound the message queue (rate limiting) and a focused bot.ts token/polling audit. Thanks for the thorough turnaround.
Summary
Tests
Closes #2093