Fix detection of system proxy from Windows registry (#1005)

src/proxy.rs

@@ -212,7 +212,7 @@ impl Proxy {
 
     pub(crate) fn system() -> Proxy {
         let mut proxy = if cfg!(feature = "__internal_proxy_sys_no_cache") {
-            Proxy::new(Intercept::System(Arc::new(get_sys_proxies())))
+            Proxy::new(Intercept::System(Arc::new(get_sys_proxies(get_from_registry()))))
         } else {
             Proxy::new(Intercept::System(SYS_PROXIES.clone()))
         };
@@ -578,6 +578,7 @@ impl fmt::Debug for ProxyScheme {
 }
 
 type SystemProxyMap = HashMap<String, ProxyScheme>;
+type RegistryProxyValues = (u32, String);
 
 #[derive(Clone, Debug)]
 enum Intercept {
@@ -667,7 +668,7 @@ impl Dst for Uri {
 }
 
 lazy_static! {
-    static ref SYS_PROXIES: Arc<SystemProxyMap> = Arc::new(get_sys_proxies());
+    static ref SYS_PROXIES: Arc<SystemProxyMap> = Arc::new(get_sys_proxies(get_from_registry()));
 }
 
 /// Get system proxies information.
@@ -679,7 +680,9 @@ lazy_static! {
 /// 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")}
-fn get_sys_proxies() -> SystemProxyMap {
+fn get_sys_proxies(
+    #[cfg_attr(not(target_os = "windows"), allow(unused_variables))] registry_values: Option<RegistryProxyValues>,
+) -> SystemProxyMap {
     let proxies = get_from_environment();
 
     // TODO: move the following #[cfg] to `if expression` when attributes on `if` expressions allowed
@@ -687,7 +690,9 @@ fn get_sys_proxies() -> SystemProxyMap {
     {
         if proxies.is_empty() {
             // don't care errors if can't get proxies from registry, just return an empty HashMap.
-            return get_from_registry();
+            if let Some(registry_values) = registry_values {
+                return parse_registry_values(registry_values);
+            }
         }
     }
     proxies
@@ -737,7 +742,7 @@ fn is_cgi() -> bool {
 }
 
 #[cfg(target_os = "windows")]
-fn get_from_registry_impl() -> Result<SystemProxyMap, Box<dyn Error>> {
+fn get_from_registry_impl() -> Result<RegistryProxyValues, Box<dyn Error>> {
     let hkcu = RegKey::predef(HKEY_CURRENT_USER);
     let internet_setting: RegKey =
         hkcu.open_subkey("Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings")?;
@@ -745,6 +750,23 @@ fn get_from_registry_impl() -> Result<SystemProxyMap, Box<dyn Error>> {
     let proxy_enable: u32 = internet_setting.get_value("ProxyEnable")?;
     let proxy_server: String = internet_setting.get_value("ProxyServer")?;
 
+    Ok((proxy_enable, proxy_server))
+}
+
+#[cfg(target_os = "windows")]
+fn get_from_registry() -> Option<RegistryProxyValues> {
+    get_from_registry_impl().ok()
+}
+
+#[cfg(not(target_os = "windows"))]
+fn get_from_registry() -> Option<RegistryProxyValues> {
+    None
+}
+
+#[cfg(target_os = "windows")]
+fn parse_registry_values_impl(registry_values: RegistryProxyValues) -> Result<SystemProxyMap, Box<dyn Error>> {
+    let (proxy_enable, proxy_server) = registry_values;
+
     if proxy_enable == 0 {
         return Ok(HashMap::new());
     }
@@ -756,7 +778,15 @@ fn get_from_registry_impl() -> Result<SystemProxyMap, Box<dyn Error>> {
             let protocol_parts: Vec<&str> = p.split("=").collect();
             match protocol_parts.as_slice() {
                 [protocol, address] => {
-                    insert_proxy(&mut proxies, *protocol, String::from(*address));
+                    // See if address has a type:// prefix
+                    let address = if !contains_type_prefix(*address) {
+                        format!("{}://{}", protocol, address)
+                    }
+                    else {
+                        String::from(*address)
+                    };
+
+                    insert_proxy(&mut proxies, *protocol, address);
                 }
                 _ => {
                     // Contains invalid protocol setting, just break the loop
@@ -779,8 +809,26 @@ fn get_from_registry_impl() -> Result<SystemProxyMap, Box<dyn Error>> {
 }
 
 #[cfg(target_os = "windows")]
-fn get_from_registry() -> SystemProxyMap {
+fn contains_type_prefix(address: &str) -> bool {
-    get_from_registry_impl().unwrap_or(HashMap::new())
+    if let Some(indice) = address.find("://") {
+        if indice == 0 {
+            false
+        }
+        else {
+            let prefix = &address[..indice];
+            let contains_banned = prefix.contains(|c| c == ':' || c == '/');
+
+            !contains_banned
+        }
+    }
+    else {
+        false
+    }
+}
+
+#[cfg(target_os = "windows")]
+fn parse_registry_values(registry_values: RegistryProxyValues) -> SystemProxyMap {
+    parse_registry_values_impl(registry_values).unwrap_or(HashMap::new())
 }
 
 #[cfg(test)]
@@ -913,13 +961,13 @@ mod tests {
 
         // Mock ENV, get the results, before doing assertions
         // to avoid assert! -> panic! -> Mutex Poisoned.
-        let baseline_proxies = get_sys_proxies();
+        let baseline_proxies = get_sys_proxies(None);
         // the system proxy setting url is invalid.
         env::set_var("http_proxy", "123465");
-        let invalid_proxies = get_sys_proxies();
+        let invalid_proxies = get_sys_proxies(None);
         // set valid proxy
         env::set_var("http_proxy", "http://127.0.0.1/");
-        let valid_proxies = get_sys_proxies();
+        let valid_proxies = get_sys_proxies(None);
 
         // reset user setting when guards drop
         drop(_g1);
@@ -935,6 +983,46 @@ mod tests {
         assert_eq!(p.host(), "127.0.0.1");
     }
 
+    #[cfg(target_os = "windows")]
+    #[test]
+    fn test_get_sys_proxies_registry_parsing() {
+        // Stop other threads from modifying process-global ENV while we are.
+        let _lock = ENVLOCK.lock();
+        // save system setting first.
+        let _g1 = env_guard("HTTP_PROXY");
+        let _g2 = env_guard("http_proxy");
+
+        // Mock ENV, get the results, before doing assertions
+        // to avoid assert! -> panic! -> Mutex Poisoned.
+        let baseline_proxies = get_sys_proxies(None);
+        // the system proxy in the registry has been disabled
+        let disabled_proxies = get_sys_proxies(Some((0, String::from("http://127.0.0.1/"))));
+        // set valid proxy
+        let valid_proxies = get_sys_proxies(Some((1, String::from("http://127.0.0.1/"))));
+        let multiple_proxies = get_sys_proxies(Some((1, String::from("http=127.0.0.1:8888;https=127.0.0.2:8888"))));
+
+        // reset user setting when guards drop
+        drop(_g1);
+        drop(_g2);
+        // Let other threads run now
+        drop(_lock);
+
+        assert_eq!(baseline_proxies.contains_key("http"), false);
+        assert_eq!(disabled_proxies.contains_key("http"), false);
+
+        let p = &valid_proxies["http"];
+        assert_eq!(p.scheme(), "http");
+        assert_eq!(p.host(), "127.0.0.1");
+
+        let p = &multiple_proxies["http"];
+        assert_eq!(p.scheme(), "http");
+        assert_eq!(p.host(), "127.0.0.1:8888");
+
+        let p = &multiple_proxies["https"];
+        assert_eq!(p.scheme(), "https");
+        assert_eq!(p.host(), "127.0.0.2:8888");
+    }
+
     #[test]
     fn test_get_sys_proxies_in_cgi() {
         // Stop other threads from modifying process-global ENV while we are.
@@ -947,11 +1035,11 @@ mod tests {
         // to avoid assert! -> panic! -> Mutex Poisoned.
         env::set_var("HTTP_PROXY", "http://evil/");
 
-        let baseline_proxies = get_sys_proxies();
+        let baseline_proxies = get_sys_proxies(None);
         // set like we're in CGI
         env::set_var("REQUEST_METHOD", "GET");
 
-        let cgi_proxies = get_sys_proxies();
+        let cgi_proxies = get_sys_proxies(None);
 
         // reset user setting when guards drop
         drop(_g1);
@@ -982,7 +1070,7 @@ mod tests {
         );
 
         // Manually construct this so we aren't use the cache
-        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies())));
+        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies(None))));
         p.no_proxy = NoProxy::new();
 
         assert_eq!(intercepted_uri(&p, "http://hyper.rs"), target);
@@ -1015,7 +1103,7 @@ mod tests {
         let domain = "lower.case";
         env::set_var("no_proxy", domain);
         // Manually construct this so we aren't use the cache
-        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies())));
+        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies(None))));
         p.no_proxy = NoProxy::new();
         assert_eq!(
             p.no_proxy.expect("should have a no proxy set").domains.0[0],
@@ -1027,7 +1115,7 @@ mod tests {
         let domain = "upper.case";
         env::set_var("NO_PROXY", domain);
         // Manually construct this so we aren't use the cache
-        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies())));
+        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies(None))));
         p.no_proxy = NoProxy::new();
         assert_eq!(
             p.no_proxy.expect("should have a no proxy set").domains.0[0],
@@ -1041,7 +1129,7 @@ mod tests {
         env::set_var("HTTP_PROXY", target);
 
         // Manually construct this so we aren't use the cache
-        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies())));
+        let mut p = Proxy::new(Intercept::System(Arc::new(get_sys_proxies(None))));
         p.no_proxy = NoProxy::new();
         assert!(p.no_proxy.is_none(), "NoProxy shouldn't have been created");
 
@@ -1055,6 +1143,18 @@ mod tests {
         drop(_lock);
     }
 
+    #[cfg(target_os = "windows")]
+    #[test]
+    fn test_type_prefix_detection() {
+        assert!(!contains_type_prefix("test"));
+        assert!(!contains_type_prefix("://test"));
+        assert!(!contains_type_prefix("some:prefix://test"));
+        assert!(!contains_type_prefix("some/prefix://test"));
+
+        assert!(contains_type_prefix("http://test"));
+        assert!(contains_type_prefix("a://test"));
+    }
+
     /// Guard an environment variable, resetting it to the original value
     /// when dropped.
     fn env_guard(name: impl Into<String>) -> EnvGuard {

tests/badssl.rs

@@ -1,7 +1,12 @@
 #[cfg(feature = "__tls")]
 #[tokio::test]
 async fn test_badssl_modern() {
-    let text = reqwest::get("https://mozilla-modern.badssl.com/")
+    let text = reqwest::Client::builder()
+        .no_proxy()
+        .build()
+        .unwrap()
+        .get("https://mozilla-modern.badssl.com/")
+        .send()
         .await
         .unwrap()
         .text()
@@ -16,6 +21,7 @@ async fn test_badssl_modern() {
 async fn test_rustls_badssl_modern() {
     let text = reqwest::Client::builder()
         .use_rustls_tls()
+        .no_proxy()
         .build()
         .unwrap()
         .get("https://mozilla-modern.badssl.com/")
@@ -34,6 +40,7 @@ async fn test_rustls_badssl_modern() {
 async fn test_badssl_self_signed() {
     let text = reqwest::Client::builder()
         .danger_accept_invalid_certs(true)
+        .no_proxy()
         .build()
         .unwrap()
         .get("https://self-signed.badssl.com/")
@@ -52,6 +59,7 @@ async fn test_badssl_self_signed() {
 async fn test_badssl_wrong_host() {
     let text = reqwest::Client::builder()
         .danger_accept_invalid_hostnames(true)
+        .no_proxy()
         .build()
         .unwrap()
         .get("https://wrong.host.badssl.com/")

tests/client.rs

@@ -28,7 +28,14 @@ async fn auto_headers() {
     });
 
     let url = format!("http://{}/1", server.addr());
-    let res = reqwest::get(&url).await.unwrap();
+    let res = reqwest::Client::builder()
+        .no_proxy()
+        .build()
+        .unwrap()
+        .get(&url)
+        .send()
+        .await
+        .unwrap();
 
     assert_eq!(res.url().as_str(), &url);
     assert_eq!(res.status(), reqwest::StatusCode::OK);

tests/timeouts.rs

@@ -76,6 +76,7 @@ async fn response_timeout() {
 
     let client = reqwest::Client::builder()
         .timeout(Duration::from_millis(500))
+        .no_proxy()
         .build()
         .unwrap();