Skip to content

Commit

Permalink
Fix request pseudo-header construction for CONNECT & OPTION methods
Browse files Browse the repository at this point in the history 
CONNECT & OPTIONS request has special requirements regarding :path & :scheme pseudo-header which were not met.

Construction of pseudo-header was fixed to not include :path & :scheme fields for CONNECT method.
Empty :path field for OPTIONS requests now translates to '*' value sent in :path field.

In addition to tests included in MR the CONNECT request changes were tested against server based on hyper 1.2.
  • Loading branch information
@mstyura
mstyura committed Apr 28, 2024
1 parent 0d66e3c commit 0f6bd46
Show file tree
Hide file tree
Showing 4 changed files with 264 additions and 57 deletions.
195 changes: 179 additions & 16 deletions src/frame/headers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,32 +554,38 @@ impl Pseudo {
pub fn request(method: Method, uri: Uri, protocol: Option<Protocol>) -> Self {
let parts = uri::Parts::from(uri);

let mut path = parts
.path_and_query
.map(|v| BytesStr::from(v.as_str()))
.unwrap_or(BytesStr::from_static(""));

match method {
Method::OPTIONS | Method::CONNECT => {}
_ if path.is_empty() => {
path = BytesStr::from_static("/");
}
_ => {}
}
let (scheme, path) = if method == Method::CONNECT && protocol.is_none() {
(None, None)
} else {
let path = parts
.path_and_query
.map(|v| BytesStr::from(v.as_str()))
.unwrap_or(BytesStr::from_static(""));

let path = if !path.is_empty() {
path
} else {
if method == Method::OPTIONS {
BytesStr::from_static("*")
} else {
BytesStr::from_static("/")
}
};

(parts.scheme, Some(path))
};

let mut pseudo = Pseudo {
method: Some(method),
scheme: None,
authority: None,
path: Some(path).filter(|p| !p.is_empty()),
path,
protocol,
status: None,
};

// If the URI includes a scheme component, add it to the pseudo headers
//
// TODO: Scheme must be set...
if let Some(scheme) = parts.scheme {
if let Some(scheme) = scheme {
pseudo.set_scheme(scheme);
}

Expand Down Expand Up @@ -1048,4 +1054,161 @@ mod test {
let mut buf = BytesMut::new();
huffman::decode(src, &mut buf).unwrap()
}

#[test]
fn test_connect_request_pseudo_headers_omits_path_and_scheme() {
// a) CONNECT requests MUST NOT include :scheme & :path pseudo-header fields
// See: https://datatracker.ietf.org/doc/html/rfc9113#section-8.5

assert_eq!(
Pseudo::request(
Method::CONNECT,
Uri::from_static("https://example.com:8443"),
None
),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com:8443").into(),
..Default::default()
}
);

assert_eq!(
Pseudo::request(
Method::CONNECT,
Uri::from_static("https://example.com/test"),
None
),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com").into(),
..Default::default()
}
);

assert_eq!(
Pseudo::request(Method::CONNECT, Uri::from_static("example.com:8443"), None),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com:8443").into(),
..Default::default()
}
);
}

#[test]
fn test_extended_connect_request_pseudo_headers_includes_path_and_scheme() {
// On requests that contain the :protocol pseudo-header field, the
// :scheme and :path pseudo-header fields of the target URI (see
// Section 5) MUST also be included.
// See: https://datatracker.ietf.org/doc/html/rfc8441#section-4

assert_eq!(
Pseudo::request(
Method::CONNECT,
Uri::from_static("https://example.com:8443"),
Protocol::from_static("the-bread-protocol").into()
),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com:8443").into(),
scheme: BytesStr::from_static("https").into(),
path: BytesStr::from_static("/").into(),
protocol: Protocol::from_static("the-bread-protocol").into(),
..Default::default()
}
);

assert_eq!(
Pseudo::request(
Method::CONNECT,
Uri::from_static("https://example.com:8443/test"),
Protocol::from_static("the-bread-protocol").into()
),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com:8443").into(),
scheme: BytesStr::from_static("https").into(),
path: BytesStr::from_static("/test").into(),
protocol: Protocol::from_static("the-bread-protocol").into(),
..Default::default()
}
);

assert_eq!(
Pseudo::request(
Method::CONNECT,
Uri::from_static("http://example.com/a/b/c"),
Protocol::from_static("the-bread-protocol").into()
),
Pseudo {
method: Method::CONNECT.into(),
authority: BytesStr::from_static("example.com").into(),
scheme: BytesStr::from_static("http").into(),
path: BytesStr::from_static("/a/b/c").into(),
protocol: Protocol::from_static("the-bread-protocol").into(),
..Default::default()
}
);
}

#[test]
fn test_options_request_with_empty_path_has_asterisk_as_pseudo_path() {
// an OPTIONS request for an "http" or "https" URI that does not include a path component;
// these MUST include a ":path" pseudo-header field with a value of '*' (see Section 7.1 of [HTTP]).
// See: https://datatracker.ietf.org/doc/html/rfc9113#section-8.3.1
assert_eq!(
Pseudo::request(Method::OPTIONS, Uri::from_static("example.com:8080"), None,),
Pseudo {
method: Method::OPTIONS.into(),
authority: BytesStr::from_static("example.com:8080").into(),
path: BytesStr::from_static("*").into(),
..Default::default()
}
);
}

#[test]
fn test_non_option_and_non_connect_requests_include_path_and_scheme() {
let methods = [
Method::GET,
Method::POST,
Method::PUT,
Method::DELETE,
Method::HEAD,
Method::PATCH,
Method::TRACE,
];

for method in methods {
assert_eq!(
Pseudo::request(
method.clone(),
Uri::from_static("http://example.com:8080"),
None,
),
Pseudo {
method: method.clone().into(),
authority: BytesStr::from_static("example.com:8080").into(),
scheme: BytesStr::from_static("http").into(),
path: BytesStr::from_static("/").into(),
..Default::default()
}
);
assert_eq!(
Pseudo::request(
method.clone(),
Uri::from_static("https://example.com/a/b/c"),
None,
),
Pseudo {
method: method.into(),
authority: BytesStr::from_static("example.com").into(),
scheme: BytesStr::from_static("https").into(),
path: BytesStr::from_static("/a/b/c").into(),
..Default::default()
}
);
}
}
}
25 changes: 9 additions & 16 deletions tests/h2-support/src/frames.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ use std::fmt;
use bytes::Bytes;
use http::{HeaderMap, StatusCode};

use h2::{
ext::Protocol,
frame::{self, Frame, StreamId},
};
use h2::frame::{self, Frame, StreamId};

pub const SETTINGS: &[u8] = &[0, 0, 0, 4, 0, 0, 0, 0, 0];
pub const SETTINGS_ACK: &[u8] = &[0, 0, 0, 4, 1, 0, 0, 0, 0];
Expand Down Expand Up @@ -124,19 +121,24 @@ impl Mock<frame::Headers> {
M::Error: fmt::Debug,
{
let method = method.try_into().unwrap();
let (id, _, fields) = self.into_parts();
let (id, pseudo, fields) = self.into_parts();
let frame = frame::Headers::new(
id,
frame::Pseudo {
scheme: None,
method: Some(method),
..Default::default()
..pseudo
},
fields,
);
Mock(frame)
}

pub fn pseudo(self, pseudo: frame::Pseudo) -> Self {
let (id, _, fields) = self.into_parts();
let frame = frame::Headers::new(id, pseudo, fields);
Mock(frame)
}

pub fn response<S>(self, status: S) -> Self
where
S: TryInto<http::StatusCode>,
Expand Down Expand Up @@ -184,15 +186,6 @@ impl Mock<frame::Headers> {
Mock(frame::Headers::new(id, pseudo, fields))
}

pub fn protocol(self, value: &str) -> Self {
let (id, mut pseudo, fields) = self.into_parts();
let value = Protocol::from(value);

pseudo.set_protocol(value);

Mock(frame::Headers::new(id, pseudo, fields))
}

pub fn eos(mut self) -> Self {
self.0.set_end_stream();
self
Expand Down
49 changes: 47 additions & 2 deletions tests/h2-tests/tests/client_request.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,45 @@ async fn http_2_request_without_scheme_or_authority() {
join(srv, h2).await;
}

#[tokio::test]
async fn http_2_connect_request_omit_scheme_and_path_fields() {
h2_support::trace_init!();
let (io, mut srv) = mock::new();

let srv = async move {
let settings = srv.assert_client_handshake().await;
assert_default_settings!(settings);
srv.recv_frame(
frames::headers(1)
.pseudo(frame::Pseudo {
method: Method::CONNECT.into(),
authority: util::byte_str("tunnel.example.com:8443").into(),
..Default::default()
})
.eos(),
)
.await;
srv.send_frame(frames::headers(1).response(200).eos()).await;
};

let h2 = async move {
let (mut client, mut h2) = client::handshake(io).await.expect("handshake");

// In HTTP_2 CONNECT request the ":scheme" and ":path" pseudo-header fields MUST be omitted.
let request = Request::builder()
.version(Version::HTTP_2)
.method(Method::CONNECT)
.uri("https://tunnel.example.com:8443/")
.body(())
.unwrap();

let (response, _) = client.send_request(request, true).unwrap();
h2.drive(response).await.unwrap();
};

join(srv, h2).await;
}

#[test]
#[ignore]
fn request_with_h1_version() {}
Expand Down Expand Up @@ -1521,8 +1560,14 @@ async fn extended_connect_request() {

srv.recv_frame(
frames::headers(1)
.request("CONNECT", "http://bread/baguette")
.protocol("the-bread-protocol")
.pseudo(frame::Pseudo {
method: Method::CONNECT.into(),
scheme: util::byte_str("http").into(),
authority: util::byte_str("bread").into(),
path: util::byte_str("/baguette").into(),
protocol: Protocol::from_static("the-bread-protocol").into(),
..Default::default()
})
.eos(),
)
.await;
Expand Down
Loading

0 comments on commit 0f6bd46

Please sign in to comment.