Rocket: Redirects to absolute URLs mangle on master

[Korvox]

The following sample on 0.3 redirects properly to Google:

#![feature(decl_macro, plugin)]
#![plugin(rocket_codegen)]

extern crate rocket;

use rocket::response::Redirect;

#[get("/")]
fn redir() -> Redirect {
    Redirect::to("https://www.google.com")
}

fn main() {
    rocket::Rocket::ignite().mount("/", routes![redir]).launch();
}

On git master right now, it mangles the url into "/https:/www.google.com" which it then tries to interpret as a local address (http://localhost:8000/https:/www.google.com).

To make sure this wasn't just a brain fart, I tried putting Rustly on git Rocket. Its redirects are similarly mangled (http://localhost:8000/gY -> http://localhost:8000/https:/www.matthias-endler.de).

[SergioBenitez]

Good catch. Confirmed. Introduced in the recent Typed URI change.

[gsquire]

I was going to submit a PR for this, but I wanted to ask a question first. Is there any reason to avoid using the url crate? The example on the front page of the docs seems like it would solve the problem of redirecting to a full URI.

I'm sure you know but the problem pointed out here is that the Display implementation for URI prints segments followed by optional query and fragments.

[Korvox]

If you want to know why Rocket (and Hyper) use their own Uri types rather than Url, you can read the discussion here about it. Rockets Uri is not the same as Hypers Uri (which is new in the async 0.11 release) but the reasoning behind why it exists is the same.

[gsquire]

Thanks for sharing!

In that case, I don't know what the best course of action is here then. I'd still be happy to submit a PR though.

[SergioBenitez]

This issue arose because of a change to how the Redirect type stores a redirect URI internally. In particular, the internal URI storage type for Redirect was changed from String to Uri.

The Uri type in Rocket only supports URIs in origin form (/a/b/c); other URIs are normalized to origin form. This normalization is applied to any string used as a Uri. This normalization results in incorrect URIs when the passed in string is not fit for normalization. In particular, in this issue, @Korvox notes that absolute-form URIs (http://google.com) are incorrectly normalized.

The fix I'd like to take is to expand the Uri type to support all request targets (origin forms, absolute forms, authority forms, and asterisk forms). This type would be similar to the Uri type in the http crate. Critically different, however, is that Rocket's Uri type will support creation without allocation as well as querying to check the form kind (ie: absolute? asterisk?, etc.). This latter ability is presently used by Rocket.

This expansion will be a full rewrite of the Uri type with a complete, zero-allocation URI parser that conforms to RFC 7230. I've started the rewrite and have reached and surpassed parity with the existing Uri type. I plan on having a complete implementation soon.

[bluetech]

This comment is pedantic, but I thought I'd mention it anyway.

@SergioBenitez what you implemented is not a URI parser but a "Request Target" parser, as specified in https://tools.ietf.org/html/rfc7230#section-5.3. A Request Target is not a URI, though it is strongly related in that is is defined by reference to the URI specification https://tools.ietf.org/html/rfc3986. Strictly speaking only the absolute-form of Request Target is a URI (though not any URI).

The http crate had made the same "mistake" of calling the type Uri rather than RequestTarget. Personally, I think the technically-correct name is preferable, even though it's not nice and short as Uri.

[SergioBenitez]

This comment is pedantic, but I thought I'd mention it anyway.

@SergioBenitez what you implemented is not a URI parser but a "Request Target" parser, as specified in https://tools.ietf.org/html/rfc7230#section-5.3. A Request Target is not a URI, though it is strongly related in that is is defined by reference to the URI specification https://tools.ietf.org/html/rfc3986. Strictly speaking only the absolute-form of Request Target is a URI (though not any URI).

The http crate had made the same "mistake" of calling the type Uri rather than RequestTarget. Personally, I think the technically-correct name is preferable, even though it's not nice and short as Uri.

My apologies for responding two years late; I'm only now working through the related URI issues. I simply wanted to state that I was and am aware of the differences, and that my use of the name Uri here is largely colloquial in nature but also refers to URI _references_ (https://tools.ietf.org/html/rfc3986#section-4.1), though I'm also aware that Uri is not actually parsing URI references. We _could_ call it RequestTarget, but my feeling is that most users have never heard the term "request target", and so the name would simply confuse rather than aid. And, while Uri may also mislead users into believing that Uri _always_ represents a full URI, my feeling is that the name is more intuitive, and that the context in which Uri is used resolves ambiguity.

I understand your point, and I agree on technical standing alone, but ultimately we choose names to convey meaning, and in this, I feel that Uri gets us closest, even if it is technically incorrect.