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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>

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 `<identifiers>` and inside a `<reason>` 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

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")

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"<begin>\d+</begin>", "<begin>0</begin>", xml)
+        long_xml = re.sub(r"<end>\d+</end>", "<end>126230400000</end>", long_xml)
+        valid_xml = re.sub(
+            r"<report_id>[^<]*</report_id>", "<report_id>second-id</report_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).