Merge lp:~msapiro/mailman/bug_949924 into lp:mailman

[Mark Sapiro]

This branch fixes mailman/rules/approved.py LP: #949924.

It is a very minimal fix. The issue is if we're looking for a Approve(d): pseudo header in the first text/plain part, and this part contains non-ascii characters, the statement "if ':' in line:" does an implicit decode to unicode which throws an exception on the non-ascii character. This apparently doesn't fail in 2.1 because 2.1 does line.find(':') instead. I fixed it by doing an explicit decode in the charset of the part with 'replace'.

I also changed a <> to != for Python 3

I didn't make any tests or NEWS entry. I did locally modify mailman/rules/docs/approve.rst (note the name discrepancy - approved.py vs approve.rst) like this:

=== modified file 'src/mailman/rules/docs/approve.rst'
--- src/mailman/rules/docs/approve.rst 2011-09-24 01:42:39 +0000
+++ src/mailman/rules/docs/approve.rst 2012-03-15 23:19:09 +0000
@@ -150,20 +150,25 @@

     >>> msg = message_from_string("""\
     ... From: <email address hidden>
+ ... MIME-Version: 1.0
+ ... Content-Type: text/plain; charset="iso-8859-1"
+ ... Content-Transfer-Encoding: quoted-printable
     ...
     ... Approved: abcxyz
     ... An important message.
+ ... A funny character =E4.
     ... """)
     >>> rule.check(mlist, msg, {})
     True

     >>> print msg.as_string()
     From: <email address hidden>
- Content-Transfer-Encoding: 7bit
+ Content-Transfer-Encoding: quoted-printable
     MIME-Version: 1.0
- Content-Type: text/plain; charset="us-ascii"
+ Content-Type: text/plain; charset="iso-8859-1"
     <BLANKLINE>
     An important message.
+ A funny character =E4.
     <BLANKLINE>

 As before, a mismatch in the pseudo-header does not approve the message, but

This does actually throw an exception without the fix and passes with it, but this is not the right place for this test, and there is no existing unit test module, and I wanted to expose this fix before taking the time to learn to construct a proper unit test.

There is also an extension to Approve.py in 2.1 which will reject the post if the (X-)Approve(d): header can't be found in an HTML part, but can be found after stripping out HTML tags. See comments #6 - #8 at LP: #266220 for more on this. I thought it was too much to try to incorporate that with this fix.

[lp:~msapiro/mailman/bug_949924]

7123. By Mark Sapiro on 2012-03-18

Merged from trunk.

7124. By Mark Sapiro on 2012-03-18

 * Added a unit test for rules/approved.py
 * Renamed rules/docs/approve.py to approved.py for consistency.

[Mark Sapiro (msapiro)]

I added a unit test. It probably needs some cleanup as I still don't really understand the unittest framework.

I renamed the approve.rst doctest to approved.rst for consistency.

I added a bug fix note to NEWS.

[Barry Warsaw (barry)]

Thanks for backporting this fix to mm3 Mark! Here are some comments:

I noticed a lot of unused imports in test_approved.py. You might want to look into pyflakes (what I use) or pylint as tools to help find these things. I'll clean them up before I commit though.

I usually like to add a comment to the test_foo() method to explain exactly what the test is checking. Sometimes it's obvious, but you'd be surprised when looking at a test from the outside. :) Also, the comment can refer to the bug #. Since you expect the execution of the rule to not raise an exception, the test does not need to transform UnicodeError and UnicodeWarnings into AssertionErrors. Just let any UnicodeError that occurs cause the test to fail. So really, all you're checking is that the rule returns False, for which you can just use self.assertFalse().

I rewrote this line in approved.py:

cset = part.get_content_charset('us-ascii')

instead of using the 'or'.

All minor stuff. Thanks! Branch approved and I'll land it on trunk.

(Aside: I noticed that we're assuming the moderator password is stored in the clear, so after I land your branch I'm going to be sure it's hashed properly. I'll do that in a separate commit though.)

review: Approve

[Mark Sapiro (msapiro)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/4/2012 2:18 PM, Barry Warsaw wrote:
>
> I noticed a lot of unused imports in test_approved.py. You might
> want to look into pyflakes (what I use) or pylint as tools to help
> find these things. I'll clean them up before I commit though.

Yes, I copied another unit test as a start point and I wasn't totally
sure if any of that stuff was required by the framework.

> I usually like to add a comment to the test_foo() method to
> explain exactly what the test is checking. Sometimes it's obvious,
> but you'd be surprised when looking at a test from the outside. :)
> Also, the comment can refer to the bug #. Since you expect the
> execution of the rule to not raise an exception, the test does not
> need to transform UnicodeError and UnicodeWarnings into
> AssertionErrors. Just let any UnicodeError that occurs cause the
> test to fail. So really, all you're checking is that the rule
> returns False, for which you can just use self.assertFalse().

Thanks.

> I rewrote this line in approved.py:
>
> cset = part.get_content_charset('us-ascii')
>
> instead of using the 'or'.

There's a reason for the or. Tokio said there are MUAs (wierd Japanese
ones) that do things like

Content-Type: xxx/xxx; charset=""

for the above, part.get_content_charset('us-ascii') returns the null
string. One could argue that the email package should handle this, but
that's not the case today. I should have commented that to indicate
why I was doing it. :(

> All minor stuff. Thanks! Branch approved and I'll land it on
> trunk.
>
> (Aside: I noticed that we're assuming the moderator password is
> stored in the clear, so after I land your branch I'm going to be
> sure it's hashed properly. I'll do that in a separate commit
> though.)

OK. Sounds good.

- --
Mark Sapiro <email address hidden> The highway is for gamblers,
San Francisco Bay Area, California better use your sense - B. Dylan

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)

iD8DBQFPfNd4VVuXXpU7hpMRAsrjAKCEKhnswux6ImO7jRprKqGGTVrXzwCgrzH5
ws4+GkSxBGwC30+h5RL4kew=
=3nS1
-----END PGP SIGNATURE-----

=== modified file 'src/mailman/docs/NEWS.rst'
--- src/mailman/docs/NEWS.rst	2012-03-17 16:54:26 +0000
+++ src/mailman/docs/NEWS.rst	2012-03-18 22:11:19 +0000
@@ -126,6 +126,8 @@
 
 Bug fixes
 ---------
+ * Fixed a UnicodeError with non-ascii message bodies in rules/approved.py
+   (LP: #949924)
  * Subscription disabled probe warning notification messages are now sent
    without a `Precedence:` header.  Given by Mark Sapiro. (LP: #808821)
  * Fixed KeyError in retry runner, contributed by Stephen A. Goss.

=== modified file 'src/mailman/rules/approved.py'
--- src/mailman/rules/approved.py	2012-01-01 19:14:46 +0000
+++ src/mailman/rules/approved.py	2012-03-18 22:11:19 +0000
@@ -73,10 +73,12 @@
                 break
             payload = part.get_payload(decode=True)
             if payload is not None:
+                cset = part.get_content_charset() or 'us-ascii'
+                payload = payload.decode(cset, 'replace')
                 line = ''
                 lines = payload.splitlines(True)
                 for lineno, line in enumerate(lines):
-                    if line.strip() <> '':
+                    if line.strip() != '':
                         break
                 if ':' in line:
                     header, value = line.split(':', 1)

=== renamed file 'src/mailman/rules/docs/approve.rst' => 'src/mailman/rules/docs/approved.rst'
=== added directory 'src/mailman/rules/tests'
=== added file 'src/mailman/rules/tests/__init__.py'
=== added file 'src/mailman/rules/tests/test_approved.py'
--- src/mailman/rules/tests/test_approved.py	1970-01-01 00:00:00 +0000
+++ src/mailman/rules/tests/test_approved.py	2012-03-18 22:11:19 +0000
@@ -0,0 +1,74 @@
+# Copyright (C) 2012 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test the mime_delete handler."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'TestApproved',
+    ]
+
+
+import unittest
+
+from zope.component import getUtility
+
+from mailman.app.lifecycle import create_list
+from mailman.config import config
+from mailman.core import errors
+from mailman.interfaces.action import FilterAction
+from mailman.interfaces.member import MemberRole
+from mailman.interfaces.usermanager import IUserManager
+from mailman.rules import approved
+from mailman.testing.helpers import (
+    LogFileMark,
+    get_queue_messages,
+    specialized_message_from_string as mfs)
+from mailman.testing.layers import ConfigLayer
+
+
+

+class TestApproved(unittest.TestCase):
+    """Test the approved handler."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('[e-mail]')
+        self._rule = approved.Approved()
+        self._msg = mfs("""\
+From: [e-mail]
+To: [e-mail]
+Subject: A Message with non-ascii body
+Message-ID: <ant>
+MIME-Version: 1.0
+Content-Type: text/plain; charset="iso-8859-1"
+Content-Transfer-Encoding: quoted-printable
+
+This is a message body with a non-ascii character =E4
+
+""")
+
+    def test_approved_nonascii(self):
+        result = True
+        try:
+            result = self._rule.check(self._mlist, self._msg, {})
+        except (UnicodeError, UnicodeWarning):
+            raise AssertionError('Non-ascii message raised UnicodeError')
+        self.assertEqual(result, False)