Repository: hyperium/h2 (rust)

Issue:
panic on invalid :authority
When the value of `:authority` is invalid, we get the following panic:

```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: InvalidUriBytes(InvalidUri(InvalidUriChar))', src/libcore/result.rs:906:4

   9: core::result::unwrap_failed
  10: <h2::server::Peer as h2::proto::peer::Peer>::convert_poll_message
  11: <h2::proto::connection::Connection<T, P, B>>::poll2
  12: <h2::server::Server<T, B> as futures::stream::Stream>::poll
```

This should probably be a User error and not a panic.


Discussion:
Looks like this is in server, so it wouldn't be a user error.

> Endpoints MUST treat a request or response that contains undefined or invalid pseudo-header fields as malformed.

If we say that a `:authority` with illegal bytes is an "invalid pseudo-header", then it's a [stream error of PROTOCOL_ERROR](http://httpwg.org/specs/rfc7540.html#malformed).
I think other "invalid request" situations are handled as a stream error currently.

Patch:
diff --git a/src/frame/headers.rs b/src/frame/headers.rs
--- a/src/frame/headers.rs
+++ b/src/frame/headers.rs
@@ -211,6 +211,11 @@ impl Headers {
         (self.header_block.pseudo, self.header_block.fields)
     }
 
+    #[cfg(feature = "unstable")]
+    pub fn pseudo_mut(&mut self) -> &mut Pseudo {
+        &mut self.header_block.pseudo
+    }
+
     pub fn fields(&self) -> &HeaderMap {
         &self.header_block.fields
     }
diff --git a/src/server.rs b/src/server.rs
--- a/src/server.rs
+++ b/src/server.rs
@@ -513,15 +513,15 @@ impl proto::Peer for Peer {
         let mut parts = uri::Parts::default();
 
         if let Some(scheme) = pseudo.scheme {
-            // TODO: Don't unwrap
-            parts.scheme = Some(uri::Scheme::from_shared(scheme.into_inner()).unwrap());
+            parts.scheme = Some(uri::Scheme::from_shared(scheme.into_inner())
+                .or_else(|_| malformed!())?);
         } else {
             malformed!();
         }
 
         if let Some(authority) = pseudo.authority {
-            // TODO: Don't unwrap
-            parts.authority = Some(uri::Authority::from_shared(authority.into_inner()).unwrap());
+            parts.authority = Some(uri::Authority::from_shared(authority.into_inner())
+                .or_else(|_| malformed!())?);
         }
 
         if let Some(path) = pseudo.path {
@@ -530,8 +530,8 @@ impl proto::Peer for Peer {
                 malformed!();
             }
 
-            // TODO: Don't unwrap
-            parts.path_and_query = Some(uri::PathAndQuery::from_shared(path.into_inner()).unwrap());
+            parts.path_and_query = Some(uri::PathAndQuery::from_shared(path.into_inner())
+                .or_else(|_| malformed!())?);
         }
 
         b.uri(parts);


Test patch:
diff --git a/tests/server.rs b/tests/server.rs
--- a/tests/server.rs
+++ b/tests/server.rs
@@ -58,3 +58,30 @@ fn serve_request() {
 #[test]
 #[ignore]
 fn accept_with_pending_connections_after_socket_close() {}
+
+#[test]
+fn sent_invalid_authority() {
+    let _ = ::env_logger::init();
+    let (io, client) = mock::new();
+
+    let bad_auth = util::byte_str("not:a/good authority");
+    let mut bad_headers: frame::Headers = frames::headers(1)
+        .request("GET", "https://example.com/")
+        .eos()
+        .into();
+    bad_headers.pseudo_mut().authority = Some(bad_auth);
+
+    let client = client
+        .assert_server_handshake()
+        .unwrap()
+        .recv_settings()
+        .send_frame(bad_headers)
+        .recv_frame(frames::reset(1).protocol_error())
+        .close();
+
+    let srv = Server::handshake(io)
+        .expect("handshake")
+        .and_then(|srv| srv.into_future().unwrap());
+
+    srv.join(client).wait().expect("wait");
+}
diff --git a/tests/support/frames.rs b/tests/support/frames.rs
--- a/tests/support/frames.rs
+++ b/tests/support/frames.rs
@@ -136,6 +136,12 @@ impl Mock<frame::Headers> {
     }
 }
 
+impl From<Mock<frame::Headers>> for frame::Headers {
+    fn from(src: Mock<frame::Headers>) -> Self {
+        src.0
+    }
+}
+
 impl From<Mock<frame::Headers>> for SendFrame {
     fn from(src: Mock<frame::Headers>) -> Self {
         Frame::Headers(src.0)
@@ -234,6 +240,11 @@ impl From<Mock<frame::GoAway>> for SendFrame {
 // ==== Reset helpers
 
 impl Mock<frame::Reset> {
+    pub fn protocol_error(self) -> Self {
+        let id = self.0.stream_id();
+        Mock(frame::Reset::new(id, frame::Reason::ProtocolError))
+    }
+
     pub fn flow_control(self) -> Self {
         let id = self.0.stream_id();
         Mock(frame::Reset::new(id, frame::Reason::FlowControlError))
diff --git a/tests/support/mod.rs b/tests/support/mod.rs
--- a/tests/support/mod.rs
+++ b/tests/support/mod.rs
@@ -12,6 +12,7 @@ pub extern crate futures;
 pub extern crate h2;
 pub extern crate http;
 pub extern crate mock_io;
+pub extern crate string;
 pub extern crate tokio_io;
 
 // Kind of annoying, but we can't use macros from crates that aren't specified
diff --git a/tests/support/util.rs b/tests/support/util.rs
--- a/tests/support/util.rs
+++ b/tests/support/util.rs
@@ -1,8 +1,13 @@
 use h2::client;
 
+use super::string::{String, TryFrom};
 use bytes::Bytes;
 use futures::{Async, Future, Poll};
 
+pub fn byte_str(s: &str) -> String<Bytes> {
+    String::try_from(Bytes::from(s)).unwrap()
+}
+
 pub fn wait_for_capacity(stream: client::Stream<Bytes>, target: usize) -> WaitForCapacity {
     WaitForCapacity {
         stream: Some(stream),