Repository: seanmonstar/reqwest (language: rust)

Issue:
HTTPS System proxies are not interpreted correctly on Windows
@seanmonstar First of all, thank you for the great library and your work!

I noticed a bug in the Windows system HTTPS proxy handling, in particular:
* After https://github.com/seanmonstar/reqwest/pull/1005 multiple proxies are detected correctly on Windows, but requests to HTTPS domains cause the client to hang, and then fail with `The token supplied to the function is invalid`, which is the same problem reported in https://github.com/seanmonstar/reqwest/issues/416
* After a bit of digging, I found the root cause of the problem. In particular, when loading the proxy settings from the registry, Reqwest makes the assumption that an HTTPS proxy should go through the HTTPS protocol, which is not the case. In general, HTTPS requests are sent to the proxy using the HTTP protocol, and then the proxy will tunnel them as TCP packets.

When setting up a test proxy, (using Fiddler), the Windows registry entry will contain the value:

```
http=127.0.0.1:8888;https=127.0.0.1:8888
```

and Reqwest will interpret those as:

```
HTTP_PROXY=http://127.0.0.1:8888
HTTPS_PROXY=https://127.0.0.1:8888
```

Unfortunately, this is most likely incorrect, as the HTTPS proxy should use the http protocol as well:

```
HTTP_PROXY=http://127.0.0.1:8888
HTTPS_PROXY=http://127.0.0.1:8888
```

I consider it a bug and not expected behavior as most other http libraries (of other languages, such as Python) and webbrowsers are handling the proxy correctly, using the HTTP protocol.

### How to reproduce

1. Download and start [Fiddler](https://www.telerik.com/fiddler)
2. Fiddler will automatically setup a proxy at 127.0.0.1:8888, including this entry in the Windows registry: `http=127.0.0.1:8888;https=127.0.0.1:8888`
3. The following code will hang and then fail with error: `Error: reqwest::Error { kind: Request, url: Url { scheme: "https", host: Some(Domain("httpbin.org")), port: None, path: "/ip", query: None, fragment: None }, source: hyper::Error(Connect, Os { code: -2146893048, kind: 
Other, message: "The token supplied to the function is invalid" }) }`

```rust
use tokio::*;
use std::collections::HashMap;

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let client = reqwest::Client::builder()
    //.proxy(reqwest::Proxy::https("http://127.0.0.1:8888")?)
    .build()?;
    let resp = client.get("https://httpbin.org/ip").send().await?
        .json::<HashMap<String, String>>()
        .await?;
    println!("{:#?}", resp);
    Ok(())
}
```

Forcing the HTTPS proxy to go though http by uncommenting the line will fix the error and behave as expected.


Discussion / hints:
Thanks for the great write-up! If it's handling things differently than most other tools do, then we should fix it! Would you like to submit a PR doing so? Also, I'm curious, how does a user on Windows normally indicate that they should use HTTPS to talk directly to the proxy?
@seanmonstar Thank you! I'll open a PR in a few days, as soon as I get some time :)

> Also, I'm curious, how does a user on Windows normally indicate that they should use HTTPS to talk directly to the proxy?

As far as I could tell (by studying other tools/libraries and reading articles), it's very unlikely for an HTTPS proxy to actually use HTTPS between the client and the proxy (for example, see: https://stackoverflow.com/questions/516323/https-connections-over-proxy-servers). Instead, HTTP proxies would handle HTTPS by converting it to a TCP/IP tunnel, which is completely blind about the HTTPS data being transmitted (as it's encrypted).

I experimented with Python, and by specifying HTTPS proxy + HTTPS protocol it works, but I'm reasonably sure that under the hoods it's communicating with the Proxy using the HTTP protocol.
Another example would be `curl`. For instance, using the same proxy configuration as the previous ticket, we get that:

```
# This hangs and then fail
https_proxy=https://127.0.0.1:8888 curl -I https://google.com

# This works correctly
https_proxy=http://127.0.0.1:8888 curl -I https://google.com
```

Because in some limited cases a user might still want to use HTTPS to communicate with the proxy, what about defaulting the protocol to HTTP if not explicitly specified?

For example, the following Registry entry:
```
http=127.0.0.1:8888;https=127.0.0.1:8888
```
would get interpreted as:
```
HTTP_PROXY=http://127.0.0.1:8888
HTTPS_PROXY=http://127.0.0.1:8888
```
But the user could specify the HTTPS protocol explicitly:
```
http=http://127.0.0.1:8888;https=https://127.0.0.1:8888
```
which could be translated to:
```
HTTP_PROXY=http://127.0.0.1:8888
HTTPS_PROXY=https://127.0.0.1:8888
```

This way, we provide a good default without limiting the user. What do you think?


> But the user could specify the HTTPS protocol explicitly:
>
> ```
> http=http://127.0.0.1:8888;https=https://127.0.0.1:8888
> ```

Yea, that makes sense. I found this [Windows documentation that seems to suggest that](https://docs.microsoft.com/en-us/windows/win32/api/winhttp/ns-winhttp-winhttp_proxy_info), but it doesn't explicitly say what should be assumed if the scheme is missing... But sounds good to me!

Fix patch:
diff --git a/src/proxy.rs b/src/proxy.rs
index 9cc94d889..e3feb2ad2 100644
--- a/src/proxy.rs
+++ b/src/proxy.rs
@@ -778,12 +778,12 @@ fn parse_registry_values_impl(registry_values: RegistryProxyValues) -> Result<Sy
             let protocol_parts: Vec<&str> = p.split("=").collect();
             match protocol_parts.as_slice() {
                 [protocol, address] => {
-                    // See if address has a type:// prefix
-                    let address = if !contains_type_prefix(*address) {
-                        format!("{}://{}", protocol, address)
-                    }
-                    else {
+                    // If address doesn't specify an explicit protocol as protocol://address
+                    // then default to HTTP
+                    let address = if extract_type_prefix(*address).is_some() {
                         String::from(*address)
+                    } else {
+                        format!("http://{}", address)
                     };
 
                     insert_proxy(&mut proxies, *protocol, address);
@@ -797,32 +797,39 @@ fn parse_registry_values_impl(registry_values: RegistryProxyValues) -> Result<Sy
             }
         }
     } else {
-        // Use one setting for all protocols.
-        if proxy_server.starts_with("http:") {
-            insert_proxy(&mut proxies, "http", proxy_server);
+        if let Some(scheme) = extract_type_prefix(&proxy_server) {
+            // Explicit protocol has been specified
+            insert_proxy(&mut proxies, scheme, proxy_server.to_owned());
         } else {
+            // No explicit protocol has been specified, default to HTTP
             insert_proxy(&mut proxies, "http", format!("http://{}", proxy_server));
-            insert_proxy(&mut proxies, "https", format!("https://{}", proxy_server));
+            insert_proxy(&mut proxies, "https", format!("http://{}", proxy_server));
         }
     }
     Ok(proxies)
 }
 
+/// Extract the protocol from the given address, if present
+/// For example, "https://example.com" will return Some("https")
 #[cfg(target_os = "windows")]
-fn contains_type_prefix(address: &str) -> bool {
+fn extract_type_prefix(address: &str) -> Option<&str> {
     if let Some(indice) = address.find("://") {
         if indice == 0 {
-            false
+            None
         }
         else {
             let prefix = &address[..indice];
             let contains_banned = prefix.contains(|c| c == ':' || c == '/');
 
-            !contains_banned
+            if !contains_banned {
+                Some(prefix)
+            } else {
+                None
+            }
         }
     }
     else {
-        false
+        None
     }
 }
 
@@ -999,7 +1006,10 @@ mod tests {
         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 valid_proxies_no_schema = get_sys_proxies(Some((1, String::from("127.0.0.1"))));
+        let valid_proxies_explicit_https = get_sys_proxies(Some((1, String::from("https://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"))));
+        let multiple_proxies_explicit_schema = get_sys_proxies(Some((1, String::from("http=http://127.0.0.1:8888;https=https://127.0.0.2:8888"))));
 
         // reset user setting when guards drop
         drop(_g1);
@@ -1014,11 +1024,31 @@ mod tests {
         assert_eq!(p.scheme(), "http");
         assert_eq!(p.host(), "127.0.0.1");
 
+        let p = &valid_proxies_no_schema["http"];
+        assert_eq!(p.scheme(), "http");
+        assert_eq!(p.host(), "127.0.0.1");
+
+        let p = &valid_proxies_no_schema["https"];
+        assert_eq!(p.scheme(), "http");
+        assert_eq!(p.host(), "127.0.0.1");
+
+        let p = &valid_proxies_explicit_https["https"];
+        assert_eq!(p.scheme(), "https");
+        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(), "http");
+        assert_eq!(p.host(), "127.0.0.2:8888");
+
+        let p = &multiple_proxies_explicit_schema["http"];
+        assert_eq!(p.scheme(), "http");
+        assert_eq!(p.host(), "127.0.0.1:8888");
+
+        let p = &multiple_proxies_explicit_schema["https"];
         assert_eq!(p.scheme(), "https");
         assert_eq!(p.host(), "127.0.0.2:8888");
     }
@@ -1145,14 +1175,14 @@ mod tests {
 
     #[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"));
+    fn test_type_prefix_extraction() {
+        assert!(extract_type_prefix("test").is_none());
+        assert!(extract_type_prefix("://test").is_none());
+        assert!(extract_type_prefix("some:prefix://test").is_none());
+        assert!(extract_type_prefix("some/prefix://test").is_none());
+
+        assert_eq!(extract_type_prefix("http://test").unwrap(), "http");
+        assert_eq!(extract_type_prefix("a://test").unwrap(), "a");
     }
 
     /// Guard an environment variable, resetting it to the original value


Test patch: