From 4a0b84ec0926effd29f2b692ae4256843eba167c Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:10:59 -0700 Subject: [PATCH] =?UTF-8?q?fix(url=5Fsafety):=20harden=20proxy=20DNS=20del?= =?UTF-8?q?egation=20=E2=80=94=20literal=20IPs=20stay=20fail-closed=20+=20?= =?UTF-8?q?regression=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the salvaged #68469 commit: - Literal-IP hostnames never take the proxy DNS-delegation path (a getaddrinfo failure on a literal IP is not a proxy-environment symptom, and IPs need no DNS) — keeps the private-IP/metadata floor intact under proxy env vars. - Adds TestProxyEnvironmentDnsDelegation: delegation fires only for hostnames, metadata hostname/IP floor holds, DNS-success path unchanged, empty proxy var ignored. - Guards the three pre-existing DNS-failure tests against ambient proxy env vars so they don't flake on developer machines. --- contributors/emails/kuangmi@nudge.com.cn | 1 + tests/tools/test_url_safety.py | 66 ++++++++++++++++++++++-- tools/url_safety.py | 12 ++++- 3 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 contributors/emails/kuangmi@nudge.com.cn diff --git a/contributors/emails/kuangmi@nudge.com.cn b/contributors/emails/kuangmi@nudge.com.cn new file mode 100644 index 000000000000..7f3575558dd2 --- /dev/null +++ b/contributors/emails/kuangmi@nudge.com.cn @@ -0,0 +1 @@ +kuangmi-bit diff --git a/tests/tools/test_url_safety.py b/tests/tools/test_url_safety.py index 18ebb8bb04d0..1dcf36fd0c10 100644 --- a/tests/tools/test_url_safety.py +++ b/tests/tools/test_url_safety.py @@ -135,11 +135,67 @@ class TestIsSafeUrl: ]): assert is_safe_url("http://[::1]:8080/") is False - def test_dns_failure_blocked(self): - """DNS failures now fail closed — block the request.""" + def test_dns_failure_blocked(self, monkeypatch): + """DNS failures fail closed — block the request (no proxy configured).""" + for var in ("HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"): + monkeypatch.delenv(var, raising=False) with patch("socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed")): assert is_safe_url("https://nonexistent.example.com") is False + +class TestProxyEnvironmentDnsDelegation: + """When an HTTP proxy is configured, DNS is delegated to the proxy. + + Sandbox / proxy-only environments (Docker + Squid, NVIDIA OpenShell, + iron-proxy egress sandboxes) block direct DNS at the network level; + only HTTP(S) via the proxy works. is_safe_url must not fail closed on + the pre-flight DNS check there — the proxy is the egress boundary. + Regression tests for #32217 / PR #68469. + """ + + @pytest.fixture(autouse=True) + def _clear_proxy_env(self, monkeypatch): + for var in ("HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"): + monkeypatch.delenv(var, raising=False) + + def test_dns_failure_allowed_when_proxy_configured(self, monkeypatch): + monkeypatch.setenv("HTTPS_PROXY", "http://host.docker.internal:9090") + with patch("socket.getaddrinfo", side_effect=socket.gaierror("blocked at network level")): + assert is_safe_url("https://api.openai.com/v1/models") is True + + def test_lowercase_proxy_var_also_recognized(self, monkeypatch): + monkeypatch.setenv("http_proxy", "http://proxy.internal:3128") + with patch("socket.getaddrinfo", side_effect=socket.gaierror("no dns")): + assert is_safe_url("https://example.com/") is True + + def test_metadata_hostname_still_blocked_with_proxy(self, monkeypatch): + """The blocked-hostname floor runs BEFORE the DNS skip.""" + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.internal:3128") + with patch("socket.getaddrinfo", side_effect=socket.gaierror("no dns")): + assert is_safe_url("http://metadata.google.internal/computeMetadata/v1/") is False + + def test_literal_metadata_ip_still_blocked_with_proxy(self, monkeypatch): + """Literal IPs never take the DNS-failure path — floor intact.""" + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.internal:3128") + assert is_safe_url("http://169.254.169.254/latest/meta-data/") is False + + def test_literal_private_ip_still_blocked_with_proxy(self, monkeypatch): + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.internal:3128") + assert is_safe_url("http://192.168.1.1/admin") is False + + def test_dns_success_path_unchanged_with_proxy(self, monkeypatch): + """When DNS resolves, the normal IP checks still apply under a proxy.""" + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.internal:3128") + with patch("socket.getaddrinfo", return_value=[ + (2, 1, 6, "", ("10.0.0.5", 0)), + ]): + assert is_safe_url("https://internal.corp/") is False + + def test_empty_proxy_var_does_not_trigger_delegation(self, monkeypatch): + monkeypatch.setenv("HTTPS_PROXY", "") + with patch("socket.getaddrinfo", side_effect=socket.gaierror("fail")): + assert is_safe_url("https://nonexistent.example.com") is False + def test_empty_url_blocked(self): assert is_safe_url("") is False @@ -275,7 +331,9 @@ class TestIsSafeUrl: ]): assert is_safe_url("http://multimedia.nt.qq.com.cn/download?id=123") is False - def test_qq_multimedia_hostname_dns_failure_still_blocked(self): + def test_qq_multimedia_hostname_dns_failure_still_blocked(self, monkeypatch): + for var in ("HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"): + monkeypatch.delenv(var, raising=False) with patch("socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed")): assert is_safe_url("https://multimedia.nt.qq.com.cn/download?id=123") is False @@ -637,6 +695,8 @@ class TestAllowPrivateUrlsIntegration: def test_dns_failure_still_blocked_with_toggle(self, monkeypatch): """DNS failures are still blocked even with toggle on.""" monkeypatch.setenv("HERMES_ALLOW_PRIVATE_URLS", "true") + for var in ("HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"): + monkeypatch.delenv(var, raising=False) with patch("socket.getaddrinfo", side_effect=socket.gaierror("fail")): assert is_safe_url("https://nonexistent.example.com") is False diff --git a/tools/url_safety.py b/tools/url_safety.py index a301fdc3f203..00cf1b498dbc 100644 --- a/tools/url_safety.py +++ b/tools/url_safety.py @@ -442,8 +442,16 @@ def is_safe_url(url: str) -> bool: # the proxy rather than blocking the request outright. # The hostname was already checked against # _BLOCKED_HOSTNAMES above so metadata endpoints remain - # blocked regardless. - if _proxy_is_configured(): + # blocked regardless. Literal IPs never qualify — they + # need no DNS, so a getaddrinfo failure on one is not a + # proxy-environment symptom; keep them on the fail-closed + # path (and the blocked-IP floor) instead of delegating. + _is_literal_ip = True + try: + ipaddress.ip_address(hostname) + except ValueError: + _is_literal_ip = False + if not _is_literal_ip and _proxy_is_configured(): logger.debug( "DNS resolution failed for %s — proxy configured, " "allowing through for proxy-side resolution",