fix(gix-transport): honour http.proxy in the blocking reqwest backend

gix-transport/src/client/blocking_io/http/mod.rs

@@ -30,8 +30,10 @@ pub mod curl;
 
 /// The experimental `reqwest` backend.
 ///
-/// It doesn't support any of the shared http options yet, but can be seen as example on how to integrate blocking `http` backends.
-/// There is also nothing that would prevent it from becoming a fully-featured HTTP backend except for demand and time.
+/// It consumes `http.extraHeader`, `http.followRedirects`, `http.proxy` and `gitoxide.http.noProxy`, while most other
+/// shared options are not supported yet. Proxy schemes other than `http` and `https` are unsupported as well.
+/// It can be seen as example on how to integrate blocking `http` backends, and there is nothing that would prevent it
+/// from becoming a fully-featured HTTP backend except for demand and time.
 #[cfg(feature = "http-client-reqwest")]
 pub mod reqwest;
 

gix-transport/src/client/blocking_io/http/reqwest/remote.rs

@@ -26,6 +26,8 @@ pub enum Error {
     ReadPostBody(#[from] std::io::Error),
     #[error("Request configuration failed")]
     ConfigureRequest(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("The proxy scheme {scheme:?} is unsupported, only 'http' and 'https' proxies can be used here")]
+    UnsupportedProxyScheme { scheme: String },
     #[error(transparent)]
     Redirect(#[from] redirect::Error),
 }
@@ -47,6 +49,105 @@ fn authority_changed(curr_url: &reqwest::Url, prev_url: &reqwest::Url) -> bool {
         || curr_url.port_or_known_default() != prev_url.port_or_known_default()
 }
 
+/// The parts of [`http::Options`] that determine how the `reqwest` client itself has to be built.
+///
+/// Proxies are configured on the client instead of on a single request, so the client has to be rebuilt
+/// whenever any of these change.
+#[derive(Clone, PartialEq, Eq)]
+struct ClientConfig {
+    proxy: Option<String>,
+    no_proxy: Option<String>,
+}
+
+impl ClientConfig {
+    fn from_options(options: &http::Options) -> Self {
+        ClientConfig {
+            proxy: options.proxy.clone(),
+            no_proxy: options.no_proxy.clone(),
+        }
+    }
+}
+
+/// Build the `reqwest` client used for all requests of this thread, applying the client-level parts of `config`.
+///
+/// Note that a configured proxy replaces the environment-based proxy `reqwest` would use otherwise, which mirrors
+/// how `git` prefers `http.proxy` over the `http_proxy` environment variable.
+fn build_client(
+    config: &http::Options,
+    redirect_action: Arc<Mutex<RedirectAction>>,
+    redirect_tail: Arc<Mutex<String>>,
+) -> Result<reqwest::blocking::Client, Error> {
+    let mut builder = reqwest::blocking::ClientBuilder::new()
+        .connect_timeout(std::time::Duration::from_secs(20))
+        .http1_title_case_headers()
+        .redirect(reqwest::redirect::Policy::custom(move |attempt| {
+            match *redirect_action.lock() {
+                RedirectAction::Follow => {
+                    let curr_url = attempt.url();
+                    let prev_urls = attempt.previous();
+                    // emulate default git behaviour which relies on curl default behaviour apparently.
+                    const CURL_DEFAULT_REDIRS: usize = 50;
+                    if prev_urls.len() >= CURL_DEFAULT_REDIRS {
+                        return attempt.error("too many redirects");
+                    }
+
+                    match prev_urls.last() {
+                        Some(prev_url) if !redirect::scheme_is_safe(curr_url.as_str(), prev_url.as_str()) => {
+                            // Don't follow insecure protocol redirects, particularly https-to-http downgrades.
+                            attempt.stop()
+                        }
+                        Some(prev_url) if authority_changed(curr_url, prev_url) => {
+                            // Allowed only if the tail doesn't change.
+                            let redirect_tail = redirect_tail.lock();
+                            if curr_url.as_str().ends_with(redirect_tail.as_str()) {
+                                attempt.follow()
+                            } else {
+                                let curr_url = curr_url.as_str().to_owned();
+                                let redirect_tail = redirect_tail.to_string();
+                                attempt.error(format!(
+                                    "redirect url {curr_url:?} does not end with expected request suffix {redirect_tail:?}",
+                                ))
+                            }
+                        }
+                        _ => attempt.follow(),
+                    }
+                }
+                RedirectAction::RejectConfiguredHeaders => {
+                    attempt.error("refusing to follow redirect after request headers were configured")
+                }
+                RedirectAction::Stop => attempt.stop(),
+            }
+        }));
+
+    match config.proxy.as_deref() {
+        // An empty string means the proxy is disabled entirely, which also undoes a proxy from the environment.
+        Some("") => builder = builder.no_proxy(),
+        Some(proxy) => {
+            // Like curl, the scheme of a proxy is optional and defaults to `http`, and only schemes `reqwest`
+            // is compiled to support can be used here.
+            let (scheme, proxy) = match proxy.split_once("://") {
+                Some((scheme, rest)) => (scheme, format!("{scheme}://{rest}")),
+                None => ("http", format!("http://{proxy}")),
+            };
+            // Schemes are case-insensitive in URLs, and only those `reqwest` is compiled to support can be used here.
+            if !(scheme.eq_ignore_ascii_case("http") || scheme.eq_ignore_ascii_case("https")) {
+                return Err(Error::UnsupportedProxyScheme {
+                    scheme: scheme.to_owned(),
+                });
+            }
+            let mut proxy = reqwest::Proxy::all(proxy)?;
+            if let Some(no_proxy) = config.no_proxy.as_deref() {
+                proxy = proxy.no_proxy(reqwest::NoProxy::from_string(no_proxy));
+            }
+            builder = builder.proxy(proxy);
+        }
+        // Without a configured proxy, `reqwest` keeps using its own environment-based proxy detection.
+        None => {}
+    }
+
+    Ok(builder.build()?)
+}
+
 impl Default for Remote {
     fn default() -> Self {
         let (req_send, req_recv) = std::sync::mpsc::sync_channel(0);
@@ -60,52 +161,8 @@ impl Default for Remote {
 
             // We may error while configuring, which is expected as part of the internal protocol. The error will be
             // received and the sender of the request might restart us.
-            let client = reqwest::blocking::ClientBuilder::new()
-                .connect_timeout(std::time::Duration::from_secs(20))
-                .http1_title_case_headers()
-                .redirect(reqwest::redirect::Policy::custom({
-                    let redirect_action = redirect_action.clone();
-                    let redirect_tail = redirect_tail.clone();
-                    move |attempt| {
-                        match *redirect_action.lock() {
-                            RedirectAction::Follow => {
-                                let curr_url = attempt.url();
-                                let prev_urls = attempt.previous();
-                                // emulate default git behaviour which relies on curl default behaviour apparently.
-                                const CURL_DEFAULT_REDIRS: usize = 50;
-                                if prev_urls.len() >= CURL_DEFAULT_REDIRS {
-                                    return attempt.error("too many redirects");
-                                }
-
-                                match prev_urls.last() {
-                                    Some(prev_url) if !redirect::scheme_is_safe(curr_url.as_str(), prev_url.as_str()) => {
-                                        // Don't follow insecure protocol redirects, particularly https-to-http downgrades.
-                                        attempt.stop()
-                                    }
-                                    Some(prev_url) if authority_changed(curr_url, prev_url) => {
-                                        // Allowed only if the tail doesn't change.
-                                        let redirect_tail = redirect_tail.lock();
-                                        if curr_url.as_str().ends_with(redirect_tail.as_str()) {
-                                            attempt.follow()
-                                        } else {
-                                            let curr_url = curr_url.as_str().to_owned();
-                                            let redirect_tail = redirect_tail.to_string();
-                                            attempt.error(format!(
-                                                "redirect url {curr_url:?} does not end with expected request suffix {redirect_tail:?}",
-                                            ))
-                                        }
-                                    }
-                                    _ => attempt.follow(),
-                                }
-                            }
-                            RedirectAction::RejectConfiguredHeaders => {
-                                attempt.error("refusing to follow redirect after request headers were configured")
-                            }
-                            RedirectAction::Stop => attempt.stop(),
-                        }
-                    }
-                }))
-                .build()?;
+            // The client is built lazily as its configuration is only known once the first request arrives.
+            let mut client: Option<(ClientConfig, reqwest::blocking::Client)> = None;
 
             for Request {
                 url,
@@ -115,6 +172,20 @@ impl Default for Remote {
                 config,
             } in req_recv
             {
+                let client_config = ClientConfig::from_options(&config);
+                if client
+                    .as_ref()
+                    .is_none_or(|(configured, _)| *configured != client_config)
+                {
+                    client = Some((
+                        client_config,
+                        build_client(&config, redirect_action.clone(), redirect_tail.clone())?,
+                    ));
+                }
+                let client = &client
+                    .as_ref()
+                    .expect("client was either built just now or is still present")
+                    .1;
                 let redirected_base_url = redirected_base_url_shared.lock().clone();
                 let effective_url = redirect::swap_tails(redirected_base_url.as_deref(), &base_url, url.clone());
                 let has_configured_extra_headers = !config.extra_headers.is_empty();

gix-transport/tests/blocking-transport-http-reqwest.rs

@@ -253,3 +253,152 @@ fn cross_authority_redirects_are_not_followed_without_matching_tail() -> Result<
     );
     Ok(())
 }
+
+/// Accept a single connection within `deadline` and return the request lines it carries, or `None` if no
+/// connection was made in time. Useful to learn whether a request was sent to a proxy at all.
+fn observe_request_within_deadline(
+    listener: std::net::TcpListener,
+    deadline: std::time::Duration,
+) -> std::thread::JoinHandle<Option<Vec<String>>> {
+    listener
+        .set_nonblocking(true)
+        .expect("nonblocking listener can be configured");
+    std::thread::spawn(move || {
+        let until = std::time::Instant::now() + deadline;
+        loop {
+            match listener.accept() {
+                Ok((stream, _)) => {
+                    stream
+                        .set_nonblocking(false)
+                        .expect("the accepted stream can be used in blocking mode");
+                    stream
+                        .set_read_timeout(Some(std::time::Duration::from_millis(1000)))
+                        .expect("a read timeout can be configured");
+                    let mut reader = std::io::BufReader::new(stream);
+                    return Some(read_request_lines(&mut reader));
+                }
+                Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => {
+                    if std::time::Instant::now() >= until {
+                        return None;
+                    }
+                    std::thread::sleep(std::time::Duration::from_millis(10));
+                }
+                Err(err) => panic!("accept should work: {err}"),
+            }
+        }
+    })
+}
+
+/// Send `url` through a fresh listener that stands in for the proxy, and return the request it received, if any.
+///
+/// `url` is expected to be unreachable without the proxy, so the handshake is expected to fail - only the question
+/// whether the request was sent to the proxy instead of the host is of interest.
+fn request_through_proxy(
+    url: &str,
+    options: impl FnOnce(std::net::SocketAddr) -> http::Options,
+) -> Result<Option<Vec<String>>, Box<dyn Error + Send + Sync>> {
+    let proxy_listener = std::net::TcpListener::bind("127.0.0.1:0")?;
+    let proxy_addr = proxy_listener.local_addr()?;
+    let observed = observe_request_within_deadline(proxy_listener, std::time::Duration::from_millis(1000));
+
+    let mut client = http::connect::<http::reqwest::Remote>(url.try_into()?, Protocol::V1, false);
+    client.configure(&options(proxy_addr))?;
+    client.handshake(Service::UploadPack, &[]).ok();
+
+    Ok(observed.join().expect("the observation thread doesn't panic"))
+}
+
+/// Regression for <https://github.com/GitoxideLabs/gitoxide/issues/3015>: `http.proxy` used to be received and
+/// then ignored, so the request was sent directly even though a proxy was configured.
+#[test]
+fn proxy_configuration_is_used() -> Result<(), Box<dyn Error + Send + Sync>> {
+    let request = request_through_proxy("http://git.invalid/repo", |addr| http::Options {
+        proxy: Some(format!("http://{addr}")),
+        ..Default::default()
+    })?
+    .expect("the request must be sent to the configured proxy");
+
+    assert_eq!(
+        request.first().map(String::as_str),
+        Some("GET http://git.invalid/repo/info/refs?service=git-upload-pack HTTP/1.1"),
+        "a proxied http request is sent in absolute form, got {request:?}"
+    );
+    Ok(())
+}
+
+/// Proxies are declared in curl-style, where the scheme is optional and defaults to `http`.
+#[test]
+fn proxy_without_scheme_defaults_to_http() -> Result<(), Box<dyn Error + Send + Sync>> {
+    let request = request_through_proxy("http://git.invalid/repo", |addr| http::Options {
+        proxy: Some(addr.to_string()),
+        ..Default::default()
+    })?
+    .expect("a proxy without a scheme must default to http and be used");
+
+    assert_eq!(
+        request.first().map(String::as_str),
+        Some("GET http://git.invalid/repo/info/refs?service=git-upload-pack HTTP/1.1"),
+        "the request must have been sent to the proxy, got {request:?}"
+    );
+    Ok(())
+}
+
+/// URL schemes are case-insensitive, also when they are part of a proxy declaration.
+#[test]
+fn proxy_scheme_is_case_insensitive() -> Result<(), Box<dyn Error + Send + Sync>> {
+    let request = request_through_proxy("http://git.invalid/repo", |addr| http::Options {
+        proxy: Some(format!("HTTP://{addr}")),
+        ..Default::default()
+    })?
+    .expect("a proxy with an upper-case scheme must be accepted and used");
+
+    assert_eq!(
+        request.first().map(String::as_str),
+        Some("GET http://git.invalid/repo/info/refs?service=git-upload-pack HTTP/1.1"),
+        "the request must have been sent to the proxy, got {request:?}"
+    );
+    Ok(())
+}
+
+#[test]
+fn no_proxy_configuration_bypasses_the_proxy() -> Result<(), Box<dyn Error + Send + Sync>> {
+    // Bind and drop a listener to obtain a port that refuses connections, so the direct connection fails
+    // immediately instead of waiting for a DNS lookup of a host that cannot be resolved.
+    let closed_addr = {
+        let listener = std::net::TcpListener::bind("127.0.0.1:0")?;
+        listener.local_addr()?
+    };
+
+    let request = request_through_proxy(&format!("http://{closed_addr}/repo"), |addr| http::Options {
+        proxy: Some(format!("http://{addr}")),
+        no_proxy: Some("127.0.0.1".into()),
+        ..Default::default()
+    })?;
+
+    assert!(
+        request.is_none(),
+        "a host matching noProxy must be contacted directly instead of being proxied, got {request:?}"
+    );
+    Ok(())
+}
+
+/// Only `http` and `https` proxies can be used by this backend, and saying so beats a confusing failure later.
+#[test]
+fn unsupported_proxy_scheme_is_reported() -> Result<(), Box<dyn Error + Send + Sync>> {
+    let mut client = http::connect::<http::reqwest::Remote>("http://git.invalid/repo".try_into()?, Protocol::V1, false);
+    client.configure(&http::Options {
+        proxy: Some("socks5://127.0.0.1:1".into()),
+        ..Default::default()
+    })?;
+
+    let err = client
+        .handshake(Service::UploadPack, &[])
+        .err()
+        .expect("an unsupported proxy scheme must fail instead of being ignored");
+    let err = format!("{err:?}");
+    assert!(
+        err.contains("socks5"),
+        "the error should name the unsupported proxy scheme, got {err}"
+    );
+    Ok(())
+}