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

Jump to bottom

fix(client): allow client GET requests with explicit body headers #1926

Merged
merged 1 commit into from Sep 4, 2019
Merged

fix(client): allow client GET requests with explicit body headers #1926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 48 additions & 43 deletions src/proto/h1/role.rs
View file
Open in desktop
Expand Up @@ -781,31 +781,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 @@ -841,9 +854,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 @@ -869,27 +895,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
Open in desktop
Expand Up @@ -356,7 +356,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 @@ -370,9 +370,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 @@ -434,6 +578,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