net/http/httputil: always deep copy the Request.Header map in ReverseProxy

We used to do it sometimes as an optimization, but the optimization is
flawed: in all non-contrived cases we need to deep clone the map
anyway. So do it always, which both simplifies the code but also fixes
the X-Forward-For value leaking to the caller's Request, as well as
modifications from the optional Director func.

Fixes #18327

Change-Id: I0c86d10c557254bf99fdd988227dcb15f968770b
Reviewed-on: https://go-review.googlesource.com/46716
Reviewed-by: Ian Lance Taylor <iant@golang.org>

1 files changed, 33 insertions, 0 deletions

diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 57503cc896..37a9992375 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -736,3 +736,36 @@ func TestServeHTTPDeepCopy(t *testing.T) {
 		t.Errorf("got = %+v; want = %+v", got, want)
 	}
 }
+
+// Issue 18327: verify we always do a deep copy of the Request.Header map
+// before any mutations.
+func TestClonesRequestHeaders(t *testing.T) {
+	req, _ := http.NewRequest("GET", "http://foo.tld/", nil)
+	req.RemoteAddr = "1.2.3.4:56789"
+	rp := &ReverseProxy{
+		Director: func(req *http.Request) {
+			req.Header.Set("From-Director", "1")
+		},
+		Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) {
+			if v := req.Header.Get("From-Director"); v != "1" {
+				t.Errorf("From-Directory value = %q; want 1", v)
+			}
+			return nil, io.EOF
+		}),
+	}
+	rp.ServeHTTP(httptest.NewRecorder(), req)
+
+	if req.Header.Get("From-Director") == "1" {
+		t.Error("Director header mutation modified caller's request")
+	}
+	if req.Header.Get("X-Forwarded-For") != "" {
+		t.Error("X-Forward-For header mutation modified caller's request")
+	}
+
+}
+
+type roundTripperFunc func(req *http.Request) (*http.Response, error)
+
+func (fn roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) {
+	return fn(req)
+}