Repository: seanmonstar/reqwest (language: rust)

Issue:
"System" proxies should be used by default
Hi,
currently proxies need to be manually added using a client builder. I strongly disagree with https://github.com/seanmonstar/reqwest/issues/30#issuecomment-271714181 that reqwest should not handle the system proxies automatically. I think it should use them by default, and for the rare where proxies should not be used, a builder can be used to disable them.

Think of the thousands of command line utilities that do http requests that were only tested in environnements that work without proxies (my last example being https://github.com/gobanos/cargo-aoc/blob/54e0a80a9bd7dc339c7d2c9dbbf43076d9112e3a/src/app.rs#L66 ). Do they all need to add code to explicitly say they want to use the user's configured proxy ?

As mentionned in https://github.com/seanmonstar/reqwest/issues/30#issuecomment-266336609 the crate env_proxy https://crates.io/crates/env_proxy can be used for basic support.

Ideally, more system methods would be recognized (java has support for the windows registry and for gnome gconf : see https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/windows/native/sun/net/spi/DefaultProxySelector.c  and https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/solaris/native/sun/net/spi/DefaultProxySelector.c )

Cheers,
Jon


Discussion / hints:
I'm sympathetic to the wish that it is set by default. I didn't have a strong opinion either way, and I can be easily convinced that it *should* check by default. Or, perhaps just offer an option on the `ClientBuilder`.
+1 from me also. For example the most popular Python http client library [does support system proxy by default](http://docs.python-requests.org/en/master/user/advanced/#proxies). What is the downside of supporting proxy automatically?
Yea, I'm fine with doing this. Here's what I'd recommend we do:

- Use something like the [env_proxy](https://crates.io/crates/env_proxy) crate. This can be combined with `reqwest::Proxy::custom`.
- Check if the appropriate environment variables are set at all. This is needed so that we don't create a "custom" proxy if none of the variables are set.
- Add a `ClientBuilder` option to disable checking the environment.

How does that sound?
Yeah that sounds great. From my experience most developers using the higher level library like this one expect things to just work. So that should be default. In rare cases you want to have way to ignore the system proxy, so the override needs to be there.

From user standpoint working in a network where we are restricted to proxy connections, I often find that the tools don't work, because most developers don't develop in such environment. So supporting it by default will make this crate and any projects using it way more useful for users like me.

I'm not sure if you need to check the proxy env variables. Maybe check with [`for_url`](https://inejge.github.io/env_proxy/env_proxy/fn.for_url.html) the specific URL if that's an option? Proxy might be required for some, but not for some other depending on `no_proxy` settings. (This is often the case in proxied networks, where internal resources are accessed directly while external might be only accessed via proxy) 
[`for_url`](https://inejge.github.io/env_proxy/env_proxy/fn.for_url.html) is the way to go imo. `no_proxy` is a must for users behind corporate networks. ~~On that note, is there a way right now to disable proxy for `reqwest::Client`? Something along the lines of `ClientBuilder::new().proxy(None)`?~~ `ClientBuilder::new().proxy(Proxy::custom(|_| None))` works.


Fix patch:
diff --git a/.travis.yml b/.travis.yml
index 1a61b792c..ad997bad3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -102,4 +102,4 @@ env:
     - RUST_BACKTRACE=1
 script:
   - cargo build $FEATURES
-  - cargo test -v $FEATURES -- --test-threads=1
+  - cargo test -v $FEATURES --features __internal_proxy_sys_no_cache -- --test-threads=1
diff --git a/Cargo.toml b/Cargo.toml
index e0361a46b..b1aecc6d1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -37,6 +37,12 @@ json = ["serde_json"]
 
 unstable-stream = []
 
+# Internal (PRIVATE!) features used to aid testing.
+# Don't rely on these whatsoever. They may disappear at anytime.
+
+# When enabled, disable using the cached SYS_PROXIES.
+__internal_proxy_sys_no_cache = []
+
 [dependencies]
 http = "0.1.15"
 url = "2.1"
@@ -49,6 +55,7 @@ futures-core-preview = { version = "=0.3.0-alpha.19" }
 futures-util-preview = { version = "=0.3.0-alpha.19" }
 http-body = "=0.2.0-alpha.3"
 hyper = { version = "=0.13.0-alpha.4", default-features = false, features = ["tcp"] }
+lazy_static = "1.4"
 log = "0.4"
 mime = "0.3.7"
 mime_guess = "2.0"
@@ -150,4 +157,3 @@ required-features = ["cookies"]
 name = "gzip"
 path = "tests/gzip.rs"
 required-features = ["gzip"]
-
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
index 6b7881b08..383b3b39e 100644
--- a/src/async_impl/client.rs
+++ b/src/async_impl/client.rs
@@ -28,7 +28,6 @@ use crate::connect::Connector;
 #[cfg(feature = "cookies")]
 use crate::cookie;
 use crate::into_url::{expect_uri, try_uri};
-use crate::proxy::get_proxies;
 use crate::redirect::{self, remove_sensitive_headers, RedirectPolicy};
 #[cfg(feature = "tls")]
 use crate::tls::TlsBackend;
@@ -69,6 +68,7 @@ struct Config {
     #[cfg(feature = "tls")]
     identity: Option<Identity>,
     proxies: Vec<Proxy>,
+    auto_sys_proxy: bool,
     redirect_policy: RedirectPolicy,
     referer: bool,
     timeout: Option<Duration>,
@@ -104,6 +104,7 @@ impl ClientBuilder {
                 connect_timeout: None,
                 max_idle_per_host: std::usize::MAX,
                 proxies: Vec::new(),
+                auto_sys_proxy: true,
                 redirect_policy: RedirectPolicy::default(),
                 referer: true,
                 timeout: None,
@@ -131,7 +132,11 @@ impl ClientBuilder {
     /// cannot load the system configuration.
     pub fn build(self) -> crate::Result<Client> {
         let config = self.config;
-        let proxies = Arc::new(config.proxies);
+        let mut proxies = config.proxies;
+        if config.auto_sys_proxy {
+            proxies.push(Proxy::system());
+        }
+        let proxies = Arc::new(proxies);
 
         let mut connector = {
             #[cfg(feature = "tls")]
@@ -365,27 +370,28 @@ impl ClientBuilder {
     // Proxy options
 
     /// Add a `Proxy` to the list of proxies the `Client` will use.
+    ///
+    /// # Note
+    ///
+    /// Adding a proxy will disable the automatic usage of the "system" proxy.
     pub fn proxy(mut self, proxy: Proxy) -> ClientBuilder {
         self.config.proxies.push(proxy);
+        self.config.auto_sys_proxy = false;
         self
     }
 
     /// Clear all `Proxies`, so `Client` will use no proxy anymore.
+    ///
+    /// This also disables the automatic usage of the "system" proxy.
     pub fn no_proxy(mut self) -> ClientBuilder {
         self.config.proxies.clear();
+        self.config.auto_sys_proxy = false;
         self
     }
 
-    /// Add system proxy setting to the list of proxies
-    pub fn use_sys_proxy(mut self) -> ClientBuilder {
-        let proxies = get_proxies();
-        self.config.proxies.push(Proxy::custom(move |url| {
-            if proxies.contains_key(url.scheme()) {
-                Some((*proxies.get(url.scheme()).unwrap()).clone())
-            } else {
-                None
-            }
-        }));
+    #[doc(hidden)]
+    #[deprecated(note = "the system proxy is used automatically")]
+    pub fn use_sys_proxy(self) -> ClientBuilder {
         self
     }
 
@@ -776,7 +782,7 @@ impl Client {
 
 impl fmt::Debug for Client {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let mut builder = f.debug_struct("ClientBuilder");
+        let mut builder = f.debug_struct("Client");
         self.inner.fmt_fields(&mut builder);
         builder.finish()
     }
diff --git a/src/blocking/client.rs b/src/blocking/client.rs
index eb70f4dce..7d1b14dd3 100644
--- a/src/blocking/client.rs
+++ b/src/blocking/client.rs
@@ -193,18 +193,25 @@ impl ClientBuilder {
     // Proxy options
 
     /// Add a `Proxy` to the list of proxies the `Client` will use.
+    ///
+    /// # Note
+    ///
+    /// Adding a proxy will disable the automatic usage of the "system" proxy.
     pub fn proxy(self, proxy: Proxy) -> ClientBuilder {
         self.with_inner(move |inner| inner.proxy(proxy))
     }
 
-    /// Disable proxy setting.
+    /// Clear all `Proxies`, so `Client` will use no proxy anymore.
+    ///
+    /// This also disables the automatic usage of the "system" proxy.
     pub fn no_proxy(self) -> ClientBuilder {
         self.with_inner(move |inner| inner.no_proxy())
     }
 
-    /// Enable system proxy setting.
+    #[doc(hidden)]
+    #[deprecated(note = "the system proxy is used automatically")]
     pub fn use_sys_proxy(self) -> ClientBuilder {
-        self.with_inner(move |inner| inner.use_sys_proxy())
+        self
     }
 
     // Timeout options
diff --git a/src/lib.rs b/src/lib.rs
index 40843c9a5..bbbd1824f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -263,6 +263,9 @@ if_hyper! {
     #[macro_use]
     extern crate doc_comment;
 
+    #[macro_use]
+    extern crate lazy_static;
+
     #[cfg(test)]
     doctest!("../README.md");
 
diff --git a/src/proxy.rs b/src/proxy.rs
index 0334a86e9..407677a25 100644
--- a/src/proxy.rs
+++ b/src/proxy.rs
@@ -180,11 +180,13 @@ impl Proxy {
         }))
     }
 
-    /*
-    pub fn unix<P: AsRef<Path>(path: P) -> Proxy {
-
+    pub(crate) fn system() -> Proxy {
+        if cfg!(feature = "__internal_proxy_sys_no_cache") {
+            Proxy::new(Intercept::System(Arc::new(get_sys_proxies())))
+        } else {
+            Proxy::new(Intercept::System(SYS_PROXIES.clone()))
+        }
     }
-    */
 
     fn new(intercept: Intercept) -> Proxy {
         Proxy { intercept }
@@ -248,6 +250,9 @@ impl Proxy {
                     None
                 }
             }
+            Intercept::System(ref map) => {
+                map.get(uri.scheme()).cloned()
+            }
             Intercept::Custom(ref custom) => custom.call(uri),
         }
     }
@@ -257,6 +262,7 @@ impl Proxy {
             Intercept::All(_) => true,
             Intercept::Http(_) => uri.scheme() == "http",
             Intercept::Https(_) => uri.scheme() == "https",
+            Intercept::System(ref map) => map.contains_key(uri.scheme()),
             Intercept::Custom(ref custom) => custom.call(uri).is_some(),
         }
     }
@@ -369,11 +375,14 @@ impl ProxyScheme {
     }
 }
 
+type SystemProxyMap = HashMap<String, ProxyScheme>;
+
 #[derive(Clone, Debug)]
 enum Intercept {
     All(ProxyScheme),
     Http(ProxyScheme),
     Https(ProxyScheme),
+    System(Arc<SystemProxyMap>),
     Custom(Custom),
 }
 
@@ -383,6 +392,7 @@ impl Intercept {
             Intercept::All(ref mut s)
             | Intercept::Http(ref mut s)
             | Intercept::Https(ref mut s) => s.set_basic_auth(username, password),
+            Intercept::System(_) => unimplemented!(),
             Intercept::Custom(ref mut custom) => {
                 let header = encode_basic_auth(username, password);
                 custom.auth = Some(header);
@@ -484,6 +494,10 @@ impl Dst for Uri {
     }
 }
 
+lazy_static! {
+    static ref SYS_PROXIES: Arc<SystemProxyMap> = Arc::new(get_sys_proxies());
+}
+
 /// Get system proxies information.
 ///
 /// It can only support Linux, Unix like, and windows system.  Note that it will always
@@ -493,8 +507,8 @@ impl Dst for Uri {
 /// Returns:
 ///     System proxies information as a hashmap like
 ///     {"http": Url::parse("http://127.0.0.1:80"), "https": Url::parse("https://127.0.0.1:80")}
-pub fn get_proxies() -> HashMap<String, Url> {
-    let proxies: HashMap<String, Url> = get_from_environment();
+fn get_sys_proxies() -> SystemProxyMap {
+    let proxies = get_from_environment();
 
     // TODO: move the following #[cfg] to `if expression` when attributes on `if` expressions allowed
     #[cfg(target_os = "windows")]
@@ -507,14 +521,14 @@ pub fn get_proxies() -> HashMap<String, Url> {
     proxies
 }
 
-fn insert_proxy(proxies: &mut HashMap<String, Url>, schema: String, addr: String) {
-    if let Ok(valid_addr) = Url::parse(&addr) {
+fn insert_proxy(proxies: &mut SystemProxyMap, schema: String, addr: String) {
+    if let Ok(valid_addr) = addr.into_proxy_scheme() {
         proxies.insert(schema, valid_addr);
     }
 }
 
-fn get_from_environment() -> HashMap<String, Url> {
-    let mut proxies: HashMap<String, Url> = HashMap::new();
+fn get_from_environment() -> SystemProxyMap {
+    let mut proxies = HashMap::new();
 
     const PROXY_KEY_ENDS: &str = "_proxy";
 
@@ -530,7 +544,7 @@ fn get_from_environment() -> HashMap<String, Url> {
 }
 
 #[cfg(target_os = "windows")]
-fn get_from_registry_impl() -> Result<HashMap<String, Url>, Box<dyn Error>> {
+fn get_from_registry_impl() -> Result<SystemProxyMap, Box<dyn Error>> {
     let hkcu = RegKey::predef(HKEY_CURRENT_USER);
     let internet_setting: RegKey =
         hkcu.open_subkey("Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings")?;
@@ -542,7 +556,7 @@ fn get_from_registry_impl() -> Result<HashMap<String, Url>, Box<dyn Error>> {
         return Ok(HashMap::new());
     }
 
-    let mut proxies: HashMap<String, Url> = HashMap::new();
+    let mut proxies = HashMap::new();
     if proxy_server.contains("=") {
         // per-protocol settings.
         for p in proxy_server.split(";") {
@@ -589,7 +603,7 @@ fn get_from_registry_impl() -> Result<HashMap<String, Url>, Box<dyn Error>> {
 }
 
 #[cfg(target_os = "windows")]
-fn get_from_registry() -> HashMap<String, Url> {
+fn get_from_registry() -> SystemProxyMap {
     get_from_registry_impl().unwrap_or(HashMap::new())
 }
 
@@ -685,24 +699,30 @@ mod tests {
     }
 
     #[test]
-    fn test_get_proxies() {
+    fn test_get_sys_proxies() {
         // save system setting first.
         let system_proxy = env::var("http_proxy");
 
         // remove proxy.
         env::remove_var("http_proxy");
-        assert_eq!(get_proxies().contains_key("http"), false);
+        assert_eq!(get_sys_proxies().contains_key("http"), false);
 
         // the system proxy setting url is invalid.
         env::set_var("http_proxy", "123465");
-        assert_eq!(get_proxies().contains_key("http"), false);
+        assert_eq!(get_sys_proxies().contains_key("http"), false);
 
         // set valid proxy
         env::set_var("http_proxy", "http://127.0.0.1/");
-        let proxies = get_proxies();
-        let http_target = proxies.get("http").unwrap().as_str();
+        let proxies = get_sys_proxies();
+
+        match &proxies["http"] {
+            ProxyScheme::Http { uri, .. } => {
+                assert_eq!(uri, "http://127.0.0.1/");
+            },
+            #[cfg(feature = "socks")]
+            _ => panic!("socks not expected"),
+        }
 
-        assert_eq!(http_target, "http://127.0.0.1/");
         // reset user setting.
         match system_proxy {
             Err(_) => env::remove_var("http_proxy"),


Test patch:
diff --git a/tests/proxy.rs b/tests/proxy.rs
index d269200fd..11cab33ab 100644
--- a/tests/proxy.rs
+++ b/tests/proxy.rs
@@ -119,30 +119,28 @@ async fn test_no_proxy() {
     assert_eq!(res.status(), reqwest::StatusCode::OK);
 }
 
+#[cfg_attr(not(feature = "__internal_proxy_sys_no_cache"), ignore)]
 #[tokio::test]
 async fn test_using_system_proxy() {
-    let url = "http://hyper.rs/prox";
+    let url = "http://not.a.real.sub.hyper.rs/prox";
     let server = server::http(move |req| {
         assert_eq!(req.method(), "GET");
         assert_eq!(req.uri(), url);
-        assert_eq!(req.headers()["host"], "hyper.rs");
+        assert_eq!(req.headers()["host"], "not.a.real.sub.hyper.rs");
 
         async { http::Response::default() }
     });
 
+    // Note: we're relying on the `__internal_proxy_sys_no_cache` feature to
+    // check the environment every time.
+
     // save system setting first.
     let system_proxy = env::var("http_proxy");
     // set-up http proxy.
     env::set_var("http_proxy", format!("http://{}", server.addr()));
 
-    let res = reqwest::Client::builder()
-        .use_sys_proxy()
-        .build()
-        .unwrap()
-        .get(url)
-        .send()
-        .await
-        .unwrap();
+    // system proxy is used by default
+    let res = reqwest::get(url).await.unwrap();
 
     assert_eq!(res.url().as_str(), url);
     assert_eq!(res.status(), reqwest::StatusCode::OK);