Skip to content

fix(gateway): scope resolve_proxy_url()'s platform_env_var read by multiplex profile - #104279

Closed
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/resolve-proxy-url-multiplex-scope
Closed

nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/resolve-proxy-url-multiplex-scope

Conversation

@nftpoetrist

Copy link
Copy Markdown

Problem

gateway/platforms/base.py::resolve_proxy_url(platform_env_var, target_hosts) is a shared chokepoint used by Telegram, Discord, Mattermost, Matrix, SMS, and Slack (tools/send_message_senders.py, plugins/platforms/{telegram,discord,mattermost,matrix,sms,slack}/adapter.py, plugins/platforms/telegram/telegram_network.py) to resolve each adapter's proxy URL. It read platform_env_var (e.g. TELEGRAM_PROXY, DISCORD_PROXY) via raw os.environ.get().

Under a secondary multiplex profile, os.environ holds the default profile's YAML-to-env bridge output — a secondary profile with its own (different or absent) proxy configuration would silently borrow the default profile's proxy, or the reverse. Proxy URLs can embed credentials (http://user:pass@host), so this is a credential/routing scoping gap, not just a config-value one.

This gap was already identified and explicitly deferred in this account's own PR #100448:

# proxy_url is deliberately NOT scoped here — resolve_proxy_url() (gateway/platforms/base.py, shared by Discord/Telegram/etc.) reads TELEGRAM_PROXY via raw os.environ regardless of this bridge; scoping only the write side would be an inconsistent partial fix. Left for a dedicated fix to that shared helper.

This PR is that dedicated fix.

Fix

platform_env_var is now read through gateway.platforms._shared.get_scoped_secret() — the same scope-aware helper (profile's own secret scope first, falling back to os.environ only when unscoped/default-profile) already used by 10+ other adapters for this exact class of setting.

The generic HTTPS_PROXY/HTTP_PROXY/ALL_PROXY fallback (used when no platform_env_var is configured and gateway.trust_env is true) is left as a raw env read — those are OS/system-level network settings, not a per-profile Hermes concept, and scoping them would be a behavior change without a corresponding per-profile convention to back it.

Testing

  • New TestResolveProxyUrlMultiplexScope class in tests/gateway/test_gateway_trust_env.py (3 tests): a scoped secondary profile uses its own DISCORD_PROXY, a scoped profile with no own value does not borrow the default profile's env value (fails closed to None), and a single-profile/unscoped control confirms the legacy env read is unchanged.
  • Mutation-verified: reverted the fix and confirmed the 2 scoping tests fail with the exact bypass symptom (reads the default profile's proxy instead of the secondary profile's own value / instead of None); the control test correctly passes either way.
  • Full tests/gateway/test_gateway_trust_env.py + test_proxy_mode.py + test_telegram_closewait_limits_31599.py + test_telegram_polling_progress.py + tests/tools/test_send_message_telegram_proxy.py + test_slack.py + test_mattermost.py + all test_matrix*.py: all green, no regressions.
  • tests/gateway/test_discord_send.py showed 3 failures in a combined run with the Slack/Discord/Mattermost suites — confirmed via mutation-verify (identical failures with the fix reverted) and isolated runs (all pass alone) that this is a pre-existing test-order-pollution artifact in this test file, completely unrelated to this change.

Scope note

Discord/Telegram/Mattermost/Matrix/SMS/Slack all call through this one function — fixing it here closes the gap for every caller at once rather than patching each adapter's own proxy resolution individually.

…ltiplex profile

resolve_proxy_url() read platform_env_var (TELEGRAM_PROXY, DISCORD_PROXY,
MATTERMOST_PROXY, MATRIX_PROXY) via raw os.environ.get() — a shared
chokepoint used by 7+ adapters. Under a secondary multiplex profile,
os.environ holds the default profile's YAML-to-env bridge output, so a
secondary profile with its own (different or absent) proxy config
would silently borrow the default profile's proxy, or vice versa —
and proxy URLs can embed credentials (http://user:pass@host).

Fix: read platform_env_var through gateway.platforms._shared's
get_scoped_secret(), which is scope-aware (profile's own scope first,
falling back to os.environ only when unscoped/default-profile) and
already used by 10+ other adapters for the same class of setting. The
generic HTTPS_PROXY/HTTP_PROXY/ALL_PROXY fallback stays a raw env read
— those are OS/system-level network settings, not a per-profile Hermes
concept.

This closes a gap PR NousResearch#100448 explicitly deferred ("proxy_url is
deliberately NOT scoped here ... left for a dedicated fix to that
shared helper").
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery area/profiles Multi-profile isolation, HERMES_HOME scoping area/config Config system, migrations, profiles sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 6, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

PR #104279 — fix(gateway): scope resolve_proxy_url()'s platform_env_var read by multiplex profile

Correct application of the #72348 scoping pattern to the shared proxy chokepoint: per-platform proxy vars can embed credentials, so a secondary profile must read its own .env via get_scoped_secret rather than the default profile's bridged process env. Keeping the generic HTTPS_PROXY/HTTP_PROXY/ALL_PROXY fallback as a raw process-env read is the right distinction (OS-level settings, not per-profile secrets), and the docstring states it explicitly. Tests cover own-value, fail-closed-without-value, and the unscoped control.

Non-blocking observations:

  • Fail-closed behavior change: a secondary profile that previously (perhaps unknowingly) inherited the default profile's DISCORD_PROXY will now resolve None and connect directly. For proxy-mandated environments that fails open network-wise (direct connection instead of proxied). This matches the class fix direction, but operators with multiplex + proxy setups need an explicit per-profile value — worth a migration note if one isn't posted elsewhere.
  • gateway/platforms/base.py:19 — function-local import of get_scoped_secret; consistent with avoiding import cycles, fine.

Verdict: looks good to merge.

@teknium1

Copy link
Copy Markdown
Collaborator

Landed on main in #108705. Cherry-picked with authorship preserved as 5725efa — resolve_proxy_url now reads platform_env_var (TELEGRAM_PROXY/DISCORD_PROXY/…) through get_scoped_secret, so a served secondary uses its own proxy and the generic HTTPS_PROXY/HTTP_PROXY/ALL_PROXY fallback stays a process-env read as your PR had it. Your TestResolveProxyUrlMultiplexScope tests came along unchanged. Thanks, @nftpoetrist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles area/profiles Multi-profile isolation, HERMES_HOME scoping comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants