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/http: on CheckRedirect failure, error message should refer to the original (redirected) URL, not the rejected redirect #34080

Open
bcmills opened this issue Sep 4, 2019 · 4 comments
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 4, 2019

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

example.com$ go1.13 version
go version go1.13 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
example.com$ go1.13 env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/usr/local/google/home/bcmills/.cache/go-build"
GOENV="/usr/local/google/home/bcmills/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/tmp/tmp.V4fL5k5JdS/_gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/google/home/bcmills/sdk/go1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/google/home/bcmills/sdk/go1.13/pkg/tool/linux_amd64"
GCCGO="/usr/local/google/home/bcmills/bin/gccgo"
AR="ar"
CC="gcc"
CXX="c++"
CGO_ENABLED="1"
GOMOD="/tmp/tmp.V4fL5k5JdS/example.com/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-build400745746=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran the program found at https://play.golang.org/p/42egMHzeuTR.

example.com$ cat >./main.go <<EOF
package main

import (
	"fmt"
	"net/http"
)

var securityPreservingHTTPClient = &http.Client{
	CheckRedirect: func(req *http.Request, via []*http.Request) error {
		if len(via) > 0 && via[0].URL.Scheme == "https" && req.URL.Scheme != "https" {
			lastHop := via[len(via)-1].URL
			return fmt.Errorf("redirected from secure URL %s to insecure URL %s", lastHop, req.URL)
		}
		return nil
	},
}

func main() {
	_, err := securityPreservingHTTPClient.Get("https://vcs-test.golang.org/insecure/go/insecure")
	fmt.Println(err)
}
EOF

example.com$ cat >./go.mod <<EOF
module example.com

go 1.13
EOF

example.com$ go1.13 run .

What did you expect to see?

Get "https://vcs-test.golang.org/insecure/go/insecure": redirected from secure URL https://vcs-test.golang.org/insecure/go/insecure to insecure URL http://vcs-test.golang.org/go/insecure

Since the http.Client should not have attempted to fetch the insecure URL in the first place, the insecure URL should not be the one reported for the failed Get operation.

What did you see instead?

Get "http://vcs-test.golang.org/go/insecure": redirected from secure URL https://vcs-test.golang.org/insecure/go/insecure to insecure URL http://vcs-test.golang.org/go/insecure

The URL that follows the Get token is one for which no HTTP GET was actually attempted.

CC @bradfitz

@bcmills bcmills added this to the Go1.14 milestone Sep 4, 2019
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Sep 26, 2019

Hey @bcmills thanks for filing this issue!

Firstly here is a standalone repro so that the code can be easily adapted to a test https://play.golang.org/p/uEh5qr5Cq_K and inlined

package main

import (
	"fmt"
	"log"
	"net/http"
	"net/http/httptest"
)

var _2HopsMaxClient = &http.Client{
	CheckRedirect: func(req *http.Request, via []*http.Request) error {
		if len(via) > 2 {
			return fmt.Errorf("max 2 hops")
		}
		return nil
	},
}

func main() {
	cst1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("Hello, world!"))
	}))
	defer cst1.Close()

	cst2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Location", cst1.URL)
		w.WriteHeader(308)
	}))
	defer cst2.Close()

	cst3 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Location", cst2.URL)
		w.WriteHeader(308)
	}))
	defer cst3.Close()

	cst4 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Location", cst3.URL)
		w.WriteHeader(308)
	}))
	defer cst4.Close()
        println("original URL: ", cst4.URL)

	if _, err := _2HopsMaxClient.Get(cst4.URL); err != nil {
		log.Fatalf("Failed to fetch URL: %v\n", err)
	}
}

So while I understand that pointing you directly to the parent URL in the subject clause with

<VERB> "<START_URL>"

I believe that unfortunately such a change would break usability for code with multiple redirects e.g. 10 redirects in and then an obscure failure, for which it is vital for one to know which URL exactly the failure occurred on, but it also will break code for users who've had it like this since Go1.7 (4 years ago) when that code was added.

With your CheckRedirect, you have access to the first URL and you can even compose for your use case a custom error that you can then unwrap since *url.Error is always returned i.e. invoke (*url.Error).Unwrap https://golang.org/pkg/net/url/#Error.Unwrap. Please let me know what you think.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Sep 26, 2019

I believe that unfortunately such a change would break usability for code with multiple redirects e.g. 10 redirects in and then an obscure failure, for which it is vital for one to know which URL exactly the failure occurred on

That depends, I think. There are two ways one might want to work around a rejected redirect: one is by fixing the server to avoid issuing that redirect, and the other is by fixing the client to avoid trying to fetch the problematic URL.

I think most Go users are in more of a position to avoid fetching a URL — or to contact the owner of a public API endpoint — than to fix an Nth-level redirect within that public endpoint's CDN configuration.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Sep 26, 2019

it also will break code for users who've had it like this since Go1.7 (4 years ago) when that code was added.

Go error messages are generally not covered under the Go 1 compatibility guarantee.

Whether the fallout in practice from changing this error message would be too large, I do not know.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Sep 26, 2019

With your CheckRedirect, you have access to the first URL and you can even compose for your use case a custom error that you can then unwrap since *url.Error is always returned

The problem here is the default behavior. Yes, we can work around the subpar error message by adding more redundancy — but that won't make the failure condition any clearer for users who don't already know about the problem (and thus don't know to apply the workaround).

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.