net/http: use HTTP 307 redirects in ServeMux

Clients receiving an HTTP 301 Moved Permanently may conservatively
change the method of a POST request to GET.
The newer HTTP 307 Temporary Redirect and 308 Permanent Redirect
explicitly allows retrying POST requests after the redirect.
These should be safe for ServeMux as this internal redirect is generated
before user provided handlers are called.
As ServeMux is making the redirect for the user without explicit
direction, and clients may cache Permanent Redirects indefinitely,
Temporary Redirect is used in case the user adds a handler for a path,
that was previously redirected but no longer should.

Fixes #50243
Fixes #60769

Change-Id: I6c0b735bab03bb7b50f05457b3b8a8ba813badb2
Reviewed-on: https://go-review.googlesource.com/c/go/+/720820
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Mark Freeman <markfreeman@google.com>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 4a16ba02af6cee57f1c7a05adbd5ee43eae618e5..4aa5b3a50fd36dea1479b437a90b928d50687a61 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -288,7 +288,7 @@ func testHostHandlers(t *testing.T, mode testMode) {
                        if s != vt.expected {
                                t.Errorf("Get(%q) = %q, want %q", vt.url, s, vt.expected)
                        }
-               case StatusMovedPermanently:
+               case StatusTemporaryRedirect:
                        s := r.Header.Get("Location")
                        if s != vt.expected {
                                t.Errorf("Get(%q) = %q, want %q", vt.url, s, vt.expected)
@@ -340,7 +340,7 @@ var serveMuxTests = []struct {
        pattern string
 }{
        {"GET", "google.com", "/", 404, ""},
-       {"GET", "google.com", "/dir", 301, "/dir/"},
+       {"GET", "google.com", "/dir", 307, "/dir/"},
        {"GET", "google.com", "/dir/", 200, "/dir/"},
        {"GET", "google.com", "/dir/file", 200, "/dir/"},
        {"GET", "google.com", "/search", 201, "/search"},
@@ -354,14 +354,14 @@ var serveMuxTests = []struct {
        {"GET", "images.google.com", "/search", 201, "/search"},
        {"GET", "images.google.com", "/search/", 404, ""},
        {"GET", "images.google.com", "/search/foo", 404, ""},
-       {"GET", "google.com", "/../search", 301, "/search"},
-       {"GET", "google.com", "/dir/..", 301, ""},
-       {"GET", "google.com", "/dir/..", 301, ""},
-       {"GET", "google.com", "/dir/./file", 301, "/dir/"},
+       {"GET", "google.com", "/../search", 307, "/search"},
+       {"GET", "google.com", "/dir/..", 307, ""},
+       {"GET", "google.com", "/dir/..", 307, ""},
+       {"GET", "google.com", "/dir/./file", 307, "/dir/"},
 
        // The /foo -> /foo/ redirect applies to CONNECT requests
        // but the path canonicalization does not.
-       {"CONNECT", "google.com", "/dir", 301, "/dir/"},
+       {"CONNECT", "google.com", "/dir", 307, "/dir/"},
        {"CONNECT", "google.com", "/../search", 404, ""},
        {"CONNECT", "google.com", "/dir/..", 200, "/dir/"},
        {"CONNECT", "google.com", "/dir/..", 200, "/dir/"},
@@ -454,7 +454,7 @@ func TestServeMuxHandlerRedirects(t *testing.T) {
                        h, _ := mux.Handler(r)
                        rr := httptest.NewRecorder()
                        h.ServeHTTP(rr, r)
-                       if rr.Code != 301 {
+                       if rr.Code != 307 {
                                if rr.Code != tt.code {
                                        t.Errorf("%s %s %s = %d, want %d", tt.method, tt.host, tt.url, rr.Code, tt.code)
                                }
@@ -473,6 +473,37 @@ func TestServeMuxHandlerRedirects(t *testing.T) {
        }
 }
 
+func TestServeMuxHandlerRedirectPost(t *testing.T) {
+       setParallel(t)
+       mux := NewServeMux()
+       mux.HandleFunc("POST /test/", func(w ResponseWriter, r *Request) {
+               w.WriteHeader(200)
+       })
+
+       var code, retries int
+       startURL := "http://example.com/test"
+       reqURL := startURL
+       for retries = 0; retries <= 1; retries++ {
+               r := httptest.NewRequest("POST", reqURL, strings.NewReader("hello world"))
+               h, _ := mux.Handler(r)
+               rr := httptest.NewRecorder()
+               h.ServeHTTP(rr, r)
+               code = rr.Code
+               switch rr.Code {
+               case 307:
+                       reqURL = rr.Result().Header.Get("Location")
+                       continue
+               case 200:
+                       // ok
+               default:
+                       t.Errorf("unhandled response code: %v", rr.Code)
+               }
+       }
+       if code != 200 {
+               t.Errorf("POST %s = %d after %d retries, want = 200", startURL, code, retries)
+       }
+}
+
 // Tests for https://golang.org/issue/900
 func TestMuxRedirectLeadingSlashes(t *testing.T) {
        setParallel(t)
@@ -492,8 +523,8 @@ func TestMuxRedirectLeadingSlashes(t *testing.T) {
                        return
                }
 
-               if code, expected := resp.Code, StatusMovedPermanently; code != expected {
-                       t.Errorf("Expected response code of StatusMovedPermanently; got %d", code)
+               if code, expected := resp.Code, StatusTemporaryRedirect; code != expected {
+                       t.Errorf("Expected response code of StatusPermanentRedirect; got %d", code)
                        return
                }
        }
@@ -579,18 +610,18 @@ func TestServeWithSlashRedirectForHostPatterns(t *testing.T) {
                want   string
        }{
                {"GET", "http://example.com/", 404, "", ""},
-               {"GET", "http://example.com/pkg/foo", 301, "/pkg/foo/", ""},
+               {"GET", "http://example.com/pkg/foo", 307, "/pkg/foo/", ""},
                {"GET", "http://example.com/pkg/bar", 200, "", "example.com/pkg/bar"},
                {"GET", "http://example.com/pkg/bar/", 200, "", "example.com/pkg/bar/"},
-               {"GET", "http://example.com/pkg/baz", 301, "/pkg/baz/", ""},
-               {"GET", "http://example.com:3000/pkg/foo", 301, "/pkg/foo/", ""},
+               {"GET", "http://example.com/pkg/baz", 307, "/pkg/baz/", ""},
+               {"GET", "http://example.com:3000/pkg/foo", 307, "/pkg/foo/", ""},
                {"CONNECT", "http://example.com/", 404, "", ""},
                {"CONNECT", "http://example.com:3000/", 404, "", ""},
                {"CONNECT", "http://example.com:9000/", 200, "", "example.com:9000/"},
-               {"CONNECT", "http://example.com/pkg/foo", 301, "/pkg/foo/", ""},
+               {"CONNECT", "http://example.com/pkg/foo", 307, "/pkg/foo/", ""},
                {"CONNECT", "http://example.com:3000/pkg/foo", 404, "", ""},
-               {"CONNECT", "http://example.com:3000/pkg/baz", 301, "/pkg/baz/", ""},
-               {"CONNECT", "http://example.com:3000/pkg/connect", 301, "/pkg/connect/", ""},
+               {"CONNECT", "http://example.com:3000/pkg/baz", 307, "/pkg/baz/", ""},
+               {"CONNECT", "http://example.com:3000/pkg/connect", 307, "/pkg/connect/", ""},
        }
 
        for i, tt := range tests {
@@ -6940,7 +6971,7 @@ func TestMuxRedirectRelative(t *testing.T) {
        if got, want := resp.Header().Get("Location"), "/"; got != want {
                t.Errorf("Location header expected %q; got %q", want, got)
        }
-       if got, want := resp.Code, StatusMovedPermanently; got != want {
+       if got, want := resp.Code, StatusTemporaryRedirect; got != want {
                t.Errorf("Expected response code %d; got %d", want, got)
        }
 }

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 02554d1a201afd329f1e8ff09323b9e12e6fc10b..1a7f7519908d8d858094aae90b202fac2e96c82e 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2704,7 +2704,7 @@ func (mux *ServeMux) findHandler(r *Request) (h Handler, patStr string, _ *patte
                // but the path canonicalization does not.
                _, _, u := mux.matchOrRedirect(host, r.Method, path, r.URL)
                if u != nil {
-                       return RedirectHandler(u.String(), StatusMovedPermanently), u.Path, nil, nil
+                       return RedirectHandler(u.String(), StatusTemporaryRedirect), u.Path, nil, nil
                }
                // Redo the match, this time with r.Host instead of r.URL.Host.
                // Pass a nil URL to skip the trailing-slash redirect logic.
@@ -2720,7 +2720,7 @@ func (mux *ServeMux) findHandler(r *Request) (h Handler, patStr string, _ *patte
                var u *url.URL
                n, matches, u = mux.matchOrRedirect(host, r.Method, path, r.URL)
                if u != nil {
-                       return RedirectHandler(u.String(), StatusMovedPermanently), n.pattern.String(), nil, nil
+                       return RedirectHandler(u.String(), StatusTemporaryRedirect), n.pattern.String(), nil, nil
                }
                if path != escapedPath {
                        // Redirect to cleaned path.
@@ -2729,7 +2729,7 @@ func (mux *ServeMux) findHandler(r *Request) (h Handler, patStr string, _ *patte
                                patStr = n.pattern.String()
                        }
                        u := &url.URL{Path: path, RawQuery: r.URL.RawQuery}
-                       return RedirectHandler(u.String(), StatusMovedPermanently), patStr, nil, nil
+                       return RedirectHandler(u.String(), StatusTemporaryRedirect), patStr, nil, nil
                }
        }
        if n == nil {

diff --git a/src/net/http/server_test.go b/src/net/http/server_test.go
index 832f9688b63d9c6007272a864a065fda8ffa8448..edcf362062672952aa5ba0fbee26094520e3fc68 100644 (file)
--- a/src/net/http/server_test.go
+++ b/src/net/http/server_test.go
@@ -91,12 +91,12 @@ func TestFindHandler(t *testing.T) {
                wantHandler string
        }{
                {"GET", "/", "&http.handler{i:1}"},
-               {"GET", "//", `&http.redirectHandler{url:"/", code:301}`},
-               {"GET", "/foo/../bar/./..//baz", `&http.redirectHandler{url:"/baz", code:301}`},
+               {"GET", "//", `&http.redirectHandler{url:"/", code:307}`},
+               {"GET", "/foo/../bar/./..//baz", `&http.redirectHandler{url:"/baz", code:307}`},
                {"GET", "/foo", "&http.handler{i:3}"},
                {"GET", "/foo/x", "&http.handler{i:2}"},
                {"GET", "/bar/x", "&http.handler{i:4}"},
-               {"GET", "/bar", `&http.redirectHandler{url:"/bar/", code:301}`},
+               {"GET", "/bar", `&http.redirectHandler{url:"/bar/", code:307}`},
                {"CONNECT", "", "(http.HandlerFunc)(.*)"},
                {"CONNECT", "/", "&http.handler{i:1}"},
                {"CONNECT", "//", "&http.handler{i:1}"},
@@ -105,7 +105,7 @@ func TestFindHandler(t *testing.T) {
                {"CONNECT", "/foo", "&http.handler{i:3}"},
                {"CONNECT", "/foo/x", "&http.handler{i:2}"},
                {"CONNECT", "/bar/x", "&http.handler{i:4}"},
-               {"CONNECT", "/bar", `&http.redirectHandler{url:"/bar/", code:301}`},
+               {"CONNECT", "/bar", `&http.redirectHandler{url:"/bar/", code:307}`},
        } {
                var r Request
                r.Method = test.method