From f20afba57d6fabb04085968342e5fd62b45bc8df Mon Sep 17 00:00:00 2001 From: Josh Leeb-du Toit Date: Sat, 9 Jun 2018 10:31:22 +1000 Subject: [PATCH] feat(http2): Strip connection headers before sending Automatically removes "connection" headers before sending over HTTP2. These headers are illegal in HTTP2, and would otherwise cause errors. Closes: #1551 --- src/proto/h2/mod.rs | 81 +++++++++++++++++++++++++++++--------------- tests/integration.rs | 78 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 27 deletions(-) diff --git a/src/proto/h2/mod.rs b/src/proto/h2/mod.rs index 2cfb475f23..167adf4f74 100644 --- a/src/proto/h2/mod.rs +++ b/src/proto/h2/mod.rs @@ -1,10 +1,13 @@ use bytes::Buf; use futures::{Async, Future, Poll}; use h2::{Reason, SendStream}; +use http::header::{ + HeaderName, CONNECTION, PROXY_AUTHENTICATE, PROXY_AUTHORIZATION, TE, TRAILER, + TRANSFER_ENCODING, UPGRADE, +}; use http::HeaderMap; -use http::header::{CONNECTION, TRANSFER_ENCODING}; -use ::body::Payload; +use body::Payload; mod client; mod server; @@ -13,13 +16,42 @@ pub(crate) use self::client::Client; pub(crate) use self::server::Server; fn strip_connection_headers(headers: &mut HeaderMap) { - if headers.remove(TRANSFER_ENCODING).is_some() { - trace!("removed illegal Transfer-Encoding header"); + // List of connection headers from: + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection + let connection_headers = [ + HeaderName::from_lowercase(b"keep-alive").unwrap(), + HeaderName::from_lowercase(b"proxy-connection").unwrap(), + PROXY_AUTHENTICATE, + PROXY_AUTHORIZATION, + TE, + TRAILER, + TRANSFER_ENCODING, + UPGRADE, + ]; + + for header in connection_headers.iter() { + if headers.remove(header).is_some() { + warn!("Connection header illegal in HTTP/2: {}", header.as_str()); + } } - if headers.contains_key(CONNECTION) { - warn!("Connection header illegal in HTTP/2"); - //TODO: actually remove it, after checking the value - //and removing all related headers + + if let Some(header) = headers.remove(CONNECTION) { + warn!( + "Connection header illegal in HTTP/2: {}", + CONNECTION.as_str() + ); + let header_contents = header.to_str().unwrap(); + + // A `Connection` header may have a comma-separated list of names of other headers that + // are meant for only this specific connection. + // + // Iterate these names and remove them as headers. Connection-specific headers are + // forbidden in HTTP2, as that information has been moved into frame types of the h2 + // protocol. + for name in header_contents.split(',') { + let name = name.trim(); + headers.remove(name); + } } } @@ -55,7 +87,8 @@ where fn send_eos_frame(&mut self) -> ::Result<()> { trace!("send body eos"); - self.body_tx.send_data(SendBuf(None), true) + self.body_tx + .send_data(SendBuf(None), true) .map_err(::Error::new_body_write) } } @@ -94,13 +127,14 @@ where ); let buf = SendBuf(Some(chunk)); - self.body_tx.send_data(buf, is_eos) + self.body_tx + .send_data(buf, is_eos) .map_err(::Error::new_body_write)?; if is_eos { - return Ok(Async::Ready(())) + return Ok(Async::Ready(())); } - }, + } None => { let is_eos = self.stream.is_end_stream(); if is_eos { @@ -109,19 +143,20 @@ where self.data_done = true; // loop again to poll_trailers } - }, + } } } else { match try_ready!(self.stream.poll_trailers().map_err(|e| self.on_err(e))) { Some(trailers) => { - self.body_tx.send_trailers(trailers) + self.body_tx + .send_trailers(trailers) .map_err(::Error::new_body_write)?; return Ok(Async::Ready(())); - }, + } None => { // There were no trailers, so send an empty DATA frame... return self.send_eos_frame().map(Async::Ready); - }, + } } } } @@ -133,24 +168,16 @@ struct SendBuf(Option); impl Buf for SendBuf { #[inline] fn remaining(&self) -> usize { - self.0 - .as_ref() - .map(|b| b.remaining()) - .unwrap_or(0) + self.0.as_ref().map(|b| b.remaining()).unwrap_or(0) } #[inline] fn bytes(&self) -> &[u8] { - self.0 - .as_ref() - .map(|b| b.bytes()) - .unwrap_or(&[]) + self.0.as_ref().map(|b| b.bytes()).unwrap_or(&[]) } #[inline] fn advance(&mut self, cnt: usize) { - self.0 - .as_mut() - .map(|b| b.advance(cnt)); + self.0.as_mut().map(|b| b.advance(cnt)); } } diff --git a/tests/integration.rs b/tests/integration.rs index 16592877d6..eb39672765 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -62,6 +62,84 @@ t! { ; } +t! { + get_strip_connection_header, + client: + request: + uri: "/", + ; + response: + status: 200, + headers: { + // h2 doesn't actually receive the connection header + }, + body: "hello world", + ; + server: + request: + uri: "/", + ; + response: + headers: { + // http2 should strip this header + "connection" => "close", + }, + body: "hello world", + ; +} + +t! { + get_strip_keep_alive_header, + client: + request: + uri: "/", + ; + response: + status: 200, + headers: { + // h2 doesn't actually receive the keep-alive header + }, + body: "hello world", + ; + server: + request: + uri: "/", + ; + response: + headers: { + // http2 should strip this header + "keep-alive" => "timeout=5, max=1000", + }, + body: "hello world", + ; +} + +t! { + get_strip_upgrade_header, + client: + request: + uri: "/", + ; + response: + status: 200, + headers: { + // h2 doesn't actually receive the upgrade header + }, + body: "hello world", + ; + server: + request: + uri: "/", + ; + response: + headers: { + // http2 should strip this header + "upgrade" => "h2c", + }, + body: "hello world", + ; +} + t! { get_body_chunked, client: