httpserver/all: Clean up and standardize request URL handling (caddys…

…erver#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 caddyserver#1624, caddyserver#1584, caddyserver#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
KISSMonX · May 2, 2017 · d5371af · d5371af
1 parent 5685a16
commit d5371af
Show file tree

Hide file tree

Showing 23 changed files with 365 additions and 573 deletions.
diff --git a/caddyhttp/browse/browse.go b/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)

diff --git a/caddyhttp/browse/browse_test.go b/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()
 

diff --git a/caddyhttp/browse/setup.go b/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

diff --git a/caddyhttp/extensions/ext.go b/caddyhttp/extensions/ext.go
@@ -31,22 +31,14 @@ 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
 			}
 		}
 	}
 	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
-}
diff --git a/caddyhttp/fastcgi/fastcgi.go b/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])
 	}
 

diff --git a/caddyhttp/fastcgi/fastcgi_test.go b/caddyhttp/fastcgi/fastcgi_test.go
@@ -1,6 +1,7 @@
 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)
 }
 

diff --git a/caddyhttp/httpserver/condition_test.go b/caddyhttp/httpserver/condition_test.go
@@ -1,6 +1,7 @@
 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
 		}
 	}
 }

diff --git a/caddyhttp/httpserver/middleware.go b/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"
 )
diff --git a/caddyhttp/httpserver/pathcleaner.go b/caddyhttp/httpserver/pathcleaner.go