Skip to content

fix(telegram): honor DM env allowlist when yaml allow_from is set - #55529

Open
xiawiie wants to merge 3 commits into
NousResearch:mainfrom
xiawiie:fix/telegram-dm-allowlist-55462
Open

fix(telegram): honor DM env allowlist when yaml allow_from is set#55529
xiawiie wants to merge 3 commits into
NousResearch:mainfrom
xiawiie:fix/telegram-dm-allowlist-55462

Conversation

@xiawiie

@xiawiie xiawiie commented Jun 30, 2026

Copy link
Copy Markdown

Summary

Follow-up to #55496. The original PR fixed the same-shape bug for group/forum senders but kept the early return False for DMs, so a user listed only in TELEGRAM_ALLOWED_USERS is still blocked when DMing the bot whenever yaml allow_from is set to a different user — which is the second half of the scenario reporter described in #55462:

The same user is also blocked when trying to DM the bot (blocked in chat 18xxxx1221).

This change removes the DM short-circuit so the existing _telegram_sender_auth_env_configured() fallback handles DMs the same way it already handles groups: yaml allow_from wins when it matches; otherwise sender-scoped env vars (TELEGRAM_ALLOWED_USERS, TELEGRAM_ALLOW_ALL_USERS, GATEWAY_ALLOWED_USERS, GATEWAY_ALLOW_ALL_USERS) get a chance. When no env sender allowlist is configured, DMs still deny — same as before.

Behavior change to note

The symmetry with the existing group path means that if a user has yaml allow_from=[A] and only TELEGRAM_GROUP_ALLOWED_USERS=B set (no DM-scoped env allowlist, no TELEGRAM_ALLOWED_USERS), a DM from any user not matching allow_from now falls through to the runner / final env fallback instead of being denied at the adapter. This matches how group messages are already handled in that exact config — it isn't a new code path, just the same fallback applied symmetrically. The "no env at all" case is unchanged (still denies, covered by test_config_allow_from_still_blocks_dm_sender_without_env).

Test plan

  • pytest tests/gateway/test_telegram_auth_check.py tests/gateway/test_telegram_group_gating.py → 73 passed
  • Reproduced the gap on the base PR branch (user in env allowlist, DM rejected)
  • No regression on test_config_allow_from_still_blocks_group_sender_without_group_allowlist

Depends on #55496. Refs #55462.

itsflownium and others added 2 commits June 30, 2026 17:53
Follow-up to NousResearch#55496. The original PR removed the adapter-level allow_from
short-circuit for group/forum senders but kept it for DMs, so a user listed
in TELEGRAM_ALLOWED_USERS is still rejected when DMing the bot whenever yaml
allow_from is set to a different user — the second half of the scenario in
issue NousResearch#55462.

Drop the DM short-circuit so the existing _telegram_sender_auth_env_configured
fallback applies symmetrically: yaml allow_from wins when it matches;
otherwise sender-scoped env vars get a chance. When neither matches and no
sender env is configured, DMs still deny.

Refs NousResearch#55462.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jun 30, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment — Complex auth logic with many code paths

This PR adds Telegram DM env allowlist support when yaml allow_from is set, plus allowed_users/group_allowed_users aliases. The auth logic now has multiple overlapping code paths for DM vs group vs forum authorization.

Observations

  • The core fix is correct: when allow_from is set in yaml, group senders should still be checkable against group-specific allowlists
  • _coerce_telegram_allowlist properly handles string, list, tuple, and set inputs
  • _telegram_sender_auth_env_configured correctly distinguishes chat-level vs sender-level env config
  • Tests cover the key scenarios: group env allowlist not masked by DM allow_from, group_allowed_users alias, allowed_users alias

Concern

  • The auth flow now has 4+ branches with overlapping conditions. Consider adding a comment or diagram showing the authorization decision tree for future maintainers.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Telegram authorization fix. Current main still has the reported intake-gate ordering problem: plugins/platforms/telegram/adapter.py:861-865 returns from allow_from before the runner/environment authorization path at :886-907; the DM change addresses that directly.

Problems

  • The PR makes allowed_users and group_allowed_users supported YAML aliases (gateway/config.py and plugins/platforms/telegram/adapter.py), but website/docs/user-guide/messaging/telegram.md:1000-1029 documents only allow_from / group_allow_from. The final “docs” commit adds an inline decision-tree comment rather than user documentation.

Suggested changes

  • Add the aliases and their canonical-key precedence to the Telegram configuration documentation.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants