Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Dec 3, 2021

WebProxy has new(Uri Address), new(string Address), new(string Host, int Port) constructors that all behave differently.

The new(string) overload will check if the address is an absolute Uri and add a "http://" prefix otherwise.
The new(string, int) overload will blindly concat the host and port $"http://{Host}:{Port}".

If you happen to create a WebProxy object like new WebProxy("socks5://127.0.0.1", 9050), what you actually end up with is a proxy to "http://socks5://127.0.0.1:9050" - an http proxy with the host socks5 and a default port.

We should either check if the host is already an absolute Uri like we do for other overloads (this PR), or we should throw. socks5://127.0.0.1 is not a valid host, it just happens to be interpreted as:
Host: socks5
Port: : (empty port is allowed and means "default for the scheme")
Path: //127.0.0.1

cc: @ManickaP who noticed our blog posts announcing socks support have broken code snippets.

@MihaZupan MihaZupan added this to the 7.0.0 milestone Dec 3, 2021
@MihaZupan MihaZupan requested a review from a team December 3, 2021 16:11
@ghost
Copy link

ghost commented Dec 3, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

WebProxy has new(Uri Address), new(string Address), new(string Host, int Port) constructors that all behave differently.

The new(string) overload will check if the address is an absolute Uri and add a "http://" prefix otherwise.
The new(string, int) overload will blindly concat the host and port $"http://{Host}:{Port}".

If you happen to create a WebProxy object like new WebProxy("socks5://127.0.0.1", 9050), what you actually end up with is a proxy to "http://socks5://127.0.0.1:9050" - an http proxy with the host socks5 and a default port.

We should either check if the host is already an absolute Uri like we do for other overloads (this PR), or we should throw. socks5://127.0.0.1 is not a valid host, it just happens to be interpreted as:
Host: socks5
Port: : (empty port is allowed and means "default for the scheme"
Path: //127.0.0.1

cc: @ManickaP who noticed our blog posts announcing socks support have broken code snippets.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net

Milestone: 7.0.0

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

That was super fast 😄

@MihaZupan MihaZupan merged commit f2c8f7c into dotnet:main Dec 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants