code review 151430043: net/http: Fix authentication info leakage in Referer he...

net/http: Fix authentication info leakage in Referer header (potential security risk)

http.Client calls URL.String() to fill in the Referer header, which may
contain authentication info. This patch removes authentication info from
the Referer header without introducing any API changes.

A new test for net/http is also provided.

That's the polished version of CL 9766046. It should handle https Referer right.

Related to CL: https://codereview.appspot.com/9766046/
Fixes issue 8417.

[jfrederich, 2014-10-05 17:54:52]

Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org,
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[gobot, 2014-10-05 17:57:56]

R=golang-dev (assigned by jfrederich@gmail.com)

[bradfitz, 2014-10-06 17:39:58]

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client.go
File src/net/http/client.go (right):

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client.go#ne...
src/net/http/client.go:104: // Returns the referer generated by the given URL
lastReqURL, which is its string
still use a complete sentence for internal funcs. See
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Comment_Sentences

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client.go#ne...
src/net/http/client.go:109: func refererForURL(lastReqURL *url.URL, newReqURL
*url.URL) string {
(a, b T) instead of (a T, b T)

And you can drop the URL suffix too, so just:

  (lastReq, newReq *url.URL)

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client_test.go
File src/net/http/client_test.go (right):

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client_test....
src/net/http/client_test.go:1052: for _, v := range cases {
rename "v" to "tt", which is a convention. I don't know what "tt" means. Think
of it as "test case" or "test test".

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client_test....
src/net/http/client_test.go:1058: for _, s := range sites {
this test has too much logic in it, dupicating the code being tested.

Just make a table with the inputs and expected output:

tests := []struct{
     lastReq, newReq string // from -> to URLs
     want                   string
}{
    // don't send a password:
    {"...", "...", "..."},

    // https to http doesn't send a referer:
    {"...", "...", "..."},

    // Verify foo
    {"...", "...", "..."},

    // Verify x
    {"...", "...", "..."},
}

for _, tt := range tests {
     // Parse both lastReq, newReq. t.Fatal or panic if it's a bad URL.
     // check that ExportRefererForURL matches exactly. No explicit checks for
"@" or "http" or "https"
}

https://codereview.appspot.com/151430043/diff/40001/src/net/http/export_test.go
File src/net/http/export_test.go (right):

https://codereview.appspot.com/151430043/diff/40001/src/net/http/export_test....
src/net/http/export_test.go:96: func RefererForURL(lastReqURL *url.URL,
newReqURL *url.URL) string {
name this "ExportRefererForURL", like the other things in this file.  The
"Export" prefix makes it very obvious in the other test code that this isn't a
real API of net/http but just being exported for testing.

[mikio, 2014-10-06 21:33:53]

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client.go
File src/net/http/client.go (right):

https://codereview.appspot.com/151430043/diff/40001/src/net/http/client.go#ne...
src/net/http/client.go:110: // http://tools.ietf.org/html/rfc2616#section-15.1.3
7231 obsoletes 2616, https://tools.ietf.org/html/rfc7231#section-5.5.2

[jfrederich, 2014-10-07 13:06:48]

Hello golang-codereviews@googlegroups.com, gobot@golang.org,
bradfitz@golang.org, mikioh.mikioh@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-07 14:02:31]

LGTM

I'll fix the test output a bit when I submit.

[bradfitz, 2014-10-07 14:13:46]

*** Submitted as https://code.google.com/p/go/source/detail?r=bdd5f6fd0274 ***

net/http: fix authentication info leakage in Referer header (potential security
risk)

http.Client calls URL.String() to fill in the Referer header, which may
contain authentication info. This patch removes authentication info from
the Referer header without introducing any API changes.

A new test for net/http is also provided.

This is the polished version of Alberto García Hierro's
https://codereview.appspot.com/9766046/

It should handle https Referer right.

Fixes issue 8417

LGTM=bradfitz
R=golang-codereviews, gobot, bradfitz, mikioh.mikioh
CC=golang-codereviews
https://codereview.appspot.com/151430043

Committer: Brad Fitzpatrick <bradfitz@golang.org>