fix(client): Do not strip path and scheme components from URIs for HTTP/2 Extended CONNECT requests (#3242)

Author: bdbai

src/client/client.rs

@@ -16,6 +16,8 @@ use crate::common::{
     exec::BoxSendFuture, lazy as hyper_lazy, sync_wrapper::SyncWrapper, task, Future, Lazy, Pin,
     Poll,
 };
+#[cfg(feature = "http2")]
+use crate::ext::Protocol;
 use crate::rt::Executor;
 
 use super::conn;
@@ -278,7 +280,13 @@ where
                 origin_form(req.uri_mut());
             }
         } else if req.method() == Method::CONNECT {
+            #[cfg(not(feature = "http2"))]
             authority_form(req.uri_mut());
+
+            #[cfg(feature = "http2")]
+            if req.extensions().get::<Protocol>().is_none() {
+                authority_form(req.uri_mut());
+            }
         }
 
         let mut res = match pooled.send_request_retryable(req).await {

tests/integration.rs

@@ -305,6 +305,48 @@ t! {
             ;
 }
 
+t! {
+    h2_connect_authority_form,
+    client:
+        request:
+            method: "CONNECT",
+            // http2 should strip scheme and path from URI (authority-form)
+            uri: "/connect_normal",
+            ;
+        response:
+            ;
+    server:
+        request:
+            method: "CONNECT",
+            // path should be stripped
+            uri: "",
+            ;
+        response:
+            ;
+}
+
+t! {
+    h2_only;
+    h2_extended_connect_full_uri,
+    client:
+        request:
+            method: "CONNECT",
+            // http2 should not strip scheme and path from URI for extended CONNECT requests
+            uri: "/connect_extended",
+            protocol: "the-bread-protocol",
+            ;
+        response:
+            ;
+    server:
+        request:
+            method: "CONNECT",
+            // path should not be stripped
+            uri: "/connect_extended",
+            ;
+        response:
+            ;
+}
+
 t! {
     get_2,
     client:

tests/support/mod.rs

@@ -13,14 +13,16 @@ use hyper::{Body, Client, Request, Response, Server, Version};
 pub use futures_util::{
     future, FutureExt as _, StreamExt as _, TryFutureExt as _, TryStreamExt as _,
 };
-pub use hyper::{HeaderMap, StatusCode};
+pub use hyper::{ext::Protocol, http::Extensions, HeaderMap, StatusCode};
 pub use std::net::SocketAddr;
 
 #[allow(unused_macros)]
 macro_rules! t {
     (
+        @impl
         $name:ident,
-        parallel: $range:expr
+        parallel: $range:expr,
+        $(h2_only: $_h2_only:expr)?
     ) => (
         #[test]
         fn $name() {
@@ -75,6 +77,7 @@ macro_rules! t {
         }
     );
     (
+        @impl
         $name:ident,
         client: $(
             request: $(
@@ -91,7 +94,8 @@ macro_rules! t {
             response: $(
                 $s_res_prop:ident: $s_res_val:tt,
             )*;
-        )*
+        )*,
+        h2_only: $h2_only:expr
     ) => (
         #[test]
         fn $name() {
@@ -116,15 +120,17 @@ macro_rules! t {
                 }
             ),)*];
 
-            __run_test(__TestConfig {
-                client_version: 1,
-                client_msgs: c.clone(),
-                server_version: 1,
-                server_msgs: s.clone(),
-                parallel: false,
-                connections: 1,
-                proxy: false,
-            });
+            if !$h2_only {
+                __run_test(__TestConfig {
+                    client_version: 1,
+                    client_msgs: c.clone(),
+                    server_version: 1,
+                    server_msgs: s.clone(),
+                    parallel: false,
+                    connections: 1,
+                    proxy: false,
+                });
+            }
 
             __run_test(__TestConfig {
                 client_version: 2,
@@ -136,15 +142,17 @@ macro_rules! t {
                 proxy: false,
             });
 
-            __run_test(__TestConfig {
-                client_version: 1,
-                client_msgs: c.clone(),
-                server_version: 1,
-                server_msgs: s.clone(),
-                parallel: false,
-                connections: 1,
-                proxy: true,
-            });
+            if !$h2_only {
+                __run_test(__TestConfig {
+                    client_version: 1,
+                    client_msgs: c.clone(),
+                    server_version: 1,
+                    server_msgs: s.clone(),
+                    parallel: false,
+                    connections: 1,
+                    proxy: true,
+                });
+            }
 
             __run_test(__TestConfig {
                 client_version: 2,
@@ -157,6 +165,12 @@ macro_rules! t {
             });
         }
     );
+    (h2_only; $($t:tt)*) => {
+        t!(@impl $($t)*, h2_only: true);
+    };
+    ($($t:tt)*) => {
+        t!(@impl $($t)*, h2_only: false);
+    };
 }
 
 macro_rules! __internal_map_prop {
@@ -245,6 +259,7 @@ pub struct __CReq {
     pub uri: &'static str,
     pub headers: HeaderMap,
     pub body: Vec<u8>,
+    pub protocol: Option<&'static str>,
 }
 
 impl Default for __CReq {
@@ -254,6 +269,7 @@ impl Default for __CReq {
             uri: "/",
             headers: HeaderMap::new(),
             body: Vec::new(),
+            protocol: None,
         }
     }
 }
@@ -371,6 +387,7 @@ async fn async_test(cfg: __TestConfig) {
 
     let server = hyper::Server::bind(&SocketAddr::from(([127, 0, 0, 1], 0)))
         .http2_only(cfg.server_version == 2)
+        .http2_enable_connect_protocol()
         .serve(new_service);
 
     let mut addr = server.local_addr();
@@ -398,6 +415,9 @@ async fn async_test(cfg: __TestConfig) {
                 //.headers(creq.headers)
                 .body(creq.body.into())
                 .expect("Request::build");
+            if let Some(protocol) = creq.protocol {
+                req.extensions_mut().insert(Protocol::from_static(protocol));
+            }
             *req.headers_mut() = creq.headers;
             let cstatus = cres.status;
             let cheaders = cres.headers;
@@ -458,18 +478,20 @@ fn naive_proxy(cfg: ProxyConfig) -> (SocketAddr, impl Future<Output = ()>) {
     let max_connections = cfg.connections;
     let counter = AtomicUsize::new(0);
 
-    let srv = Server::bind(&([127, 0, 0, 1], 0).into()).serve(make_service_fn(move |_| {
-        let prev = counter.fetch_add(1, Ordering::Relaxed);
-        assert!(max_connections > prev, "proxy max connections");
-        let client = client.clone();
-        future::ok::<_, hyper::Error>(service_fn(move |mut req| {
-            let uri = format!("http://{}{}", dst_addr, req.uri().path())
-                .parse()
-                .expect("proxy new uri parse");
-            *req.uri_mut() = uri;
-            client.request(req)
-        }))
-    }));
+    let srv = Server::bind(&([127, 0, 0, 1], 0).into())
+        .http2_enable_connect_protocol()
+        .serve(make_service_fn(move |_| {
+            let prev = counter.fetch_add(1, Ordering::Relaxed);
+            assert!(max_connections > prev, "proxy max connections");
+            let client = client.clone();
+            future::ok::<_, hyper::Error>(service_fn(move |mut req| {
+                let uri = format!("http://{}{}", dst_addr, req.uri().path())
+                    .parse()
+                    .expect("proxy new uri parse");
+                *req.uri_mut() = uri;
+                client.request(req)
+            }))
+        }));
     let proxy_addr = srv.local_addr();
     (proxy_addr, srv.map(|res| res.expect("proxy error")))
 }