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

Closes #1925

src/proto/h1/role.rs

@@ -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.
+
+        // 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;
+
+        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)
+            };
         }
-    }
-}
 
-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.
-
-    let existing_con_len = headers::content_length_parse_all(headers);
-    let mut should_remove_con_len = false;
-
-    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)
@@ -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
                 }
@@ -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
     }
 }
 

tests/client.rs

@@ -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",
@@ -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,
 }
 
@@ -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,