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() 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..b276586 100644 --- a/tests/test_dnssec.py +++ b/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", 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(