Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions tests/tools/test_url_safety.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,29 +166,52 @@ def test_non_cgnat_100_allowed(self):
# 100.0.0.1 is a global IP, not in CGNAT range
assert is_safe_url("http://legit-host.example/") is True

def test_benchmark_ip_blocked_for_non_allowlisted_host(self):
def test_benchmark_range_allowed_for_any_host(self):
"""198.18.0.0/15 (benchmark/DNS-filtering range) is not blocked."""
with patch("socket.getaddrinfo", return_value=[
(2, 1, 6, "", ("198.18.0.23", 0)),
]):
assert is_safe_url("https://example.com/file.jpg") is False
assert is_safe_url("https://example.com/file.jpg") is True

def test_cdimage_ubuntu_allowed_on_filtered_network(self):
"""cdimage.ubuntu.com resolving to benchmark range should be allowed.

Regression test for https://github.com/NousResearch/hermes-agent/issues/35423
DNS-based content-filtering services (OpenDNS, etc.) may resolve
public domains into the 198.18.0.0/15 range.
"""
with patch("socket.getaddrinfo", return_value=[
(2, 1, 6, "", ("198.18.0.65", 0)),
]):
assert is_safe_url("https://cdimage.ubuntu.com/ubuntu/releases/26.04/release/") is True

def test_qq_multimedia_hostname_allowed_with_benchmark_ip(self):
with patch("socket.getaddrinfo", return_value=[
(2, 1, 6, "", ("198.18.0.23", 0)),
]):
assert is_safe_url("https://multimedia.nt.qq.com.cn/download?id=123") is True

def test_qq_multimedia_hostname_exception_is_exact_match(self):
def test_qq_multimedia_subdomain_also_allowed_via_benchmark_range(self):
"""Subdomain is allowed because the entire benchmark range is allowed.

Previously only exact _TRUSTED_PRIVATE_IP_HOSTS entries were exempt;
now 198.18.0.0/15 is excluded from _is_blocked_ip entirely.
"""
with patch("socket.getaddrinfo", return_value=[
(2, 1, 6, "", ("198.18.0.23", 0)),
]):
assert is_safe_url("https://sub.multimedia.nt.qq.com.cn/download?id=123") is False
assert is_safe_url("https://sub.multimedia.nt.qq.com.cn/download?id=123") is True

def test_qq_multimedia_hostname_exception_requires_https(self):
def test_qq_multimedia_http_allowed_via_benchmark_range(self):
"""HTTP is now allowed because the benchmark range is excluded from blocking.

Previously the QQ allowlist required HTTPS; now 198.18.0.0/15 is
excluded from _is_blocked_ip entirely, so both HTTP and HTTPS pass.
"""
with patch("socket.getaddrinfo", return_value=[
(2, 1, 6, "", ("198.18.0.23", 0)),
]):
assert is_safe_url("http://multimedia.nt.qq.com.cn/download?id=123") is False
assert is_safe_url("http://multimedia.nt.qq.com.cn/download?id=123") is True

def test_qq_multimedia_hostname_dns_failure_still_blocked(self):
with patch("socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed")):
Expand All @@ -201,7 +224,7 @@ class TestIsBlockedIp:
@pytest.mark.parametrize("ip_str", [
"127.0.0.1", "10.0.0.1", "172.16.0.1", "192.168.1.1",
"169.254.169.254", "0.0.0.0", "224.0.0.1", "255.255.255.255",
"100.64.0.1", "100.100.100.100", "100.127.255.254", "198.18.0.23",
"100.64.0.1", "100.100.100.100", "100.127.255.254",
"::1", "fe80::1", "fc00::1", "fd12::1", "ff02::1",
"::ffff:127.0.0.1", "::ffff:169.254.169.254",
])
Expand All @@ -211,6 +234,7 @@ def test_blocked_ips(self, ip_str):

@pytest.mark.parametrize("ip_str", [
"8.8.8.8", "93.184.216.34", "1.1.1.1", "100.0.0.1",
"198.18.0.23", "198.19.255.255",
"2606:4700::1", "2001:4860:4860::8888",
])
def test_allowed_ips(self, ip_str):
Expand Down Expand Up @@ -514,6 +538,7 @@ def test_ipv4_mapped_blocked_ips(self, ip_str):
"::ffff:8.8.8.8", # Public DNS
"::ffff:93.184.216.34", # example.com
"::ffff:100.0.0.1", # Not in CGNAT range
"::ffff:198.18.0.65", # Benchmark range (DNS filtering)
])
def test_ipv4_mapped_allowed_ips(self, ip_str):
"""IPv4-mapped IPv6 addresses that should be allowed."""
Expand Down
20 changes: 19 additions & 1 deletion tools/url_safety.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@

# Exact HTTPS hostnames allowed to resolve to private/benchmark-space IPs.
# This is intentionally narrow: QQ media downloads can legitimately resolve
# to 198.18.0.0/15 behind local proxy/benchmark infrastructure.
# to private IPs behind local proxy infrastructure. Note: the 198.18.0.0/15
# benchmark range is now excluded from _is_blocked_ip entirely (see
# _BENCHMARK_RANGE), so this allowlist only matters for hosts that resolve
# to *other* private ranges.
_TRUSTED_PRIVATE_IP_HOSTS = frozenset({
"multimedia.nt.qq.com.cn",
})
Expand All @@ -80,6 +83,16 @@
# VPNs, and some cloud internal networks.
_CGNAT_NETWORK = ipaddress.ip_network("100.64.0.0/10")

# 198.18.0.0/15 (Benchmarking / DNS-filtering proxy range) IS marked as
# is_private by Python's ipaddress module, but it is NOT a real private
# network used for internal services. DNS-based content-filtering services
# (OpenDNS/Cisco Umbrella, CleanBrowsing, etc.) resolve blocked or
# sinkholed domains into this range. Public sites like cdimage.ubuntu.com
# can legitimately resolve here on filtered networks. Blocking the entire
# range produces false positives without meaningful SSRF protection β€” the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default-wide bypass changes the deliberate current policy from one exact HTTPS host to every hostname and scheme resolving into 198.18.0.0/15. Please preserve the default SSRF boundary and make broader allowance an explicit config.yaml opt-in if maintainers choose that direction.

# actual SSRF targets (RFC 1918, cloud metadata) are handled separately.
_BENCHMARK_RANGE = ipaddress.ip_network("198.18.0.0/15")

# ---------------------------------------------------------------------------
# Global toggle: allow private/internal IP resolution
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -152,13 +165,18 @@ def _is_blocked_ip(ip: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool:
# by their embedded IPv4 address, not as IPv6
if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped is not None:
embedded_ip = ip.ipv4_mapped
if embedded_ip in _BENCHMARK_RANGE:
return False
return (embedded_ip.is_private or embedded_ip.is_loopback or
embedded_ip.is_link_local or embedded_ip.is_reserved or
embedded_ip.is_multicast or embedded_ip.is_unspecified or
embedded_ip in _CGNAT_NETWORK)

# Standard IPv4/IPv6 address checking
if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved:
# Exclude 198.18.0.0/15 benchmark range β€” not a real private network
if ip in _BENCHMARK_RANGE:
return False
return True
if ip.is_multicast or ip.is_unspecified:
return True
Expand Down
Loading