Skip to content

G
go
Commit 7db996ee authored by Emmanuel Odeke's avatar Emmanuel Odeke Committed by Brad Fitzpatrick
Browse files
Options

net/http: handle 3xx redirects properly 

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: default avatarBrad Fitzpatrick <bradfitz@golang.org>
parent f030eb63
Hide whitespace changes
Inline Side-by-side
Showing with 203 additions and 103 deletions
src/net/http/client.go
View file @ 7db996ee
...@@ -156,40 +156,6 @@ func (c *Client) send(req *Request, deadline time.Time) (*Response, error) { ...@@ -156,40 +156,6 @@ func (c *Client) send(req *Request, deadline time.Time) (*Response, error) {
return resp, nil 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 { func (c *Client) deadline() time.Time {
if c.Timeout > 0 { if c.Timeout > 0 {
return time.Now().Add(c.Timeout) return time.Now().Add(c.Timeout)
...@@ -351,26 +317,6 @@ func basicAuth(username, password string) string { ...@@ -351,26 +317,6 @@ func basicAuth(username, password string) string {
return base64.StdEncoding.EncodeToString([]byte(auth)) 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 // 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 // the following redirect codes, Get follows the redirect, up to a
// maximum of 10 redirects: // maximum of 10 redirects:
...@@ -379,6 +325,7 @@ func shouldRedirectPost(statusCode int) bool { ...@@ -379,6 +325,7 @@ func shouldRedirectPost(statusCode int) bool {
// 302 (Found) // 302 (Found)
// 303 (See Other) // 303 (See Other)
// 307 (Temporary Redirect) // 307 (Temporary Redirect)
// 308 (Permanent Redirect)
// //
// An error is returned if there were too many redirects or if there // 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 // 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) { ...@@ -403,6 +350,7 @@ func Get(url string) (resp *Response, err error) {
// 302 (Found) // 302 (Found)
// 303 (See Other) // 303 (See Other)
// 307 (Temporary Redirect) // 307 (Temporary Redirect)
// 308 (Permanent Redirect)
// //
// An error is returned if the Client's CheckRedirect function fails // 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 // 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) { ...@@ -417,7 +365,7 @@ func (c *Client) Get(url string) (resp *Response, err error) {
if err != nil { if err != nil {
return nil, err return nil, err
} }
return c.doFollowingRedirects(req, shouldRedirectGet) return c.Do(req)
} }
func alwaysFalse() bool { return false } func alwaysFalse() bool { return false }
...@@ -438,17 +386,56 @@ func (c *Client) checkRedirect(req *Request, via []*Request) error { ...@@ -438,17 +386,56 @@ func (c *Client) checkRedirect(req *Request, via []*Request) error {
return fn(req, via) 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 { if req.URL == nil {
req.closeBody() req.closeBody()
return nil, errors.New("http: nil Request.URL") return nil, errors.New("http: nil Request.URL")
} }
var ( var (
deadline = c.deadline() deadline = c.deadline()
reqs []*Request reqs []*Request
resp *Response resp *Response
copyHeaders = c.makeHeadersCopier(req) copyHeaders = c.makeHeadersCopier(req)
redirectMethod string
) )
uerr := func(err error) error { uerr := func(err error) error {
req.closeBody() req.closeBody()
...@@ -479,7 +466,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo ...@@ -479,7 +466,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
} }
ireq := reqs[0] ireq := reqs[0]
req = &Request{ req = &Request{
Method: ireq.Method, Method: redirectMethod,
Response: resp, Response: resp,
URL: u, URL: u,
Header: make(Header), Header: make(Header),
...@@ -491,10 +478,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo ...@@ -491,10 +478,7 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
if err != nil { if err != nil {
return nil, uerr(err) return nil, uerr(err)
} }
} req.ContentLength = ireq.ContentLength
if ireq.Method == "POST" || ireq.Method == "PUT" {
req.Method = "GET"
req.Body = nil // TODO: fix this when 307/308 support happens
} }
// Copy original headers before setting the Referer, // Copy original headers before setting the Referer,
...@@ -540,7 +524,6 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo ...@@ -540,7 +524,6 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
} }
reqs = append(reqs, req) reqs = append(reqs, req)
var err error var err error
if resp, err = c.send(req, deadline); err != nil { if resp, err = c.send(req, deadline); err != nil {
if !deadline.IsZero() && !time.Now().Before(deadline) { if !deadline.IsZero() && !time.Now().Before(deadline) {
...@@ -552,9 +535,13 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo ...@@ -552,9 +535,13 @@ func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) boo
return nil, uerr(err) return nil, uerr(err)
} }
if !shouldRedirect(resp.StatusCode) { var shouldRedirect bool
redirectMethod, shouldRedirect = redirectBehavior(req.Method, resp.StatusCode)
if !shouldRedirect {
return resp, nil return resp, nil
} }
req.closeBody()
} }
} }
...@@ -657,7 +644,7 @@ func (c *Client) Post(url string, contentType string, body io.Reader) (resp *Res ...@@ -657,7 +644,7 @@ func (c *Client) Post(url string, contentType string, body io.Reader) (resp *Res
return nil, err return nil, err
} }
req.Header.Set("Content-Type", contentType) 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 // 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 ...@@ -694,6 +681,7 @@ func (c *Client) PostForm(url string, data url.Values) (resp *Response, err erro
// 302 (Found) // 302 (Found)
// 303 (See Other) // 303 (See Other)
// 307 (Temporary Redirect) // 307 (Temporary Redirect)
// 308 (Permanent Redirect)
// //
// Head is a wrapper around DefaultClient.Head // Head is a wrapper around DefaultClient.Head
func Head(url string) (resp *Response, err error) { func Head(url string) (resp *Response, err error) {
...@@ -708,12 +696,13 @@ func Head(url string) (resp *Response, err error) { ...@@ -708,12 +696,13 @@ func Head(url string) (resp *Response, err error) {
// 302 (Found) // 302 (Found)
// 303 (See Other) // 303 (See Other)
// 307 (Temporary Redirect) // 307 (Temporary Redirect)
// 308 (Permanent Redirect)
func (c *Client) Head(url string) (resp *Response, err error) { func (c *Client) Head(url string) (resp *Response, err error) {
req, err := NewRequest("HEAD", url, nil) req, err := NewRequest("HEAD", url, nil)
if err != nil { if err != nil {
return nil, err return nil, err
} }
return c.doFollowingRedirects(req, shouldRedirectGet) return c.Do(req)
} }
// cancelTimerBody is an io.ReadCloser that wraps rc with two features: // cancelTimerBody is an io.ReadCloser that wraps rc with two features:
......
src/net/http/client_test.go
View file @ 7db996ee
...@@ -208,7 +208,7 @@ func TestClientRedirects(t *testing.T) { ...@@ -208,7 +208,7 @@ func TestClientRedirects(t *testing.T) {
} }
} }
if n < 15 { if n < 15 {
Redirect(w, r, fmt.Sprintf("/?n=%d", n+1), StatusFound) Redirect(w, r, fmt.Sprintf("/?n=%d", n+1), StatusTemporaryRedirect)
return return
} }
fmt.Fprintf(w, "n=%d", n) fmt.Fprintf(w, "n=%d", n)
...@@ -296,7 +296,7 @@ func TestClientRedirects(t *testing.T) { ...@@ -296,7 +296,7 @@ func TestClientRedirects(t *testing.T) {
func TestClientRedirectContext(t *testing.T) { func TestClientRedirectContext(t *testing.T) {
defer afterTest(t) defer afterTest(t)
ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
Redirect(w, r, "/", StatusFound) Redirect(w, r, "/", StatusTemporaryRedirect)
})) }))
defer ts.Close() defer ts.Close()
...@@ -320,7 +320,91 @@ func TestClientRedirectContext(t *testing.T) { ...@@ -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) { 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) defer afterTest(t)
var log struct { var log struct {
sync.Mutex sync.Mutex
...@@ -329,29 +413,35 @@ func TestPostRedirects(t *testing.T) { ...@@ -329,29 +413,35 @@ func TestPostRedirects(t *testing.T) {
var ts *httptest.Server var ts *httptest.Server
ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
log.Lock() 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() 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) code, _ := strconv.Atoi(v)
if code/100 == 3 { if code/100 == 3 {
w.Header().Set("Location", ts.URL) w.Header().Set("Location", location)
} }
w.WriteHeader(code) w.WriteHeader(code)
} }
})) }))
defer ts.Close() defer ts.Close()
tests := []struct {
suffix string for _, tt := range table {
want int // response code content := tt.redirectBody
}{ req, _ := NewRequest(method, ts.URL+tt.suffix, strings.NewReader(content))
{"/", 200}, req.GetBody = func() (io.ReadCloser, error) { return ioutil.NopCloser(strings.NewReader(content)), nil }
{"/?code=301", 301}, res, err := DefaultClient.Do(req)
{"/?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"))
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
...@@ -362,9 +452,12 @@ func TestPostRedirects(t *testing.T) { ...@@ -362,9 +452,12 @@ func TestPostRedirects(t *testing.T) {
log.Lock() log.Lock()
got := log.String() got := log.String()
log.Unlock() 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 { 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) { ...@@ -1439,22 +1532,45 @@ func TestClientRedirectTypes(t *testing.T) {
defer afterTest(t) defer afterTest(t)
tests := [...]struct { tests := [...]struct {
broken int // broken is bug number
method string method string
serverStatus int serverStatus int
wantMethod string // desired subsequent client method wantMethod string // desired subsequent client method
}{ }{
0: {method: "POST", serverStatus: 301, wantMethod: "GET"}, 0: {method: "POST", serverStatus: 301, wantMethod: "GET"},
1: {method: "POST", serverStatus: 302, wantMethod: "GET"}, 1: {method: "POST", serverStatus: 302, wantMethod: "GET"},
2: {method: "POST", serverStatus: 307, wantMethod: "POST", broken: 16840}, 2: {method: "POST", serverStatus: 303, wantMethod: "GET"},
3: {method: "POST", serverStatus: 307, wantMethod: "POST"},
5: {method: "GET", serverStatus: 301, wantMethod: "GET"}, 4: {method: "POST", serverStatus: 308, wantMethod: "POST"},
6: {method: "GET", serverStatus: 302, wantMethod: "GET"},
7: {method: "GET", serverStatus: 303, wantMethod: "GET"}, 5: {method: "HEAD", serverStatus: 301, wantMethod: "GET"},
8: {method: "GET", serverStatus: 307, wantMethod: "GET"}, 6: {method: "HEAD", serverStatus: 302, wantMethod: "GET"},
9: {method: "GET", serverStatus: 308, wantMethod: "GET"}, 7: {method: "HEAD", serverStatus: 303, wantMethod: "GET"},
8: {method: "HEAD", serverStatus: 307, wantMethod: "HEAD"},
10: {method: "DELETE", serverStatus: 308, wantMethod: "DELETE", broken: 13994}, 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) handlerc := make(chan HandlerFunc, 1)
...@@ -1466,11 +1582,6 @@ func TestClientRedirectTypes(t *testing.T) { ...@@ -1466,11 +1582,6 @@ func TestClientRedirectTypes(t *testing.T) {
defer ts.Close() defer ts.Close()
for i, tt := range tests { 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) { handlerc <- func(w ResponseWriter, r *Request) {
w.Header().Set("Location", ts.URL) w.Header().Set("Location", ts.URL)
w.WriteHeader(tt.serverStatus) w.WriteHeader(tt.serverStatus)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7