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 Wagner Macedo, 8 years ago

What the most proper exception to raise here?

comment:2 by Jarek Glowacki, 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 Claude Paroz, 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 Jarek Glowacki, 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 Claude Paroz, 8 years ago

Triage Stage: UnreviewedAccepted

Thanks for the explanation, it's also useful for building an appropriate regression test.

comment:6 by Claude Paroz, 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 Wagner Macedo, 8 years ago

A regression test isn't needed in this case?

Version 0, edited 8 years ago by Wagner Macedo (next)

comment:8 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

Assuming the PR fulfills the use case, the patch looks good to me.

comment:9 by Claude Paroz <claude@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 8858631:

Fixed #27469 -- Prevented sending email to empty addresses

Thanks Jarek Glowacki for the report.

Note: See TracTickets for help on using tickets.
Back to Top