[release-branch.go1.12] net/http: avoid writing to Transport.ProxyConnectHeader

Previously, we accidentally wrote the Proxy-Authorization header for
the initial CONNECT request to the shared ProxyConnectHeader map when
it was non-nil.

Updates #36431
Fixes #36433

Change-Id: I5cb414f391dddf8c23d85427eb6973f14c949025
Reviewed-on: https://go-review.googlesource.com/c/go/+/213638
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 249c85d3aab2ad2d0bcbf36efe606fdd66f25c72)
Reviewed-on: https://go-review.googlesource.com/c/go/+/213677

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index e946760963c5b9877c4193de9ad6e7c035b6e1ca..df41cbd7743d8d884befd3eef71d6545c782846e 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1309,15 +1309,16 @@ func (t *Transport) dialConn(ctx context.Context, cm connectMethod) (*persistCon
                if hdr == nil {
                        hdr = make(Header)
                }
+               if pa := cm.proxyAuth(); pa != "" {
+                       hdr = hdr.clone()
+                       hdr.Set("Proxy-Authorization", pa)
+               }
                connectReq := &Request{
                        Method: "CONNECT",
                        URL:    &url.URL{Opaque: cm.targetAddr},
                        Host:   cm.targetAddr,
                        Header: hdr,
                }
-               if pa := cm.proxyAuth(); pa != "" {
-                       connectReq.Header.Set("Proxy-Authorization", pa)
-               }
                connectReq.Write(conn)
 
                // Read response.

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 5e5438a708b48aef3e3a720d4c93a02b64ff079f..a2f95911d938585acb3b3a034b8f51e5605d8774 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -1371,6 +1371,47 @@ func TestTransportDialPreservesNetOpProxyError(t *testing.T) {
        }
 }
 
+// Issue 36431: calls to RoundTrip should not mutate t.ProxyConnectHeader.
+//
+// (A bug caused dialConn to instead write the per-request Proxy-Authorization
+// header through to the shared Header instance, introducing a data race.)
+func TestTransportProxyDialDoesNotMutateProxyConnectHeader(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+
+       proxy := httptest.NewTLSServer(NotFoundHandler())
+       defer proxy.Close()
+       c := proxy.Client()
+
+       tr := c.Transport.(*Transport)
+       tr.Proxy = func(*Request) (*url.URL, error) {
+               u, _ := url.Parse(proxy.URL)
+               u.User = url.UserPassword("aladdin", "opensesame")
+               return u, nil
+       }
+       h := tr.ProxyConnectHeader
+       if h == nil {
+               h = make(Header)
+       }
+       tr.ProxyConnectHeader = make(Header, len(h))
+       for k, vals := range h {
+               tr.ProxyConnectHeader[k] = append([]string(nil), vals...)
+       }
+
+       req, err := NewRequest("GET", "https://golang.fake.tld/", nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+       _, err = c.Do(req)
+       if err == nil {
+               t.Errorf("unexpected Get success")
+       }
+
+       if !reflect.DeepEqual(tr.ProxyConnectHeader, h) {
+               t.Errorf("tr.ProxyConnectHeader = %v; want %v", tr.ProxyConnectHeader, h)
+       }
+}
+
 // TestTransportGzipRecursive sends a gzip quine and checks that the
 // client gets the same value back. This is more cute than anything,
 // but checks that we don't recurse forever, and checks that