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

Don't panic when parsing invalid Url to Uri #2040

Merged
merged 1 commit into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/async_impl/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
Expand All @@ -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());
}
}
4 changes: 4 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
8 changes: 2 additions & 6 deletions src/into_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
}
}

Expand Down