Skip to content

feat: add Telegram channel support#2168

Merged
esengine merged 1 commit into
esengine:mainfrom
PorunC:feat/add-Telegram-channel-support
May 29, 2026
Merged

feat: add Telegram channel support#2168
esengine merged 1 commit into
esengine:mainfrom
PorunC:feat/add-Telegram-channel-support

Conversation

@PorunC

@PorunC PorunC commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add Telegram bot/channel configuration and access control
  • add Telegram message handling, channel lifecycle wiring, and slash command support
  • add English and Chinese UI strings plus config/channel tests

Tests

  • npm run test

Closes #2093

@esengine esengine left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@PorunC PorunC force-pushed the feat/add-Telegram-channel-support branch from 7640bbc to 0883723 Compare May 28, 2026 16:52
@PorunC PorunC force-pushed the feat/add-Telegram-channel-support branch from 0883723 to da3172f Compare May 28, 2026 16:55
@PorunC

PorunC commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review.

Updated in the latest push:

  1. Rebased onto current main and re-resolved the conflicts in config.ts, App.tsx, slash command wiring, i18n files, and package-lock.json.
  2. Changed Telegram access to fail closed by default. If neither elegram.ownerUserId nor elegram.allowlist is configured, the channel now refuses to start with a clear access-control error instead of binding the first sender.
  3. Added coverage for the fail-closed behavior in ests/telegram-access.test.ts.
  4. Confirmed grammy is intentionally added and pinned through package-lock.json.

Local verification now passes:

sh npm run verify

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 esengine left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the rebased version — both blockers are resolved:

  1. Rebased onto main; conflicts in config.ts/App.tsx/slash wiring/i18n/lockfile re-resolved.
  2. Fail-closed default confirmed. TelegramAccessMode dropped open entirely, and decideTelegramAccess now ends in { accept: false, reason: "unauthorized" } — so with no ownerUserId/allowlist/runtimeBound, every sender is rejected, and the channel refuses to start with the accessRequired error ('Set telegram.ownerUserId or telegram.allowlist'). The TOFU runtime mode survives but only as an explicit opt-in and only accepts the bound id. tests/telegram-access.test.ts covers 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.ts token 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 esengine left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@esengine esengine merged commit 7854d42 into esengine:main May 29, 2026
4 checks passed
esengine pushed a commit that referenced this pull request May 29, 2026
- auto-git-rollback: write_file output changed from 'wrote N chars' to
  'edited ... (M→N chars)' unified-diff format (#2173); update regex.
- slash + ui-slash-suggestions: Telegram /telegram command added by #2168
  brings the release command count from 43 to 44; update both assertions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: add Telegram channel support

2 participants