launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/893612-mail-too-big into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #893612 in Launchpad itself: "Email too big message not helpful"
  https://bugs.launchpad.net/launchpad/+bug/893612

For more details, see:
https://code.launchpad.net/~mbp/launchpad/893612-mail-too-big/+merge/101865

At the moment, if you send an >10MB message to Launchpad, it records an oops, and sends a message back to the user saying there was an oops.

This happens fairly late in processing, while running the handler.

This patch changes it so we do an up-front check as early as is easy, and give the user a regular error notification.  This ought to eliminate one stream of oopses (I don't know if it's very common.)

There's a unit test and I also tried running a message through process-one-mail.  I also confirmed the bug's still live on lp.
-- 
https://code.launchpad.net/~mbp/launchpad/893612-mail-too-big/+merge/101865
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/893612-mail-too-big into lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2012-01-04 03:23:19 +0000
+++ lib/lp/services/mail/incoming.py	2012-04-13 07:12:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions dealing with mails coming into Launchpad."""
@@ -45,6 +45,7 @@
 from lp.services.mail.notification import send_process_error_notification
 from lp.services.mail.sendmail import do_paranoid_envelope_to_validation
 from lp.services.mail.signedmessage import signed_message_from_string
+from lp.services.messages.model.message import MAX_EMAIL_SIZE
 from lp.services.webapp.errorlog import (
     ErrorReportingUtility,
     ScriptRequest,
@@ -461,6 +462,22 @@
         log.info("Got a message with a precedence header.")
         return
 
+    if mail.raw_length > MAX_EMAIL_SIZE:
+        complaint = (
+            "The mail you sent to Launchpad is too long.\n\n"
+            "Your message <%s>\nwas %d MB and the limit is %d MB." %
+            (mail['message-id'], mail.raw_length / 1e6, MAX_EMAIL_SIZE / 1e6))
+        log.info(complaint)
+        # It's probably big and it's probably mostly binary, so trim it pretty
+        # aggressively.
+        send_process_error_notification(
+            mail['From'],
+            'Mail to Launchpad was too large',
+            complaint,
+            mail,
+            max_return_size=8192)
+        return
+
     try:
         principal = authenticateEmail(
             mail, signature_timestamp_checker)

=== modified file 'lib/lp/services/mail/signedmessage.py'
--- lib/lp/services/mail/signedmessage.py	2011-08-12 14:36:25 +0000
+++ lib/lp/services/mail/signedmessage.py	2012-04-13 07:12:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Classes for simpler handling of PGP signed email messages."""
@@ -153,6 +153,11 @@
         signature, signed_content = self._getSignatureAndSignedContent()
         return signature
 
+    @property
+    def raw_length(self):
+        """Return the length in bytes of the underlying raw form."""
+        return len(self.parsed_string)
+
 
 def strip_pgp_signature(text):
     """Strip any PGP signature from the supplied text."""

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2012-01-20 15:42:44 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2012-04-13 07:12:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from doctest import DocTestSuite
@@ -69,6 +69,32 @@
             "(7, 58, u'No data')",
             body)
 
+    def test_mail_too_big(self):
+        """Much-too-big mail should generate a bounce, not an OOPS.
+
+        See <https://bugs.launchpad.net/launchpad/+bug/893612>.
+        """
+        person = self.factory.makePerson()
+        transaction.commit()
+        email_address = person.preferredemail.email
+        fat_body = '\n'.join(
+            ['some big mail with this line repeated many many times\n']
+            * 1000000)
+        ctrl = MailController(
+            email_address, 'to@xxxxxxxxxxx', 'subject', fat_body,
+            bulk=False)
+        ctrl.send()
+        handleMail()
+        self.assertEqual([], self.oopses)
+        [notification] = pop_notifications()
+        body = notification.get_payload()[0].get_payload(decode=True)
+        self.assertIn(
+            "The mail you sent to Launchpad is too long.",
+            body)
+        self.assertIn(
+            "was 55 MB\nand the limit is 10 MB.",
+            body)
+
     def test_invalid_to_addresses(self):
         """Invalid To: header should not be handled as an OOPS."""
         raw_mail = open(os.path.join(

=== modified file 'lib/lp/services/messages/model/message.py'
--- lib/lp/services/messages/model/message.py	2012-01-06 15:07:17 +0000
+++ lib/lp/services/messages/model/message.py	2012-04-13 07:12:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'DirectEmailAuthorization',
+    'MAX_EMAIL_SIZE',
     'Message',
     'MessageChunk',
     'MessageJob',
@@ -310,7 +311,10 @@
         if not rfc822msgid:
             raise InvalidEmailMessage('Missing Message-Id')
 
-        # make sure we don't process anything too long
+        # Make sure we don't process anything too long.  Large messages should
+        # normally be rejected at the handle_one_mail level, which gives a
+        # nice message back to the user, but we check again here in case there
+        # are other paths.
         if len(email_message) > MAX_EMAIL_SIZE:
             raise InvalidEmailMessage('Msg %s size %d exceeds limit %d' % (
                 rfc822msgid, len(email_message), MAX_EMAIL_SIZE))