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 documentation does not adequately explain escaping rules or RFC compliance #30611

Open
neilalexander opened this issue Mar 5, 2019 · 6 comments

Comments

Projects
None yet
6 participants
@neilalexander
Copy link

commented Mar 5, 2019

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

$ go version
go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes, see playground.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/neilalexander/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/neilalexander/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cv/wv7k9w2s4qdfjfd60nd7_5t40000gn/T/go-build007721452=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

See playground:

if _, err := url.Parse("http://[fe80::1%en0]:12345"); err != nil {
	panic(err)
}

What did you expect to see?

IPv6 link-local literals should be accepted and fe80::1%en0 should be returned within the Host.

What did you see instead?

Parsing error due to invalid URL escape with the percent sign - in above example produces:

panic: parse http://[fe80::1%en0]:12345: invalid URL escape "%en"

@mikioh mikioh changed the title net/url: url.Parse fails on link-local IPv6 address literals net/url: Parse fails on link-local IPv6 address literals Mar 6, 2019

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

See https://tools.ietf.org/html/rfc6874, the delimiter is "%25". Also, #20669 might describe the limitation of IPv6 LLA /w zone in URI.

@mikioh mikioh closed this Mar 6, 2019

@neilalexander

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Thanks for the fast response. If not an issue with Go per-se then I consider this an issue with documentation, as the godoc for url.Parse does not state compliance to RFC 6874, nor does it suggest that there are particular escape codes that may cover situations like this.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Then, please re-title this issue.

@mikioh mikioh reopened this Mar 6, 2019

@neilalexander neilalexander changed the title net/url: Parse fails on link-local IPv6 address literals net/url: Parse documentation does not adequately explain escaping rules or RFC compliance Mar 6, 2019

@mikioh mikioh added the help wanted label Mar 6, 2019

@agnivade agnivade added this to the Unplanned milestone Mar 6, 2019

@cosmonawt

This comment has been minimized.

Copy link

commented Apr 5, 2019

Would adding "IPv6 addresses must be encoded according to RFC 3986 and 6874." to the documentation of Parse and ParseRequestURI adequately explain the rules? If yes, I would like to make a CL. However I see that compliance is already documented in line 625, so I am not sure if it is really needed.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@neilalexander, would @cosmonawt's proposed change address your issue?

(@rsc and @bradfitz are listed as the owners of this package, so I'll leave it to them to decide whether such a change would be a net improvement from a maintainer's perspective.)

@neilalexander

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

I think it would help, but it would be better if it makes an explicit mention of the fact that it affects link-local addresses containing the interface specifier as it will save people time in the future.

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