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

net/url: Parse returns URL whose Host contains colon #25511

Closed
crvv opened this issue May 23, 2018 · 7 comments
Closed

net/url: Parse returns URL whose Host contains colon #25511

crvv opened this issue May 23, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@crvv
Copy link
Contributor

crvv commented May 23, 2018

What version of Go are you using (go version)?

1.10.2 and tip(5776bd5)

Does this issue reproduce with the latest release?

Yes

What did you do?

https://play.golang.org/p/vYNt0qipkxf
Parse an invalid URL, http://socks5h://server

What did you see?

&url.URL{Scheme:"http", Host:"socks5h:", Path:"//server"}

What did you expect to see?

An error.
As per https://tools.ietf.org/html/rfc3986#section-3.2.2, host can't contain :

host        = IP-literal / IPv4address / reg-name
reg-name    = *( unreserved / pct-encoded / sub-delims )
unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
pct-encoded = "%" HEXDIG HEXDIG
reserved    = gen-delims / sub-delims
gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"
sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
            / "*" / "+" / "," / ";" / "="
@fraenkel
Copy link
Contributor

While you are correct about : being invalid, the URL.Host allows host:port which is why the : is present.

@andybons
Copy link
Member

@bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 23, 2018
@andybons andybons added this to the Unplanned milestone May 23, 2018
@bradfitz
Copy link
Contributor

Every time we change the URL package we break tons of code. That might not happen in this case, but we've said that dozens of times before.

So I'd rather not change anything here. We don't strictly follow the RFCs for legacy reasons. There would need to be a good reason to change something here.

I also note that if I copy/paste these into Chrome, it works fine:

     https://golang.org:/
     https://golang.org:/ref/spec

(note the colons in the authority with an empty port)

I also note that this doesn't break the Port method on URL: https://play.golang.org/p/I3OO4NvwBTb

So what do we gain by changing something?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 23, 2018
@crvv
Copy link
Contributor Author

crvv commented May 24, 2018

The issue comes up when I use proxy for http package.

package main
import "net/http"
func main() {
    _, err := http.Get("https://golang.org")
    println(err.Error())
}

And run it with HTTPS_PROXY=socks5h://localhost go run main.go
The error is Get https://golang.org: proxyconnect tcp: dial tcp: lookup socks5h: no such host
I think this is an unexpected error.

http package prepends http:// to the proxy url at this line.

if proxyURL, err := url.Parse("http://" + proxy); err == nil {

Then http will try to connect to socks5h:.
Maybe it is better to handle this in http package. #24135

@crvv
Copy link
Contributor Author

crvv commented May 24, 2018

For url package, I just found a duplicate issue
#14836

According to https://tools.ietf.org/html/rfc3986#section-6.2.3
http://example.com:/ is a valid url, and its host is example.com, not example.com:
But I think this should be handled in url package, not in http.

@crvv
Copy link
Contributor Author

crvv commented May 24, 2018

I just found that url accepts http://fe80::/foo, and host is an IPv6 address fe80::
Then trimming the rightmost : correctly is not trivial
So I don't think it's worth changing anything for this

@adamdecaf
Copy link
Contributor

So I don't think it's worth changing anything for this

Sounds like we should close this then?

@crvv crvv closed this as completed May 25, 2018
@golang golang locked and limited conversation to collaborators May 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants