fix(telegram): button-caller authorization breaks on multiplexed profiles (handler introspection) - #65589
Conversation
|
Thanks for the focused authorization fix. Current The PR reuses that existing callback mechanism, retains legacy/fail-closed fallback behavior, and adds allow, deny, and callback-error coverage. No blocking issue found. Automated hermes-sweeper review. |
…uth check, not handler introspection _is_callback_user_authorized resolved the gateway's auth chain through _message_handler.__self__. For a secondary multiplexed adapter the message handler is a per-profile closure with no __self__, so the introspection silently fell through to the env-only fallback -- which knows nothing about config allowlists or the pairing store, denying every button caller on that profile (fail-closed, but wrong). Prefer the auth callback GatewayRunner already injects at connection time via set_authorization_check (registered for primary and multiplexed adapters alike, delegating to the full _is_user_authorized chain), and keep the introspection plus env fallback for adapters wired without it. Same resolution pattern the admin-tier gate uses.
9a444b4 to
9f0517e
Compare
Graph note (no action implied — a maintainer has already reviewed this thread). Our triage graph places this PR in a complex with 1 related pull request ( Full neighbourhood: https://hermes-triage.gottz.de/?node=65589 This note exists so the relationship stays discoverable from the thread itself. |
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. #65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes #86296 Fixes #92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies #72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram #86296 / #65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. #65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes #86296 Fixes #92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies #72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram #86296 / #65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. #65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes #86296 Fixes #92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies #72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram #86296 / #65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. #65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes #86296 Fixes #92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies #72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram #86296 / #65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. #65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes #86296 Fixes #92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies #72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram #86296 / #65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
|
Thanks @elphamale — Merged via #101250 (11f932c) on current main. Your commits from this PR were cherry-picked onto the salvage branch with your git authorship preserved, so the credit is yours in Closing this PR since the work is now on main. |
…ugh the adapter auth check Under `multiplex_profiles` the primary adapter's message handler is a profile closure, so the Telegram inline-button gate (and the early message prefilter) cannot recover the runner via `_message_handler.__self__` and fell to env-only auth. NousResearch#65589 made the gate prefer the injected `_authorization_check`, but `_make_adapter_auth_check` built a bare `(user_id, chat_type, chat_id)` source: never route-stamped, never `is_bot`. - `_make_adapter_auth_check`: for the shared primary adapter under multiplex, mirror the inbound message path exactly — stamp the `profile_routes` match so the routed profile's pairing store is consulted, and authorize under the TRANSPORT home via `_is_user_authorized_for_source` (same split as `_make_default_profile_message_handler`, 2afed50). A rejected route fails closed like the ingress gate. Retain the receiving adapter as `_transport_adapter_ref` so config.yaml policy reads stay on it. Accept `is_bot` / `thread_id` keywords. (NousResearch#86296) - `BasePlatformAdapter._is_sender_authorized`: forward `is_bot` / `thread_id` as keywords only when set, so legacy 3-positional callbacks keep working. - Telegram `_source_from_message_for_auth` carries `from_user.is_bot`; the prefilter forwards it so `TELEGRAM_ALLOW_BOTS=mentions|all` is honored at the early gate under multiplex. (NousResearch#92840) - Telegram `_should_pass_unauthorized_dm_for_pairing`: same `__self__` introspection class — fall back to the injected `gateway_runner` and the adapter's owner profile. Fixes NousResearch#86296 Fixes NousResearch#92840 Co-authored-by: PRATHAMESH75 <118293218+PRATHAMESH75@users.noreply.github.com> Co-authored-by: Ahmett101 <297889955+Ahmett101@users.noreply.github.com>
… profile check; gate reads never fall through to os.environ
`SlackAdapter._is_interactive_user_authorized` (approval / slash-confirm /
clarify Block Kit clicks) and the early pre-fetch gate in the message
handler recovered the runner via `_message_handler.__self__`, which is
None on a multiplexed adapter (closure handler) — so both fell to env-only
auth. The fallback read `SLACK_ALLOW_ALL_USERS` raw from `os.environ` and
its `_env` helper fell through to `os.environ` on a scoped miss: the
DEFAULT profile's allow-all flag / allowlist authorized callers on every
other profile's bot.
- Prefer the wired `set_authorization_check` callback (profile-bound
`_make_adapter_auth_check`) at both sites; keep `__self__` introspection
only for adapters wired without one.
- Env-only fallback reads go through `authz_mixin._platform_gate_env`
(scoped miss under multiplex → "", never os.environ); drop the raw
`os.getenv("SLACK_ALLOW_ALL_USERS")` pre-read.
Reapplies NousResearch#72657 onto current main (original commit carried a bot
co-author trailer). Same class as Telegram NousResearch#86296 / NousResearch#65589.
Co-authored-by: MilaArtyNew <261982280+MilaArtyNew@users.noreply.github.com>
Problem
TelegramAdapter._is_callback_user_authorized— the gate for inline-button callers (exec-approval buttons, and the guest-mode caller gate in #56476 once that lands) — resolves the gateway's authorization chain by introspecting_message_handler.__self__.For a secondary multiplexed adapter, the message handler is a per-profile closure built by
GatewayRunner._make_profile_message_handler, which has no__self__. The introspection silently resolves to nothing and the method falls through to its env-only fallback — which consults onlyTELEGRAM_ALLOWED_USERS/GATEWAY_ALLOW_ALL_USERSand knows nothing about config-file allowlists or the pairing store. Every button caller authorized via those is denied on that profile. The failure direction is closed (deny, not allow), so it's not an escalation — but it makes button auth structurally broken on multiplexed profiles, and it's the same introspection gap #59857's review flagged for the admin-tier check.Fix
Prefer the auth callback
GatewayRunneralready injects at adapter-connection time viaset_authorization_check— registered for primary and multiplexed adapters alike, delegating to the full_is_user_authorizedchain (env allowlists, group allowlists, pairing store, allow-all flags). The__self__introspection and the fail-closed env fallback are kept, in that order, for adapters wired without the injected check (bare-adapter embedding, existing tests). Same resolution pattern #59857 uses for the admin-tier gate.Decision-equivalence note: the injected callback carries
(user_id, chat_type, chat_id)but notuser_name/thread_id; verifiedis_authorized()never consults either, so nothing is lost relative to the introspection path's fullerSessionSource.Tests
TestCallbackAuthPrefersInjectedCheck(3 tests): a closure-handler adapter (multiplexed shape) with the injected check registered is decided by that check rather than the env fallback; an injected deny wins over an env allowlist entry; a raising injected check degrades to the env fallback instead of crashing the button. The two behavior-asserting tests fail against the previous code. Full Telegram gateway sweep (48 files, 1071 tests): all passing. The 5 pre-existing fail-closed fallback tests (#24457) are unchanged and still pass.