Skip to content
/ django Public

Fixed #30608 -- Fixed non-unicode EmailMessage crash when domain name for localhost is non-ASCII.#11532

Merged
felixxm merged 2 commits intodjango:masterfrom
chason:patch-30608
Jul 3, 2019
Merged

Fixed #30608 -- Fixed non-unicode EmailMessage crash when domain name for localhost is non-ASCII.#11532
felixxm merged 2 commits intodjango:masterfrom
chason:patch-30608

Conversation

@chason
Copy link
Contributor

@chason chason commented Jul 2, 2019

When sending out an email, Django attaches the domain name as part
of the Message-ID header. This was causing an error when the domain
name contains unicode characters and the message is set to an
encoding that cannot encode them. These domains should probably
always be set to encode to punycode in any case, so this patch
changes the CachedDnsName class in django.core.mail.utils to return
this fqdn as punycode when it can't be encoded as ascii.

Assisted by felixxm

@felixxm
Copy link
Member

felixxm commented Jul 2, 2019

@ewjoachim Can you take a look?

@felixxm felixxm self-assigned this Jul 2, 2019
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chason Thanks for this patch 👍 I left some comments.

Copy link
Contributor

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Is it a regression from the patch we merged the other day or a whole new bug ? I'm guessing this is new.

BTW, this whole "punycoding domain names in case they're not punycode" looks a lot like

try:
domain.encode('ascii')
except UnicodeEncodeError:
domain = domain.encode('idna').decode('ascii')
, maybe it would be worth factorizing this in a utility function ? Not sure what the common practice is in django codebase.

And finally, that was nice thinking of me to review this, and just to say it explicitly, feel free to continue doing that.

@chason
Copy link
Contributor Author

chason commented Jul 2, 2019

@ewjoachim I believe it is new. I only discovered it because I have a bit of a strange configuration on my computer and have a unicode hostname set.

@felixxm
Copy link
Member

felixxm commented Jul 2, 2019

@ewjoachim Thanks for the review 👍 Yes, it is an old 🐛. I pinged you because you've recently sent PR in this area. Thanks for offering yourself for the future.

@felixxm
Copy link
Member

felixxm commented Jul 2, 2019

I rebased from master and added punycode() hook.

@felixxm felixxm changed the title Fixed #30608 - Fixed utf8 domains in emails Fixed #30608 -- Fixed non-unicode EmailMessage crash when domain name for localhost is non-ASCII. Jul 2, 2019
@chason
Copy link
Contributor Author

chason commented Jul 3, 2019

since "idna" encodes in ascii anyway, instead of doing a try/except would it make sense to just run the "idna" encode on everything?

@felixxm
Copy link
Member

felixxm commented Jul 3, 2019

@charettes @chason Thanks, I updated PR.

… for localhost is non-ASCII.

Assisted by felixxm.
@felixxm felixxm merged commit 55b68de into django:master Jul 3, 2019
@felixxm
Copy link
Member

felixxm commented Jul 3, 2019

@chason Thanks again 🚀 Welcome aboard ⛵

@chason chason deleted the patch-30608 branch July 3, 2019 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants