From bbd7b044179a1aa41d3473e373c98848ee1d927d Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:09:45 -0500 Subject: [PATCH] Skip a message with an invalid report attachment instead of aborting the mbox (#955) * Skip a message with an invalid report attachment instead of aborting the mbox parse_report_email() raised a bare ParserError when a report attachment was invalid, which get_dmarc_reports_from_mbox() re-raises, so one such message ended the whole file. Raise InvalidDMARCReport (a ParserError subclass), which the mbox loop skips with a warning. Co-Authored-By: Claude Sonnet 5.5 * Skip an invalid SMTP TLS attachment in an mbox too Address the Copilot review of #955. An invalid SMTP TLS attachment raised InvalidSMTPTLSReport, which is not an InvalidDMARCReport, so it still stopped the mbox. parse_report_email() now names the message's subject in that error, and both mbox loops skip it with a warning. A message that cannot be parsed at all still raises a plain ParserError. Co-Authored-By: Claude Fable 5.2 --------- Co-authored-by: Claude Sonnet 5.5 --- CHANGELOG.md | 1 + parsedmarc/__init__.py | 21 ++++++- tests/test_init.py | 132 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63fe2296..bec2ea9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Bug fixes +- **One invalid report attachment no longer stops the rest of an mbox file from being parsed.** `parse_report_email()` raised a bare `ParserError` for a message whose aggregate XML or SMTP TLS JSON attachment was invalid (for example an aggregate report whose date range spans more than 366 days), and the mbox loop only skipped `InvalidDMARCReport`, so that one message aborted the whole file. It now raises `InvalidDMARCReport`, or `InvalidSMTPTLSReport` for an SMTP TLS attachment (both still `ParserError` subclasses), and the mbox loop skips both with a warning and parses the rest. A message that cannot be parsed at all still stops the file with a `ParserError`. Mailbox connections were not affected. Reported by mjatin2007. - **Unknown child elements under an aggregate record's `` and inside a `` are no longer copied into the parsed report** ([#948](https://github.com/domainaware/parsedmarc/issues/948)). `_parse_report_record()` copied both mappings whole, so an element that RFC 9990 Appendix A and RFC 7489 Appendix C do not define reached the JSON, Kafka, S3 and webhook outputs with whatever structure the report gave it. Only `header_from`, `envelope_from` and `envelope_to`, and a reason's `type` and `comment`, are kept. **Compatibility:** a consumer of those outputs that read such an extra key no longer receives it. ## 11.2.2 diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index 444cac90..dcf7aec5 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -2834,6 +2834,11 @@ def parse_report_email( Raises: ParserError: If the message is not a valid report. Whatever the input, only ``ParserError`` subclasses are raised. + A message that is not a valid report, or whose attachment is + not one, raises ``InvalidDMARCReport``, or + ``InvalidSMTPTLSReport`` for an SMTP TLS attachment (both are + ``ParserError`` subclasses). Any other failure to parse the + message raises a plain ``ParserError``. """ cfg = _resolve_config( config, @@ -3036,11 +3041,21 @@ def parse_report_email( except InvalidDMARCReport as e: error = f'Message with subject "{subject_text}" is not a valid DMARC report: {e}' - raise ParserError(error) from e + raise InvalidDMARCReport(error) from e + + except InvalidSMTPTLSReport: + # Named with the message's subject by the handler below. + raise except Exception as e: error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}' raise ParserError(error) from e + except InvalidSMTPTLSReport as e: + # From either the tlsrpt content-type branches or a JSON attachment. + error = ( + f'Message with subject "{subject_text}" is not a valid SMTP TLS report: {e}' + ) + raise InvalidSMTPTLSReport(error) from e except ParserError: raise except Exception as e: @@ -3628,7 +3643,7 @@ def get_dmarc_reports_from_mbox( total=total_messages, disable=None, ): - if isinstance(result, InvalidDMARCReport): + if isinstance(result, (InvalidDMARCReport, InvalidSMTPTLSReport)): logger.warning(str(result)) elif isinstance(result, ParserError): raise result @@ -3654,7 +3669,7 @@ def get_dmarc_reports_from_mbox( smtp_tls_reports, seen_aggregate_report_ids=cfg.seen_aggregate_report_ids, ) - except InvalidDMARCReport as error: + except (InvalidDMARCReport, InvalidSMTPTLSReport) as error: logger.warning(error.__str__()) except mailbox.NoSuchMailboxError: raise InvalidDMARCReport(f"Mailbox {input_} does not exist") diff --git a/tests/test_init.py b/tests/test_init.py index 2dfb4af8..a83c6b98 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -4124,6 +4124,133 @@ class TestGetDmarcReportsFromMbox(unittest.TestCase): finally: os.remove(path) + def _invalid_then_valid_messages(self): + """Two report emails: the first carries an aggregate report whose + date range spans 4,000 years (invalid), the second a valid one.""" + import re + + with open("samples/aggregate/!example.com!1538204542!1538463818.xml") as f: + xml = f.read() + long_xml = re.sub(r"\d+", "0", xml) + long_xml = re.sub(r"\d+", "126230400000", long_xml) + valid_xml = re.sub( + r"[^<]*", "second-id", xml + ) + self.assertNotEqual(long_xml, xml) + self.assertNotEqual(valid_xml, xml) + messages = [] + for subject, report_xml in (("bad", long_xml), ("good", valid_xml)): + msg = MIMEMultipart() + msg["From"] = "dmarc@example.com" + msg["To"] = "postmaster@example.com" + msg["Subject"] = subject + msg["Date"] = "Thu, 1 Jan 2024 00:00:00 +0000" + att = MIMEApplication(gzip.compress(report_xml.encode()), "gzip") + att.add_header("Content-Disposition", "attachment", filename="r.xml.gz") + msg.attach(att) + messages.append(msg) + return messages + + def testInvalidAttachmentRaisesInvalidDMARCReport(self): + """An invalid report attachment is an ``InvalidDMARCReport``, the + class the mbox loop skips, not a bare ``ParserError``.""" + bad, _ = self._invalid_then_valid_messages() + with self.assertRaises(parsedmarc.InvalidDMARCReport) as ctx: + parsedmarc.parse_report_email(bad.as_bytes(), offline=True) + self.assertIn("spans more than 366 days", str(ctx.exception)) + + def testInvalidAttachmentDoesNotStopMbox(self): + """One message with an invalid report attachment is skipped with a + warning and the messages after it are still parsed, sequentially + and with n_procs=2. + + Previously parse_report_email() raised a bare ``ParserError`` for it, + which get_dmarc_reports_from_mbox() re-raises, aborting the file.""" + tmp = mkdtemp() + self.addCleanup(rmtree, tmp) + path = os.path.join(tmp, "reports.mbox") + box = mailbox.mbox(path) + box.lock() + try: + for msg in self._invalid_then_valid_messages(): + box.add(msg) + box.flush() + finally: + box.unlock() + box.close() + for n_procs in (1, 2): + with self.subTest(n_procs=n_procs): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + self.addCleanup(parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear) + with self.assertLogs("parsedmarc.log", level="WARNING") as logs: + result = parsedmarc.get_dmarc_reports_from_mbox( + path, offline=True, n_procs=n_procs + ) + reports = result["aggregate_reports"] + self.assertEqual(len(reports), 1) + self.assertEqual( + reports[0]["report_metadata"]["report_id"], "second-id" + ) + warning = "\n".join(logs.output) + self.assertIn('subject "bad"', warning) + self.assertIn("spans more than 366 days", warning) + + def testInvalidSmtpTlsAttachmentDoesNotStopMbox(self): + """A message whose SMTP TLS JSON attachment is invalid raises + ``InvalidSMTPTLSReport`` naming its subject, and the mbox loop skips + it like an invalid DMARC report, sequentially and with n_procs=2.""" + _, good = self._invalid_then_valid_messages() + bad = MIMEMultipart() + bad["From"] = "tlsrpt@example.com" + bad["To"] = "postmaster@example.com" + bad["Subject"] = "bad tls" + bad["Date"] = "Thu, 1 Jan 2024 00:00:00 +0000" + att = MIMEApplication( + gzip.compress(b'{"organization-name": "x"}'), "tlsrpt+gzip" + ) + att.add_header("Content-Disposition", "attachment", filename="r.json.gz") + bad.attach(att) + with self.assertRaises(parsedmarc.InvalidSMTPTLSReport) as ctx: + parsedmarc.parse_report_email(bad.as_bytes(), offline=True) + self.assertIn('subject "bad tls"', str(ctx.exception)) + # The same invalid JSON as a generic attachment takes the other + # parsing branch and must end the same way. + generic = MIMEMultipart() + generic["Subject"] = "bad tls generic" + part = MIMEApplication(b'{"organization-name": "x"}', "octet-stream") + part.add_header("Content-Disposition", "attachment", filename="r.json") + generic.attach(part) + with self.assertRaises(parsedmarc.InvalidSMTPTLSReport) as ctx: + parsedmarc.parse_report_email(generic.as_bytes(), offline=True) + self.assertIn('subject "bad tls generic"', str(ctx.exception)) + + tmp = mkdtemp() + self.addCleanup(rmtree, tmp) + path = os.path.join(tmp, "reports.mbox") + box = mailbox.mbox(path) + box.lock() + try: + box.add(bad) + box.add(good) + box.flush() + finally: + box.unlock() + box.close() + for n_procs in (1, 2): + with self.subTest(n_procs=n_procs): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + self.addCleanup(parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear) + with self.assertLogs("parsedmarc.log", level="WARNING") as logs: + result = parsedmarc.get_dmarc_reports_from_mbox( + path, offline=True, n_procs=n_procs + ) + reports = result["aggregate_reports"] + self.assertEqual(len(reports), 1) + self.assertEqual( + reports[0]["report_metadata"]["report_id"], "second-id" + ) + self.assertIn('subject "bad tls"', "\n".join(logs.output)) + def testPoisonMessageRaisesParserError(self): """A message that cannot be re-serialized reaches the parser and surfaces as a ``ParserError``, sequentially and with n_procs=2. @@ -4135,7 +4262,8 @@ class TestGetDmarcReportsFromMbox(unittest.TestCase): the message reached the parser. get_bytes() returns the stored message as-is, and parse_report_email() turns the content error into a ``ParserError``. The mbox loop's policy is unchanged: it skips only - ``InvalidDMARCReport`` and re-raises any other ``ParserError``.""" + an invalid report (``InvalidDMARCReport`` or + ``InvalidSMTPTLSReport``) and re-raises any other ``ParserError``.""" # The error text, not __cause__, identifies the content error: an # exception returned from a worker process loses its __cause__. exercised = 0 @@ -4195,7 +4323,7 @@ class TestGetDmarcReportsFromMboxParallel(unittest.TestCase): Confirms the parallel branch classifies and dedups identically to the sequential branch, and that the junk message produces a warning log instead of raising -- both branches must only catch InvalidDMARCReport - (a ParserError subclass) around a single message and continue, since a + and InvalidSMTPTLSReport (ParserError subclasses) around a single message and continue, since a bare ParserError is deliberately re-raised (see the n_procs > 1 branch of get_dmarc_reports_from_mbox, which mirrors the sequential branch's `except InvalidDMARCReport` scope).