launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-612754-2 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #612754 Submit Request Failure: Signature couldn't be verified: (7, 8, u'Bad signature') - with email signed and sent from sup-mail
  https://bugs.launchpad.net/bugs/612754


Bug 612754 is about people having perfectly valid GPG signed messages
rejected by LP.

It turns out that they were rejected because LP doesn't remove trailing
whitespace before checking the signature (as required by RFC 4880 and
implemented in GPG).  The fix was to remove the trailing whitespace as
part of the newline canonicalization.

The test added by this branch can be run by

    bin/test -c test_signedmessage

while more email tests can be run by

    bin/test -c mail

Lint fixed:
./lib/canonical/launchpad/mail/incoming.py
      64: E302 expected 2 blank lines, found 1
     156: W291 trailing whitespace
     345: E202 whitespace before ')'
     358: E202 whitespace before ')'
     407: E202 whitespace before ')'
     156: Line has trailing whitespace.
./lib/lp/services/mail/tests/test_signedmessage.py
      39: E501 line too long (86 characters)
     169: W391 blank line at end of file
      39: Line exceeds 78 characters.

Recommended listening for this MP: http://www.youtube.com/watch?v=o7gw_2W_oCU
-- 
https://code.launchpad.net/~benji/launchpad/bug-612754-2/+merge/37797
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-612754-2 into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/mail/incoming.py'
--- lib/canonical/launchpad/mail/incoming.py	2010-10-03 15:30:06 +0000
+++ lib/canonical/launchpad/mail/incoming.py	2010-10-06 21:35:10 +0000
@@ -55,9 +55,11 @@
 from lp.services.mail.signedmessage import signed_message_from_string
 
 # Match '\n' and '\r' line endings. That is, all '\r' that are not
-# followed by a # '\n', and all '\n' that are not preceded by a '\r'.
+# followed by a '\n', and all '\n' that are not preceded by a '\r'.
 non_canonicalised_line_endings = re.compile('((?<!\r)\n)|(\r(?!\n))')
 
+# Match trailing whitespace.
+trailing_whitespace = re.compile(' *$')
 
 def canonicalise_line_endings(text):
     r"""Canonicalise the line endings to '\r\n'.
@@ -73,6 +75,8 @@
     """
     if non_canonicalised_line_endings.search(text):
         text = non_canonicalised_line_endings.sub('\r\n', text)
+    if trailing_whitespace.search(text):
+        text = trailing_whitespace.sub('', text)
     return text
 
 

=== modified file 'lib/lp/services/mail/tests/test_signedmessage.py'
--- lib/lp/services/mail/tests/test_signedmessage.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/mail/tests/test_signedmessage.py	2010-10-06 21:35:10 +0000
@@ -59,17 +59,18 @@
             IWeaklyAuthenticatedPrincipal.providedBy(principle))
         self.assertIs(None, msg.signature)
 
-    def _get_clearsigned_for_person(self, sender):
+    def _get_clearsigned_for_person(self, sender, body=None):
         # Create a signed message for the sender specified with the test
         # secret key.
         key = import_secret_test_key()
         signing_context = GPGSigningContext(key.fingerprint, password='test')
-        body = dedent("""\
-            This is a multi-line body.
+        if body is None:
+            body = dedent("""\
+                This is a multi-line body.
 
-            Sincerely,
-            Your friendly tester.
-            """)
+                Sincerely,
+                Your friendly tester.
+                """)
         msg = self.factory.makeSignedMessage(
             email_address=sender.preferredemail.email,
             body=body, signing_context=signing_context)
@@ -97,6 +98,18 @@
         self.assertFalse(
             IWeaklyAuthenticatedPrincipal.providedBy(principle))
 
+    def test_trailing_whitespace(self):
+        # Trailing whitespace should be ignored when verifying a message's
+        # signature.
+        sender = getUtility(IPersonSet).getByEmail('test@xxxxxxxxxxxxx')
+        body = 'A message with trailing whitespace.   '
+        msg = self._get_clearsigned_for_person(sender, body)
+        principle = authenticateEmail(msg)
+        self.assertIsNot(None, msg.signature)
+        self.assertEqual(sender, principle.person)
+        self.assertFalse(
+            IWeaklyAuthenticatedPrincipal.providedBy(principle))
+
     def _get_detached_message_for_person(self, sender):
         # Return a signed message that contains a detached signature.
         body = dedent("""\