diff options
author | Matt Holt <[email protected]> | 2017-05-01 23:11:10 -0600 |
---|---|---|
committer | GitHub <[email protected]> | 2017-05-01 23:11:10 -0600 |
commit | d5371aff2265540fb8600541bfdf3c3c4fc963e4 (patch) | |
tree | 0b1896887bff734583b9610afff5b69b97098f26 | |
parent | 5685a16449239d982ebe52ec66fa90eaecabe546 (diff) | |
download | caddy-d5371aff2265540fb8600541bfdf3c3c4fc963e4.tar.gz caddy-d5371aff2265540fb8600541bfdf3c3c4fc963e4.zip |
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
23 files changed, 365 insertions, 573 deletions
diff --git a/caddyhttp/browse/browse.go b/caddyhttp/browse/browse.go index 8cf77c5a4..8694d2dea 100644 --- 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 index d3ab9730f..6952f35c0 100644 --- 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 index 196881a1f..64a54c237 100644 --- 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 index 6a3c77120..2c02fa734 100644 --- a/caddyhttp/extensions/ext.go +++ b/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 -} diff --git a/caddyhttp/fastcgi/fastcgi.go b/caddyhttp/fastcgi/fastcgi.go index 99d5e9cc5..ca9eeb8d2 100644 --- 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 index 85e451261..917489ec5 100644 --- 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 index 8f2afdca7..76ba0475f 100644 --- 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 index c6ffa22de..52bc8df66 100644 --- 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 deleted file mode 100644 index cf5f1aa06..000000000 --- a/caddyhttp/httpserver/pathcleaner.go +++ /dev/null @@ -1,76 +0,0 @@ -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) -} diff --git a/caddyhttp/httpserver/pathcleaner_test.go b/caddyhttp/httpserver/pathcleaner_test.go deleted file mode 100644 index a5c610bbd..000000000 --- a/caddyhttp/httpserver/pathcleaner_test.go +++ /dev/null @@ -1,120 +0,0 @@ -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) - } - } -} diff --git a/caddyhttp/httpserver/replacer.go b/caddyhttp/httpserver/replacer.go index 1bb77fe5e..e9b51a033 100644 --- a/caddyhttp/httpserver/replacer.go +++ b/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}": diff --git a/caddyhttp/httpserver/replacer_test.go b/caddyhttp/httpserver/replacer_test.go index f7cb4b658..c59619f03 100644 --- a/caddyhttp/httpserver/replacer_test.go +++ b/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) } } diff --git a/caddyhttp/httpserver/server.go b/caddyhttp/httpserver/server.go index 6a00a27ab..6db0c0bd6 100644 --- a/caddyhttp/httpserver/server.go +++ b/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") diff --git a/caddyhttp/httpserver/context.go b/caddyhttp/httpserver/tplcontext.go index 0535a012c..0535a012c 100644 --- a/caddyhttp/httpserver/context.go +++ b/caddyhttp/httpserver/tplcontext.go diff --git a/caddyhttp/httpserver/context_test.go b/caddyhttp/httpserver/tplcontext_test.go index 01ca55bcb..01ca55bcb 100644 --- a/caddyhttp/httpserver/context_test.go +++ b/caddyhttp/httpserver/tplcontext_test.go diff --git a/caddyhttp/proxy/proxy_test.go b/caddyhttp/proxy/proxy_test.go index d81c411b0..608053793 100644 --- a/caddyhttp/proxy/proxy_test.go +++ b/caddyhttp/proxy/proxy_test.go @@ -895,11 +895,10 @@ 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 - expectURL string + requestURL string + targetURL string + without string + expectURL string }{ { requestURL: `http://localhost:2020/test`, @@ -970,17 +969,15 @@ 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`, + 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`, - without: "/test", + requestURL: `http://localhost:2020/test/%2F/mypath`, + targetURL: `https://localhost:2021/t/`, + expectURL: `https://localhost:2021/t/%2F/mypath`, + without: "/test", }, } { targetURL, err := url.Parse(c.targetURL) diff --git a/caddyhttp/proxy/reverseproxy.go b/caddyhttp/proxy/reverseproxy.go index c2bdbaff3..23deefe57 100644 --- a/caddyhttp/proxy/reverseproxy.go +++ b/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, diff --git a/caddyhttp/redirect/redirect_test.go b/caddyhttp/redirect/redirect_test.go index bc817580a..faf3846ee 100644 --- a/caddyhttp/redirect/redirect_test.go +++ b/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) } } diff --git a/caddyhttp/rewrite/rewrite_test.go b/caddyhttp/rewrite/rewrite_test.go index bb86cbc19..6be2f2211 100644 --- a/caddyhttp/rewrite/rewrite_test.go +++ b/caddyhttp/rewrite/rewrite_test.go @@ -1,6 +1,7 @@ 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) } } } diff --git a/caddyhttp/rewrite/to.go b/caddyhttp/rewrite/to.go index 45434da34..6e296dc9c 100644 --- a/caddyhttp/rewrite/to.go +++ b/caddyhttp/rewrite/to.go @@ -1,7 +1,6 @@ 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 != "" { diff --git a/caddyhttp/rewrite/to_test.go b/caddyhttp/rewrite/to_test.go index ad41bc324..e809eec9d 100644 --- a/caddyhttp/rewrite/to_test.go +++ b/caddyhttp/rewrite/to_test.go @@ -1,9 +1,12 @@ 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)) diff --git a/caddyhttp/staticfiles/fileserver.go b/caddyhttp/staticfiles/fileserver.go index b8bbc73b9..c44d9715d 100644 --- a/caddyhttp/staticfiles/fileserver.go +++ b/caddyhttp/staticfiles/fileserver.go @@ -1,3 +1,7 @@ +// Package staticfiles provides middleware for serving static files from disk. +// Its handler is the default HTTP handler for the HTTP server. +// +// TODO: Should this package be rolled into the httpserver package? package staticfiles import ( @@ -5,6 +9,7 @@ import ( "net/http" "net/url" "os" + "path" "path/filepath" "runtime" "strconv" @@ -25,47 +30,30 @@ import ( // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. type FileServer struct { - // Jailed disk access - Root http.FileSystem - - // List of files to treat as "Not Found" - Hide []string + Root http.FileSystem // jailed access to the file system + Hide []string // list of files for which to respond with "Not Found" } // ServeHTTP serves static files for r according to fs's configuration. func (fs FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { - // r.URL.Path has already been cleaned by Caddy. - if r.URL.Path == "" { - r.URL.Path = "/" - } - return fs.serveFile(w, r, r.URL.Path) -} - -// calculateEtag produces a strong etag by default. Prefix the result with "W/" to convert this into a weak one. -// see https://tools.ietf.org/html/rfc7232#section-2.3 -func calculateEtag(d os.FileInfo) string { - t := strconv.FormatInt(d.ModTime().Unix(), 36) - s := strconv.FormatInt(d.Size(), 36) - return `"` + t + s + `"` + return fs.serveFile(w, r) } // serveFile writes the specified file to the HTTP response. // name is '/'-separated, not filepath.Separator. -func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name string) (int, error) { - - location := name +func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request) (int, error) { + reqPath := r.URL.Path // Prevent absolute path access on Windows. // TODO remove when stdlib http.Dir fixes this. - if runtime.GOOS == "windows" { - if filepath.IsAbs(name[1:]) { - return http.StatusNotFound, nil - } + if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) { + return http.StatusNotFound, nil } - f, err := fs.Root.Open(name) + // open the requested file + f, err := fs.Root.Open(reqPath) if err != nil { - // TODO: remove when http.Dir handles this + // TODO: remove when http.Dir handles this (Go 1.9?) // Go issue #18984 err = mapFSRootOpenErr(err) if os.IsNotExist(err) { @@ -73,13 +61,14 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri } else if os.IsPermission(err) { return http.StatusForbidden, err } - // Likely the server is under load and ran out of file descriptors + // otherwise, maybe the server is under load and ran out of file descriptors? 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() + // get information about the file d, err := f.Stat() if err != nil { if os.IsNotExist(err) { @@ -87,84 +76,105 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri } else if os.IsPermission(err) { return http.StatusForbidden, err } - // Return a different status code than above so as to distinguish these cases + // return a different status code than above to distinguish these cases return http.StatusInternalServerError, err } - // redirect to canonical path + // redirect to canonical path (being careful to preserve other parts of URL and + // considering cases where a site is defined with a path prefix that gets stripped) + u := r.Context().Value(caddy.CtxKey("original_url")).(url.URL) + if u.Path == "" { + u.Path = "/" + } if d.IsDir() { - // Ensure / at end of directory url. If the original URL path is - // used then ensure / exists as well. - if !strings.HasSuffix(r.URL.Path, "/") { - RedirectToDir(w, r) + // ensure there is a trailing slash + if u.Path[len(u.Path)-1] != '/' { + u.Path += "/" + http.Redirect(w, r, u.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, "/") { - RedirectToFile(w, r) + // ensure no trailing slash + redir := false + if u.Path[len(u.Path)-1] == '/' { + u.Path = u.Path[:len(u.Path)-1] + redir = true + } + + // if an index file was explicitly requested, strip file name from the request + // ("/foo/index.html" -> "/foo/") + for _, indexPage := range IndexPages { + if strings.HasSuffix(u.Path, indexPage) { + u.Path = u.Path[:len(u.Path)-len(indexPage)] + redir = true + break + } + } + + if redir { + http.Redirect(w, r, u.String(), http.StatusMovedPermanently) return http.StatusMovedPermanently, nil } } - // use contents of an index file, if present, for directory + // use contents of an index file, if present, for directory requests if d.IsDir() { for _, indexPage := range IndexPages { - index := strings.TrimSuffix(name, "/") + "/" + indexPage - ff, err := fs.Root.Open(index) + indexPath := path.Join(reqPath, indexPage) + indexFile, err := fs.Root.Open(indexPath) if err != nil { continue } - // this defer does not leak fds because previous iterations - // of the loop must have had an err, so nothing to close - defer ff.Close() - - dd, err := ff.Stat() + indexInfo, err := indexFile.Stat() if err != nil { - ff.Close() + indexFile.Close() continue } - // Close previous file - release fd immediately + // this defer does not leak fds even though we are in a loop, + // because previous iterations of the loop must have had an + // err, so there's nothing to close from earlier iterations. + defer indexFile.Close() + + // close previously-opened file immediately to release fd f.Close() - d = dd - f = ff - location = index + // switch to using the index file, and we're done here + d = indexInfo + f = indexFile + reqPath = indexPath break } } - // Still a directory? (we didn't find an index file) - // Return 404 to hide the fact that the folder exists - if d.IsDir() { + // return Not Found if we either did not find an index file (and thus are + // still a directory) or if this file is supposed to be hidden + if d.IsDir() || fs.IsHidden(d) { return http.StatusNotFound, nil } - if fs.IsHidden(d) { - return http.StatusNotFound, nil - } - - filename := d.Name() - etag := calculateEtag(d) // strong + etag := calculateEtag(d) + // look for compressed versions of the file on disk, if the client supports that encoding for _, encoding := range staticEncodingPriority { + // see if the client accepts a compressed encoding we offer acceptEncoding := strings.Split(r.Header.Get("Accept-Encoding"), ",") - accepted := false for _, acc := range acceptEncoding { - if accepted || strings.TrimSpace(acc) == encoding { + if strings.TrimSpace(acc) == encoding { accepted = true + break } } + // if client doesn't support this encoding, don't even bother; try next one if !accepted { continue } - encodedFile, err := fs.Root.Open(location + staticEncoding[encoding]) + // see if the compressed version of this file exists + encodedFile, err := fs.Root.Open(reqPath + staticEncoding[encoding]) if err != nil { continue } @@ -175,19 +185,17 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri continue } - // Close previous file - release fd + // close the encoded file when we're done, and close the + // previously-opened file immediately to release the fd + defer encodedFile.Close() f.Close() - etag = calculateEtag(encodedFileInfo) - - // Encoded file will be served + // the encoded file is now what we're serving f = encodedFile - + etag = calculateEtag(encodedFileInfo) w.Header().Add("Vary", "Accept-Encoding") w.Header().Set("Content-Encoding", encoding) w.Header().Set("Content-Length", strconv.FormatInt(encodedFileInfo.Size(), 10)) - - defer f.Close() break } @@ -197,16 +205,17 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri // Note: Errors generated by ServeContent are written immediately // to the response. This usually only happens if seeking fails (rare). - http.ServeContent(w, r, filename, d.ModTime(), f) + // Its signature does not bubble the error up to us, so we cannot + // return it for any logging middleware to record. Oh well. + http.ServeContent(w, r, d.Name(), d.ModTime(), f) return http.StatusOK, nil } // IsHidden checks if file with FileInfo d is on hide list. func (fs FileServer) IsHidden(d os.FileInfo) bool { - // If the file is supposed to be hidden, return a 404 for _, hiddenPath := range fs.Hide { - // Check if the served file is exactly the hidden file. + // TODO: Could these FileInfos be stored instead of their paths, to avoid opening them all the time? if hFile, err := fs.Root.Open(hiddenPath); err == nil { fs, _ := hFile.Stat() hFile.Close() @@ -218,34 +227,15 @@ func (fs FileServer) IsHidden(d os.FileInfo) bool { return false } -// 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 - } - 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) +// calculateEtag produces a strong etag by default, although, for +// efficiency reasons, it does not actually consume the contents +// of the file to make a hash of all the bytes. ¯\_(ツ)_/¯ +// Prefix the etag with "W/" to convert it into a weak etag. +// See: https://tools.ietf.org/html/rfc7232#section-2.3 +func calculateEtag(d os.FileInfo) string { + t := strconv.FormatInt(d.ModTime().Unix(), 36) + s := strconv.FormatInt(d.Size(), 36) + return `"` + t + s + `"` } // IndexPages is a list of pages that may be understood as @@ -277,7 +267,7 @@ var staticEncodingPriority = []string{ // to a possibly better non-nil error. In particular, it turns OS-specific errors // about opening files in non-directories into os.ErrNotExist. // -// TODO: remove when http.Dir handles this +// TODO: remove when http.Dir handles this (slated for Go 1.9) // Go issue #18984 func mapFSRootOpenErr(originalErr error) error { if os.IsNotExist(originalErr) || os.IsPermission(originalErr) { @@ -304,8 +294,3 @@ func mapFSRootOpenErr(originalErr error) error { } return originalErr } - -// URLPathCtxKey is a context key. It can be used in HTTP handlers with -// context.WithValue to access the original request URI that accompanied the -// server request. The associated value will be of type string. -const URLPathCtxKey caddy.CtxKey = "url_path" diff --git a/caddyhttp/staticfiles/fileserver_test.go b/caddyhttp/staticfiles/fileserver_test.go index 0645a8cba..d5f3a42bc 100644 --- a/caddyhttp/staticfiles/fileserver_test.go +++ b/caddyhttp/staticfiles/fileserver_test.go @@ -3,72 +3,26 @@ package staticfiles import ( "context" "errors" + "io/ioutil" "net/http" "net/http/httptest" - "net/url" "os" "path/filepath" "strconv" "strings" "testing" "time" -) - -var ( - ErrCustom = errors.New("Custom Error") - testDir = filepath.Join(os.TempDir(), "caddy_testdir") - testWebRoot = filepath.Join(testDir, "webroot") -) - -var ( - webrootFile1HTML = filepath.Join("webroot", "file1.html") - webrootDirFile2HTML = filepath.Join("webroot", "dir", "file2.html") - webrootDirHiddenHTML = filepath.Join("webroot", "dir", "hidden.html") - webrootDirwithindexIndeHTML = filepath.Join("webroot", "dirwithindex", "index.html") - webrootSubGzippedHTML = filepath.Join("webroot", "sub", "gzipped.html") - webrootSubGzippedHTMLGz = filepath.Join("webroot", "sub", "gzipped.html.gz") - webrootSubGzippedHTMLBr = filepath.Join("webroot", "sub", "gzipped.html.br") - webrootSubBrotliHTML = filepath.Join("webroot", "sub", "brotli.html") - webrootSubBrotliHTMLGz = filepath.Join("webroot", "sub", "brotli.html.gz") - webrootSubBrotliHTMLBr = filepath.Join("webroot", "sub", "brotli.html.br") - webrootSubBarDirWithIndexIndexHTML = filepath.Join("webroot", "bar", "dirwithindex", "index.html") + "github.com/mholt/caddy" ) -// 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/ -// '-- unreachable.html -// '-- webroot/ -// '---- file1.html -// '---- dirwithindex/ -// '------ index.html -// '---- dir/ -// '------ file2.html -// '------ hidden.html -var testFiles = map[string]string{ - "unreachable.html": "<h1>must not leak</h1>", - webrootFile1HTML: "<h1>file1.html</h1>", - webrootDirFile2HTML: "<h1>dir/file2.html</h1>", - webrootDirwithindexIndeHTML: "<h1>dirwithindex/index.html</h1>", - webrootDirHiddenHTML: "<h1>dir/hidden.html</h1>", - webrootSubGzippedHTML: "<h1>gzipped.html</h1>", - webrootSubGzippedHTMLGz: "1.gzipped.html.gz", - webrootSubGzippedHTMLBr: "2.gzipped.html.br", - webrootSubBrotliHTML: "3.brotli.html", - webrootSubBrotliHTMLGz: "4.brotli.html.gz", - webrootSubBrotliHTMLBr: "5.brotli.html.br", - webrootSubBarDirWithIndexIndexHTML: "<h1>bar/dirwithindex/index.html</h1>", -} - // TestServeHTTP covers positive scenarios when serving files. func TestServeHTTP(t *testing.T) { - - beforeServeHTTPTest(t) - defer afterServeHTTPTest(t) + tmpWebRootDir := beforeServeHTTPTest(t) + defer afterServeHTTPTest(t, tmpWebRootDir) fileserver := FileServer{ - Root: http.Dir(testWebRoot), + Root: http.Dir(filepath.Join(tmpWebRootDir, webrootName)), Hide: []string{"dir/hidden.html"}, } @@ -76,7 +30,7 @@ func TestServeHTTP(t *testing.T) { tests := []struct { url string - cleanedPath string + stripPathPrefix string // for when sites are defined with a path (e.g. "example.com/foo/") acceptEncoding string expectedLocation string expectedStatus int @@ -148,53 +102,62 @@ func TestServeHTTP(t *testing.T) { url: "https://foo/dir/hidden.html", expectedStatus: http.StatusNotFound, }, - // Test 10 - access a index file directly + // Test 10 - access an index file directly { - url: "https://foo/dirwithindex/index.html", - expectedStatus: http.StatusOK, - expectedBodyContent: testFiles[webrootDirwithindexIndeHTML], - expectedEtag: `"2n9cw"`, - expectedContentLength: strconv.Itoa(len(testFiles[webrootDirwithindexIndeHTML])), + url: "https://foo/dirwithindex/index.html", + expectedStatus: http.StatusMovedPermanently, + expectedLocation: "https://foo/dirwithindex/", + }, + // Test 11 - access an index file with a trailing slash + { + url: "https://foo/dirwithindex/index.html/", + expectedStatus: http.StatusMovedPermanently, + expectedLocation: "https://foo/dirwithindex/", }, - // Test 11 - send a request with query params + // Test 12 - send a request with query params { url: "https://foo/dir?param1=val", expectedStatus: http.StatusMovedPermanently, expectedLocation: "https://foo/dir/?param1=val", expectedBodyContent: movedPermanently, }, - // Test 12 - attempt to bypass hidden file + // Test 13 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html%20", expectedStatus: http.StatusNotFound, }, - // Test 13 - attempt to bypass hidden file + // Test 14 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html.", expectedStatus: http.StatusNotFound, }, - // Test 14 - attempt to bypass hidden file + // Test 15 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html.%20", expectedStatus: http.StatusNotFound, }, - // Test 15 - attempt to bypass hidden file + // Test 16 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html%20.", acceptEncoding: "br, gzip", expectedStatus: http.StatusNotFound, }, - // Test 16 - serve another file with same name as hidden file. + // Test 17 - serve another file with same name as hidden file. { url: "https://foo/hidden.html", expectedStatus: http.StatusNotFound, }, - // Test 17 - try to get below the root directory. + // Test 18 - try to get below the root directory. + { + url: "https://foo/../unreachable.html", + expectedStatus: http.StatusNotFound, + }, + // Test 19 - try to get below the root directory (encoded slashes). { - url: "https://foo/%2f..%2funreachable.html", + url: "https://foo/..%2funreachable.html", expectedStatus: http.StatusNotFound, }, - // Test 18 - try to get pre-gzipped file. + // Test 20 - try to get pre-gzipped file. { url: "https://foo/sub/gzipped.html", acceptEncoding: "gzip", @@ -205,7 +168,7 @@ func TestServeHTTP(t *testing.T) { expectedEncoding: "gzip", expectedContentLength: strconv.Itoa(len(testFiles[webrootSubGzippedHTMLGz])), }, - // Test 19 - try to get pre-brotli encoded file. + // Test 21 - try to get pre-brotli encoded file. { url: "https://foo/sub/brotli.html", acceptEncoding: "br,gzip", @@ -216,7 +179,7 @@ func TestServeHTTP(t *testing.T) { expectedEncoding: "br", expectedContentLength: strconv.Itoa(len(testFiles[webrootSubBrotliHTMLBr])), }, - // Test 20 - not allowed to get pre-brotli encoded file. + // Test 22 - not allowed to get pre-brotli encoded file. { url: "https://foo/sub/brotli.html", acceptEncoding: "nicebrew", // contains "br" substring but not "br" @@ -227,33 +190,31 @@ func TestServeHTTP(t *testing.T) { expectedEncoding: "", expectedContentLength: strconv.Itoa(len(testFiles[webrootSubBrotliHTML])), }, - // Test 20 - treat existing file as a directory. + // Test 23 - treat existing file as a directory. { url: "https://foo/file1.html/other", expectedStatus: http.StatusNotFound, }, - // Test 20 - access folder with index file without trailing slash, with - // cleaned path + // Test 24 - access folder with index file without trailing slash, with stripped path { url: "https://foo/bar/dirwithindex", - cleanedPath: "/dirwithindex", + stripPathPrefix: "/bar/", expectedStatus: http.StatusMovedPermanently, expectedLocation: "https://foo/bar/dirwithindex/", expectedBodyContent: movedPermanently, }, - // Test 21 - access folder with index file without trailing slash, with - // cleaned path and query params + // Test 25 - access folder with index file without trailing slash, with stripped path and query params { url: "https://foo/bar/dirwithindex?param1=val", - cleanedPath: "/dirwithindex", + stripPathPrefix: "/bar/", expectedStatus: http.StatusMovedPermanently, expectedLocation: "https://foo/bar/dirwithindex/?param1=val", expectedBodyContent: movedPermanently, }, - // Test 22 - access file with trailing slash with cleaned path + // Test 26 - site defined with path ("bar"), which has that prefix stripped { url: "https://foo/bar/file1.html/", - cleanedPath: "file1.html/", + stripPathPrefix: "/bar/", expectedStatus: http.StatusMovedPermanently, expectedLocation: "https://foo/bar/file1.html", expectedBodyContent: movedPermanently, @@ -261,26 +222,26 @@ func TestServeHTTP(t *testing.T) { } for i, test := range tests { + // set up response writer and rewuest responseRecorder := httptest.NewRecorder() request, err := http.NewRequest("GET", test.url, nil) - ctx := context.WithValue(request.Context(), URLPathCtxKey, request.URL.Path) + if err != nil { + t.Errorf("Test %d: Error making request: %v", i, err) + continue + } + + // set the original URL on the context + ctx := context.WithValue(request.Context(), caddy.CtxKey("original_url"), *request.URL) request = request.WithContext(ctx) request.Header.Add("Accept-Encoding", test.acceptEncoding) - if err != nil { - t.Errorf("Test %d: Error making request: %v", i, err) - } - // prevent any URL sanitization within Go: we need unmodified paths here - if u, _ := url.Parse(test.url); u.RawPath != "" { - request.URL.Path = u.RawPath - } - // Caddy may trim a request's URL path. Overwrite the path with - // the cleanedPath to test redirects when the path has been - // modified. - if test.cleanedPath != "" { - request.URL.Path = test.cleanedPath + // simulate cases where a site is defined with a path prefix (e.g. "localhost/foo/") + if test.stripPathPrefix != "" { + request.URL.Path = strings.TrimPrefix(request.URL.Path, test.stripPathPrefix) } + + // perform the test status, err := fileserver.ServeHTTP(responseRecorder, request) etag := responseRecorder.Header().Get("Etag") body := responseRecorder.Body.String() @@ -318,6 +279,7 @@ func TestServeHTTP(t *testing.T) { t.Errorf("Test %d: Expected body to contain %q, found %q", i, test.expectedBodyContent, body) } + // check Location header if test.expectedLocation != "" { l := responseRecorder.Header().Get("Location") if test.expectedLocation != l { @@ -334,20 +296,16 @@ 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.MkdirAll(testWebRoot, os.ModePerm) +func beforeServeHTTPTest(t *testing.T) string { + tmpdir, err := ioutil.TempDir("", testDirPrefix) if err != nil { - if !os.IsExist(err) { - t.Fatalf("Failed to create test dir. Error was: %v", err) - return - } + t.Fatalf("failed to create test directory: %v", err) } fixedTime := time.Unix(123456, 0) for relFile, fileContent := range testFiles { - absFile := filepath.Join(testDir, relFile) + absFile := filepath.Join(tmpdir, relFile) // make sure the parent directories exist parentDir := filepath.Dir(absFile) @@ -360,14 +318,12 @@ func beforeServeHTTPTest(t *testing.T) { f, err := os.Create(absFile) if err != nil { t.Fatalf("Failed to create test file %s. Error was: %v", absFile, err) - return } // and fill them with content _, err = f.WriteString(fileContent) if err != nil { t.Fatalf("Failed to write to %s. Error was: %v", absFile, err) - return } f.Close() @@ -378,14 +334,18 @@ func beforeServeHTTPTest(t *testing.T) { } } + return tmpdir } // afterServeHTTPTest removes the test dir and all its content -func afterServeHTTPTest(t *testing.T) { +func afterServeHTTPTest(t *testing.T, webroot string) { + if !strings.Contains(webroot, testDirPrefix) { + t.Fatalf("Cannot clean up after test because webroot is: %s", webroot) + } // cleans up everything under the test dir. No need to clean the individual files. - err := os.RemoveAll(testDir) + err := os.RemoveAll(webroot) if err != nil { - t.Fatalf("Failed to clean up test dir %s. Error was: %v", testDir, err) + t.Fatalf("Failed to clean up test dir %s. Error was: %v", webroot, err) } } @@ -416,9 +376,9 @@ func (ff failingFile) Close() error { return nil } -// TestServeHTTPFailingFS tests error cases where the Open function fails with various errors. +// TestServeHTTPFailingFS tests error cases where the Open +// function fails with various errors. func TestServeHTTPFailingFS(t *testing.T) { - tests := []struct { fsErr error expectedStatus int @@ -436,9 +396,9 @@ func TestServeHTTPFailingFS(t *testing.T) { expectedErr: os.ErrPermission, }, { - fsErr: ErrCustom, + fsErr: errCustom, expectedStatus: http.StatusServiceUnavailable, - expectedErr: ErrCustom, + expectedErr: errCustom, expectedHeaders: map[string]string{"Retry-After": "5"}, }, } @@ -478,9 +438,9 @@ func TestServeHTTPFailingFS(t *testing.T) { } } -// TestServeHTTPFailingStat tests error cases where the initial Open function succeeds, but the Stat method on the opened file fails. +// TestServeHTTPFailingStat tests error cases where the initial Open function succeeds, +// but the Stat method on the opened file fails. func TestServeHTTPFailingStat(t *testing.T) { - tests := []struct { statErr error expectedStatus int @@ -497,9 +457,9 @@ func TestServeHTTPFailingStat(t *testing.T) { expectedErr: os.ErrPermission, }, { - statErr: ErrCustom, + statErr: errCustom, expectedStatus: http.StatusInternalServerError, - expectedErr: ErrCustom, + expectedErr: errCustom, }, } @@ -528,6 +488,54 @@ func TestServeHTTPFailingStat(t *testing.T) { } } +// Paths for the fake site used temporarily during testing. +var ( + webrootFile1HTML = filepath.Join(webrootName, "file1.html") + webrootDirFile2HTML = filepath.Join(webrootName, "dir", "file2.html") + webrootDirHiddenHTML = filepath.Join(webrootName, "dir", "hidden.html") + webrootDirwithindexIndeHTML = filepath.Join(webrootName, "dirwithindex", "index.html") + webrootSubGzippedHTML = filepath.Join(webrootName, "sub", "gzipped.html") + webrootSubGzippedHTMLGz = filepath.Join(webrootName, "sub", "gzipped.html.gz") + webrootSubGzippedHTMLBr = filepath.Join(webrootName, "sub", "gzipped.html.br") + webrootSubBrotliHTML = filepath.Join(webrootName, "sub", "brotli.html") + webrootSubBrotliHTMLGz = filepath.Join(webrootName, "sub", "brotli.html.gz") + webrootSubBrotliHTMLBr = filepath.Join(webrootName, "sub", "brotli.html.br") + webrootSubBarDirWithIndexIndexHTML = filepath.Join(webrootName, "bar", "dirwithindex", "index.html") +) + +// 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/ +// '-- unreachable.html +// '-- webroot/ +// '---- file1.html +// '---- dirwithindex/ +// '------ index.html +// '---- dir/ +// '------ file2.html +// '------ hidden.html +var testFiles = map[string]string{ + "unreachable.html": "<h1>must not leak</h1>", + webrootFile1HTML: "<h1>file1.html</h1>", + webrootDirFile2HTML: "<h1>dir/file2.html</h1>", + webrootDirwithindexIndeHTML: "<h1>dirwithindex/index.html</h1>", + webrootDirHiddenHTML: "<h1>dir/hidden.html</h1>", + webrootSubGzippedHTML: "<h1>gzipped.html</h1>", + webrootSubGzippedHTMLGz: "1.gzipped.html.gz", + webrootSubGzippedHTMLBr: "2.gzipped.html.br", + webrootSubBrotliHTML: "3.brotli.html", + webrootSubBrotliHTMLGz: "4.brotli.html.gz", + webrootSubBrotliHTMLBr: "5.brotli.html.br", + webrootSubBarDirWithIndexIndexHTML: "<h1>bar/dirwithindex/index.html</h1>", +} + +var errCustom = errors.New("custom error") + +const ( + testDirPrefix = "caddy_fileserver_test" + webrootName = "webroot" // name of the folder inside the tmp dir that has the site +) + //------------------------------------------------------------------------------------------------- type fileInfo struct { @@ -564,8 +572,6 @@ func (fi fileInfo) Sys() interface{} { var _ os.FileInfo = fileInfo{} -//------------------------------------------------------------------------------------------------- - func BenchmarkEtag(b *testing.B) { d := fileInfo{ size: 1234567890, |