http1.1 server allows missing and duplicate Host header

[littledivy]

Hyper currently allows processing requests without and with multiple Host headers.

As per rfc7230 3.3.3:

A server MUST respond with a 400 (Bad Request) status code to any HTTP/1.1 request message that lacks a Host header field and to any request message that contains more than one Host header field or a Host header field with an invalid field-value

I'm guessing the spec deviation is intentional to support some legacy clients. Would you be open to adding a check_host_header option?

diff --git a/src/error.rs b/src/error.rs
index 9ad4c0e5..229b6274 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -113,6 +113,10 @@ pub(super) enum Header {
     ContentLengthInvalid,
     #[cfg(feature = "server")]
     TransferEncodingInvalid,
+    #[cfg(feature = "server")]
+    HostMissing,
+    #[cfg(feature = "server")]
+    HostDuplicate,
     #[cfg(any(feature = "client", feature = "server"))]
     TransferEncodingUnexpected,
 }
@@ -435,6 +439,10 @@ impl Error {
             Kind::Parse(Parse::Header(Header::TransferEncodingInvalid)) => {
                 "invalid transfer-encoding parsed"
             }
+            #[cfg(all(feature = "http1", feature = "server"))]
+            Kind::Parse(Parse::Header(Header::HostMissing)) => "missing host header",
+            #[cfg(all(feature = "http1", feature = "server"))]
+            Kind::Parse(Parse::Header(Header::HostDuplicate)) => "duplicate host header",
             #[cfg(all(feature = "http1", any(feature = "client", feature = "server")))]
             Kind::Parse(Parse::Header(Header::TransferEncodingUnexpected)) => {
                 "unexpected transfer-encoding parsed"
@@ -557,6 +565,16 @@ impl Parse {
         Parse::Header(Header::TransferEncodingInvalid)
     }

+    #[cfg(feature = "server")]
+    pub(crate) fn host_missing() -> Self {
+        Parse::Header(Header::HostMissing)
+    }
+
+    #[cfg(feature = "server")]
+    pub(crate) fn host_duplicate() -> Self {
+        Parse::Header(Header::HostDuplicate)
+    }
+
     #[cfg(any(feature = "client", feature = "server"))]
     pub(crate) fn transfer_encoding_unexpected() -> Self {
         Parse::Header(Header::TransferEncodingUnexpected)
diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs
index 4f04acec..5c22518f 100644
--- a/src/proto/h1/role.rs
+++ b/src/proto/h1/role.rs
@@ -229,6 +229,7 @@ impl Http1Transaction for Server {
         let mut con_len = None;
         let mut is_te = false;
         let mut is_te_chunked = false;
+        let mut has_host = false;
         let mut wants_upgrade = subject.0 == Method::CONNECT;

         let mut header_case_map = if ctx.preserve_header_case {
@@ -312,6 +313,13 @@ impl Http1Transaction for Server {
                     // Upgrades are only allowed with HTTP/1.1
                     wants_upgrade = is_http_11;
                 }
+                header::HOST => {
+                    if has_host {
+                        debug!("multiple Host headers");
+                        return Err(Parse::host_duplicate());
+                    }
+                    has_host = true;
+                }

                 _ => (),
             }
@@ -333,6 +341,11 @@ impl Http1Transaction for Server {
             return Err(Parse::transfer_encoding_invalid());
         }

+        if !has_host {
+            debug!("request without host header");
+            return Err(Parse::host_missing());
+        }
+
         let mut extensions = http::Extensions::default();

         if let Some(header_case_map) = header_case_map {

As prior art: Node.js changed it's behavior to follow spec strictly and added option to disable host header checks.

[seanmonstar]

I'm guessing the spec deviation is intentional to support some legacy clients.

I want to say it was on purpose. But my memory is hazy on the subject (sigh, these things should be documented). I think while working on linkerd, we wanted a transparent proxy that only rejected things that were horribly wrong, but otherwise, you shouldn't notice it. If a system was working with wonky host headers, who were we to break it?

It also felt like one of those requirements that could be validated by the user code if they cared, but if they didn't, why should we spend the processing on it? As opposed to message size headers, which affect framing, host headers seemed less consequential. If your server cares about the host, then most likely you'll have to handle a missing host anyways.

Additionally, with the way RFC 9113 talks about allowed missing :authority fields, and that :authority should be used if translating to HTTP/1, it seemed like there could be instances where there is no host header. Indeed, the part you pointed out (which still exists in the newer 9110/9112), it sounds like even if you don't know a host, you should send host: with an empty value. Elsewhere in 9112, there's mention "If there is no Host header field or if its field value is empty or invalid, the target URI's authority component is empty."

So, in short, I don't know. Could use more discussion. If it must be fixed in hyper, we can. If there isn't a demanding reason, then at least we should document that.