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

@@ -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
 	}
 

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)
+		}
+	}
+}

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())
+			RedirectToDir(w, r)
-
-			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)
 			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())
+			RedirectToFile(w, r)
-
-			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)
 			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
+// RedirectToDir replies to the request with a redirect to the URL in r, which
-// the query string for the new path. Based on http.localRedirect
+// has been transformed to indicate that the resource being requested is a
-// from the Go standard library.
+// directory.
-func Redirect(w http.ResponseWriter, r *http.Request, newPath string, statusCode int) {
+func RedirectToDir(w http.ResponseWriter, r *http.Request) {
-	if q := r.URL.RawQuery; q != "" {
+	toURL, _ := url.Parse(r.URL.String())
-		newPath += "?" + q
+
+	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