Merge lp:~mbp/launchpad/893612-mail-too-big into lp:launchpad

[Martin Pool]

Detect incoming too-big email messages early, and give a notication not an oops

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.

[Ian Booth (wallyworld)]

Excellent fix. Thank you.

review: Approve

=== 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:22:18 +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."""
@@ -62,6 +62,10 @@
 # Match trailing whitespace.
 trailing_whitespace = re.compile(r'[ \t]*((?=\r\n)|$)')
 
+# this is a hard limit on the size of email we will be willing to store in
+# the database.
+MAX_EMAIL_SIZE = 10 * 1024 * 1024
+
 
 def canonicalise_line_endings(text):
     r"""Canonicalise the line endings to '\r\n'.
@@ -461,6 +465,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:22:18 +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:22:18 +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, '[e-mail]', '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:22:18 +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
@@ -85,10 +85,6 @@
     )
 from lp.services.propertycache import cachedproperty
 
-# this is a hard limit on the size of email we will be willing to store in
-# the database.
-MAX_EMAIL_SIZE = 10 * 1024 * 1024
-
 
 def utcdatetime_from_field(field_value):
     """Turn an RFC 2822 Date: header value into a Python datetime (UTC).
@@ -310,10 +306,7 @@
         if not rfc822msgid:
             raise InvalidEmailMessage('Missing Message-Id')
 
-        # make sure we don't process anything too long
-        if len(email_message) > MAX_EMAIL_SIZE:
-            raise InvalidEmailMessage('Msg %s size %d exceeds limit %d' % (
-                rfc822msgid, len(email_message), MAX_EMAIL_SIZE))
+        # Over-long messages are checked for at the handle_on_message level.
 
         # Stuff a copy of the raw email into the Librarian, if it isn't
         # already in there.