httpserver/all: Clean up and standardize request URL handling (#1633)

* httpserver/all: Clean up and standardize request URL handling

The HTTP server now always creates a context value on the request which
is a copy of the request's URL struct. It should not be modified by
middlewares, but it is safe to get the value out of the request and make
changes to it locally-scoped. Thus, the value in the context always
stores the original request URL information as it was received. Any
rewrites that happen will be to the request's URL field directly.

The HTTP server no longer cleans /sanitizes the request URL. It made too
many strong assumptions and ended up making a lot of middleware more
complicated, including upstream proxying (and fastcgi). To alleviate
this complexity, we no longer change the request URL. Middlewares are
responsible to access the disk safely by using http.Dir or, if not
actually opening files, they can use httpserver.SafePath().

I'm hoping this will address issues with #1624, #1584, #1582, and others.

* staticfiles: Fix test on Windows

@abiosoft: I still can't figure out exactly what this is for. 😅

* Use (potentially) changed URL for browse redirects, as before

* Use filepath.ToSlash, clean up a couple proxy test cases

* Oops, fix variable name

caddyhttp/browse/browse.go

@@ -333,9 +333,14 @@ 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.RedirectToDir(w, r)
-		return 0, nil
+	u := *r.URL
+	if u.Path == "" {
+		u.Path = "/"
+	}
+	if u.Path[len(u.Path)-1] != '/' {
+		u.Path += "/"
+		http.Redirect(w, r, u.String(), http.StatusMovedPermanently)
+		return http.StatusMovedPermanently, nil
 	}
 
 	return b.ServeListing(w, r, requestedFilepath, bc)

caddyhttp/browse/browse_test.go

@@ -142,6 +142,8 @@ func TestBrowseHTTPMethods(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Test: Could not create HTTP request: %v", err)
 		}
+		ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+		req = req.WithContext(ctx)
 
 		code, _ := b.ServeHTTP(rec, req)
 		if code != expected {
@@ -177,6 +179,8 @@ func TestBrowseTemplate(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Test: Could not create HTTP request: %v", err)
 	}
+	ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+	req = req.WithContext(ctx)
 
 	rec := httptest.NewRecorder()
 
@@ -322,6 +326,8 @@ func TestBrowseJson(t *testing.T) {
 		if err != nil && !test.shouldErr {
 			t.Errorf("Test %d errored when making request, but it shouldn't have; got '%v'", i, err)
 		}
+		ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+		req = req.WithContext(ctx)
 
 		req.Header.Set("Accept", "application/json")
 		rec := httptest.NewRecorder()
@@ -394,13 +400,13 @@ func TestBrowseRedirect(t *testing.T) {
 		{
 			"http://www.example.com/photos",
 			http.StatusMovedPermanently,
-			0,
+			http.StatusMovedPermanently,
 			"http://www.example.com/photos/",
 		},
 		{
 			"/photos",
 			http.StatusMovedPermanently,
-			0,
+			http.StatusMovedPermanently,
 			"/photos/",
 		},
 	}
@@ -422,12 +428,11 @@ func TestBrowseRedirect(t *testing.T) {
 		}
 
 		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)
 		}
+		ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+		req = req.WithContext(ctx)
 
 		rec := httptest.NewRecorder()
 

caddyhttp/browse/setup.go

@@ -65,7 +65,7 @@ func browseParse(c *caddy.Controller) ([]Config, error) {
 
 		bc.Fs = staticfiles.FileServer{
 			Root: http.Dir(cfg.Root),
-			Hide: httpserver.GetConfig(c).HiddenFiles,
+			Hide: cfg.HiddenFiles,
 		}
 
 		// Second argument would be the template file to use

caddyhttp/extensions/ext.go

@@ -31,9 +31,10 @@ type Ext struct {
 // ServeHTTP implements the httpserver.Handler interface.
 func (e Ext) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
 	urlpath := strings.TrimSuffix(r.URL.Path, "/")
-	if path.Ext(urlpath) == "" && len(r.URL.Path) > 0 && r.URL.Path[len(r.URL.Path)-1] != '/' {
+	if len(r.URL.Path) > 0 && path.Ext(urlpath) == "" && r.URL.Path[len(r.URL.Path)-1] != '/' {
 		for _, ext := range e.Extensions {
-			if resourceExists(e.Root, urlpath+ext) {
+			_, err := os.Stat(httpserver.SafePath(e.Root, urlpath) + ext)
+			if err == nil {
 				r.URL.Path = urlpath + ext
 				break
 			}
@@ -41,12 +42,3 @@ func (e Ext) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
 	}
 	return e.Next.ServeHTTP(w, r)
 }
-
-// resourceExists returns true if the file specified at
-// root + path exists; false otherwise.
-func resourceExists(root, path string) bool {
-	_, err := os.Stat(root + path)
-	// technically we should use os.IsNotExist(err)
-	// but we don't handle any other kinds of errors anyway
-	return err == nil
-}

caddyhttp/fastcgi/fastcgi.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"net/url"
 	"os"
 	"path"
 	"path/filepath"
@@ -213,23 +214,19 @@ func (h Handler) buildEnv(r *http.Request, rule Rule, fpath string) (map[string]
 	// Strip PATH_INFO from SCRIPT_NAME
 	scriptName = strings.TrimSuffix(scriptName, pathInfo)
 
-	// Get the request URI from context. The request URI might be as it came in over the wire,
-	// or it might have been rewritten internally by the rewrite middleware (see issue #256).
-	// If it was rewritten, there will be a context value with the original URL,
-	// which is needed to get the correct RequestURI value for PHP apps.
-	reqURI := r.URL.RequestURI()
-	if origURI, _ := r.Context().Value(httpserver.URIxRewriteCtxKey).(string); origURI != "" {
-		reqURI = origURI
-	}
+	// Get the request URI from context. The context stores the original URI in case
+	// it was changed by a middleware such as rewrite. By default, we pass the
+	// original URI in as the value of REQUEST_URI (the user can overwrite this
+	// if desired). Most PHP apps seem to want the original URI. Besides, this is
+	// how nginx defaults: http://stackoverflow.com/a/12485156/1048862
+	reqURL, _ := r.Context().Value(httpserver.OriginalURLCtxKey).(url.URL)
 
-	// Retrieve name of remote user that was set by some downstream middleware,
-	// possibly basicauth.
-	remoteUser, _ := r.Context().Value(httpserver.RemoteUserCtxKey).(string) // Blank if not set
+	// Retrieve name of remote user that was set by some downstream middleware such as basicauth.
+	remoteUser, _ := r.Context().Value(httpserver.RemoteUserCtxKey).(string)
 
 	// Some variables are unused but cleared explicitly to prevent
 	// the parent environment from interfering.
 	env = map[string]string{
-
 		// Variables defined in CGI 1.1 spec
 		"AUTH_TYPE":         "", // Not used
 		"CONTENT_LENGTH":    r.Header.Get("Content-Length"),
@@ -252,13 +249,13 @@ func (h Handler) buildEnv(r *http.Request, rule Rule, fpath string) (map[string]
 		"DOCUMENT_ROOT":   rule.Root,
 		"DOCUMENT_URI":    docURI,
 		"HTTP_HOST":       r.Host, // added here, since not always part of headers
-		"REQUEST_URI":     reqURI,
+		"REQUEST_URI":     reqURL.RequestURI(),
 		"SCRIPT_FILENAME": scriptFilename,
 		"SCRIPT_NAME":     scriptName,
 	}
 
-	// compliance with the CGI specification that PATH_TRANSLATED
-	// should only exist if PATH_INFO is defined.
+	// compliance with the CGI specification requires that
+	// PATH_TRANSLATED should only exist if PATH_INFO is defined.
 	// Info: https://www.ietf.org/rfc/rfc3875 Page 14
 	if env["PATH_INFO"] != "" {
 		env["PATH_TRANSLATED"] = filepath.Join(rule.Root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
@@ -269,10 +266,9 @@ func (h Handler) buildEnv(r *http.Request, rule Rule, fpath string) (map[string]
 		env["HTTPS"] = "on"
 	}
 
+	// Add env variables from config (with support for placeholders in values)
 	replacer := httpserver.NewReplacer(r, nil, "")
-	// Add env variables from config
 	for _, envVar := range rule.EnvVars {
-		// replace request placeholders in environment variables
 		env[envVar[0]] = replacer.Replace(envVar[1])
 	}
 

caddyhttp/fastcgi/fastcgi_test.go

 package fastcgi
 
 import (
+	"context"
 	"net"
 	"net/http"
 	"net/http/fcgi"
@@ -10,6 +11,8 @@ import (
 	"sync"
 	"testing"
 	"time"
+
+	"github.com/mholt/caddy/caddyhttp/httpserver"
 )
 
 func TestServeHTTP(t *testing.T) {
@@ -130,6 +133,8 @@ func TestPersistent(t *testing.T) {
 					if err != nil {
 						t.Errorf("Unable to create request: %v", err)
 					}
+					ctx := context.WithValue(r.Context(), httpserver.OriginalURLCtxKey, *r.URL)
+					r = r.WithContext(ctx)
 					w := httptest.NewRecorder()
 
 					status, err := handler.ServeHTTP(w, r)
@@ -222,13 +227,13 @@ func TestBuildEnv(t *testing.T) {
 	}
 
 	rule := Rule{}
-	url, err := url.Parse("http://localhost:2015/fgci_test.php?test=blabla")
+	url, err := url.Parse("http://localhost:2015/fgci_test.php?test=foobar")
 	if err != nil {
 		t.Error("Unexpected error:", err.Error())
 	}
 
 	var newReq = func() *http.Request {
-		return &http.Request{
+		r := http.Request{
 			Method:     "GET",
 			URL:        url,
 			Proto:      "HTTP/1.1",
@@ -241,6 +246,8 @@ func TestBuildEnv(t *testing.T) {
 				"Foo": {"Bar", "two"},
 			},
 		}
+		ctx := context.WithValue(r.Context(), httpserver.OriginalURLCtxKey, *r.URL)
+		return r.WithContext(ctx)
 	}
 
 	fpath := "/fgci_test.php"
@@ -250,7 +257,7 @@ func TestBuildEnv(t *testing.T) {
 			"REMOTE_ADDR":     "2b02:1810:4f2d:9400:70ab:f822:be8a:9093",
 			"REMOTE_PORT":     "51688",
 			"SERVER_PROTOCOL": "HTTP/1.1",
-			"QUERY_STRING":    "test=blabla",
+			"QUERY_STRING":    "test=foobar",
 			"REQUEST_METHOD":  "GET",
 			"HTTP_HOST":       "localhost:2015",
 		}
@@ -300,8 +307,8 @@ func TestBuildEnv(t *testing.T) {
 	}
 	envExpected = newEnv()
 	envExpected["HTTP_HOST"] = "localhost:2015"
-	envExpected["CUSTOM_URI"] = "custom_uri/fgci_test.php?test=blabla"
-	envExpected["CUSTOM_QUERY"] = "custom=true&test=blabla"
+	envExpected["CUSTOM_URI"] = "custom_uri/fgci_test.php?test=foobar"
+	envExpected["CUSTOM_QUERY"] = "custom=true&test=foobar"
 	testBuildEnv(r, rule, fpath, envExpected)
 }
 

caddyhttp/httpserver/condition_test.go

 package httpserver
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"strings"
@@ -94,16 +95,21 @@ func TestConditions(t *testing.T) {
 	for i, test := range replaceTests {
 		r, err := http.NewRequest("GET", test.url, nil)
 		if err != nil {
-			t.Error(err)
+			t.Errorf("Test %d: failed to create request: %v", i, err)
+			continue
 		}
+		ctx := context.WithValue(r.Context(), OriginalURLCtxKey, *r.URL)
+		r = r.WithContext(ctx)
 		str := strings.Fields(test.condition)
 		ifCond, err := newIfCond(str[0], str[1], str[2])
 		if err != nil {
-			t.Error(err)
+			t.Errorf("Test %d: failed to create 'if' condition %v", i, err)
+			continue
 		}
 		isTrue := ifCond.True(r)
 		if isTrue != test.isTrue {
 			t.Errorf("Test %v: expected %v found %v", i, test.isTrue, isTrue)
+			continue
 		}
 	}
 }

caddyhttp/httpserver/middleware.go

@@ -198,15 +198,11 @@ func SameNext(next1, next2 Handler) bool {
 	return fmt.Sprintf("%v", next1) == fmt.Sprintf("%v", next2)
 }
 
-// Context key constants
+// Context key constants.
 const (
-	// URIxRewriteCtxKey is a context key used to store original unrewritten
-	// URI in context.WithValue
-	URIxRewriteCtxKey caddy.CtxKey = "caddy_rewrite_original_uri"
-
-	// RemoteUserCtxKey is a context key used to store remote user for request
+	// RemoteUserCtxKey is the key for the remote user of the request, if any (basicauth).
 	RemoteUserCtxKey caddy.CtxKey = "remote_user"
 
-	// MitmCtxKey stores Mitm result
+	// MitmCtxKey is the key for the result of MITM detection
 	MitmCtxKey caddy.CtxKey = "mitm"
 )

caddyhttp/httpserver/pathcleaner.go

-package httpserver
-
-import (
-	"math/rand"
-	"path"
-	"strings"
-	"time"
-)
-
-// CleanMaskedPath prevents one or more of the path cleanup operations:
-//   - collapse multiple slashes into one
-//   - eliminate "/." (current directory)
-//   - eliminate "<parent_directory>/.."
-// by masking certain patterns in the path with a temporary random string.
-// This could be helpful when certain patterns in the path are desired to be preserved
-// that would otherwise be changed by path.Clean().
-// One such use case is the presence of the double slashes as protocol separator
-// (e.g., /api/endpoint/http://example.com).
-// This is a common pattern in many applications to allow passing URIs as path argument.
-func CleanMaskedPath(reqPath string, masks ...string) string {
-	var replacerVal string
-	maskMap := make(map[string]string)
-
-	// Iterate over supplied masks and create temporary replacement strings
-	// only for the masks that are present in the path, then replace all occurrences
-	for _, mask := range masks {
-		if strings.Index(reqPath, mask) >= 0 {
-			replacerVal = "/_caddy" + generateRandomString() + "__"
-			maskMap[mask] = replacerVal
-			reqPath = strings.Replace(reqPath, mask, replacerVal, -1)
-		}
-	}
-
-	reqPath = path.Clean(reqPath)
-
-	// Revert the replaced masks after path cleanup
-	for mask, replacerVal := range maskMap {
-		reqPath = strings.Replace(reqPath, replacerVal, mask, -1)
-	}
-	return reqPath
-}
-
-// CleanPath calls CleanMaskedPath() with the default mask of "://"
-// to preserve double slashes of protocols
-// such as "http://", "https://", and "ftp://" etc.
-func CleanPath(reqPath string) string {
-	return CleanMaskedPath(reqPath, "://")
-}
-
-// An efficient and fast method for random string generation.
-// Inspired by http://stackoverflow.com/a/31832326.
-const randomStringLength = 4
-const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
-const (
-	letterIdxBits = 6
-	letterIdxMask = 1<<letterIdxBits - 1
-	letterIdxMax  = 63 / letterIdxBits
-)
-
-var src = rand.NewSource(time.Now().UnixNano())
-
-func generateRandomString() string {
-	b := make([]byte, randomStringLength)
-	for i, cache, remain := randomStringLength-1, src.Int63(), letterIdxMax; i >= 0; {
-		if remain == 0 {
-			cache, remain = src.Int63(), letterIdxMax
-		}
-		if idx := int(cache & letterIdxMask); idx < len(letterBytes) {
-			b[i] = letterBytes[idx]
-			i--
-		}
-		cache >>= letterIdxBits
-		remain--
-	}
-	return string(b)
-}

caddyhttp/httpserver/pathcleaner_test.go

-package httpserver
-
-import (
-	"path"
-	"testing"
-)
-
-var paths = map[string]map[string]string{
-	"/../a/b/../././/c": {
-		"preserve_all":      "/../a/b/../././/c",
-		"preserve_protocol": "/a/c",
-		"preserve_slashes":  "/a//c",
-		"preserve_dots":     "/../a/b/../././c",
-		"clean_all":         "/a/c",
-	},
-	"/path/https://www.google.com": {
-		"preserve_all":      "/path/https://www.google.com",
-		"preserve_protocol": "/path/https://www.google.com",
-		"preserve_slashes":  "/path/https://www.google.com",
-		"preserve_dots":     "/path/https:/www.google.com",
-		"clean_all":         "/path/https:/www.google.com",
-	},
-	"/a/b/../././/c/http://example.com/foo//bar/../blah": {
-		"preserve_all":      "/a/b/../././/c/http://example.com/foo//bar/../blah",
-		"preserve_protocol": "/a/c/http://example.com/foo/blah",
-		"preserve_slashes":  "/a//c/http://example.com/foo/blah",
-		"preserve_dots":     "/a/b/../././c/http:/example.com/foo/bar/../blah",
-		"clean_all":         "/a/c/http:/example.com/foo/blah",
-	},
-}
-
-func assertEqual(t *testing.T, expected, received string) {
-	if expected != received {
-		t.Errorf("\tExpected: %s\n\t\t\tReceived: %s", expected, received)
-	}
-}
-
-func maskedTestRunner(t *testing.T, variation string, masks ...string) {
-	for reqPath, transformation := range paths {
-		assertEqual(t, transformation[variation], CleanMaskedPath(reqPath, masks...))
-	}
-}
-
-// No need to test the built-in path.Clean() function.
-// However, it could be useful to cross-examine the test dataset.
-func TestPathClean(t *testing.T) {
-	for reqPath, transformation := range paths {
-		assertEqual(t, transformation["clean_all"], path.Clean(reqPath))
-	}
-}
-
-func TestCleanAll(t *testing.T) {
-	maskedTestRunner(t, "clean_all")
-}
-
-func TestPreserveAll(t *testing.T) {
-	maskedTestRunner(t, "preserve_all", "//", "/..", "/.")
-}
-
-func TestPreserveProtocol(t *testing.T) {
-	maskedTestRunner(t, "preserve_protocol", "://")
-}
-
-func TestPreserveSlashes(t *testing.T) {
-	maskedTestRunner(t, "preserve_slashes", "//")
-}
-
-func TestPreserveDots(t *testing.T) {
-	maskedTestRunner(t, "preserve_dots", "/..", "/.")
-}
-
-func TestDefaultMask(t *testing.T) {
-	for reqPath, transformation := range paths {
-		assertEqual(t, transformation["preserve_protocol"], CleanPath(reqPath))
-	}
-}
-
-func maskedBenchmarkRunner(b *testing.B, masks ...string) {
-	for n := 0; n < b.N; n++ {
-		for reqPath := range paths {
-			CleanMaskedPath(reqPath, masks...)
-		}
-	}
-}
-
-func BenchmarkPathClean(b *testing.B) {
-	for n := 0; n < b.N; n++ {
-		for reqPath := range paths {
-			path.Clean(reqPath)
-		}
-	}
-}
-
-func BenchmarkCleanAll(b *testing.B) {
-	maskedBenchmarkRunner(b)
-}
-
-func BenchmarkPreserveAll(b *testing.B) {
-	maskedBenchmarkRunner(b, "//", "/..", "/.")
-}
-
-func BenchmarkPreserveProtocol(b *testing.B) {
-	maskedBenchmarkRunner(b, "://")
-}
-
-func BenchmarkPreserveSlashes(b *testing.B) {
-	maskedBenchmarkRunner(b, "//")
-}
-
-func BenchmarkPreserveDots(b *testing.B) {
-	maskedBenchmarkRunner(b, "/..", "/.")
-}
-
-func BenchmarkDefaultMask(b *testing.B) {
-	for n := 0; n < b.N; n++ {
-		for reqPath := range paths {
-			CleanPath(reqPath)
-		}
-	}
-}

caddyhttp/httpserver/replacer.go

@@ -238,37 +238,24 @@ func (r *replacer) getSubstitution(key string) string {
 		}
 		return host
 	case "{path}":
-		// if a rewrite has happened, the original URI should be used as the path
-		// rather than the rewritten URI
-		var path string
-		origpath, _ := r.request.Context().Value(URIxRewriteCtxKey).(string)
-		if origpath == "" {
-			path = r.request.URL.Path
-		} else {
-			parsedURL, _ := url.Parse(origpath)
-			path = parsedURL.Path
-		}
-		return path
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return u.Path
 	case "{path_escaped}":
-		var path string
-		origpath, _ := r.request.Context().Value(URIxRewriteCtxKey).(string)
-		if origpath == "" {
-			path = r.request.URL.Path
-		} else {
-			parsedURL, _ := url.Parse(origpath)
-			path = parsedURL.Path
-		}
-		return url.QueryEscape(path)
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return url.QueryEscape(u.Path)
 	case "{rewrite_path}":
 		return r.request.URL.Path
 	case "{rewrite_path_escaped}":
 		return url.QueryEscape(r.request.URL.Path)
 	case "{query}":
-		return r.request.URL.RawQuery
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return u.RawQuery
 	case "{query_escaped}":
-		return url.QueryEscape(r.request.URL.RawQuery)
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return url.QueryEscape(u.RawQuery)
 	case "{fragment}":
-		return r.request.URL.Fragment
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return u.Fragment
 	case "{proto}":
 		return r.request.Proto
 	case "{remote}":
@@ -284,17 +271,11 @@ func (r *replacer) getSubstitution(key string) string {
 		}
 		return port
 	case "{uri}":
-		uri, _ := r.request.Context().Value(URIxRewriteCtxKey).(string)
-		if uri == "" {
-			uri = r.request.URL.RequestURI()
-		}
-		return uri
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return u.RequestURI()
 	case "{uri_escaped}":
-		uri, _ := r.request.Context().Value(URIxRewriteCtxKey).(string)
-		if uri == "" {
-			uri = r.request.URL.RequestURI()
-		}
-		return url.QueryEscape(uri)
+		u, _ := r.request.Context().Value(OriginalURLCtxKey).(url.URL)
+		return url.QueryEscape(u.RequestURI())
 	case "{rewrite_uri}":
 		return r.request.URL.RequestURI()
 	case "{rewrite_uri_escaped}":

caddyhttp/httpserver/replacer_test.go

@@ -41,8 +41,11 @@ func TestReplace(t *testing.T) {
 
 	request, err := http.NewRequest("POST", "http://localhost/?foo=bar", reader)
 	if err != nil {
-		t.Fatal("Request Formation Failed\n")
+		t.Fatalf("Failed to make request: %v", err)
 	}
+	ctx := context.WithValue(request.Context(), OriginalURLCtxKey, *request.URL)
+	request = request.WithContext(ctx)
+
 	request.Header.Set("Custom", "foobarbaz")
 	request.Header.Set("ShorterVal", "1")
 	repl := NewReplacer(request, recordRequest, "-")
@@ -52,7 +55,7 @@ func TestReplace(t *testing.T) {
 
 	hostname, err := os.Hostname()
 	if err != nil {
-		t.Fatal("Failed to determine hostname\n")
+		t.Fatalf("Failed to determine hostname: %v", err)
 	}
 
 	old := now
@@ -161,25 +164,26 @@ func TestPathRewrite(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Request Formation Failed: %s\n", err.Error())
 	}
-
-	ctx := context.WithValue(request.Context(), URIxRewriteCtxKey, "a/custom/path.php?key=value")
+	urlCopy := *request.URL
+	urlCopy.Path = "a/custom/path.php"
+	ctx := context.WithValue(request.Context(), OriginalURLCtxKey, urlCopy)
 	request = request.WithContext(ctx)
 
 	repl := NewReplacer(request, recordRequest, "")
 
-	if repl.Replace("This path is '{path}'") != "This path is 'a/custom/path.php'" {
-		t.Error("Expected host {path} replacement failed  (" + repl.Replace("This path is '{path}'") + ")")
+	if got, want := repl.Replace("This path is '{path}'"), "This path is 'a/custom/path.php'"; got != want {
+		t.Errorf("{path} replacement failed; got '%s', want '%s'", got, want)
 	}
 
-	if repl.Replace("This path is {rewrite_path}") != "This path is /index.php" {
-		t.Error("Expected host {rewrite_path} replacement failed (" + repl.Replace("This path is {rewrite_path}") + ")")
+	if got, want := repl.Replace("This path is {rewrite_path}"), "This path is /index.php"; got != want {
+		t.Errorf("{rewrite_path} replacement failed; got '%s', want '%s'", got, want)
 	}
-	if repl.Replace("This path is '{uri}'") != "This path is 'a/custom/path.php?key=value'" {
-		t.Error("Expected host {uri} replacement failed  (" + repl.Replace("This path is '{uri}'") + ")")
+	if got, want := repl.Replace("This path is '{uri}'"), "This path is 'a/custom/path.php?key=value'"; got != want {
+		t.Errorf("{uri} replacement failed; got '%s', want '%s'", got, want)
 	}
 
-	if repl.Replace("This path is {rewrite_uri}") != "This path is /index.php?key=value" {
-		t.Error("Expected host {rewrite_uri} replacement failed (" + repl.Replace("This path is {rewrite_uri}") + ")")
+	if got, want := repl.Replace("This path is {rewrite_uri}"), "This path is /index.php?key=value"; got != want {
+		t.Errorf("{rewrite_uri} replacement failed; got '%s', want '%s'", got, want)
 	}
 
 }

caddyhttp/httpserver/server.go

@@ -9,7 +9,10 @@ import (
 	"log"
 	"net"
 	"net/http"
+	"net/url"
 	"os"
+	"path"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
@@ -290,11 +293,18 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}()
 
-	w.Header().Set("Server", "Caddy")
-	c := context.WithValue(r.Context(), staticfiles.URLPathCtxKey, r.URL.Path)
+	// copy the original, unchanged URL into the context
+	// so it can be referenced by middlewares
+	urlCopy := *r.URL
+	if r.URL.User != nil {
+		userInfo := new(url.Userinfo)
+		*userInfo = *r.URL.User
+		urlCopy.User = userInfo
+	}
+	c := context.WithValue(r.Context(), OriginalURLCtxKey, urlCopy)
 	r = r.WithContext(c)
 
-	sanitizePath(r)
+	w.Header().Set("Server", "Caddy")
 
 	status, _ := s.serveHTTP(w, r)
 
@@ -353,6 +363,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error)
 	// The error returned by MaxBytesReader is meant to be handled
 	// by whichever middleware/plugin that receives it when calling
 	// .Read() or a similar method on the request body
+	// TODO: Make this middleware instead?
 	if r.Body != nil {
 		for _, pathlimit := range vhost.MaxRequestBodySizes {
 			if Path(r.URL.Path).Matches(pathlimit.Path) {
@@ -407,28 +418,6 @@ func (s *Server) Stop() error {
 	return nil
 }
 
-// sanitizePath collapses any ./ ../ /// madness which helps prevent
-// path traversal attacks. Note to middleware: use the value within the
-// request's context at key caddy.URLPathContextKey to access the
-// "original" URL.Path value.
-func sanitizePath(r *http.Request) {
-	if r.URL.Path == "/" {
-		return
-	}
-	cleanedPath := CleanPath(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
-	}
-}
-
 // OnStartupComplete lists the sites served by this server
 // and any relevant information, assuming caddy.Quiet == false.
 func (s *Server) OnStartupComplete() {
@@ -558,3 +547,20 @@ func WriteTextResponse(w http.ResponseWriter, status int, body string) {
 	w.WriteHeader(status)
 	w.Write([]byte(body))
 }
+
+// SafePath joins siteRoot and reqPath and converts it to a path that can
+// be used to access a path on the local disk. It ensures the path does
+// not traverse outside of the site root.
+//
+// If opening a file, use http.Dir instead.
+func SafePath(siteRoot, reqPath string) string {
+	reqPath = filepath.ToSlash(reqPath)
+	reqPath = strings.Replace(reqPath, "\x00", "", -1) // NOTE: Go 1.9 checks for null bytes in the syscall package
+	if siteRoot == "" {
+		siteRoot = "."
+	}
+	return filepath.Join(siteRoot, filepath.FromSlash(path.Clean("/"+reqPath)))
+}
+
+// OriginalURLCtxKey is the key for accessing the original, incoming URL on an HTTP request.
+const OriginalURLCtxKey = caddy.CtxKey("original_url")

caddyhttp/proxy/proxy_test.go

@@ -895,7 +895,6 @@ func basicAuthTestcase(t *testing.T, upstreamUser, clientUser *url.Userinfo) {
 
 func TestProxyDirectorURL(t *testing.T) {
 	for i, c := range []struct {
-		originalPath string
 		requestURL string
 		targetURL  string
 		without    string
@@ -970,13 +969,11 @@ func TestProxyDirectorURL(t *testing.T) {
 			expectURL:  `https://localhost:2021/%2C/%2C`,
 		},
 		{
-			originalPath: `///test`,
 			requestURL: `http://localhost:2020/%2F/test`,
 			targetURL:  `https://localhost:2021/`,
 			expectURL:  `https://localhost:2021/%2F/test`,
 		},
 		{
-			originalPath: `/test///mypath`,
 			requestURL: `http://localhost:2020/test/%2F/mypath`,
 			targetURL:  `https://localhost:2021/t/`,
 			expectURL:  `https://localhost:2021/t/%2F/mypath`,

caddyhttp/proxy/reverseproxy.go

@@ -118,9 +118,7 @@ func NewSingleHostReverseProxy(target *url.URL, without string, keepalive int) *
 			req.URL.Host = target.Host
 		}
 
-		// We should remove the `without` prefix at first.
-		// TODO(mholt): See #1582 (and below)
-		// untouchedPath, _ := req.Context().Value(staticfiles.URLPathCtxKey).(string)
+		// remove the `without` prefix
 		if without != "" {
 			req.URL.Path = strings.TrimPrefix(req.URL.Path, without)
 			if req.URL.Opaque != "" {
@@ -129,10 +127,6 @@ func NewSingleHostReverseProxy(target *url.URL, without string, keepalive int) *
 			if req.URL.RawPath != "" {
 				req.URL.RawPath = strings.TrimPrefix(req.URL.RawPath, without)
 			}
-			// TODO(mholt): See #1582 (and above)
-			// if untouchedPath != "" {
-			// 	untouchedPath = strings.TrimPrefix(untouchedPath, without)
-			// }
 		}
 
 		// prefer returns val if it isn't empty, otherwise def
@@ -142,6 +136,7 @@ func NewSingleHostReverseProxy(target *url.URL, without string, keepalive int) *
 			}
 			return def
 		}
+
 		// Make up the final URL by concatenating the request and target URL.
 		//
 		// If there is encoded part in request or target URL,

caddyhttp/redirect/redirect_test.go

@@ -2,6 +2,7 @@ package redirect
 
 import (
 	"bytes"
+	"context"
 	"crypto/tls"
 	"io/ioutil"
 	"net/http"
@@ -96,14 +97,16 @@ func TestParametersRedirect(t *testing.T) {
 
 	req, err := http.NewRequest("GET", "/a?b=c", nil)
 	if err != nil {
-		t.Fatalf("Test: Could not create HTTP request: %v", err)
+		t.Fatalf("Test 1: Could not create HTTP request: %v", err)
 	}
+	ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+	req = req.WithContext(ctx)
 
 	rec := httptest.NewRecorder()
 	re.ServeHTTP(rec, req)
 
-	if rec.Header().Get("Location") != "http://example.com/a?b=c" {
-		t.Fatalf("Test: expected location header %q but was %q", "http://example.com/a?b=c", rec.Header().Get("Location"))
+	if got, want := rec.Header().Get("Location"), "http://example.com/a?b=c"; got != want {
+		t.Fatalf("Test 1: expected location header %s but was %s", want, got)
 	}
 
 	re = Redirect{
@@ -114,13 +117,15 @@ func TestParametersRedirect(t *testing.T) {
 
 	req, err = http.NewRequest("GET", "/d?e=f", nil)
 	if err != nil {
-		t.Fatalf("Test: Could not create HTTP request: %v", err)
+		t.Fatalf("Test 2: Could not create HTTP request: %v", err)
 	}
+	ctx = context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+	req = req.WithContext(ctx)
 
 	re.ServeHTTP(rec, req)
 
-	if "http://example.com/a/d?b=c&e=f" != rec.Header().Get("Location") {
-		t.Fatalf("Test: expected location header %q but was %q", "http://example.com/a/d?b=c&e=f", rec.Header().Get("Location"))
+	if got, want := rec.Header().Get("Location"), "http://example.com/a/d?b=c&e=f"; got != want {
+		t.Fatalf("Test 2: expected location header %s but was %s", want, got)
 	}
 }
 

caddyhttp/rewrite/rewrite_test.go

 package rewrite
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -104,13 +105,14 @@ func TestRewrite(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Test %d: Could not create HTTP request: %v", i, err)
 		}
+		ctx := context.WithValue(req.Context(), httpserver.OriginalURLCtxKey, *req.URL)
+		req = req.WithContext(ctx)
 
 		rec := httptest.NewRecorder()
 		rw.ServeHTTP(rec, req)
 
-		if rec.Body.String() != test.expectedTo {
-			t.Errorf("Test %d: Expected URL to be '%s' but was '%s'",
-				i, test.expectedTo, rec.Body.String())
+		if got, want := rec.Body.String(), test.expectedTo; got != want {
+			t.Errorf("Test %d: Expected URL to be '%s' but was '%s'", i, want, got)
 		}
 	}
 }

caddyhttp/rewrite/to.go

 package rewrite
 
 import (
-	"context"
 	"log"
 	"net/http"
 	"net/url"
@@ -48,10 +47,6 @@ func To(fs http.FileSystem, r *http.Request, to string, replacer httpserver.Repl
 		return RewriteIgnored
 	}
 
-	// take note of this rewrite for internal use by fastcgi
-	// all we need is the URI, not full URL
-	*r = *r.WithContext(context.WithValue(r.Context(), httpserver.URIxRewriteCtxKey, r.URL.RequestURI()))
-
 	// perform rewrite
 	r.URL.Path = u.Path
 	if query != "" {

caddyhttp/rewrite/to_test.go

 package rewrite
 
 import (
+	"context"
 	"net/http"
 	"net/url"
 	"testing"
+
+	"github.com/mholt/caddy/caddyhttp/httpserver"
 )
 
 func TestTo(t *testing.T) {
@@ -39,6 +42,8 @@ func TestTo(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
+		ctx := context.WithValue(r.Context(), httpserver.OriginalURLCtxKey, *r.URL)
+		r = r.WithContext(ctx)
 		To(fs, r, test.to, newReplacer(r))
 		if uri(r.URL) != test.expected {
 			t.Errorf("Test %v: expected %v found %v", i, test.expected, uri(r.URL))