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>
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index fd78d45..0d514f5 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -114,6 +114,16 @@
}
}
+func cloneHeader(h http.Header) http.Header {
+ h2 := make(http.Header, len(h))
+ for k, vv := range h {
+ vv2 := make([]string, len(vv))
+ copy(vv2, vv)
+ h2[k] = vv2
+ }
+ return h2
+}
+
// Hop-by-hop headers. These are removed when sent to the backend.
// http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html
var hopHeaders = []string{
@@ -154,23 +164,16 @@
outreq.Body = nil // Issue 16036: nil Body for http.Transport retries
}
+ outreq.Header = cloneHeader(req.Header)
+
p.Director(outreq)
outreq.Close = false
- // We are modifying the same underlying map from req (shallow
- // copied above) so we only copy it if necessary.
- copiedHeaders := false
-
// Remove hop-by-hop headers listed in the "Connection" header.
// See RFC 2616, section 14.10.
if c := outreq.Header.Get("Connection"); c != "" {
for _, f := range strings.Split(c, ",") {
if f = strings.TrimSpace(f); f != "" {
- if !copiedHeaders {
- outreq.Header = make(http.Header)
- copyHeader(outreq.Header, req.Header)
- copiedHeaders = true
- }
outreq.Header.Del(f)
}
}
@@ -181,11 +184,6 @@
// connection, regardless of what the client sent to us.
for _, h := range hopHeaders {
if outreq.Header.Get(h) != "" {
- if !copiedHeaders {
- outreq.Header = make(http.Header)
- copyHeader(outreq.Header, req.Header)
- copiedHeaders = true
- }
outreq.Header.Del(h)
}
}
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 57503cc..37a9992 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -736,3 +736,36 @@
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)
+}
