net/http: tweak the new Client 307/308 redirect behavior a bit

This CL tweaks the new (unreleased) 307/308 support added in
https://golang.org/cl/29852 for #10767.

Change 1: if a 307/308 response doesn't have a Location header in its
response (as observed in the wild in #17773), just do what we used to
do in Go 1.7 and earlier, and don't try to follow that redirect.

Change 2: don't follow a 307/308 if we sent a body on the first
request and the caller's Request.GetBody func is nil so we can't
"rewind" the body to send it again.

Updates #17773 (will be fixed more elsewhere)

Change-Id: I183570f7346917828a4b6f7f1773094122a30406
Reviewed-on: https://go-review.googlesource.com/32595
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/client.go b/src/net/http/client.go
index 181494643086cb44636e75936eb32aafeb22e5fd..6780e3ee7a7e1392887c11a7d778f9a1dafd2d15 100644 (file)
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -537,6 +537,34 @@ func (c *Client) Do(req *Request) (*Response, error) {
 
                var shouldRedirect bool
                redirectMethod, shouldRedirect = redirectBehavior(req.Method, resp.StatusCode)
+
+               // Treat 307 and 308 specially, since they're new in
+               // Go 1.8, and they also require re-sending the
+               // request body.
+               //
+               // TODO: move this logic into func redirectBehavior?
+               // It would need to take a bunch more things then.
+               switch resp.StatusCode {
+               case 307, 308:
+                       loc := resp.Header.Get("Location")
+                       if loc == "" {
+                               // 308s have been observed in the wild being served
+                               // without Location headers. Since Go 1.7 and earlier
+                               // didn't follow these codes, just stop here instead
+                               // of returning an error.
+                               shouldRedirect = false
+                               break
+                       }
+                       ireq := reqs[0]
+                       if ireq.GetBody == nil && ireq.outgoingLength() != 0 {
+                               // We had a request body, and 307/308 require
+                               // re-sending it, but GetBody is not defined. So just
+                               // return this response to the user instead of an
+                               // error, like we did in Go 1.7 and earlier.
+                               shouldRedirect = false
+                       }
+               }
+
                if !shouldRedirect {
                        return resp, nil
                }

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index f60c9a5a7fd417dc81325502dddff2fe79b3c79b..59603def6774612d6c654e194b061bed69ddf5e0 100644 (file)
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -499,6 +499,57 @@ func TestClientRedirectUseResponse(t *testing.T) {
        }
 }
 
+// Issue 17773: don't follow a 308 (or 307) if the response doesn't
+// have a Location header.
+func TestClientRedirect308NoLocation(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               w.Header().Set("Foo", "Bar")
+               w.WriteHeader(308)
+       }))
+       defer ts.Close()
+       res, err := Get(ts.URL)
+       if err != nil {
+               t.Fatal(err)
+       }
+       res.Body.Close()
+       if res.StatusCode != 308 {
+               t.Errorf("status = %d; want %d", res.StatusCode, 308)
+       }
+       if got := res.Header.Get("Foo"); got != "Bar" {
+               t.Errorf("Foo header = %q; want Bar", got)
+       }
+}
+
+// Don't follow a 307/308 if we can't resent the request body.
+func TestClientRedirect308NoGetBody(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+       const fakeURL = "https://localhost:1234/" // won't be hit
+       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               w.Header().Set("Location", fakeURL)
+               w.WriteHeader(308)
+       }))
+       defer ts.Close()
+       req, err := NewRequest("POST", ts.URL, strings.NewReader("some body"))
+       if err != nil {
+               t.Fatal(err)
+       }
+       req.GetBody = nil // so it can't rewind.
+       res, err := DefaultClient.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+       res.Body.Close()
+       if res.StatusCode != 308 {
+               t.Errorf("status = %d; want %d", res.StatusCode, 308)
+       }
+       if got := res.Header.Get("Location"); got != fakeURL {
+               t.Errorf("Location header = %q; want %q", got, fakeURL)
+       }
+}
+
 var expectedCookies = []*Cookie{
        {Name: "ChocolateChip", Value: "tasty"},
        {Name: "First", Value: "Hit"},