From ab40107708042ded6bdc1fb841c7cf2c2ab002ab Mon Sep 17 00:00:00 2001 From: Kunpei Sakai Date: Sun, 3 Sep 2017 02:08:02 +0900 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Tom Bergan Reviewed-by: Emmanuel Odeke --- src/net/http/serve_test.go | 62 ++++++++++++++++++++++++++++++++++++ src/net/http/server.go | 65 +++++++++++++++++++++++++------------- 2 files changed, 105 insertions(+), 22 deletions(-) diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 7137599c42..68b78301cb 100644 --- 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 d29d3a462a..b02544bc2a 100644 --- 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. -- 2.50.0