net/http: make ServeMux preserve query string during redirects

Ensure that the implicitly created redirect
for
  "/route"
after
  "/route/"
has been registered doesn't lose the query string information.
A previous attempt (https://golang.org/cl/43779) changed http.Redirect, however, that change broke direct calls to http.Redirect.
To avoid that problem, this change touches ServeMux.Handler only.

Fixes #17841

Change-Id: I303c1b1824615304ae68147e254bb41b0ea339be
Reviewed-on: https://go-review.googlesource.com/61210
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 7137599c42eff36e5a864e125bb950dbde0e8dc3..68b78301cb978085a63ef1fbd9401dc504f9f01d 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -461,6 +461,68 @@ func TestMuxRedirectLeadingSlashes(t *testing.T) {
        }
 }
 
+// Test that the special cased "/route" redirect
+// implicitly created by a registered "/route/"
+// properly sets the query string in the redirect URL.
+// See Issue 17841.
+func TestServeWithSlashRedirectKeepsQueryString(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+
+       writeBackQuery := func(w ResponseWriter, r *Request) {
+               fmt.Fprintf(w, "%s", r.URL.RawQuery)
+       }
+
+       mux := NewServeMux()
+       mux.HandleFunc("/testOne", writeBackQuery)
+       mux.HandleFunc("/testTwo/", writeBackQuery)
+       mux.HandleFunc("/testThree", writeBackQuery)
+       mux.HandleFunc("/testThree/", func(w ResponseWriter, r *Request) {
+               fmt.Fprintf(w, "%s:bar", r.URL.RawQuery)
+       })
+
+       ts := httptest.NewServer(mux)
+       defer ts.Close()
+
+       tests := [...]struct {
+               path     string
+               method   string
+               want     string
+               statusOk bool
+       }{
+               0: {"/testOne?this=that", "GET", "this=that", true},
+               1: {"/testTwo?foo=bar", "GET", "foo=bar", true},
+               2: {"/testTwo?a=1&b=2&a=3", "GET", "a=1&b=2&a=3", true},
+               3: {"/testTwo?", "GET", "", true},
+               4: {"/testThree?foo", "GET", "foo", true},
+               5: {"/testThree/?foo", "GET", "foo:bar", true},
+               6: {"/testThree?foo", "CONNECT", "foo", true},
+               7: {"/testThree/?foo", "CONNECT", "foo:bar", true},
+
+               // canonicalization or not
+               8: {"/testOne/foo/..?foo", "GET", "foo", true},
+               9: {"/testOne/foo/..?foo", "CONNECT", "404 page not found\n", false},
+       }
+
+       for i, tt := range tests {
+               req, _ := NewRequest(tt.method, ts.URL+tt.path, nil)
+               res, err := ts.Client().Do(req)
+               if err != nil {
+                       continue
+               }
+               slurp, _ := ioutil.ReadAll(res.Body)
+               res.Body.Close()
+               if !tt.statusOk {
+                       if got, want := res.StatusCode, 404; got != want {
+                               t.Errorf("#%d: Status = %d; want = %d", i, got, want)
+                       }
+               }
+               if got, want := string(slurp), tt.want; got != want {
+                       t.Errorf("#%d: Body = %q; want = %q", i, got, want)
+               }
+       }
+}
+
 func BenchmarkServeMux(b *testing.B) {
 
        type test struct {

diff --git a/src/net/http/server.go b/src/net/http/server.go
index d29d3a462a4f3af79bca578c717260a56229146d..b02544bc2a889475f1910af831553f9aacaf5676 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2112,9 +2112,8 @@ type ServeMux struct {
 }
 
 type muxEntry struct {
-       explicit bool
-       h        Handler
-       pattern  string
+       h       Handler
+       pattern string
 }
 
 // NewServeMux allocates and returns a new ServeMux.
@@ -2192,6 +2191,31 @@ func (mux *ServeMux) match(path string) (h Handler, pattern string) {
        return
 }
 
+// redirectToPathSlash determines if the given path needs appending "/" to it.
+// This occurs when a handler for path + "/" was already registered, but
+// not for path itself. If the path needs appending to, it creates a new
+// URL, setting the path to u.Path + "/" and returning true to indicate so.
+func (mux *ServeMux) redirectToPathSlash(path string, u *url.URL) (*url.URL, bool) {
+       if !mux.shouldRedirect(path) {
+               return u, false
+       }
+       path = path + "/"
+       u = &url.URL{Path: path, RawQuery: u.RawQuery}
+       return u, true
+}
+
+// shouldRedirect reports whether the given path should be redirected to
+// path+"/". This should happen if a handler is registered for path+"/" but
+// not path -- see comments at ServeMux.
+func (mux *ServeMux) shouldRedirect(path string) bool {
+       if _, exist := mux.m[path]; exist {
+               return false
+       }
+       n := len(path)
+       _, exist := mux.m[path+"/"]
+       return n > 0 && path[n-1] != '/' && exist
+}
+
 // Handler returns the handler to use for the given request,
 // consulting r.Method, r.Host, and r.URL.Path. It always returns
 // a non-nil handler. If the path is not in its canonical form, the
@@ -2211,6 +2235,13 @@ func (mux *ServeMux) Handler(r *Request) (h Handler, pattern string) {
 
        // CONNECT requests are not canonicalized.
        if r.Method == "CONNECT" {
+               // If r.URL.Path is /tree and its handler is not registered,
+               // the /tree -> /tree/ redirect applies to CONNECT requests
+               // but the path canonicalization does not.
+               if u, ok := mux.redirectToPathSlash(r.URL.Path, r.URL); ok {
+                       return RedirectHandler(u.String(), StatusMovedPermanently), u.Path
+               }
+
                return mux.handler(r.Host, r.URL.Path)
        }
 
@@ -2218,6 +2249,13 @@ func (mux *ServeMux) Handler(r *Request) (h Handler, pattern string) {
        // before passing to mux.handler.
        host := stripHostPort(r.Host)
        path := cleanPath(r.URL.Path)
+
+       // If the given path is /tree and its handler is not registered,
+       // redirect for /tree/.
+       if u, ok := mux.redirectToPathSlash(path, r.URL); ok {
+               return RedirectHandler(u.String(), StatusMovedPermanently), u.Path
+       }
+
        if path != r.URL.Path {
                _, pattern = mux.handler(host, path)
                url := *r.URL
@@ -2273,35 +2311,18 @@ func (mux *ServeMux) Handle(pattern string, handler Handler) {
        if handler == nil {
                panic("http: nil handler")
        }
-       if mux.m[pattern].explicit {
+       if _, exist := mux.m[pattern]; exist {
                panic("http: multiple registrations for " + pattern)
        }
 
        if mux.m == nil {
                mux.m = make(map[string]muxEntry)
        }
-       mux.m[pattern] = muxEntry{explicit: true, h: handler, pattern: pattern}
+       mux.m[pattern] = muxEntry{h: handler, pattern: pattern}
 
        if pattern[0] != '/' {
                mux.hosts = true
        }
-
-       // Helpful behavior:
-       // If pattern is /tree/, insert an implicit permanent redirect for /tree.
-       // It can be overridden by an explicit registration.
-       n := len(pattern)
-       if n > 0 && pattern[n-1] == '/' && !mux.m[pattern[0:n-1]].explicit {
-               // If pattern contains a host name, strip it and use remaining
-               // path for redirect.
-               path := pattern
-               if pattern[0] != '/' {
-                       // In pattern, at least the last character is a '/', so
-                       // strings.Index can't be -1.
-                       path = pattern[strings.Index(pattern, "/"):]
-               }
-               url := &url.URL{Path: path}
-               mux.m[pattern[0:n-1]] = muxEntry{h: RedirectHandler(url.String(), StatusMovedPermanently), pattern: pattern}
-       }
 }
 
 // HandleFunc registers the handler function for the given pattern.