Fix normalization for multi-segment TLDs using Fastmail#12
Open
ryangsteele wants to merge 4 commits intogmr:masterfrom
Open
Fix normalization for multi-segment TLDs using Fastmail#12ryangsteele wants to merge 4 commits intogmr:masterfrom
ryangsteele wants to merge 4 commits intogmr:masterfrom
Conversation
Owner
|
Few thoughts:
|
Author
|
Apologies, this MR absolutely needed some more context to describe the problem and proposed solution, as well as docs and a version bump. I've since added all of these things; mind taking another gander and letting me know if the proposed change seems well-reasoned and appropriate, @gmr ? |
34ef4a4 to
adff0cf
Compare
aiodns was returning TTL values of -1 for Gmail's MX records,
causing cache entries to be considered immediately expired. This
meant that on the second call to mx_records('gmail.com'), the
cache entry was deleted and recreated instead of being reused,
resetting the hit counter to 1. The fix filters out invalid TTL
values (<= 0) and falls back to failure_ttl when no valid TTL
values are available, ensuring cache entries have reasonable
expiration times.
adff0cf to
e276b12
Compare
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.
There are two changes in this MR:
1. Fix improper normalization for addresses with multi-segment TLDs whose domains use Fastmail MXs
The library currently assumes that if the
LOCAL_PART_AS_HOSTNAMEflag is set for a provider (as is the case with Fastmail), and the domain of the email address to be normalized is greater than 2 segments, that the last two segments are the domain. This logic works if the TLD is a single segment, like.comor.net, but fails with multi-segment TLDs likecom.auand.co.uk.For example, if the email address being normalized is
foo+bar@baz.co.uk, andbaz.co.uk's MX records indicate it's using Fastmail as a mail provider (e.g.,in1-smtp.messagingengine.com.), then the email address is incorrectly normalized asbaz@co.ukinstead offoo@baz.co.uk.To facilitate this change, I've made use of the
tldextractlibrary to correctly determine the TLD and avoid making assumptions based strictly upon the number of segments. Given that psl was recently archived, tldextract appeared to be the best available choice for this purpose.2. Cache entries expired immediately if aiodns returned TTL of -1 for MX record lookup
aiodns was returning invalid (as per RFC 1035) TTL value of -1 for Gmail's MX records, causing cache entries to be considered immediately expired. This meant that on the second call to mx_records('gmail.com'), the cache entry was deleted and recreated instead of being reused, resetting the hit counter to 1. In such situations the tests would fail:
The change I implemented here is to filter out invalid TTL values (< 0) and fall back to
failure_ttlwhen no valid TTL values are available.