fix(server): fix Content-Lenth/Transfer-Encoding inference

Fixes #2427

204s must never have either Content-Length or Transfer-Encoding headers.
Nor should any 1xx response. Nor should any 2xx response made to a
CONNECT request.

On the other hand, any other kind of response may have either a
Content-Length or Transfer-Encoding header.  This includes 304s in
general, and any response made to a HEAD request. In those of those
cases, the headers should match what would have been sent with a normal
response.

The trick then is to ensure that such headers are not mis-inferred. When
responding to a HEAD request, a service may reasonably opt out of
computing the full response, and thus may not know how large it would
be. To add automatically add `Content-Length: 0` would be a mistake.

As Body::empty() seems to be the defacto "no response" Body, it shall be
used as such.  If a service responds to a HEAD request, or to a
conditional GET request with a 304, it may specify Body::empty() as the
body, and no Content-Length or Transfer-Encoding header will be added.
In all other cases when a Content-Length header is required for HTTP
message framing, `Content-Length: 0` will be still automatically added.

Body::from("") on the other hand will now implie a specific empty Body.
It will continue to imply `Content-Length: 0` in all cases, now
including a 304 response.

Either Content-Length or Transfer-Encoding may be added explicitly as
headers for either HEAD requests or 304 responses.

Author: jeddenlea

rustfmt.toml

@@ -0,0 +1 @@
+edition = "2018"

src/body/body.rs

@@ -480,7 +480,7 @@ impl From<Bytes> for Body {
     #[inline]
     fn from(chunk: Bytes) -> Body {
         if chunk.is_empty() {
-            Body::empty()
+            Body::new(Kind::Once(Some(Bytes::new()))) // drop chunk ASAP
         } else {
             Body::new(Kind::Once(Some(chunk)))
         }

src/proto/h1/role.rs

@@ -385,7 +385,7 @@ impl Http1Transaction for Server {
             }};
         }
 
-        'headers: for (opt_name, value) in msg.head.headers.drain() {
+        for (opt_name, value) in msg.head.headers.drain() {
             if let Some(n) = opt_name {
                 cur_name = Some(n);
                 handle_is_name_written!();
@@ -399,6 +399,14 @@ impl Http1Transaction for Server {
                         rewind(dst);
                         return Err(crate::Error::new_user_header());
                     }
+                    // Can this response even contain Content-Length?
+                    if !Server::can_have_body(msg.req_method, msg.head.subject) {
+                        warn!(
+                            "illegal content-length ({:?}) seen for {:?} response to {:?} request",
+                            value, msg.head.subject, msg.req_method
+                        );
+                        continue;
+                    }
                     match msg.body {
                         Some(BodyLength::Known(known_len)) => {
                             // The HttpBody claims to know a length, and
@@ -421,13 +429,15 @@ impl Http1Transaction for Server {
                             }
 
                             if !is_name_written {
-                                encoder = Encoder::length(known_len);
+                                if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                                    encoder = Encoder::length(known_len);
+                                }
                                 extend(dst, b"content-length: ");
                                 extend(dst, value.as_bytes());
                                 wrote_len = true;
                                 is_name_written = true;
                             }
-                            continue 'headers;
+                            continue;
                         }
                         Some(BodyLength::Unknown) => {
                             // The HttpBody impl didn't know how long the
@@ -446,16 +456,18 @@ impl Http1Transaction for Server {
                                         return Err(crate::Error::new_user_header());
                                     }
                                     debug_assert!(is_name_written);
-                                    continue 'headers;
+                                    continue;
                                 } else {
                                     // we haven't written content-length yet!
-                                    encoder = Encoder::length(len);
+                                    if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                                        encoder = Encoder::length(len);
+                                    }
                                     extend(dst, b"content-length: ");
                                     extend(dst, value.as_bytes());
                                     wrote_len = true;
                                     is_name_written = true;
                                     prev_con_len = Some(len);
-                                    continue 'headers;
+                                    continue;
                                 }
                             } else {
                                 warn!("illegal Content-Length value: {:?}", value);
@@ -464,13 +476,13 @@ impl Http1Transaction for Server {
                             }
                         }
                         None => {
-                            // We have no body to actually send,
-                            // but the headers claim a content-length.
-                            // There's only 2 ways this makes sense:
+                            // We have no body to actually send, but the headers claim a
+                            // content-length.  There are 3 ways this makes sense:
                             //
                             // - The header says the length is `0`.
+                            // - The response is NOT_MODIFIED
                             // - This is a response to a `HEAD` request.
-                            if msg.req_method == &Some(Method::HEAD) {
+                            if Server::imply_body_only(msg.req_method, msg.head.subject) {
                                 debug_assert_eq!(encoder, Encoder::length(0));
                             } else {
                                 if value.as_bytes() != b"0" {
@@ -479,7 +491,7 @@ impl Http1Transaction for Server {
                                         value
                                     );
                                 }
-                                continue 'headers;
+                                continue;
                             }
                         }
                     }
@@ -492,9 +504,13 @@ impl Http1Transaction for Server {
                         return Err(crate::Error::new_user_header());
                     }
                     // check that we actually can send a chunked body...
-                    if msg.head.version == Version::HTTP_10
-                        || !Server::can_chunked(msg.req_method, msg.head.subject)
-                    {
+                    if msg.head.version == Version::HTTP_10 {
+                        continue;
+                    } else if !Server::can_have_body(msg.req_method, msg.head.subject) {
+                        warn!(
+                            "illegal transfer-encoding ({:?}) seen for {:?} response to {:?} request",
+                            value, msg.head.subject, msg.req_method
+                        );
                         continue;
                     }
                     wrote_len = true;
@@ -503,15 +519,17 @@ impl Http1Transaction for Server {
                     must_write_chunked = !headers::is_chunked_(&value);
 
                     if !is_name_written {
-                        encoder = Encoder::chunked();
+                        if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                            encoder = Encoder::chunked();
+                        }
                         is_name_written = true;
                         extend(dst, b"transfer-encoding: ");
                         extend(dst, value.as_bytes());
                     } else {
                         extend(dst, b", ");
                         extend(dst, value.as_bytes());
                     }
-                    continue 'headers;
+                    continue;
                 }
                 header::CONNECTION => {
                     if !is_last && headers::connection_close(&value) {
@@ -525,7 +543,7 @@ impl Http1Transaction for Server {
                         extend(dst, b", ");
                         extend(dst, value.as_bytes());
                     }
-                    continue 'headers;
+                    continue;
                 }
                 header::DATE => {
                     wrote_date = true;
@@ -549,44 +567,50 @@ impl Http1Transaction for Server {
 
         handle_is_name_written!();
 
-        if !wrote_len {
-            encoder = match msg.body {
-                Some(BodyLength::Unknown) => {
-                    if msg.head.version == Version::HTTP_10
-                        || !Server::can_chunked(msg.req_method, msg.head.subject)
-                    {
-                        Encoder::close_delimited()
-                    } else {
-                        extend(dst, b"transfer-encoding: chunked\r\n");
-                        Encoder::chunked()
-                    }
+        // We should provide either a Content-Length or Transfer-Encoding header based on what we
+        // know about the Body, unless:
+        //   - the request was HEAD and the body is Body::empty(), because a service may simply not
+        //     provide generate the real body for a HEAD request.
+        //   - the response is a 304, like a HEAD, may imply things about a particular body, but a
+        //     service probably did not provide the full body in its Response.
+        //   - the response must neither contain nor imply a body.
+        if !wrote_len && Server::can_have_body(msg.req_method, msg.head.subject) {
+            match msg.body {
+                Some(BodyLength::Known(0)) => extend(dst, b"content-length: 0\r\n"),
+                Some(BodyLength::Known(len)) => {
+                    extend(dst, b"content-length: ");
+                    let _ = ::itoa::write(&mut dst, len);
+                    extend(dst, b"\r\n");
                 }
-                None | Some(BodyLength::Known(0)) => {
-                    if msg.head.subject != StatusCode::NOT_MODIFIED {
+                None => {
+                    if !Server::imply_body_only(msg.req_method, msg.head.subject) {
                         extend(dst, b"content-length: 0\r\n");
                     }
-                    Encoder::length(0)
                 }
-                Some(BodyLength::Known(len)) => {
-                    if msg.head.subject == StatusCode::NOT_MODIFIED {
-                        Encoder::length(0)
-                    } else {
-                        extend(dst, b"content-length: ");
-                        let _ = ::itoa::write(&mut dst, len);
-                        extend(dst, b"\r\n");
-                        Encoder::length(len)
+                Some(BodyLength::Unknown) => {
+                    if msg.head.version != Version::HTTP_10 {
+                        extend(dst, b"transfer-encoding: chunked\r\n");
                     }
                 }
             };
+
+            if !Server::imply_body_only(msg.req_method, msg.head.subject) {
+                encoder = match msg.body {
+                    Some(BodyLength::Unknown) if msg.head.version == Version::HTTP_10 => {
+                        Encoder::close_delimited()
+                    }
+                    Some(BodyLength::Unknown) => Encoder::chunked(),
+                    Some(BodyLength::Known(len)) => Encoder::length(len),
+                    None => Encoder::length(0),
+                };
+            }
         }
 
-        if !Server::can_have_body(msg.req_method, msg.head.subject) {
-            trace!(
-                "server body forced to 0; method={:?}, status={:?}",
-                msg.req_method,
-                msg.head.subject
-            );
-            encoder = Encoder::length(0);
+        #[cfg(debug_assertions)]
+        if Server::imply_body_only(msg.req_method, msg.head.subject)
+            || !Server::can_have_body(msg.req_method, msg.head.subject)
+        {
+            assert_eq!(encoder, Encoder::length(0));
         }
 
         // cached date is much faster than formatting every request
@@ -631,24 +655,19 @@ impl Http1Transaction for Server {
 #[cfg(feature = "server")]
 impl Server {
     fn can_have_body(method: &Option<Method>, status: StatusCode) -> bool {
-        Server::can_chunked(method, status)
-    }
-
-    fn can_chunked(method: &Option<Method>, status: StatusCode) -> bool {
-        if method == &Some(Method::HEAD) || method == &Some(Method::CONNECT) && status.is_success()
+        if method == &Some(Method::CONNECT) && status.is_success()
+            || status == StatusCode::NO_CONTENT
+            || status.is_informational()
         {
             false
         } else {
-            match status {
-                // TODO: support for 1xx codes needs improvement everywhere
-                // would be 100...199 => false
-                StatusCode::SWITCHING_PROTOCOLS
-                | StatusCode::NO_CONTENT
-                | StatusCode::NOT_MODIFIED => false,
-                _ => true,
-            }
+            true
         }
     }
+
+    fn imply_body_only(method: &Option<Method>, status: StatusCode) -> bool {
+        method == &Some(Method::HEAD) || status == StatusCode::NOT_MODIFIED
+    }
 }
 
 #[cfg(feature = "client")]

tests/client.rs

@@ -359,6 +359,25 @@ test! {
         expected: "GET / HTTP/1.1\r\nhost: {addr}\r\n\r\n",
         reply: REPLY_OK,
 
+    client:
+        request: {
+            method: GET,
+            url: "http://{addr}/",
+            // body is Body::empty
+        },
+        response:
+            status: OK,
+            headers: {},
+            body: None,
+}
+
+test! {
+    name: client_get_req_body_explicitly_empty,
+
+    server:
+        expected: "GET / HTTP/1.1\r\nhost: {addr}\r\ncontent-length: 0\r\n\r\n",
+        reply: REPLY_OK,
+
     client:
         request: {
             method: GET,