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

cmd/go: get should always send go-get parameter, even after redirects #23081

Open
terinjokes opened this Issue Dec 11, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@terinjokes
Contributor

terinjokes commented Dec 11, 2017

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

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/terin/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build604286265=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Ran go get -v -insecure example.org/pkg/foo whilst "example.org" was directed to a local HTTP server (via host file).

What did you expect to see?

Code downloaded from a VCS at $GOPATH/src/example.org/pkg/foo.

What did you see instead?

Fetching https://example.org/pkg/foo?go-get=1
https fetch failed: Get https://example.org/pkg/foo?go-get=1: http: server gave HTTP response to HTTPS client
Fetching http://example.org/pkg/foo?go-get=1
Parsing meta tags from http://example.org/pkg/foo?go-get=1 (status code 500)
package example.org/pkg/foo: unrecognized import path "example.org/pkg/foo" (parse http://example.org/pkg/foo?go-get=1: no go-import meta tags ())

The behavior of the server is as follows:

  • If the request method is not GET or HEAD, responds with a 405 Method Not Allowed
  • If the request URL path does not end in a slash, it responds with a 301 Moved Permanently to the URL path with the slash appended
  • If the request URL contains the query parameter "go-get=1", it responds with a basic HTTP page with the go-imports meta tag
  • If the request URL does not contain the query parameter, it responds with a 303 See Other redirect to godoc.org

Due to an issue in the server's HTTP muxer implementation[^1], the query parameters during the 301 redirect are lost. go get dutifully follows the redirect, and as the request no longer contains the "go-get" parameter, it gets and follows the redirection to godoc.org, which responds with a 500 HTTP error.

This seems to be allowed behavior from the HTTP specification; the server should include the query parameters in the Location header to preserve semantics. However, I still found the behavior of go get surprising, both that it will sends requests without the "go-get" query parameter, and that it silently follows redirects off the import path domain.

I propose change the behavior of go get to always include the "go-get" query parameter, even after redirects, and to log the redirections when the "-v" flag is provided.


[^1]: Fortunately, it looks like that HTTP muxer is getting fixed sometime soon CL#61210. Now that this author knows about it, he can work around it.

@odeke-em

This comment has been minimized.

Member

odeke-em commented Dec 11, 2017

@terinjokes thank you for filing this issue.
So CL 61210 is included in Go1.10 beta 1(eventually Go1.10), and the beta 1 that was released mid last week https://twitter.com/golang/status/938920118293286913. Could you please try with that?

@bradfitz bradfitz added this to the Go1.11 milestone Dec 11, 2017

@terinjokes

This comment has been minimized.

Contributor

terinjokes commented Dec 12, 2017

@odeke-em I can confirm that for mux.HandleFunc('/pkg/foo/', http.HandlerFunc) a request for "example.com/pkg/foo" now responds with a 301 Moved Permanently redirect that includes the query parameters. go get, of any vintage, should work with this server.

Running go1.10beta1 get -v -insecure example.org/pkg/foo with a server from 1.9 results in the same behavior described here. (Since nothing's changed, this is as I expect)

An aside, CL 61210 looks to be incomplete. When a handler is added for a pattern that is prefixed with a host name (as documented in http.ServeMux), the request with the trailing slash works as expected, but a request without the trailing slash gets a 404 response; the path argument to shouldRedirect has had the host stripped away, but the map key contains the host. If someone doesn't beat me to it, I can try to fix this and open a CL.

@terinjokes

This comment has been minimized.

Contributor

terinjokes commented Dec 19, 2017

Quick aside: I opened #23183 for the 404 response issue mentioned above.

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 18, 2018

I think adding ?go-get=1 to redirected URLs is a good idea.

@rsc rsc added NeedsFix and removed NeedsDecision labels Apr 18, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 18, 2018

/cc @bcmills

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 10, 2018

This has been going on forever, so it can persist another release.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 10, 2018

@bcmills bcmills modified the milestones: Go1.12, Go1.13 Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment