mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-25 17:18:11 +00:00
fix(url_safety): harden proxy DNS delegation — literal IPs stay fail-closed + regression tests
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.
This commit is contained in:
parent
931ca437ff
commit
4a0b84ec09
3 changed files with 74 additions and 5 deletions
1
contributors/emails/kuangmi@nudge.com.cn
Normal file
1
contributors/emails/kuangmi@nudge.com.cn
Normal file
|
|
@ -0,0 +1 @@
|
|||
kuangmi-bit
|
||||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue