From 522c0a73d70cb1aa76881376705c7384759580a9 Mon Sep 17 00:00:00 2001 From: Mark Sapiro Date: Tue, 13 Jun 2017 09:32:12 -0700 Subject: Catch FileNotFoundError and PermissionError on html_to_plain_text_command. --- src/mailman/docs/NEWS.rst | 6 ++++++ src/mailman/handlers/mime_delete.py | 5 ++++- src/mailman/handlers/tests/test_mimedel.py | 25 +++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst index 2bc2335f3..df8032284 100644 --- a/src/mailman/docs/NEWS.rst +++ b/src/mailman/docs/NEWS.rst @@ -12,6 +12,12 @@ Here is a history of user visible changes to Mailman. ============================== (201X-XX-XX) +Bugs +---- + * A missing html_to_plain_text_command is now properly detected and logged. + (closes #345) + + 3.1.0 -- "Between The Wheels" ============================= (2017-05-25) diff --git a/src/mailman/handlers/mime_delete.py b/src/mailman/handlers/mime_delete.py index 14aee1a0e..4f1b08ac7 100644 --- a/src/mailman/handlers/mime_delete.py +++ b/src/mailman/handlers/mime_delete.py @@ -279,7 +279,10 @@ def to_plaintext(msg): try: stdout = subprocess.check_output( command, universal_newlines=True) - except subprocess.CalledProcessError: + except (FileNotFoundError, + PermissionError, + subprocess.CalledProcessError, + ): log.exception('HTML -> text/plain command error') else: # Replace the payload of the subpart with the converted text diff --git a/src/mailman/handlers/tests/test_mimedel.py b/src/mailman/handlers/tests/test_mimedel.py index 3a50852da..40ebd4439 100644 --- a/src/mailman/handlers/tests/test_mimedel.py +++ b/src/mailman/handlers/tests/test_mimedel.py @@ -220,6 +220,31 @@ MIME-Version: 1.0 payload_lines = msg.get_payload().splitlines() self.assertEqual(payload_lines[0], 'Converted text/html to text/plain') + def test_missing_html_to_plain_text_command(self): + # Calling a missing html_to_plain_text_command is properly logged. + msg = mfs("""\ +From: aperson@example.com +Content-Type: text/html +MIME-Version: 1.0 + + + +""") + process = config.handlers['mime-delete'].process + config.push('html_filter', """\ +[mailman] +html_to_plain_text_command = /non/existent/path/to/bogus/command $filename +""") + self.addCleanup(config.pop, 'html_filter') + mark = LogFileMark('mailman.error') + process(self._mlist, msg, {}) + line = mark.readline()[:-1] + self.assertTrue(line.endswith('HTML -> text/plain command error')) + self.assertEqual(msg.get_content_type(), 'text/html') + self.assertIsNone(msg['x-content-filtered-by']) + payload_lines = msg.get_payload().splitlines() + self.assertEqual(payload_lines[0], '') + class TestMiscellaneous(unittest.TestCase): """Test various miscellaneous filtering actions.""" -- cgit v1.2.3-70-g09d2 From 249f47b3c947c4a64c5d0cca949e70f20a4d3644 Mon Sep 17 00:00:00 2001 From: Mark Sapiro Date: Thu, 15 Jun 2017 15:17:41 -0700 Subject: Did some reformatting and added tests. --- src/mailman/handlers/mime_delete.py | 10 ++--- src/mailman/handlers/tests/test_mimedel.py | 67 ++++++++++++++++++++++++++---- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/mailman/handlers/mime_delete.py b/src/mailman/handlers/mime_delete.py index 4f1b08ac7..662c773ae 100644 --- a/src/mailman/handlers/mime_delete.py +++ b/src/mailman/handlers/mime_delete.py @@ -28,7 +28,6 @@ import os import shutil import logging import tempfile -import subprocess from contextlib import ExitStack, suppress from email.iterators import typed_subpart_iterator @@ -46,6 +45,7 @@ from mailman.utilities.string import oneline from mailman.version import VERSION from public import public from string import Template +from subprocess import CalledProcessError, check_output from zope.interface import implementer @@ -277,12 +277,8 @@ def to_plaintext(msg): template = Template(config.mailman.html_to_plain_text_command) command = template.safe_substitute(filename=filename).split() try: - stdout = subprocess.check_output( - command, universal_newlines=True) - except (FileNotFoundError, - PermissionError, - subprocess.CalledProcessError, - ): + stdout = check_output(command, universal_newlines=True) + except (CalledProcessError, FileNotFoundError, PermissionError): log.exception('HTML -> text/plain command error') else: # Replace the payload of the subpart with the converted text diff --git a/src/mailman/handlers/tests/test_mimedel.py b/src/mailman/handlers/tests/test_mimedel.py index 40ebd4439..cde557d78 100644 --- a/src/mailman/handlers/tests/test_mimedel.py +++ b/src/mailman/handlers/tests/test_mimedel.py @@ -43,22 +43,34 @@ from zope.component import getUtility @contextmanager -def dummy_script(): +def dummy_script(arg=''): + exe = sys.executable + extra = '' + if arg == 'scripterr': + extra = 'error' with ExitStack() as resources: tempdir = tempfile.mkdtemp() resources.callback(shutil.rmtree, tempdir) filter_path = os.path.join(tempdir, 'filter.py') + if arg in ('noperm', 'nonexist'): + exe = filter_path with open(filter_path, 'w', encoding='utf-8') as fp: print("""\ import sys +if len(sys.argv) > 2: + sys.exit(1) print('Converted text/html to text/plain') print('Filename:', sys.argv[1]) """, file=fp) config.push('dummy script', """\ [mailman] -html_to_plain_text_command = {exe} {script} $filename -""".format(exe=sys.executable, script=filter_path)) +html_to_plain_text_command = {exe} {script} {extra} $filename +""".format(exe=exe, script=filter_path, extra=extra)) resources.callback(config.pop, 'dummy script') + if arg == 'nonexist': + os.rename(filter_path, filter_path + 'xxx') + elif arg == 'noperm': + os.chmod(filter_path, 0o644) yield @@ -220,6 +232,27 @@ MIME-Version: 1.0 payload_lines = msg.get_payload().splitlines() self.assertEqual(payload_lines[0], 'Converted text/html to text/plain') + def test_convert_html_to_plaintext_error_return(self): + # Calling a script which returns an error status is properly logged. + msg = mfs("""\ +From: aperson@example.com +Content-Type: text/html +MIME-Version: 1.0 + + + +""") + process = config.handlers['mime-delete'].process + mark = LogFileMark('mailman.error') + with dummy_script('scripterr'): + process(self._mlist, msg, {}) + line = mark.readline()[:-1] + self.assertTrue(line.endswith('HTML -> text/plain command error')) + self.assertEqual(msg.get_content_type(), 'text/html') + self.assertIsNone(msg['x-content-filtered-by']) + payload_lines = msg.get_payload().splitlines() + self.assertEqual(payload_lines[0], '') + def test_missing_html_to_plain_text_command(self): # Calling a missing html_to_plain_text_command is properly logged. msg = mfs("""\ @@ -231,13 +264,31 @@ MIME-Version: 1.0 """) process = config.handlers['mime-delete'].process - config.push('html_filter', """\ -[mailman] -html_to_plain_text_command = /non/existent/path/to/bogus/command $filename + mark = LogFileMark('mailman.error') + with dummy_script('nonexist'): + process(self._mlist, msg, {}) + line = mark.readline()[:-1] + self.assertTrue(line.endswith('HTML -> text/plain command error')) + self.assertEqual(msg.get_content_type(), 'text/html') + self.assertIsNone(msg['x-content-filtered-by']) + payload_lines = msg.get_payload().splitlines() + self.assertEqual(payload_lines[0], '') + + def test_no_permission_html_to_plain_text_command(self): + # Calling an html_to_plain_text_command without permission is + # properly logged. + msg = mfs("""\ +From: aperson@example.com +Content-Type: text/html +MIME-Version: 1.0 + + + """) - self.addCleanup(config.pop, 'html_filter') + process = config.handlers['mime-delete'].process mark = LogFileMark('mailman.error') - process(self._mlist, msg, {}) + with dummy_script('noperm'): + process(self._mlist, msg, {}) line = mark.readline()[:-1] self.assertTrue(line.endswith('HTML -> text/plain command error')) self.assertEqual(msg.get_content_type(), 'text/html') -- cgit v1.2.3-70-g09d2