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: Redirect Referer uses previous URL instead of Referer #44160

Open
DemonTPx opened this issue Feb 8, 2021 · 5 comments
Open

net/http: Redirect Referer uses previous URL instead of Referer #44160

DemonTPx opened this issue Feb 8, 2021 · 5 comments

Comments

@DemonTPx
Copy link

@DemonTPx DemonTPx commented Feb 8, 2021

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

$ go version
go version go1.15.4 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/bert/.cache/go-build"
GOENV="/home/bert/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/bert/go/go1.15.4/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/bert/go/go1.15.4"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/bert/go/go1.15.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/bert/go/go1.15.4/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build287000937=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Consider the following code:

	req, err := http.NewRequest("GET", "http://google.com", nil)
	if err != nil {
		panic(err)
	}
	req.Header.Set("Referer", "https://github.com")
	trace := &httptrace.ClientTrace{
		WroteHeaderField: func(key string, value []string) {
			fmt.Printf("Wrote hdr: %+v %+v\n", key, value)
		},
	}
	req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))

	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		panic(err)
	}

	fmt.Printf("%+v\n", resp)

I've made a request to http://google.com which I know will be redirected and added a Referer header. The tracing is added so I can see which headers are being sent per request.

What did you expect to see?

The first request includes the Referer header set to https://github.com.

The second request had the Referer header set to http://google.com

What did you see instead?

I expected the second request to have the same Referer header as the first request (https://github.com instead of http://google.com)

I'm not sure what http clients are supposed to do, but cURL and Google Chrome both use the original Referer header for the second request.

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 8, 2021

@dmitshur dmitshur added this to the Backlog milestone Feb 8, 2021
@tpaschalis
Copy link
Contributor

@tpaschalis tpaschalis commented Feb 11, 2021

I can verify that Firefox 85.0 uses the original Referer header for the second request as well.

The header is overriden in refererForURL and the default HTTP client. First, it removes the header if we're going from https->http, which adheres to the spec, and afterwards it always substitutes the header with the last request URL, regardless of the incoming Referer.

I think the spec is a little vague regarding what should happen in cases of redirects, but it does mention the following

An intermediary SHOULD NOT modify or delete the Referer header field when the field value shares the same scheme and host as the request target.

My 2c is that we could add a condition inside refererForURL to keep the https->http logic, and only override the header if it's currently empty; otherwise keep it as is. This will bring parity with Chrome, Firefox and cURL.

One impact on current functionality, eg. for the following redirect chain GET domainA->domainB->domainC->domainD. Starting out with an empty Referer header, in the current implementation when landing on domainD we'd have Referer: domainC, but with the proposed change it would have Referer: domainA.

Would it be okay for me to open a CL and continue the conversation there? Or should we wait for the cc'ed people to chime in?

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 11, 2021

@tpaschalis It's generally better to have a discussion in the issue before writing significant code (see https://golang.org/doc/contribute.html#before_contributing). But you can send a CL sooner if isn't a lot of work and you feel it will help move the discussion forward or provide useful data.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2021

Change https://golang.org/cl/291636 mentions this issue: net/http: continue using referer header if it's present

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 28, 2021

cc @neild

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
5 participants