Merge lp:~benji/launchpad/bug-612754-2 into lp:launchpad

Proposed by Benji York
Status: Merged
Approved by: Benji York
Approved revision: no longer in the source branch.
Merged at revision: 11744
Proposed branch: lp:~benji/launchpad/bug-612754-2
Merge into: lp:launchpad
Diff against target: 76 lines (+27/-7)
2 files modified
lib/lp/services/mail/incoming.py (+5/-1)
lib/lp/services/mail/tests/test_signedmessage.py (+22/-6)
To merge this branch: bzr merge lp:~benji/launchpad/bug-612754-2
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Review via email: mp+37797@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :
Download full text (3.4 KiB)

Hi Benji,

I think the regex and the test need some work to handle trailing whitespace on any line.

-Edwin

>=== 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 22:26:05 +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(' *$')

Since you are not using rstrip(), I assume you want the regex to
strip the whitespace at the end of each line. In that case, you need
to tell re.compile() that you want to use a multiline expression.
Also, " *$" will not match " \r\n".

> 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 22:26:05 +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('<email address hidden>')
>+ body = 'A message with trailing whitespace. '

A multiline body with trailing whitespace on each line will fail.

>+ msg = self._get_clearsigned_for_person(sender, body)
>+ principle = authenticateEmail(msg)
>+ self.assertIsNot(None, msg.sig...

Read more...

review: Needs Fixing (code)
Revision history for this message
Benji York (benji) wrote :

On Wed, Oct 6, 2010 at 6:54 PM, Edwin Grubbs <email address hidden> wrote:
> Review: Needs Fixing code
> Hi Benji,
>
> I think the regex and the test need some work to handle trailing whitespace
> on any line.

That's odd. I had made a change along those lines before even
submitting the branch for review but the diff on the MP doesn't reflect
the change.

Darn! I forgot to push after commit. The more appropriate regex and
test should be visible after the diff updates.
--
Benji York

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi Benji,

It looks so much better since I have all your revisions.

-Edwin

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/mail/incoming.py'
2--- lib/lp/services/mail/incoming.py 2010-10-12 14:03:44 +0000
3+++ lib/lp/services/mail/incoming.py 2010-10-18 14:49:45 +0000
4@@ -55,9 +55,11 @@
5 from lp.services.mail.signedmessage import signed_message_from_string
6
7 # Match '\n' and '\r' line endings. That is, all '\r' that are not
8-# followed by a # '\n', and all '\n' that are not preceded by a '\r'.
9+# followed by a '\n', and all '\n' that are not preceded by a '\r'.
10 non_canonicalised_line_endings = re.compile('((?<!\r)\n)|(\r(?!\n))')
11
12+# Match trailing whitespace.
13+trailing_whitespace = re.compile(r'[ \t]*((?=\r\n)|$)')
14
15 def canonicalise_line_endings(text):
16 r"""Canonicalise the line endings to '\r\n'.
17@@ -73,6 +75,8 @@
18 """
19 if non_canonicalised_line_endings.search(text):
20 text = non_canonicalised_line_endings.sub('\r\n', text)
21+ if trailing_whitespace.search(text):
22+ text = trailing_whitespace.sub('', text)
23 return text
24
25
26
27=== modified file 'lib/lp/services/mail/tests/test_signedmessage.py'
28--- lib/lp/services/mail/tests/test_signedmessage.py 2010-10-11 17:56:08 +0000
29+++ lib/lp/services/mail/tests/test_signedmessage.py 2010-10-18 14:49:45 +0000
30@@ -59,17 +59,18 @@
31 IWeaklyAuthenticatedPrincipal.providedBy(principle))
32 self.assertIs(None, msg.signature)
33
34- def _get_clearsigned_for_person(self, sender):
35+ def _get_clearsigned_for_person(self, sender, body=None):
36 # Create a signed message for the sender specified with the test
37 # secret key.
38 key = import_secret_test_key()
39 signing_context = GPGSigningContext(key.fingerprint, password='test')
40- body = dedent("""\
41- This is a multi-line body.
42+ if body is None:
43+ body = dedent("""\
44+ This is a multi-line body.
45
46- Sincerely,
47- Your friendly tester.
48- """)
49+ Sincerely,
50+ Your friendly tester.
51+ """)
52 msg = self.factory.makeSignedMessage(
53 email_address=sender.preferredemail.email,
54 body=body, signing_context=signing_context)
55@@ -97,6 +98,21 @@
56 self.assertFalse(
57 IWeaklyAuthenticatedPrincipal.providedBy(principle))
58
59+ def test_trailing_whitespace(self):
60+ # Trailing whitespace should be ignored when verifying a message's
61+ # signature.
62+ sender = getUtility(IPersonSet).getByEmail('test@canonical.com')
63+ body = (
64+ 'A message with trailing spaces. \n'+
65+ 'And tabs\t\t\n'+
66+ 'Also mixed. \t ')
67+ msg = self._get_clearsigned_for_person(sender, body)
68+ principle = authenticateEmail(msg)
69+ self.assertIsNot(None, msg.signature)
70+ self.assertEqual(sender, principle.person)
71+ self.assertFalse(
72+ IWeaklyAuthenticatedPrincipal.providedBy(principle))
73+
74 def _get_detached_message_for_person(self, sender):
75 # Return a signed message that contains a detached signature.
76 body = dedent("""\