fix(irc): scope server/port/nickname/channel/use_tls reads to the active profile under multiplexing - #100640
Closed
nftpoetrist wants to merge 1 commit into
Closed
fix(irc): scope server/port/nickname/channel/use_tls reads to the active profile under multiplexing#100640nftpoetrist wants to merge 1 commit into
nftpoetrist wants to merge 1 commit into
Conversation
…ive profile under multiplexing
IRCAdapter.__init__, check_requirements, validate_config, is_connected,
_env_enablement, and _standalone_send all read IRC_SERVER/IRC_PORT/
IRC_NICKNAME/IRC_CHANNEL/IRC_USE_TLS via raw os.getenv -- only
IRC_SERVER_PASSWORD/IRC_NICKSERV_PASSWORD already went through the
module's _get_scoped_secret helper. Under gateway.multiplex_profiles,
env_enablement_fn/check_fn/is_connected all run inside the registry-
enablement loop in load_gateway_config() (gateway/config.py, ~lines
2704-2820), scoped for secondary profiles via _profile_runtime_scope,
and adapter construction runs scoped the same way -- so os.environ there
still holds the DEFAULT profile's env-bridge output.
Notably, __init__'s original `os.getenv("IRC_SERVER") or extra.get(...)`
ordering let a raw env read override even an explicitly configured
config.yaml extra -- a secondary profile that set its own server/channel
via config.yaml extra would still silently connect to the default
profile's IRC server/channel/nick if the default profile bridged its own
config to env (which it always does under multiplex). This is a stronger
variant of the same bug fixed for the sibling LINE/DingTalk/Teams/SMS/
WeCom/ntfy adapters in this series -- there, extra already won because
of the `extra.get(...) or os.getenv(...)` order.
Switch every raw IRC_* read (except IRC_SERVER_PASSWORD/
IRC_NICKSERV_PASSWORD, already scoped) to _get_scoped_secret(), matching
the module's existing helper. Also collapses a double os.getenv("IRC_USE_TLS")
read in __init__ into a single _get_scoped_secret() call (same behavior,
one scope lookup instead of two).
Adds a new TestMultiplexProfileScope class to tests/gateway/test_irc_adapter.py
(6 tests) mirroring the fixture/assertion style established in
tests/gateway/test_line_plugin.py's TestMultiplexProfileScope. Mutation-
verified: stashed the production fix and confirmed 5 of 6 new tests fail
against pre-fix code -- including the "extra wins" test, since IRC's
original env-first ordering meant even an explicit extra config was not
a safe differentiator boundary before the fix (only the DEFAULT-profile-
unscoped-precedence test is a non-differentiating regression guard that
correctly passes either way). Restored the fix; all 23 tests in the file
pass, plus the file's 5 parametrized _get_scoped_secret tests in
test_adapter_startup_secret_scope.py.
Contributor
Consistent scope-aware fix for the IRC adapter: all
|
Collaborator
|
Thanks @nftpoetrist — Merged via #101252 (2e25b47) on current main. Your commits from this PR were cherry-picked onto the salvage branch with your git authorship preserved, so the credit is yours in Closing this PR since the work is now on main. |
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
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.
Summary
IRCAdapter.__init__,check_requirements,validate_config,is_connected,_env_enablement, and_standalone_sendall readIRC_SERVER/IRC_PORT/IRC_NICKNAME/IRC_CHANNEL/IRC_USE_TLSvia rawos.getenv()— onlyIRC_SERVER_PASSWORD/IRC_NICKSERV_PASSWORDwere already routed through the module's_get_scoped_secret()helper.gateway.multiplex_profiles,env_enablement_fn/check_fn/is_connectedall run inside the same registry-enablement loop inload_gateway_config()(gateway/config.py, ~lines 2704-2820) which runs inside_profile_runtime_scopefor secondary profiles, and adapter construction runs scoped the same way — soos.environthere still holds the DEFAULT profile's env-bridge output.__init__'s originalos.getenv("IRC_SERVER") or extra.get(...)ordering let a raw env read override even an explicitly configuredconfig.yamlextra. A secondary profile that set its ownserver/channelviaextrawould still silently connect to the default profile's IRC server/channel/nick, because the default profile's config is always bridged toos.environunder multiplex and env was checked first. (The LINE/DingTalk/Teams/SMS/WeCom/ntfy fixes in this series didn't have this extra layer of severity — those usedextra.get(...) or os.getenv(...), soextraalready won.)Fix
Switch every raw
IRC_*read (except the two secret-material fields already scoped) to the module's existing_get_scoped_secret()helper — same fallback semantics as the rest of the series (scoped miss returns the default, never cross-profile-borrowsos.environ; the DEFAULT profile's own unscoped construction still falls back toos.environ). Also collapses a doubleos.getenv("IRC_USE_TLS")call in__init__into a single_get_scoped_secret()lookup (identical behavior, one scope read instead of two).Tests
Added
TestMultiplexProfileScope(6 tests) totests/gateway/test_irc_adapter.py, mirroring the fixture/assertion style already established intests/gateway/test_line_plugin.py's class of the same name.Mutation-verified: stashed the production fix and confirmed 5 of 6 new tests fail against pre-fix code — including the "extra wins" test, since IRC's original env-first ordering meant even an explicit
extraconfig wasn't a safe boundary before the fix. The 1 passing test (test_default_profile_unscoped_keeps_env_precedence) is a non-differentiating regression guard that correctly passes either way. Restored the fix; all 23 tests in the file pass, plus the file's 5 parametrized_get_scoped_secrettests intests/gateway/test_adapter_startup_secret_scope.py.Competitor check
Searched
IRC_SERVER,irc multiplex,irc scope. Three open PRs touchplugins/platforms/irc/adapter.py:connect()'s lock-acquisition logic only, no overlap with the env-read lines here.__init__'s existing body ends, no overlap with the env-read lines this PR changes.IRC_SERVER_PASSWORD/IRC_NICKSERV_PASSWORDreads as unchanged context, so it needs a rebase againstmainregardless of this PR). It touches the same 3-lineuse_tlsexpression in both__init__and_standalone_send, but only theextra.get("use_tls", True)→_coerce_bool(extra.get("use_tls"), True)half — a different sub-concern (YAML string coercion) from theos.getenv(...)→_get_scoped_secret(...)half this PR changes. Pure textual adjacency, trivially reconcilable on rebase; flagging transparently rather than dropping theuse_tlsscoping from scope.No open or merged PR touches the actual scope-leak fixed here.
Checklist
_get_scoped_secrethelper)