From 2a5c6295a61bb1ba06d822a31e4f4a1487fa5430 Mon Sep 17 00:00:00 2001 From: Sean Whalen Date: Wed, 20 May 2026 19:48:56 -0400 Subject: [PATCH 1/5] Remove coverage-chasing tests; fix BIMI x/y attribute check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop tests that asserted on contrived inputs solely to keep coverage high, and fix the underlying bugs they were papering over. - checkdmarc/bimi.py: SVG x/y forbidden-attribute capture was reading the wrong xmltodict keys ("x"/"y" matches child elements, not attributes), so the check_svg_requirements rejection never fired on real SVGs. Switch to "@x"/"@y" and fix a typo that wrote the y value into metadata["x"]. - checkdmarc/dnssec.py: Narrow three broad "except Exception" clauses to (dns.exception.DNSException, OSError, EOFError) per AGENTS.md. - tests/test_bimi.py: Delete TestSvgMetadataExtraAttributes; it built malformed SVGs (with child /// elements) to exercise unreachable branches and even asserted on a known source bug. Replace with one honest end-to-end test that proves the fixed forbidden-attribute path works. - tests/test_init.py: Delete testKnownGoodMocked — it patched all nine check_* helpers to return canned valid dicts then asserted the canned values came back. The network counterpart testKnownGood is the real test. - tests/test_dnssec.py: Delete testValidationExceptionContinues — it existed only to exercise the broad except we've now narrowed away. Co-Authored-By: Claude Opus 4.7 (1M context) --- checkdmarc/bimi.py | 8 +++--- checkdmarc/dnssec.py | 7 ++--- tests/test_bimi.py | 62 +++++++------------------------------------- tests/test_dnssec.py | 25 ------------------ tests/test_init.py | 49 ---------------------------------- 5 files changed, 18 insertions(+), 133 deletions(-) diff --git a/checkdmarc/bimi.py b/checkdmarc/bimi.py index ebff2c9..adb17d6 100644 --- a/checkdmarc/bimi.py +++ b/checkdmarc/bimi.py @@ -503,10 +503,10 @@ def get_svg_metadata(raw_xml: Union[str, bytes]) -> dict[str, Any]: view_box = view_box.split(" ") width = float(view_box[-2]) height = float(view_box[-1]) - if "x" in svg.keys(): - metadata["x"] = svg["x"] - if "y" in svg.keys(): - metadata["x"] = svg["y"] + if "@x" in svg.keys(): + metadata["x"] = svg["@x"] + if "@y" in svg.keys(): + metadata["y"] = svg["@y"] if "title" in svg.keys(): metadata["title"] = svg["title"] description = None diff --git a/checkdmarc/dnssec.py b/checkdmarc/dnssec.py index 33895af..48abd9f 100644 --- a/checkdmarc/dnssec.py +++ b/checkdmarc/dnssec.py @@ -8,6 +8,7 @@ from collections.abc import Sequence import dns.dnssec +import dns.exception import dns.message import dns.query import dns.resolver @@ -105,7 +106,7 @@ def get_dnskey( key = {name: rrset} cache[domain] = key return key - except Exception as e: + except (dns.exception.DNSException, OSError, EOFError) as e: cache[domain] = None logging.debug(f"DNSKEY query error: {e}") @@ -169,7 +170,7 @@ def test_dnssec( logging.debug(f"Found a signed {rdatatype.name} record") cache[domain] = True return True - except Exception as e: + except (dns.exception.DNSException, OSError, EOFError) as e: logging.debug(f"DNSSEC query error: {e}") cache[domain] = False @@ -246,7 +247,7 @@ def get_tlsa_records( tlsa_records = list(map(lambda x: str(x), list(rrset.items.keys()))) cache[query_hostname] = tlsa_records return tlsa_records - except Exception as e: + except (dns.exception.DNSException, OSError, EOFError) as e: logging.debug(f"TLSA query error: {e}") return tlsa_records return tlsa_records diff --git a/tests/test_bimi.py b/tests/test_bimi.py index 43461de..3f34f9c 100644 --- a/tests/test_bimi.py +++ b/tests/test_bimi.py @@ -463,66 +463,24 @@ def testRecordNotFoundError(self): self.assertIn("error", cast(Any, result)) -class TestSvgMetadataExtraAttributes(unittest.TestCase): - """Cover the optional-attribute branches of get_svg_metadata. +class TestSvgMetadataForbiddenAttributes(unittest.TestCase): + """SVG x/y attributes on the root are forbidden by BIMI; they + should be captured in metadata so check_svg_requirements can flag them.""" - Note: the source checks ``"x" in svg.keys()`` (etc.) — xmltodict puts - attributes under ``@x``, so these branches only fire when the SVG has - *child elements* literally named x / y / overflow. That's contrived - but the tests below construct such SVGs to exercise the branches. - """ - - def testXChildElementPopulatesMetadata(self): - svg = ( - '' - '' - "10" - "Brand" - "" - ) - metadata = checkdmarc.bimi.get_svg_metadata(svg) - self.assertEqual(metadata["x"], "10") - - def testYChildElementPopulatesMetadata(self): - """bimi.py:509 writes the y value to metadata["x"] (source bug); - we exercise the branch only.""" - svg = ( - '' - '' - "5" - "Brand" - "" - ) - metadata = checkdmarc.bimi.get_svg_metadata(svg) - # The branch wrote to metadata["x"] (not metadata["y"]) per the - # source bug — the goal here is line coverage, not correctness. - self.assertEqual(metadata["x"], "5") - - def testDescriptionChildElement(self): + def testRootXYAttributesCaptured(self): svg = ( '' '' - "Brand" - "A brand logo" - "" - ) - metadata = checkdmarc.bimi.get_svg_metadata(svg) - self.assertEqual(metadata["description"], "A brand logo") - - def testOverflowChildElement(self): - svg = ( - '' - '' - "hidden" + 'baseProfile="tiny-ps" viewBox="0 0 64 64" x="0" y="0">' "Brand" "" ) metadata = checkdmarc.bimi.get_svg_metadata(svg) - self.assertEqual(metadata["overflow"], "hidden") + self.assertEqual(metadata["x"], "0") + self.assertEqual(metadata["y"], "0") + errors = checkdmarc.bimi.check_svg_requirements(metadata) + self.assertTrue(any("cannot include x" in e for e in errors)) + self.assertTrue(any("cannot include y" in e for e in errors)) class TestExtractLogoFromPemBytes(unittest.TestCase): diff --git a/tests/test_dnssec.py b/tests/test_dnssec.py index 4461aa5..e21b4e6 100644 --- a/tests/test_dnssec.py +++ b/tests/test_dnssec.py @@ -173,31 +173,6 @@ def testNoSignedRecordsReturnsFalse(self): ) self.assertFalse(result) - def testValidationExceptionContinues(self): - """A failure on dns.dnssec.validate is swallowed and we fall through to False""" - import dns.rdatatype - - rrset = MagicMock() - rrset.rdtype = dns.rdatatype.A - rrsig = MagicMock() - rrsig.rdtype = dns.rdatatype.RRSIG - response = MagicMock() - response.answer = [rrset, rrsig] - - with patch("checkdmarc.dnssec.get_dnskey", return_value=MagicMock()): - with patch("dns.query.tcp", return_value=response): - with patch( - "dns.dnssec.validate", - side_effect=Exception("invalid signature"), - ): - result = checkdmarc.dnssec.test_dnssec( - "example.com", - nameservers=["1.1.1.1"], - cache=self._fresh_cache(), - ) - self.assertFalse(result) - - class TestGetTlsaRecords(unittest.TestCase): @staticmethod def _fresh_cache(): diff --git a/tests/test_init.py b/tests/test_init.py index 69194bc..c85818c 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -16,9 +16,6 @@ network_test = unittest.skipIf( OFFLINE_MODE, "Real-network test skipped on GitHub Actions" ) -mocked_only = unittest.skipUnless( - OFFLINE_MODE, "Mocked counterpart skipped locally; network test covers this" -) class Test(unittest.TestCase): @@ -53,52 +50,6 @@ def testKnownGood(self): ), ) - @mocked_only - def testKnownGoodMocked(self): - """check_domains orchestrates per-check helpers and returns valid=True (mocked) - - The component check_* helpers each have their own focused tests; this - covers the orchestration in check_domains for the multi-domain code path. - """ - from contextlib import ExitStack - - check_returns = { - "checkdmarc.test_dnssec": False, - "checkdmarc.check_soa": {"valid": True, "values": {}}, - "checkdmarc.check_ns": { - "hostnames": ["ns1.example.com"], - "warnings": [], - }, - "checkdmarc.check_mta_sts": {"valid": False, "error": "not found"}, - "checkdmarc.check_mx": {"hosts": [], "warnings": []}, - "checkdmarc.check_spf": { - "record": "v=spf1 -all", - "valid": True, - "warnings": [], - }, - "checkdmarc.check_dmarc": { - "record": "v=DMARC1; p=reject", - "valid": True, - "warnings": [], - "tags": {}, - }, - "checkdmarc.check_smtp_tls_reporting": { - "valid": False, - "error": "not found", - }, - "checkdmarc.check_bimi": {"valid": True, "warnings": []}, - } - with ExitStack() as stack: - for target, return_value in check_returns.items(): - stack.enter_context(patch(target, return_value=return_value)) - results = checkdmarc.check_domains(known_good_domains) - - if not isinstance(results, list): - results = [results] - for result in results: - self.assertTrue(cast(Any, result["spf"])["valid"]) - self.assertTrue(result["dmarc"]["valid"]) - def testResultsToJson(self): """results_to_json produces valid JSON""" results = cast( From d5f1c673085f267d585e0b427bfccfc0a5aa3ed3 Mon Sep 17 00:00:00 2001 From: Sean Whalen Date: Wed, 20 May 2026 19:50:59 -0400 Subject: [PATCH 2/5] Apply ruff format to tests/test_dnssec.py Trailing blank line at end of file. The previous commit ran ruff check but missed ruff format. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_dnssec.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_dnssec.py b/tests/test_dnssec.py index e21b4e6..d35edb2 100644 --- a/tests/test_dnssec.py +++ b/tests/test_dnssec.py @@ -173,6 +173,7 @@ def testNoSignedRecordsReturnsFalse(self): ) self.assertFalse(result) + class TestGetTlsaRecords(unittest.TestCase): @staticmethod def _fresh_cache(): From efb4e0a6c7dbcaa0b7e5a88842613dc2c1cab58b Mon Sep 17 00:00:00 2001 From: Sean Whalen Date: Wed, 20 May 2026 19:52:53 -0400 Subject: [PATCH 3/5] Bump version to 5.16.2 and update changelog Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 13 +++++++++++++ checkdmarc/_constants.py | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6cb994..3034200 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## 5.16.2 + +### Changes + +- BIMI: forbidden `x`/`y` attributes on the root `` element are now + actually rejected. `get_svg_metadata` was reading the wrong xmltodict + keys, so the existing rejection in `check_svg_requirements` never fired + on real SVGs. The metadata also lost the `y` value to a typo that + clobbered `metadata["x"]`. +- DNSSEC: narrowed three broad `except Exception` clauses to specific + exception types (`dns.exception.DNSException`, `OSError`, `EOFError`) + so programming errors propagate instead of being silently swallowed. + ## 5.16.1 ### Changes diff --git a/checkdmarc/_constants.py b/checkdmarc/_constants.py index 2fc2d62..3134791 100644 --- a/checkdmarc/_constants.py +++ b/checkdmarc/_constants.py @@ -19,7 +19,7 @@ See the License for the specific language governing permissions and limitations under the License.""" -__version__ = "5.16.1" +__version__ = "5.16.2" OS = platform.system() OS_RELEASE = platform.release() From 9584aab3d6cdf8c6ca298f2a21329f6e16991f03 Mon Sep 17 00:00:00 2001 From: Sean Whalen Date: Wed, 20 May 2026 19:59:29 -0400 Subject: [PATCH 4/5] Cover the narrowed DNSSEC except clause with a real ValidationFailure The narrowed exception handler in test_dnssec catches the actual exception types dnspython raises. Exercise it with a real dns.dnssec.ValidationFailure (the exception thrown on a genuine bad signature) and assert on the function's contract: invalid DNSSEC reports as not validated, rather than propagating. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_dnssec.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test_dnssec.py b/tests/test_dnssec.py index d35edb2..1ebbd1f 100644 --- a/tests/test_dnssec.py +++ b/tests/test_dnssec.py @@ -173,6 +173,32 @@ def testNoSignedRecordsReturnsFalse(self): ) self.assertFalse(result) + def testInvalidSignatureReturnsFalse(self): + """A bad signature (ValidationFailure) at every rdatatype reports + DNSSEC as not validated, rather than propagating the exception.""" + import dns.dnssec + import dns.rdatatype + + rrset = MagicMock() + rrset.rdtype = dns.rdatatype.A + rrsig = MagicMock() + rrsig.rdtype = dns.rdatatype.RRSIG + response = MagicMock() + response.answer = [rrset, rrsig] + + with patch("checkdmarc.dnssec.get_dnskey", return_value=MagicMock()): + with patch("dns.query.tcp", return_value=response): + with patch( + "dns.dnssec.validate", + side_effect=dns.dnssec.ValidationFailure("bad signature"), + ): + result = checkdmarc.dnssec.test_dnssec( + "example.com", + nameservers=["1.1.1.1"], + cache=self._fresh_cache(), + ) + self.assertFalse(result) + class TestGetTlsaRecords(unittest.TestCase): @staticmethod From a8a87480c7ed28686d8455ca57cd47689b320991 Mon Sep 17 00:00:00 2001 From: Sean Whalen Date: Wed, 20 May 2026 20:02:55 -0400 Subject: [PATCH 5/5] Import ValidationFailure from dns.exception dns.dnssec re-imports it from dns.exception but doesn't re-export it, so pyright flags the dns.dnssec.ValidationFailure reference as a reportPrivateImportUsage error. Use the canonical path. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_dnssec.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_dnssec.py b/tests/test_dnssec.py index 1ebbd1f..b276586 100644 --- a/tests/test_dnssec.py +++ b/tests/test_dnssec.py @@ -176,7 +176,7 @@ def testNoSignedRecordsReturnsFalse(self): def testInvalidSignatureReturnsFalse(self): """A bad signature (ValidationFailure) at every rdatatype reports DNSSEC as not validated, rather than propagating the exception.""" - import dns.dnssec + import dns.exception import dns.rdatatype rrset = MagicMock() @@ -190,7 +190,7 @@ def testInvalidSignatureReturnsFalse(self): with patch("dns.query.tcp", return_value=response): with patch( "dns.dnssec.validate", - side_effect=dns.dnssec.ValidationFailure("bad signature"), + side_effect=dns.exception.ValidationFailure("bad signature"), ): result = checkdmarc.dnssec.test_dnssec( "example.com",