Read a Dynadot zone the way Dynadot actually reports one

A live zone was unreadable for two reasons, and the second was hidden behind
the first.

The TTL arrives as a string. "300" failed isinstance(ttl, int), while the write
path already coerced the same value with int(record.ttl): reading was stricter
than writing, so we refused zones we were perfectly capable of writing. A bool
is now excluded explicitly, because True is an int in Python and would have been
read as a TTL of 1 and then rejected for being out of range rather than for
being a bool.

Records arrive as record_value1 and are written back as value1. The reader knew
only the write spelling, the one it never receives, so every record on a real
zone came back incomplete. Both are accepted, response spelling first.

Either alone makes every Dynadot zone unreadable, which is why neither had been
noticed: no Dynadot zone had ever been read successfully.

This matters more than a parse error usually would. set_hosts replaces the whole
zone with what the caller passes, so a value the reader drops is a record
deleted from a live domain. The test is shaped from an actual response rather
than from the documentation, and asserts that all six records survive with their
values intact, including an MX preference and a record type as unusual as
STEALTH.

ampg/dynadot_dns.py

@@ -80,6 +80,29 @@ def _validated_origin(value: str) -> str:
     return origin
 
 
+def _zone_ttl(value: Any) -> int:
+    """Dynadot reports the zone TTL as a string.
+
+    A live zone came back with "300" and the read path required an int, so every
+    Dynadot zone was unreadable while the write path, which already coerces with
+    int(record.ttl), would have accepted the same value. Reading was stricter
+    than writing, which is the wrong way round.
+
+    A bool is excluded explicitly because it is an int in Python and True would
+    otherwise pass as a TTL of 1 and be rejected for the wrong reason.
+    """
+    if isinstance(value, str):
+        text = value.strip()
+        if not text.isdigit():
+            raise RuntimeError("Dynadot returned an invalid zone TTL")
+        value = int(text)
+    if isinstance(value, bool) or not isinstance(value, int):
+        raise RuntimeError("Dynadot returned an invalid zone TTL")
+    if value < 60 or value > 86400:
+        raise RuntimeError("Dynadot returned an invalid zone TTL")
+    return value
+
+
 def _validated_source_cidr(value: str) -> str:
     try:
         network = ipaddress.ip_network(value, strict=True)
@@ -202,9 +225,7 @@ class DynadotDNSProvider:
         glue_type = str(glue.get("glue_type", "")).upper()
         if glue_type and glue_type != "DNS":
             raise RuntimeError("Dynadot zone is not using Dynadot DNS")
-        ttl = glue.get("ttl", DEFAULT_TTL)
-        if not isinstance(ttl, int) or ttl < 60 or ttl > 86400:
-            raise RuntimeError("Dynadot returned an invalid zone TTL")
+        ttl = _zone_ttl(glue.get("ttl", DEFAULT_TTL))
         records: list[ProviderDNSRecord] = []
         for field, name_field in (
             ("dns_main_list", None),
@@ -223,8 +244,16 @@ class DynadotDNSProvider:
                         "Dynadot returned an unsupported DNS record type"
                     )
                 name = "@" if name_field is None else _record_value(item, name_field)
-                value1 = _record_value(item, "value1")
-                value2 = _record_value(item, "value2")
+                # Dynadot answers with record_value1 and writes back value1.
+                # The read path only knew the write spelling, so every record on
+                # a live zone came back incomplete. Both are accepted, response
+                # spelling first, because that is what a real zone sends.
+                value1 = _record_value(item, "record_value1")
+                if value1 is None:
+                    value1 = _record_value(item, "value1")
+                value2 = _record_value(item, "record_value2")
+                if value2 is None:
+                    value2 = _record_value(item, "value2")
                 if not name or value1 is None:
                     raise RuntimeError("Dynadot returned an incomplete DNS record")
                 mx_pref = None

tests/test_dynadot_dns.py

@@ -283,6 +283,106 @@ class DynadotDNSTests(unittest.TestCase):
         self.assertNotIn("X-Request-Id", captured)
         self.assertNotIn("X-request-id", captured)
 
+    def test_a_string_zone_ttl_is_accepted_because_dynadot_sends_one(self):
+        """A live zone returned ttl as "300".
+
+        The read path required an int and rejected every Dynadot zone, while the
+        write path already coerced the same value with int(record.ttl). Reading
+        was stricter than writing, which is the wrong way round: it made zones
+        unreadable that we were perfectly capable of writing.
+        """
+        from ampg.dynadot_dns import _zone_ttl
+
+        self.assertEqual(300, _zone_ttl("300"))
+        self.assertEqual(1800, _zone_ttl(" 1800 "))
+        self.assertEqual(300, _zone_ttl(300))
+
+    def test_a_ttl_that_is_not_a_whole_number_of_seconds_is_refused(self):
+        from ampg.dynadot_dns import _zone_ttl
+
+        for value in ("", "abc", "300.0", None, 3.5, [], {}):
+            with self.subTest(value=value):
+                with self.assertRaises(RuntimeError):
+                    _zone_ttl(value)
+
+    def test_a_bool_is_not_a_ttl(self):
+        # True is an int in Python and would otherwise be read as 1, then
+        # rejected for being out of range rather than for being a bool.
+        from ampg.dynadot_dns import _zone_ttl
+
+        with self.assertRaises(RuntimeError):
+            _zone_ttl(True)
+
+    def test_the_range_still_holds_after_coercion(self):
+        from ampg.dynadot_dns import _zone_ttl
+
+        for value in ("59", "86401", 59, 86401):
+            with self.subTest(value=value):
+                with self.assertRaises(RuntimeError):
+                    _zone_ttl(value)
+        self.assertEqual(60, _zone_ttl("60"))
+        self.assertEqual(86400, _zone_ttl("86400"))
+
+    def test_a_real_zone_response_reads_end_to_end(self):
+        """Shaped from an actual suchshop.lol response, not from the docs.
+
+        Two things only a live zone showed. The TTL arrives as a string, and the
+        records use record_value1 while the write path uses value1, so the
+        reader knew only the spelling it never receives. Either one alone makes
+        every Dynadot zone unreadable, and the second was hidden behind the
+        first.
+        """
+        def opener(request, timeout=None):
+            return FakeResponse({
+                "code": 200,
+                "data": {"glue_info": {
+                    "glue_type": "DNS",
+                    "ttl": "300",
+                    "dns_main_list": [
+                        {"record_type": "stealth", "record_value1": "mail.example.test",
+                         "record_value2": "10"},
+                        {"record_type": "txt",
+                         "record_value1": "v=spf1 mx include:spf-c.mailbaby.net -all"},
+                        {"record_type": "a", "record_value1": "203.0.113.10"},
+                    ],
+                    "dns_sub_list": [
+                        {"record_type": "a", "record_value1": "203.0.113.11", "sub_host": "mail"},
+                        {"record_type": "mx", "record_value1": "mail.example.test",
+                         "record_value2": "10", "sub_host": "mail"},
+                        {"record_type": "txt", "record_value1": "v=DKIM1;k=rsa;p=AAAA",
+                         "sub_host": "modoboa._domainkey"},
+                    ],
+                }},
+            })
+
+        provider = DynadotDNSProvider(credentials=self.credentials, opener=opener)
+        snapshot = provider.get_hosts("example.test")
+
+        self.assertEqual(6, len(snapshot.records), "no record may be dropped")
+        by_name = {(r.name, r.type): r for r in snapshot.records}
+        # A full-zone write sends back everything read, so a value lost here is
+        # a record deleted from a live zone.
+        self.assertEqual("203.0.113.10", by_name[("@", "A")].value)
+        self.assertEqual("v=DKIM1;k=rsa;p=AAAA", by_name[("modoboa._domainkey", "TXT")].value)
+        self.assertEqual("mail.example.test", by_name[("mail", "MX")].value)
+        self.assertEqual(10, by_name[("mail", "MX")].mx_pref)
+        self.assertIn(("@", "STEALTH"), by_name, "an unusual type is still a record")
+
+    def test_the_write_spelling_is_still_accepted(self):
+        # Kept so the two spellings cannot quietly become one.
+        def opener(request, timeout=None):
+            return FakeResponse({
+                "code": 200,
+                "data": {"glue_info": {
+                    "glue_type": "DNS", "ttl": 1800,
+                    "dns_main_list": [{"record_type": "txt", "value1": "v=spf1 -all"}],
+                    "dns_sub_list": [],
+                }},
+            })
+
+        provider = DynadotDNSProvider(credentials=self.credentials, opener=opener)
+        self.assertEqual("v=spf1 -all", provider.get_hosts("example.test").records[0].value)
+
     def test_mixed_ttl_and_broad_source_allowlist_fail_closed(self):
         provider = DynadotDNSProvider(credentials=self.credentials)
         with self.assertRaisesRegex(ValueError, "one observed"):