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

Explicit port 80 gets lost for http proxies #6

Closed
kinnison opened this issue Apr 27, 2019 · 6 comments

Comments

@kinnison
Copy link

commented Apr 27, 2019

Hi,

A user of rustup came to us yesterday with an issue they have. Their corporate web proxy runs on port 80, not port 8080 and they cannot use rustup via our env_proxy support (forcing them to fall back to cURL mode).

As an example, I wrote a trivial echo-proxy program:

fn main() {
    println!("{:?}", env_proxy::for_url(&url::Url::parse("http://www.example.org").unwrap()).to_url());
}

If you run it as:

$ env http_proxy=http://corporate-proxy:80 cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/echoproxy`
Some("http://corporate-proxy:8080/")
$

You can see that the explicit :80 on the proxy variable has been eaten somewhere along the way.

I believe I have traced this through env_proxy::ProxyUrl::to_url() through its use of url.port().is_some() (which seems reasonable) into url::parser::Parser::parse_port() where it has:

if !has_any_digit || opt_port == default_port() {
            opt_port = None;
}

This means that if a URL has an explicit :PORTNR where that matches the default port number for the scheme in question (http and :80 here) then it claims the incoming URL had no port number. As such your approach to detecting the port is not going to always work.

As I see it there are two options here:

  1. An alternative, more explicitly preserving-of-form URL parser is used
  2. A change to url is proposed in which the port (if present) is always recorded, and then perhaps not serialised back out if it's not needed.

Either way, right now this is blocking some rustup users from being able to use env_proxy and reqwest because of their corporate proxies using port 80 :(

@inejge

This comment has been minimized.

Copy link
Owner

commented Apr 27, 2019

@kinnison Nice analysis. While running a proxy on port 80 is... eccentric, it should be supported. The root of the problem is that a proxy URL uses the http scheme, but its default port is 1080 (cURL) or 8080 (env_proxy), instead of 80, and there is no way to tell that to Url::parse(). I know how I'd change url, and I'll propose the change by and by, but meanwhile I believe that I can patch env_proxy to sidestep the problem.

@inejge

This comment has been minimized.

Copy link
Owner

commented Apr 27, 2019

Ok, you can try the "explicit80" branch.

@kinnison

This comment has been minimized.

Copy link
Author

commented Apr 28, 2019

Okay, running that through my echo proxy test confirms that it works.

I am uncomfortable with the use of unsafe and while I understand the efficiency argument, I wonder if it might be better to have ProxyUrl carry an Option<String> for the scheme which should be used, and then unconditionally replace the scheme in the stored URL when it's set into ProxyUrl ? If you have a better discussion ongoing with url's upstream then ignore me :D

If you can make a release in which this functionality (in whatever form) is present, I'll udpate rustup for it.

thank you

@inejge inejge closed this in edc7120 Apr 28, 2019

@inejge

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

I lost the unsafe and 0.3.1 will be up on crates.io shortly.

@kinnison

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

Awesome, thank you so much @inejge -- I shall update rustup tonight and ask the original bug reporter to check.

@kinnison

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

Confirmed as good by the bug reporter, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.