feat!: interpret the FollowRedirects option for the curl HTTP backend.

This comes with changes to the `HTTP` trait which now requires a base-url
to be provided as well.

Author: Byron

git-transport/src/client/blocking_io/http/curl/mod.rs

@@ -1,5 +1,4 @@
 use std::{
-    error::Error,
     sync::mpsc::{Receiver, SyncSender},
     thread,
 };
@@ -10,10 +9,20 @@ use crate::client::blocking_io::http;
 
 mod remote;
 
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error(transparent)]
+    Curl(#[from] curl::Error),
+    #[error(transparent)]
+    Redirect(#[from] http::redirect::Error),
+    #[error(transparent)]
+    Authenticate(#[from] git_credentials::protocol::Error),
+}
+
 pub struct Curl {
     req: SyncSender<remote::Request>,
     res: Receiver<remote::Response>,
-    handle: Option<thread::JoinHandle<Result<(), curl::Error>>>,
+    handle: Option<thread::JoinHandle<Result<(), Error>>>,
     config: http::Options,
 }
 
@@ -36,6 +45,7 @@ impl Curl {
     fn make_request(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
         upload: bool,
     ) -> Result<http::PostResponse<io::pipe::Reader, io::pipe::Reader, io::pipe::Writer>, http::Error> {
@@ -47,6 +57,7 @@ impl Curl {
             .req
             .send(remote::Request {
                 url: url.to_owned(),
+                base_url: base_url.to_owned(),
                 headers: list,
                 upload,
                 config: self.config.clone(),
@@ -92,20 +103,25 @@ impl http::Http for Curl {
     fn get(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<http::GetResponse<Self::Headers, Self::ResponseBody>, http::Error> {
-        self.make_request(url, headers, false).map(Into::into)
+        self.make_request(url, base_url, headers, false).map(Into::into)
     }
 
     fn post(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<http::PostResponse<Self::Headers, Self::ResponseBody, Self::PostBody>, http::Error> {
-        self.make_request(url, headers, true)
+        self.make_request(url, base_url, headers, true)
     }
 
-    fn configure(&mut self, config: &dyn std::any::Any) -> Result<(), Box<dyn Error + Send + Sync + 'static>> {
+    fn configure(
+        &mut self,
+        config: &dyn std::any::Any,
+    ) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
         if let Some(config) = config.downcast_ref::<http::Options>() {
             self.config = config.clone();
         }

git-transport/src/client/blocking_io/http/curl/remote.rs

@@ -9,8 +9,8 @@ use std::{
 use curl::easy::{Auth, Easy2};
 use git_features::io::pipe;
 
-use crate::client::blocking_io::http;
-use crate::client::http::options::ProxyAuthMethod;
+use crate::client::blocking_io::http::{self, curl::Error, redirect};
+use crate::client::http::options::{FollowRedirects, ProxyAuthMethod};
 
 #[derive(Default)]
 struct Handler {
@@ -19,6 +19,7 @@ struct Handler {
     receive_body: Option<pipe::Reader>,
     checked_status: bool,
     last_status: usize,
+    follow: FollowRedirects,
 }
 
 impl Handler {
@@ -34,9 +35,13 @@ impl Handler {
         let code = std::str::from_utf8(code)?;
         code.parse().map_err(Into::into)
     }
-    fn parse_status(data: &[u8]) -> Option<(usize, Box<dyn std::error::Error + Send + Sync>)> {
+    fn parse_status(data: &[u8], follow: FollowRedirects) -> Option<(usize, Box<dyn std::error::Error + Send + Sync>)> {
+        let valid_end = match follow {
+            FollowRedirects::Initial | FollowRedirects::All => 308,
+            FollowRedirects::None => 299,
+        };
         match Self::parse_status_inner(data) {
-            Ok(status) if !(200..=299).contains(&status) => {
+            Ok(status) if !(200..=valid_end).contains(&status) => {
                 Some((status, format!("Received HTTP status {}", status).into()))
             }
             Ok(_) => None,
@@ -68,7 +73,7 @@ impl curl::easy::Handler for Handler {
                 } else {
                     self.checked_status = true;
                     self.last_status = 200;
-                    match Handler::parse_status(data) {
+                    match Handler::parse_status(data, self.follow) {
                         None => true,
                         Some((status, err)) => {
                             self.last_status = status;
@@ -95,6 +100,7 @@ impl curl::easy::Handler for Handler {
 
 pub struct Request {
     pub url: String,
+    pub base_url: String,
     pub headers: curl::easy::List,
     pub upload: bool,
     pub config: http::Options,
@@ -107,23 +113,26 @@ pub struct Response {
 }
 
 pub fn new() -> (
-    thread::JoinHandle<Result<(), curl::Error>>,
+    thread::JoinHandle<Result<(), Error>>,
     SyncSender<Request>,
     Receiver<Response>,
 ) {
     let (req_send, req_recv) = sync_channel(0);
     let (res_send, res_recv) = sync_channel(0);
-    let handle = std::thread::spawn(move || -> Result<(), curl::Error> {
+    let handle = std::thread::spawn(move || -> Result<(), Error> {
         let mut handle = Easy2::new(Handler::default());
+        let mut follow = None;
+        let mut redirected_base_url = None::<String>;
 
         for Request {
             url,
+            base_url,
             mut headers,
             upload,
             config:
                 http::Options {
                     extra_headers,
-                    follow_redirects: _,
+                    follow_redirects,
                     low_speed_limit_bytes_per_second,
                     low_speed_time_seconds,
                     connect_timeout,
@@ -135,7 +144,8 @@ pub fn new() -> (
                 },
         } in req_recv
         {
-            handle.url(&url)?;
+            let effective_url = redirect::swap_tails(redirected_base_url.as_deref(), &base_url, url.clone());
+            handle.url(&effective_url)?;
 
             // GitHub sends 'chunked' to avoid unknown clients to choke on the data, I suppose
             handle.post(upload)?;
@@ -160,12 +170,7 @@ pub fn new() -> (
                 handle.proxy_type(proxy_type)?;
 
                 if let Some((obtain_creds_action, authenticate)) = proxy_authenticate {
-                    let creds = authenticate.lock().expect("no panics in other threads")(obtain_creds_action)
-                        .map_err(|err| {
-                            let mut e = curl::Error::new(0);
-                            e.set_extra(err.to_string());
-                            e
-                        })?
+                    let creds = authenticate.lock().expect("no panics in other threads")(obtain_creds_action)?
                         .expect("action to fetch credentials");
                     handle.proxy_username(&creds.identity.username)?;
                     handle.proxy_password(&creds.identity.password)?;
@@ -214,6 +219,14 @@ pub fn new() -> (
                 (receive_data, receive_headers, send_body)
             };
 
+            let follow = follow.get_or_insert(follow_redirects);
+            handle.get_mut().follow = *follow;
+            handle.follow_location(matches!(*follow, FollowRedirects::Initial | FollowRedirects::All))?;
+
+            if *follow == FollowRedirects::Initial {
+                *follow = FollowRedirects::None;
+            }
+
             if res_send
                 .send(Response {
                     headers: receive_headers,
@@ -256,24 +269,33 @@ pub fn new() -> (
                         action.store()
                     } else {
                         action.erase()
-                    })
-                    .map_err(|err| {
-                        let mut e = curl::Error::new(0);
-                        e.set_extra(err.to_string());
-                        e
                     })?;
                 }
                 handler.reset();
                 handler.receive_body.take();
                 handler.send_header.take();
                 handler.send_data.take();
+                let actual_url = handle
+                    .effective_url()?
+                    .expect("effective url is present and valid UTF-8");
+                if actual_url != effective_url {
+                    redirected_base_url = redirect::base_url(actual_url, &base_url, url)?.into();
+                }
             }
         }
         Ok(())
     });
     (handle, req_send, res_recv)
 }
 
+impl From<Error> for http::Error {
+    fn from(err: Error) -> Self {
+        http::Error::Detail {
+            description: err.to_string(),
+        }
+    }
+}
+
 impl From<curl::Error> for http::Error {
     fn from(err: curl::Error) -> Self {
         http::Error::Detail {

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

@@ -252,7 +252,9 @@ impl<H: Http> client::TransportWithoutIO for Transport<H> {
             headers,
             body,
             post_body,
-        } = self.http.post(&url, static_headers.iter().chain(&dynamic_headers))?;
+        } = self
+            .http
+            .post(&url, &self.url, static_headers.iter().chain(&dynamic_headers))?;
         let line_provider = self
             .line_provider
             .as_mut()
@@ -319,9 +321,9 @@ impl<H: Http> client::Transport for Transport<H> {
             dynamic_headers.push(format!("Git-Protocol: {}", parameters).into());
         }
         self.add_basic_auth_if_present(&mut dynamic_headers)?;
-        let GetResponse { headers, body } = self
-            .http
-            .get(url.as_ref(), static_headers.iter().chain(&dynamic_headers))?;
+        let GetResponse { headers, body } =
+            self.http
+                .get(url.as_ref(), &self.url, static_headers.iter().chain(&dynamic_headers))?;
         <Transport<H>>::check_content_type(service, "advertisement", headers)?;
 
         let line_reader = self
@@ -431,3 +433,74 @@ pub fn connect_http<H: Http>(http: H, url: &str, desired_version: Protocol) -> T
 pub fn connect(url: &str, desired_version: Protocol) -> Transport<Impl> {
     Transport::new(url, desired_version)
 }
+
+///
+#[cfg(feature = "http-client-curl")]
+pub mod redirect {
+    /// The error provided when redirection went beyond what we deem acceptable.
+    #[derive(Debug, thiserror::Error)]
+    #[error("Redirect url {redirect_url:?} could not be reconciled with original url {expected_url} as they don't share the same suffix")]
+    pub struct Error {
+        redirect_url: String,
+        expected_url: String,
+    }
+
+    pub(crate) fn base_url(redirect_url: &str, base_url: &str, url: String) -> Result<String, Error> {
+        let tail = url
+            .strip_prefix(base_url)
+            .expect("BUG: caller assures `base_url` is subset of `url`");
+        redirect_url
+            .strip_suffix(tail)
+            .ok_or_else(|| Error {
+                redirect_url: redirect_url.into(),
+                expected_url: url,
+            })
+            .map(ToOwned::to_owned)
+    }
+
+    pub(crate) fn swap_tails(effective_base_url: Option<&str>, base_url: &str, mut url: String) -> String {
+        match effective_base_url {
+            Some(effective_base) => {
+                url.replace_range(..base_url.len(), effective_base);
+                url
+            }
+            None => url,
+        }
+    }
+
+    #[cfg(test)]
+    mod tests {
+        use super::*;
+
+        #[test]
+        fn base_url_complete() {
+            assert_eq!(
+                base_url(
+                    "https://redirected.org/b/info/refs?hi",
+                    "https://original/a",
+                    "https://original/a/info/refs?hi".into()
+                )
+                .unwrap(),
+                "https://redirected.org/b"
+            );
+        }
+
+        #[test]
+        fn swap_tails_complete() {
+            assert_eq!(
+                swap_tails(None, "not interesting", "used".into()),
+                "used",
+                "without effective base url, it passes url, no redirect happened yet"
+            );
+            assert_eq!(
+                swap_tails(
+                    Some("https://redirected.org/b"),
+                    "https://original/a",
+                    "https://original/a/info/refs?something".into()
+                ),
+                "https://redirected.org/b/info/refs?something",
+                "the tail stays the same if redirection happened"
+            )
+        }
+    }
+}

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

@@ -118,6 +118,7 @@ impl Remote {
     fn make_request(
         &mut self,
         url: &str,
+        _base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
         upload: bool,
     ) -> Result<http::PostResponse<pipe::Reader, pipe::Reader, pipe::Writer>, http::Error> {
@@ -182,17 +183,19 @@ impl http::Http for Remote {
     fn get(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<http::GetResponse<Self::Headers, Self::ResponseBody>, http::Error> {
-        self.make_request(url, headers, false).map(Into::into)
+        self.make_request(url, base_url, headers, false).map(Into::into)
     }
 
     fn post(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<http::PostResponse<Self::Headers, Self::ResponseBody, Self::PostBody>, http::Error> {
-        self.make_request(url, headers, true)
+        self.make_request(url, base_url, headers, true)
     }
 
     fn configure(&mut self, config: &dyn Any) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {

git-transport/src/client/blocking_io/http/traits.rs

@@ -53,16 +53,21 @@ pub trait Http {
     /// A type allowing to write the content to post.
     type PostBody: std::io::Write;
 
-    /// Initiate a `GET` request to `url` provided the given `headers`.
+    /// Initiate a `GET` request to `url` provided the given `headers`, where `base_url` is so that `base_url + tail == url`.
+    ///
+    /// The `base_url` helps to validate redirects and to swap it with the effective base after a redirect.
     ///
     /// The `headers` are provided verbatim and include both the key as well as the value.
     fn get(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<GetResponse<Self::Headers, Self::ResponseBody>, Error>;
 
-    /// Initiate a `POST` request to `url` providing with the given `headers`.
+    /// Initiate a `POST` request to `url` providing with the given `headers`, where `base_url` is so that `base_url + tail == url`.
+    ///
+    /// The `base_url` helps to validate redirects and to swap it with the effective base after a redirect.
     ///
     /// The `headers` are provided verbatim and include both the key as well as the value.
     /// Note that the [`PostResponse`] contains the [`post_body`][PostResponse::post_body] field which implements [`std::io::Write`]
@@ -71,6 +76,7 @@ pub trait Http {
     fn post(
         &mut self,
         url: &str,
+        base_url: &str,
         headers: impl IntoIterator<Item = impl AsRef<str>>,
     ) -> Result<PostResponse<Self::Headers, Self::ResponseBody, Self::PostBody>, Error>;