[go] net/http: continue using referer header if it's present
Paschalis Tsilias (Gerrit)
Paschalis Tsilias has uploaded this change for review.
net/http: continue using referer header if it's present
Currently, net/http replaces the Referer header with the URL of the
previous request, regardless of its status. This CL changes this
behavior, respecting the Referer header for secure connections, if it is
set.
Fixes #44160
Change-Id: I2d7fe37dd681549136329e832188294691584870
---
M src/net/http/client.go
M src/net/http/client_test.go
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/net/http/client.go b/src/net/http/client.go
index 88e2028..cae2573 100644
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -143,7 +143,7 @@
// refererForURL returns a referer without any authentication info or
// an empty string if lastReq scheme is https and newReq scheme is http.
-func refererForURL(lastReq, newReq *url.URL) string {
+func refererForURL(lastReq, newReq *url.URL, lastRef string) string {
// https://tools.ietf.org/html/rfc7231#section-5.5.2
// "Clients SHOULD NOT include a Referer header field in a
// (non-secure) HTTP request if the referring page was
@@ -151,6 +151,10 @@
if lastReq.Scheme == "https" && newReq.Scheme == "http" {
return ""
}
+ if lastRef != "" {
+ return lastRef
+ }
+
referer := lastReq.String()
if lastReq.User != nil {
// This is not very efficient, but is the best we can
@@ -677,7 +681,7 @@
// Add the Referer header from the most recent
// request URL to the new one, if it's not https->http:
- if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL); ref != "" {
+ if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL, req.Header.Get("Referer")); ref != "" {
req.Header.Set("Referer", ref)
}
err = c.checkRedirect(req, reqs)
diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index d90b484..0523f42 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -1406,24 +1406,32 @@
func TestReferer(t *testing.T) {
tests := []struct {
- lastReq, newReq string // from -> to URLs
- want string
+ lastReq, newReq, lastRef string // from -> to URLs, last Referer value
+ want string
}{
// don't send user:
- {"http://gop...@test.com", "http://link.com", "http://test.com"},
- {"https://gop...@test.com", "https://link.com", "https://test.com"},
+ {"http://gop...@test.com", "http://link.com", "", "http://test.com"},
+ {"https://gop...@test.com", "https://link.com", "", "https://test.com"},
// don't send a user and password:
- {"http://gopher:g...@test.com", "http://link.com", "http://test.com"},
- {"https://gopher:g...@test.com", "https://link.com", "https://test.com"},
+ {"http://gopher:g...@test.com", "http://link.com", "", "http://test.com"},
+ {"https://gopher:g...@test.com", "https://link.com", "", "https://test.com"},
// nothing to do:
- {"http://test.com", "http://link.com", "http://test.com"},
- {"https://test.com", "https://link.com", "https://test.com"},
+ {"http://test.com", "http://link.com", "", "http://test.com"},
+ {"https://test.com", "https://link.com", "", "https://test.com"},
// https to http doesn't send a referer:
- {"https://test.com", "http://link.com", ""},
- {"https://gopher:g...@test.com", "http://link.com", ""},
+ {"https://test.com", "http://link.com", "", ""},
+ {"https://gopher:g...@test.com", "http://link.com", "", ""},
+
+ // https to http should remove an existing referer:
+ {"https://test.com", "http://link.com", "https://foo.com", ""},
+ {"https://gopher:g...@test.com", "http://link.com", "https://foo.com", ""},
+
+ // don't override an existing referer:
+ {"https://test.com", "https://link.com", "https://foo.com", "https://foo.com"},
+ {"https://gopher:g...@test.com", "https://link.com", "https://foo.com", "https://foo.com"},
}
for _, tt := range tests {
l, err := url.Parse(tt.lastReq)
@@ -1434,7 +1442,7 @@
if err != nil {
t.Fatal(err)
}
- r := ExportRefererForURL(l, n)
+ r := ExportRefererForURL(l, n, tt.lastRef)
if r != tt.want {
t.Errorf("refererForURL(%q, %q) = %q; want %q", tt.lastReq, tt.newReq, r, tt.want)
}
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
1 comment:
Patchset:
I'm not very experienced with the HTTP/1.1 spec, so feel free to correct me.
After looking into the RFC, and how other popular tools (cURL, Firefox, Chrome) are handling Referer headers, I tried to replicate the behavior of _not_ overriding the header with the last request's URL, but respecting its value if it is set.
I understand the spec is a little vague, but it does mention the following at https://tools.ietf.org/html/rfc7231#section-5.5.2
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.
Another solution would be to implement this weaker check for lastReq's and newReq's scheme and host, but the aforementioned tools maintain the header even when redirecting to different domains.
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
1 comment:
Patchset:
Sorry for the ping, but could I get a review on this when there's some time? Thanks a lot in advance!
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Paschalis Tsilias.
2 comments:
File src/net/http/client.go:
Patch Set #1, Line 145:
// an empty string if lastReq scheme is https and newReq scheme is http.document lastRef?
But is this really "last"? It's more "explicit Referer" from the original HTTP request the user sent out, right? Is it ever the case that lastRef is non-empty and not the original explicit referer that was set from the user's http.Request?
File src/net/http/client_test.go:
Patch Set #1, Line 1413:
{"http://gop...@test.com", "http://link.com", "", "http://test.com"},The unkeyed structs with all the same types were borderline dubious before but with 4 unkeyed strings in a row, this gets a little hard to read & easy to transpose things.
Type to add field names to these and omit lastRef when it's empty.
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Paschalis Tsilias.
Paschalis Tsilias uploaded patch set #2 to this change.
net/http: continue using referer header if it's present
Currently, net/http replaces the Referer header with the URL of the
previous request, regardless of its status. This CL changes this
behavior, respecting the Referer header for secure connections, if it is
set.
Fixes #44160
Change-Id: I2d7fe37dd681549136329e832188294691584870
---
M src/net/http/client.go
M src/net/http/client_test.go
2 files changed, 26 insertions(+), 13 deletions(-)
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
2 comments:
File src/net/http/client.go:
Patch Set #1, Line 145:
// an empty string if lastReq scheme is https and newReq scheme is http.
document lastRef? […]
If you mean a part of the redirect chain 'hijacking/modifying' the Referer header, I've never seen that happen. So 'explicit' is a better name indeed.
I've changed the variable name and updated the docstring to provide some clarity, please let me know what you think!
File src/net/http/client_test.go:
The unkeyed structs with all the same types were borderline dubious before but with 4 unkeyed string […]
You're right; done. I didn't omit the `want` field so it's more obvious what the test result should be.
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
1 comment:
Patchset:
Gentle ping for a review
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Nick Craig-Wood (Gerrit)
Attention is currently required from: Paschalis Tsilias, Brad Fitzpatrick.
Patch set 2:Code-Review +1
1 comment:
Patchset:
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?
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Paschalis Tsilias, Brad Fitzpatrick.
Patch set 2:Run-TryBot +1
Damien Neil (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Emmanuel Odeke, Paschalis Tsilias.
Patch set 3:Run-TryBot +1
Damien Neil (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Emmanuel Odeke, Paschalis Tsilias.
Patch set 3:Auto-Submit +1Code-Review +2
1 comment:
Patchset:
Thanks.
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Emmanuel Odeke, Paschalis Tsilias.
Patch set 3:Code-Review +1
Gopher Robot (Gerrit)
Gopher Robot submitted this change.
net/http: continue using referer header if it's present
Currently, net/http replaces the Referer header with the URL of the
previous request, regardless of its status. This CL changes this
behavior, respecting the Referer header for secure connections, if it is
set.
Fixes #44160
Change-Id: I2d7fe37dd681549136329e832188294691584870
Reviewed-on: https://go-review.googlesource.com/c/go/+/291636
Reviewed-by: Damien Neil <dn...@google.com>
Reviewed-by: Nick Craig-Wood <nic...@gmail.com>
Run-TryBot: Damien Neil <dn...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Auto-Submit: Damien Neil <dn...@google.com>
Reviewed-by: Ian Lance Taylor <ia...@google.com>
---
M src/net/http/client.go
M src/net/http/client_test.go
2 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/src/net/http/client.go b/src/net/http/client.go
index 2f7d4f6..1e300ac 100644
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -144,7 +144,8 @@
// refererForURL returns a referer without any authentication info or
// an empty string if lastReq scheme is https and newReq scheme is http.
-func refererForURL(lastReq, newReq *url.URL) string {
+// If the referer was explicitly set, then it will continue to be used.
+func refererForURL(lastReq, newReq *url.URL, explicitRef string) string {
// https://tools.ietf.org/html/rfc7231#section-5.5.2
// "Clients SHOULD NOT include a Referer header field in a
// (non-secure) HTTP request if the referring page was
@@ -152,6 +153,10 @@
if lastReq.Scheme == "https" && newReq.Scheme == "http" {
return ""
}
+ if explicitRef != "" {
+ return explicitRef
+ }
+
referer := lastReq.String()
if lastReq.User != nil {
// This is not very efficient, but is the best we can
@@ -676,7 +681,7 @@
// Add the Referer header from the most recent
// request URL to the new one, if it's not https->http:
- if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL); ref != "" {
+ if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL, req.Header.Get("Referer")); ref != "" {
req.Header.Set("Referer", ref)
}
err = c.checkRedirect(req, reqs)
diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index bb1ed6f..b8c914b 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -1411,24 +1411,32 @@
func TestReferer(t *testing.T) {
tests := []struct {
- lastReq, newReq string // from -> to URLs
- want string
+ lastReq, newReq, explicitRef string // from -> to URLs, explicitly set Referer value
+ want string
}{
// don't send user:
- {"http://gop...@test.com", "http://link.com", "http://test.com"},
- {"https://gop...@test.com", "https://link.com", "https://test.com"},
+ {lastReq: "http://gop...@test.com", newReq: "http://link.com", want: "http://test.com"},
+ {lastReq: "https://gop...@test.com", newReq: "https://link.com", want: "https://test.com"},
// don't send a user and password:
- {"http://gopher:g...@test.com", "http://link.com", "http://test.com"},
- {"https://gopher:g...@test.com", "https://link.com", "https://test.com"},
+ {lastReq: "http://gopher:g...@test.com", newReq: "http://link.com", want: "http://test.com"},
+ {lastReq: "https://gopher:g...@test.com", newReq: "https://link.com", want: "https://test.com"},
// nothing to do:
- {"http://test.com", "http://link.com", "http://test.com"},
- {"https://test.com", "https://link.com", "https://test.com"},
+ {lastReq: "http://test.com", newReq: "http://link.com", want: "http://test.com"},
+ {lastReq: "https://test.com", newReq: "https://link.com", want: "https://test.com"},
// https to http doesn't send a referer:
- {"https://test.com", "http://link.com", ""},
- {"https://gopher:g...@test.com", "http://link.com", ""},
+ {lastReq: "https://test.com", newReq: "http://link.com", want: ""},
+ {lastReq: "https://gopher:g...@test.com", newReq: "http://link.com", want: ""},
+
+ // https to http should remove an existing referer:
+ {lastReq: "https://test.com", newReq: "http://link.com", explicitRef: "https://foo.com", want: ""},
+ {lastReq: "https://gopher:g...@test.com", newReq: "http://link.com", explicitRef: "https://foo.com", want: ""},
+
+ // don't override an existing referer:
+ {lastReq: "https://test.com", newReq: "https://link.com", explicitRef: "https://foo.com", want: "https://foo.com"},
+ {lastReq: "https://gopher:g...@test.com", newReq: "https://link.com", explicitRef: "https://foo.com", want: "https://foo.com"},
}
for _, tt := range tests {
l, err := url.Parse(tt.lastReq)
@@ -1439,7 +1447,7 @@
if err != nil {
t.Fatal(err)
}
- r := ExportRefererForURL(l, n)
+ r := ExportRefererForURL(l, n, tt.explicitRef)
if r != tt.want {
t.Errorf("refererForURL(%q, %q) = %q; want %q", tt.lastReq, tt.newReq, r, tt.want)
}
To view, visit change 291636. To unsubscribe, or for help writing mail filters, visit settings.