fix(qqbot): resolve credentials under the active profile secret scope - #60420
fix(qqbot): resolve credentials under the active profile secret scope#60420Da7-Tech wants to merge 3 commits into
Conversation
|
looks mergeable Security evidence:
Signed: GPT-5.5-xhigh in Codex |
3d82651 to
2e97fca
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real multiplex credential-isolation gap. Current main still reads QQ adapter credentials and STT fallback values directly from os.getenv (gateway/platforms/qqbot/adapter.py:204-207, 2198-2204) even though secondary adapters are created under _profile_runtime_scope (gateway/run.py:8657-8689).
Problems
- The new
QQ_ALLOW_ALL_USERSresolver only changes the adapter intake check. Gateway authorization independently reads the same flag fromos.getenvatgateway/authz_mixin.py:426-429, so a secondary profile's scoped opt-in is still denied after intake. - Secondary startup validation has the same bypass:
_start_one_profile_adaptersinvokes_own_policy_open_startup_violationinside the profile scope (gateway/run.py:8657-8660), but that validator reads the platform opt-in fromos.getenv(gateway/run.py:1845-1851). tools/send_message_tool.py:1863-1867retains raw QQ credential fallbacks for the direct-send path.
Suggested changes
- Apply the same scoped resolution to those gateway authorization, startup-validation, and direct-send paths; add end-to-end scope tests for
QQ_ALLOW_ALL_USERSand allQQ_STT_*values.
Automated hermes-sweeper review.
| @@ -3145,7 +3170,7 @@ def _strip_at_mention(content: str) -> str: | |||
| def _open_dm_opted_in(self) -> bool: | |||
There was a problem hiding this comment.
This scopes the adapter intake gate, but GatewayAuthorizationMixin._is_user_authorized() separately checks QQ_ALLOW_ALL_USERS via raw os.getenv at gateway/authz_mixin.py:426-429; a secondary profile's scoped allow-all value will still be denied after intake. Please scope that gateway check (and the startup validator at gateway/run.py:1845-1851) in the same change.
|
Addressed the three flagged paths in
End-to-end tests added in |
|
Follow-up notes after an adversarial self-review of this change:
|
|
fully addressed The three paths identified in the earlier review are addressed on the current head, and the 17 focused QQ credential/authorization tests passed when the patch was replayed onto current I still think extending this PR to enter the appropriate profile scope in The submitted branch currently conflicts with Signed: GPT-5.6-sol-xhigh in Codex |
…cope
`resolve_openai_audio_api_key()` reads the key that authenticates the audio
client straight from the process environment:
return (
os.getenv("VOICE_TOOLS_OPENAI_KEY", "")
or os.getenv("OPENAI_API_KEY", "")
).strip()
That value is not advisory. It flows through
`_resolve_openai_audio_client_config()` into `OpenAIClient(api_key=...)` for
TTS, and through `transcription_tools` for voice-note STT — both on the
per-turn tool path, inside the profile secret scope the gateway installs.
`agent/vertex_adapter` states the contract this breaks:
in a multiplex gateway serving several profiles from one process,
os.environ reflects whichever profile's .env happened to be loaded at
boot, not the profile the current turn belongs to. Reading it directly
here would let one profile mint tokens from — and get billed against —
a different profile's service-account file.
Reproduced with the real resolver, multiplexing on and profile A's scope
installed:
scope-aware get_secret -> sk-PROFILE-A-key
voice/STT resolver -> sk-PROFILE-B-key
So profile A's spoken reply and its users' voice notes are sent to OpenAI on
profile B's account, and billed there.
Route both reads through `agent.secret_scope.get_secret`, the same fix already
merged for the WeChat send path (#59662) and pending for QQ (#60420) — neither
covers the audio credential family. Under multiplexing the scope stays
authoritative, so a scope miss now yields no key instead of borrowing another
profile's; with multiplexing off `get_secret` falls through to `os.environ`
exactly as before, so single-profile deployments are untouched. The
VOICE_TOOLS_OPENAI_KEY > OPENAI_API_KEY precedence is unchanged.
Deliberately narrow: `fal_key_is_configured()` and
`has_direct_modal_credentials()` in this file are presence checks, not
authentication, and the former is already being reworked in open PR #20929.
tests/tools/test_tool_backend_helpers.py: the scope wins over another
profile's `os.environ`; a scope miss does not borrow another profile's key;
voice-key precedence holds inside a scope; and a control proves the
single-profile path still reads `os.environ`. The three isolation tests fail
on main; the control passes there. 320 passed across the helper, secret-scope,
and consumer suites (the fluctuating voice_mode/voice_cli failures are
pre-existing PulseAudio/ordering artifacts — the differing test passes 3/3 in
isolation on both main and this branch).
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 NousResearch#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.
51704e9 to
eec5531
Compare
…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.
…cope
`resolve_openai_audio_api_key()` reads the key that authenticates the audio
client straight from the process environment:
return (
os.getenv("VOICE_TOOLS_OPENAI_KEY", "")
or os.getenv("OPENAI_API_KEY", "")
).strip()
That value is not advisory. It flows through
`_resolve_openai_audio_client_config()` into `OpenAIClient(api_key=...)` for
TTS, and through `transcription_tools` for voice-note STT — both on the
per-turn tool path, inside the profile secret scope the gateway installs.
`agent/vertex_adapter` states the contract this breaks:
in a multiplex gateway serving several profiles from one process,
os.environ reflects whichever profile's .env happened to be loaded at
boot, not the profile the current turn belongs to. Reading it directly
here would let one profile mint tokens from — and get billed against —
a different profile's service-account file.
Reproduced with the real resolver, multiplexing on and profile A's scope
installed:
scope-aware get_secret -> sk-PROFILE-A-key
voice/STT resolver -> sk-PROFILE-B-key
So profile A's spoken reply and its users' voice notes are sent to OpenAI on
profile B's account, and billed there.
Route both reads through `agent.secret_scope.get_secret`, the same fix already
merged for the WeChat send path (NousResearch#59662) and pending for QQ (NousResearch#60420) — neither
covers the audio credential family. Under multiplexing the scope stays
authoritative, so a scope miss now yields no key instead of borrowing another
profile's; with multiplexing off `get_secret` falls through to `os.environ`
exactly as before, so single-profile deployments are untouched. The
VOICE_TOOLS_OPENAI_KEY > OPENAI_API_KEY precedence is unchanged.
Deliberately narrow: `fal_key_is_configured()` and
`has_direct_modal_credentials()` in this file are presence checks, not
authentication, and the former is already being reworked in open PR NousResearch#20929.
tests/tools/test_tool_backend_helpers.py: the scope wins over another
profile's `os.environ`; a scope miss does not borrow another profile's key;
voice-key precedence holds inside a scope; and a control proves the
single-profile path still reads `os.environ`. The three isolation tests fail
on main; the control passes there. 320 passed across the helper, secret-scope,
and consumer suites (the fluctuating voice_mode/voice_cli failures are
pre-existing PulseAudio/ordering artifacts — the differing test passes 3/3 in
isolation on both main and this branch).
…ixin gate PR The cherry-picked NousResearch#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.
…cope
`resolve_openai_audio_api_key()` reads the key that authenticates the audio
client straight from the process environment:
return (
os.getenv("VOICE_TOOLS_OPENAI_KEY", "")
or os.getenv("OPENAI_API_KEY", "")
).strip()
That value is not advisory. It flows through
`_resolve_openai_audio_client_config()` into `OpenAIClient(api_key=...)` for
TTS, and through `transcription_tools` for voice-note STT — both on the
per-turn tool path, inside the profile secret scope the gateway installs.
`agent/vertex_adapter` states the contract this breaks:
in a multiplex gateway serving several profiles from one process,
os.environ reflects whichever profile's .env happened to be loaded at
boot, not the profile the current turn belongs to. Reading it directly
here would let one profile mint tokens from — and get billed against —
a different profile's service-account file.
Reproduced with the real resolver, multiplexing on and profile A's scope
installed:
scope-aware get_secret -> sk-PROFILE-A-key
voice/STT resolver -> sk-PROFILE-B-key
So profile A's spoken reply and its users' voice notes are sent to OpenAI on
profile B's account, and billed there.
Route both reads through `agent.secret_scope.get_secret`, the same fix already
merged for the WeChat send path (NousResearch#59662) and pending for QQ (NousResearch#60420) — neither
covers the audio credential family. Under multiplexing the scope stays
authoritative, so a scope miss now yields no key instead of borrowing another
profile's; with multiplexing off `get_secret` falls through to `os.environ`
exactly as before, so single-profile deployments are untouched. The
VOICE_TOOLS_OPENAI_KEY > OPENAI_API_KEY precedence is unchanged.
Deliberately narrow: `fal_key_is_configured()` and
`has_direct_modal_credentials()` in this file are presence checks, not
authentication, and the former is already being reworked in open PR NousResearch#20929.
tests/tools/test_tool_backend_helpers.py: the scope wins over another
profile's `os.environ`; a scope miss does not borrow another profile's key;
voice-key precedence holds inside a scope; and a control proves the
single-profile path still reads `os.environ`. The three isolation tests fail
on main; the control passes there. 320 passed across the helper, secret-scope,
and consumer suites (the fluctuating voice_mode/voice_cli failures are
pre-existing PulseAudio/ordering artifacts — the differing test passes 3/3 in
isolation on both main and this branch).
…ixin gate PR The cherry-picked NousResearch#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.
…cope
`resolve_openai_audio_api_key()` reads the key that authenticates the audio
client straight from the process environment:
return (
os.getenv("VOICE_TOOLS_OPENAI_KEY", "")
or os.getenv("OPENAI_API_KEY", "")
).strip()
That value is not advisory. It flows through
`_resolve_openai_audio_client_config()` into `OpenAIClient(api_key=...)` for
TTS, and through `transcription_tools` for voice-note STT — both on the
per-turn tool path, inside the profile secret scope the gateway installs.
`agent/vertex_adapter` states the contract this breaks:
in a multiplex gateway serving several profiles from one process,
os.environ reflects whichever profile's .env happened to be loaded at
boot, not the profile the current turn belongs to. Reading it directly
here would let one profile mint tokens from — and get billed against —
a different profile's service-account file.
Reproduced with the real resolver, multiplexing on and profile A's scope
installed:
scope-aware get_secret -> sk-PROFILE-A-key
voice/STT resolver -> sk-PROFILE-B-key
So profile A's spoken reply and its users' voice notes are sent to OpenAI on
profile B's account, and billed there.
Route both reads through `agent.secret_scope.get_secret`, the same fix already
merged for the WeChat send path (NousResearch#59662) and pending for QQ (NousResearch#60420) — neither
covers the audio credential family. Under multiplexing the scope stays
authoritative, so a scope miss now yields no key instead of borrowing another
profile's; with multiplexing off `get_secret` falls through to `os.environ`
exactly as before, so single-profile deployments are untouched. The
VOICE_TOOLS_OPENAI_KEY > OPENAI_API_KEY precedence is unchanged.
Deliberately narrow: `fal_key_is_configured()` and
`has_direct_modal_credentials()` in this file are presence checks, not
authentication, and the former is already being reworked in open PR NousResearch#20929.
tests/tools/test_tool_backend_helpers.py: the scope wins over another
profile's `os.environ`; a scope miss does not borrow another profile's key;
voice-key precedence holds inside a scope; and a control proves the
single-profile path still reads `os.environ`. The three isolation tests fail
on main; the control passes there. 320 passed across the helper, secret-scope,
and consumer suites (the fluctuating voice_mode/voice_cli failures are
pre-existing PulseAudio/ordering artifacts — the differing test passes 3/3 in
isolation on both main and this branch).
…ixin gate PR The cherry-picked NousResearch#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.
What does this PR do?
The QQ (
qqbot) gateway adapter read its per-profile settings —QQ_APP_ID,QQ_CLIENT_SECRET, theQQ_STT_*speech-to-text config, and theQQ_ALLOW_ALL_USERSpolicy flag — through rawos.getenv. That bypasses the active profile secret scope, so in multiplex mode a secondary profile whose secret lives in its own.env(loaded as an isolated scope, not intoos.environ) silently falls back to the default/primary profile's value — the same cross-profile credential collision fixed for the WeChat/weixinadapter in #59662.This routes those reads through a small scope-aware resolver,
_resolve_qq_secret:_profile_runtime_scope, as is each per-turn inbound message) → read from the scope viaget_secret, so profiles never see each other'sos.environvalues.os.environ. This fallback is deliberate: the primary/active profile is constructed without a scope (gateway/run.py) while multiplexing is active, so a bareget_secretwould raiseUnscopedSecretErrorand break the active profile's startup. The resolver mirrors the existing scope-awaregateway.config._getenv.Because secondary profiles are always scoped during construction and runtime (contextvars propagate to their asyncio tasks), they never reach the
os.environfallback — no leak. Single-profile deployments are unaffected (multiplex inactive →os.environ, exactly as before).GATEWAY_ALLOW_ALL_USERSand the network proxy vars are intentionally left as rawos.getenv— they are deployment-global settings, not per-profile secrets.Related Issue
Same class as #59662 (the WeChat/
weixinadapter). No qqbot-specific issue exists yet.Type of Change
Changes Made
gateway/platforms/qqbot/adapter.py: add_resolve_qq_secret(scope-aware, withos.environfallback for the unscoped active profile) and route all per-profileQQ_*reads through it. Returnsstr, so the STTbase_url/modelare no longerOptional-typed.tests/gateway/test_qqbot_credential_isolation.py: new regression tests — scope-wins-over-environ, two-profile isolation, single-profile fallback, explicit-config precedence, STT key scoping, and active-profile-no-scope construction (proves the fail-closed case does not raise).How to Test
Run:
pytest tests/gateway/test_qqbot_credential_isolation.py -qChecklist
Review follow-up (hermes-sweeper)
Commit
7875c0a20extends the same scoped resolution to the three flagged paths:gateway/authz_mixin.py): the per-platform allow-all flag, per-platform/group allowlists, and allow-bots reads now go through the scope-awaregateway.config._getenv. The deployment-globalGATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERSreads intentionally remain rawos.getenv. Because these checks are platform-generic, the fix applies to every own-policy platform (not just QQ); with no scope installed_getenvis byte-identical toos.getenv, so single-profile deployments are unaffected.gateway/run.py::_own_policy_open_startup_violation): the per-platform dm/group policy and allow-all opt-in resolve via_getenv; the secondary-profile caller already runs inside_profile_runtime_scope, so each profile's own opt-in is honored.tools/send_message_tool.py::_send_qqbot): theQQ_APP_ID/QQ_CLIENT_SECRETfallbacks now honor the active profile scope.New end-to-end tests in
tests/gateway/test_qqbot_scope_paths.pycover all three paths (scoped value wins over environ, a profile without its own opt-in does not inherit the primary's environ opt-in, unscoped single-profile behavior unchanged), and the STT suite now assertsQQ_STT_BASE_URL/QQ_STT_MODELscoping alongside the API key. All five scoped-behavior tests fail on the previous head and pass on this one.