Skip to content
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.

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

Uri::from_parts to return SlashInPathMissing #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lukaspiatkowski
Copy link

When running this code:

let mut parts = Parts::default();
parts.scheme = Some(Scheme::HTTP);
parts.authority = Some(Authority::from_str("www.example.com").unwrap());
parts.path_and_query = Some(PathAndQuery::from_str("my_path").unwrap());

let uri = Uri::from_parts(parts).unwrap();
println!("{}", uri);

the intention would most likely be to print "http://www.example.com/my_path", but in reality it is "http://www.example.commy_path" (notice the missing slash).

Rust playground here: https://play.rust-lang.org/?gist=903f0d760c9b94b358a21540301c2a02&version=stable&mode=debug&edition=2015

This can lead into annoying bugs and furthermore is against the RFC3986 sections 3 and 3.3 where it states that:

URI         = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
hier-part   = "//" authority path-abempty
              / path-absolute
              / path-rootless
              / path-empty
path-abempty  = *( "/" segment )

If authority is given, but path is not empty and doesn't start with "/",
return error.
@seanmonstar
Copy link
Member

Oh my, yea, that's definitely wrong! Thanks for filing this. I wonder, should perhaps PathAndQuery::from_str("my_path") actually be the error? A path should always start with a slash, right?

@lukaspiatkowski
Copy link
Author

According to the section 3 of the RFC mentioned above:

   The scheme and path components are required, though the path may be
   empty (no characters).  When authority is present, the path must
   either be empty or begin with a slash ("/") character.  When
   authority is not present, the path cannot begin with two slash
   characters ("//").  These restrictions result in five different ABNF
   rules for a path (Section 3.3), only one of which will match any
   given URI reference.
   The following are two example URIs and their component parts:

         foo://example.com:8042/over/there?name=ferret#nose
         \_/   \______________/\_________/ \_________/ \__/
          |           |            |            |        |
       scheme     authority       path        query   fragment
          |   _____________________|__
         / \ /                        \
         urn:example:animal:ferret:nose

the "urn:example:animal:ferret:nose" is a valid URI with schema "urn" and path "example:animal:ferret:nose" with no leading "/".

Taking into account that this is URI implementation for http crate one might argue that it is irrelevant to support generic URI, but then if we want to support relative references as defined in section 4.2. of the aforementioned RFC then

relative-part = "//" authority path-abempty
              / path-absolute   ; begins with "/" but not "//"
              / path-noscheme   ; begins with a non-colon segment
              / path-empty      ; zero characters

allows creation of such URIs that can be used in this kind of html:

<a href="sub_page">Relative link</a>

which kind of makes sense to support by a crate whose name is "http".

OTOH, neither hyper client nor server would have any use for a URI whose schema is not HTTP or HTTPS and if schema is provided then path can be either empty or has to have a leading "/".

@lukaspiatkowski
Copy link
Author

lukaspiatkowski commented Jul 20, 2018

tl;dr What I am saying is that it is up to your ambition whether you want to support generic URI or to be more hyper specific in which case a lot of code from https://github.com/hyperium/http/blob/master/src/uri/scheme.rs and this lines https://github.com/hyperium/http/blob/master/src/uri/mod.rs#L189-L193 can go away (unless I am missing something and there actually is a use case for relative references and generic URI in hyper)

@seanmonstar
Copy link
Member

I'm not decided which makes more sense... @carllerche?

@carllerche
Copy link
Collaborator

I believe the goal of this Uri type is only to support cases from HTTP 1.x and 2.0. For 1.x, this would be the request line and for 2.0, it would be representing the related pseudo headers.

I guess, the question is what is valid for HTTP 1.x, because 2.0 is pretty strict.

@carllerche
Copy link
Collaborator

In this case, at the very least, Uri::from_path should validate that if there is an authority, path is empty or starts with /. That seems like a good next step.

@seanmonstar seanmonstar added the B-rfc Blocked: request for comments. More discussion would help move this along. label Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: request for comments. More discussion would help move this along.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants