fix(url_safety): allow DNS failure in proxy/sandbox environments (salvage of #68469 by @kuangmi-bit) - #70874
Merged
Conversation
When the runtime blocks direct DNS (NVIDIA OpenShell, Docker + Squid, corporate proxy with DNS-only-via-proxy), socket.getaddrinfo() fails and is_safe_url() blocks *all* requests — including legitimate public URLs via the configured proxy. Add _proxy_is_configured() helper that checks HTTPS_PROXY, HTTP_PROXY, http_proxy, https_proxy, ALL_PROXY, all_proxy. When DNS fails AND a proxy is configured, delegate DNS resolution to the proxy rather than blocking outright. Blocked hostnames (metadata.google.internal, 169.254.169.254, etc.) are checked BEFORE DNS resolution, so cloud metadata endpoints remain blocked regardless of proxy status. Fixes #32217
…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.
Contributor
૮ >ﻌ< ა ci reviewran on ffc2b85 all good! |
alt-glitch
added
type/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Web tools work again in proxy-only sandboxes (Docker + Squid, NVIDIA OpenShell, iron-proxy egress containers) where direct DNS is blocked at the network level: when an HTTP proxy env var is configured,
is_safe_url()delegates hostname resolution to the proxy instead of failing closed on the pre-flight DNS check. Fixes #32217. Salvage of #68469 by @kuangmi-bit with authorship preserved, plus a hardening pass.Root cause:
is_safe_url()requiressocket.getaddrinfo()to succeed before allowing any outbound request. In proxy-only environments DNS is intentionally unavailable — only HTTP(S) through the proxy works — so every web tool call was blocked even though the request would succeed. This includes our own iron-proxy egress sandboxes (#70848) when the container has no direct DNS.Changes
tools/url_safety.py(salvaged from fix(url_safety): allow DNS failure in proxy/sandbox environments #68469): ongaierror, ifHTTP_PROXY/HTTPS_PROXY/ALL_PROXY(any case) is set, delegate resolution to the proxy and allow; otherwise fail closed as before. Blocked-hostname floor (cloud metadata) runs before the skip.tools/url_safety.py(our hardening): literal-IP hostnames never take the delegation path — an IP needs no DNS, so a resolution failure on one is not a proxy symptom; they stay on the fail-closed path and the blocked-IP floor.tests/tools/test_url_safety.py: newTestProxyEnvironmentDnsDelegationclass (7 regression tests: delegation fires, floor intact for metadata hostname + literal metadata IP + private IP, DNS-success path unchanged under proxy, empty proxy var ignored) and ambient-proxy-env guards on the 3 pre-existing DNS-failure tests.Validation
tests/tools/test_url_safety.pyInfographic