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 thinks /// does not start an authority #46277

Open
TimothyGu opened this issue May 19, 2021 · 2 comments
Open

net/url: Parse thinks /// does not start an authority #46277

TimothyGu opened this issue May 19, 2021 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@TimothyGu
Copy link
Contributor

TimothyGu commented May 19, 2021

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

$ go version
go version devel go1.17-6c1c055d1e Wed May 19 16:03:23 2021 -0700 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/timothy-gu/.cache/go-build"
GOENV="/home/timothy-gu/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/timothy-gu/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/timothy-gu/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/timothy-gu/dev/go/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/timothy-gu/dev/go/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-a3a75c15dd Wed May 19 16:03:23 2021 -0700"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/timothy-gu/dev/go/go/src/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1447504073=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/eDpmB6DyF_f

What did you expect to see?

I expected the following URL to be parsed:

&url.URL{Path: "/threeslashes"}

This is since the first two slashes in ///threeslashes should start the host, while only the last slash belongs to the path.

What did you see instead?

&url.URL{Path: "///threeslashes"}

There's actually a test case for this in net/url already:

go/src/net/url/url_test.go

Lines 212 to 223 in 6c1c055

// Three leading slashes isn't an authority, but doesn't return an error.
// (We can't return an error, as this code is also used via
// ServeHTTP -> ReadRequest -> Parse, which is arguably a
// different URL parsing context, but currently shares the
// same codepath)
{
"///threeslashes",
&URL{
Path: "///threeslashes",
},
"",
},
However, the comment there is incorrect for two reasons:

  • The comment says that "Three leading slashes isn't an authority." But it is. RFC 3986 gives the following definition:

    relative-ref  = relative-part [ "?" query ] [ "#" fragment ]
    relative-part = "//" authority path-abempty
    authority   = [ userinfo "@" ] host [ ":" port ]
    host        = IP-literal / IPv4address / reg-name
    reg-name    = *( unreserved / pct-encoded / sub-delims )
    path-abempty  = *( "/" segment ) ; begins with "/" or is empty
    

    We see that authority can absolutely be the empty string. In fact, url.Parse no longer returns an error if the Host is empty, so the comment is moot.

  • The comment also says that ReadRequest depends on the url.Parse code path. However, this is not correct: it now uses url.ParseRequestURI, which does correctly parse the original URL as Path: "///threeslashes".

@TimothyGu
Copy link
Contributor Author

This is important when we resolve ///threeslashes against file://somehost/file. Technically, according to web browsers and Node.js, this should result in file:///threeslashes with the original somehost removed. However, Go currently does file://somehost///threeslashes.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 20, 2021
@seankhliao
Copy link
Member

cc @rsc @bradfitz

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
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

No branches or pull requests

2 participants