Skip to content

Commit

Permalink
fix(client): allow client GET requests with explicit body headers
Browse files Browse the repository at this point in the history 
Closes #1925
  • Loading branch information
@seanmonstar
seanmonstar committed Sep 4, 2019
1 parent ac45f1a commit 0867ad5
Show file tree
Hide file tree
Showing 2 changed files with 221 additions and 45 deletions.
91 changes: 48 additions & 43 deletions src/proto/h1/role.rs
View file
Expand Up @@ -780,31 +780,44 @@ impl Client {

impl Client {
fn set_length(head: &mut RequestHead, body: Option<BodyLength>) -> Encoder {
if let Some(body) = body {
let can_chunked = head.version == Version::HTTP_11
&& (head.subject.0 != Method::HEAD)
&& (head.subject.0 != Method::GET)
&& (head.subject.0 != Method::CONNECT);
set_length(&mut head.headers, body, can_chunked)
let body = if let Some(body) = body {
body
} else {
head.headers.remove(header::TRANSFER_ENCODING);
Encoder::length(0)
}
}
}
return Encoder::length(0)
};

// HTTP/1.0 doesn't know about chunked
let can_chunked = head.version == Version::HTTP_11;
let headers = &mut head.headers;

// If the user already set specific headers, we should respect them, regardless
// of what the Payload knows about itself. They set them for a reason.

fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> Encoder {
// If the user already set specific headers, we should respect them, regardless
// of what the Payload knows about itself. They set them for a reason.
// Because of the borrow checker, we can't check the for an existing
// Content-Length header while holding an `Entry` for the Transfer-Encoding
// header, so unfortunately, we must do the check here, first.

// Because of the borrow checker, we can't check the for an existing
// Content-Length header while holding an `Entry` for the Transfer-Encoding
// header, so unfortunately, we must do the check here, first.
let existing_con_len = headers::content_length_parse_all(headers);
let mut should_remove_con_len = false;

let existing_con_len = headers::content_length_parse_all(headers);
let mut should_remove_con_len = false;
if !can_chunked {
// Chunked isn't legal, so if it is set, we need to remove it.
if headers.remove(header::TRANSFER_ENCODING).is_some() {
trace!("removing illegal transfer-encoding header");
}

return if let Some(len) = existing_con_len {
Encoder::length(len)
} else if let BodyLength::Known(len) = body {
set_content_length(headers, len)
} else {
// HTTP/1.0 client requests without a content-length
// cannot have any body at all.
Encoder::length(0)
};
}

if can_chunked {
// If the user set a transfer-encoding, respect that. Let's just
// make sure `chunked` is the final encoding.
let encoder = match headers.entry(header::TRANSFER_ENCODING)
Expand Down Expand Up @@ -840,9 +853,22 @@ fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> E
if let Some(len) = existing_con_len {
Some(Encoder::length(len))
} else if let BodyLength::Unknown = body {
should_remove_con_len = true;
te.insert(HeaderValue::from_static("chunked"));
Some(Encoder::chunked())
// GET, HEAD, and CONNECT almost never have bodies.
//
// So instead of sending a "chunked" body with a 0-chunk,
// assume no body here. If you *must* send a body,
// set the headers explicitly.
match head.subject.0 {
Method::GET |
Method::HEAD |
Method::CONNECT => {
Some(Encoder::length(0))
},
_ => {
te.insert(HeaderValue::from_static("chunked"));
Some(Encoder::chunked())
},
}
} else {
None
}
Expand All @@ -868,27 +894,6 @@ fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> E
};

set_content_length(headers, len)
} else {
// Chunked isn't legal, so if it is set, we need to remove it.
// Also, if it *is* set, then we shouldn't replace with a length,
// since the user tried to imply there isn't a length.
let encoder = if headers.remove(header::TRANSFER_ENCODING).is_some() {
trace!("removing illegal transfer-encoding header");
should_remove_con_len = true;
Encoder::close_delimited()
} else if let Some(len) = existing_con_len {
Encoder::length(len)
} else if let BodyLength::Known(len) = body {
set_content_length(headers, len)
} else {
Encoder::close_delimited()
};

if should_remove_con_len && existing_con_len.is_some() {
headers.remove(header::CONTENT_LENGTH);
}

encoder
}
}

Expand Down
175 changes: 173 additions & 2 deletions tests/client.rs
View file
Expand Up @@ -353,7 +353,7 @@ test! {
}

test! {
name: client_get_implicitly_empty,
name: client_get_req_body_implicitly_empty,

server:
expected: "GET / HTTP/1.1\r\nhost: {addr}\r\n\r\n",
Expand All @@ -367,9 +367,153 @@ test! {
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_get_req_body_chunked,

server:
expected: "\
GET / HTTP/1.1\r\n\
transfer-encoding: chunked\r\n\
host: {addr}\r\n\
\r\n\
5\r\n\
hello\r\n\
0\r\n\r\n\
",
reply: REPLY_OK,

client:
request: {
method: GET,
url: "http://{addr}/",
headers: {
"Content-Length" => "0",
"transfer-encoding" => "chunked",
},
body: "hello", // not Body::empty
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_get_req_body_chunked_http10,

server:
expected: "\
GET / HTTP/1.0\r\n\
host: {addr}\r\n\
content-length: 5\r\n\
\r\n\
hello\
",
reply: "HTTP/1.0 200 OK\r\ncontent-length: 0\r\n\r\n",

client:
request: {
method: GET,
url: "http://{addr}/",
headers: {
"transfer-encoding" => "chunked",
},
version: HTTP_10,
body: "hello",
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_get_req_body_sized,

server:
expected: "\
GET / HTTP/1.1\r\n\
content-length: 5\r\n\
host: {addr}\r\n\
\r\n\
hello\
",
reply: REPLY_OK,

client:
request: {
method: GET,
url: "http://{addr}/",
headers: {
"Content-Length" => "5",
},
body: (Body::wrap_stream(Body::from("hello"))),
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_get_req_body_unknown,

server:
expected: "\
GET / HTTP/1.1\r\n\
host: {addr}\r\n\
\r\n\
",
reply: REPLY_OK,

client:
request: {
method: GET,
url: "http://{addr}/",
// wrap_steam means we don't know the content-length,
// but we're wrapping a non-empty stream.
//
// But since the headers cannot tell us, and the method typically
// doesn't have a body, the body must be ignored.
body: (Body::wrap_stream(Body::from("hello"))),
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_get_req_body_unknown_http10,

server:
expected: "\
GET / HTTP/1.0\r\n\
host: {addr}\r\n\
\r\n\
",
reply: "HTTP/1.0 200 OK\r\ncontent-length: 0\r\n\r\n",

client:
request: {
method: GET,
url: "http://{addr}/",
headers: {
"transfer-encoding" => "chunked",
},
version: HTTP_10,
// wrap_steam means we don't know the content-length,
// but we're wrapping a non-empty stream.
//
// But since the headers cannot tell us, the body must be ignored.
body: (Body::wrap_stream(Body::from("hello"))),
},
response:
status: OK,
headers: {},
body: None,
}

Expand Down Expand Up @@ -431,6 +575,33 @@ test! {
body: None,
}

test! {
name: client_post_unknown,

server:
expected: "\
POST /chunks HTTP/1.1\r\n\
host: {addr}\r\n\
transfer-encoding: chunked\r\n\
\r\n\
B\r\n\
foo bar baz\r\n\
0\r\n\r\n\
",
reply: REPLY_OK,

client:
request: {
method: POST,
url: "http://{addr}/chunks",
body: (Body::wrap_stream(Body::from("foo bar baz"))),
},
response:
status: OK,
headers: {},
body: None,
}

test! {
name: client_post_empty,

Expand Down

0 comments on commit 0867ad5

Please sign in to comment.