Skip to content

Normalize HTTP request path. #228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 8, 2018
Merged

Normalize HTTP request path. #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ce02cc
Normalize HTTP request path.
carllerche Feb 28, 2018
e31c63a
Add a test for issuing requests with OPTIONS
carllerche Feb 28, 2018
32e7552
Move test comment
carllerche Mar 7, 2018
7c3624e
Remove headers from debug output
carllerche Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/frame/headers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,10 +440,14 @@ impl Pseudo {
pub fn request(method: Method, uri: Uri) -> Self {
let parts = uri::Parts::from(uri);

let path = parts
let mut path = parts
.path_and_query
.map(|v| v.into())
.unwrap_or_else(|| Bytes::from_static(b"/"));
.unwrap_or_else(|| Bytes::new());

if path.is_empty() && method != Method::OPTIONS {
Copy link
Contributor

@briansmith briansmith Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the URI is * it isn't clear to me what path is supposed to be here. is it * like Uri::path() says or is it ``?

When URI is * and the method isn't OPTIONS then it seems like we should return an error somewhere here, but that doesn't seem to be done. Or maybe I'm misunderstanding some details of how * is handled in the parsing code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant spec section:

The :path pseudo-header field includes the path and query parts of the target URI (the path-absolute production and optionally a '?' character followed by the query production (see Sections 3.3 and 3.4 of [RFC3986]). A request in asterisk form includes the value '*' for the :path pseudo-header field.

This pseudo-header field MUST NOT be empty for http or https URIs; http or https URIs that do not contain a path component MUST include a value of '/'. The exception to this rule is 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 [RFC7230], Section 5.3.4).

Does this make sense?

I also saw you opened hyperium/http#176, so we can discuss the details of the http path API there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the path can also be not set of the scheme is not http or https, but those cases are not really supported yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, but it should also check for `Method::CONNECT.

RFC7450#CONNECT:

The :scheme and :path pseudo-header fields MUST be omitted.

path = Bytes::from_static(b"/");
}

let mut pseudo = Pseudo {
method: Some(method),
Expand Down
66 changes: 66 additions & 0 deletions tests/client_request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,72 @@ fn recv_too_big_headers() {

}

#[test]
fn request_without_path() {
let _ = ::env_logger::try_init();
let (io, srv) = mock::new();

let srv = srv.assert_client_handshake()
.unwrap()
.recv_settings()
.recv_frame(frames::headers(1).request("GET", "http://example.com/").eos())
.send_frame(frames::headers(1).response(200).eos())
.close();

let client = client::handshake(io)
.expect("handshake")
.and_then(move |(mut client, conn)| {
// Note the lack of trailing slash.
let request = Request::get("http://example.com")
Copy link
Contributor

@briansmith briansmith Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to see tests for OPTIONS * and GET * as well as OPTIONS /* and GET /*

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to write a test for OPTIONS, but there seems to be some problems unrelated to this change. So, I will open a new issue to track this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I figured out how to write a test, but #229 still applies as it is kind of janky.

.body(())
.unwrap();

let (response, _) = client.send_request(request, true).unwrap();

conn.drive(response)
});

client.join(srv).wait().unwrap();
}

#[test]
fn request_options_with_star() {
let _ = ::env_logger::try_init();
let (io, srv) = mock::new();

// Note the lack of trailing slash.
let uri = uri::Uri::from_parts({
let mut parts = uri::Parts::default();
parts.scheme = Some(uri::Scheme::HTTP);
parts.authority = Some(uri::Authority::from_shared("example.com".into()).unwrap());
parts.path_and_query = Some(uri::PathAndQuery::from_static("*"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a PathAndQuery::STAR const in the http crate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful!

parts
}).unwrap();

let srv = srv.assert_client_handshake()
.unwrap()
.recv_settings()
.recv_frame(frames::headers(1).request("OPTIONS", uri.clone()).eos())
.send_frame(frames::headers(1).response(200).eos())
.close();

let client = client::handshake(io)
.expect("handshake")
.and_then(move |(mut client, conn)| {
let request = Request::builder()
.method(Method::OPTIONS)
.uri(uri)
.body(())
.unwrap();

let (response, _) = client.send_request(request, true).unwrap();

conn.drive(response)
});

client.join(srv).wait().unwrap();
}

const SETTINGS: &'static [u8] = &[0, 0, 0, 4, 0, 0, 0, 0, 0];
const SETTINGS_ACK: &'static [u8] = &[0, 0, 0, 4, 1, 0, 0, 0, 0];

Expand Down
2 changes: 1 addition & 1 deletion tests/prioritization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ fn single_stream_send_large_body() {

#[test]
fn multiple_streams_with_payload_greater_than_default_window() {
let _ = ::env_logger::init();
let _ = ::env_logger::try_init();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to the PR but it can cause tests to fail and it didn't seem worth splitting it out.


let payload = vec![0; 16384*5-1];

Expand Down
2 changes: 1 addition & 1 deletion tests/support/prelude.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, Version};
pub use self::http::{uri, HeaderMap, Method, Request, Response, StatusCode, Version};

pub use self::bytes::{Buf, BufMut, Bytes, BytesMut, IntoBuf};

Expand Down