fix(email): let an explicit config.yaml value win over a bridged env one - #114244
Open
EloquentBrush0x wants to merge 1 commit into
Open
EloquentBrush0x wants to merge 1 commit into
EloquentBrush0x wants to merge 1 commit into
Conversation
EmailAdapter's __init__ built _address/_imap_host/_imap_security/ _smtp_host/_smtp_security/_imap_tls_verify/_smtp_tls_verify env-first (_get_secret(env) or extra.get(key)) — the reversed, more severe variant of this project's extra-vs-env ordering bug: a secondary profile's own explicit config.yaml value was silently overridden whenever the corresponding EMAIL_* var resolved truthy (e.g. bridged from the default profile's env under multiplex, or simply present in a shared process environment). _standalone_send's address/smtp_host already got this right (extra.get(...) or _get_secret(...)); its smtp_security/smtp_tls_verify did not. Flipped both the __init__ setting()/tls_verify() helpers and _standalone_send's two remaining reversed reads to extra-first, matching the already-correct sites in the same functions. A prior attempt at this file's os.getenv->get_secret migration (NousResearch#59076) closed without comment, apparently superseded by the later NousResearch#109602 mega-refactor that did the migration for ~40 adapters at once — but that refactor carried the ordering forward as-is rather than fixing it, so the bug survived under the new helper names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
EmailAdapter.__init__built several settings env-first:This is the reversed, more severe variant of this project's extra-vs-env ordering bug (the class already fixed for LINE/Mattermost/IRC elsewhere): a secondary profile's own explicit
config.yamlvalue (platforms.email.address,.smtp_host,.smtp_security,.imap_security,.smtp_tls_verify,.imap_tls_verify) is silently overridden whenever the correspondingEMAIL_*env var resolves truthy — which can happen even outside multiplex, whenever the process environment happens to carry a value (e.g. bridged from another profile, or simply present in a shared environment).Notably, this same file already gets it right in one place:
_standalone_send'saddress/smtp_hostareextra.get(...) or _get_secret(...)(extra-first) — but itssmtp_security/smtp_tls_verify, and every setting in__init__'ssetting/tls_verifyhelpers, had the order reversed.Changes
__init__'ssetting/tls_verifylambdas now checkextrafirst, falling back to_get_secret(the scoped env reader) only whenextrahas no value._standalone_send'ssmtp_security/smtp_tls_verifyflipped to match its own already-correctaddress/smtp_hostlines.Context on a prior attempt
An older PR (#59076, "honor profile secret scope for email adapter env reads") proposed migrating this file's
os.getenvcalls toget_secret, but closed with no comment — checking the timeline, it looks superseded rather than rejected: the later #109602 mega-refactor ("one scoped-secret reader... replaces ~40 adapter copies") did that exact migration for this file (and ~40 others) at once. That refactor carried the order of the fallback forward unchanged, though, so the ordering half of the bug survived under the new helper names — this PR is scoped to just that remaining half.Test plan
test_explicit_config_yaml_wins_over_bridged_environ(tests/gateway/test_email_secret_scope.py) — setsEMAIL_ADDRESS/EMAIL_SMTP_SECURITY/etc. inos.environ(simulating a bridged/leaked default-profile value) and asserts an explicitextraconfig wins for every affected field (_address,_imap_host,_smtp_host,_smtp_security,_imap_security,_smtp_tls_verify,_imap_tls_verify).test_explicit_smtp_security_wins_over_environ(tests/gateway/test_email.py) — confirmsextra['smtp_security']='tls'makes_standalone_senduse a direct-SSL connection (SMTP_SSL) instead of plaintext-then-STARTTLS, even withEMAIL_SMTP_SECURITY=starttlsinos.environ.SMTP_SSLnever called) with the exact assertion traps firing; restored the fix, all 70 tests across the five email test files pass.ruff checkclean on all three touched files.🤖 Generated with Claude Code