[Django] #27469: Email crashes cryptically when emptystring passed to django.core.mail.message.sanitize_address
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Reporter: Jarek | Owner: nobody
Glowacki |
Type: Bug | Status: new
Component: Core | Version: master
(Mail) | Keywords: sanitize_address
Severity: Normal | mail
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
{{{
In [10]: sanitize_address('', 'utf-8')
---------------------------------------------------------------------------
IndexError Traceback (most recent call
last)
<ipython-input-10-ad3601147d6a> in <module>()
----> 1 sanitize_address('', 'utf-8')
/usr/local/lib/python3.5/site-packages/django/core/mail/message.py in
sanitize_address(addr, encoding)
159
160 try:
--> 161 address = Address(nm, addr_spec=addr)
162 except (InvalidHeaderDefect, NonASCIILocalPartDefect):
163 localpart, domain = split_addr(addr, encoding)
/usr/local/lib/python3.5/email/headerregistry.py in __init__(self,
display_name, username, domain, addr_spec)
40 raise TypeError("addrspec specified when username
and/or "
41 "domain also specified")
---> 42 a_s, rest = parser.get_addr_spec(addr_spec)
43 if rest:
44 raise ValueError("Invalid addr_spec; only '{}' "
/usr/local/lib/python3.5/email/_header_value_parser.py in
get_addr_spec(value)
1986 """
1987 addr_spec = AddrSpec()
-> 1988 token, value = get_local_part(value)
1989 addr_spec.append(token)
1990 if not value or value[0] != '@':
/usr/local/lib/python3.5/email/_header_value_parser.py in
get_local_part(value)
1798 local_part = LocalPart()
1799 leader = None
-> 1800 if value[0] in CFWS_LEADER:
1801 leader, value = get_cfws(value)
1802 if not value:
IndexError: string index out of range
}}}
Could we reraise some more intuitive error back to the user for this case?
A single `@` symbol also raises an exception for this method, but at least
it yields a more meaningful error that the dev can track down:
`HeaderParseError: Expected 'atom' or 'quoted-string' but found '@'`
Though maybe this could be beefed up to something nicer also.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage:
mail | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by wagnerluis1982):
What the most proper exception to raise here?
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:1>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage:
mail | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jarek Glowacki):
I'd be happy with any of:
`InvalidEmailException`
`EmptyEmailException`
`InvalidEmailAddressException`
`EmptyEmailAddressException`
Or maybe even silencing the exception and having something else in the
food chain worry about it.
I don't know much about the Mail component of Django, so best for someone
else to make that call, so that it's consistent with the way other kinds
of invalid emails behave.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:2>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage:
mail | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
I'd like to know a bit more about the use case to pass an empty string to
`sanitize_address`, please.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:3>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage:
mail | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jarek Glowacki):
We don't use `sanitize_addresss` directly, it ends up getting called
during an `email.send()` call on our end.
We have a pretty complex django project that has numerous apps set up to
do some form of emailing. The email 'to' and 'cc' are set by the end user
on a `Contact` model, both fields are allowed to be empty. The system then
builds emails in various configurations, in some cases ccing contacts in
on one email, in other cases creating separate emails per contact. Long
story short the logic is nontrivial.
A couple of times now (usually after rounds of refactoring) we've hit a
case where either the 'to' or 'cc' list contains an emptystring, causing
this cryptic error to surface when `email.send()` is called. Each time
it's been someone else in the office hitting it, consuming a moderate
amount of time to track down the cause.
It would be nice if this raised a proper error, so that we could either
have something nice to catch and handle during runtime, or be able to
quickly understand what's going if it shows up again in our error logs.
Worst of all, this doesn't show up when we test the project locally, as we
swap out the smtp backend with a dummy one in debug mode. So another idea
could maybe be to catch this problem earlier to make it backend-
independent.
This is a non issue for those that are careful and validate email
addresses before trying to call `.send()`, but for those that do fall into
this trap, they probably shouldn't be hit with an unhelpful `IndexError:
string index out of range`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:4>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
mail |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Thanks for the explanation, it's also useful for building an appropriate
regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:5>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage: Accepted
mail |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
In that [https://github.com/django/django/pull/7535 PR], I chose to
prevent empty addresses in `message.recipients()`. Hopefully solves your
issue!
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:6>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
Keywords: sanitize_address | Triage Stage: Accepted
mail |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Wagner Macedo):
A regression test isn't needed in this case?
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:7>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Mail) | Version: master
Severity: Normal | Resolution:
mail | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Assuming the PR fulfills the use case, the patch looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:8>
Django
django.core.mail.message.sanitize_address
-------------------------------------+-------------------------------------
Component: Core (Mail) | Version: master
Keywords: sanitize_address | Triage Stage: Ready for
mail | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"8858631498bca33feb2acde71f9cf6ca2884b1bd" 8858631]:
{{{
#!CommitTicketReference repository=""
revision="8858631498bca33feb2acde71f9cf6ca2884b1bd"
Fixed #27469 -- Prevented sending email to empty addresses
Thanks Jarek Glowacki for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27469#comment:9>