Rework proxy selection (getsentry#309)

crepererum · untitaker · commit 78d17d1847f8 · 2019-04-25T14:35:30.000+02:00
* split and extend proxy tests * fix HTTP fallback for proxy env vars Fixes getsentry#308 * Fine-grained control over proxy selection. Fixes getsentry#178
diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py
@@ -156,10 +156,15 @@ def _make_pool(
         ca_certs,  # type: Optional[Any]
     ):
         # type: (...) -> Union[PoolManager, ProxyManager]
-        # Use http_proxy if scheme is https and https_proxy is not set
-        proxy = parsed_dsn.scheme == "https" and https_proxy or http_proxy
-        if not proxy:
-            proxy = getproxies().get(parsed_dsn.scheme)
+        proxy = None
+
+        # try HTTPS first
+        if parsed_dsn.scheme == "https" and (https_proxy != ""):
+            proxy = https_proxy or getproxies().get("https")
+
+        # maybe fallback to HTTP proxy
+        if not proxy and (http_proxy != ""):
+            proxy = http_proxy or getproxies().get("http")
 
         opts = self._get_pool_options(ca_certs)
 
diff --git a/tests/test_client.py b/tests/test_client.py
@@ -1,6 +1,7 @@
 # coding: utf-8
 import json
 import logging
+import os
 import pytest
 import subprocess
 import sys
@@ -52,25 +53,106 @@ def test_transport_option(monkeypatch):
     assert str(Client(transport=transport).dsn) == dsn
 
 
-def test_http_proxy(monkeypatch):
-    client = Client("https://foo@sentry.io/123", http_proxy="http://localhost/123")
+def test_proxy_http_use(monkeypatch):
+    client = Client("http://foo@sentry.io/123", http_proxy="http://localhost/123")
     assert client.transport._pool.proxy.scheme == "http"
 
+
+def test_proxy_https_use(monkeypatch):
+    client = Client("https://foo@sentry.io/123", http_proxy="https://localhost/123")
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_both_select_http(monkeypatch):
     client = Client(
-        "https://foo@sentry.io/123",
+        "http://foo@sentry.io/123",
         https_proxy="https://localhost/123",
         http_proxy="http://localhost/123",
     )
-    assert client.transport._pool.proxy.scheme == "https"
-
-    client = Client("http://foo@sentry.io/123", http_proxy="http://localhost/123")
     assert client.transport._pool.proxy.scheme == "http"
 
+
+def test_proxy_both_select_https(monkeypatch):
     client = Client(
-        "http://foo@sentry.io/123",
+        "https://foo@sentry.io/123",
         https_proxy="https://localhost/123",
         http_proxy="http://localhost/123",
     )
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_http_fallback_http(monkeypatch):
+    client = Client("https://foo@sentry.io/123", http_proxy="http://localhost/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_none_noenv(monkeypatch):
+    client = Client("http://foo@sentry.io/123")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_none_httpenv_select(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("http://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_none_httpsenv_select(monkeypatch):
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_none_httpenv_fallback(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("https://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_bothselect_bothen(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy="", https_proxy="")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_bothavoid_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_bothselect_httpenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_httpselect_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy="")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_httpsselect_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy="", https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_httpselect_httpsenv(monkeypatch):
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy="")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_httpsselect_bothenv_http(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("http://foo@sentry.io/123", http_proxy=None, https_proxy=None)
     assert client.transport._pool.proxy.scheme == "http"
 
 
