Client should validate request URI. (#181)

This patch adds checks for the request URI and rejects invalid URIs. In
the case of forwarding an HTTP 1.1 request with a path, an "http" pseudo
header is added to satisfy the HTTP/2.0 spec.

Closes #179

src/client.rs

@@ -1,12 +1,12 @@
 //! HTTP2 client side.
 use {SendStream, RecvStream, ReleaseCapacity};
-use codec::{Codec, RecvError};
+use codec::{Codec, RecvError, SendError, UserError};
 use frame::{Headers, Pseudo, Reason, Settings, StreamId};
 use proto;
 
 use bytes::{Bytes, IntoBuf};
 use futures::{Async, Future, MapErr, Poll};
-use http::{Request, Response};
+use http::{uri, Request, Response, Method, Version};
 use tokio_io::{AsyncRead, AsyncWrite};
 use tokio_io::io::WriteAll;
 
@@ -46,7 +46,11 @@ pub struct ResponseFuture {
 /// Build a Client.
 #[derive(Clone, Debug)]
 pub struct Builder {
+    /// Initial `Settings` frame to send as part of the handshake.
     settings: Settings,
+
+    /// The stream ID of the first (lowest) stream. Subsequent streams will use
+    /// monotonically increasing stream IDs.
     stream_id: StreamId,
 }
 
@@ -359,20 +363,12 @@ impl Future for ResponseFuture {
 
 // ===== impl Peer =====
 
-impl proto::Peer for Peer {
+impl Peer {
-    type Send = Request<()>;
+    pub fn convert_send_message(
-    type Poll = Response<()>;
+        id: StreamId,
-
+        request: Request<()>,
-
+        end_of_stream: bool) -> Result<Headers, SendError>
-    fn dyn() -> proto::DynPeer {
+    {
-        proto::DynPeer::Client
-    }
-
-    fn is_server() -> bool {
-        false
-    }
-
-    fn convert_send_message(id: StreamId, request: Self::Send, end_of_stream: bool) -> Headers {
         use http::request::Parts;
 
         let (
@@ -380,14 +376,45 @@ impl proto::Peer for Peer {
                 method,
                 uri,
                 headers,
+                version,
                 ..
             },
             _,
         ) = request.into_parts();
 
+        let is_connect = method == Method::CONNECT;
+
         // Build the set pseudo header set. All requests will include `method`
         // and `path`.
-        let pseudo = Pseudo::request(method, uri);
+        let mut pseudo = Pseudo::request(method, uri);
+
+        if pseudo.scheme.is_none() {
+            // If the scheme is not set, then there are a two options.
+            //
+            // 1) Authority is not set. In this case, a request was issued with
+            //    a relative URI. This is permitted **only** when forwarding
+            //    HTTP 1.x requests. If the HTTP version is set to 2.0, then
+            //    this is an error.
+            //
+            // 2) Authority is set, then the HTTP method *must* be CONNECT.
+            //
+            // It is not possible to have a scheme but not an authority set (the
+            // `http` crate does not allow it).
+            //
+            if pseudo.authority.is_none() {
+                if version == Version::HTTP_2 {
+                    return Err(UserError::MissingUriSchemeAndAuthority.into());
+                } else {
+                    // This is acceptable as per the above comment. However,
+                    // HTTP/2.0 requires that a scheme is set. Since we are
+                    // forwarding an HTTP 1.1 request, the scheme is set to
+                    // "http".
+                    pseudo.set_scheme(uri::Scheme::HTTP);
+                }
+            } else if !is_connect {
+                // TODO: Error
+            }
+        }
 
         // Create the HEADERS frame
         let mut frame = Headers::new(id, pseudo, headers);
@@ -396,7 +423,19 @@ impl proto::Peer for Peer {
             frame.set_end_stream()
         }
 
-        frame
+        Ok(frame)
+    }
+}
+
+impl proto::Peer for Peer {
+    type Poll = Response<()>;
+
+    fn dyn() -> proto::DynPeer {
+        proto::DynPeer::Client
+    }
+
+    fn is_server() -> bool {
+        false
     }
 
     fn convert_poll_message(headers: Headers) -> Result<Self::Poll, RecvError> {

src/codec/error.rs

@@ -47,6 +47,9 @@ pub enum UserError {
 
     /// Illegal headers, such as connection-specific headers.
     MalformedHeaders,
+
+    /// Request submitted with relative URI.
+    MissingUriSchemeAndAuthority,
 }
 
 // ===== impl RecvError =====
@@ -125,6 +128,7 @@ impl error::Error for UserError {
             ReleaseCapacityTooBig => "release capacity too big",
             OverflowedStreamId => "stream ID overflowed",
             MalformedHeaders => "malformed headers",
+            MissingUriSchemeAndAuthority => "request URI missing scheme and authority",
         }
     }
 }

src/frame/headers.rs

@@ -63,6 +63,7 @@ pub struct Continuation {
     headers: Iter,
 }
 
+// TODO: These fields shouldn't be `pub`
 #[derive(Debug, Default, Eq, PartialEq)]
 pub struct Pseudo {
     // Request
@@ -405,10 +406,6 @@ impl Pseudo {
     pub fn request(method: Method, uri: Uri) -> Self {
         let parts = uri::Parts::from(uri);
 
-        fn to_string(src: Bytes) -> String<Bytes> {
-            unsafe { String::from_utf8_unchecked(src) }
-        }
-
         let path = parts
             .path_and_query
             .map(|v| v.into())
@@ -426,7 +423,7 @@ impl Pseudo {
         //
         // TODO: Scheme must be set...
         if let Some(scheme) = parts.scheme {
-            pseudo.set_scheme(to_string(scheme.into()));
+            pseudo.set_scheme(scheme);
         }
 
         // If the URI includes an authority component, add it to the pseudo
@@ -448,8 +445,8 @@ impl Pseudo {
         }
     }
 
-    pub fn set_scheme(&mut self, scheme: String<Bytes>) {
+    pub fn set_scheme(&mut self, scheme: uri::Scheme) {
-        self.scheme = Some(scheme);
+        self.scheme = Some(to_string(scheme.into()));
     }
 
     pub fn set_authority(&mut self, authority: String<Bytes>) {
@@ -457,6 +454,10 @@ impl Pseudo {
     }
 }
 
+fn to_string(src: Bytes) -> String<Bytes> {
+    unsafe { String::from_utf8_unchecked(src) }
+}
+
 // ===== impl Iter =====
 
 impl Iterator for Iter {

src/proto/peer.rs

@@ -8,9 +8,6 @@ use std::fmt;
 
 /// Either a Client or a Server
 pub trait Peer {
-    /// Message type sent into the transport
-    type Send;
-
     /// Message type polled from the transport
     type Poll: fmt::Debug;
 
@@ -18,8 +15,6 @@ pub trait Peer {
 
     fn is_server() -> bool;
 
-    fn convert_send_message(id: StreamId, headers: Self::Send, end_of_stream: bool) -> Headers;
-
     fn convert_poll_message(headers: Headers) -> Result<Self::Poll, RecvError>;
 
     fn is_local_init(id: StreamId) -> bool {

src/proto/streams/send.rs

@@ -74,8 +74,6 @@ impl Send {
             }
         }
 
-
-
         let end_stream = frame.is_end_stream();
 
         // Update the state

src/proto/streams/streams.rs

@@ -494,7 +494,8 @@ where
             }
 
             // Convert the message
-            let headers = client::Peer::convert_send_message(stream_id, request, end_of_stream);
+            let headers = client::Peer::convert_send_message(
+                stream_id, request, end_of_stream)?;
 
             let mut stream = me.store.insert(stream.id, stream);
 

src/server.rs

@@ -375,23 +375,12 @@ impl<T, B> fmt::Debug for Handshake<T, B>
     }
 }
 
-impl proto::Peer for Peer {
+impl Peer {
-    type Send = Response<()>;
+    pub fn convert_send_message(
-    type Poll = Request<()>;
-
-    fn is_server() -> bool {
-        true
-    }
-
-    fn dyn() -> proto::DynPeer {
-        proto::DynPeer::Server
-    }
-
-    fn convert_send_message(
         id: StreamId,
-        response: Self::Send,
+        response: Response<()>,
-        end_of_stream: bool,
+        end_of_stream: bool) -> frame::Headers
-    ) -> frame::Headers {
+    {
         use http::response::Parts;
 
         // Extract the components of the HTTP request
@@ -417,6 +406,18 @@ impl proto::Peer for Peer {
 
         frame
     }
+}
+
+impl proto::Peer for Peer {
+    type Poll = Request<()>;
+
+    fn is_server() -> bool {
+        true
+    }
+
+    fn dyn() -> proto::DynPeer {
+        proto::DynPeer::Server
+    }
 
     fn convert_poll_message(headers: frame::Headers) -> Result<Self::Poll, RecvError> {
         use http::{uri, Version};

tests/client_request.rs

@@ -121,7 +121,6 @@ fn request_stream_id_overflows() {
                     .body(())
                     .unwrap();
 
-
                 // second cannot use the next stream id, it's over
 
                 let poll_err = client.poll_ready().unwrap_err();
@@ -279,8 +278,71 @@ fn request_over_max_concurrent_streams_errors() {
 }
 
 #[test]
-#[ignore]
+fn http_11_request_without_scheme_or_authority() {
-fn request_without_scheme() {}
+    let _ = ::env_logger::init();
+    let (io, srv) = mock::new();
+
+    let srv = srv.assert_client_handshake()
+        .unwrap()
+        .recv_settings()
+        .recv_frame(
+            frames::headers(1)
+                .request("GET", "/")
+                .scheme("http")
+                .eos(),
+        )
+        .send_frame(frames::headers(1).response(200).eos())
+        .close();
+
+    let h2 = Client::handshake(io)
+        .expect("handshake")
+        .and_then(|(mut client, h2)| {
+            // we send a simple req here just to drive the connection so we can
+            // receive the server settings.
+            let request = Request::builder()
+                .method(Method::GET)
+                .uri("/")
+                .body(())
+                .unwrap();
+
+            // first request is allowed
+            let (response, _) = client.send_request(request, true).unwrap();
+            h2.drive(response)
+                .map(move |(h2, _)| (client, h2))
+        });
+
+    h2.join(srv).wait().expect("wait");
+}
+
+#[test]
+fn http_2_request_without_scheme_or_authority() {
+    let _ = ::env_logger::init();
+    let (io, srv) = mock::new();
+
+    let srv = srv.assert_client_handshake()
+        .unwrap()
+        .recv_settings()
+        .close();
+
+    let h2 = Client::handshake(io)
+        .expect("handshake")
+        .and_then(|(mut client, h2)| {
+            // we send a simple req here just to drive the connection so we can
+            // receive the server settings.
+            let request = Request::builder()
+                .version(Version::HTTP_2)
+                .method(Method::GET)
+                .uri("/")
+                .body(())
+                .unwrap();
+
+            // first request is allowed
+            assert!(client.send_request(request, true).is_err());
+            h2.expect("h2")
+        });
+
+    h2.join(srv).wait().expect("wait");
+}
 
 #[test]
 #[ignore]

tests/support/frames.rs

@@ -137,6 +137,16 @@ impl Mock<frame::Headers> {
         Mock(frame)
     }
 
+    pub fn scheme(self, value: &str) -> Self
+    {
+        let (id, mut pseudo, fields) = self.into_parts();
+        let value = value.parse().unwrap();
+
+        pseudo.set_scheme(value);
+
+        Mock(frame::Headers::new(id, pseudo, fields))
+    }
+
     pub fn eos(mut self) -> Self {
         self.0.set_end_stream();
         self

tests/support/prelude.rs

@@ -32,7 +32,7 @@ pub use self::futures::{Future, IntoFuture, Sink, Stream};
 pub use super::future_ext::{FutureExt, Unwrap};
 
 // Re-export HTTP types
-pub use self::http::{HeaderMap, Method, Request, Response, StatusCode};
+pub use self::http::{HeaderMap, Method, Request, Response, StatusCode, Version};
 
 pub use self::bytes::{Buf, BufMut, Bytes, BytesMut, IntoBuf};