Repository: seanmonstar/reqwest (language: rust)

Issue:
Panic when making request, could not parse Url as Uri
I just came across a panic in the latest `reqwest` release (both `0.10` and `master`). This happens when making a request.

It seems that some input in `Client.get(url)` parses successfully as URL at first, and then panics when attempting to parse the URL as `http::Uri` (internally).

### Example
Here's a very basic (abstracted) example that causes this panic:

```rust
#[tokio::main]
async fn main() -> Result<(), reqwest::Error> {
    reqwest::Client::new()
        .get("http://\"")
        .send()
        .await?;
    Ok(())
}
```

Which outputs:

```
thread 'main' panicked at 'a parsed Url should always be a valid Uri: InvalidUri(InvalidUriChar)', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
```

I believe the `Client.get(url)` function should return an error in all cases where the URL is invalid. This actually happens when providing something like `"http://a b"` (`InvalidDomainCharacter`). However, `"http://\""` succeeds and panics when making the request with `.send()`.

The panic is located [here](https://github.com/seanmonstar/reqwest/blob/v0.10.0-alpha.1/src/into_url.rs#L44).

### Assumption
I have the following assumption on what is going on:

I think [`Url`](https://docs.rs/reqwest/0.10.0-alpha.1/reqwest/struct.Url.html) is used first to parse the provided URL ([here](https://docs.rs/reqwest/0.10.0-alpha.1/reqwest/struct.Client.html#method.get)). Then the assumption is made this URL is valid once parsing succeeds. Internally, when making a request through `hyper`, this URL is parsed to an [`Uri`](https://docs.rs/hyper/0.13.0-alpha.4/hyper/struct.Uri.html). Assuming the URL is valid this _should_ always succeed, but it does not. That probably means that either implementation of [`Url`](https://docs.rs/reqwest/0.10.0-alpha.1/reqwest/struct.Url.html) or [`Uri`](https://docs.rs/hyper/0.13.0-alpha.4/hyperj/struct.Uri.html) is incorrect, or at least not compatible.

Al right. I'm not too familiar with the URL/URI specs, and don't want to go through it right now, to confirm either implementation is correct.

### Fixing this
I see the following options:

- Immediately parse to an `Uri` when creating the request (and check for validity).
- Fix [`Url`](https://docs.rs/reqwest/0.10.0-alpha.1/reqwest/struct.Url.html) implementation (if invalid)
- Fix [`Uri`](https://docs.rs/hyper/0.13.0-alpha.4/hyperj/struct.Uri.html) implementation (if invalid)
- Use `Uri` throughout the crate.
- Propagate this parsing issue up to the calling function, return as error in a `Result`.

I think the first is easiest to do on a short notice.

What would be the correct way of doing things? What are your thoughts, is the assumption I made right, and is this undesired behaviour?

I might be happy to implement a fix for this in code once I know what path I should take.


Discussion / hints:
Repro'd on the [`Url` side of things](https://github.com/servo/rust-url/issues/552) 
Here are a few other random ones that came up in my logs, parseable as `Url` but not as `Uri`:

```rust
"http://‘nvme@nip2"
"http://-1@"
"https://a@b"
"https://€@qq"
```
As a non-ideal bandage, I'm using these in the short term: https://stackoverflow.com/questions/3809401/what-is-a-good-regular-expression-to-match-a-url

`https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)`
> `https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)`

Thanks, as a workaround, I'm currently actually testing whether it will parse as `Uri`, to be 100% sure it works before providing it to `reqwest`.
I believe `URI` is more forgiving in terms of the spec right?
> I believe `URI` is more forgiving in terms of the spec right?

Not sure, it might be. But the panic came up because an `Url` (successfully parsed) couldn't be parsed as `Uri` (internally in `reqwest`). Testing this exact transformation before giving the URL to `reqwest` allows you to test whether it would panic or not, that's why I'm using this approach.

But in the end I think this issue should be fixed no matter what, of course.
I've also hit this bug, on URLs that contain "<" and ">".

LIke @timvisee I am parsing URLs as `http::Uri` first, which does stop panics.

It seems to me that reqwest should parse as `http::Uri` as soon as possible, and return an error to the caller if it isn't valid.
> It seems to me that reqwest should parse as http::Uri as soon as possible, and return an error to the caller if it isn't valid.

I don't think `Uri` can directly replace `Url` in the request struct, because `Url` is used to manage various URL parts. An explicit parse check to mitigate this panic upon creating the `Url` could be done, but this introduces undesirable overhead.

I believe https://github.com/seanmonstar/reqwest/issues/668#issuecomment-540310377 is correct, in that the best approach would be to fix the `Url` implementation.
I don't believe this is necessarily a bug in `url`, as the URL spec allows for parsing a *lot* of non-sensical URLs.

As you've mentioned, reqwest should likely check sooner that the URL can also be a valid URI.
My code panicked tonight and because the fix should be the same, I’m adding my example to this issue.

I am using a Telegram bot to inform me when something goes wrong. To send a message, you call a URL and pass the message in the query part of it. Unfortunately, because I included some logs in that message, the URL got too long and reqwest paniced at `src/into_url.rs:44`.

I’d expect that `.send()` returns a `Result::Err` instead.

### Example to reproduce

```rust
use reqwest::blocking::Client; // reqwest 0.10.9
use url::Url; // url 2.2.0

fn main() {
    let text = "abcdefghijklmnopqrstuvwxyz".repeat(100_000);

    let mut url = Url::parse("https://www.example.com/").unwrap();
    url.query_pairs_mut().append_pair("text", &text);

    Client::new()
        .get(url)
        .send() // panics
        .expect("THIS IS NOT REACHED");
}
```

```toml
[package]
name = "bug"
version = "0.1.0"
edition = "2018"

[dependencies]
reqwest = { version = "0.10.9", features = [ "blocking" ] }
url = "2.2.0"
```
@gralpli I'm pretty sure your issue here is because you have way to much char in your url due to the `repeat(100_000)`.
I've not looked at the underlying URL lib being used to parse the Url, but it should be normal that they have hardcoded a maximum length. 

Can you try without the repeat and check if it works? 
@Martichou Yes, the intended issue is that the URL is too long. But it is reqwest‘s `.send()` that panics. It should return a `Result::Err` instead.
@gralpli Oh yeah I see what you mean.
Indeed it would be better, don't know if @seanmonstar have the same opinion.

Fix patch:
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
index b6b515b89..11d9106dc 100644
--- a/src/async_impl/client.rs
+++ b/src/async_impl/client.rs
@@ -37,7 +37,7 @@ use crate::cookie;
 use crate::dns::trust_dns::TrustDnsResolver;
 use crate::dns::{gai::GaiResolver, DnsResolverWithOverrides, DynResolver, Resolve};
 use crate::error;
-use crate::into_url::{expect_uri, try_uri};
+use crate::into_url::try_uri;
 use crate::redirect::{self, remove_sensitive_headers};
 #[cfg(feature = "__tls")]
 use crate::tls::{self, TlsBackend};
@@ -1797,7 +1797,10 @@ impl Client {
             }
         }
 
-        let uri = expect_uri(&url);
+        let uri = match try_uri(&url) {
+            Ok(uri) => uri,
+            _ => return Pending::new_err(error::url_invalid_uri(url)),
+        };
 
         let (reusable, body) = match body {
             Some(body) => {
@@ -2165,7 +2168,8 @@ impl PendingRequest {
         }
         self.retry_count += 1;
 
-        let uri = expect_uri(&self.url);
+        // If it parsed once, it should parse again
+        let uri = try_uri(&self.url).expect("URL was already validated as URI");
 
         *self.as_mut().in_flight().get_mut() = match *self.as_mut().in_flight().as_ref() {
             #[cfg(feature = "http3")]
@@ -2338,7 +2342,7 @@ impl Future for PendingRequest {
                     //
                     // If not, just log it and skip the redirect.
                     let loc = loc.and_then(|url| {
-                        if try_uri(&url).is_some() {
+                        if try_uri(&url).is_ok() {
                             Some(url)
                         } else {
                             None
@@ -2379,7 +2383,7 @@ impl Future for PendingRequest {
                                 std::mem::replace(self.as_mut().headers(), HeaderMap::new());
 
                             remove_sensitive_headers(&mut headers, &self.url, &self.urls);
-                            let uri = expect_uri(&self.url);
+                            let uri = try_uri(&self.url)?;
                             let body = match self.body {
                                 Some(Some(ref body)) => Body::reusable(body.clone()),
                                 _ => Body::empty(),
@@ -2479,7 +2483,7 @@ fn add_cookie_header(headers: &mut HeaderMap, cookie_store: &dyn cookie::CookieS
 #[cfg(test)]
 mod tests {
     #[tokio::test]
-    async fn execute_request_rejects_invald_urls() {
+    async fn execute_request_rejects_invalid_urls() {
         let url_str = "hxxps://www.rust-lang.org/";
         let url = url::Url::parse(url_str).unwrap();
         let result = crate::get(url.clone()).await;
@@ -2489,4 +2493,17 @@ mod tests {
         assert!(err.is_builder());
         assert_eq!(url_str, err.url().unwrap().as_str());
     }
+
+    /// https://github.com/seanmonstar/reqwest/issues/668
+    #[tokio::test]
+    async fn execute_request_rejects_invalid_hostname() {
+        let url_str = "https://{{hostname}}/";
+        let url = url::Url::parse(url_str).unwrap();
+        let result = crate::get(url.clone()).await;
+
+        assert!(result.is_err());
+        let err = result.err().unwrap();
+        assert!(err.is_builder());
+        assert_eq!(url_str, err.url().unwrap().as_str());
+    }
 }
diff --git a/src/error.rs b/src/error.rs
index 9453fcd92..0d405a929 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -275,6 +275,10 @@ pub(crate) fn url_bad_scheme(url: Url) -> Error {
     Error::new(Kind::Builder, Some(BadScheme)).with_url(url)
 }
 
+pub(crate) fn url_invalid_uri(url: Url) -> Error {
+    Error::new(Kind::Builder, Some("Parsed Url is not a valid Uri")).with_url(url)
+}
+
 if_wasm! {
     pub(crate) fn wasm(js_val: wasm_bindgen::JsValue) -> BoxError {
         format!("{:?}", js_val).into()
diff --git a/src/into_url.rs b/src/into_url.rs
index d60f5c952..a46ae5702 100644
--- a/src/into_url.rs
+++ b/src/into_url.rs
@@ -74,14 +74,10 @@ impl<'a> IntoUrlSealed for String {
 }
 
 if_hyper! {
-    pub(crate) fn expect_uri(url: &Url) -> http::Uri {
+    pub(crate) fn try_uri(url: &Url) -> crate::Result<http::Uri> {
         url.as_str()
             .parse()
-            .expect("a parsed Url should always be a valid Uri")
-    }
-
-    pub(crate) fn try_uri(url: &Url) -> Option<http::Uri> {
-        url.as_str().parse().ok()
+            .map_err(|_| crate::error::url_invalid_uri(url.clone()))
     }
 }
 


Test patch: