Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handling of 307 and 308 redirects #21

Merged
merged 3 commits into from
Nov 29, 2016
Merged

Add handling of 307 and 308 redirects #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a54447c
Add handling of 307 and 308 redirects
aidanhs Nov 17, 2016
3544e48
dont try to redirect if post body cannot be reset
seanmonstar Nov 28, 2016
dbfaf9c
add a note to about redirects
seanmonstar Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ pub struct Body {

impl Body {
/// Instantiate a `Body` from a reader.
///
/// # Note
///
/// While allowing for many types to be used, these bodies do not have
/// a way to reset to the beginning and be reused. This means that when
/// encountering a 307 or 308 status code, instead of repeating the
/// request at the new location, the `Response` will be returned with
/// the redirect status code set.
///
/// A `Body` constructed from a set of bytes, like `String` or `Vec<u8>`,
/// are stored differently and can be reused.
pub fn new<R: Read + 'static>(reader: R) -> Body {
Body {
reader: Kind::Reader(Box::new(reader), None),
Expand Down Expand Up @@ -115,3 +126,10 @@ pub fn as_hyper_body<'a>(body: &'a mut Body) -> ::hyper::client::Body<'a> {
}
}
}

pub fn can_reset(body: &Body) -> bool {
match body.reader {
Kind::Bytes(_) => true,
Kind::Reader(..) => false,
}
}
97 changes: 54 additions & 43 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,57 +198,68 @@ impl<'a> RequestBuilder<'a> {

try!(req.send())
};
body.take();

match res.status {
let should_redirect = match res.status {
StatusCode::MovedPermanently |
StatusCode::Found |
StatusCode::SeeOther => {

//TODO: turn this into self.redirect_policy.check()
if redirect_count > 10 {
return Err(::Error::TooManyRedirects);
}
redirect_count += 1;

method = match method {
Method::Post | Method::Put => Method::Get,
m => m
};

headers.set(Referer(url.to_string()));

let loc = {
let loc = res.headers.get::<Location>().map(|loc| url.join(loc));
if let Some(loc) = loc {
loc
} else {
return Ok(Response {
inner: res
});
body = None;
match method {
Method::Get | Method::Head => {},
_ => {
method = Method::Get;
}
};

url = match loc {
Ok(u) => u,
Err(e) => {
debug!("Location header had invalid URI: {:?}", e);
return Ok(Response {
inner: res
})
}
};
}
true
},
StatusCode::TemporaryRedirect |
StatusCode::PermanentRedirect => {
if let Some(ref body) = body {
body::can_reset(body)
} else {
true
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor personal preference, but you could write this as body.map(body::can_reset).unwrap_or(true).

},
_ => false,
};

debug!("redirecting to '{}'", url);
if should_redirect {
//TODO: turn this into self.redirect_policy.check()
if redirect_count > 10 {
return Err(::Error::TooManyRedirects);
}
redirect_count += 1;

headers.set(Referer(url.to_string()));

let loc = {
let loc = res.headers.get::<Location>().map(|loc| url.join(loc));
if let Some(loc) = loc {
loc
} else {
return Ok(Response {
inner: res
});
}
};

url = match loc {
Ok(u) => u,
Err(e) => {
debug!("Location header had invalid URI: {:?}", e);
return Ok(Response {
inner: res
})
}
};

//TODO: removeSensitiveHeaders(&mut headers, &url);
debug!("redirecting to {:?} '{}'", method, url);

},
_ => {
return Ok(Response {
inner: res
});
}
//TODO: removeSensitiveHeaders(&mut headers, &url);
} else {
return Ok(Response {
inner: res
});
}
}
}
Expand Down
157 changes: 121 additions & 36 deletions tests/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,45 +33,130 @@ fn test_get() {
}

#[test]
fn test_redirect_302_changes_post_to_get() {
fn test_redirect_301_and_302_and_303_changes_post_to_get() {
let client = reqwest::Client::new().unwrap();
let codes = [301, 302, 303];

let redirect = server! {
request: b"\
POST /302 HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Content-Length: 0\r\n\
\r\n\
",
response: b"\
HTTP/1.1 302 Found\r\n\
Server: test-redirect\r\n\
Content-Length: 0\r\n\
Location: /dst\r\n\
Connection: close\r\n\
\r\n\
",
for code in codes.iter() {
let redirect = server! {
request: format!("\
POST /{} HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Content-Length: 0\r\n\
\r\n\
", code),
response: format!("\
HTTP/1.1 {} reason\r\n\
Server: test-redirect\r\n\
Content-Length: 0\r\n\
Location: /dst\r\n\
Connection: close\r\n\
\r\n\
", code),

request: b"\
GET /dst HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Referer: http://$HOST/302\r\n\
\r\n\
",
response: b"\
HTTP/1.1 200 OK\r\n\
Server: test-dst\r\n\
Content-Length: 0\r\n\
\r\n\
"
};
request: format!("\
GET /dst HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Referer: http://$HOST/{}\r\n\
\r\n\
", code),
response: b"\
HTTP/1.1 200 OK\r\n\
Server: test-dst\r\n\
Content-Length: 0\r\n\
\r\n\
"
};

let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
.send()
.unwrap();
assert_eq!(res.status(), &reqwest::StatusCode::Ok);
assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
}
}

#[test]
fn test_redirect_307_and_308_tries_to_post_again() {
let client = reqwest::Client::new().unwrap();
let res = client.post(&format!("http://{}/302", redirect.addr()))
.send()
.unwrap();
assert_eq!(res.status(), &reqwest::StatusCode::Ok);
assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
let codes = [307, 308];
for code in codes.iter() {
let redirect = server! {
request: format!("\
POST /{} HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Content-Length: 5\r\n\
\r\n\
Hello\
", code),
response: format!("\
HTTP/1.1 {} reason\r\n\
Server: test-redirect\r\n\
Content-Length: 0\r\n\
Location: /dst\r\n\
Connection: close\r\n\
\r\n\
", code),

request: format!("\
POST /dst HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Referer: http://$HOST/{}\r\n\
Content-Length: 5\r\n\
\r\n\
Hello\
", code),
response: b"\
HTTP/1.1 200 OK\r\n\
Server: test-dst\r\n\
Content-Length: 0\r\n\
\r\n\
"
};

let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
.body("Hello")
.send()
.unwrap();
assert_eq!(res.status(), &reqwest::StatusCode::Ok);
assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
}
}

#[test]
fn test_redirect_307_does_not_try_if_reader_cannot_reset() {
let client = reqwest::Client::new().unwrap();
let codes = [307, 308];
for &code in codes.iter() {
let redirect = server! {
request: format!("\
POST /{} HTTP/1.1\r\n\
Host: $HOST\r\n\
User-Agent: $USERAGENT\r\n\
Transfer-Encoding: chunked\r\n\
\r\n\
5\r\n\
Hello\r\n\
0\r\n\r\n\
", code),
response: format!("\
HTTP/1.1 {} reason\r\n\
Server: test-redirect\r\n\
Content-Length: 0\r\n\
Location: /dst\r\n\
Connection: close\r\n\
\r\n\
", code)
};

let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
.body(reqwest::Body::new(&b"Hello"[..]))
.send()
.unwrap();
assert_eq!(res.status(), &reqwest::StatusCode::from_u16(code));
}
}