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

Jump to bottom

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

Closed
DemonTPx opened this issue Feb 8, 2021 · 7 comments
Closed

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

DemonTPx opened this issue Feb 8, 2021 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@DemonTPx
Copy link

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 commented Feb 8, 2021

CC @dneil, @bradfitz, @empijei.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 8, 2021
@dmitshur dmitshur added this to the Backlog milestone Feb 8, 2021
@tpaschalis
Copy link
Contributor

tpaschalis commented Feb 11, 2021 •
edited

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 commented Feb 11, 2021 •
edited

@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

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

@networkimprov
Copy link

cc @neild

@ncw
Copy link
Contributor

ncw commented Apr 26, 2022

This looks like a good fix.

Sending the original referer is what all the browsers do even if the RFC is a bit vague so can we merge this?

See: https://stackoverflow.com/a/5441932/164234

@ncw ncw mentioned this issue Apr 26, 2022
Closed
@simon-ding simon-ding mentioned this issue Mar 13, 2023
Closed
4 tasks
@simon-ding
Copy link

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

when can we merge this?

@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Mar 24, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 24, 2023
@golang golang locked and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
7 participants
@networkimprov @ncw @dmitshur @DemonTPx @gopherbot @simon-ding @tpaschalis