From e077136b60932ca03e017c42eedaed64a1143218 Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:42:34 -0400 Subject: [PATCH] Fix blank SMTP TLS failure-detail fields crashing saves; read RFC 8460 additional-information (11.0.3) (#916) * Treat blank SMTP TLS failure-detail fields as absent; read RFC 8460 additional-information Some reporters send optional failure-details fields as empty strings (e.g. "sending-mta-ip": "" on sts-policy-fetch-error). The parser copied them verbatim, so the PostgreSQL save failed on the INET columns ("invalid input syntax for type inet") and the Elasticsearch/OpenSearch saves failed Ip() validation, discarding the whole report. Blank or whitespace-only optional values are now omitted. RFC 8460 section 4.4 names the URI key "additional-information"; the parser only read "additional-info-uri" (the schema's value placeholder), so conformant reports lost the URI. The RFC key is now read, with the old key kept as a fallback. Bump version to 11.0.3. Closes #915 Co-Authored-By: Claude Opus 5.5 (1M context) * Import parse_report_file directly in the ES/OpenSearch tests Addresses github-code-quality's "Module is imported with 'import' and 'import from'" findings on tests/test_elastic.py and tests/test_opensearch.py. Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 ++ parsedmarc/__init__.py | 62 ++++++++---- parsedmarc/constants.py | 2 +- .../smtp_tls/empty_failure_detail_fields.json | 38 ++++++++ tests/test_elastic.py | 43 ++++++++- tests/test_init.py | 95 +++++++++++++++++++ tests/test_opensearch.py | 43 ++++++++- tests/test_postgres.py | 40 ++++++++ 8 files changed, 307 insertions(+), 23 deletions(-) create mode 100644 samples/smtp_tls/empty_failure_detail_fields.json diff --git a/CHANGELOG.md b/CHANGELOG.md index fffd4e31..034b8687 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## 11.0.3 + +### Bug fixes + +- **An SMTP TLS (RFC 8460) failure report whose `failure-details` entry sends an empty string for an optional field — e.g. `"sending-mta-ip": ""` and `"receiving-ip": ""`, which real reporters send for results like `sts-policy-fetch-error` — no longer crashes the PostgreSQL save.** `_parse_smtp_tls_failure_details()` copied every optional field verbatim whenever the JSON key was present, so an empty string reached the PostgreSQL `sending_mta_ip`/`receiving_ip` `INET` columns and failed with `invalid input syntax for type inet: ""`, discarding the whole report. The same blank values would also have crashed the Elasticsearch and OpenSearch saves: those outputs' `sending_mta_ip`/`receiving_ip` fields are IP-typed (`Ip()` in `elastic.py`/`opensearch.py`), and `Document.save()`'s validation pass rejects an empty string for an `Ip()` field with a bare `ValueError` — `'' does not appear to be an IPv4 or IPv6 address` — raised before any network call, so the whole report was lost rather than indexed with an empty IP. A `None` or blank/whitespace-only value for any optional failure-detail field (`sending-mta-ip`, `receiving-ip`, `receiving-mx-hostname`, `receiving-mx-helo`, the additional-information URI, `failure-reason-code`) is now treated as absent, so it is stored as `NULL` rather than `''`. (Closes #915) +- **The RFC 8460 §4.4 `additional-information` failure-details key is now read.** The JSON Report Schema names the key `additional-information` (its value is described by the placeholder `additional-info-uri`), but the parser only read the non-RFC key `additional-info-uri`, so a conformant reporter's URI was silently dropped — including in the RFC's own worked example, `samples/smtp_tls/rfc8460.json`. The non-RFC `additional-info-uri` key is still read as a fallback for backward compatibility, and is used when the RFC key is absent or blank. + ## 11.0.2 ### Changes diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index ea290320..2e710c32 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -656,6 +656,26 @@ def _parse_report_record( return new_record +def _non_blank(value: Any) -> Any: + """Treat ``None`` or a blank/whitespace-only string as absent. + + Some SMTP TLS (RFC 8460) reporters send an empty string for an optional + failure-details field (e.g. ``sending-mta-ip``/``receiving-ip`` on a + ``sts-policy-fetch-error``) instead of omitting the key. An empty string + is not a valid IP address, so passing it through crashed the PostgreSQL + save (``INET`` columns) and would also have crashed the Elasticsearch + and OpenSearch saves: their ``Ip()`` fields' validation + (``elasticsearch.dsl``/``opensearchpy``'s ``Document.save()``) rejects + an empty string with a bare ``ValueError`` (issue #915). Non-string, + non-blank values pass through unchanged. + """ + if value is None: + return None + if isinstance(value, str) and value.strip() == "": + return None + return value + + def _parse_smtp_tls_failure_details(failure_details: dict[str, Any]): try: new_failure_details: dict[str, Any] = { @@ -663,26 +683,28 @@ def _parse_smtp_tls_failure_details(failure_details: dict[str, Any]): "failed_session_count": failure_details["failed-session-count"], } - if "sending-mta-ip" in failure_details: - new_failure_details["sending_mta_ip"] = failure_details["sending-mta-ip"] - if "receiving-ip" in failure_details: - new_failure_details["receiving_ip"] = failure_details["receiving-ip"] - if "receiving-mx-hostname" in failure_details: - new_failure_details["receiving_mx_hostname"] = failure_details[ - "receiving-mx-hostname" - ] - if "receiving-mx-helo" in failure_details: - new_failure_details["receiving_mx_helo"] = failure_details[ - "receiving-mx-helo" - ] - if "additional-info-uri" in failure_details: - new_failure_details["additional_info_uri"] = failure_details[ - "additional-info-uri" - ] - if "failure-reason-code" in failure_details: - new_failure_details["failure_reason_code"] = failure_details[ - "failure-reason-code" - ] + optional_fields = [ + ("sending-mta-ip", "sending_mta_ip"), + ("receiving-ip", "receiving_ip"), + ("receiving-mx-hostname", "receiving_mx_hostname"), + ("receiving-mx-helo", "receiving_mx_helo"), + ("failure-reason-code", "failure_reason_code"), + ] + for src_key, dest_key in optional_fields: + value = _non_blank(failure_details.get(src_key)) + if value is not None: + new_failure_details[dest_key] = value + + # RFC 8460 §4.4 names the JSON key "additional-information" (its + # value is described as an "additional-info-uri"). Some reporters + # instead send the non-RFC key "additional-info-uri" directly; fall + # back to it for backward compatibility when the RFC key is absent + # or blank. + additional_info_uri = _non_blank(failure_details.get("additional-information")) + if additional_info_uri is None: + additional_info_uri = _non_blank(failure_details.get("additional-info-uri")) + if additional_info_uri is not None: + new_failure_details["additional_info_uri"] = additional_info_uri return new_failure_details diff --git a/parsedmarc/constants.py b/parsedmarc/constants.py index f7e4a3d0..581d74b2 100644 --- a/parsedmarc/constants.py +++ b/parsedmarc/constants.py @@ -1,4 +1,4 @@ -__version__ = "11.0.2" +__version__ = "11.0.3" USER_AGENT = f"parsedmarc/{__version__}" diff --git a/samples/smtp_tls/empty_failure_detail_fields.json b/samples/smtp_tls/empty_failure_detail_fields.json new file mode 100644 index 00000000..da856cad --- /dev/null +++ b/samples/smtp_tls/empty_failure_detail_fields.json @@ -0,0 +1,38 @@ +{ + "organization-name":"Reporter Example", + "date-range":{ + "start-datetime":"2024-06-01T00:00:00Z", + "end-datetime":"2024-06-01T23:59:59Z" + }, + "contact-info":"smtp-tls-reporting@reporter.example", + "report-id":"2024-06-01T00:00:00Z_example.com", + "policies":[ + { + "policy":{ + "policy-type":"sts", + "policy-string":[ + "version: STSv1", + "mode: enforce", + "mx: example.com", + "max_age: 86400" + ], + "policy-domain":"example.com" + }, + "summary":{ + "total-successful-session-count":0, + "total-failure-session-count":1 + }, + "failure-details":[ + { + "result-type":"sts-policy-fetch-error", + "sending-mta-ip":"", + "receiving-ip":"", + "receiving-mx-hostname":"", + "failed-session-count":1, + "additional-information":"", + "failure-reason-code":"no-policy-served" + } + ] + } + ] +} diff --git a/tests/test_elastic.py b/tests/test_elastic.py index a6da2a5f..7dfb7784 100644 --- a/tests/test_elastic.py +++ b/tests/test_elastic.py @@ -8,10 +8,11 @@ queries, error wrapping — without needing a running Elasticsearch cluster. import time import unittest +from typing import cast from unittest.mock import MagicMock, call, patch import parsedmarc.elastic as elastic_module -from parsedmarc import InvalidFailureReport +from parsedmarc import InvalidFailureReport, parse_report_file from parsedmarc.elastic import ( AlreadySaved, ElasticsearchError, @@ -1486,6 +1487,46 @@ class TestSaveSmtpTlsReport(unittest.TestCase): "https://reports.example.com/tls-help", ) + def test_save_blank_failure_detail_fields_omitted_from_ip_fields(self): + """Blank optional failure-detail fields must not reach the Ip() + fields (issue #915). + + samples/smtp_tls/empty_failure_detail_fields.json reproduces a + real reporter's shape: sending-mta-ip/receiving-ip sent as "" on + an sts-policy-fetch-error. Verified directly against + elasticsearch.dsl: Document.save() validates by calling + full_clean() before any network call, and an Ip() field's + validation calls Python's ipaddress.ip_address(""), which raises a + bare ValueError ("'' does not appear to be an IPv4 or IPv6 + address") rather than a caught ValidationException -- so an empty + string here previously discarded the whole report instead of + indexing it with a blank IP. + + save() is mocked (per this module's convention, so no real + cluster is needed), which also means it never runs full_clean() + itself; call it explicitly on the captured document so this test + exercises the same validation a real save() would, and fails with + that ValueError on unfixed parsedmarc/__init__.py. + """ + result = parse_report_file( + "samples/smtp_tls/empty_failure_detail_fields.json", offline=True + ) + report = cast(dict, result["report"]) + with ( + patch("parsedmarc.elastic.Search", return_value=_empty_search()), + patch("parsedmarc.elastic.Index"), + patch.object( + elastic_module._SMTPTLSReportDoc, "save", autospec=True + ) as mock_save, + ): + save_smtp_tls_report_to_elasticsearch(report) + doc = mock_save.call_args[0][0] + doc.full_clean() + failure_detail = doc.policies[0].failure_details[0] + self.assertIsNone(failure_detail.sending_mta_ip) + self.assertIsNone(failure_detail.receiving_ip) + self.assertEqual(failure_detail.failure_reason_code, "no-policy-served") + class TestBackwardCompatAlias(unittest.TestCase): def test_save_forensic_alias_points_to_save_failure(self): diff --git a/tests/test_init.py b/tests/test_init.py index f45284df..c63bb161 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -963,6 +963,26 @@ class Test(unittest.TestCase): ) print("Passed!") + def testSmtpTlsRfc8460SampleReadsAdditionalInformation(self): + """samples/smtp_tls/rfc8460.json is RFC 8460 §4.4's own worked + example, whose second failure-details entry sets + "additional-information" (not the non-RFC "additional-info-uri"). + Before the fix, the parser only read "additional-info-uri", so this + URI was silently dropped even from the RFC's own example.""" + result = parsedmarc.parse_report_file( + "samples/smtp_tls/rfc8460.json", offline=True + ) + report = cast(SMTPTLSReport, result["report"]) + failure_details = report["policies"][0].get("failure_details", []) + starttls_detail = next( + d for d in failure_details if d["result_type"] == "starttls-not-supported" + ) + self.assertEqual( + starttls_detail.get("additional_info_uri"), + "https://reports.company-x.example/report_info?" + "id=5065427c-23d3#StarttlsNotSupported", + ) + def testSmtpTlsCsvStripsNulFromFields(self): """A NUL character in an SMTP TLS report text field is stripped from CSV output instead of reaching the ``csv`` writer. @@ -1487,6 +1507,81 @@ class Test(unittest.TestCase): self.assertEqual(result["additional_info_uri"], "https://example.com/info") self.assertEqual(result["failure_reason_code"], "TLS_ERROR") + def testParseSmtpTlsFailureDetailsBlankOptionalFieldsOmitted(self): + """Empty-string and whitespace-only optional fields are treated as + absent, not passed through. + + Real-world reporters send "" for sending-mta-ip/receiving-ip on + results like sts-policy-fetch-error (see samples/smtp_tls/ + empty_failure_detail_fields.json). An empty string is not a valid + IP address, so it previously reached PostgreSQL's INET columns + verbatim and crashed the PostgreSQL save, and would also have + crashed the Elasticsearch/OpenSearch saves' Ip() field validation + with a bare ValueError (issue #915). + """ + details = { + "result-type": "sts-policy-fetch-error", + "failed-session-count": 1, + "sending-mta-ip": "", + "receiving-ip": " ", + "receiving-mx-hostname": "", + "receiving-mx-helo": "\t", + "additional-information": "", + "additional-info-uri": "", + "failure-reason-code": "", + } + result = parsedmarc._parse_smtp_tls_failure_details(details) + self.assertEqual(result["result_type"], "sts-policy-fetch-error") + self.assertEqual(result["failed_session_count"], 1) + self.assertNotIn("sending_mta_ip", result) + self.assertNotIn("receiving_ip", result) + self.assertNotIn("receiving_mx_hostname", result) + self.assertNotIn("receiving_mx_helo", result) + self.assertNotIn("additional_info_uri", result) + self.assertNotIn("failure_reason_code", result) + + def testParseSmtpTlsFailureDetailsAdditionalInformationKey(self): + """RFC 8460 §4.4's JSON key is "additional-information" (its value + is described as an "additional-info-uri"); it is read into the + parser's additional_info_uri field. + """ + details = { + "result-type": "starttls-not-supported", + "failed-session-count": 1, + "additional-information": "https://example.com/rfc-key-info", + } + result = parsedmarc._parse_smtp_tls_failure_details(details) + self.assertEqual( + result["additional_info_uri"], "https://example.com/rfc-key-info" + ) + + def testParseSmtpTlsFailureDetailsAdditionalInformationWinsOverLegacy(self): + """When both the RFC key and the legacy key are present and + non-blank, the RFC key's value wins.""" + details = { + "result-type": "starttls-not-supported", + "failed-session-count": 1, + "additional-information": "https://example.com/rfc-key-info", + "additional-info-uri": "https://example.com/legacy-key-info", + } + result = parsedmarc._parse_smtp_tls_failure_details(details) + self.assertEqual( + result["additional_info_uri"], "https://example.com/rfc-key-info" + ) + + def testParseSmtpTlsFailureDetailsAdditionalInformationBlankFallsBack(self): + """A blank RFC-key value falls back to a non-blank legacy value.""" + details = { + "result-type": "starttls-not-supported", + "failed-session-count": 1, + "additional-information": "", + "additional-info-uri": "https://example.com/legacy-key-info", + } + result = parsedmarc._parse_smtp_tls_failure_details(details) + self.assertEqual( + result["additional_info_uri"], "https://example.com/legacy-key-info" + ) + def testParseSmtpTlsFailureDetailsMissingRequired(self): """Missing required field raises InvalidSMTPTLSReport""" with self.assertRaises(parsedmarc.InvalidSMTPTLSReport): diff --git a/tests/test_opensearch.py b/tests/test_opensearch.py index 41218f4b..c310b3bb 100644 --- a/tests/test_opensearch.py +++ b/tests/test_opensearch.py @@ -8,10 +8,11 @@ queries, error wrapping — without needing a running OpenSearch cluster. import time import unittest +from typing import cast from unittest.mock import MagicMock, call, patch import parsedmarc.opensearch as opensearch_module -from parsedmarc import InvalidFailureReport +from parsedmarc import InvalidFailureReport, parse_report_file from parsedmarc.opensearch import ( AlreadySaved, OpenSearchError, @@ -1477,6 +1478,46 @@ class TestSaveSmtpTlsReport(unittest.TestCase): "https://reports.example.com/tls-help", ) + def test_save_blank_failure_detail_fields_omitted_from_ip_fields(self): + """Blank optional failure-detail fields must not reach the Ip() + fields (issue #915). + + samples/smtp_tls/empty_failure_detail_fields.json reproduces a + real reporter's shape: sending-mta-ip/receiving-ip sent as "" on + an sts-policy-fetch-error. Verified directly against opensearchpy: + Document.save() validates by calling full_clean() before any + network call, and an Ip() field's + validation calls Python's ipaddress.ip_address(""), which raises + a bare ValueError ("'' does not appear to be an IPv4 or IPv6 + address") rather than a caught ValidationException -- so an empty + string here previously discarded the whole report instead of + indexing it with a blank IP. + + save() is mocked (per this module's convention, so no real + cluster is needed), which also means it never runs full_clean() + itself; call it explicitly on the captured document so this test + exercises the same validation a real save() would, and fails with + that ValueError on unfixed parsedmarc/__init__.py. + """ + result = parse_report_file( + "samples/smtp_tls/empty_failure_detail_fields.json", offline=True + ) + report = cast(dict, result["report"]) + with ( + patch("parsedmarc.opensearch.Search", return_value=_empty_search()), + patch("parsedmarc.opensearch.Index"), + patch.object( + opensearch_module._SMTPTLSReportDoc, "save", autospec=True + ) as mock_save, + ): + save_smtp_tls_report_to_opensearch(report) + doc = mock_save.call_args[0][0] + doc.full_clean() + failure_detail = doc.policies[0].failure_details[0] + self.assertIsNone(failure_detail.sending_mta_ip) + self.assertIsNone(failure_detail.receiving_ip) + self.assertEqual(failure_detail.failure_reason_code, "no-policy-served") + class TestBackwardCompatAlias(unittest.TestCase): def test_save_forensic_alias_points_to_save_failure(self): diff --git a/tests/test_postgres.py b/tests/test_postgres.py index 8dd59be9..a6b6a205 100644 --- a/tests/test_postgres.py +++ b/tests/test_postgres.py @@ -658,6 +658,46 @@ class TestPostgreSQLClientSave(unittest.TestCase): self.assertIn(100, policy_params) self.assertIn(2, policy_params) + def test_save_smtp_tls_report_blank_failure_detail_fields_become_null(self): + """Blank optional failure-detail fields reach PostgreSQL as None, + not "" (issue #915). + + samples/smtp_tls/empty_failure_detail_fields.json reproduces a + real reporter's shape: sending-mta-ip/receiving-ip/ + receiving-mx-hostname/additional-information sent as "" on an + sts-policy-fetch-error. "" is not a valid PostgreSQL INET value, so + before the parser fix in parsedmarc/__init__.py this reached + cur.execute() as an empty string and crashed the save with + "invalid input syntax for type inet: \"\"" against a real server. + This test goes through the real parser (parsedmarc.parse_report_file) + rather than a hand-built dict, so it fails on unfixed + parsedmarc/__init__.py even though this test file only mocks + psycopg. + """ + client, mock_conn = _make_client() + cur = _mock_cursor(mock_conn, [(1,), (10,)]) + + result = parsedmarc.parse_report_file( + "samples/smtp_tls/empty_failure_detail_fields.json", offline=True + ) + report = result["report"] + + client.save_smtp_tls_report_to_postgresql(cast(dict, report)) + + sqls = _executed_sql(cur) + self.assertIn("smtp_tls_failure_detail", sqls[2]) + + detail_params = _named_params(cur.execute.call_args_list[2]) + self.assertIsNone(detail_params["sending_mta_ip"]) + self.assertIsNone(detail_params["receiving_ip"]) + self.assertIsNone(detail_params["receiving_mx_hostname"]) + self.assertIsNone(detail_params["additional_info_uri"]) + # A non-blank optional field is preserved. + self.assertEqual(detail_params["failure_reason_code"], "no-policy-served") + # Required fields are preserved. + self.assertEqual(detail_params["result_type"], "sts-policy-fetch-error") + self.assertEqual(detail_params["failed_session_count"], 1) + def test_save_smtp_tls_report_already_saved(self): """AlreadySaved is raised when ON CONFLICT returns no row.""" client, mock_conn = _make_client()