Repository navigation
fix(gateway): scope the remaining adapter authz env reads (qqbot, teams, email) - #102752
salch-cred wants to merge 3 commits into
Conversation
…ms, email) Under gateway.multiplex_profiles, os.environ holds the DEFAULT profile's authorization values; a secondary profile's .env lives only in its secret scope (installed per-turn by _profile_runtime_scope). Three adapter- internal authorization gates still read the allow-all / allowlist flags with bare os.getenv, so a secondary profile inherited the DEFAULT profile's opt-in — the NousResearch#72348 / NousResearch#93522 / NousResearch#93705 leak class, sibling sites of the already-fixed gateway choke point (_auth_env) and the Matrix fix (NousResearch#94657, open): - qqbot _open_dm_opted_in (gateway/platforms/qqbot/adapter.py): read GATEWAY_ALLOW_ALL_USERS via _resolve_qq_secret. A secondary profile with dm_policy=open forwarded DMs at intake when only the DEFAULT profile had opted in. - teams _on_card_action (plugins/platforms/teams/adapter.py): read TEAMS_ALLOWED_USERS / TEAMS_ALLOW_ALL_USERS via _get_scoped_secret. This gate is the ONLY one before resolve_gateway_approval executes, so the leaked opt-in authorized any clicker to approve dangerous commands on the secondary profile (the exact attack the gate was added to stop, reading from the wrong source). - email _allow_all_senders / _allowlist_in_effect / dispatch guard (plugins/platforms/email/adapter.py): read GATEWAY_ALLOW_ALL_USERS / GATEWAY_ALLOWED_USERS via _get_secret. The leaked allow-all skipped the From:-authentication gate (GHSA-rxqh-5572-8m77 class) and the dispatch allowlist guard on the secondary profile. All three adapters already had the correct scoped helper in-file; the authorization reads just were not routed through it. A scoped miss now returns the default (fail closed); the unscoped default-profile path keeps the os.environ read. Tests: tests/gateway/test_adapter_authz_env_scope.py drives the real intake / card-action / dispatch paths. On current main, 7 of the tests reproduce the leak (secondary profile authorized by the DEFAULT profile's env); with the fix, 13/13 pass and the existing qqbot/email/authz scope suites stay green (54/54 across the cluster).
Related: #77548 (open) also scopes |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 5d595b98cd2a8733e40196c32123860afb04640c against current main / base 63279301bcbdc185c1b07b98a9312eb0c862f26d, including the full 4-file diff, the scope helpers they route through, the real intake/card-action/dispatch regressions, exact-head CI/Docker/Nix, the three-commit history, and the live overlapping PR/issue graph.
The security mechanism in these hunks is correct: under multiplexing, authorization policy cannot borrow the DEFAULT profile's process environment. _resolve_qq_secret, _get_scoped_secret, and Email's _get_secret are scope-authoritative on a named-profile scope, a scoped miss resolves to the deny/default value, and the unscoped/default-profile path retains the legacy os.environ fallback. The new tests are also materially better than helper-only probes: QQ drives _is_dm_intake_allowed, Teams proves an unauthorized secondary-profile click never reaches resolve_gateway_approval, and Email exercises the adapter gates. Exact-head CI is green (CI 33847149031, Docker 33847148345, Nix 33847148355).
There is, however, a merge-blocking ownership/interlock problem: all three production hunks already have earlier open implementations, and the Teams overlap is architecturally stronger than this PR's version. This needs to be reconciled before this becomes a merge owner.
-
QQ is an exact duplicate of earlier work. #77548 (Drexuxux, opened Aug 3) already changes
QQAdapter._open_dm_opted_in()from rawos.getenv("GATEWAY_ALLOW_ALL_USERS")to_resolve_qq_secret(...)and carries scoped-miss/scoped-opt-in/unscoped regressions. #88559 also contains the same QQ substitution. The current hunk is a clean current-main reimplementation, but it is not new ownership. If this PR intentionally supersedes the stale/conflicted QQ work, the PR needs to say so explicitly and preserve the #77548 lineage/credit; otherwise drop the QQ production hunk and move the stronger real-intake test to the existing owner. -
Email's three substitutions are already in #88559 (FirmaSpring, opened Aug 17). That PR has broader unrelated work and has had its own blockers, but its Email hunk is the same
_get_secret("GATEWAY_ALLOW_ALL_USERS" / "GATEWAY_ALLOWED_USERS")correction at_allow_all_senders,_allowlist_in_effect, and the dispatch guard. I previously found those Email substitutions themselves sound. Same rule: either explicitly establish this as a current-main salvage/supersession of the Email slice with attribution, or do not silently create a second owner for the same lines. -
Teams must interlock with #93516 (shanthans-es), not race it. #93516 already scopes the fallback reads through
_get_scoped_secret, but more importantly it removes the bespokeTEAMS_ALLOWED_USERSraw-AAD-ID authorization path and routes approval-card authorization throughGatewayRunner._is_user_authorized(), with correct group/channelchat_typepropagation and a scope-aware env fallback only when the runner is unavailable. That is the other side of this shape: profile-local policy is necessary, but the approval sink also needs the same identity normalization/pairing-store authority as ordinary message intake. If this smaller Teams hunk lands first, #93516 collides on_on_card_action; if #93516 lands first, this hunk is obsolete and the new tests need to target_is_card_action_authorized/its fallback instead. I would sequence #93516 first or remove the Teams production hunk here and port this PR's excellentresolve_gateway_approvalnon-execution regression into #93516.
The wider lineage supports treating this as one ambient-authority defect class with distinct sink owners, not three fresh discoveries: merged #93639 fixed scoped adapter authorization for Weixin/Yuanbao/Signal/WeCom (with salvaged contributor provenance); closed #93705 tracked the shared authz_mixin scoped-miss fallback; open #94657 owns the Matrix invite/prompt-reactor sibling; #77548 owns the self-gating QQ/Weixin/WhatsApp open-DM slice. The invariant is consistent across them: selected profile identity -> profile-local authorization source -> authorization decision -> privileged sink, with no process-global fallback after profile selection. This PR's tests strengthen that invariant, but the production changes need a single, explicit ownership topology.
One separate release-proof gate: the current head is exact-green, but the history is not every-commit green. a342a12b2abc30f2770de518e95dd2485c5a7bf0 has no PR workflow receipts; bc1096de8a4f6e01d7553afc09203896698a08fc has CI failure 33845573762 solely at the missing review-label gate (the substantive Python tests/lints/OSV/OS-specific jobs were green; Docker and Nix were green). Both are no-op "retrigger CI" commits with the same tree as base 63279301..., so they add no implementation value. Dropping/squashing those no-op commits would leave the actual fix commit as the proof object and remove the historical red/missing-receipt ambiguity.
So: mechanism sound; tests useful; not ready to merge as an independent owner yet. Reconcile the QQ/Email supersession and attribution explicitly, sequence Teams behind/into #93516 rather than overwriting its shared-auth design, and leave a clean every-commit proof chain. The security work here is worth preserving — especially the real sink-level regressions — just with the repository topology made as rigorous as the code.
crazyief
left a comment
There was a problem hiding this comment.
Lab (infosecvm 8GB): ruff clean; tests/gateway/test_adapter_authz_env_scope.py → 13 passed. Scoping qqbot/teams/email authz env reads is the right leftover. Not an approval.
| # Multiplex inactive, no scope: legacy os.environ behavior preserved. | ||
| monkeypatch.setenv("GATEWAY_ALLOW_ALL_USERS", "true") | ||
| adapter = self._adapter() | ||
| assert adapter._allow_all_senders() is True |
There was a problem hiding this comment.
Unscoped single-profile keeping os.environ is the load-bearing legacy path. Keep it.
| assert adapter._allow_all_senders() is True | |
| adapter = self._adapter() | |
| assert adapter._allow_all_senders() is True |
Summary
Under
gateway.multiplex_profiles,os.environholds the DEFAULT profile's authorization values; a secondary profile's.envlives only in its secret scope (installed per-turn by_profile_runtime_scope). Three adapter-internal authorization gates still read allow-all / allowlist flags with bareos.getenv, so a secondary profile inherited the DEFAULT profile's opt-in — the #72348 / #93522 / #93705 leak class, sibling sites of the already-fixed gateway choke point (_auth_env/_platform_gate_env) and the Matrix fix (#94657):gateway/platforms/qqbot/adapter.py_open_dm_opted_inGATEWAY_ALLOW_ALL_USERSviaos.getenvdm_policy: openforwarded DMs at intake when only the DEFAULT profile opted inplugins/platforms/teams/adapter.py_on_card_actionTEAMS_ALLOWED_USERS/TEAMS_ALLOW_ALL_USERSviaos.getenvresolve_gateway_approvalexecutes — the leaked opt-in authorized any clicker to approve dangerous commands (the exact attack the gate was added to stop, reading from the wrong source)plugins/platforms/email/adapter.py_allow_all_senders/_allowlist_in_effect/ dispatch guardGATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERSviaos.getenvAll three adapters already had the correct scoped helper in-file (
_resolve_qq_secret,_get_scoped_secret,_get_secret); the authorization reads just were not routed through it. A scoped miss now returns the default (fail closed); the unscoped default-profile path keeps theos.environread (there it IS the profile's own value). Single-profile deployments are unchanged.Reproduction on current main
tests/gateway/test_adapter_authz_env_scope.pydrives the real intake / card-action / dispatch paths. On currentmain(commit 6327930), 7 of the 13 tests fail — e.g. withGATEWAY_ALLOW_ALL_USERS=trueonly inos.environand an empty secondary scope:QQAdapter._is_dm_intake_allowed(user-1)returnsTrue(leak)resolve_gateway_approval(leak)EmailAdapter._allow_all_senders()returnsTrue→ From-auth skipped (leak)With the fix: 13/13 pass, and the existing scope/authz suites stay green (54/54 across
test_qqbot_scope_paths,test_qqbot_credential_isolation,test_email_secret_scope,test_multiplex_profile_authz, and the new file).Pre-existing failure, not from this PR:
tests/gateway/test_teams_dotenv_isolation.py::test_sdk_import_path_suppresses_dotenvfails identically with and without these changes on this host (SDK-stub/env-dependent).Notes for reviewers
_platform_gate_env.adapter.py:500/541) and LINE (adapter.py:741-748) snapshot allowlists into__init__attributes viaos.getenv— construction happens inside_profile_runtime_scopefor secondary profiles so the value is correct at build time, but the snapshot is frozen for the process lifetime. Different tradeoff (stale-vs-live), sibling-of rather than same-bug; happy to follow up separately if maintainers prefer live reads.git diffis 3 hunks in adapters (each reusing the file's own existing scoped helper) + one new test file.