Repository: sagebind/isahc (rust)

Issue:
Breaking change from 0.9.10 to 0.9.11 due to #240 (presumably)
#240 rewrote the redirect implementation away from curl. I noticed that this broke redirects in my use case.

Test code:

```rust
use isahc::{
    config::{Configurable, RedirectPolicy},
    http::request::Request,
    HttpClient,
};
use std::error::Error;

#[async_std::main]
async fn main() -> Result<(), Box<dyn Error + Send + Sync>> {
    let client = HttpClient::builder()
        .redirect_policy(RedirectPolicy::Limit(8))
        .build()
        .expect("FATAL: failed to create http client");

    let req = Request::post("http://localhost:8265/command/help").body(())?;
    let res = client.send_async(req).await?;
    dbg!(res);
    Ok(())
}
```

The URL issues a 302 redirect.

### Output with isahc 0.9.10

```
[src/bin/isahc-test.rs:21] res = Response {
    status: 200,
    version: HTTP/1.1,
    headers: {
        "server": "nginx/1.18.0",
        "date": "Tue, 10 Nov 2020 08:09:31 GMT",
        "content-type": "text/plain",
        "content-length": "614",
        "last-modified": "Thu, 05 Nov 2020 10:51:25 GMT",
        "connection": "keep-alive",
        "etag": "\"5fa3d92d-266\"",
        "accept-ranges": "bytes",
    },
    body: Body(614),
}
```

### Output with isahc 0.9.11

```
[src/bin/isahc-test.rs:21] res = Response {
    status: 302,
    version: HTTP/1.1,
    headers: {
        "server": "nginx/1.18.0",
        "date": "Tue, 10 Nov 2020 08:04:33 GMT",
        "content-type": "text/html",
        "content-length": "145",
        "location": "http://localhost:8265/static/help.txt",
        "connection": "keep-alive",
    },
    body: Body(145),
}
```

Pull request:
Fix redirect handling with response bodies
Previously we were relying on curl to resolve the redirect location with `CURLINFO_REDIRECT_URL`, but this value is only populated once the response body stream has been consumed and the handle is complete. This means that redirects were working properly if the response containing the redirect had an empty body, but not if a nonempty body is included.

Since we don't really want to wait for the response body to be consumed before we decide whether we should redirect or not, change the redirect interceptor to derive the redirect location ourselves. Since the algorithm is nontrivial, pull in the `url` crate to do this resolution.

Also add a test for following redirects when response bodies are present to catch this bug.

Fixes #250.

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index cc1ff43..38aa179 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -41,6 +41,7 @@ log = "0.4"
 once_cell = "1"
 slab = "0.4"
 sluice = "0.5"
+url = "2.2"
 waker-fn = "1"
 
 [dependencies.chrono]
diff --git a/src/handler.rs b/src/handler.rs
index 10e5c1f..b9784e2 100644
--- a/src/handler.rs
+++ b/src/handler.rs
@@ -2,8 +2,7 @@
 
 use crate::{
     headers,
-    redirect::RedirectUri,
-    response::{EffectiveUri, LocalAddr, RemoteAddr},
+    response::{LocalAddr, RemoteAddr},
     Body, Error, Metrics,
 };
 use crossbeam_utils::atomic::AtomicCell;
@@ -11,12 +10,11 @@ use curl::easy::{InfoType, ReadError, SeekResult, WriteError};
 use curl_sys::CURL;
 use flume::Sender;
 use futures_lite::{io::{AsyncRead, AsyncWrite}, pin};
-use http::{Response, Uri};
+use http::Response;
 use once_cell::sync::OnceCell;
 use sluice::pipe;
 use std::{
     ascii,
-    convert::TryInto,
     ffi::CStr,
     fmt,
     future::Future,
@@ -244,14 +242,6 @@ impl RequestHandler {
                 headers.extend(self.response_headers.drain());
             }
 
-            if let Some(uri) = self.get_effective_uri() {
-                builder = builder.extension(EffectiveUri(uri));
-            }
-
-            if let Some(uri) = self.get_redirect_uri() {
-                builder = builder.extension(RedirectUri(uri));
-            }
-
             if let Some(addr) = self.get_local_addr() {
                 builder = builder.extension(LocalAddr(addr));
             }
@@ -290,36 +280,6 @@ impl RequestHandler {
         }
     }
 
-    fn get_effective_uri(&mut self) -> Option<Uri> {
-        self.get_uri(curl_sys::CURLINFO_EFFECTIVE_URL)
-    }
-
-    fn get_redirect_uri(&mut self) -> Option<Uri> {
-        self.get_uri(curl_sys::CURLINFO_REDIRECT_URL)
-    }
-
-    fn get_uri(&mut self, info: curl_sys::CURLINFO) -> Option<Uri> {
-        if self.handle.is_null() {
-            return None;
-        }
-
-        let mut ptr = ptr::null::<c_char>();
-
-        unsafe {
-            if curl_sys::curl_easy_getinfo(self.handle, info, &mut ptr)
-                != curl_sys::CURLE_OK
-            {
-                return None;
-            }
-        }
-
-        if ptr.is_null() {
-            return None;
-        }
-
-        unsafe { CStr::from_ptr(ptr) }.to_bytes().try_into().ok()
-    }
-
     fn get_primary_addr(&mut self) -> Option<SocketAddr> {
         let ip = self.get_primary_ip()?.parse().ok()?;
         let port = self.get_primary_port()?;
diff --git a/src/redirect.rs b/src/redirect.rs
index bf16f18..f50f08b 100644
--- a/src/redirect.rs
+++ b/src/redirect.rs
@@ -3,19 +3,20 @@ use crate::{
     handler::RequestBody,
     interceptor::{Context, Interceptor, InterceptorFuture},
     request::RequestExt,
-    Body,
-    Error,
+    Body, Error,
 };
-use http::{Request, Uri};
+use http::{Request, Response, Uri};
+use std::convert::TryFrom;
+use url::Url;
 
 /// How many redirects to follow by default if a limit is not specified. We
 /// don't actually allow infinite redirects as that could result in a dangerous
 /// infinite loop, so by default we actually limit redirects to a large amount.
 const DEFAULT_REDIRECT_LIMIT: u32 = 1024;
 
-/// Extension containing the redirect target on the response determined by curl,
-/// if any.
-pub(crate) struct RedirectUri(pub(crate) Uri);
+/// Extension containing the final "effective" URI that was visited, after
+/// following any redirects.
+pub(crate) struct EffectiveUri(pub(crate) Uri);
 
 /// Interceptor that implements automatic following of HTTP redirects.
 pub(crate) struct RedirectInterceptor;
@@ -23,95 +24,149 @@ pub(crate) struct RedirectInterceptor;
 impl Interceptor for RedirectInterceptor {
     type Err = Error;
 
-    fn intercept<'a>(&'a self, request: Request<Body>, ctx: Context<'a>) -> InterceptorFuture<'a, Self::Err> {
+    fn intercept<'a>(
+        &'a self,
+        mut request: Request<Body>,
+        ctx: Context<'a>,
+    ) -> InterceptorFuture<'a, Self::Err> {
         Box::pin(async move {
+            // Store the effective URI to include in the response.
+            let mut effective_uri = request.uri().clone();
+
             // Get the redirect policy for this request.
-            let policy = request.extensions()
+            let policy = request
+                .extensions()
                 .get::<RedirectPolicy>()
                 .cloned()
                 .unwrap_or_default();
 
             // No redirect handling, just proceed normally.
             if policy == RedirectPolicy::None {
-                return ctx.send(request).await;
+                let mut response = ctx.send(request).await?;
+                response
+                    .extensions_mut()
+                    .insert(EffectiveUri(effective_uri));
+
+                return Ok(response);
             }
 
-            let auto_referer = request.extensions()
+            let auto_referer = request
+                .extensions()
                 .get::<crate::config::redirect::AutoReferer>()
                 .is_some();
 
-            // Make a copy of the request before sending it.
-            let mut request_builder = request.to_builder();
-
-            // Send the request to get the ball rolling.
-            let mut response = ctx.send(request).await?;
-
-            let mut redirect_count: u32 = 0;
             let limit = match policy {
                 RedirectPolicy::Limit(limit) => limit,
                 _ => DEFAULT_REDIRECT_LIMIT,
             };
 
-            // Check for redirects. If a redirect should happen, then curl will
-            // return a URI to redirect to, which the request handler will
-            // attach to the response as this extension.
-            while let Some(RedirectUri(redirect_uri)) = response.extensions_mut().remove::<RedirectUri>() {
-                // Sanity check.
-                if !response.status().is_redirection() {
-                    break;
-                }
+            // Keep track of how many redirects we've done.
+            let mut redirect_count: u32 = 0;
 
-                // If we've reached the limit, return an error as requested.
-                if redirect_count >= limit {
-                    return Err(Error::TooManyRedirects);
+            loop {
+                // Preserve a clone of the request before sending it.
+                let mut request_builder = request.to_builder();
+
+                // Send the request to get the ball rolling.
+                let mut response = ctx.send(request).await?;
+
+                // Check for a redirect.
+                if let Some(location) = get_redirect_location(&effective_uri, &response) {
+                    // If we've reached the limit, return an error as requested.
+                    if redirect_count >= limit {
+                        return Err(Error::TooManyRedirects);
+                    }
+
+                    // Set referer header.
+                    if auto_referer {
+                        let referer = request_builder.uri_ref().unwrap().to_string();
+                        request_builder = request_builder.header(http::header::REFERER, referer);
+                    }
+
+                    // Check if we should change the request method into a GET. HTTP
+                    // specs don't really say one way or another when this should
+                    // happen for most status codes, so we just mimic curl's
+                    // behavior here since it is so common.
+                    if response.status() == 301 || response.status() == 302 || response.status() == 303
+                    {
+                        request_builder = request_builder.method(http::Method::GET);
+                    }
+
+                    // Grab the request body back from the internal handler, as we
+                    // might need to send it again (if possible...)
+                    let mut request_body = response
+                        .extensions_mut()
+                        .remove::<RequestBody>()
+                        .map(|v| v.0)
+                        .unwrap_or_default();
+
+                    // Redirect handling is tricky when we are uploading something.
+                    // If we can, reset the body stream to the beginning. This might
+                    // work if the body to upload is an in-memory byte buffer, but
+                    // for arbitrary streams we can't do this.
+                    //
+                    // There's not really a good way of handling this gracefully, so
+                    // we just return an error so that the user knows about it.
+                    if !request_body.reset() {
+                        return Err(Error::RequestBodyError(Some(String::from(
+                            "could not follow redirect because request body is not rewindable",
+                        ))));
+                    }
+
+                    // Update the request to point to the new URI.
+                    effective_uri = location.clone();
+                    request = request_builder.uri(location).body(request_body)?;
+                    redirect_count += 1;
                 }
 
-                // Set referer header.
-                if auto_referer {
-                    let referer = request_builder.uri_ref().unwrap().to_string();
-                    request_builder = request_builder.header(http::header::REFERER, referer);
-                }
+                // No more redirects; set the effective URI we finally settled on and return.
+                else {
+                    response
+                        .extensions_mut()
+                        .insert(EffectiveUri(effective_uri));
 
-                // Check if we should change the request method into a GET. HTTP
-                // specs don't really say one way or another when this should
-                // happen for most status codes, so we just mimic curl's
-                // behavior here since it is so common.
-                if response.status() == 301 || response.status() == 302 || response.status() == 303 {
-                    request_builder = request_builder.method(http::Method::GET);
+                    return Ok(response);
                 }
+            }
+        })
+    }
+}
 
-                // Grab the request body back from the internal handler, as we
-                // might need to send it again (if possible...)
-                let mut request_body = response.extensions_mut()
-                    .remove::<RequestBody>()
-                    .map(|v| v.0)
-                    .unwrap_or_default();
-
-                // Redirect handling is tricky when we are uploading something.
-                // If we can, reset the body stream to the beginning. This might
-                // work if the body to upload is an in-memory byte buffer, but
-                // for arbitrary streams we can't do this.
-                //
-                // There's not really a good way of handling this gracefully, so
-                // we just return an error so that the user knows about it.
-                if !request_body.reset() {
-                    return Err(Error::RequestBodyError(Some(String::from("could not follow redirect because request body is not rewindable"))));
+fn get_redirect_location<T>(request_uri: &Uri, response: &Response<T>) -> Option<Uri> {
+    if response.status().is_redirection() {
+        let location = response.headers().get(http::header::LOCATION)?;
+
+        match location.to_str() {
+            Ok(location) => {
+                match resolve(request_uri, location) {
+                    Ok(uri) => return Some(uri),
+                    Err(e) => {
+                        tracing::debug!("bad redirect location: {}", e);
+                    }
                 }
+            }
+            Err(e) => {
+                tracing::debug!("bad redirect location: {}", e);
+            }
+        }
+    }
 
-                let request = request_builder.uri(redirect_uri)
-                    .body(request_body)?;
+    None
+}
 
-                // Keep another clone of the request around again, in case we
-                // need to follow yet another redirect.
-                request_builder = request.to_builder();
+/// Resolve one URI in terms of another.
+fn resolve(base: &Uri, target: &str) -> Result<Uri, Box<dyn std::error::Error>> {
+    // Optimistically check if this is an absolute URI.
+    match Url::parse(target) {
+        Ok(url) => Ok(Uri::try_from(url.as_str())?),
 
-                // Send the redirected request.
-                response = ctx.send(request).await?;
+        // Relative URI, resolve against the base.
+        Err(url::ParseError::RelativeUrlWithoutBase) => {
+            let base = Url::parse(base.to_string().as_str())?;
 
-                redirect_count += 1;
-            }
+            Ok(Uri::try_from(base.join(target)?.as_str())?)
+        }
 
-            Ok(response)
-        })
+        Err(e) => Err(Box::new(e)),
     }
 }
diff --git a/src/response.rs b/src/response.rs
index 119f139..a499272 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -1,4 +1,4 @@
-use crate::Metrics;
+use crate::{redirect::EffectiveUri, Metrics};
 use futures_lite::io::AsyncRead;
 use http::{Response, Uri};
 use std::{
@@ -226,8 +226,6 @@ impl<T> ResponseExt<T> for Response<T> {
     }
 }
 
-pub(crate) struct EffectiveUri(pub(crate) Uri);
-
 pub(crate) struct LocalAddr(pub(crate) SocketAddr);
 
 pub(crate) struct RemoteAddr(pub(crate) SocketAddr);


Test patch:
diff --git a/tests/redirects.rs b/tests/redirects.rs
index 1af83aa..de218a8 100644
--- a/tests/redirects.rs
+++ b/tests/redirects.rs
@@ -144,6 +144,36 @@ fn redirect_also_sends_post(status: u16) {
     assert_eq!(m2.request().method, "POST");
 }
 
+// Issue #250
+#[test]
+fn redirect_with_response_body() {
+    let m2 = mock! {
+        body: "OK",
+    };
+    let location = m2.url();
+
+    let m1 = mock! {
+        status: 302,
+        headers {
+            "Location": location,
+        }
+        body: "REDIRECT",
+    };
+
+    let response = Request::post(m1.url())
+        .redirect_policy(RedirectPolicy::Follow)
+        .body(())
+        .unwrap()
+        .send()
+        .unwrap();
+
+    assert_eq!(response.status(), 200);
+    assert_eq!(response.effective_uri().unwrap().to_string(), m2.url());
+
+    assert_eq!(m1.request().method, "POST");
+    assert_eq!(m2.request().method, "GET");
+}
+
 // Issue #250
 #[test]
 fn redirect_policy_from_client() {