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.

Sign up for GitHub

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

Jump to bottom

issue-1538-troubled-proxy-parsing #1539

Merged
merged 5 commits into from May 5, 2022
Merged

issue-1538-troubled-proxy-parsing #1539

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4633ab9
Check for schema during URL parse error handling. Lots of unit tests.
May 4, 2022
a03207d
Update from Check Style failure.
May 4, 2022
cc9b090
Another attempt at cargo fmt.
May 4, 2022
5802566
Fix a Linux-ism.
May 4, 2022
47e331a
Introduce BadScheme; an error source. Change schema to scheme. Use …
May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
252 changes: 247 additions & 5 deletions src/proxy.rs
View file
Open in desktop
Expand Up @@ -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")]
Expand Down Expand Up @@ -124,13 +123,36 @@ impl<S: IntoUrl> 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(|_| {
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::<url::ParseError>() {
match parse_error {
url::ParseError::RelativeUrlWithoutBase => {
presumed_to_have_schema = false;
break;
}
_ => {}
}
} else {
let text = format!("{}", err);
if text == "URL scheme is not allowed" {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matching on the Display output of an error makes me nervous. In pretty much all error types, the output is not considered stable. Is there no other way to check? Is this check important?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. That's not my proudest moment.

The check is important. The domain:port case ends up here. The string comparison was the easiest way to check for that error. I think a better option would be to carry a typed thing instead of a string. There's some risk but the reward is a better solution. I'll poke at it for a bit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, if that error is owned by the reqwest crate, we can just make it a private unit struct, instead of a string, and then check via downcasting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Just pushed the update. Windows looks good. Checking Linux. Running cargo fmt. All good!

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)
Expand Down Expand Up @@ -1511,3 +1533,223 @@ mod tests {
);
}
}

#[cfg(test)]
mod test {
mod into_proxy_scheme {
use crate::Proxy;
use std::error::Error;
use std::mem::discriminant;

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::<url::ParseError>() {
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);
}
}
}
}
}