fix(gateway): route platform authorization reads through the profile secret scope - #93605
aniruddhaadak80 wants to merge 1 commit into
Conversation
…secret scope Under gateway.multiplex_profiles, secondary profiles are constructed inside _profile_runtime_scope and their .env lives in the profile's secret scope - gateway/run.py explicitly does NOT mutate os.environ with it. Four adapters still read their AUTHORIZATION config via raw os.getenv, so every secondary profile either (a) silently missed its own env-only allowlists/policies (fail-closed: all DMs dropped at intake) or (b) inherited the default profile's GATEWAY_ALLOW_ALL_USERS=true / allowlists from the shared process env (fail-open admissions): - weixin.py: WEIXIN_DM_POLICY / WEIXIN_ALLOWED_USERS / WEIXIN_GROUP_ALLOWED_USERS / WEIXIN_ALLOW_ALL_USERS + GATEWAY_ALLOW_ALL_USERS in _open_dm_opted_in - yuanbao.py: YUANBAO_DM_POLICY / DM_ALLOW_FROM / GROUP_POLICY / GROUP_ALLOW_FROM / ALLOW_ALL_USERS (new _yb_secret helper; AccessPolicy hard-gates intake) - signal.py: SIGNAL_GROUP_ALLOWED_USERS / SIGNAL_ALLOWED_USERS (new _sig_secret helper; empty scoped group list previously meant "drop all groups" silently) - wecom/adapter.py: WECOM_DM_POLICY / WECOM_ALLOWED_USERS / WECOM_GROUP_POLICY / WECOM_ALLOW_ALL_USERS + GATEWAY_ALLOW_ALL_USERS - while credentials one line above already used _get_scoped_secret - gateway/run.py::_own_policy_open_startup_violation: the open-policy startup guard validated GATEWAY_ALLOW_ALL_USERS via raw os.getenv even though its sibling dm/group reads already used the scoped _getenv All reads now go through the canonical fail-closed scoped shape QQ's _resolve_qq_secret already used (scope hit wins; unscoped single-profile callers keep legacy os.environ behavior). Regression suite drives the real scope contextvar across all four helpers plus the admission gates and the startup guard, asserting both directions: profile values are visible under multiplex, default-profile values never leak. Fixes NousResearch#93522
Related: #88559 repairs the shared authorization-mixin path; this PR covers remaining adapter-local admission reads and the startup check for the same multiplexed-profile isolation family. |
trevorgordon981
left a comment
There was a problem hiding this comment.
Reviewed from a Hermes audit pass. Correct direction — routing authorization reads through the per-profile scope is exactly what #93522 needs, and routing it through _get_scoped_secret / _getenv is the right canonical shape. One fail-open concern inline, plus a merge-coordination note:
Cross-PR: this substantially overlaps #93545 (same issue #93522, same files) but diverges in approach — #93545 uses the canonical gateway.authz_mixin._platform_gate_env and explicitly leaves weixin/wecom _open_dm_opted_in and Signal unscoped (deferring to the 1,091-commit-stale #88559), whereas this PR fully closes those but invents per-file _sig_secret/_yb_secret helpers instead of routing through a shared one. Merging both will conflict. Worth reconciling with the other author which lands, and confirming #93545's base isn't divergent — the canonical helper it cites isn't present in the current tree.
| try: | ||
| val = get_secret(name, default) | ||
| except UnscopedSecretError: | ||
| val = os.getenv(name) |
There was a problem hiding this comment.
This except-fallback fails OPEN to os.environ. For an authorization gate, an UnscopedSecretError resolving to the global/default-profile env is the exact cross-profile leak #93522 describes. It is only safe if every secondary profile is guaranteed to construct inside _profile_runtime_scope; if that invariant ever breaks, a secondary profile silently inherits the default profile's allow-all / allowlists. Safer to fall back only when multiplex is genuinely disabled (an explicit primary-profile path) rather than on a blanket exception swallow. Same pattern applies to _yb_secret (yuanbao.py L1280) and the wecom helper.
|
Merged via #93639: your commit was cherry-picked on top of #93545's base (authorship preserved) — your PR was the only one covering signal and the weixin/wecom allow-all gates. Thanks @aniruddhaadak80! |
What does this PR do?
Fixes a multiplexed-profile isolation break in platform authorization (issue #93522). Under
gateway.multiplex_profiles, every secondary profile is constructed inside_profile_runtime_scopeand its.envlives in that profile's secret scope —gateway/run.pyexplicitly does not mutateos.environwith it. Four adapters still read their authorization config via rawos.getenv, producing two failure modes for any non-default profile using them:WEIXIN_ALLOWED_USERS=alice(env-only) → raw getenv misses it → intake drops every DM/group message with no error, before the runner-level scoped authz check ever runs.GATEWAY_ALLOW_ALL_USERS=true/ allowlists live in the shared process env and leak into B's admission gates.What changed
gateway/platforms/weixin.pyWEIXIN_DM_POLICY,WEIXIN_ALLOWED_USERS,WEIXIN_GROUP_ALLOWED_USERS,WEIXIN_ALLOW_ALL_USERS,GATEWAY_ALLOW_ALL_USERS(via existing_wx_secret)gateway/platforms/yuanbao.py_yb_secret()helper;YUANBAO_DM_POLICY,YUANBAO_DM_ALLOW_FROM,YUANBAO_GROUP_POLICY,YUANBAO_GROUP_ALLOW_FROM;AccessPolicy._open_dm_opted_ingateway/platforms/signal.py_sig_secret()helper;SIGNAL_GROUP_ALLOWED_USERS,SIGNAL_ALLOWED_USERSplugins/platforms/wecom/adapter.py_get_scoped_secretextended to the authz reads beside the already-scoped credential readsgateway/run.py::_own_policy_open_startup_violationGATEWAY_ALLOW_ALL_USERSnow via scoped_getenv, matching its sibling dm/group readsAll reads use the canonical fail-closed shape QQ's
_resolve_qq_secretalready established: scope hit wins; with no scope installed (single-profile deployments), legacyos.environbehavior is preserved exactly. This is the authorization-axis counterpart of #59662/#59739/#86905's credential work, per AGENTS.md §Profiles rule 7 which names{PLATFORM}_ALLOW_ALL_USERS/{PLATFORM}_ALLOWED_USERS/GATEWAY_ALLOW_ALL_USERSexplicitly as must-be-scope-aware.Related Issue
Fixes #93522
Type of Change
Changes Made
_yb_secret,_sig_secret) mirroring the canonical shape used across ~16 other call sites. No behavior change for single-profile deployments (covered by tests).tests/gateway/test_platform_authz_scope.py— 17 regression tests driving the real scope contextvar:os.environ;_MULTIPLEX_ACTIVE=True), a scoped miss returns the default and never falls through to process env (the leak direction), for all four adapters;WeixinAdapter/ wecom_open_dm_opted_inand yuanbaoAccessPolicygates stay closed when only the default profile'sGATEWAY_ALLOW_ALL_USERS=trueis present, and open on their own scope's opt-in;os.environhas it.How to Test
uv run python -m pytest tests/gateway/test_platform_authz_scope.py -q→ 17 passed.uv run python -m pytest tests/gateway/test_multiplex_profile_authz.py -q→ 5 passed.ruff check gateway/platforms/weixin.py gateway/platforms/yuanbao.py gateway/platforms/signal.py plugins/platforms/wecom/adapter.py gateway/run.py tests/gateway/test_platform_authz_scope.py→ clean.WEIXIN_ALLOWED_USERS=<id>in B's.envonly → B now answers allowlisted senders (previously silent); setGATEWAY_ALLOW_ALL_USERS=truein A only → B still denies strangers (previously admitted).Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
Screenshots / Logs
N/A