]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: handle 3xx redirects properly
authorEmmanuel Odeke <emm.odeke@gmail.com>
Tue, 27 Sep 2016 03:38:57 +0000 (20:38 -0700)
committerBrad Fitzpatrick <bradfitz@golang.org>
Sun, 30 Oct 2016 02:17:28 +0000 (02:17 +0000)
Provides redirection support for 307, 308 server statuses.
Provides redirection support for DELETE method.

Updates old tests that assumed all redirects were treated
the way 301, 302 and 303 are processed.

Fixes #9348
Fixes #10767
Fixes #13994

Change-Id: Iffa8dbe0ff28a1afa8da59869290ec805b1dd2c4
Reviewed-on: https://go-review.googlesource.com/29852
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
src/net/http/client.go
src/net/http/client_test.go

index 3125fdddcf6c48df9f19fb71c1fc5d1a876cbcc6..181494643086cb44636e75936eb32aafeb22e5fd 100644 (file)
@@ -156,40 +156,6 @@ func (c *Client) send(req *Request, deadline time.Time) (*Response, error) {
        return resp, nil
 }
 
-// Do sends an HTTP request and returns an HTTP response, following
-// policy (such as redirects, cookies, auth) as configured on the
-// client.
-//
-// An error is returned if caused by client policy (such as
-// CheckRedirect), or failure to speak HTTP (such as a network
-// connectivity problem). A non-2xx status code doesn't cause an
-// error.
-//
-// If the returned error is nil, the Response will contain a non-nil
-// Body which the user is expected to close. If the Body is not
-// closed, the Client's underlying RoundTripper (typically Transport)
-// may not be able to re-use a persistent TCP connection to the server
-// for a subsequent "keep-alive" request.
-//
-// The request Body, if non-nil, will be closed by the underlying
-// Transport, even on errors.
-//
-// On error, any Response can be ignored. A non-nil Response with a
-// non-nil error only occurs when CheckRedirect fails, and even then
-// the returned Response.Body is already closed.
-//
-// Generally Get, Post, or PostForm will be used instead of Do.
-func (c *Client) Do(req *Request) (*Response, error) {
-       method := valueOrDefault(req.Method, "GET")
-       if method == "GET" || method == "HEAD" {
-               return c.doFollowingRedirects(req, shouldRedirectGet)
-       }
-       if method == "POST" || method == "PUT" {
-               return c.doFollowingRedirects(req, shouldRedirectPost)
-       }
-       return c.send(req, c.deadline())
-}
-
 func (c *Client) deadline() time.Time {
        if c.Timeout > 0 {
                return time.Now().Add(c.Timeout)
@@ -351,26 +317,6 @@ func basicAuth(username, password string) string {
        return base64.StdEncoding.EncodeToString([]byte(auth))
 }
 
-// True if the specified HTTP status code is one for which the Get utility should
-// automatically redirect.
-func shouldRedirectGet(statusCode int) bool {
-       switch statusCode {
-       case StatusMovedPermanently, StatusFound, StatusSeeOther, StatusTemporaryRedirect:
-               return true
-       }
-       return false
-}
-
-// True if the specified HTTP status code is one for which the Post utility should
-// automatically redirect.
-func shouldRedirectPost(statusCode int) bool {
-       switch statusCode {
-       case StatusFound, StatusSeeOther:
-               return true
-       }
-       return false
-}
-
 // Get issues a GET to the specified URL. If the response is one of
 // the following redirect codes, Get follows the redirect, up to a
 // maximum of 10 redirects:
@@ -379,6 +325,7 @@ func shouldRedirectPost(statusCode int) bool {
 //    302 (Found)
 //    303 (See Other)
 //    307 (Temporary Redirect)
+//    308 (Permanent Redirect)
 //
 // An error is returned if there were too many redirects or if there
 // was an HTTP protocol error. A non-2xx response doesn't cause an
@@ -403,6 +350,7 @@ func Get(url string) (resp *Response, err error) {
 //    302 (Found)
 //    303 (See Other)
 //    307 (Temporary Redirect)
+//    308 (Permanent Redirect)
 //
 // An error is returned if the Client's CheckRedirect function fails
 // or if there was an HTTP protocol error. A non-2xx response doesn't
@@ -417,7 +365,7 @@ func (c *Client) Get(url string) (resp *Response, err error) {
        if err != nil {
                return nil, err
        }
-       return c.doFollowingRedirects(req, shouldRedirectGet)
+       return c.Do(req)
 }
 
 func alwaysFalse() bool { return false }
@@ -438,17 +386,56 @@ func (c *Client) checkRedirect(req *Request, via []*Request) error {
        return fn(req, via)
 }
 
-func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) bool) (*Response, error) {
+// redirectBehavior describes what should happen when the
+// client encounters a 3xx status code from the server
+func redirectBehavior(reqMethod string, serverStatus int) (redirectMethod string, canRedirect bool) {
+       switch serverStatus {
+       case 301, 302, 303:
+               redirectMethod = "GET"
+               canRedirect = true
+       case 307, 308:
+               redirectMethod = reqMethod
+               canRedirect = true
+       }
+
+       return redirectMethod, canRedirect
+}
+
+// Do sends an HTTP request and returns an HTTP response, following
+// policy (such as redirects, cookies, auth) as configured on the
+// client.
+//
+// An error is returned if caused by client policy (such as
+// CheckRedirect), or failure to speak HTTP (such as a network
+// connectivity problem). A non-2xx status code doesn't cause an
+// error.
+//
+// If the returned error is nil, the Response will contain a non-nil
+// Body which the user is expected to close. If the Body is not
+// closed, the Client's underlying RoundTripper (typically Transport)
+// may not be able to re-use a persistent TCP connection to the server
+// for a subsequent "keep-alive" request.
+//
+// The request Body, if non-nil, will be closed by the underlying
+// Transport, even on errors.
+//
+// On error, any Response can be ignored. A non-nil Response with a
+// non-nil error only occurs when CheckRedirect fails, and even then
+// the returned Response.Body is already closed.
+//
+// Generally Get, Post, or PostForm will be used instead of Do.
+func (c *Client) Do(req *Request) (*Response, error) {
        if req.URL == nil {
                req.closeBody()
                return nil, errors.New("http: nil Request.URL")
        }
 
        var (
-               deadline    = c.deadline()
-               reqs        []*Request
-               resp        *Response
-               copyHeaders = c.makeHeadersCopier(req)
+               deadline       = c.deadline()
+               reqs           []*Request
+               resp           *Response
+               copyHeaders    = c.makeHeadersCopier(req)
+               redirectMethod string
        )
        uerr := func(err error) error {
                req.closeBody()
@@ -479,7 +466,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                        }
                        ireq := reqs[0]
                        req = &Request{
-                               Method:   ireq.Method,
+                               Method:   redirectMethod,
                                Response: resp,
                                URL:      u,
                                Header:   make(Header),
@@ -491,10 +478,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                                if err != nil {
                                        return nil, uerr(err)
                                }
-                       }
-                       if ireq.Method == "POST" || ireq.Method == "PUT" {
-                               req.Method = "GET"
-                               req.Body = nil // TODO: fix this when 307/308 support happens
+                               req.ContentLength = ireq.ContentLength
                        }
 
                        // Copy original headers before setting the Referer,
@@ -540,7 +524,6 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                }
 
                reqs = append(reqs, req)
-
                var err error
                if resp, err = c.send(req, deadline); err != nil {
                        if !deadline.IsZero() && !time.Now().Before(deadline) {
@@ -552,9 +535,13 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
                        return nil, uerr(err)
                }
 
-               if !shouldRedirect(resp.StatusCode) {
+               var shouldRedirect bool
+               redirectMethod, shouldRedirect = redirectBehavior(req.Method, resp.StatusCode)
+               if !shouldRedirect {
                        return resp, nil
                }
+
+               req.closeBody()
        }
 }
 
@@ -657,7 +644,7 @@ func (c *Client) Post(url string, contentType string, body io.Reader) (resp *Res
                return nil, err
        }
        req.Header.Set("Content-Type", contentType)
-       return c.doFollowingRedirects(req, shouldRedirectPost)
+       return c.Do(req)
 }
 
 // PostForm issues a POST to the specified URL, with data's keys and
@@ -694,6 +681,7 @@ func (c *Client) PostForm(url string, data url.Values) (resp *Response, err erro
 //    302 (Found)
 //    303 (See Other)
 //    307 (Temporary Redirect)
+//    308 (Permanent Redirect)
 //
 // Head is a wrapper around DefaultClient.Head
 func Head(url string) (resp *Response, err error) {
@@ -708,12 +696,13 @@ func Head(url string) (resp *Response, err error) {
 //    302 (Found)
 //    303 (See Other)
 //    307 (Temporary Redirect)
+//    308 (Permanent Redirect)
 func (c *Client) Head(url string) (resp *Response, err error) {
        req, err := NewRequest("HEAD", url, nil)
        if err != nil {
                return nil, err
        }
-       return c.doFollowingRedirects(req, shouldRedirectGet)
+       return c.Do(req)
 }
 
 // cancelTimerBody is an io.ReadCloser that wraps rc with two features:
index 369a50ded69346749fb6063d77afe99db03a9959..f60c9a5a7fd417dc81325502dddff2fe79b3c79b 100644 (file)
@@ -208,7 +208,7 @@ func TestClientRedirects(t *testing.T) {
                        }
                }
                if n < 15 {
-                       Redirect(w, r, fmt.Sprintf("/?n=%d", n+1), StatusFound)
+                       Redirect(w, r, fmt.Sprintf("/?n=%d", n+1), StatusTemporaryRedirect)
                        return
                }
                fmt.Fprintf(w, "n=%d", n)
@@ -296,7 +296,7 @@ func TestClientRedirects(t *testing.T) {
 func TestClientRedirectContext(t *testing.T) {
        defer afterTest(t)
        ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
-               Redirect(w, r, "/", StatusFound)
+               Redirect(w, r, "/", StatusTemporaryRedirect)
        }))
        defer ts.Close()
 
@@ -320,7 +320,91 @@ func TestClientRedirectContext(t *testing.T) {
        }
 }
 
+type redirectTest struct {
+       suffix       string
+       want         int // response code
+       redirectBody string
+}
+
 func TestPostRedirects(t *testing.T) {
+       postRedirectTests := []redirectTest{
+               {"/", 200, "first"},
+               {"/?code=301&next=302", 200, "c301"},
+               {"/?code=302&next=302", 200, "c302"},
+               {"/?code=303&next=301", 200, "c303wc301"}, // Issue 9348
+               {"/?code=304", 304, "c304"},
+               {"/?code=305", 305, "c305"},
+               {"/?code=307&next=303,308,302", 200, "c307"},
+               {"/?code=308&next=302,301", 200, "c308"},
+               {"/?code=404", 404, "c404"},
+       }
+
+       wantSegments := []string{
+               `POST / "first"`,
+               `POST /?code=301&next=302 "c301"`,
+               `GET /?code=302 "c301"`,
+               `GET / "c301"`,
+               `POST /?code=302&next=302 "c302"`,
+               `GET /?code=302 "c302"`,
+               `GET / "c302"`,
+               `POST /?code=303&next=301 "c303wc301"`,
+               `GET /?code=301 "c303wc301"`,
+               `GET / "c303wc301"`,
+               `POST /?code=304 "c304"`,
+               `POST /?code=305 "c305"`,
+               `POST /?code=307&next=303,308,302 "c307"`,
+               `POST /?code=303&next=308,302 "c307"`,
+               `GET /?code=308&next=302 "c307"`,
+               `GET /?code=302 "c307"`,
+               `GET / "c307"`,
+               `POST /?code=308&next=302,301 "c308"`,
+               `POST /?code=302&next=301 "c308"`,
+               `GET /?code=301 "c308"`,
+               `GET / "c308"`,
+               `POST /?code=404 "c404"`,
+       }
+       want := strings.Join(wantSegments, "\n")
+       testRedirectsByMethod(t, "POST", postRedirectTests, want)
+}
+
+func TestDeleteRedirects(t *testing.T) {
+       deleteRedirectTests := []redirectTest{
+               {"/", 200, "first"},
+               {"/?code=301&next=302,308", 200, "c301"},
+               {"/?code=302&next=302", 200, "c302"},
+               {"/?code=303", 200, "c303"},
+               {"/?code=307&next=301,308,303,302,304", 304, "c307"},
+               {"/?code=308&next=307", 200, "c308"},
+               {"/?code=404", 404, "c404"},
+       }
+
+       wantSegments := []string{
+               `DELETE / "first"`,
+               `DELETE /?code=301&next=302,308 "c301"`,
+               `GET /?code=302&next=308 "c301"`,
+               `GET /?code=308 "c301"`,
+               `GET / "c301"`,
+               `DELETE /?code=302&next=302 "c302"`,
+               `GET /?code=302 "c302"`,
+               `GET / "c302"`,
+               `DELETE /?code=303 "c303"`,
+               `GET / "c303"`,
+               `DELETE /?code=307&next=301,308,303,302,304 "c307"`,
+               `DELETE /?code=301&next=308,303,302,304 "c307"`,
+               `GET /?code=308&next=303,302,304 "c307"`,
+               `GET /?code=303&next=302,304 "c307"`,
+               `GET /?code=302&next=304 "c307"`,
+               `GET /?code=304 "c307"`,
+               `DELETE /?code=308&next=307 "c308"`,
+               `DELETE /?code=307 "c308"`,
+               `DELETE / "c308"`,
+               `DELETE /?code=404 "c404"`,
+       }
+       want := strings.Join(wantSegments, "\n")
+       testRedirectsByMethod(t, "DELETE", deleteRedirectTests, want)
+}
+
+func testRedirectsByMethod(t *testing.T, method string, table []redirectTest, want string) {
        defer afterTest(t)
        var log struct {
                sync.Mutex
@@ -329,29 +413,35 @@ func TestPostRedirects(t *testing.T) {
        var ts *httptest.Server
        ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
                log.Lock()
-               fmt.Fprintf(&log.Buffer, "%s %s ", r.Method, r.RequestURI)
+               slurp, _ := ioutil.ReadAll(r.Body)
+               fmt.Fprintf(&log.Buffer, "%s %s %q\n", r.Method, r.RequestURI, slurp)
                log.Unlock()
-               if v := r.URL.Query().Get("code"); v != "" {
+               urlQuery := r.URL.Query()
+               if v := urlQuery.Get("code"); v != "" {
+                       location := ts.URL
+                       if final := urlQuery.Get("next"); final != "" {
+                               splits := strings.Split(final, ",")
+                               first, rest := splits[0], splits[1:]
+                               location = fmt.Sprintf("%s?code=%s", location, first)
+                               if len(rest) > 0 {
+                                       location = fmt.Sprintf("%s&next=%s", location, strings.Join(rest, ","))
+                               }
+                       }
                        code, _ := strconv.Atoi(v)
                        if code/100 == 3 {
-                               w.Header().Set("Location", ts.URL)
+                               w.Header().Set("Location", location)
                        }
                        w.WriteHeader(code)
                }
        }))
        defer ts.Close()
-       tests := []struct {
-               suffix string
-               want   int // response code
-       }{
-               {"/", 200},
-               {"/?code=301", 301},
-               {"/?code=302", 200},
-               {"/?code=303", 200},
-               {"/?code=404", 404},
-       }
-       for _, tt := range tests {
-               res, err := Post(ts.URL+tt.suffix, "text/plain", strings.NewReader("Some content"))
+
+       for _, tt := range table {
+               content := tt.redirectBody
+               req, _ := NewRequest(method, ts.URL+tt.suffix, strings.NewReader(content))
+               req.GetBody = func() (io.ReadCloser, error) { return ioutil.NopCloser(strings.NewReader(content)), nil }
+               res, err := DefaultClient.Do(req)
+
                if err != nil {
                        t.Fatal(err)
                }
@@ -362,9 +452,12 @@ func TestPostRedirects(t *testing.T) {
        log.Lock()
        got := log.String()
        log.Unlock()
-       want := "POST / POST /?code=301 POST /?code=302 GET / POST /?code=303 GET / POST /?code=404 "
+
+       got = strings.TrimSpace(got)
+       want = strings.TrimSpace(want)
+
        if got != want {
-               t.Errorf("Log differs.\n Got: %q\nWant: %q", got, want)
+               t.Errorf("Log differs.\n Got:\n%s\nWant:\n%s\n", got, want)
        }
 }
 
@@ -1439,22 +1532,45 @@ func TestClientRedirectTypes(t *testing.T) {
        defer afterTest(t)
 
        tests := [...]struct {
-               broken       int // broken is bug number
                method       string
                serverStatus int
                wantMethod   string // desired subsequent client method
        }{
                0: {method: "POST", serverStatus: 301, wantMethod: "GET"},
                1: {method: "POST", serverStatus: 302, wantMethod: "GET"},
-               2: {method: "POST", serverStatus: 307, wantMethod: "POST", broken: 16840},
-
-               5: {method: "GET", serverStatus: 301, wantMethod: "GET"},
-               6: {method: "GET", serverStatus: 302, wantMethod: "GET"},
-               7: {method: "GET", serverStatus: 303, wantMethod: "GET"},
-               8: {method: "GET", serverStatus: 307, wantMethod: "GET"},
-               9: {method: "GET", serverStatus: 308, wantMethod: "GET"},
-
-               10: {method: "DELETE", serverStatus: 308, wantMethod: "DELETE", broken: 13994},
+               2: {method: "POST", serverStatus: 303, wantMethod: "GET"},
+               3: {method: "POST", serverStatus: 307, wantMethod: "POST"},
+               4: {method: "POST", serverStatus: 308, wantMethod: "POST"},
+
+               5: {method: "HEAD", serverStatus: 301, wantMethod: "GET"},
+               6: {method: "HEAD", serverStatus: 302, wantMethod: "GET"},
+               7: {method: "HEAD", serverStatus: 303, wantMethod: "GET"},
+               8: {method: "HEAD", serverStatus: 307, wantMethod: "HEAD"},
+               9: {method: "HEAD", serverStatus: 308, wantMethod: "HEAD"},
+
+               10: {method: "GET", serverStatus: 301, wantMethod: "GET"},
+               11: {method: "GET", serverStatus: 302, wantMethod: "GET"},
+               12: {method: "GET", serverStatus: 303, wantMethod: "GET"},
+               13: {method: "GET", serverStatus: 307, wantMethod: "GET"},
+               14: {method: "GET", serverStatus: 308, wantMethod: "GET"},
+
+               15: {method: "DELETE", serverStatus: 301, wantMethod: "GET"},
+               16: {method: "DELETE", serverStatus: 302, wantMethod: "GET"},
+               17: {method: "DELETE", serverStatus: 303, wantMethod: "GET"},
+               18: {method: "DELETE", serverStatus: 307, wantMethod: "DELETE"},
+               19: {method: "DELETE", serverStatus: 308, wantMethod: "DELETE"},
+
+               20: {method: "PUT", serverStatus: 301, wantMethod: "GET"},
+               21: {method: "PUT", serverStatus: 302, wantMethod: "GET"},
+               22: {method: "PUT", serverStatus: 303, wantMethod: "GET"},
+               23: {method: "PUT", serverStatus: 307, wantMethod: "PUT"},
+               24: {method: "PUT", serverStatus: 308, wantMethod: "PUT"},
+
+               25: {method: "MADEUPMETHOD", serverStatus: 301, wantMethod: "GET"},
+               26: {method: "MADEUPMETHOD", serverStatus: 302, wantMethod: "GET"},
+               27: {method: "MADEUPMETHOD", serverStatus: 303, wantMethod: "GET"},
+               28: {method: "MADEUPMETHOD", serverStatus: 307, wantMethod: "MADEUPMETHOD"},
+               29: {method: "MADEUPMETHOD", serverStatus: 308, wantMethod: "MADEUPMETHOD"},
        }
 
        handlerc := make(chan HandlerFunc, 1)
@@ -1466,11 +1582,6 @@ func TestClientRedirectTypes(t *testing.T) {
        defer ts.Close()
 
        for i, tt := range tests {
-               if tt.broken != 0 {
-                       t.Logf("#%d: skipping known broken test case. See Issue #%d", i, tt.broken)
-                       continue
-               }
-
                handlerc <- func(w ResponseWriter, r *Request) {
                        w.Header().Set("Location", ts.URL)
                        w.WriteHeader(tt.serverStatus)