Merge ~cjwatson/launchpad:py3-process-mail into launchpad:master

[Colin Watson]

process-mail: Improve handling of non-MIME-encoded headers on py3

Python 3's email package is somewhat stricter about various aspects of email encoding, so we need to be more careful to handle the case of incoming email with non-MIME-encoded non-ASCII From: headers.

[Cristian Gonzalez (cristiangsp)]

Looks good!

review: Approve

diff --git a/lib/lp/services/mail/incoming.py b/lib/lp/services/mail/incoming.py
index 958985b..92ca11c 100644
--- a/lib/lp/services/mail/incoming.py
+++ b/lib/lp/services/mail/incoming.py
@@ -186,7 +186,7 @@ def _verifyDkimOrigin(signed_message):
     for origin in ['From', 'Sender']:
         if signed_message[origin] is None:
             continue
-        name, addr = parseaddr(signed_message[origin])
+        addr = parseaddr(str(signed_message[origin]))[1]
         try:
             origin_domain = addr.split('@')[1]
         except IndexError:
@@ -265,7 +265,7 @@ def authenticateEmail(mail, signature_timestamp_checker=None):
 
     principal, dkim_trusted_address = _getPrincipalByDkim(mail)
     if dkim_trusted_address is None:
-        from_addr = parseaddr(mail['From'])[1]
+        from_addr = parseaddr(str(mail['From']))[1]
         try:
             principal = authutil.getPrincipalByLogin(from_addr)
         except (TypeError, UnicodeDecodeError):
@@ -310,7 +310,7 @@ def _gpgAuthenticateEmail(mail, principal, person,
     """
     log = logging.getLogger('process-mail')
     signature = mail.signature
-    email_addr = parseaddr(mail['From'])[1]
+    email_addr = parseaddr(str(mail['From']))[1]
     if signature is None:
         # Mark the principal so that application code can check that the
         # user was weakly authenticated.
diff --git a/lib/lp/services/mail/notification.py b/lib/lp/services/mail/notification.py
index 827cdf9..784a20e 100644
--- a/lib/lp/services/mail/notification.py
+++ b/lib/lp/services/mail/notification.py
@@ -12,13 +12,13 @@ __all__ = [
 
 
 from difflib import unified_diff
-from email import message_from_string
 from email.mime.message import MIMEMessage
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
 import re
 
 from lp.bugs.mail.bugnotificationbuilder import get_bugmail_error_address
+from lp.services.compat import message_from_bytes
 from lp.services.config import config
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.mailwrapper import MailWrapper
@@ -71,10 +71,10 @@ def send_process_error_notification(to_address, subject, error_msg,
     msg['From'] = get_bugmail_error_address()
     msg['Subject'] = subject
     msg.attach(error_part)
-    original_msg_str = str(original_msg)
+    original_msg_str = bytes(original_msg)
     if len(original_msg_str) > max_return_size:
         truncated_msg_str = original_msg_str[:max_return_size]
-        original_msg = message_from_string(truncated_msg_str)
+        original_msg = message_from_bytes(truncated_msg_str)
     msg.attach(MIMEMessage(original_msg))
     sendmail(msg)
 
diff --git a/lib/lp/services/mail/sendmail.py b/lib/lp/services/mail/sendmail.py
index 72ca82d..f1ab3c6 100644
--- a/lib/lp/services/mail/sendmail.py
+++ b/lib/lp/services/mail/sendmail.py
@@ -396,9 +396,9 @@ def sendmail(message, to_addrs=None, bulk=True):
     """
     validate_message(message)
     if to_addrs is None:
-        to_addrs = get_addresses_from_header(message['to'])
+        to_addrs = get_addresses_from_header(str(message['to']))
         if message['cc']:
-            to_addrs = to_addrs + get_addresses_from_header(message['cc'])
+            to_addrs = to_addrs + get_addresses_from_header(str(message['cc']))
 
     do_paranoid_envelope_to_validation(to_addrs)
 
diff --git a/lib/lp/services/mail/tests/incomingmail.txt b/lib/lp/services/mail/tests/incomingmail.txt
index 341c248..4c21007 100644
--- a/lib/lp/services/mail/tests/incomingmail.txt
+++ b/lib/lp/services/mail/tests/incomingmail.txt
@@ -244,6 +244,7 @@ attempting to process incoming mail.
     ...     pass
     >>> @implementer(IMailHandler)
     ... class OopsHandler:
+    ...     allow_unknown_users = True
     ...     def process(self, mail, to_addr, filealias):
     ...         raise TestOopsException()
     >>> mail_handlers.add('oops.com', OopsHandler())
@@ -298,6 +299,58 @@ to the user, citing the OOPS ID, with the original message attached.
     >>> print(original_message.get_payload(0)['Subject'])
     doesn't matter
 
+OOPS notifications work even if the From: address isn't properly MIME-encoded.
+
+    >>> from lp.services.compat import message_from_bytes
+    >>> msg = message_from_bytes(
+    ... u"""From: \u05D1 <[e-mail]>
+    ... To: [e-mail]
+    ... X-Launchpad-Original-To: [e-mail]
+    ... Subject: doesn't matter
+    ...
+    ... doesn't matter
+    ... """.encode('UTF-8'))
+    >>> msgid = sendmail(msg, ['edit@malone-domain'])
+    >>> handleMailForTest()
+    ERROR:process-mail:An exception was raised inside the handler:
+    ...
+    TestOopsException
+
+    >>> from email.header import (
+    ...     decode_header,
+    ...     make_header,
+    ...     )
+    >>> from email.utils import parseaddr
+    >>> notification = pop_notifications()[-1]
+    >>> print(notification.get_content_type())
+    multipart/mixed
+    >>> print(pretty(six.ensure_text(decode_header(notification['To'])[0][0])))
+    '\u05d1 <[e-mail]>'
+    >>> error_message, original_message = notification.get_payload()
+    >>> print(error_message.get_content_type())
+    text/plain
+    >>> print(error_message.get_payload(decode=True).decode())
+    An error occurred while processing a mail you sent to Launchpad's email
+    interface.
+    ...
+    Sorry, something went wrong when Launchpad tried processing your mail.
+    We've recorded what happened, and we'll fix it as soon as possible.
+    Apologies for the inconvenience.
+    <BLANKLINE>
+    If this is blocking your work, please file a question at
+    https://answers.launchpad.net/launchpad/+addquestion
+    and include the error ID OOPS-... in the descr...
+    >>> print(original_message.get_content_type())
+    message/rfc822
+    >>> print(parseaddr(str(original_message.get_payload(0)['From']))[1])
+    [e-mail]
+    >>> print(original_message.get_payload(0)['To'])
+    [e-mail]
+    >>> print(original_message.get_payload(0)['X-Launchpad-Original-To'])
+    [e-mail]
+    >>> print(original_message.get_payload(0)['Subject'])
+    doesn't matter
+
 Unauthorized exceptions, which are ignored for the purpose of OOPS
 reporting in the web interface, are not ignored in the email interface.
 
diff --git a/lib/lp/services/mail/tests/test_incoming.py b/lib/lp/services/mail/tests/test_incoming.py
index 57735c3..15b751e 100644
--- a/lib/lp/services/mail/tests/test_incoming.py
+++ b/lib/lp/services/mail/tests/test_incoming.py
@@ -8,8 +8,10 @@ from email.header import (
     make_header,
     )
 from email.mime.multipart import MIMEMultipart
+from email.utils import parseaddr
 import logging
 import os
+from textwrap import dedent
 import unittest
 
 import six
@@ -277,6 +279,31 @@ class IncomingTestCase(TestCaseWithFactory):
             six.text_type(make_header(decode_header(
                 test_handler.handledMails[0]['From']))))
 
+    def test_invalid_from_address_no_mime_encoding(self):
+        # An invalid From: header with non-MIME-encoded non-ASCII characters
+        # is handled.
+        test_handler = FakeHandler()
+        mail_handlers.add('lp.dev', test_handler)
+        raw_message = dedent(u"""\
+            Content-Type: text/plain; charset="UTF-8"
+            MIME-Version: 1.0
+            Message-Id: <message-id>
+            To: [e-mail]
+            From: \u05D0 <[e-mail]>
+            Subject: subject
+
+            body
+            """).encode('UTF-8')
+        TestMailer().send(
+            u'\u05D0 <[e-mail]>'.encode('UTF-8'), '[e-mail]',
+            raw_message)
+        handleMail()
+        self.assertEqual([], self.oopses)
+        self.assertEqual(1, len(test_handler.handledMails))
+        self.assertEqual(
+            '[e-mail]',
+            parseaddr(str(test_handler.handledMails[0]['From']))[1])
+
     def test_invalid_cc_address_unicode(self):
         # Invalid Cc: header such as no "@" is handled.
         message, test_handler = self.makeSentMessage(