From 3f60b047882f7cd161ae9908ed96f9edf56d3c2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B2=99=E6=BC=A0=E4=B9=8B=E5=AD=90?= <7850715+maboloshi@users.noreply.github.com> Date: Thu, 24 Sep 2026 01:15:08 +0800 Subject: [PATCH] fix(gix-transport): honour `http.proxy` in the blocking `reqwest` backend The `reqwest` backend received the shared HTTP options - including `proxy` and `no_proxy` - via `TransportWithoutIO::configure()`, stored them, and then never read them. A configured proxy was therefore silently ignored and the request was sent directly, which only surfaced as an unrelated DNS or connection error. The `curl` backend has always honoured the same values. Proxies are configured on the `reqwest` client rather than on a single request, and the client was built in `Default for Remote`, before the first request and thus before any configuration was available. It is now built lazily from the configuration of the first request, and rebuilt whenever the client-level part of the configuration changes. The proxy value is interpreted the way `curl` does it: the scheme is optional and defaults to `http`, and an empty value disables proxying entirely (which also undoes a proxy from the environment, as `git` defines it). `no_proxy` is attached to the configured proxy. Schemes are matched case-insensitively, as they are in URLs generally, and schemes other than `http` and `https` are rejected with a dedicated error instead of being passed to a connector that cannot handle them. That fixes https://github.com/GitoxideLabs/gitoxide/issues/3015 --- .../src/client/blocking_io/http/mod.rs | 6 +- .../client/blocking_io/http/reqwest/remote.rs | 163 +++++++++++++----- .../tests/blocking-transport-http-reqwest.rs | 149 ++++++++++++++++ 3 files changed, 270 insertions(+), 48 deletions(-) diff --git a/gix-transport/src/client/blocking_io/http/mod.rs b/gix-transport/src/client/blocking_io/http/mod.rs index fdcb04fe825..3c7fa975504 100644 --- a/gix-transport/src/client/blocking_io/http/mod.rs +++ b/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; diff --git a/gix-transport/src/client/blocking_io/http/reqwest/remote.rs b/gix-transport/src/client/blocking_io/http/reqwest/remote.rs index 70eb70035e5..c69e1c2ee3b 100644 --- a/gix-transport/src/client/blocking_io/http/reqwest/remote.rs +++ b/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), + #[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, + no_proxy: Option, +} + +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>, + redirect_tail: Arc>, +) -> Result { + 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(); diff --git a/gix-transport/tests/blocking-transport-http-reqwest.rs b/gix-transport/tests/blocking-transport-http-reqwest.rs index 9ca1d884447..ff4f2bc0835 100644 --- a/gix-transport/tests/blocking-transport-http-reqwest.rs +++ b/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>> { + 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>, Box> { + 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::(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 : `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> { + 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> { + 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> { + 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> { + // 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> { + let mut client = http::connect::("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(()) +}