From 36d2027493a65e76463d7075d37cd0a139dcb7f7 Mon Sep 17 00:00:00 2001 From: ericdreeves Date: Wed, 15 Mar 2017 11:17:12 -0500 Subject: [PATCH] browse: Use helper functions in staticfiles to redirect (#1497) * Use helper functions in staticfiles to redirect. Previously the browse package invoked staticfiles.Redirect when redirecting clients who requested a directory but with a Request-URI that did not contain a trailing '/'. staticfiles.Redirect only used a relative URI. This change defers the decision of how to format the Location header value to the helper methods in the staticfiles package. * Update const URLPathCtxKey in browse package. --- caddyhttp/browse/browse.go | 2 +- caddyhttp/browse/browse_test.go | 66 +++++++++++++++++++++++++++++ caddyhttp/staticfiles/fileserver.go | 54 ++++++++++++----------- 3 files changed, 96 insertions(+), 26 deletions(-) diff --git a/caddyhttp/browse/browse.go b/caddyhttp/browse/browse.go index 36064c1d..9da9b860 100644 --- a/caddyhttp/browse/browse.go +++ b/caddyhttp/browse/browse.go @@ -299,7 +299,7 @@ func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { // Browsing navigation gets messed up if browsing a directory // that doesn't end in "/" (which it should, anyway) if !strings.HasSuffix(r.URL.Path, "/") { - staticfiles.Redirect(w, r, r.URL.Path+"/", http.StatusTemporaryRedirect) + staticfiles.RedirectToDir(w, r) return 0, nil } diff --git a/caddyhttp/browse/browse_test.go b/caddyhttp/browse/browse_test.go index c3f945c9..500c6e16 100644 --- a/caddyhttp/browse/browse_test.go +++ b/caddyhttp/browse/browse_test.go @@ -1,6 +1,7 @@ package browse import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -362,3 +363,68 @@ func isReversed(data sort.Interface) bool { } return true } + +func TestBrowseRedirect(t *testing.T) { + testCases := []struct { + url string + statusCode int + returnCode int + location string + }{ + { + "http://www.example.com/photos", + http.StatusMovedPermanently, + 0, + "http://www.example.com/photos/", + }, + { + "/photos", + http.StatusMovedPermanently, + 0, + "/photos/", + }, + } + + for i, tc := range testCases { + b := Browse{ + Next: httpserver.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { + t.Fatalf("Test %d - Next shouldn't be called", i) + return 0, nil + }), + Configs: []Config{ + { + PathScope: "/photos", + Fs: staticfiles.FileServer{ + Root: http.Dir("./testdata"), + }, + }, + }, + } + + req, err := http.NewRequest("GET", tc.url, nil) + u, _ := url.Parse(tc.url) + ctx := context.WithValue(req.Context(), staticfiles.URLPathCtxKey, u.Path) + req = req.WithContext(ctx) + if err != nil { + t.Fatalf("Test %d - could not create HTTP request: %v", i, err) + } + + rec := httptest.NewRecorder() + + returnCode, _ := b.ServeHTTP(rec, req) + if returnCode != tc.returnCode { + t.Fatalf("Test %d - wrong return code, expected %d, got %d", + i, tc.returnCode, returnCode) + } + + if got := rec.Code; got != tc.statusCode { + t.Errorf("Test %d - wrong status, expected %d, got %d", + i, tc.statusCode, got) + } + + if got := rec.Header().Get("Location"); got != tc.location { + t.Errorf("Test %d - wrong Location header, expected %s, got %s", + i, tc.location, got) + } + } +} diff --git a/caddyhttp/staticfiles/fileserver.go b/caddyhttp/staticfiles/fileserver.go index 091090fc..b8bbc73b 100644 --- a/caddyhttp/staticfiles/fileserver.go +++ b/caddyhttp/staticfiles/fileserver.go @@ -96,30 +96,14 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri // Ensure / at end of directory url. If the original URL path is // used then ensure / exists as well. if !strings.HasSuffix(r.URL.Path, "/") { - toURL, _ := url.Parse(r.URL.String()) - - path, ok := r.Context().Value(URLPathCtxKey).(string) - if ok && !strings.HasSuffix(path, "/") { - toURL.Path = path - } - toURL.Path += "/" - - http.Redirect(w, r, toURL.String(), http.StatusMovedPermanently) + RedirectToDir(w, r) return http.StatusMovedPermanently, nil } } else { // Ensure no / at end of file url. If the original URL path is // used then ensure no / exists as well. if strings.HasSuffix(r.URL.Path, "/") { - toURL, _ := url.Parse(r.URL.String()) - - path, ok := r.Context().Value(URLPathCtxKey).(string) - if ok && strings.HasSuffix(path, "/") { - toURL.Path = path - } - toURL.Path = strings.TrimSuffix(toURL.Path, "/") - - http.Redirect(w, r, toURL.String(), http.StatusMovedPermanently) + RedirectToFile(w, r) return http.StatusMovedPermanently, nil } } @@ -234,14 +218,34 @@ func (fs FileServer) IsHidden(d os.FileInfo) bool { return false } -// Redirect sends an HTTP redirect to the client but will preserve -// the query string for the new path. Based on http.localRedirect -// from the Go standard library. -func Redirect(w http.ResponseWriter, r *http.Request, newPath string, statusCode int) { - if q := r.URL.RawQuery; q != "" { - newPath += "?" + q +// RedirectToDir replies to the request with a redirect to the URL in r, which +// has been transformed to indicate that the resource being requested is a +// directory. +func RedirectToDir(w http.ResponseWriter, r *http.Request) { + toURL, _ := url.Parse(r.URL.String()) + + path, ok := r.Context().Value(URLPathCtxKey).(string) + if ok && !strings.HasSuffix(path, "/") { + toURL.Path = path } - http.Redirect(w, r, newPath, statusCode) + toURL.Path += "/" + + http.Redirect(w, r, toURL.String(), http.StatusMovedPermanently) +} + +// RedirectToFile replies to the request with a redirect to the URL in r, which +// has been transformed to indicate that the resource being requested is a +// file. +func RedirectToFile(w http.ResponseWriter, r *http.Request) { + toURL, _ := url.Parse(r.URL.String()) + + path, ok := r.Context().Value(URLPathCtxKey).(string) + if ok && strings.HasSuffix(path, "/") { + toURL.Path = path + } + toURL.Path = strings.TrimSuffix(toURL.Path, "/") + + http.Redirect(w, r, toURL.String(), http.StatusMovedPermanently) } // IndexPages is a list of pages that may be understood as