Repository navigation
fix(secrets): scope qqbot, email adapter, and TUI session.branch under multiplex profiles (salvage #60420, #59076, #72299) - #76665
Merged
Conversation
The QQ adapter read QQ_APP_ID, QQ_CLIENT_SECRET, the QQ_STT_* backend config and the QQ_ALLOW_ALL_USERS policy flag through raw os.getenv, bypassing the active profile secret scope. In multiplex mode a secondary profile whose secret lives in its own .env (installed as an isolated scope, not into os.environ) would silently fall back to the default/primary profile's value — the same cross-profile collision fixed for the WeChat/weixin adapter in #59662. Route these reads through a scope-aware resolver that reads the profile scope when one is installed (secondary profiles and per-turn inbound) and falls back to os.environ otherwise. The fallback is deliberate: the primary/active profile is constructed without a scope and owns os.environ, so a bare get_secret would raise UnscopedSecretError and break its startup. Mirrors gateway.config._getenv. Adds regression tests including active-profile-no-scope construction (the fail-closed case), plus scope-wins-over-environ, two-profile isolation, single-profile fallback, explicit-config precedence and STT key scoping.
…eads Review follow-up: the adapter-level resolver alone left three paths reading per-profile QQ_* values from raw os.getenv, so a secondary multiplex profile's scoped opt-in or credentials were ignored (or the primary's environ values leaked in): - gateway/authz_mixin.py: route the per-platform allow-all flag and the per-platform/group allowlist + allow-bots reads through the scope-aware gateway.config._getenv. Deployment-global GATEWAY_* reads intentionally stay on os.getenv. This makes the same fix effective for every own-policy platform, not just QQ; unscoped behavior is byte-identical to os.getenv. - gateway/run.py (_own_policy_open_startup_violation): resolve the per-platform dm/group policy and allow-all opt-in via _getenv; the secondary-profile caller already runs inside _profile_runtime_scope. - tools/send_message_tool.py (_send_qqbot): the QQ_APP_ID / QQ_CLIENT_SECRET fallbacks now honor the active profile scope. Tests: tests/gateway/test_qqbot_scope_paths.py covers all three paths end-to-end (scope wins, no environ inheritance for non-opted profiles, single-profile environ fallback unchanged); the STT suite now asserts QQ_STT_BASE_URL and QQ_STT_MODEL scoping alongside the API key. All five scoped-behavior tests fail on the previous commit and pass here.
Follow-up to the review-hardening commit: the existing cases exercised QQ_ALLOW_ALL_USERS but not the QQ_ALLOWED_USERS read at authz_mixin.py line 459, so a revert of that line to raw os.getenv would still pass. Add a scoped-allowlist DM case (scope admits the sender, environ does not) plus its isolation counterpart (a secondary scope listing a different user must not inherit the primary's environ allowlist). Both fail if line 459 reverts to os.getenv.
…ixin gate PR The cherry-picked #60420 hunks that converted gateway/authz_mixin.py are dropped here: main's _auth_env/_platform_gate_env supersede them, and the remaining authz_mixin raw-read conversions (allow-all flag + allowlists at L459/501/879-885) land in a separate PR. Until that PR flips the allow-all read to scope-authoritative semantics, the cross-profile environ-opt-in inheritance case is a known gap — pin it as strict xfail so the separate PR flips it green.
The email adapter (plugins/platforms/email/adapter.py) read EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST, EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv() directly. In a multiplexed gateway, os.environ holds the default profile's .env values, so every secondary profile inherited the default profile's email credentials instead of its own. This was a sibling of the api_server env-leak bug (#52307/#50051): the same os.getenv→get_secret migration that PR #50094 applies to gateway/config.py, but for the email adapter itself, which neither PR #50094 nor #51374 covers. Changes: - plugins/platforms/email/adapter.py: replace os.getenv with agent.secret_scope.get_secret for all EMAIL_* credential reads (adapter __init__, check_email_requirements, _allowlist_in_effect, _dispatch_message allowlist gate, _send_email SMTP helper). - gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers (from PR #50094) and replace os.getenv with _getenv for the email block in _apply_env_overrides, so config.platforms[EMAIL].extra is populated from the scoped value. - tests/gateway/test_email_secret_scope.py: 5 new tests covering scoped credential reads, environ fallback without scope, missing- key-no-leak, allowlist scoping, and check_email_requirements scoping. Related: #50051, #52307, PR #50094, PR #51374
… + scope ports/trust flag Follow-up to the salvaged #59076 commit: - Replace the bare get_secret import with a module-level Slack-pattern helper (_get_esecret): try get_secret, on UnscopedSecretError fall back to os.getenv. The DEFAULT profile's email adapter constructs UNSCOPED under multiplexing, where a bare get_secret raises and would crash the email path on startup — the exact WhatsApp defect fixed in 5438e9c (whatsapp_common._get_wsecret). - Extend scope coverage to the remaining scope-blind reads: EMAIL_IMAP_PORT / EMAIL_SMTP_PORT / EMAIL_POLL_INTERVAL (_esecret_int replacing utils.env_int) and EMAIL_TRUST_FROM_HEADER (_esecret_bool replacing utils.env_bool). - Add tests: default-profile unscoped-under-multiplex construction, and scoped ports/trust-flag no-environ-inheritance.
…n.branch alongside every set_hermes_home_override() call site on the tui_gateway path, and converted session.create, session.resume (both the _make_agent and _init_session scopes), the lazy _build resume and the per-turn submit handler. session.branch was missed. The branch handler already binds the parent's HERMES_HOME and its profile-scoped state.db — its own comment says it mirrors session.create/resume — but builds the branched agent with no secret scope. get_secret() then falls through to process os.environ, which in an app-global/remote backend is the LAUNCH profile's environment: a session branched off profile X authenticates with profile Y's credentials. That is the same cross-profile resolution #67605 fixed for the sibling paths, and it is silent (multiplexing is only activated by the messaging gateway, so the fail-closed UnscopedSecretError path never fires here). Install set_secret_scope(build_profile_secret_scope(parent_home)) for the build and release it in the same finally block as the home override. Salvage-note: code relocated from tui_gateway/server.py to tui_gateway/methods_session.py (session handlers moved after the PR was opened); test patch targets unchanged — HandlerRegistry rebinds handlers onto server.py globals
૮ >ﻌ< ა ci reviewran on 2ce32d8 all good! |
alt-glitch
added
type/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvages three contributor PRs that scope per-profile credential/config reads under multiplex profiles, resolved against current main.
Infographic
Piece 1 — qqbot (salvage #60420, credit @Da7-Tech)
Cherry-picked all three commits with authorship preserved:
fix(qqbot): resolve credentials under the active profile secret scope— adapter-level_resolve_qq_secretforQQ_APP_ID/QQ_CLIENT_SECRET/QQ_STT_*/QQ_ALLOW_ALL_USERS.fix(qqbot): scope the authz, startup-validation, and direct-send QQ reads—gateway/run.pystartup validation +tools/send_message_tool.py::_send_qqbotfallbacks now route throughgateway.config._getenv.test(qqbot): pin the scoped platform user-allowlist authz read.Adjustments during salvage:
_resolve_qq_secretfrom the original'sif current_secret_scope() is not Nonegate to the canonical Slack pattern (try get_secret / except UnscopedSecretError → os.getenv), matchingwhatsapp_common._get_wsecret(Multiplex: Slack adapter reads SLACK_APP_TOKEN from process env — all profiles connect Socket Mode to the default profile's Slack app #59739, 5438e9c).gateway/authz_mixin.pyhunks entirely — main's_auth_env/_platform_gate_envsupersede them; the remaining raw reads in that file (L459/501/879-885) are handled in a separate PR. One test case that depends on the allow-all read becoming scope-authoritative is pinned as a strictxfailso that PR flips it green.Piece 2 — email adapter (salvage #59076, credit @phantom-instruction-set)
Cherry-picked
fix(email): honor profile secret scope for email adapter env readswith authorship preserved, dropping thegateway/config.pyhunk (superseded — main already has scoped_getenvhelpers).Follow-up commit (Hermes Agent):
get_secretcalls would crash the DEFAULT profile's adapter, which constructs unscoped under multiplexing (UnscopedSecretErroron startup) — the exact WhatsApp defect fixed in 5438e9c. Wrapped them in a module-level Slack-pattern helper_get_esecret.EMAIL_IMAP_PORT/EMAIL_SMTP_PORT/EMAIL_POLL_INTERVAL(_esecret_intreplacingutils.env_int) andEMAIL_TRUST_FROM_HEADER(_esecret_boolreplacingutils.env_bool).Piece 3 — TUI session.branch (salvage #72299, credit @Frowtek)
Cherry-picked
fix(tui_gateway): install the parent profile's secret scope on session.branchwith authorship preserved.Adjustment during salvage: the session handlers moved out of
tui_gateway/server.pyafter the PR was opened — both hunks relocated totui_gateway/methods_session.py(session.branchhandler, mirroring the resume path's scope install at :567/:645 in the same file). The test still patches viaserver.*becauseHandlerRegistryrebinds the split-module handlers ontoserver.py's globals, so no test retargeting was ultimately required beyond verifying resolution.Test summary
tests/gateway/test_qqbot.py,test_qqbot_credential_isolation.py,test_qqbot_scope_paths.pytests/gateway/test_email.py,test_email_robustness.py,test_email_secret_scope.pytests/test_tui_gateway_server.pyFull run: 7 files, 622 tests passed, 0 failed. Ruff clean on all touched files.
Also maps the salvaged authors' emails in
contributors/emails/(Da7-Tech, phantom-instruction-set; Frowtek was already mapped).Closes #60420. Closes #59076. Closes #72299.
Credit: @Da7-Tech (#60420), @phantom-instruction-set (#59076), @Frowtek (#72299) — commits cherry-picked with original authorship.