From f3693aa8ba7be410c5d892eaf5d11580d6a969b2 Mon Sep 17 00:00:00 2001 From: Adrian Date: Mon, 27 Jul 2026 18:23:56 +0200 Subject: [PATCH] Fix port issue with cached DNS (#7772) * Fix port issue with cached DNS * Keep ports off the cache * Complete coverage --- scrapy/resolver.py | 17 +++++++++-- tests/test_resolver.py | 69 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/scrapy/resolver.py b/scrapy/resolver.py index 270a7fbf5..0e9775fd1 100644 --- a/scrapy/resolver.py +++ b/scrapy/resolver.py @@ -2,6 +2,7 @@ from __future__ import annotations from typing import TYPE_CHECKING, Any +import attr from twisted.internet import defer from twisted.internet.base import ReactorBase, ThreadedResolver from twisted.internet.interfaces import ( @@ -70,6 +71,12 @@ class CachingThreadedResolver(ThreadedResolver): return result +def _address_with_port(address: IAddress, port: int) -> IAddress: + if getattr(address, "port", port) == port: + return address + return attr.evolve(address, port=port) + + @implementer(IHostResolution) class HostResolution: def __init__(self, name: str): @@ -97,7 +104,11 @@ class _CachingResolutionReceiver: def resolutionComplete(self) -> None: self.resolutionReceiver.resolutionComplete() if self.addresses: - dnscache[self.hostName] = self.addresses + # Name resolution does not depend on the port, so cache entries are + # kept port-agnostic and the requested port is set on cache hits. + dnscache[self.hostName] = [ + _address_with_port(address, 0) for address in self.addresses + ] @implementer(IHostnameResolver) @@ -142,7 +153,7 @@ class CachingHostnameResolver: transportSemantics, ) resolutionReceiver.resolutionBegan(HostResolution(hostName)) - for addr in addresses: - resolutionReceiver.addressResolved(addr) + for address in addresses: + resolutionReceiver.addressResolved(_address_with_port(address, portNumber)) resolutionReceiver.resolutionComplete() return resolutionReceiver diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 7cca45ed1..8e69f62a0 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -3,6 +3,7 @@ from __future__ import annotations from unittest.mock import Mock import pytest +from twisted.internet.address import IPv4Address, IPv6Address from scrapy.resolver import CachingHostnameResolver, CachingThreadedResolver, dnscache from scrapy.utils.defer import maybe_deferred_to_future @@ -55,11 +56,77 @@ def test_caching_hostname_resolver_no_addresses_not_cached(): assert "example.com" not in dnscache +def test_caching_hostname_resolver_cached_addresses_have_no_port(): + def fake_resolve(receiver, *_): + receiver.resolutionBegan(Mock()) + receiver.addressResolved(IPv4Address("TCP", "1.2.3.4", 80)) + receiver.addressResolved(IPv6Address("TCP", "::1", 80)) + receiver.resolutionComplete() + return receiver + + reactor = Mock() + reactor.nameResolver.resolveHostName.side_effect = fake_resolve + + receiver = Mock() + resolver = CachingHostnameResolver(reactor, cache_size=10) + resolver.resolveHostName(receiver, "example.com", portNumber=80) + + # The port requested on a cache miss is passed through unchanged, but it is + # not part of what gets cached. + resolved_ports = [ + call.args[0].port for call in receiver.addressResolved.call_args_list + ] + assert resolved_ports == [80, 80] + assert [address.port for address in dnscache["example.com"]] == [0, 0] + + +def test_caching_hostname_resolver_cache_hit_without_port(): + cached_addresses = [ + IPv4Address("TCP", "1.2.3.4", 0), + IPv6Address("TCP", "::1", 0), + ] + dnscache["example.com"] = cached_addresses + + receiver = Mock() + resolver = CachingHostnameResolver(Mock(), cache_size=10) + resolver.resolveHostName(receiver, "example.com") + + # Cached addresses already use the requested port, so they are reused as is. + resolved_addresses = [ + call.args[0] for call in receiver.addressResolved.call_args_list + ] + assert all( + resolved is cached + for resolved, cached in zip(resolved_addresses, cached_addresses, strict=True) + ) + + +def test_caching_hostname_resolver_cache_hit_uses_requested_port(): + dnscache["example.com"] = [ + IPv4Address("TCP", "1.2.3.4", 0), + IPv6Address("TCP", "::1", 0), + ] + + receiver = Mock() + resolver = CachingHostnameResolver(Mock(), cache_size=10) + resolver.resolveHostName(receiver, "example.com", portNumber=443) + + resolved_addresses = [ + call.args[0] for call in receiver.addressResolved.call_args_list + ] + assert resolved_addresses == [ + IPv4Address("TCP", "1.2.3.4", 443), + IPv6Address("TCP", "::1", 443), + ] + # The cached addresses must not be mutated in place. + assert [address.port for address in dnscache["example.com"]] == [0, 0] + + def test_caching_hostname_resolver_dnscache_disabled_rejects_storage(): def fake_resolve(receiver, *_): receiver.resolutionBegan(Mock()) - receiver.addressResolved(Mock()) + receiver.addressResolved(IPv4Address("TCP", "1.2.3.4", 80)) receiver.resolutionComplete() return receiver