fix(telegram): parse ALLOW_ALL_USERS as boolean so an explicit false unblocks DM pairing - #68823
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Telegram regression fix. The premise remains present on current main: plugins/platforms/telegram/adapter.py:1014-1022 treats a non-empty GATEWAY_ALLOW_ALL_USERS=false as configured, and :1074-1083 then invokes runner authorization before the normal pairing path.
Problems
tests/gateway/test_telegram_auth_check.py:304-330is described as end-to-end, but it calls_is_user_authorized_from_message()directly with a fakeRunner. It does not exercise the real unauthorized-DM path, which generates and sends the pairing code ingateway/run.py:11531-11560.
Suggested changes
- Add a real-runner regression covering
GATEWAY_ALLOW_ALL_USERS=falseand asserting pairing-code generation/delivery. The focused boolean-matrix test is still useful.
This is an automated hermes-sweeper review.
| assert adapter._telegram_auth_env_configured() is True | ||
|
|
||
|
|
||
| def test_unknown_dm_reaches_pairing_when_allow_all_is_false(monkeypatch): |
There was a problem hiding this comment.
This test is an adapter-level unit test, not end-to-end: the fake Runner proves the prefilter bypasses its authorization method, but it never executes GatewayRunner's unauthorized-DM branch that generates and sends a pairing code. Please add a real-runner regression for that delivery path.
4ffcd03 to
c635174
Compare
|
Thanks — good catch. You're right that Added a real-runner regression, Kept the focused boolean-matrix and adapter-prefilter tests as you suggested — they stay useful for the narrow parse contract. |
SummaryOne PR addresses issue #68794. #68823 changes Telegram’s auth-prefilter configuration check so false-valued Related pull requests
Suggested consolidationKeep #68823 open with a salvage path: retain the focused parser fix, boolean-matrix coverage, and new real-runner pairing regression, and request contributor re-review because the current diff addresses the concrete gap identified by the still-visible Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I68794(["issue #68794 (open)"])
P68823["PR #68823 (open)"]
P68823 -->|best fix| I68794
class I68794 open
class P68823 open
class P68823 best
class P68823 target
click I68794 "https://github.com/NousResearch/hermes-agent/issues/68794"
click P68823 "https://github.com/NousResearch/hermes-agent/pull/68823"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 10 kB of PR diffs, 6 kB of issue/PR text, 5 kB of discussion (5 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…unblocks DM pairing
The intake prefilter's _telegram_auth_env_configured() tested every auth env
var — including the *_ALLOW_ALL_USERS toggles — for a non-empty string. A
literal GATEWAY_ALLOW_ALL_USERS=false (a common .env / config-template default)
therefore counted as 'auth configured', so the prefilter consulted the runner's
allowlist for an unknown DM sender and dropped it before the pairing flow could
run — the victim never got a pairing code. An unset key and a false key are
semantically identical everywhere else in the gateway.
Split the keys: allowlist keys (identities/chats) still count as configured on
any non-empty value, but the *_ALLOW_ALL_USERS booleans are parsed as booleans
(truthy in {true,1,yes}), matching the existing parse in _is_user_authorized and
the Discord adapter. An explicit false now reads as not configured, so unknown
DMs fall through to pairing as documented.
Fixes NousResearch#68794
…_ALL=false (NousResearch#68794) Add a GatewayRunner-level regression that drives an unknown Telegram DM through _handle_message with only GATEWAY_ALLOW_ALL_USERS=false set, and asserts the pairing branch actually generates and delivers a code. The existing adapter-prefilter tests prove the intake filter lets the DM through; this covers the downstream runner path the sweeper review flagged as untested.
b8874b8 to
38285c3
Compare
What
The Telegram intake prefilter (
_telegram_auth_env_configured, introduced in #54164 for #40863) decided whether auth was "configured" by testing every auth env var — including the*_ALLOW_ALL_USERStoggles — for a non-empty string. So an explicitGATEWAY_ALLOW_ALL_USERS=false(a common.env/ config-template default) counted as configured, the prefilter consulted the runner's allowlist for an unknown DM sender, and the message was dropped before the pairing flow could run:The victim never received a pairing code. An unset key and a
falsekey are semantically identical everywhere else in the gateway — including_is_user_authorized(adapter.py:920) which already parsesGATEWAY_ALLOW_ALL_USERSas a boolean, and the Discord adapter.Fix
Split the keys in
_telegram_auth_env_configured:TELEGRAM_ALLOWED_USERS,TELEGRAM_GROUP_ALLOWED_USERS,TELEGRAM_GROUP_ALLOWED_CHATS,GATEWAY_ALLOWED_USERS) — carry identities/chats, so any non-empty value still counts as configured.*_ALLOW_ALL_USERStoggles — parsed as booleans (truthy in{true, 1, yes}), matching the existing parse atadapter.py:920and the Discord adapter.An explicit
falsenow reads as not configured, so unknown DMs fall through to pairing as the docstring already promises.Tests
tests/gateway/test_telegram_auth_check.py:test_allow_all_users_false_is_not_configured—false/False/0/no/""ALLOW_ALL toggles → not configured;true→ configured; a real allowlist value → still configured.test_unknown_dm_reaches_pairing_when_allow_all_is_false— end-to-end: with onlyGATEWAY_ALLOW_ALL_USERS=false, an unknown DM reaches pairing (True) and the runner allowlist is never consulted.Both fail on
mainand pass with the fix. Preflight (windows-footguns, ruff, affected tests) green.Fixes #68794
The arm64-fork-Docker CI job is expected to fail on fork PRs and is unrelated to this change.