Opened 8 years ago
Closed 8 years ago
#27469 closed Bug (fixed)
Email crashes cryptically when emptystring passed to django.core.mail.message.sanitize_address
Reported by: | Jarek Glowacki | Owned by: | nobody |
---|---|---|---|
Component: | Core (Mail) | Version: | dev |
Severity: | Normal | Keywords: | sanitize_address mail |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
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.
Change History (9)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
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.
comment:3 by , 8 years ago
I'd like to know a bit more about the use case to pass an empty string to sanitize_address
, please.
comment:4 by , 8 years ago
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
.
comment:5 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Thanks for the explanation, it's also useful for building an appropriate regression test.
comment:6 by , 8 years ago
Has patch: | set |
---|
In that PR, I chose to prevent empty addresses in message.recipients()
. Hopefully solves your issue!
comment:7 by , 8 years ago
A regression test isn't needed in this case?
comment:8 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Assuming the PR fulfills the use case, the patch looks good to me.
What the most proper exception to raise here?