Honor raise_on_serial_went_backwards on both inbound_xfr paths (#1298)

dns.query.inbound_xfr() forwarded the flag on its TCP path but not its
UDP one, and dns.asyncquery.inbound_xfr() did the opposite, so each
implementation honored the option on exactly one of its two paths.  For
the async version that is the default path, since udp_mode defaults to
UDPMode.NEVER.

_inbound_xfr()'s default for the parameter is dropped as well: it is
private, all five call sites now pass the flag, and the default is what
turned the two missed call sites into silent no-ops rather than errors.

Co-authored-by: Dylan Pulver <dylanpulver@users.noreply.github.com>

Author: dylanpulver

dns/asyncquery.py

@@ -850,7 +850,7 @@ async def _inbound_xfr(
     serial: int | None,
     timeout: float | None,
     expiration: float | None,
-    raise_on_serial_went_backwards: bool = True,
+    raise_on_serial_went_backwards: bool,
 ) -> Any:
     """Given a socket, does the zone transfer."""
     rdtype = query.question[0].rdtype
@@ -960,6 +960,12 @@ async def inbound_xfr(
     )
     async with s:
         async for _ in _inbound_xfr(  # pyright: ignore
-            txn_manager, s, query, serial, timeout, expiration  # pyright: ignore
+            txn_manager,
+            s,
+            query,
+            serial,
+            timeout,
+            expiration,  # pyright: ignore
+            raise_on_serial_went_backwards,
         ):
             pass

dns/query.py

@@ -1500,7 +1500,7 @@ def _inbound_xfr(
     serial: int | None,
     timeout: float | None,
     expiration: float | None,
-    raise_on_serial_went_backwards: bool = True,
+    raise_on_serial_went_backwards: bool,
 ) -> Any:
     """Given a socket, does the zone transfer."""
     rdtype = query.question[0].rdtype
@@ -1723,7 +1723,13 @@ def inbound_xfr(
             _connect(s, destination, expiration)
             try:
                 for _ in _inbound_xfr(
-                    txn_manager, s, query, serial, timeout, expiration
+                    txn_manager,
+                    s,
+                    query,
+                    serial,
+                    timeout,
+                    expiration,
+                    raise_on_serial_went_backwards,
                 ):
                     pass
                 return

tests/test_xfr.py

@@ -851,3 +851,123 @@ def old_runner(awaitable):
 
         runner = old_runner
     runner(run())
+
+
+#
+# The master's serial is older than ours, in a single-message IXFR response.
+#
+went_backwards_ixfr = """id 1
+opcode QUERY
+rcode NOERROR
+flags AA
+;QUESTION
+example. IN IXFR
+;ANSWER
+@ 3600 IN SOA foo bar 5 2 3 4 5
+"""
+
+# Newer than what went_backwards_ixfr offers, so an IXFR from it goes backwards.
+went_backwards_base = """@ 3600 IN SOA foo bar 6 2 3 4 5
+@ 3600 IN NS ns1
+"""
+
+
+class WentBackwardsNanoNameserver(Server):
+    def __init__(self):
+        super().__init__(origin=dns.name.from_text("example"))
+
+    def handle(self, request):
+        try:
+            r = dns.message.from_text(
+                went_backwards_ixfr, one_rr_per_rrset=True, origin=self.origin
+            )
+            r.id = request.message.id
+            return r
+        except Exception:
+            pass
+
+
+def went_backwards_zone():
+    return dns.zone.from_text(
+        went_backwards_base, "example", zone_factory=dns.versioned.Zone
+    )
+
+
+def check_went_backwards_zone(zone):
+    # Declining to raise ends the transfer without applying anything, so the
+    # zone must be untouched.
+    assert zone == dns.zone.from_text(went_backwards_base, "example")
+
+
+#
+# raise_on_serial_went_backwards has to reach dns.xfr.Inbound on both the UDP
+# and the TCP path.  UDPMode.NEVER is TCP only; TRY_FIRST sends the IXFR over
+# UDP first.
+#
+udp_modes = [dns.query.UDPMode.NEVER, dns.query.UDPMode.TRY_FIRST]
+
+
+@pytest.mark.skipif(not _nanonameserver_available, reason="requires nanonameserver")
+@pytest.mark.parametrize("udp_mode", udp_modes)
+def test_sync_inbound_xfr_serial_went_backwards(udp_mode):
+    with WentBackwardsNanoNameserver() as ns:
+        where, port = ns.tcp_address
+        with pytest.raises(dns.xfr.SerialWentBackwards):
+            dns.query.inbound_xfr(
+                where, went_backwards_zone(), port=port, udp_mode=udp_mode
+            )
+        zone = went_backwards_zone()
+        dns.query.inbound_xfr(
+            where,
+            zone,
+            port=port,
+            udp_mode=udp_mode,
+            raise_on_serial_went_backwards=False,
+        )
+        check_went_backwards_zone(zone)
+
+
+async def async_inbound_xfr_serial_went_backwards(udp_mode):
+    with WentBackwardsNanoNameserver() as ns:
+        where, port = ns.tcp_address
+        with pytest.raises(dns.xfr.SerialWentBackwards):
+            await dns.asyncquery.inbound_xfr(
+                where, went_backwards_zone(), port=port, udp_mode=udp_mode
+            )
+        zone = went_backwards_zone()
+        await dns.asyncquery.inbound_xfr(
+            where,
+            zone,
+            port=port,
+            udp_mode=udp_mode,
+            raise_on_serial_went_backwards=False,
+        )
+        check_went_backwards_zone(zone)
+
+
+@pytest.mark.skipif(not _nanonameserver_available, reason="requires nanonameserver")
+@pytest.mark.parametrize("udp_mode", udp_modes)
+def test_asyncio_inbound_xfr_serial_went_backwards(udp_mode):
+    dns.asyncbackend.set_default_backend("asyncio")
+
+    async def run():
+        await async_inbound_xfr_serial_went_backwards(udp_mode)
+
+    asyncio.run(run())
+
+
+try:
+    import trio
+
+    @pytest.mark.skipif(not _nanonameserver_available, reason="requires nanonameserver")
+    @pytest.mark.parametrize("udp_mode", udp_modes)
+    def test_trio_inbound_xfr_serial_went_backwards(udp_mode):
+        dns.asyncbackend.set_default_backend("trio")
+
+        async def run():
+            await async_inbound_xfr_serial_went_backwards(udp_mode)
+
+        trio.run(run)
+
+except ImportError:
+    pass