net/http/httputil: don't add X-Forwarded-{Host,Proto} after invoking Director funcs

This reverts CL 407414.

When forwarding an inbound request that contains an existing
X-Forwarded-Host or X-Forwarded-Proto header, a proxy might want
to preserve the header from the inbound request, replace it with
its own header, or not include any header at all.

CL 407414 replaces inbound X-Forwarded-{Host,Proto} headers by default,
and allows a Director func to disable sending these headers at all.
However, the Director hook API isn't sufficiently flexible to permit the
previous behavior of preserving inbound values unchanged.

The new Rewrite API does have this flexibility; users of Rewrite can
easily pick the exact behavior they want.

Revert the change to ReverseProxy when using a Director func.
Users who want a convenient way to set X-Forwarded-* headers to
reasonable values can migrate to Rewrite at their convenience,
and users depending on the current behavior will be unaffected.

For #50465.
Fixes #57132.

Change-Id: Ic42449c1bb525d6c9920bf721efbc519697f4f20
Reviewed-on: https://go-review.googlesource.com/c/go/+/457595
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 190279ca007d6c49228ff8d55571184b1276a888..58064a53322627d393d9d183681b21524c99b480 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -131,17 +131,17 @@ type ReverseProxy struct {
        // Director must not access the provided Request
        // after returning.
        //
-       // By default, the X-Forwarded-For, X-Forwarded-Host, and
-       // X-Forwarded-Proto headers of the ourgoing request are
-       // set as by the ProxyRequest.SetXForwarded function.
+       // By default, the X-Forwarded-For header is set to the
+       // value of the client IP address. If an X-Forwarded-For
+       // header already exists, the client IP is appended to the
+       // existing values. As a special case, if the header
+       // exists in the Request.Header map but has a nil value
+       // (such as when set by the Director func), the X-Forwarded-For
+       // header is not modified.
        //
-       // If an X-Forwarded-For header already exists, the client IP is
-       // appended to the existing values. To prevent IP spoofing, be
-       // sure to delete any pre-existing X-Forwarded-For header
-       // coming from the client or an untrusted proxy.
-       //
-       // If a header exists in the Request.Header map but has a nil value
-       // (such as when set by the Director func), it is not modified.
+       // To prevent IP spoofing, be sure to delete any pre-existing
+       // X-Forwarded-For header coming from the client or
+       // an untrusted proxy.
        //
        // Hop-by-hop headers are removed from the request after
        // Director returns, which can remove headers added by
@@ -446,16 +446,6 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
                                outreq.Header.Set("X-Forwarded-For", clientIP)
                        }
                }
-               if prior, ok := outreq.Header["X-Forwarded-Host"]; !(ok && prior == nil) {
-                       outreq.Header.Set("X-Forwarded-Host", req.Host)
-               }
-               if prior, ok := outreq.Header["X-Forwarded-Proto"]; !(ok && prior == nil) {
-                       if req.TLS == nil {
-                               outreq.Header.Set("X-Forwarded-Proto", "http")
-                       } else {
-                               outreq.Header.Set("X-Forwarded-Proto", "https")
-                       }
-               }
        }
 
        if _, ok := outreq.Header["User-Agent"]; !ok {

diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 5b882d3a45638c54aee1bbba4fdfad13fe8b9502..d5b0fb4244c6e0dc9894f678aa303f516a104b97 100644 (file)
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -52,12 +52,6 @@ func TestReverseProxy(t *testing.T) {
                if r.Header.Get("X-Forwarded-For") == "" {
                        t.Errorf("didn't get X-Forwarded-For header")
                }
-               if r.Header.Get("X-Forwarded-Host") == "" {
-                       t.Errorf("didn't get X-Forwarded-Host header")
-               }
-               if r.Header.Get("X-Forwarded-Proto") == "" {
-                       t.Errorf("didn't get X-Forwarded-Proto header")
-               }
                if c := r.Header.Get("Connection"); c != "" {
                        t.Errorf("handler got Connection header value %q", c)
                }
@@ -307,7 +301,6 @@ func TestXForwardedFor(t *testing.T) {
        const prevForwardedFor = "client ip"
        const backendResponse = "I am the backend"
        const backendStatus = 404
-       const host = "some-name"
        backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                if r.Header.Get("X-Forwarded-For") == "" {
                        t.Errorf("didn't get X-Forwarded-For header")
@@ -315,12 +308,6 @@ func TestXForwardedFor(t *testing.T) {
                if !strings.Contains(r.Header.Get("X-Forwarded-For"), prevForwardedFor) {
                        t.Errorf("X-Forwarded-For didn't contain prior data")
                }
-               if got, want := r.Header.Get("X-Forwarded-Host"), host; got != want {
-                       t.Errorf("X-Forwarded-Host = %q, want %q", got, want)
-               }
-               if got, want := r.Header.Get("X-Forwarded-Proto"), "http"; got != want {
-                       t.Errorf("X-Forwarded-Proto = %q, want %q", got, want)
-               }
                w.WriteHeader(backendStatus)
                w.Write([]byte(backendResponse))
        }))
@@ -334,7 +321,6 @@ func TestXForwardedFor(t *testing.T) {
        defer frontend.Close()
 
        getReq, _ := http.NewRequest("GET", frontend.URL, nil)
-       getReq.Host = host
        getReq.Header.Set("Connection", "close")
        getReq.Header.Set("X-Forwarded-For", prevForwardedFor)
        getReq.Close = true
@@ -351,36 +337,11 @@ func TestXForwardedFor(t *testing.T) {
        }
 }
 
-func TestXForwardedProtoTLS(t *testing.T) {
-       backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-               if got, want := r.Header.Get("X-Forwarded-Proto"), "https"; got != want {
-                       t.Errorf("X-Forwarded-Proto = %q, want %q", got, want)
-               }
-       }))
-       defer backend.Close()
-       backendURL, err := url.Parse(backend.URL)
-       if err != nil {
-               t.Fatal(err)
-       }
-       proxyHandler := NewSingleHostReverseProxy(backendURL)
-       frontend := httptest.NewTLSServer(proxyHandler)
-       defer frontend.Close()
-
-       getReq, _ := http.NewRequest("GET", frontend.URL, nil)
-       getReq.Host = "some-host"
-       _, err = frontend.Client().Do(getReq)
-       if err != nil {
-               t.Fatalf("Get: %v", err)
-       }
-}
-
 // Issue 38079: don't append to X-Forwarded-For if it's present but nil
 func TestXForwardedFor_Omit(t *testing.T) {
        backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-               for _, h := range []string{"X-Forwarded-For", "X-Forwarded-Host", "X-Forwarded-Proto"} {
-                       if v := r.Header.Get(h); v != "" {
-                               t.Errorf("got %v header: %q", h, v)
-                       }
+               if v := r.Header.Get("X-Forwarded-For"); v != "" {
+                       t.Errorf("got X-Forwarded-For header: %q", v)
                }
                w.Write([]byte("hi"))
        }))
@@ -396,8 +357,6 @@ func TestXForwardedFor_Omit(t *testing.T) {
        oldDirector := proxyHandler.Director
        proxyHandler.Director = func(r *http.Request) {
                r.Header["X-Forwarded-For"] = nil
-               r.Header["X-Forwarded-Host"] = nil
-               r.Header["X-Forwarded-Proto"] = nil
                oldDirector(r)
        }
 
@@ -1106,8 +1065,6 @@ func TestClonesRequestHeaders(t *testing.T) {
        for _, h := range []string{
                "From-Director",
                "X-Forwarded-For",
-               "X-Forwarded-Host",
-               "X-Forwarded-Proto",
        } {
                if req.Header.Get(h) != "" {
                        t.Errorf("%v header mutation modified caller's request", h)