fix(gateway): scope multiplex-profile authorization reads (weixin/yuanbao/wecom) - #93545
fix(gateway): scope multiplex-profile authorization reads (weixin/yuanbao/wecom)#93545chelsealong wants to merge 1 commit into
Conversation
…nbao/wecom) WEIXIN_DM_POLICY/ALLOWED_USERS/GROUP_ALLOWED_USERS, YUANBAO's equivalents, WECOM_DM_POLICY/ALLOWED_USERS/GROUP_POLICY, and the startup guard's GATEWAY_ALLOW_ALL_USERS check still read raw os.getenv at adapter construction time. Under gateway.multiplex_profiles that reads the process env instead of the per-profile secret scope, so a secondary profile either silently drops every DM (its own env-only allowlist is invisible) or inherits the default profile's allow-all/allowlist config. Route these reads through the existing scoped helpers (_wx_secret, _get_scoped_secret, gateway.authz_mixin._platform_gate_env, and gateway.config._getenv) already used for the adjacent credential reads in the same adapters. Fixes NousResearch#93522.
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 25920a8f075f1f05ebfcb97a146623a156fbfef8 against exact/current main ec44116d596d798d6cb230825f1a635bc6dd38e9.
The code changed in this PR is directionally and mechanically correct: the Weixin/WeCom construction-time reads now use their existing scoped readers, Yuanbao uses the canonical _platform_gate_env, and the startup guard now uses scoped _getenv. The regressions exercise real adapter construction/authorization state (not just helper aliases) in both directions: a secondary profile sees its own scoped value, and a scoped miss does not borrow the default profile's process env. Exact-head CI 32689969937, Docker 32689969471, and Nix 32689969464 are all green; the Python test/lint/e2e jobs executed successfully.
There is still one merge blocker in the publication/topology, though: this head says Fixes #93522, but it does not actually close all of #93522 on current main.
#93522 explicitly includes the fail-open intake/authorization reads in Weixin, Signal, and WeCom. Those sites are still live on main after applying this PR's delta:
gateway/platforms/weixin.py::_open_dm_opted_in()still reads bothGATEWAY_ALLOW_ALL_USERSandWEIXIN_ALLOW_ALL_USERSthrough rawos.getenv. A default-profile allow-all therefore still leaks into a scoped secondary profile whendm_policy == "open".plugins/platforms/wecom/adapter.py::_open_dm_opted_in()has the same raw global/platform allow-all reads and the same cross-profile admission failure.gateway/platforms/signal.py::__init__()still readsSIGNAL_GROUP_ALLOWED_USERS,SIGNAL_REQUIRE_MENTION, andSIGNAL_ALLOWED_USERSthrough rawos.getenv; the latter still defaults to"*", which is especially important because Signal's reaction hook runs before the central auth gate.
The PR body points at #88559 for those remaining reads, and that relationship is real, but #88559 is not branch truth: its current head is 4102f30bb602f2a7d5ed9d5c965fc0b563f20a16, it is 1,091 commits behind current main, and it directly overlaps this PR (including Yuanbao's _open_dm_opted_in hunk plus the same Weixin/WeCom files). Its current source does contain the missing Weixin/WeCom opt-in and Signal scope fixes, so it is an overlapping predecessor/adjacent owner, not something whose unmerged state can satisfy Fixes #93522.
Required before this PR should auto-close #93522: either port the remaining #93522 intake sites from #88559 onto this current-main head (preserving @FirmaSpring's provenance for that work), or remove the Fixes #93522 claim and make the dependency/merge order explicit until those remaining sites land. Given #88559's age/divergence and the direct same-class overlap, the cleaner topology is one current-main delivery owner for #93522 rather than closing the issue while the fail-open half is still reachable.
The wider invariant is the same one established by #59739 (Slack token cross-profile bleed) and #69090 (Matrix recovery-key bleed): profile-local authorization/capability state must remain profile-local at every consuming boundary, not merely at the first constructor read. This PR closes the constructor/startup side well; the remaining _open_dm_opted_in/Signal pre-auth consumers are the other side of that shape.
So: no new defect found in the hunks this head changes, and the exact-head evidence is strong, but I would not merge it with Fixes #93522 until the remaining live authorization consumers are either composed here or explicitly left under a non-closed dependent owner.
|
No code change here — this PR's own hunks are correct as reviewed. The blocker was scope-vs-claim: |
|
Merged via #93639 with your commit cherry-picked as the base (authorship preserved — you were the earliest submitter). #93605's signal coverage and allow-all gates were layered on top. Thanks @chelsealong! |
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in #76664/#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per #93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
…iplexing WeComAdapter.__init__ read WECOM_BOT_ID via a raw os.getenv() call, while the immediately adjacent line for WECOM_SECRET already used the module's _get_scoped_secret() helper. Under gateway.multiplex_profiles, a secondary profile's adapter is constructed inside a scoped context where os.environ still holds the DEFAULT profile's env-bridge output -- so a secondary profile's bot would silently connect using the default profile's bot_id while (correctly) using its own secret, or vice versa on a scope miss. Switch the bot_id read to _get_scoped_secret(), matching the sibling _secret/_dm_policy/_group_policy/allow_from reads in the same __init__ that were already migrated in NousResearch#76664/NousResearch#93545. _standalone_send's out-of-process fallback branch constructs a fresh WeComAdapter(pconfig) and therefore inherits this fix automatically -- no separate change needed there. Adds two regression tests to the existing TestWeComAdapterAuthzScope class (already covering dm_policy/allow_from scoping per NousResearch#93522), mirroring its established fixture/assertion style. Mutation-verified: both fail against the pre-fix code (asserting the default profile's bot_id leaks into a secondary profile's scope) and pass with the fix.
Summary
Under
gateway.multiplex_profiles, several platform adapters read their authorization configuration (DM/group policies, user allowlists, allow-all opt-ins) through rawos.getenvat adapter-construction time, bypassing the per-profile secret scope installed by_profile_runtime_scope. The credential reads in these same adapters were already migrated to scoped helpers (e.g._wx_secret,_get_scoped_secret); the adjacent authorization reads were missed. This produces two failure modes for any non-default profile:WEIXIN_ALLOWED_USERS(etc.) is invisible, so the intake gate drops every DM/group message with no error.GATEWAY_ALLOW_ALL_USERS=true/ allowlists live in the shared process env and leak into a secondary profile, admitting senders it never allowed.Addresses part of #93522 — the construction-time authorization reads for Weixin/Yuanbao/WeCom and the startup guard. Does not close #93522. The remaining fail-open consumers named in that issue are still live on
mainafter this PR:gateway/platforms/weixin.py::_open_dm_opted_in()andplugins/platforms/wecom/adapter.py::_open_dm_opted_in()still readGATEWAY_ALLOW_ALL_USERS/*_ALLOW_ALL_USERSvia rawos.getenv, andgateway/platforms/signal.py::__init__()still readsSIGNAL_GROUP_ALLOWED_USERS/SIGNAL_REQUIRE_MENTION/SIGNAL_ALLOWED_USERSunscoped. Those sites exist, unmerged, on #88559 (currently 1,091 commits behindmain). #93522 should stay open until those sites are ported onto currentmain(here or in a follow-up) or #88559 lands.What changed
gateway/platforms/weixin.py:WEIXIN_DM_POLICY/WEIXIN_GROUP_POLICY/WEIXIN_ALLOWED_USERS/WEIXIN_GROUP_ALLOWED_USERSnow read through the existing_wx_secretscoped helper (already used for the adjacent credential reads in this file).gateway/platforms/yuanbao.py:YUANBAO_DM_POLICY/YUANBAO_DM_ALLOW_FROM/YUANBAO_GROUP_POLICY/YUANBAO_GROUP_ALLOW_FROM, plusAccessPolicy._open_dm_opted_in'sGATEWAY_ALLOW_ALL_USERS/YUANBAO_ALLOW_ALL_USERSreads, now go throughgateway.authz_mixin._platform_gate_env(the canonical scoped gate-env reader used elsewhere in the gateway).plugins/platforms/wecom/adapter.py:WECOM_DM_POLICY/WECOM_ALLOWED_USERS/WECOM_GROUP_POLICYnow read through the existing_get_scoped_secrethelper (already used forWECOM_SECRETin the same__init__).gateway/run.py:_own_policy_open_startup_violation'sGATEWAY_ALLOW_ALL_USERScheck now uses the already-imported scoped_getenv(the siblingdm_env/group_env/allow_all_envreads two lines above it were already scoped; only this one was still raw).Not touched (out of scope for this PR, see dependency note above): weixin/wecom
_open_dm_opted_inand Signal's construction-time allowlist/mention reads. The previously-open PR #88559 covers those, but is 1,091 commits behindmainand unmerged, so it cannot be relied on to satisfy #93522 as-is.Test plan
Added regression tests proving the scoped-vs-leaked behavior (each fails on the pre-fix code, verified via
git checkout HEAD~1 -- <file>):tests/gateway/test_weixin_secret_scope.py— newTestWeixinAdapterAuthzScopeclass: a scopeddm_policy/allowlist is honored, and a scoped miss does not admit the default profile's env-only allowlist.tests/gateway/test_wecom.py— newTestWeComAdapterAuthzScopeclass: same shape for WeCom.tests/gateway/test_yuanbao_secret_scope.py— new file:AccessPolicyconstruction and_open_dm_opted_inscoping.tests/gateway/test_multiplex_profile_authz.py— newtest_startup_guard_gateway_allow_all_reads_scope_not_environ: the startup guard's allow-all check respects the scope in both directions (no leak-in, and a legitimate scoped opt-in is honored).Also ran via the canonical runner:
ruff checkon all changed files passed.python -m py_compileon all changed files passed.git diff --checkpassed.AI-assistance disclosure
This fix, its tests, and this description were prepared by an autonomous Claude Code agent, and reviewed against the repo's existing scoped-secret patterns (
_wx_secret,_get_scoped_secret,_platform_gate_env,gateway.config._getenv) before being applied.