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: parsing URLs with port > 65536 should fail #69443

Open
dunglas opened this issue Sep 13, 2024 · 5 comments · May be fixed by #69444
Open

net/url: parsing URLs with port > 65536 should fail #69443

dunglas opened this issue Sep 13, 2024 · 5 comments · May be fixed by #69444
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dunglas
Copy link
Contributor

dunglas commented Sep 13, 2024

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/dunglas/Library/Caches/go-build'
GOENV='/Users/dunglas/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/dunglas/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/dunglas/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/dunglas/go/pkg/mod/golang.org/[email protected]'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='go1.22.0+auto'
GOTOOLDIR='/Users/dunglas/go/pkg/mod/golang.org/[email protected]/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/nf/_zwcfbtx7m9d5g8jgympqy5r0000gn/T/go-build2393813559=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

url.Parse("https://example.org:70000")

What did you see happen?

Parsing succeeds, and no error is returned.

What did you expect to see?

Per the WHATWG Living Standard (and, if I'm not mistaken, internet RFCs), parsing should fail because the port is out of range: https://url.spec.whatwg.org/#port-out-of-range

dunglas added a commit to dunglas/go that referenced this issue Sep 13, 2024
The existing implementation does not validate
that the port number is in the allowed range.

WHATWG URL Living Standard mandates that
parsing URLs with invalid ports fails:
https://url.spec.whatwg.org/#port-out-of-range

Fixes golang#69443.
@dunglas dunglas linked a pull request Sep 13, 2024 that will close this issue
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/613035 mentions this issue: net/url: return an error if port is out of range

@timothy-king timothy-king added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 13, 2024
@timothy-king timothy-king added this to the Backlog milestone Sep 13, 2024
@timothy-king
Copy link
Contributor

CC @neild, @rsc.

@ianlancetaylor
Copy link
Contributor

The net/url package tries to follow RFC 3986, which does not impose any limit on the port number. In particular net/url works for schemes other than http, and there is no requirement that it based on TCP. Admittedly in practice it is based on TCP, so this may be splitting hairs. But I'm concerned that adding this kind of check will break existing working code.

Naturally actually attempting to use an HTTP URL with a large port will fail with an error like dial tcp: address 70000: invalid port.

I'm not really opposed to making this kind of change, I just want to raise the opposing viewpoint.

@puellanivis
Copy link

I recall a bit the frustration that occurred when we introduced ports needing to be strictly decimal. There were a few libraries (particularly MySQL’s driver) that broke because their DSNs were using tcp(...) encapsulated authority sections so that they could support TCP and unix(...) for Unix sockets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants