Skip to content

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

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

felixxm merged 2 commits into
django:masterfrom
chason:patch-30608

Conversation

@chason

@chason chason commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

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

felixxm commented Jul 2, 2019

Copy link
Copy Markdown
Member

@ewjoachim Can you take a look?

@felixxm felixxm self-assigned this Jul 2, 2019

@felixxm felixxm left a comment

Copy link
Copy Markdown
Member

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.

Comment thread tests/mail/tests.py Resolved
Comment thread tests/mail/tests.py Resolved
Comment thread tests/mail/tests.py Resolved
Comment thread tests/mail/tests.py Resolved
Comment thread tests/mail/tests.py Resolved

@ewjoachim ewjoachim left a comment

Copy link
Copy Markdown
Contributor

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

chason commented Jul 2, 2019

Copy link
Copy Markdown
Contributor Author

@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

felixxm commented Jul 2, 2019

Copy link
Copy Markdown
Member

@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

felixxm commented Jul 2, 2019

Copy link
Copy Markdown
Member

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

chason commented Jul 3, 2019

Copy link
Copy Markdown
Contributor Author

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

Comment thread django/core/mail/utils.py Resolved
@felixxm

felixxm commented Jul 3, 2019

Copy link
Copy Markdown
Member

@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

felixxm commented Jul 3, 2019

Copy link
Copy Markdown
Member

@chason Thanks again 🚀 Welcome aboard ⛵

@chason
chason deleted the patch-30608 branch July 3, 2019 10:06
davidgegiya added a commit to davidgegiya/agent-orchestrator that referenced this pull request Nov 9, 2026
…l encoding is non-unicode

Pin a5308514fb4bc5086c9a16a8a24a945eeebb073c and its regression selectors for reproducible runs.

Source: django/django#11532
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