Skip to content

Commit

Permalink
Fix redirect handling with response bodies (#255)
Browse files Browse the repository at this point in the history
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.
sagebind authored Nov 14, 2020
1 parent bf6e5de commit 05ca1de
Showing 5 changed files with 155 additions and 111 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -41,6 +41,7 @@ log = "0.4"
once_cell = "1"
slab = "0.4"
sluice = "0.5"
url = "2.2"
waker-fn = "1"

[dependencies.chrono]
44 changes: 2 additions & 42 deletions src/handler.rs
Original file line number Diff line number Diff line change
@@ -2,21 +2,19 @@

use crate::{
headers,
redirect::RedirectUri,
response::{EffectiveUri, LocalAddr, RemoteAddr},
response::{LocalAddr, RemoteAddr},
Body, Error, Metrics,
};
use crossbeam_utils::atomic::AtomicCell;
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()?;
187 changes: 121 additions & 66 deletions src/redirect.rs
Original file line number Diff line number Diff line change
@@ -3,115 +3,170 @@ 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;

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)),
}
}
4 changes: 1 addition & 3 deletions src/response.rs
Original file line number Diff line number Diff line change
@@ -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);
30 changes: 30 additions & 0 deletions tests/redirects.rs
Original file line number Diff line number Diff line change
@@ -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() {

0 comments on commit 05ca1de

Please sign in to comment.