launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-process-mail into launchpad:master.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/405924

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-process-mail into launchpad:master.

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 <bet@xxxxxxxxxxxxx>
+    ... To: launchpad@xxxxxxxx
+    ... X-Launchpad-Original-To: launchpad@xxxxxxxx
+    ... 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 <bet@xxxxxxxxxxxxx>'
+    >>> 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])
+    bet@xxxxxxxxxxxxx
+    >>> print(original_message.get_payload(0)['To'])
+    launchpad@xxxxxxxx
+    >>> print(original_message.get_payload(0)['X-Launchpad-Original-To'])
+    launchpad@xxxxxxxx
+    >>> 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: test@xxxxxx
+            From: \u05D0 <alef@xxxxxx>
+            Subject: subject
+
+            body
+            """).encode('UTF-8')
+        TestMailer().send(
+            u'\u05D0 <alef@xxxxxx>'.encode('UTF-8'), 'test@xxxxxx',
+            raw_message)
+        handleMail()
+        self.assertEqual([], self.oopses)
+        self.assertEqual(1, len(test_handler.handledMails))
+        self.assertEqual(
+            'alef@xxxxxx',
+            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(