From 4633ab9b37233101a1faa8d62f46c2009df831b2 Mon Sep 17 00:00:00 2001 From: brian-cook Date: Wed, 4 May 2022 16:09:44 -0400 Subject: [PATCH 1/5] Check for schema during URL parse error handling. Lots of unit tests. --- src/proxy.rs | 246 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 239 insertions(+), 7 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index b4387c36e..d6861d366 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -124,13 +124,38 @@ impl IntoProxyScheme for S { let url = match self.as_str().into_url() { Ok(ok) => ok, Err(e) => { - // the issue could have been caused by a missing scheme, so we try adding http:// - format!("http://{}", self.as_str()) - .into_url() - .map_err(|_| { - // return the original error - crate::error::builder(e) - })? + let mut presumed_to_have_schema = true; + let mut source = e.source(); + while let Some(err) = source { + if let Some(parse_error) = err.downcast_ref::() { + match parse_error { + url::ParseError::RelativeUrlWithoutBase => { + presumed_to_have_schema = false; + break; + } + _ => {} + } + } else { + let text = format!("{}", err); + if text == "URL scheme is not allowed" { + presumed_to_have_schema = false; + break; + } + } + source = err.source(); + } + if ! presumed_to_have_schema { + // the issue could have been caused by a missing scheme, so we try adding http:// + let try_this = format!("http://{}", self.as_str()); + try_this + .into_url() + .map_err(|_| { + // return the original error + crate::error::builder(e) + })? + } else { + return Err(crate::error::builder(e)); + } } }; ProxyScheme::parse(url) @@ -1511,3 +1536,210 @@ mod tests { ); } } + +#[cfg(test)] +mod test { + mod into_proxy_scheme { + use std::error::Error; + use std::mem::discriminant; + use crate::Proxy; + + fn includes(haystack: &crate::error::Error, needle: url::ParseError) -> bool { + let mut source = haystack.source(); + while let Some(error) = source { + if let Some(parse_error) = error.downcast_ref::() { + if discriminant(parse_error) == discriminant(&needle) { + return true; + } + } + source = error.source(); + } + false + } + + fn check_parse_error(url: &str, needle: url::ParseError) { + let error = Proxy::http(url).unwrap_err(); + if !includes(&error, needle ) { + panic!("{:?} expected; {:?}, {} found", needle, error, error); + } + } + + mod when_schema_missing { + mod and_url_is_valid { + use crate::Proxy; + + #[test] + fn lookback_works() { + let _ = Proxy::http("127.0.0.1").unwrap(); + } + + #[test] + fn loopback_port_works() { + let _ = Proxy::http("127.0.0.1:8080").unwrap(); + } + + #[test] + fn loopback_username_works() { + let _ = Proxy::http("username@127.0.0.1").unwrap(); + } + + #[test] + fn loopback_username_password_works() { + let _ = Proxy::http("username:password@127.0.0.1").unwrap(); + } + + #[test] + fn loopback_username_password_port_works() { + let _ = Proxy::http("ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); + } + + #[test] + fn domain_works() { + let _ = Proxy::http("proxy.example.com").unwrap(); + } + + #[test] + fn domain_port_works() { + let _ = Proxy::http("proxy.example.com:8080").unwrap(); + } + + #[test] + fn domain_username_works() { + let _ = Proxy::http("username@proxy.example.com").unwrap(); + } + + #[test] + fn domain_username_password_works() { + let _ = Proxy::http("username:password@proxy.example.com").unwrap(); + } + + #[test] + fn domain_username_password_port_works() { + let _ = Proxy::http("ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080").unwrap(); + } + } + mod and_url_has_bad { + use super::super::check_parse_error; + + #[test] + fn host() { + check_parse_error("username@", url::ParseError::RelativeUrlWithoutBase ); + } + + #[test] + fn idna_encoding() { + check_parse_error("xn---", url::ParseError::RelativeUrlWithoutBase); + } + + #[test] + fn port() { + check_parse_error("127.0.0.1:808080", url::ParseError::RelativeUrlWithoutBase); + } + + #[test] + fn ip_v4_address() { + check_parse_error("421.627.718.469", url::ParseError::RelativeUrlWithoutBase); + } + + #[test] + fn ip_v6_address() { + check_parse_error("[56FE::2159:5BBC::6594]", url::ParseError::RelativeUrlWithoutBase); + } + + #[test] + fn invalid_domain_character() { + check_parse_error("abc 123", url::ParseError::RelativeUrlWithoutBase); + } + } + } + + mod when_schema_present { + mod and_url_is_valid { + use crate::Proxy; + + #[test] + fn loopback_works() { + let _ = Proxy::http("http://127.0.0.1").unwrap(); + } + + #[test] + fn loopback_port_works() { + let _ = Proxy::http("https://127.0.0.1:8080").unwrap(); + } + + #[test] + fn loopback_username_works() { + let _ = Proxy::http("http://username@127.0.0.1").unwrap(); + } + + #[test] + fn loopback_username_password_works() { + let _ = Proxy::http("https://username:password@127.0.0.1").unwrap(); + } + + #[test] + fn loopback_username_password_port_works() { + let _ = Proxy::http("http://ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); + } + + #[test] + fn domain_works() { + let _ = Proxy::http("https://proxy.example.com").unwrap(); + } + + #[test] + fn domain_port_works() { + let _ = Proxy::http("http://proxy.example.com:8080").unwrap(); + } + + #[test] + fn domain_username_works() { + let _ = Proxy::http("https://username@proxy.example.com").unwrap(); + } + + #[test] + fn domain_username_password_works() { + let _ = Proxy::http("http://username:password@proxy.example.com").unwrap(); + } + + #[test] + fn domain_username_password_port_works() { + let _ = Proxy::http("https://ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080").unwrap(); + } + } + mod and_url_has_bad { + use super::super::check_parse_error; + + #[test] + fn host() { + check_parse_error("http://username@", url::ParseError::EmptyHost); + } + + #[test] + fn idna_encoding() { + check_parse_error("http://xn---", url::ParseError::IdnaError); + } + + #[test] + fn port() { + check_parse_error("http://127.0.0.1:808080", url::ParseError::InvalidPort); + } + + #[test] + fn ip_v4_address() { + check_parse_error("http://421.627.718.469", url::ParseError::InvalidIpv4Address); + } + + #[test] + fn ip_v6_address() { + check_parse_error("http://[56FE::2159:5BBC::6594]", url::ParseError::InvalidIpv6Address); + } + + #[test] + fn invalid_domain_character() { + check_parse_error("http://abc 123/", url::ParseError::InvalidDomainCharacter); + } + } + } + } +} From a03207d7aa7b142e00610c11b9133e78ce1c0078 Mon Sep 17 00:00:00 2001 From: brian-cook Date: Wed, 4 May 2022 18:06:18 -0400 Subject: [PATCH 2/5] Update from Check Style failure. --- src/proxy.rs | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index d6861d366..14bedcb50 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -144,15 +144,13 @@ impl IntoProxyScheme for S { } source = err.source(); } - if ! presumed_to_have_schema { + if !presumed_to_have_schema { // the issue could have been caused by a missing scheme, so we try adding http:// let try_this = format!("http://{}", self.as_str()); - try_this - .into_url() - .map_err(|_| { - // return the original error - crate::error::builder(e) - })? + try_this.into_url().map_err(|_| { + // return the original error + crate::error::builder(e) + })? } else { return Err(crate::error::builder(e)); } @@ -1540,9 +1538,9 @@ mod tests { #[cfg(test)] mod test { mod into_proxy_scheme { + use crate::Proxy; use std::error::Error; use std::mem::discriminant; - use crate::Proxy; fn includes(haystack: &crate::error::Error, needle: url::ParseError) -> bool { let mut source = haystack.source(); @@ -1559,7 +1557,7 @@ mod test { fn check_parse_error(url: &str, needle: url::ParseError) { let error = Proxy::http(url).unwrap_err(); - if !includes(&error, needle ) { + if !includes(&error, needle) { panic!("{:?} expected; {:?}, {} found", needle, error, error); } } @@ -1590,7 +1588,8 @@ mod test { #[test] fn loopback_username_password_port_works() { - let _ = Proxy::http("ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); + let _ = + Proxy::http("ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); } #[test] @@ -1615,7 +1614,8 @@ mod test { #[test] fn domain_username_password_port_works() { - let _ = Proxy::http("ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080").unwrap(); + let _ = + Proxy::http("ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080").unwrap(); } } mod and_url_has_bad { @@ -1623,7 +1623,7 @@ mod test { #[test] fn host() { - check_parse_error("username@", url::ParseError::RelativeUrlWithoutBase ); + check_parse_error("username@", url::ParseError::RelativeUrlWithoutBase); } #[test] @@ -1638,12 +1638,18 @@ mod test { #[test] fn ip_v4_address() { - check_parse_error("421.627.718.469", url::ParseError::RelativeUrlWithoutBase); + check_parse_error( + "421.627.718.469", + url::ParseError::RelativeUrlWithoutBase + ); } #[test] fn ip_v6_address() { - check_parse_error("[56FE::2159:5BBC::6594]", url::ParseError::RelativeUrlWithoutBase); + check_parse_error( + "[56FE::2159:5BBC::6594]", + url::ParseError::RelativeUrlWithoutBase + ); } #[test] @@ -1679,7 +1685,8 @@ mod test { #[test] fn loopback_username_password_port_works() { - let _ = Proxy::http("http://ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); + let _ = + Proxy::http("http://ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); } #[test] @@ -1732,7 +1739,10 @@ mod test { #[test] fn ip_v6_address() { - check_parse_error("http://[56FE::2159:5BBC::6594]", url::ParseError::InvalidIpv6Address); + check_parse_error( + "http://[56FE::2159:5BBC::6594]", + url::ParseError::InvalidIpv6Address + ); } #[test] From cc9b090750f5665a1a8f7ed87c273d6f9435ffe6 Mon Sep 17 00:00:00 2001 From: brian-cook Date: Wed, 4 May 2022 18:23:17 -0400 Subject: [PATCH 3/5] Another attempt at cargo fmt. --- src/proxy.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index 14bedcb50..574e2f47a 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -1588,8 +1588,7 @@ mod test { #[test] fn loopback_username_password_port_works() { - let _ = - Proxy::http("ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); + let _ = Proxy::http("ldap%5Cgremlin:pass%3Bword@127.0.0.1:8080").unwrap(); } #[test] @@ -1638,17 +1637,14 @@ mod test { #[test] fn ip_v4_address() { - check_parse_error( - "421.627.718.469", - url::ParseError::RelativeUrlWithoutBase - ); + check_parse_error("421.627.718.469", url::ParseError::RelativeUrlWithoutBase); } #[test] fn ip_v6_address() { check_parse_error( "[56FE::2159:5BBC::6594]", - url::ParseError::RelativeUrlWithoutBase + url::ParseError::RelativeUrlWithoutBase, ); } @@ -1711,7 +1707,9 @@ mod test { #[test] fn domain_username_password_port_works() { - let _ = Proxy::http("https://ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080").unwrap(); + let _ = + Proxy::http("https://ldap%5Cgremlin:pass%3Bword@proxy.example.com:8080") + .unwrap(); } } mod and_url_has_bad { @@ -1734,14 +1732,17 @@ mod test { #[test] fn ip_v4_address() { - check_parse_error("http://421.627.718.469", url::ParseError::InvalidIpv4Address); + check_parse_error( + "http://421.627.718.469", + url::ParseError::InvalidIpv4Address, + ); } #[test] fn ip_v6_address() { check_parse_error( "http://[56FE::2159:5BBC::6594]", - url::ParseError::InvalidIpv6Address + url::ParseError::InvalidIpv6Address, ); } From 5802566b888835f2de45f33144b86453eb3e2184 Mon Sep 17 00:00:00 2001 From: brian-cook Date: Wed, 4 May 2022 18:29:06 -0400 Subject: [PATCH 4/5] Fix a Linux-ism. --- src/proxy.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/proxy.rs b/src/proxy.rs index 574e2f47a..f5764b795 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -10,7 +10,6 @@ use ipnet::IpNet; use percent_encoding::percent_decode; use std::collections::HashMap; use std::env; -#[cfg(target_os = "windows")] use std::error::Error; use std::net::IpAddr; #[cfg(target_os = "windows")] From 47e331a6131b25c97171c6f2946f9ab0e5c145a1 Mon Sep 17 00:00:00 2001 From: brian-cook Date: Wed, 4 May 2022 23:20:13 -0400 Subject: [PATCH 5/5] Introduce BadScheme; an error source. Change schema to scheme. Use BadScheme instead of the error text to determine that a scheme is not present. --- src/error.rs | 13 ++++++++++++- src/proxy.rs | 31 ++++++++++++++----------------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/error.rs b/src/error.rs index 66550cc58..3f829d99a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -265,7 +265,7 @@ pub(crate) fn status_code(url: Url, status: StatusCode) -> Error { } pub(crate) fn url_bad_scheme(url: Url) -> Error { - Error::new(Kind::Builder, Some("URL scheme is not allowed")).with_url(url) + Error::new(Kind::Builder, Some(BadScheme)).with_url(url) } if_wasm! { @@ -306,6 +306,17 @@ impl fmt::Display for TimedOut { impl StdError for TimedOut {} +#[derive(Debug)] +pub(crate) struct BadScheme; + +impl fmt::Display for BadScheme { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("URL scheme is not allowed") + } +} + +impl StdError for BadScheme {} + #[cfg(test)] mod tests { use super::*; diff --git a/src/proxy.rs b/src/proxy.rs index f5764b795..e98d7ff6e 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -123,27 +123,24 @@ impl IntoProxyScheme for S { let url = match self.as_str().into_url() { Ok(ok) => ok, Err(e) => { - let mut presumed_to_have_schema = true; + let mut presumed_to_have_scheme = true; let mut source = e.source(); while let Some(err) = source { if let Some(parse_error) = err.downcast_ref::() { match parse_error { url::ParseError::RelativeUrlWithoutBase => { - presumed_to_have_schema = false; + presumed_to_have_scheme = false; break; } _ => {} } - } else { - let text = format!("{}", err); - if text == "URL scheme is not allowed" { - presumed_to_have_schema = false; - break; - } + } else if let Some(_) = err.downcast_ref::() { + presumed_to_have_scheme = false; + break; } source = err.source(); } - if !presumed_to_have_schema { + if !presumed_to_have_scheme { // the issue could have been caused by a missing scheme, so we try adding http:// let try_this = format!("http://{}", self.as_str()); try_this.into_url().map_err(|_| { @@ -1129,14 +1126,14 @@ mod tests { let disabled_proxies = get_sys_proxies(Some((0, String::from("http://127.0.0.1/")))); // set valid proxy let valid_proxies = get_sys_proxies(Some((1, String::from("http://127.0.0.1/")))); - let valid_proxies_no_schema = get_sys_proxies(Some((1, String::from("127.0.0.1")))); + let valid_proxies_no_scheme = get_sys_proxies(Some((1, String::from("127.0.0.1")))); let valid_proxies_explicit_https = get_sys_proxies(Some((1, String::from("https://127.0.0.1/")))); let multiple_proxies = get_sys_proxies(Some(( 1, String::from("http=127.0.0.1:8888;https=127.0.0.2:8888"), ))); - let multiple_proxies_explicit_schema = get_sys_proxies(Some(( + let multiple_proxies_explicit_scheme = get_sys_proxies(Some(( 1, String::from("http=http://127.0.0.1:8888;https=https://127.0.0.2:8888"), ))); @@ -1154,11 +1151,11 @@ mod tests { assert_eq!(p.scheme(), "http"); assert_eq!(p.host(), "127.0.0.1"); - let p = &valid_proxies_no_schema["http"]; + let p = &valid_proxies_no_scheme["http"]; assert_eq!(p.scheme(), "http"); assert_eq!(p.host(), "127.0.0.1"); - let p = &valid_proxies_no_schema["https"]; + let p = &valid_proxies_no_scheme["https"]; assert_eq!(p.scheme(), "http"); assert_eq!(p.host(), "127.0.0.1"); @@ -1174,11 +1171,11 @@ mod tests { assert_eq!(p.scheme(), "http"); assert_eq!(p.host(), "127.0.0.2:8888"); - let p = &multiple_proxies_explicit_schema["http"]; + let p = &multiple_proxies_explicit_scheme["http"]; assert_eq!(p.scheme(), "http"); assert_eq!(p.host(), "127.0.0.1:8888"); - let p = &multiple_proxies_explicit_schema["https"]; + let p = &multiple_proxies_explicit_scheme["https"]; assert_eq!(p.scheme(), "https"); assert_eq!(p.host(), "127.0.0.2:8888"); } @@ -1561,7 +1558,7 @@ mod test { } } - mod when_schema_missing { + mod when_scheme_missing { mod and_url_is_valid { use crate::Proxy; @@ -1654,7 +1651,7 @@ mod test { } } - mod when_schema_present { + mod when_scheme_present { mod and_url_is_valid { use crate::Proxy;