Repository: hyperium/h2 (rust)

Issue:
Strip connection level header fields
> Such intermediaries SHOULD also remove other connection-specific header fields, such as Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection header field.

If a header frame is sent that includes a `Connection` field, it should be stripped including all fields referenced by the `Connection` field value.


Discussion:
Proxies should do that, not the library. The proxies need to inspect those headers.
Either way, the library has to check if those headers are set and error.
How so? A proxy can receive those headers, and make some decisions based on them. And then it can build some new headers, that might include the same names, for the next hop.
AIUI, connection headers are forbidden in h2:

> HTTP/2 does not use the Connection header field to indicate connection-specific header fields; in this protocol, connection-specific metadata is conveyed by other means. An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed (Section 8.1.2.6).

The section quoted above applies to endpoints that upgrade h1 to h2:

> This means that an intermediary transforming an HTTP/1.x message to HTTP/2 will need to remove any header fields nominated by the Connection header field, along with the Connection header field itself. Such intermediaries SHOULD also remove other connection-specific header fields, such as Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection header field.



Patch:
diff --git a/src/codec/error.rs b/src/codec/error.rs
--- a/src/codec/error.rs
+++ b/src/codec/error.rs
@@ -44,6 +44,9 @@ pub enum UserError {
     ///
     /// A new connection is needed.
     OverflowedStreamId,
+
+    /// Illegal headers, such as connection-specific headers.
+    MalformedHeaders,
 }
 
 // ===== impl RecvError =====
@@ -121,6 +124,7 @@ impl error::Error for UserError {
             Rejected => "rejected",
             ReleaseCapacityTooBig => "release capacity too big",
             OverflowedStreamId => "stream ID overflowed",
+            MalformedHeaders => "malformed headers",
         }
     }
 }
diff --git a/src/frame/headers.rs b/src/frame/headers.rs
--- a/src/frame/headers.rs
+++ b/src/frame/headers.rs
@@ -635,7 +635,12 @@ impl HeaderBlock {
                     // Connection level header fields are not supported and must
                     // result in a protocol error.
 
-                    if name == header::CONNECTION {
+                    if name == header::CONNECTION
+                        || name == header::TRANSFER_ENCODING
+                        || name == header::UPGRADE
+                        || name == "keep-alive"
+                        || name == "proxy-connection"
+                    {
                         trace!("load_hpack; connection level header");
                         malformed = true;
                     } else if name == header::TE && value != "trailers" {
diff --git a/src/proto/streams/send.rs b/src/proto/streams/send.rs
--- a/src/proto/streams/send.rs
+++ b/src/proto/streams/send.rs
@@ -1,3 +1,4 @@
+use http;
 use super::*;
 use codec::{RecvError, UserError};
 use codec::UserError::*;
@@ -56,6 +57,25 @@ impl Send {
             self.init_window_sz
         );
 
+        // 8.1.2.2. Connection-Specific Header Fields
+        if frame.fields().contains_key(http::header::CONNECTION)
+            || frame.fields().contains_key(http::header::TRANSFER_ENCODING)
+            || frame.fields().contains_key(http::header::UPGRADE)
+            || frame.fields().contains_key("keep-alive")
+            || frame.fields().contains_key("proxy-connection")
+        {
+            debug!("illegal connection-specific headers found");
+            return Err(UserError::MalformedHeaders);
+        } else if let Some(te) = frame.fields().get(http::header::TE) {
+            if te != "trailers" {
+                debug!("illegal connection-specific headers found");
+                return Err(UserError::MalformedHeaders);
+
+            }
+        }
+
+
+
         let end_stream = frame.is_end_stream();
 
         // Update the state


Test patch:
diff --git a/tests/client_request.rs b/tests/client_request.rs
--- a/tests/client_request.rs
+++ b/tests/client_request.rs
@@ -281,6 +281,43 @@ fn request_without_scheme() {}
 #[ignore]
 fn request_with_h1_version() {}
 
+#[test]
+fn request_with_connection_headers() {
+    let _ = ::env_logger::init();
+    let (io, srv) = mock::new();
+
+    let srv = srv.assert_client_handshake()
+        .unwrap()
+        .recv_settings()
+        .close();
+
+    let headers = vec![
+        ("connection", "foo"),
+        ("keep-alive", "5"),
+        ("proxy-connection", "bar"),
+        ("transfer-encoding", "chunked"),
+        ("upgrade", "HTTP/2.0"),
+        ("te", "boom"),
+    ];
+
+    let client = Client::handshake(io)
+        .expect("handshake")
+        .and_then(move |(mut client, conn)| {
+            for (name, val) in headers {
+                let req = Request::builder()
+                    .uri("https://http2.akamai.com/")
+                    .header(name, val)
+                    .body(())
+                    .unwrap();
+                let err = client.send_request(req, true).expect_err(name);
+
+                assert_eq!(err.to_string(), "user error: malformed headers");
+            }
+            conn.unwrap()
+        });
+
+    client.join(srv).wait().expect("wait");
+}
 
 #[test]
 fn sending_request_on_closed_connection() {
diff --git a/tests/server.rs b/tests/server.rs
--- a/tests/server.rs
+++ b/tests/server.rs
@@ -110,7 +110,7 @@ fn serve_request() {
 fn accept_with_pending_connections_after_socket_close() {}
 
 #[test]
-fn sent_invalid_authority() {
+fn recv_invalid_authority() {
     let _ = ::env_logger::init();
     let (io, client) = mock::new();
 
@@ -136,6 +136,41 @@ fn sent_invalid_authority() {
     srv.join(client).wait().expect("wait");
 }
 
+#[test]
+fn recv_connection_header() {
+    let _ = ::env_logger::init();
+    let (io, client) = mock::new();
+
+    let req = |id, name, val| {
+        frames::headers(id)
+            .request("GET", "https://example.com/")
+            .field(name, val)
+            .eos()
+    };
+
+    let client = client
+        .assert_server_handshake()
+        .unwrap()
+        .recv_settings()
+        .send_frame(req(1, "connection", "foo"))
+        .send_frame(req(3, "keep-alive", "5"))
+        .send_frame(req(5, "proxy-connection", "bar"))
+        .send_frame(req(7, "transfer-encoding", "chunked"))
+        .send_frame(req(9, "upgrade", "HTTP/2.0"))
+        .recv_frame(frames::reset(1).protocol_error())
+        .recv_frame(frames::reset(3).protocol_error())
+        .recv_frame(frames::reset(5).protocol_error())
+        .recv_frame(frames::reset(7).protocol_error())
+        .recv_frame(frames::reset(9).protocol_error())
+        .close();
+
+    let srv = Server::handshake(io)
+        .expect("handshake")
+        .and_then(|srv| srv.into_future().unwrap());
+
+    srv.join(client).wait().expect("wait");
+}
+
 #[test]
 fn sends_reset_cancel_when_body_is_dropped() {
     let _ = ::env_logger::init();
diff --git a/tests/support/frames.rs b/tests/support/frames.rs
--- a/tests/support/frames.rs
+++ b/tests/support/frames.rs
@@ -126,6 +126,17 @@ impl Mock<frame::Headers> {
         Mock(frame)
     }
 
+    pub fn field<K, V>(self, key: K, value: V) -> Self
+    where
+        K: HttpTryInto<http::header::HeaderName>,
+        V: HttpTryInto<http::header::HeaderValue>,
+    {
+        let (id, pseudo, mut fields) = self.into_parts();
+        fields.insert(key.try_into().unwrap(), value.try_into().unwrap());
+        let frame = frame::Headers::new(id, pseudo, fields);
+        Mock(frame)
+    }
+
     pub fn eos(mut self) -> Self {
         self.0.set_end_stream();
         self