Barry Warsaw pushed to branch master at mailman / Mailman
Commits:
472eb976 by Barry Warsaw at 2016-03-20T21:31:50-04:00
Close #208
Don't let crashes in IArchiver plugins break handlers or runners.
- - - - -
8 changed files:
- src/mailman/docs/NEWS.rst
- src/mailman/handlers/decorate.py
- src/mailman/handlers/rfc_2369.py
- src/mailman/handlers/tests/test_decorate.py
- src/mailman/handlers/tests/test_rfc_2369.py
- src/mailman/model/mailinglist.py
- src/mailman/runners/archive.py
- src/mailman/runners/tests/test_archiver.py
Changes:
=====================================
src/mailman/docs/NEWS.rst
=====================================
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -71,6 +71,8 @@ Bugs
* Cross-posting messages held on both lists no longer fails. (Closes #176)
* Don't let unknown charsets crash the "approved" rule. Given by Aurélien
Bompard. (Closes #203)
+ * Don't let crashes in IArchiver plugins break handlers or runners.
+ (Closes #208)
Configuration
-------------
=====================================
src/mailman/handlers/decorate.py
=====================================
--- a/src/mailman/handlers/decorate.py
+++ b/src/mailman/handlers/decorate.py
@@ -40,6 +40,7 @@ from zope.interface import implementer
log = logging.getLogger('mailman.error')
+alog = logging.getLogger('mailman.archiver')
@@ -64,8 +65,14 @@ def process(mlist, msg, msgdata):
# the $<archive-name>_url placeholder for every enabled archiver.
for archiver in IListArchiverSet(mlist).archivers:
if archiver.is_enabled:
- # Get the permalink of the message from the archiver.
- archive_url = archiver.system_archiver.permalink(mlist, msg)
+ # Get the permalink of the message from the archiver. Watch out
+ # for exceptions in the archiver plugin.
+ try:
+ archive_url = archiver.system_archiver.permalink(mlist, msg)
+ except Exception:
+ alog.exception('Exception in "{}" archiver'.format(
+ archiver.system_archiver.name))
+ archive_url = None
if archive_url is not None:
placeholder = '{}_url'.format(archiver.system_archiver.name)
d[placeholder] = archive_url
=====================================
src/mailman/handlers/rfc_2369.py
=====================================
--- a/src/mailman/handlers/rfc_2369.py
+++ b/src/mailman/handlers/rfc_2369.py
@@ -22,6 +22,8 @@ __all__ = [
]
+import logging
+
from email.utils import formataddr
from mailman.core.i18n import _
from mailman.handlers.cook_headers import uheader
@@ -33,6 +35,8 @@ from zope.interface import implementer
CONTINUATION = ',\n\t'
+log = logging.getLogger('mailman.archiver')
+
def process(mlist, msg, msgdata):
@@ -85,11 +89,22 @@ def process(mlist, msg, msgdata):
for archiver in archiver_set.archivers:
if not archiver.is_enabled:
continue
- archiver_url = archiver.system_archiver.list_url(mlist)
+ # Watch out for exceptions in the archiver plugin.
+ try:
+ archiver_url = archiver.system_archiver.list_url(mlist)
+ except Exception:
+ log.exception('Exception in "{}" archiver'.format(
+ archiver.system_archiver.name))
+ archiver_url = None
if archiver_url is not None:
headers.append(('List-Archive',
'<{}>'.format(archiver_url)))
- permalink = archiver.system_archiver.permalink(mlist, msg)
+ try:
+ permalink = archiver.system_archiver.permalink(mlist, msg)
+ except Exception:
+ log.exception('Exception in "{}" archiver'.format(
+ archiver.system_archiver.name))
+ permalink = None
if permalink is not None:
headers.append(('Archived-At', '<{}>'.format(permalink)))
# XXX RFC 2369 also defines a List-Owner header which we are not currently
=====================================
src/mailman/handlers/tests/test_decorate.py
=====================================
--- a/src/mailman/handlers/tests/test_decorate.py
+++ b/src/mailman/handlers/tests/test_decorate.py
@@ -18,6 +18,7 @@
"""Test the decorate handler."""
__all__ = [
+ 'TestBrokenPermalink',
'TestDecorate',
]
@@ -29,7 +30,8 @@ from mailman.app.lifecycle import create_list
from mailman.config import config
from mailman.handlers import decorate
from mailman.interfaces.archiver import IArchiver
-from mailman.testing.helpers import specialized_message_from_string as mfs
+from mailman.testing.helpers import (
+ LogFileMark, specialized_message_from_string as mfs)
from mailman.testing.layers import ConfigLayer
from tempfile import TemporaryDirectory
from zope.interface import implementer
@@ -48,6 +50,16 @@ class TestArchiver:
return 'http://example.com/link_to_message'
+@implementer(IArchiver)
+class BrokenArchiver:
+ name = 'broken'
+ is_enabled = True
+
+ @staticmethod
+ def permalink(mlist, msg):
+ raise RuntimeError('Cannot get permalink')
+
+
class TestDecorate(unittest.TestCase):
"""Test the cook_headers handler."""
@@ -116,3 +128,46 @@ This is a test message.
decorate.process(self._mlist, self._msg, {})
self.assertIn('http://example.com/link_to_message',
self._msg.as_string())
+
+
+class TestBrokenPermalink(unittest.TestCase):
+ layer = ConfigLayer
+
+ def setUp(self):
+ self._mlist = create_list('a...@example.com')
+ self._msg = mfs("""\
+To: a...@example.com
+From: aper...@example.com
+Message-ID: <alpha>
+Content-Type: text/plain;
+
+This is a test message.
+""")
+ temporary_dir = TemporaryDirectory()
+ self.addCleanup(temporary_dir.cleanup)
+ template_dir = temporary_dir.name
+ config.push('archiver', """\
+ [paths.testing]
+ template_dir: {}
+ [archiver.testarchiver]
+ class: mailman.handlers.tests.test_decorate.BrokenArchiver
+ enable: yes
+ """.format(template_dir))
+ self.addCleanup(config.pop, 'archiver')
+
+ def test_broken_permalink(self):
+ # GL issue #208 - IArchive messages raise exceptions, breaking the
+ # rfc-2369 handler and shunting messages.
+ site_dir = os.path.join(config.TEMPLATE_DIR, 'site', 'en')
+ os.makedirs(site_dir)
+ footer_path = os.path.join(site_dir, 'myfooter.txt')
+ with open(footer_path, 'w', encoding='utf-8') as fp:
+ print('${broken_url}', file=fp)
+ self._mlist.footer_uri = 'mailman:///myfooter.txt'
+ self._mlist.preferred_language = 'en'
+ mark = LogFileMark('mailman.archiver')
+ decorate.process(self._mlist, self._msg, {})
+ log_messages = mark.read()
+ self.assertNotIn('http:', self._msg.as_string())
+ self.assertIn('Exception in "broken" archiver', log_messages)
+ self.assertIn('RuntimeError: Cannot get permalink', log_messages)
=====================================
src/mailman/handlers/tests/test_rfc_2369.py
=====================================
--- a/src/mailman/handlers/tests/test_rfc_2369.py
+++ b/src/mailman/handlers/tests/test_rfc_2369.py
@@ -28,7 +28,8 @@ from mailman.app.lifecycle import create_list
from mailman.config import config
from mailman.handlers import rfc_2369
from mailman.interfaces.archiver import ArchivePolicy, IArchiver
-from mailman.testing.helpers import specialized_message_from_string as mfs
+from mailman.testing.helpers import (
+ LogFileMark, specialized_message_from_string as mfs)
from mailman.testing.layers import ConfigLayer
from urllib.parse import urljoin
from zope.interface import implementer
@@ -56,6 +57,23 @@ class DummyArchiver:
return None
+@implementer(IArchiver)
+class BrokenArchiver:
+ """An archiver that has some broken methods."""
+
+ name = 'broken'
+
+ def list_url(self, mlist):
+ raise RuntimeError('Cannot get list URL')
+
+ def permalink(self, mlist, msg):
+ raise RuntimeError('Cannot get permalink')
+
+ @staticmethod
+ def archive_message(mlist, message):
+ raise RuntimeError('Cannot archive message')
+
+
class TestRFC2369(unittest.TestCase):
"""Test the rfc_2369 handler."""
@@ -123,3 +141,24 @@ Dummy text
rfc_2369.process(self._mlist, self._msg, {})
self.assertNotIn('List-Archive', self._msg)
self.assertNotIn('Archived-At', self._msg)
+
+ def test_broken_archiver(self):
+ # GL issue #208 - IArchive messages raise exceptions, breaking the
+ # rfc-2369 handler and shunting messages.
+ config.push('archiver', """
+ [archiver.broken]
+ class: {}.BrokenArchiver
+ enable: yes
+ """.format(BrokenArchiver.__module__))
+ self.addCleanup(config.pop, 'archiver')
+ mark = LogFileMark('mailman.archiver')
+ rfc_2369.process(self._mlist, self._msg, {})
+ log_messages = mark.read()
+ # Because .list_url() was broken, there will be no List-Archive header.
+ self.assertIsNone(self._msg.get('list-archive'))
+ self.assertIn('Exception in "broken" archiver', log_messages)
+ self.assertIn('RuntimeError: Cannot get list URL', log_messages)
+ # Because .permalink() was broken, there will be no Archived-At header.
+ self.assertIsNone(self._msg.get('archived-at'))
+ self.assertIn('Exception in "broken" archiver', log_messages)
+ self.assertIn('RuntimeError: Cannot get permalink', log_messages)
=====================================
src/mailman/model/mailinglist.py
=====================================
--- a/src/mailman/model/mailinglist.py
+++ b/src/mailman/model/mailinglist.py
@@ -605,7 +605,7 @@ class ListArchiverSet:
for archiver_name in system_archivers:
exists = store.query(ListArchiver).filter(
ListArchiver.mailing_list == mailing_list,
- ListArchiver.name == archiver_name).first()
+ ListArchiver.name == archiver_name).one_or_none()
if exists is None:
store.add(ListArchiver(mailing_list, archiver_name,
system_archivers[archiver_name]))
@@ -621,7 +621,7 @@ class ListArchiverSet:
def get(self, store, archiver_name):
return store.query(ListArchiver).filter(
ListArchiver.mailing_list == self._mailing_list,
- ListArchiver.name == archiver_name).first()
+ ListArchiver.name == archiver_name).one_or_none()
@implementer(IHeaderMatch)
=====================================
src/mailman/runners/archive.py
=====================================
--- a/src/mailman/runners/archive.py
+++ b/src/mailman/runners/archive.py
@@ -35,7 +35,7 @@ from mailman.utilities.datetime import RFC822_DATE_FMT, now
from mailman.interfaces.mailinglist import IListArchiverSet
-log = logging.getLogger('mailman.error')
+log = logging.getLogger('mailman.archiver')
@@ -106,4 +106,5 @@ class ArchiveRunner(Runner):
try:
archiver.system_archiver.archive_message(mlist, msg_copy)
except Exception:
- log.exception('Broken archiver: %s' % archiver.name)
+ log.exception('Exception in "{}" archiver'.format(
+ archiver.name))
=====================================
src/mailman/runners/tests/test_archiver.py
=====================================
--- a/src/mailman/runners/tests/test_archiver.py
+++ b/src/mailman/runners/tests/test_archiver.py
@@ -32,7 +32,7 @@ from mailman.interfaces.archiver import IArchiver
from mailman.interfaces.mailinglist import IListArchiverSet
from mailman.runners.archive import ArchiveRunner
from mailman.testing.helpers import (
- configuration, make_testable_runner,
+ LogFileMark, configuration, get_queue_messages, make_testable_runner,
specialized_message_from_string as mfs)
from mailman.testing.layers import ConfigLayer
from mailman.utilities.datetime import RFC822_DATE_FMT, factory, now
@@ -63,6 +63,23 @@ class DummyArchiver:
return path
+@implementer(IArchiver)
+class BrokenArchiver:
+ """An archiver that has some broken methods."""
+
+ name = 'broken'
+
+ def list_url(self, mlist):
+ raise RuntimeError('Cannot get list URL')
+
+ def permalink(self, mlist, msg):
+ raise RuntimeError('Cannot get permalink')
+
+ @staticmethod
+ def archive_message(mlist, message):
+ raise RuntimeError('Cannot archive message')
+
+
class TestArchiveRunner(unittest.TestCase):
"""Test the archive runner."""
@@ -77,6 +94,9 @@ class TestArchiveRunner(unittest.TestCase):
[archiver.dummy]
class: mailman.runners.tests.test_archiver.DummyArchiver
enable: no
+ [archiver.broken]
+ class: mailman.runners.tests.test_archiver.BrokenArchiver
+ enable: no
[archiver.prototype]
enable: no
[archiver.mhonarc]
@@ -84,6 +104,7 @@ class TestArchiveRunner(unittest.TestCase):
[archiver.mail_archive]
enable: no
""")
+ self.addCleanup(config.pop, 'dummy')
self._archiveq = config.switchboards['archive']
self._msg = mfs("""\
From: aper...@example.com
@@ -97,9 +118,6 @@ First post!
self._runner = make_testable_runner(ArchiveRunner)
IListArchiverSet(self._mlist).get('dummy').is_enabled = True
- def tearDown(self):
- config.pop('dummy')
-
@configuration('archiver.dummy', enable='yes')
def test_archive_runner(self):
# Ensure that the archive runner ends up archiving the message.
@@ -247,3 +265,21 @@ First post!
listid=self._mlist.list_id)
self._runner.run()
self.assertEqual(os.listdir(config.MESSAGES_DIR), [])
+
+ @configuration('archiver.broken', enable='yes')
+ def test_broken_archiver(self):
+ # GL issue #208 - IArchive messages raise exceptions, breaking the
+ # rfc-2369 handler and shunting messages.
+ mark = LogFileMark('mailman.archiver')
+ self._archiveq.enqueue(
+ self._msg, {},
+ listid=self._mlist.list_id,
+ received_time=now())
+ IListArchiverSet(self._mlist).get('broken').is_enabled = True
+ self._runner.run()
+ # The archiver is broken, so there are no messages on the file system,
+ # but there is a log message and the message was not shunted.
+ log_messages = mark.read()
+ self.assertIn('Exception in "broken" archiver', log_messages)
+ self.assertIn('RuntimeError: Cannot archive message', log_messages)
+ self.assertEqual(len(get_queue_messages('shunt')), 0)
View it on GitLab:
https://gitlab.com/mailman/mailman/commit/472eb9765f4163aad785f483584ff607f01fdeeb
_______________________________________________
Mailman-checkins mailing list
Mailman-checkins@python.org
Unsubscribe:
https://mail.python.org/mailman/options/mailman-checkins/archive%40jab.org
