Release 5.16.2: BIMI x/y attribute fix; narrower dnssec exceptions (#254)

* Remove coverage-chasing tests; fix BIMI x/y attribute check

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 <x>/<y>/<description>/<overflow> 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) <noreply@anthropic.com>

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

* Bump version to 5.16.2 and update changelog

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

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

---------

Co-authored-by: Sean Whalen <seanthegeek@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: seanthegeek

CHANGELOG.md

@@ -1,5 +1,18 @@
 # Changelog
 
+## 5.16.2
+
+### Changes
+
+- BIMI: forbidden `x`/`y` attributes on the root `<svg>` 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

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

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

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

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 <svg> 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 = (
-            '<?xml version="1.0" encoding="UTF-8"?>'
-            '<svg xmlns="http://www.w3.org/2000/svg" version="1.2" '
-            'baseProfile="tiny-ps" viewBox="0 0 64 64">'
-            "<x>10</x>"
-            "<title>Brand</title>"
-            "</svg>"
-        )
-        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 = (
-            '<?xml version="1.0" encoding="UTF-8"?>'
-            '<svg xmlns="http://www.w3.org/2000/svg" version="1.2" '
-            'baseProfile="tiny-ps" viewBox="0 0 64 64">'
-            "<y>5</y>"
-            "<title>Brand</title>"
-            "</svg>"
-        )
-        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 = (
             '<?xml version="1.0" encoding="UTF-8"?>'
             '<svg xmlns="http://www.w3.org/2000/svg" version="1.2" '
-            'baseProfile="tiny-ps" viewBox="0 0 64 64">'
-            "<title>Brand</title>"
-            "<description>A brand logo</description>"
-            "</svg>"
-        )
-        metadata = checkdmarc.bimi.get_svg_metadata(svg)
-        self.assertEqual(metadata["description"], "A brand logo")
-
-    def testOverflowChildElement(self):
-        svg = (
-            '<?xml version="1.0" encoding="UTF-8"?>'
-            '<svg xmlns="http://www.w3.org/2000/svg" version="1.2" '
-            'baseProfile="tiny-ps" viewBox="0 0 64 64">'
-            "<overflow>hidden</overflow>"
+            'baseProfile="tiny-ps" viewBox="0 0 64 64" x="0" y="0">'
             "<title>Brand</title>"
             "</svg>"
         )
         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):

tests/test_dnssec.py

@@ -173,8 +173,10 @@ def testNoSignedRecordsReturnsFalse(self):
                 )
         self.assertFalse(result)
 
-    def testValidationExceptionContinues(self):
-        """A failure on dns.dnssec.validate is swallowed and we fall through to False"""
+    def testInvalidSignatureReturnsFalse(self):
+        """A bad signature (ValidationFailure) at every rdatatype reports
+        DNSSEC as not validated, rather than propagating the exception."""
+        import dns.exception
         import dns.rdatatype
 
         rrset = MagicMock()
@@ -188,7 +190,7 @@ def testValidationExceptionContinues(self):
             with patch("dns.query.tcp", return_value=response):
                 with patch(
                     "dns.dnssec.validate",
-                    side_effect=Exception("invalid signature"),
+                    side_effect=dns.exception.ValidationFailure("bad signature"),
                 ):
                     result = checkdmarc.dnssec.test_dnssec(
                         "example.com",

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(