Repository: encode/httpx
Pull request: Cleaner no proxy support
Builds on #1099, and #1098.
Closes #1062.

Refactors our NO_PROXY logic, so that instead of calling `should_not_be_proxied` we instead setup the `proxies` infomation in such a way that it has any exclusions within it, eg...

```
# The following environment...

# ALL_PROXY=http://localhost:1234
# NO_PROXY=example.com

# Is equivalent to...
proxies={"all": httpx.Proxy("http://localhost:1234"), "all://*example.com": None}
```

Note that this PR also introduces wildcard domain prefixing for proxy configuration, since it's required in order to handle `NO_PROXY` equivalents correctly, so we now support...

```python
# Match any domain
proxies={"all://*": httpx.Proxy("http://localhost:1234")}

# Match "example.com" exactly.
proxies={"all://example.com": httpx.Proxy("http://localhost:1234")}

# Match "www.example.com" and "example.com". (But not "wwwexample.com")
proxies={"all://*example.com": httpx.Proxy("http://localhost:1234")}

# Match "www.example.com" (But not "example.com")
proxies={"all://*.example.com": httpx.Proxy("http://localhost:1234")}
```

Problem statement:
Don't call `should_not_be_proxied` on each request
Currently we're calling `should_not_be_proxied` on each request, which is problematic for two reasons...

* It's ignoring the `trust_env`, and relying on an environment variable unilaterally.
* It's preforming an extra chunk of work on each request that we'd rather not do.

Hints from issue discussion:
@tomchristie 

Do I understand correctly, that you told about working with requests on session level?

Something a-la introducing url->proxy transport cache on the Session level

Also there is a question about "trust_env". Do you mean this piece of code: [httpx/_utils.py#L267](https://github.com/encode/httpx/blob/master/httpx/_utils.py#L267) ? 

So, please, could you help me, what should someone (e.g. me) do to complete the issue?
@tomchristie , @florimondmanca ^ if so, I can try to work on it. Please just advise me if I was right.
Not sure if I'd recommend contributors jumping on this one, since it might be a bit fiddly.

Commit message: Tweak comment on domain wildcards

Patch to review:
diff --git a/httpx/_client.py b/httpx/_client.py
index 4a7fc030a0..246defcba1 100644
--- a/httpx/_client.py
+++ b/httpx/_client.py
@@ -49,7 +49,6 @@
     get_environment_proxies,
     get_logger,
     same_origin,
-    should_not_be_proxied,
 )
 
 logger = get_logger(__name__)
@@ -90,7 +89,7 @@ def _get_proxy_map(
         if proxies is None:
             if trust_env:
                 return {
-                    key: Proxy(url=url)
+                    key: None if url is None else Proxy(url=url)
                     for key, url in get_environment_proxies().items()
                 }
             return {}
@@ -544,10 +543,9 @@ def _transport_for_url(self, url: URL) -> httpcore.SyncHTTPTransport:
         """
         enforce_http_url(url)
 
-        if self._proxies and not should_not_be_proxied(url):
-            for matcher, transport in self._proxies.items():
-                if matcher.matches(url):
-                    return self._transport if transport is None else transport
+        for matcher, transport in self._proxies.items():
+            if matcher.matches(url):
+                return self._transport if transport is None else transport
 
         return self._transport
 
@@ -1066,10 +1064,9 @@ def _transport_for_url(self, url: URL) -> httpcore.AsyncHTTPTransport:
         """
         enforce_http_url(url)
 
-        if self._proxies and not should_not_be_proxied(url):
-            for matcher, transport in self._proxies.items():
-                if matcher.matches(url):
-                    return self._transport if transport is None else transport
+        for matcher, transport in self._proxies.items():
+            if matcher.matches(url):
+                return self._transport if transport is None else transport
 
         return self._transport
 
diff --git a/httpx/_utils.py b/httpx/_utils.py
index 2533a86d2a..c6f21c5ce7 100644
--- a/httpx/_utils.py
+++ b/httpx/_utils.py
@@ -294,42 +294,39 @@ def same_origin(url: "URL", other: "URL") -> bool:
     )
 
 
-def should_not_be_proxied(url: "URL") -> bool:
-    """
-    Return True if url should not be proxied,
-    return False otherwise.
-    """
-    no_proxy = getproxies().get("no")
-    if not no_proxy:
-        return False
-    no_proxy_list = [host.strip() for host in no_proxy.split(",")]
-    for name in no_proxy_list:
-        if name == "*":
-            return True
-        if name:
-            name = name.lstrip(".")  # ignore leading dots
-            name = re.escape(name)
-            pattern = r"(.+\.)?%s$" % name
-            if re.match(pattern, url.host, re.I) or re.match(
-                pattern, url.authority, re.I
-            ):
-                return True
-    return False
-
-
-def get_environment_proxies() -> typing.Dict[str, str]:
+def get_environment_proxies() -> typing.Dict[str, typing.Optional[str]]:
     """Gets proxy information from the environment"""
 
     # urllib.request.getproxies() falls back on System
     # Registry and Config for proxies on Windows and macOS.
     # We don't want to propagate non-HTTP proxies into
     # our configuration such as 'TRAVIS_APT_PROXY'.
-    supported_proxy_schemes = ("http", "https", "all")
-    return {
-        key: val
-        for key, val in getproxies().items()
-        if ("://" in key or key in supported_proxy_schemes)
-    }
+    proxy_info = getproxies()
+    mounts: typing.Dict[str, typing.Optional[str]] = {}
+
+    for scheme in ("http", "https", "all"):
+        if proxy_info.get(scheme):
+            mounts[scheme] = proxy_info[scheme]
+
+    no_proxy_hosts = [host.strip() for host in proxy_info.get("no", "").split(",")]
+    for hostname in no_proxy_hosts:
+        # See https://curl.haxx.se/libcurl/c/CURLOPT_NOPROXY.html for details
+        # on how names in `NO_PROXY` are handled.
+        if hostname == "*":
+            # If NO_PROXY=* is used or if "*" occurs as any one of the comma
+            # seperated hostnames, then we should just bypass any information
+            # from HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and always ignore
+            # proxies.
+            return {}
+        elif hostname:
+            # NO_PROXY=.google.com is marked as "all://*.google.com,
+            #   which disables "www.google.com" but not "google.com"
+            # NO_PROXY=google.com is marked as "all://*google.com,
+            #   which disables "www.google.com" and "google.com".
+            #   (But not "wwwgoogle.com")
+            mounts[f"all://*{hostname}"] = None
+
+    return mounts
 
 
 def to_bytes(value: typing.Union[str, bytes], encoding: str = "utf-8") -> bytes:
@@ -480,13 +477,32 @@ def __init__(self, pattern: str) -> None:
         url = URL(pattern)
         self.pattern = pattern
         self.scheme = "" if url.scheme == "all" else url.scheme
-        self.host = url.host
+        self.host = "" if url.host == "*" else url.host
         self.port = url.port
+        if not url.host or url.host == "*":
+            self.host_regex: typing.Optional[typing.Pattern[str]] = None
+        else:
+            if url.host.startswith("*."):
+                # *.example.com should match "www.example.com", but not "example.com"
+                domain = re.escape(url.host[2:])
+                self.host_regex = re.compile(f"^.+\\.{domain}$")
+            elif url.host.startswith("*"):
+                # *example.com should match "www.example.com" and "example.com"
+                domain = re.escape(url.host[1:])
+                self.host_regex = re.compile(f"^(.+\\.)?{domain}$")
+            else:
+                # example.com should match "example.com" but not "www.example.com"
+                domain = re.escape(url.host)
+                self.host_regex = re.compile(f"^{domain}$")
 
     def matches(self, other: "URL") -> bool:
         if self.scheme and self.scheme != other.scheme:
             return False
-        if self.host and self.host != other.host:
+        if (
+            self.host
+            and self.host_regex is not None
+            and not self.host_regex.match(other.host)
+        ):
             return False
         if self.port is not None and self.port != other.port:
             return False
@@ -496,10 +512,13 @@ def matches(self, other: "URL") -> bool:
     def priority(self) -> tuple:
         """
         The priority allows URLMatcher instances to be sortable, so that
-        we can match from most specific to least specific.
+        if we can match from most specific to least specific.
         """
-        port_priority = -1 if self.port is not None else 0
+        # URLs with a port should take priority over URLs without a port.
+        port_priority = 0 if self.port is not None else 1
+        # Longer hostnames should match first.
         host_priority = -len(self.host)
+        # Longer schemes should match first.
         scheme_priority = -len(self.scheme)
         return (port_priority, host_priority, scheme_priority)
 
diff --git a/tests/client/test_proxies.py b/tests/client/test_proxies.py
index d361372c1f..3eb884ca6e 100644
--- a/tests/client/test_proxies.py
+++ b/tests/client/test_proxies.py
@@ -56,6 +56,20 @@ def test_proxies_parameter(proxies, expected_proxies):
         ("http://example.com", {}, None),
         ("http://example.com", {"https": PROXY_URL}, None),
         ("http://example.com", {"http://example.net": PROXY_URL}, None),
+        # Using "*" should match any domain name.
+        ("http://example.com", {"http://*": PROXY_URL}, PROXY_URL),
+        ("https://example.com", {"http://*": PROXY_URL}, None),
+        # Using "example.com" should match example.com, but not www.example.com
+        ("http://example.com", {"http://example.com": PROXY_URL}, PROXY_URL),
+        ("http://www.example.com", {"http://example.com": PROXY_URL}, None),
+        # Using "*.example.com" should match www.example.com, but not example.com
+        ("http://example.com", {"http://*.example.com": PROXY_URL}, None),
+        ("http://www.example.com", {"http://*.example.com": PROXY_URL}, PROXY_URL),
+        # Using "*example.com" should match example.com and www.example.com
+        ("http://example.com", {"http://*example.com": PROXY_URL}, PROXY_URL),
+        ("http://www.example.com", {"http://*example.com": PROXY_URL}, PROXY_URL),
+        ("http://wwwexample.com", {"http://*example.com": PROXY_URL}, None),
+        # ...
         ("http://example.com:443", {"http://example.com": PROXY_URL}, PROXY_URL),
         ("http://example.com", {"all": PROXY_URL}, PROXY_URL),
         ("http://example.com", {"all": PROXY_URL, "http://example.com": None}, None),
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 88fb1000bb..fcbec55a30 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -15,7 +15,6 @@
     obfuscate_sensitive_headers,
     parse_header_links,
     same_origin,
-    should_not_be_proxied,
 )
 from tests.utils import override_log_level
 
@@ -228,75 +227,85 @@ def test_obfuscate_sensitive_headers(headers, output):
     [
         (
             "http://127.0.0.1",
-            {"NO_PROXY": ""},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": ""},
             False,
         ),  # everything proxied when no_proxy is empty/unset
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "127.0.0.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "127.0.0.1"},
             True,
         ),  # no_proxy as ip case is matched
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "https://127.0.0.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "https://127.0.0.1"},
             False,
         ),  # no_proxy with scheme is ignored
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "1.1.1.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "1.1.1.1"},
             False,
         ),  # different no_proxy means its proxied
         (
             "http://courses.mit.edu",
-            {"NO_PROXY": "mit.edu"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu"},
             True,
         ),  # no_proxy for sub-domain matches
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu"},
             False,
         ),  # domain is actually edu.info, so should be proxied
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu,edu.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu,edu.info"},
             True,
         ),  # list in no_proxy, matches second domain
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu, edu.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu, edu.info"},
             True,
         ),  # list with spaces in no_proxy
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu,mit.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu,mit.info"},
             False,
         ),  # list in no_proxy, without any domain matching
         (
             "https://foo.example.com",
-            {"NO_PROXY": "www.example.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "www.example.com"},
             False,
         ),  # different subdomains foo vs www means we still proxy
         (
             "https://www.example1.com",
-            {"NO_PROXY": ".example1.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": ".example1.com"},
             True,
         ),  # no_proxy starting with dot
         (
             "https://www.example2.com",
-            {"NO_PROXY": "ample2.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "ample2.com"},
             False,
         ),  # whole-domain matching
         (
             "https://www.example3.com",
-            {"NO_PROXY": "*"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "*"},
             True,
         ),  # wildcard * means nothing proxied
     ],
 )
 def test_should_not_be_proxied(url, no_proxy, expected):
     os.environ.update(no_proxy)
+    proxies = {
+        URLMatcher(key): value for key, value in get_environment_proxies().items()
+    }
     parsed_url = httpx.URL(url)
-    assert should_not_be_proxied(parsed_url) == expected
+
+    should_not_proxy = True
+    for matcher, value in sorted(proxies.items()):
+        if matcher.matches(parsed_url):
+            should_not_proxy = value is None
+            break
+
+    assert should_not_proxy == expected
 
 
 def test_same_origin():


Code review comments:
diff_hunk: @@ -228,75 +227,85 @@ def test_obfuscate_sensitive_headers(headers, output):
     [
         (
             "http://127.0.0.1",
-            {"NO_PROXY": ""},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": ""},
             False,
         ),  # everything proxied when no_proxy is empty/unset
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "127.0.0.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "127.0.0.1"},
             True,
         ),  # no_proxy as ip case is matched
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "https://127.0.0.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "https://127.0.0.1"},
             False,
         ),  # no_proxy with scheme is ignored
         (
             "http://127.0.0.1",
-            {"NO_PROXY": "1.1.1.1"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "1.1.1.1"},
             False,
         ),  # different no_proxy means its proxied
         (
             "http://courses.mit.edu",
-            {"NO_PROXY": "mit.edu"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu"},
             True,
         ),  # no_proxy for sub-domain matches
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu"},
             False,
         ),  # domain is actually edu.info, so should be proxied
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu,edu.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu,edu.info"},
             True,
         ),  # list in no_proxy, matches second domain
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu, edu.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu, edu.info"},
             True,
         ),  # list with spaces in no_proxy
         (
             "https://mit.edu.info",
-            {"NO_PROXY": "mit.edu,mit.info"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "mit.edu,mit.info"},
             False,
         ),  # list in no_proxy, without any domain matching
         (
             "https://foo.example.com",
-            {"NO_PROXY": "www.example.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "www.example.com"},
             False,
         ),  # different subdomains foo vs www means we still proxy
         (
             "https://www.example1.com",
-            {"NO_PROXY": ".example1.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": ".example1.com"},
             True,
         ),  # no_proxy starting with dot
         (
             "https://www.example2.com",
-            {"NO_PROXY": "ample2.com"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "ample2.com"},
             False,
         ),  # whole-domain matching
         (
             "https://www.example3.com",
-            {"NO_PROXY": "*"},
+            {"ALL_PROXY": "http://localhost:123", "NO_PROXY": "*"},
             True,
         ),  # wildcard * means nothing proxied
     ],
 )
 def test_should_not_be_proxied(url, no_proxy, expected):
path: tests/test_utils.py
text: @user1:
Should we move this test to `test_proxies.py` now that the `should_not_be_proxied` util is gone?

@author:
Good call! 😀

@author:
I've merged these tests into the existing `test_proxies_environ` case now.
Looks much nicer!