Debug assertion panics due to invalid URIs from upstream Hyper

[ZoeyR]

Description

Using any rocket server I attempted to visit a url with [ in the path. I get a "Connection Reset" error in my browser and the rocket log fills with the following:

thread 'rocket-worker-thread' panicked at 'assertion failed: Origin::parse(uri.as_str()).is_ok()', C:\Users\zoey\.cargo\git\checkouts\rocket-8bf16d9ca7e90bdc\786db9b\core\lib\src\request\request.rs:978:9

To Reproduce

// main.rs
#[macro_use] extern crate rocket;

#[get("/hello/<name>/<age>")]
fn hello(name: &str, age: u8) -> String {
    format!("Hello, {} year old named {}!", age, name)
}

#[launch]
fn rocket() -> _ {
    rocket::build().mount("/", routes![hello])
}

# Cargo.toml
[package]
name = "rocket_panic_test"
version = "0.1.0"
edition = "2018"

[dependencies]
rocket = { git = "https://github.com/SergioBenitez/Rocket", branch = "master" }

Visit http://localhost:8000/[

Expected Behavior

Rocket returns a 404 since the route does not exist.

Environment:

• OS Distribution and Kernel: Windows 10
• Rocket Version: master@786db9b

Additional Context

This came up due to a malformed js fetch that accidentally requested http://localhost:8000/[object%20Object]. I reduced the url to the part that causes the panic.

[a-nickol]

The panic! error message comes from a debug_assert! which validates that the URL is valid.

In a production build this assert is skipped and a 404 error screen is shown.

Maybe it would be better not to use an assert, but give more details what the problem is in debug-mode, using an additional error message and just proceed like in production-mode?

[ZoeyR]

Even debug asserts indicate that some assumption is made in the code, and since that assert is failing it indicates that an assumption is wrong. There is this comment near the assertion:

// In debug, make sure we agree with Hyper. Otherwise, cross our fingers
// and trust that it only gives us valid URIs like it's supposed to.

I don't think we should be "crossing our fingers" for production.

[SergioBenitez]

/[ is not a valid target URI. This is a bug in hyper that's being detected by Rocket. I'm certain that there are many more like this.

Despite its claims, hyper's correctness is an ongoing challenge for Rocket (see #17) which will likely see us moving away from hyper before a 1.0 release. Rocket works around such issues where it can; this is one such case. We implement our own URI parser because we don't trust hyper's. What you're seeing is a failing cross-validation check that was put in place to detect disagreements between hyper and Rocket. In this case, Hyper is wrong.

Even debug asserts indicate that some assumption is made in the code, and since that assert is failing it indicates that an assumption is wrong.

This isn't a precise enough analysis of what's going on with this debug_assert!, the details of which I've already commented on above. It isn't so much that an assumption is wrong - this particular failure isn't going to lead to any issues downstream - but that we've caught a bug in hyper.

I don't think we should be "crossing our fingers" for production.

This comment should be taken tongue-in-cheek, not literally. We take security seriously (hence why we have our own, heavily tested, URI parser), so this "crossing our fingers" is an allusion to the fact that we know hyper may be wrong but that even if it is, it's extremely unlikely to cause issues in this case. We'd still like to detect them. After all, if hyper is really wrong, you're liable to have lots of issues that Rocket simply can't ever hope to work-around while keeping hyper around.

In all, I see a few paths forward:

1. Close this issue, open one in hyper. Perhaps add a message to the debug assert explaining the assertion.
2. Always check to see if Rocket believes the URI is valid. If it isn't, return a 400. This has a performance penalty, of course.
3. Do nothing.

I'm inclined to go with 1.

[ZoeyR]

I'm wondering if there is some disagreement with the whatwg URL spec and the current reality of what is allowed. I did some testing of what characters browsers will automatically percent-encode, and what they wont. It seems that chromium, firefox, and internet explorer all consider [ to not need percent encoding, even though its not allowed in the whatwg spec. I also found via seanmonstar/reqwest#442 that it is apparently not unheard of for URL proxies to include ^ in their URL. Perhaps rocket needs to relax its allowed characters?

[jebrosen]

I'm wondering if there is some disagreement with the whatwg URL spec and the current reality of what is allowed.

Yes, this is the case with several major browsers - and it turns out there is an open issue about [ and some other characters in the WHATWG URL repo: whatwg/url#379 . Firefox actually changed this on purpose 6 years ago because it was the only browser that was encoding them, for the sake of consistency: https://bugzilla.mozilla.org/show_bug.cgi?id=1163028.

Perhaps rocket needs to relax its allowed characters?

Yeah, maybe. It's really unfortunate that it's not possible to validate input against any of the current specifications and also be compatible with the behavior of major browsers. In principle I expect at least WHATWG to eventually match up with browsers, but I don't see a lot of recent interest/progress on this particular point.

[ta32]

I was implementing a web proxy server for a website and my app was crashing in debug mode because one of the request has '[' in it. However it works on my browser and in release mode.

Is it ok to remove the debug assert and replace it with a warn or error log instead?