]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: make Client copy headers on redirect
authorBrad Fitzpatrick <bradfitz@golang.org>
Fri, 9 Sep 2016 18:06:56 +0000 (18:06 +0000)
committerBrad Fitzpatrick <bradfitz@golang.org>
Fri, 9 Sep 2016 22:55:40 +0000 (22:55 +0000)
Copy all of the original request's headers on redirect, unless they're
sensitive. Only send sensitive ones to the same origin, or subdomains
thereof.

Fixes #4800

Change-Id: Ie9fa75265c9d5e4c1012c028d31fd1fd74465712
Reviewed-on: https://go-review.googlesource.com/28930
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Francesc Campoy Flores <campoy@golang.org>
Reviewed-by: Ross Light <light@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/net/http/client.go
src/net/http/client_test.go
src/net/http/export_test.go

index facfb41e3807e933e535cb8a08ac2c6ee9501f10..fb00f714ff2be1ef0e1aafe26ab136869233271c 100644 (file)
@@ -447,6 +447,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                deadline = c.deadline()
                reqs     []*Request
                resp     *Response
+               ireqhdr  = req.Header.clone()
        )
        uerr := func(err error) error {
                req.closeBody()
@@ -487,6 +488,17 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                        if ireq.Method == "POST" || ireq.Method == "PUT" {
                                req.Method = "GET"
                        }
+                       // Copy the initial request's Header values
+                       // (at least the safe ones).  Do this before
+                       // setting the Referer, in case the user set
+                       // Referer on their first request. If they
+                       // really want to override, they can do it in
+                       // their CheckRedirect func.
+                       for k, vv := range ireqhdr {
+                               if shouldCopyHeaderOnRedirect(k, ireq.URL, u) {
+                                       req.Header[k] = vv
+                               }
+                       }
                        // Add the Referer header from the most recent
                        // request URL to the new one, if it's not https->http:
                        if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL); ref != "" {
@@ -669,3 +681,52 @@ func (b *cancelTimerBody) Close() error {
        b.stop()
        return err
 }
+
+func shouldCopyHeaderOnRedirect(headerKey string, initial, dest *url.URL) bool {
+       switch CanonicalHeaderKey(headerKey) {
+       case "Authorization", "Www-Authenticate", "Cookie", "Cookie2":
+               // Permit sending auth/cookie headers from "foo.com"
+               // to "sub.foo.com".
+
+               // Note that we don't send all cookies to subdomains
+               // automatically. This function is only used for
+               // Cookies set explicitly on the initial outgoing
+               // client request. Cookies automatically added via the
+               // CookieJar mechanism continue to follow each
+               // cookie's scope as set by Set-Cookie. But for
+               // outgoing requests with the Cookie header set
+               // directly, we don't know their scope, so we assume
+               // it's for *.domain.com.
+
+               // TODO(bradfitz): once issue 16142 is fixed, make
+               // this code use those URL accessors, and consider
+               // "http://foo.com" and "http://foo.com:80" as
+               // equivalent?
+
+               // TODO(bradfitz): better hostname canonicalization,
+               // at least once we figure out IDNA/Punycode (issue
+               // 13835).
+               ihost := strings.ToLower(initial.Host)
+               dhost := strings.ToLower(dest.Host)
+               return isDomainOrSubdomain(dhost, ihost)
+       }
+       // All other headers are copied:
+       return true
+}
+
+// isDomainOrSubdomain reports whether sub is a subdomain (or exact
+// match) of the parent domain.
+//
+// Both domains must already be in canonical form.
+func isDomainOrSubdomain(sub, parent string) bool {
+       if sub == parent {
+               return true
+       }
+       // If sub is "foo.example.com" and parent is "example.com",
+       // that means sub must end in "."+parent.
+       // Do it without allocating.
+       if !strings.HasSuffix(sub, parent) {
+               return false
+       }
+       return sub[len(sub)-len(parent)-1] == '.'
+}
index f5500b6d887a9f6311becd99e6525bb04080ff8a..dc9995b152b70f029e8b92f5d4a634fe1a8ba6bd 100644 (file)
@@ -21,6 +21,7 @@ import (
        . "net/http"
        "net/http/httptest"
        "net/url"
+       "reflect"
        "strconv"
        "strings"
        "sync"
@@ -1229,3 +1230,111 @@ func TestClientRedirectResponseWithoutRequest(t *testing.T) {
        // Check that this doesn't crash:
        c.Get("http://dummy.tld")
 }
+
+// Issue 4800: copy (some) headers when Client follows a redirect
+func TestClientCopyHeadersOnRedirect(t *testing.T) {
+       const (
+               ua   = "some-agent/1.2"
+               xfoo = "foo-val"
+       )
+       var ts2URL string
+       ts1 := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               want := Header{
+                       "User-Agent":      []string{ua},
+                       "X-Foo":           []string{xfoo},
+                       "Referer":         []string{ts2URL},
+                       "Accept-Encoding": []string{"gzip"},
+               }
+               if !reflect.DeepEqual(r.Header, want) {
+                       t.Errorf("Request.Header = %#v; want %#v", r.Header, want)
+               }
+               if t.Failed() {
+                       w.Header().Set("Result", "got errors")
+               } else {
+                       w.Header().Set("Result", "ok")
+               }
+       }))
+       defer ts1.Close()
+       ts2 := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               Redirect(w, r, ts1.URL, StatusFound)
+       }))
+       defer ts2.Close()
+       ts2URL = ts2.URL
+
+       tr := &Transport{}
+       defer tr.CloseIdleConnections()
+       c := &Client{
+               Transport: tr,
+               CheckRedirect: func(r *Request, via []*Request) error {
+                       want := Header{
+                               "User-Agent": []string{ua},
+                               "X-Foo":      []string{xfoo},
+                               "Referer":    []string{ts2URL},
+                       }
+                       if !reflect.DeepEqual(r.Header, want) {
+                               t.Errorf("CheckRedirect Request.Header = %#v; want %#v", r.Header, want)
+                       }
+                       return nil
+               },
+       }
+
+       req, _ := NewRequest("GET", ts2.URL, nil)
+       req.Header.Add("User-Agent", ua)
+       req.Header.Add("X-Foo", xfoo)
+       req.Header.Add("Cookie", "foo=bar")
+       req.Header.Add("Authorization", "secretpassword")
+       res, err := c.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer res.Body.Close()
+       if res.StatusCode != 200 {
+               t.Fatal(res.Status)
+       }
+       if got := res.Header.Get("Result"); got != "ok" {
+               t.Errorf("result = %q; want ok", got)
+       }
+}
+
+// Part of Issue 4800
+func TestShouldCopyHeaderOnRedirect(t *testing.T) {
+       tests := []struct {
+               header     string
+               initialURL string
+               destURL    string
+               want       bool
+       }{
+               {"User-Agent", "http://foo.com/", "http://bar.com/", true},
+               {"X-Foo", "http://foo.com/", "http://bar.com/", true},
+
+               // Sensitive headers:
+               {"cookie", "http://foo.com/", "http://bar.com/", false},
+               {"cookie2", "http://foo.com/", "http://bar.com/", false},
+               {"authorization", "http://foo.com/", "http://bar.com/", false},
+               {"www-authenticate", "http://foo.com/", "http://bar.com/", false},
+
+               // But subdomains should work:
+               {"www-authenticate", "http://foo.com/", "http://foo.com/", true},
+               {"www-authenticate", "http://foo.com/", "http://sub.foo.com/", true},
+               {"www-authenticate", "http://foo.com/", "http://notfoo.com/", false},
+               // TODO(bradfitz): make this test work, once issue 16142 is fixed:
+               // {"www-authenticate", "http://foo.com:80/", "http://foo.com/", true},
+       }
+       for i, tt := range tests {
+               u0, err := url.Parse(tt.initialURL)
+               if err != nil {
+                       t.Errorf("%d. initial URL %q parse error: %v", i, tt.initialURL, err)
+                       continue
+               }
+               u1, err := url.Parse(tt.destURL)
+               if err != nil {
+                       t.Errorf("%d. dest URL %q parse error: %v", i, tt.destURL, err)
+                       continue
+               }
+               got := Export_shouldCopyHeaderOnRedirect(tt.header, u0, u1)
+               if got != tt.want {
+                       t.Errorf("%d. shouldCopyHeaderOnRedirect(%q, %q => %q) = %v; want %v",
+                               i, tt.header, tt.initialURL, tt.destURL, got, tt.want)
+               }
+       }
+}
index 9c5ba0809ad0ab200193b1dafd83e6f8eb695432..7fc3546caaf741525185ab018cee761854d1b69a 100644 (file)
@@ -160,3 +160,5 @@ func ExportHttp2ConfigureTransport(t *Transport) error {
        t.h2transport = t2
        return nil
 }
+
+var Export_shouldCopyHeaderOnRedirect = shouldCopyHeaderOnRedirect