Skip to content

fix(gateway): revert Weixin get_secret() calls that crash in multiplex mode - #68854

Closed
WenhuaXia wants to merge 1 commit into
NousResearch:mainfrom
WenhuaXia:fix/weixin-get-secret-multiplex
Closed

fix(gateway): revert Weixin get_secret() calls that crash in multiplex mode#68854
WenhuaXia wants to merge 1 commit into
NousResearch:mainfrom
WenhuaXia:fix/weixin-get-secret-multiplex

Conversation

@WenhuaXia

Copy link
Copy Markdown

Problem

Commit 6160a80 replaced os.getenv() with get_secret() for Weixin credential resolution. However, WeixinAdapter.__init__() and send_weixin_direct() are called outside any secret_scope context. When multiplex_profiles: true is enabled, get_secret() raises UnscopedSecretError, preventing the platform adapter from initializing and blocking the entire gateway startup sequence.

Reproduction:

  1. Set multiplex_profiles: true in config.yaml
  2. Configure weixin platform with credentials in .env
  3. Start gateway → crashes with UnscopedSecretError: get_secret('WEIXIN_CDN_BASE_URL') called with no profile secret scope active

Fix

Revert to os.getenv() which is the correct fallback for credential resolution outside a scoped context. The config.yaml → env bridge in gateway/config.py already loads WEIXIN_* values into the PlatformConfig object (token/account_id), so os.getenv() serves only as the final fallback for extra config keys.

Files Changed

  • gateway/platforms/weixin.py: Reverted 4 get_secret() calls to os.getenv() in both WeixinAdapter.__init__() and send_weixin_direct()

Verification

  • Gateway starts successfully with multiplex_profiles: true
  • All Weixin credentials resolved correctly via env bridge
  • Weixin platform connects and processes inbound messages

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter area/auth Authentication, OAuth, credential pools needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 21, 2026
@WenhuaXia
WenhuaXia force-pushed the fix/weixin-get-secret-multiplex branch 2 times, most recently from 814b449 to 7350d54 Compare July 29, 2026 17:46
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the primary-adapter startup failure; the current main path does reproduce it.

Problems

  • gateway/run.py:8453 constructs primary adapters without _profile_runtime_scope, while secondary construction is scoped at gateway/run.py:10412-10413; this is the missing lifecycle boundary.
  • The replacement at gateway/platforms/weixin.py:1190-1195 and :2328-2331 bypasses the fail-closed secret contract. agent/secret_scope.py:166-173 explicitly rejects unscoped reads in multiplex mode because os.environ can expose another profile's credential. Commit 6160a80253 moved these exact Weixin fallbacks to get_secret() for that reason.

Suggested changes

  • Scope primary adapter creation in GatewayRunner under the default profile when multiplexing is enabled, including the relevant reconnect path, and retain get_secret() in Weixin.
  • Add a regression test for primary Weixin construction using the default profile's isolated secret scope. Related bug(gateway): primary Weixin adapter startup lacks multiplex secret scope #70652 describes this scope-preserving approach.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
…x mode

Commit 6160a80 replaced os.getenv() with get_secret() for Weixin
credential resolution, but WeixinAdapter.__init__() and
send_weixin_direct() are called outside any secret_scope context.
When multiplex_profiles is enabled, get_secret() raises
UnscopedSecretError, preventing the platform adapter from
initializing and blocking the entire gateway startup sequence.

Revert to os.getenv() which is the correct fallback for credential
resolution outside a scoped context. The config.yaml → env bridge
in gateway/config.py already loads WEIXIN_* values into the
PlatformConfig object (token/account_id), so os.getenv() serves
only as the final fallback for extra config keys.
@WenhuaXia
WenhuaXia force-pushed the fix/weixin-get-secret-multiplex branch from 7350d54 to 8bf2fb2 Compare August 2, 2026 05:13
teknium1 added a commit that referenced this pull request Aug 2, 2026
The adapter's __init__ and send_weixin_direct read WEIXIN_ACCOUNT_ID/
TOKEN/BASE_URL/CDN_BASE_URL via bare get_secret, which raises
UnscopedSecretError when the DEFAULT profile's adapter constructs or
sends unscoped under multiplexing (corrects the direction of #66073 /
#68854, which tried to solve this by borrowing os.environ on every
read — a cross-profile leak).

Add a module-level _wx_secret helper following the established Slack
SLACK_APP_TOKEN pattern (#59739) and WhatsApp's _get_wsecret: a SCOPED
miss returns the default (the scope is authoritative — no environ
borrow), while an UNSCOPED read under multiplex falls back to
os.environ, which is the default profile's own value.

Regression tests cover both directions: scoped construction reads the
scope's value and a scoped miss yields empty (no borrow); unscoped
construction falls back to os.environ instead of raising.
@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Closing in favor of the merged correction (#76663), same grounds as the sibling #66073: the unscoped-construction crash was real, but reverting to raw environ reads re-opens the cross-profile token leak for secondary profiles. Main now applies the Slack-pattern wx_secret helper to all WEIXIN* reads — including send_weixin_direct (:2316-2319), which your PR correctly identified as also affected; that finding carried into the fix. Thanks @WenhuaXia.

@teknium1 teknium1 closed this Aug 2, 2026
@WenhuaXia
WenhuaXia deleted the fix/weixin-get-secret-multiplex branch August 4, 2026 14:35
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
The adapter's __init__ and send_weixin_direct read WEIXIN_ACCOUNT_ID/
TOKEN/BASE_URL/CDN_BASE_URL via bare get_secret, which raises
UnscopedSecretError when the DEFAULT profile's adapter constructs or
sends unscoped under multiplexing (corrects the direction of NousResearch#66073 /
NousResearch#68854, which tried to solve this by borrowing os.environ on every
read — a cross-profile leak).

Add a module-level _wx_secret helper following the established Slack
SLACK_APP_TOKEN pattern (NousResearch#59739) and WhatsApp's _get_wsecret: a SCOPED
miss returns the default (the scope is authoritative — no environ
borrow), while an UNSCOPED read under multiplex falls back to
os.environ, which is the default profile's own value.

Regression tests cover both directions: scoped construction reads the
scope's value and a scoped miss yields empty (no borrow); unscoped
construction falls back to os.environ instead of raising.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants