diff --git a/CHANGELOG.md b/CHANGELOG.md index 46da12db..97af067d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Changes + +- **mailsuite 2.3.3 or later is now required.** mailsuite 2.3.3 ([GHSA-9jx3-p8wm-g8vw](https://github.com/seanthegeek/mailsuite/security/advisories/GHSA-9jx3-p8wm-g8vw)) makes the Maildir connection return a message's stored bytes from `fetch_message()` and rename the file in `move_message()`, instead of re-serializing it. A report email that cannot be re-serialized (the [GHSA-qxwv-2vwj-v828](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-qxwv-2vwj-v828) messages) is therefore now moved to the `Invalid` folder on Maildir too, instead of staying in the inbox and being logged on every run. + ## 11.1.0 ### Security diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index bd60bfaf..85251238 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -2625,9 +2625,10 @@ def _fetch_mailbox_message( Returns ``(message_id, msg_content)``; ``message_id`` is the backend-appropriate id to use for later move/delete calls. - Some backends re-serialize the stored message while fetching it (the - Maildir backend calls ``Message.as_string()``), so the message's own - content can make the fetch fail: ``email.errors.HeaderWriteError`` on a + A backend that re-serializes the stored message while fetching it (the + Maildir backend of mailsuite before 2.3.3 called ``Message.as_string()``; + 2.3.3 returns the stored bytes) can fail on the message's own content: + ``email.errors.HeaderWriteError`` on a control character in a header, ``RecursionError`` on deeply nested multiparts. Those content errors are returned, not raised, as ``msg_content`` -- a ``ParserError`` the caller treats like a parse @@ -2675,14 +2676,14 @@ def _dispose_invalid_message( A delete or move the backend rejects is logged and the message is left in place, as the archival disposal of parsed reports does, rather than aborting the rest of the batch. Besides transient backend errors, this - covers messages the backend cannot rewrite: the Maildir backend moves a - message by storing it again with ``mailbox.Maildir.add()``, which - re-serializes it through ``email.generator.BytesGenerator``. That raises - ``RecursionError`` for deeply nested multiparts on every Python, and + covers messages the backend cannot rewrite: the Maildir backend of + mailsuite before 2.3.3 moved a message by storing it again with + ``mailbox.Maildir.add()``, which re-serializes it and raises + ``RecursionError`` for deeply nested multiparts (and ``email.errors.HeaderWriteError`` for a control character in a header - only where ``BytesGenerator`` also verifies generated headers (some - distribution builds; upstream CPython checks only the text generator - used by ``as_string()``) (GHSA-qxwv-2vwj-v828). + where ``BytesGenerator`` verifies headers), so such a message stayed in + the inbox. mailsuite 2.3.3 renames the file instead + (GHSA-qxwv-2vwj-v828, GHSA-9jx3-p8wm-g8vw). """ try: if delete: diff --git a/pyproject.toml b/pyproject.toml index 446af2da..ee3f021b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,7 @@ dependencies = [ # No extras: the base mailsuite supplies IMAP and Maildir connections # plus mail-parser. Gmail and Microsoft Graph come from the gmail and # msgraph extras below. - "mailsuite>=2.3.2", + "mailsuite>=2.3.3", "maxminddb>=2.0.0", "publicsuffixlist>=0.10.0", # Imported directly at utils.py:39 (dateutil.parser). It used to @@ -93,13 +93,13 @@ loganalytics = [ "azure-monitor-ingestion>=1.0.0", ] msgraph = [ - "mailsuite[msgraph]>=2.3.1", + "mailsuite[msgraph]>=2.3.3", # Imported directly in cli.py for Graph error handling; otherwise # only a transitive dep of mailsuite[msgraph] -> msgraph-sdk. "microsoft-kiota-abstractions>=1.8.0", ] gmail = [ - "mailsuite[gmail]>=2.3.1", + "mailsuite[gmail]>=2.3.3", ] # `all` must stay the union of every extra above — and must keep # excluding `postgresql`: psycopg's prebuilt binary wheels do not exist @@ -115,7 +115,7 @@ all = [ "boto3>=1.16.63", "elasticsearch>=8.18,<9", "kafka-python>=2.3.2", - "mailsuite[gmail,msgraph]>=2.3.1", + "mailsuite[gmail,msgraph]>=2.3.3", "microsoft-kiota-abstractions>=1.8.0", "opensearch-py>=2.4.2,<=4.0.0", "pygelf>=0.4.2", diff --git a/tests/test_init.py b/tests/test_init.py index e3a73ffd..be03d740 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -4975,46 +4975,30 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): "is not a poison message here" ) - def _maildir_stores_parsed(self, poison): - """Whether this Python's ``mailbox.Maildir.add()`` can store - ``poison`` once it has been read back as a message object, which is - what MaildirConnection.move_message() does.""" - scratch = mkdtemp(dir=self._tmp) - source = mailbox.Maildir(os.path.join(scratch, "source"), create=True) - destination = mailbox.Maildir(os.path.join(scratch, "dest"), create=True) - try: - destination.add(source[source.add(poison)]) - except (email.errors.MessageError, RecursionError): - return False - finally: - rmtree(scratch, ignore_errors=True) - return True - def test_poison_message_does_not_abort_the_batch(self): - """A message the Maildir backend cannot re-serialize while fetching - it no longer aborts the batch: the other report is still parsed and - archived, sequentially and with n_procs=2. + """A message that cannot be re-serialized no longer aborts the + batch: the other report is still parsed and archived, sequentially + and with n_procs=2, and the poison message is moved to + ``Archive/Invalid``. - Regression test for GHSA-qxwv-2vwj-v828: MaildirConnection's - fetch_message() calls Message.as_string(), so the + Regression test for GHSA-qxwv-2vwj-v828: parse_report_email() + serializes the message's parts, so the email.errors.HeaderWriteError (a control character in a sub-message header) or RecursionError (deeply nested multiparts) escaped from the - fetch, outside the per-message ``except ParserError``, and every - run aborted on the same message. + mailbox loop's per-message ``except ParserError`` and every run + aborted on the same message. parse_report_email() now raises a + ParserError, which logs "Unable to parse message with subject ..." + and disposes of the message as invalid. - The message is then disposed of as invalid. move_message() stores - it with ``mailbox.Maildir.add()``, which re-serializes it through - ``email.generator.BytesGenerator``. The nested message overflows the - stack there on every Python, so its move fails, is logged, and the - message stays in the INBOX instead of aborting the batch a second - way. Whether the control-character message can be stored depends on - the interpreter: upstream CPython (e.g. 3.12.9) applies the - CVE-2024-6923 header check only in the text ``Generator`` that - ``as_string()`` uses, so the move succeeds, while some distribution - builds (e.g. Ubuntu's 3.12.3) also apply it in ``BytesGenerator``, - so the move fails like the nested one. The expected outcome is - decided per message, at runtime.""" - kept_in_inbox = 0 + mailsuite 2.3.3 (GHSA-9jx3-p8wm-g8vw) returns the stored bytes from + fetch_message() and renames the file in move_message(), so neither + step re-serializes the message: the fetch succeeds and the failure + surfaces in the parser, and the move always succeeds. (Before 2.3.3 + the fetch raised, and the move re-added the message through + ``mailbox.Maildir.add()``, which left the nested message in the + inbox.) A Python whose ``Message.as_string()`` can write the + control-character message is skipped for that message, as it is not + a poison message there.""" for poison_name, poison in ( ("header", POISON_HEADER_EMAIL), ("nested", _nested_multipart_email(POISON_NESTING_LEVELS)), @@ -5022,15 +5006,10 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): if _reserializes(poison): # Not a poison message on this Python (see _reserializes). continue - stored = self._maildir_stores_parsed(poison) - if not stored: - kept_in_inbox += 1 for n_procs in (1, 2): with self.subTest(poison=poison_name, n_procs=n_procs): parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() - path, poison_key = self._poison_maildir( - f"{poison_name}-{n_procs}", poison - ) + path, _ = self._poison_maildir(f"{poison_name}-{n_procs}", poison) conn = MaildirConnection(path, maildir_create=True) with self.assertLogs("parsedmarc.log", level="WARNING") as cm: result = parsedmarc.get_dmarc_reports_from_mailbox( @@ -5048,32 +5027,18 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): self.assertEqual(len(conn.fetch_messages("Archive/Aggregate")), 1) self.assertTrue( any( - f"Unable to fetch message UID {poison_key}" in line + "Unable to parse message with subject" in line for line in cm.output ), cm.output, ) - move_errors = [ - line - for line in cm.output - if "Mailbox error: Error moving invalid message UID " - f"{poison_key}" in line - ] - if stored: - self.assertEqual(conn.fetch_messages("INBOX"), []) - self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) - self.assertFalse( - [line for line in cm.output if "Mailbox error" in line], - cm.output, - ) - else: - self.assertEqual(conn.fetch_messages("INBOX"), [poison_key]) - self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) - self.assertTrue(move_errors, cm.output) - # The nested message cannot be stored on any Python, so the - # kept-in-INBOX branch always runs; the stored branch runs for the - # control-character message on upstream CPython. - self.assertGreater(kept_in_inbox, 0) + # The poison message was moved, not left to fail again. + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) + self.assertFalse( + [line for line in cm.output if "Mailbox error" in line], + cm.output, + ) def test_poison_message_is_deleted_with_delete_invalid(self): """With delete_invalid, a message that failed to fetch is deleted