fix(gateway): scope authz allowlist reads to the routed profile - #61985
fix(gateway): scope authz allowlist reads to the routed profile#61985rlaehddus302 wants to merge 5 commits into
Conversation
|
suggesting changes Security evidence:
Signed: GPT-5.5-xhigh in Codex |
|
Fixed in e1300322d — moved the |
|
fully addressed Security evidence:
I reviewed a run-owned patch replay against current GitHub Signed: GPT-5.5-xhigh in Codex |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the request-time allowlist leak; current main still has the raw os.getenv reads this targets (gateway/authz_mixin.py:459-465, :701-707).
Problems
- The new scope is selected from
source.profile(gateway/authz_mixin.py:361), but secondary adapters register_make_adapter_auth_check(adapter.platform)without their profile (gateway/run.py:8591). The callback creates a profile-lessSessionSource(gateway/run.py:8788-8794), so Slack/Discord external-context authorization still resolves the active profile rather than the secondary adapter profile. - The diff adds no regression coverage for scoped
.envallowlists or the unauthorized-DM fallback. Existingtests/gateway/test_multiplex_profile_authz.pycovers adapter policy selection, not profile secret-scope reads.
Suggested changes
- Bind
profile_nameinto the secondary adapter authorization callback and place it on the callback'sSessionSource. - Add positive/negative two-profile tests for direct authz, unauthorized-DM behavior, and the adapter callback path.
Automated hermes-sweeper review.
| @@ -314,6 +358,19 @@ def _is_user_authorized(self, source: SessionSource) -> bool: | |||
| ): | |||
| return True | |||
|
|
|||
| with self._profile_scope_for(source.profile): | |||
There was a problem hiding this comment.
This scopes correctly only when the caller stamps source.profile. Secondary adapters register _make_adapter_auth_check(adapter.platform) at gateway/run.py:8591; that callback constructs a profile-less SessionSource at gateway/run.py:8788-8794, so its Slack/Discord context checks still select the active profile. Bind profile_name into that callback and set it on its source.
There was a problem hiding this comment.
Fixed — bound profile_name into _make_adapter_auth_check and stamped it onto the callback's SessionSource (gateway/run.py:8611, 8785-8814), so secondary-adapter auth callbacks now resolve their own multiplex profile instead of the active one. Added regression tests covering both the secondary-profile callback (stamps its own profile) and the primary/no-profile callback (still resolves the active profile). All 83 authz/telegram-authz tests pass.
Commit: f4d9be5eb
|
suggesting changes Security evidence:
Review setup: I reviewed a run-owned patch replay against current GitHub Signed: GPT-5.6-sol-xhigh in Codex |
…r multiplex_profiles _is_user_authorized and _get_unauthorized_dm_behavior read *_ALLOWED_USERS, *_ALLOW_ALL_USERS, and GATEWAY_ALLOWED_USERS via raw os.getenv, which always resolves to the process-global environment (the profile that started the gateway). Under gateway.multiplex_profiles, a secondary profile's own .env values are never mutated into os.environ (by design, to keep credentials isolated), so its allowlist configuration was silently ignored in favor of the active profile's. Add a small profile-scope helper (_profile_scope_for, mirroring the existing _pairing_store_for isolation pattern) and a scope-aware _getenv (mirroring gateway.config._getenv), then split both methods so the env-reading portion runs inside the routed profile's secret scope. Single-profile gateways are unaffected: _profile_scope_for is a no-op (nullcontext) when multiplex_profiles is off, so _getenv falls through to the same os.environ read as before. Verified against the real (unmodified otherwise) code path: with two profiles configured with distinct MATTERMOST_ALLOWED_USERS values, each profile's allowlist now isolates correctly, and the non-multiplex path is unchanged.
The lazy 'from gateway.run import logger' import stayed in the outer _is_user_authorized() frame after the previous commit split its body into _is_user_authorized_scoped(). The legacy TELEGRAM_GROUP_ALLOWED_USERS chat-ID compat branch (moved into the scoped helper) still calls logger.warning(...), which raised NameError since that name was never imported in the new frame. Move the import to _is_user_authorized_scoped(), the only place that now uses it. Verified the legacy chat-ID branch authorizes a listed chat, denies an unlisted one, and no longer raises. Reported by automated review (CodeRabbit/Codex) on PR NousResearch#61985.
_make_adapter_auth_check built a profile-less SessionSource for secondary multiplex adapters' external-context authorization (Slack/ Discord thread-reply sender checks), so it kept resolving the active profile's allowlist scope instead of the adapter's own profile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…back The shared primary Slack/Discord adapter registers its fetched-context authorization callback without a profile, so a channel routed elsewhere by `gateway.profile_routes` was checked against the active profile's allowlist: users allowed only by the routed profile were marked `[unverified]`, while default-profile users were treated as verified. Resolve the route via `_profile_name_for_source` when no profile is bound. Route matching is conjunctive, so guild- and thread-scoped routes only match when that context is supplied — thread it from the Slack/Discord fetch paths through `_is_sender_authorized`. The context is forwarded only to callbacks marked `_accepts_route_ctx`, so the existing 3-arg callbacks keep the legacy call unchanged. An explicitly bound `profile_name` (secondary adapters) still wins over routes, and `_profile_name_for_source` returns None when multiplexing is off or no route matches, leaving unrouted gateways unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nv reads `_auth_env` fell back to `os.getenv` whenever the profile scope did not define a var. In a multiplexer `os.environ` holds whichever profile started the process, so a routed profile that simply omits an allowlist var was authorized against the starting profile's allowlist — the cross-profile leak the scoping exists to prevent. `agent.secret_scope.get_secret` already treats an installed scope as authoritative and does not fall through to `os.environ`; mirror that rule here for the blank-value case. Outside any scope (single-profile gateways) the read stays plain `os.getenv`, unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f4d9be5 to
36ded63
Compare
Subset of PR #61985: _make_adapter_auth_check gains a profile_name parameter and secondary-profile adapters (started in _start_one_profile_adapters) bind it, so the auth callback's SessionSource resolves the routed profile's adapter and pairing store instead of silently falling back to the default profile. This is the gap left open by the #65629 merge — adapter-internal auth checks (e.g. Slack thread-context fetch) fire outside the wrapped message handler. The PR's authz_mixin.py hunks are dropped: main's _auth_env (merged via PR #65629) already covers the scoped allowlist reads they targeted.
|
Merged via PR #65700 — the The |
Subset of PR NousResearch#61985: _make_adapter_auth_check gains a profile_name parameter and secondary-profile adapters (started in _start_one_profile_adapters) bind it, so the auth callback's SessionSource resolves the routed profile's adapter and pairing store instead of silently falling back to the default profile. This is the gap left open by the NousResearch#65629 merge — adapter-internal auth checks (e.g. Slack thread-context fetch) fire outside the wrapped message handler. The PR's authz_mixin.py hunks are dropped: main's _auth_env (merged via PR NousResearch#65629) already covers the scoped allowlist reads they targeted.
… multiplex_profiles
Replace raw os.getenv("TELEGRAM_ALLOWED_USERS") reads in the Telegram
adapter's pre-filter with gateway.authz_mixin._auth_env so the served
profile's .env is consulted when its secret scope is installed. This
mirrors the gateway-layer fix in PRs NousResearch#61985/NousResearch#65629/NousResearch#65700 down to the
adapter pre-filter, fixing the Telegram mirror of issue NousResearch#72348.
See README.md for details, scope notes, and end-to-end test evidence.
Fixes NousResearch#72348 (Telegram mirror)
Subset of PR NousResearch#61985: _make_adapter_auth_check gains a profile_name parameter and secondary-profile adapters (started in _start_one_profile_adapters) bind it, so the auth callback's SessionSource resolves the routed profile's adapter and pairing store instead of silently falling back to the default profile. This is the gap left open by the NousResearch#65629 merge — adapter-internal auth checks (e.g. Slack thread-context fetch) fire outside the wrapped message handler. The PR's authz_mixin.py hunks are dropped: main's _auth_env (merged via PR NousResearch#65629) already covers the scoped allowlist reads they targeted.
Summary
_is_user_authorizedand_get_unauthorized_dm_behaviorread*_ALLOWED_USERS/*_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERSvia rawos.getenv, which only ever sees the process-global environment.gateway.multiplex_profiles, a secondary profile's.envis intentionally never merged intoos.environ(_profile_runtime_scopeonly installs a per-turn secret scope, to keep credentials isolated) — so a secondary profile's own allowlist configuration was silently ignored, and the allowlist actually enforced was whichever profile started the gateway process.authz_mixin.py's env-reading logic).Reproduction (before this fix)
Two profiles (
default,coder), each configuring a distinctMATTERMOST_ALLOWED_USERS:default/.env:MATTERMOST_ALLOWED_USERS=user-acoder/.env:MATTERMOST_ALLOWED_USERS=user-bWith multiplexing on,
user-bmessaging thecoderprofile's bot was rejected ("Unauthorized user"), whileuser-awas authorized on both bots — thecoderprofile's own allowlist was never consulted.Fix
_profile_scope_for(profile): enters the routed profile's secret scope for one authz decision, mirroring the existing_pairing_store_forper-profile isolation pattern and theif multiplex_profiles: with _profile_runtime_scope(...)guard already used elsewhere in the gateway. No-op (nullcontext) when multiplexing is off, so single-profile gateways are unaffected._getenvlocal to this module, mirroringgateway.config._getenv/hermes_cli.runtime_provider._getenv(the existing convention for this in the codebase)._is_user_authorized/_get_unauthorized_dm_behaviorso the allowlist-reading portion runs inside that scope, and replace theiros.getenvcalls with_getenv.How to test
I wasn't able to get a Python 3.11–3.13 environment running locally to run
scripts/run_tests.shend-to-end (the checkout I had available was on 3.10). Rather than add pytest coverage I couldn't actually execute against the full suite, I verified the real, unmodified code path with a standalone script that stubs onlygateway.run.logger(the one thingauthz_mixinimports from that module at call time) and exercisesGatewayAuthorizationMixin._is_user_authorizedunder a manually-installedagent.secret_scopescope per profile:All assertions pass. Happy to add this as a proper
tests/gateway/pytest module (following the existingtest_multiplex_profile_authz.py/test_multiplex_credential_isolation.pypatterns) if a maintainer wants it before merge — I only left it out of the diff because I couldn't run it against the full suite myself in this environment.Platforms tested
The changed logic is platform-agnostic (same
os.getenvpattern applied to every*_ALLOWED_USERSvar); verified withPlatform.MATTERMOSTon Linux (WSL2).Related: #57417 (fixes credential isolation; this PR addresses the separate authz-allowlist isolation gap it doesn't cover).