Normalize HTTP request path. #228

carllerche · 2018-02-28T05:02:02Z

The HTTP/2.0 specification requires that the path pseudo header is never
empty for requests unless the request uses the OPTIONS method.

This is currently not correctly enforced.

This patch provides a test and a fix.

The HTTP/2.0 specification requires that the path pseudo header is never empty for requests unless the request uses the OPTIONS method. This is currently not correctly enforced. This patch provides a test and a fix.

briansmith · 2018-02-28T07:12:04Z

src/frame/headers.rs

-            .unwrap_or_else(|| Bytes::from_static(b"/"));
+            .unwrap_or_else(|| Bytes::new());
+
+        if path.is_empty() && method != Method::OPTIONS {


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?

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.

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

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.

briansmith · 2018-02-28T07:13:16Z

tests/client_request.rs

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


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

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.

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

carllerche · 2018-02-28T20:22:01Z

tests/prioritization.rs

 #[test]
 fn multiple_streams_with_payload_greater_than_default_window() {
-    let _ = ::env_logger::init();
+    let _ = ::env_logger::try_init();


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

briansmith · 2018-03-05T22:07:25Z

I reviewed the initial PR without thinking too much. But, I actually think that the proposed fix is not a good design. We shouldn't default every request path to "/" when the request path isn't provided by the caller. Instead, we should refuse to issue the request. If the caller forgot to provide the request path then that's a logic failure on the caller's part. Defaulting to "/" will result in applications accidentally sending requests to the wrong path, which is dangerous.

In the case of OPTIONS, it is especially confusing: should the default be "*", or should it be "/" like everything else? It seems wrong for it to be inconsistent with the others, but "/" also seems wrong. It's better to require the user to be explicit.

seanmonstar · 2018-03-06T00:11:05Z

src/frame/headers.rs

            .field("stream_id", &self.stream_id)
            .field("stream_dep", &self.stream_dep)
            .field("flags", &self.flags)
+            .field("header_block", &self.header_block)


The comment right after this (while slight outdated) suggests that this field is purposefully not included :)

seanmonstar · 2018-03-06T00:17:39Z

Defaulting to "/" will result in applications accidentally sending requests to the wrong path, which is dangerous.

Is there an instance where that is true? I cannot think of any.

I actually think that perhaps the behavior should be fixed in http. If the Uri should have a path (either absolute-form or origin-form (aka ?foo=bar or http://foo.bar), then PathAndQuery should match the already existing path() behavior of including a /.

briansmith · 2018-03-06T01:24:23Z

Defaulting to "/" will result in applications accidentally sending requests to the wrong path, which is dangerous.

Is there an instance where that is true? I cannot think of any.

I'm sure there are many cases. One that comes to mind: Some domain ownership verification procedures require one to add a token somewhere in the file at /.well-known/something-something. If the request to verify the token is present at /.well-known/something-something is accidentally sent to / then any user could potentially do the domain verification procedure if they can modify the home page (e.g. by submitting a popular story to reddit, one can get on its home page with arbitrary text).

PathAndQuery should match the already existing path() behavior of including a /.

That's basically hyperium/http#176.

seanmonstar · 2018-03-06T01:28:46Z

One that comes to mind: Some domain ownership verification procedures require one to add a token somewhere in the file at /.well-known/something-something. If the request to verify the token is present at /.well-known/something-something is accidentally sent to / then any user could potentially do the domain verification procedure if they can modify the home page.

I don't see how this bug occurs from assuming that the path of http://example.com is /.

carllerche · 2018-03-06T18:47:42Z

Also, assuming that http://example.com implies a path of / is what browsers do.

I would say that constructing a URI from parts should possibly not make that assumption though.

seanmonstar

Overall looks fine, just want the header_block removed from the debug output, at least.

We could fix this for CONNECT also, or this can stay focused on just OPTIONS *, and connect can be a separate PR.

seanmonstar · 2018-03-06T19:11:30Z

tests/client_request.rs

+    let client = client::handshake(io)
+        .expect("handshake")
+        .and_then(move |(mut client, conn)| {
+            // Note the lack of trailing slash.


I'd move this comment up to the let uri = ... line, since here it just left me confused ("Note? But I don't see any lack of trailing slash here!")

seanmonstar · 2018-03-06T19:12:37Z

tests/client_request.rs

+        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("*"));


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

It might be useful!

seanmonstar · 2018-03-06T19:15:48Z

src/frame/headers.rs

-            .unwrap_or_else(|| Bytes::from_static(b"/"));
+            .unwrap_or_else(|| Bytes::new());
+
+        if path.is_empty() && method != Method::OPTIONS {


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.

carllerche · 2018-03-07T22:42:50Z

@seanmonstar generally, I don't think CONNECT is fully supported / implemented yet. I'd recommend punting on any connect work in this PR.

carllerche · 2018-03-07T22:50:11Z

Re the / default path discussion, I asked the question here.

It seems like this is a general question and I am unsure if it merits holding up this PR.

Normalize HTTP request path.

1ce02cc

The HTTP/2.0 specification requires that the path pseudo header is never empty for requests unless the request uses the OPTIONS method. This is currently not correctly enforced. This patch provides a test and a fix.

carllerche requested a review from seanmonstar February 28, 2018 05:02

briansmith reviewed Feb 28, 2018

View reviewed changes

carllerche mentioned this pull request Feb 28, 2018

Supporting OPTIONS requests #229

Open

Add a test for issuing requests with OPTIONS

e31c63a

carllerche commented Feb 28, 2018

View reviewed changes

seanmonstar reviewed Mar 6, 2018

View reviewed changes

seanmonstar requested changes Mar 6, 2018

View reviewed changes

carllerche mentioned this pull request Mar 7, 2018

Support CONNECT requests #234

Closed

Move test comment

32e7552

carllerche mentioned this pull request Mar 7, 2018

Uri::path() and and Uri::path_and_query() have confusing semantics hyperium/http#176

Open

Remove headers from debug output

7c3624e

seanmonstar approved these changes Mar 8, 2018

View reviewed changes

carllerche merged commit 02841eb into master Mar 8, 2018

carllerche deleted the request-normalize-path branch March 8, 2018 17:36

Uh oh!

Normalize HTTP request path. #228

Normalize HTTP request path. #228

Uh oh!

Conversation

carllerche commented Feb 28, 2018

Uh oh!

briansmith Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansmith Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansmith commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanmonstar commented Mar 6, 2018

Uh oh!

briansmith commented Mar 6, 2018

Uh oh!

seanmonstar commented Mar 6, 2018

Uh oh!

carllerche commented Mar 6, 2018

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche commented Mar 7, 2018

Uh oh!

carllerche commented Mar 7, 2018

Uh oh!

Uh oh!

briansmith Feb 28, 2018 •

edited

Loading

briansmith Feb 28, 2018 •

edited

Loading