diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index e4cb11545b..902f93beaf 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -781,31 +781,44 @@ impl Client { impl Client { fn set_length(head: &mut RequestHead, body: Option) -> 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) @@ -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 } @@ -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 } } diff --git a/tests/client.rs b/tests/client.rs index 54d7e1d650..b2d5fa08a5 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -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", @@ -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, } @@ -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,