Merge pull request #759 from mholt/paths-cleanup

Tell usage of 'path' from 'filepath' and fix *path checking

caddy/restart.go

@@ -11,7 +11,7 @@ import (
 	"net"
 	"os"
 	"os/exec"
-	"path"
+	"path/filepath"
 	"sync/atomic"
 
 	"github.com/mholt/caddy/caddy/https"
@@ -138,7 +138,7 @@ func Restart(newCaddyfile Input) error {
 func getCertsForNewCaddyfile(newCaddyfile Input) error {
 	// parse the new caddyfile only up to (and including) TLS
 	// so we can know what we need to get certs for.
-	configs, _, _, err := loadConfigsUpToIncludingTLS(path.Base(newCaddyfile.Path()), bytes.NewReader(newCaddyfile.Body()))
+	configs, _, _, err := loadConfigsUpToIncludingTLS(filepath.Base(newCaddyfile.Path()), bytes.NewReader(newCaddyfile.Body()))
 	if err != nil {
 		return errors.New("loading Caddyfile: " + err.Error())
 	}

caddy/setup/ext.go

@@ -2,6 +2,7 @@ package setup
 
 import (
 	"os"
+	"path/filepath"
 
 	"github.com/mholt/caddy/middleware"
 	"github.com/mholt/caddy/middleware/extensions"
@@ -47,7 +48,7 @@ func extParse(c *Controller) ([]string, error) {
 // resourceExists returns true if the file specified at
 // root + path exists; false otherwise.
 func resourceExists(root, path string) bool {
-	_, err := os.Stat(root + path)
+	_, err := os.Stat(filepath.Join(root, path))
 	// technically we should use os.IsNotExist(err)
 	// but we don't handle any other kinds of errors anyway
 	return err == nil

middleware/browse/browse.go

@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"os"
 	"path"
+	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
@@ -173,9 +174,11 @@ func (l Listing) applySort() {
 }
 
 func directoryListing(files []os.FileInfo, r *http.Request, canGoUp bool, root string, ignoreIndexes bool, vars interface{}) (Listing, error) {
-	var fileinfos []FileInfo
-	var dirCount, fileCount int
-	var urlPath = r.URL.Path
+	var (
+		fileinfos           []FileInfo
+		dirCount, fileCount int
+		urlPath             = r.URL.Path
+	)
 
 	for _, f := range files {
 		name := f.Name()
@@ -226,143 +229,150 @@ func directoryListing(files []os.FileInfo, r *http.Request, canGoUp bool, root s
 
 // ServeHTTP implements the middleware.Handler interface.
 func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
-	filename := b.Root + r.URL.Path
-	info, err := os.Stat(filename)
-	if err != nil {
+	var bc *Config
+	// See if there's a browse configuration to match the path
+	for i := range b.Configs {
+		if middleware.Path(r.URL.Path).Matches(b.Configs[i].PathScope) {
+			bc = &b.Configs[i]
+			break
+		}
+	}
+	if bc == nil {
 		return b.Next.ServeHTTP(w, r)
 	}
 
+	// Browse works on existing directories; delegate everything else
+	requestedFilepath := filepath.Join(b.Root, r.URL.Path)
+	info, err := os.Stat(requestedFilepath)
+	if err != nil {
+		switch {
+		case os.IsPermission(err):
+			return http.StatusForbidden, err
+		case os.IsExist(err):
+			return http.StatusNotFound, err
+		default:
+			return b.Next.ServeHTTP(w, r)
+		}
+	}
 	if !info.IsDir() {
 		return b.Next.ServeHTTP(w, r)
 	}
 
-	// See if there's a browse configuration to match the path
-	for _, bc := range b.Configs {
-		if !middleware.Path(r.URL.Path).Matches(bc.PathScope) {
-			continue
-		}
-		switch r.Method {
-		case http.MethodGet, http.MethodHead:
-		default:
-			return http.StatusMethodNotAllowed, nil
-		}
+	// Do not reply to anything else because it might be nonsensical
+	switch r.Method {
+	case http.MethodGet, http.MethodHead:
+	default:
+		return http.StatusMethodNotAllowed, nil
+	}
 
-		// Browsing navigation gets messed up if browsing a directory
-		// that doesn't end in "/" (which it should, anyway)
-		if r.URL.Path[len(r.URL.Path)-1] != '/' {
-			http.Redirect(w, r, r.URL.Path+"/", http.StatusTemporaryRedirect)
-			return 0, nil
-		}
+	// Browsing navigation gets messed up if browsing a directory
+	// that doesn't end in "/" (which it should, anyway)
+	if !strings.HasSuffix(r.URL.Path, "/") {
+		http.Redirect(w, r, r.URL.Path+"/", http.StatusTemporaryRedirect)
+		return 0, nil
+	}
 
-		// Load directory contents
-		file, err := os.Open(b.Root + r.URL.Path)
-		if err != nil {
-			if os.IsPermission(err) {
-				return http.StatusForbidden, err
-			}
-			return http.StatusNotFound, err
-		}
-		defer file.Close()
-
-		files, err := file.Readdir(-1)
-		if err != nil {
+	// Load directory contents
+	file, err := os.Open(requestedFilepath)
+	if err != nil {
+		if os.IsPermission(err) {
 			return http.StatusForbidden, err
 		}
+		return http.StatusInternalServerError, err
+	}
+	defer file.Close()
 
-		// Determine if user can browse up another folder
-		var canGoUp bool
-		curPath := strings.TrimSuffix(r.URL.Path, "/")
-		for _, other := range b.Configs {
-			if strings.HasPrefix(path.Dir(curPath), other.PathScope) {
-				canGoUp = true
-				break
+	files, err := file.Readdir(-1)
+	if err != nil {
+		if os.IsPermission(err) {
+			return http.StatusForbidden, err
+		}
+		return http.StatusInternalServerError, err
+	}
+
+	// Determine if user can browse up another folder
+	var canGoUp bool
+	curPath := strings.TrimSuffix(r.URL.Path, "/")
+	for _, other := range b.Configs {
+		if strings.HasPrefix(path.Dir(curPath), other.PathScope) {
+			canGoUp = true
+			break
+		}
+	}
+	// Assemble listing of directory contents
+	listing, err := directoryListing(files, r, canGoUp, b.Root, b.IgnoreIndexes, bc.Variables)
+	if err != nil { // directory isn't browsable
+		return http.StatusInternalServerError, err
+	}
+
+	// Copy the query values into the Listing struct
+	listing.Sort, listing.Order = r.URL.Query().Get("sort"), r.URL.Query().Get("order")
+
+	// If the query 'sort' or 'order' is empty, use defaults or any values previously saved in Cookies
+	if listing.Sort == "" {
+		listing.Sort = "name"
+		if sortCookie, sortErr := r.Cookie("sort"); sortErr == nil {
+			listing.Sort = sortCookie.Value
+		}
+	} else { // Save the query value of 'sort' and 'order' as cookies.
+		http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: bc.PathScope, Secure: r.TLS != nil})
+		http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: bc.PathScope, Secure: r.TLS != nil})
+	}
+
+	if listing.Order == "" {
+		listing.Order = "asc"
+		if orderCookie, orderErr := r.Cookie("order"); orderErr == nil {
+			listing.Order = orderCookie.Value
+		}
+	} else {
+		http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: bc.PathScope, Secure: r.TLS != nil})
+	}
+
+	listing.applySort()
+
+	var buf bytes.Buffer
+	// Check if we should provide json
+	acceptHeader := strings.Join(r.Header["Accept"], ",")
+	if strings.Contains(strings.ToLower(acceptHeader), "application/json") {
+		var marsh []byte
+		// Check if we are limited
+		if limitQuery := r.URL.Query().Get("limit"); limitQuery != "" {
+			limit, err := strconv.Atoi(limitQuery)
+			if err != nil { // if the 'limit' query can't be interpreted as a number, return err
+				return http.StatusBadRequest, err
 			}
-		}
-		// Assemble listing of directory contents
-		listing, err := directoryListing(files, r, canGoUp, b.Root, b.IgnoreIndexes, bc.Variables)
-		if err != nil { // directory isn't browsable
-			continue
-		}
-
-		// Get the query vales and store them in the Listing struct
-		listing.Sort, listing.Order = r.URL.Query().Get("sort"), r.URL.Query().Get("order")
-
-		// If the query 'sort' or 'order' is empty, check the cookies
-		if listing.Sort == "" {
-			sortCookie, sortErr := r.Cookie("sort")
-			// if there's no sorting values in the cookies, default to "name" and "asc"
-			if sortErr != nil {
-				listing.Sort = "name"
-			} else { // if we have values in the cookies, use them
-				listing.Sort = sortCookie.Value
-			}
-		} else { // save the query value of 'sort' and 'order' as cookies
-			http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: "/"})
-			http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"})
-		}
-
-		if listing.Order == "" {
-			orderCookie, orderErr := r.Cookie("order")
-			// if there's no sorting values in the cookies, default to "name" and "asc"
-			if orderErr != nil {
-				listing.Order = "asc"
-			} else { // if we have values in the cookies, use them
-				listing.Order = orderCookie.Value
-			}
-		} else { // save the query value of 'sort' and 'order' as cookies
-			http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"})
-		}
-
-		// Apply the sorting
-		listing.applySort()
-
-		var buf bytes.Buffer
-		// check if we should provide json
-		acceptHeader := strings.Join(r.Header["Accept"], ",")
-		if strings.Contains(strings.ToLower(acceptHeader), "application/json") {
-			var marsh []byte
-			// check if we are limited
-			if limitQuery := r.URL.Query().Get("limit"); limitQuery != "" {
-				limit, err := strconv.Atoi(limitQuery)
-				if err != nil { // if the 'limit' query can't be interpreted as a number, return err
-					return http.StatusBadRequest, err
-				}
-				// if `limit` is equal or less than len(listing.Items) and bigger than 0, list them
-				if limit <= len(listing.Items) && limit > 0 {
-					marsh, err = json.Marshal(listing.Items[:limit])
-				} else { // if the 'limit' query is empty, or has the wrong value, list everything
-					marsh, err = json.Marshal(listing.Items)
-				}
-				if err != nil {
-					return http.StatusInternalServerError, err
-				}
-			} else { // there's no 'limit' query, list them all
+			// if `limit` is equal or less than len(listing.Items) and bigger than 0, list them
+			if limit <= len(listing.Items) && limit > 0 {
+				marsh, err = json.Marshal(listing.Items[:limit])
+			} else { // if the 'limit' query is empty, or has the wrong value, list everything
 				marsh, err = json.Marshal(listing.Items)
-				if err != nil {
-					return http.StatusInternalServerError, err
-				}
 			}
-
-			// write the marshaled json to buf
-			if _, err = buf.Write(marsh); err != nil {
-				return http.StatusInternalServerError, err
-			}
-			w.Header().Set("Content-Type", "application/json; charset=utf-8")
-
-		} else { // there's no 'application/json' in the 'Accept' header, browse normally
-			err = bc.Template.Execute(&buf, listing)
 			if err != nil {
 				return http.StatusInternalServerError, err
 			}
-			w.Header().Set("Content-Type", "text/html; charset=utf-8")
-
+		} else { // There's no 'limit' query; list them all
+			marsh, err = json.Marshal(listing.Items)
+			if err != nil {
+				return http.StatusInternalServerError, err
+			}
 		}
 
-		buf.WriteTo(w)
+		// Write the marshaled json to buf
+		if _, err = buf.Write(marsh); err != nil {
+			return http.StatusInternalServerError, err
+		}
+		w.Header().Set("Content-Type", "application/json; charset=utf-8")
+
+	} else { // There's no 'application/json' in the 'Accept' header; browse normally
+		err = bc.Template.Execute(&buf, listing)
+		if err != nil {
+			return http.StatusInternalServerError, err
+		}
+		w.Header().Set("Content-Type", "text/html; charset=utf-8")
 
-		return http.StatusOK, nil
 	}
 
-	// Didn't qualify; pass-thru
-	return b.Next.ServeHTTP(w, r)
+	buf.WriteTo(w)
+
+	return http.StatusOK, nil
 }

middleware/fileserver.go

@@ -2,10 +2,12 @@ package middleware
 
 import (
 	"fmt"
+	"math/rand"
 	"net/http"
 	"os"
 	"path"
 	"path/filepath"
+	"strconv"
 	"strings"
 )
 
@@ -40,12 +42,11 @@ type fileHandler struct {
 }
 
 func (fh *fileHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
-	upath := r.URL.Path
-	if !strings.HasPrefix(upath, "/") {
-		upath = "/" + upath
-		r.URL.Path = upath
+	// r.URL.Path has already been cleaned in caddy/server by path.Clean().
+	if r.URL.Path == "" {
+		r.URL.Path = "/"
 	}
-	return fh.serveFile(w, r, path.Clean(upath))
+	return fh.serveFile(w, r, r.URL.Path)
 }
 
 // serveFile writes the specified file to the HTTP response.
@@ -66,7 +67,8 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st
 			return http.StatusForbidden, err
 		}
 		// Likely the server is under load and ran out of file descriptors
-		w.Header().Set("Retry-After", "5") // TODO: 5 seconds enough delay? Or too much?
+		backoff := int(3 + rand.Int31()%3) // 3–5 seconds to prevent a stampede
+		w.Header().Set("Retry-After", strconv.Itoa(backoff))
 		return http.StatusServiceUnavailable, err
 	}
 	defer f.Close()
@@ -86,13 +88,13 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st
 	url := r.URL.Path
 	if d.IsDir() {
 		// Ensure / at end of directory url
-		if url[len(url)-1] != '/' {
+		if !strings.HasSuffix(url, "/") {
 			redirect(w, r, path.Base(url)+"/")
 			return http.StatusMovedPermanently, nil
 		}
 	} else {
 		// Ensure no / at end of file url
-		if url[len(url)-1] == '/' {
+		if strings.HasSuffix(url, "/") {
 			redirect(w, r, "../"+path.Base(url))
 			return http.StatusMovedPermanently, nil
 		}

middleware/fileserver_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -11,23 +12,30 @@ import (
 	"time"
 )
 
-var testDir = filepath.Join(os.TempDir(), "caddy_testdir")
-var ErrCustom = errors.New("Custom Error")
+var (
+	ErrCustom = errors.New("Custom Error")
+
+	testDir     = filepath.Join(os.TempDir(), "caddy_testdir")
+	testWebRoot = filepath.Join(testDir, "webroot")
+)
 
 // testFiles is a map with relative paths to test files as keys and file content as values.
 // The map represents the following structure:
 // - $TEMP/caddy_testdir/
-// '-- file1.html
-// '-- dirwithindex/
-// '---- index.html
-// '-- dir/
-// '---- file2.html
-// '---- hidden.html
+// '-- unreachable.html
+// '-- webroot/
+// '---- file1.html
+// '---- dirwithindex/
+// '------ index.html
+// '---- dir/
+// '------ file2.html
+// '------ hidden.html
 var testFiles = map[string]string{
-	"file1.html":                                "<h1>file1.html</h1>",
-	filepath.Join("dirwithindex", "index.html"): "<h1>dirwithindex/index.html</h1>",
-	filepath.Join("dir", "file2.html"):          "<h1>dir/file2.html</h1>",
-	filepath.Join("dir", "hidden.html"):         "<h1>dir/hidden.html</h1>",
+	"unreachable.html":                                     "<h1>must not leak</h1>",
+	filepath.Join("webroot", "file1.html"):                 "<h1>file1.html</h1>",
+	filepath.Join("webroot", "dirwithindex", "index.html"): "<h1>dirwithindex/index.html</h1>",
+	filepath.Join("webroot", "dir", "file2.html"):          "<h1>dir/file2.html</h1>",
+	filepath.Join("webroot", "dir", "hidden.html"):         "<h1>dir/hidden.html</h1>",
 }
 
 // TestServeHTTP covers positive scenarios when serving files.
@@ -36,7 +44,7 @@ func TestServeHTTP(t *testing.T) {
 	beforeServeHTTPTest(t)
 	defer afterServeHTTPTest(t)
 
-	fileserver := FileServer(http.Dir(testDir), []string{"dir/hidden.html"})
+	fileserver := FileServer(http.Dir(testWebRoot), []string{"dir/hidden.html"})
 
 	movedPermanently := "Moved Permanently"
 
@@ -142,11 +150,20 @@ func TestServeHTTP(t *testing.T) {
 			url:            "https://foo/hidden.html",
 			expectedStatus: http.StatusNotFound,
 		},
+		// Test 17 - try to get below the root directory.
+		{
+			url:            "https://foo/%2f..%2funreachable.html",
+			expectedStatus: http.StatusNotFound,
+		},
 	}
 
 	for i, test := range tests {
 		responseRecorder := httptest.NewRecorder()
-		request, err := http.NewRequest("GET", test.url, strings.NewReader(""))
+		request, err := http.NewRequest("GET", test.url, nil)
+		// prevent any URL sanitization within Go: we need unmodified paths here
+		if u, _ := url.Parse(test.url); u.RawPath != "" {
+			request.URL.Path = u.RawPath
+		}
 		status, err := fileserver.ServeHTTP(responseRecorder, request)
 		etag := responseRecorder.Header().Get("Etag")
 
@@ -176,7 +193,7 @@ func TestServeHTTP(t *testing.T) {
 // beforeServeHTTPTest creates a test directory with the structure, defined in the variable testFiles
 func beforeServeHTTPTest(t *testing.T) {
 	// make the root test dir
-	err := os.Mkdir(testDir, os.ModePerm)
+	err := os.MkdirAll(testWebRoot, os.ModePerm)
 	if err != nil {
 		if !os.IsExist(err) {
 			t.Fatalf("Failed to create test dir. Error was: %v", err)

server/server.go

@@ -14,7 +14,7 @@ import (
 	"net"
 	"net/http"
 	"os"
-	"path/filepath"
+	"path"
 	"runtime"
 	"strings"
 	"sync"
@@ -336,11 +336,18 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// Use URL.RawPath If you need the original, "raw" URL.Path in your middleware.
 	// Collapse any ./ ../ /// madness here instead of doing that in every plugin.
 	if r.URL.Path != "/" {
-		path := filepath.Clean(r.URL.Path)
-		if !strings.HasPrefix(path, "/") {
-			path = "/" + path
+		cleanedPath := path.Clean(r.URL.Path)
+		if cleanedPath == "." {
+			r.URL.Path = "/"
+		} else {
+			if !strings.HasPrefix(cleanedPath, "/") {
+				cleanedPath = "/" + cleanedPath
+			}
+			if strings.HasSuffix(r.URL.Path, "/") && !strings.HasSuffix(cleanedPath, "/") {
+				cleanedPath = cleanedPath + "/"
+			}
+			r.URL.Path = cleanedPath
 		}
-		r.URL.Path = path
 	}
 
 	// Execute the optional request callback if it exists and it's not disabled