caddyhttp: Refactor and export SanitizedPathJoin for use in fastcgi (#4207)

This is the more correct implementation of  23dadc0d86 (#4179)... I think. This commit effectively undoes the revert in 8848df9c5d, but with corrections to the logic.

We *do* need to use the original request path (the path the browser knows) for redirects, since they are external, and rewrites are only internal.

However, if the path was rewritten to a non-canonical path, we should not redirect to canonicalize that, since rewrites are intentional by the site owner. Canonicalizing the path involves modifying only the suffix (base element, or filename) of the path. Thus, if a rewrite involves only the prefix (like how handle_path strips a path prefix), then we can (hopefully!) safely redirect using the original URI since the filename was not rewritten.

So basically, if rewrites modify the filename, we should not canonicalize those requests. If rewrites only modify another part of the path (commonly a prefix), we should be OK to redirect.

.github/SECURITY.md

@@ -2,9 +2,6 @@
 
 The Caddy project would like to make sure that it stays on top of all practically-exploitable vulnerabilities.
 
-Some security problems are more the result of interplay between different components of the Web, rather than a vulnerability in the web server itself. Please report only vulnerabilities in the web server itself, as we cannot coerce the rest of the Web to be fixed (for example, we do not consider IP spoofing or BGP hijacks a vulnerability in the Caddy web server).
-
-Please note that we consider publicly-registered domain names to be public information. This necessary in order to maintain the integrity of certificate transparency, public DNS, and other public trust systems.
 
 ## Supported Versions
 
@@ -14,11 +11,46 @@ Please note that we consider publicly-registered domain names to be public infor
 | 1.x     | :x:                |
 | < 1.x   | :x:                |
 
+
+## Acceptable Scope
+
+A security report must demonstrate a security bug in the source code from this repository.
+
+Some security problems are the result of interplay between different components of the Web, rather than a vulnerability in the web server itself. Please only report vulnerabilities in the web server itself, as we cannot coerce the rest of the Web to be fixed (for example, we do not consider IP spoofing, BGP hijacks, or missing/misconfigured HTTP headers a vulnerability in the Caddy web server).
+
+Vulnerabilities caused by misconfigurations are out of scope. Yes, it is entirely possible to craft and use a configuration that is unsafe, just like with every other web server; we recommend against doing that.
+
+We do not accept reports if the steps imply or require a compromised system or third-party software, as we cannot control those. We expect that users secure their own systems and keep all their software patched. For example, if untrusted users are able to upload/write/host arbitrary files in the web root directory, it is NOT a security bug in Caddy if those files get served to clients; however, it _would_ be a valid report if a bug in Caddy's source code unintentionally gave unauthorized users the ability to upload unsafe files or delete files without relying on an unpatched system or piece of software.
+
+Client-side exploits are out of scope. In other words, it is not a bug in Caddy if the web browser does something unsafe, even if the downloaded content was served by Caddy. (Those kinds of exploits can generally be mitigated by proper configuration of HTTP headers.) As a general rule, the content served by Caddy is not considered in scope because content is configurable by the site owner or the associated web application.
+
+Security bugs in code dependencies are out of scope. Instead, if a dependency has patched a relevant security bug, please feel free to open a public issue or pull request to update that dependency in our code.
+
+
 ## Reporting a Vulnerability
 
-Please email Matt Holt (the author) directly: matt [at] lightcodelabs [dot com].
+We get a lot of difficult reports that turn out to be invalid. Clear, obvious reports tend to be the most credible (but are also rare).
 
-We'll need enough information to verify the bug and make a patch. It will speed things up if you suggest a working patch, such as a code diff, and explain why and how it works. Reports that are not actionable, do not contain enough information, are too pushy/demanding, or are not able to convince us that it is a viable and practical attack on the web server itself may be deferred to a later time or possibly ignored, resources permitting. Priority will be given to credible, responsible reports that are constructive, specific, and actionable. Thank you for understanding.
+First please ensure your report falls within the accepted scope of security bugs (above).
+
+We'll need enough information to verify the bug and make a patch. To speed things up, please include:
+
+- Most minimal possible config (without redactions!)
+- Command(s)
+- Precise HTTP requests (`curl -v` and its output please)
+- Full log output (please enable debug mode)
+- Specific minimal steps to reproduce the issue from scratch
+- A working patch
+
+Please DO NOT use containers, VMs, cloud instances or services, or any other complex infrastructure in your steps. Always prefer `curl` instead of web browsers.
+
+We consider publicly-registered domain names to be public information. This necessary in order to maintain the integrity of certificate transparency, public DNS, and other public trust systems. Do not redact domain names from your reports. The actual content of your domain name affects Caddy's behavior, so we need the exact domain name(s) to reproduce with, or your report will be ignored.
+
+It will speed things up if you suggest a working patch, such as a code diff, and explain why and how it works. Reports that are not actionable, do not contain enough information, are too pushy/demanding, or are not able to convince us that it is a viable and practical attack on the web server itself may be deferred to a later time or possibly ignored, depending on available resources. Priority will be given to credible, responsible reports that are constructive, specific, and actionable. (We get a lot of invalid reports.) Thank you for understanding.
+
+When you are ready, please email Matt Holt (the author) directly: matt [at] lightcodelabs [dot com].
+
+Please don't encrypt the email body. It only makes the process more complicated.
 
 Please also understand that due to our nature as an open source project, we do not have a budget to award security bounties. We can only thank you.
 

caddytest/integration/caddyfile_adapt/reverse_proxy_options.txt

@@ -21,7 +21,7 @@ https://example.com {
 			versions h2c 2
 			compression off
 			max_conns_per_host 5
-			max_idle_conns_per_host 2
+			keepalive_idle_conns_per_host 2
 		}
 	}
 }
@@ -79,8 +79,10 @@ https://example.com {
 														"dial_fallback_delay": 5000000000,
 														"dial_timeout": 3000000000,
 														"expect_continue_timeout": 9000000000,
+														"keep_alive": {
+															"max_idle_conns_per_host": 2
+														},
 														"max_conns_per_host": 5,
-														"max_idle_conns_per_host": 2,
 														"max_response_header_size": 30000000,
 														"protocol": "http",
 														"read_buffer_size": 10000000,

modules/caddyhttp/caddyhttp.go

@@ -20,7 +20,9 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
@@ -217,6 +219,31 @@ func StatusCodeMatches(actual, configured int) bool {
 	return false
 }
 
+// SanitizedPathJoin performs filepath.Join(root, reqPath) that
+// is safe against directory traversal attacks. It uses logic
+// similar to that in the Go standard library, specifically
+// in the implementation of http.Dir. The root is assumed to
+// be a trusted path, but reqPath is not; and the output will
+// never be outside of root. The resulting path can be used
+// with the local file system.
+func SanitizedPathJoin(root, reqPath string) string {
+	if root == "" {
+		root = "."
+	}
+
+	path := filepath.Join(root, filepath.Clean("/"+reqPath))
+
+	// filepath.Join also cleans the path, and cleaning strips
+	// the trailing slash, so we need to re-add it afterwards.
+	// if the length is 1, then it's a path to the root,
+	// and that should return ".", so we don't append the separator.
+	if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
+		path += separator
+	}
+
+	return path
+}
+
 // tlsPlaceholderWrapper is a no-op listener wrapper that marks
 // where the TLS listener should be in a chain of listener wrappers.
 // It should only be used if another listener wrapper must be placed
@@ -242,6 +269,8 @@ const (
 	DefaultHTTPSPort = 443
 )
 
+const separator = string(filepath.Separator)
+
 // Interface guard
 var _ caddy.ListenerWrapper = (*tlsPlaceholderWrapper)(nil)
 var _ caddyfile.Unmarshaler = (*tlsPlaceholderWrapper)(nil)

modules/caddyhttp/caddyhttp_test.go

@@ -0,0 +1,94 @@
+package caddyhttp
+
+import (
+	"net/url"
+	"path/filepath"
+	"testing"
+)
+
+func TestSanitizedPathJoin(t *testing.T) {
+	// For reference:
+	// %2e = .
+	// %2f = /
+	// %5c = \
+	for i, tc := range []struct {
+		inputRoot string
+		inputPath string
+		expect    string
+	}{
+		{
+			inputPath: "",
+			expect:    ".",
+		},
+		{
+			inputPath: "/",
+			expect:    ".",
+		},
+		{
+			inputPath: "/foo",
+			expect:    "foo",
+		},
+		{
+			inputPath: "/foo/",
+			expect:    "foo" + separator,
+		},
+		{
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("foo", "bar"),
+		},
+		{
+			inputRoot: "/a",
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("/", "a", "foo", "bar"),
+		},
+		{
+			inputPath: "/foo/../bar",
+			expect:    "bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/foo/../bar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/..%2fbar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2fbar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2f%2e%2e%2f",
+			expect:    filepath.Join("/", "a", "b") + separator,
+		},
+		{
+			inputRoot: "C:\\www",
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("C:\\www", "foo", "bar"),
+		},
+		{
+			inputRoot: "C:\\www",
+			inputPath: "/D:\\foo\\bar",
+			expect:    filepath.Join("C:\\www", "D:\\foo\\bar"),
+		},
+	} {
+		// we don't *need* to use an actual parsed URL, but it
+		// adds some authenticity to the tests since real-world
+		// values will be coming in from URLs; thus, the test
+		// corpus can contain paths as encoded by clients, which
+		// more closely emulates the actual attack vector
+		u, err := url.Parse("http://test:9999" + tc.inputPath)
+		if err != nil {
+			t.Fatalf("Test %d: invalid URL: %v", i, err)
+		}
+		actual := SanitizedPathJoin(tc.inputRoot, u.Path)
+		if actual != tc.expect {
+			t.Errorf("Test %d: SanitizedPathJoin('%s', '%s') =>  %s (expected '%s')",
+				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
+		}
+	}
+}

modules/caddyhttp/fileserver/browse.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"path"
 	"strings"
+	"sync"
 	"text/template"
 
 	"github.com/caddyserver/caddy/v2"
@@ -34,8 +35,6 @@ import (
 type Browse struct {
 	// Use this template file instead of the default browse template.
 	TemplateFile string `json:"template_file,omitempty"`
-
-	template *template.Template
 }
 
 func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
@@ -43,16 +42,28 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 		zap.String("path", dirPath),
 		zap.String("root", root))
 
-	// navigation on the client-side gets messed up if the
-	// URL doesn't end in a trailing slash because hrefs like
-	// "/b/c" on a path like "/a" end up going to "/b/c" instead
+	// Navigation on the client-side gets messed up if the
+	// URL doesn't end in a trailing slash because hrefs to
+	// "b/c" at path "/a" end up going to "/b/c" instead
 	// of "/a/b/c" - so we have to redirect in this case
-	oldReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
-	if !strings.HasSuffix(oldReq.URL.Path, "/") {
-		fsrv.logger.Debug("redirecting to trailing slash to preserve hrefs", zap.String("request_path", oldReq.URL.Path))
-		oldReq.URL.Path += "/"
-		http.Redirect(w, r, oldReq.URL.String(), http.StatusMovedPermanently)
-		return nil
+	// so that the path is "/a/" and the client constructs
+	// relative hrefs "b/c" to be "/a/b/c".
+	//
+	// Only redirect if the last element of the path (the filename) was not
+	// rewritten; if the admin wanted to rewrite to the canonical path, they
+	// would have, and we have to be very careful not to introduce unwanted
+	// redirects and especially redirect loops! (Redirecting using the
+	// original URI is necessary because that's the URI the browser knows,
+	// we don't want to redirect from internally-rewritten URIs.)
+	// See https://github.com/caddyserver/caddy/issues/4205.
+	origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
+	if path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
+		if !strings.HasSuffix(origReq.URL.Path, "/") {
+			fsrv.logger.Debug("redirecting to trailing slash to preserve hrefs", zap.String("request_path", r.URL.Path))
+			origReq.URL.Path += "/"
+			http.Redirect(w, r, origReq.URL.String(), http.StatusMovedPermanently)
+			return nil
+		}
 	}
 
 	dir, err := fsrv.openFile(dirPath, w)
@@ -76,11 +87,14 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 
 	fsrv.browseApplyQueryParams(w, r, &listing)
 
-	// write response as either JSON or HTML
-	var buf *bytes.Buffer
+	buf := bufPool.Get().(*bytes.Buffer)
+	defer bufPool.Put(buf)
+
 	acceptHeader := strings.ToLower(strings.Join(r.Header["Accept"], ","))
+
+	// write response as either JSON or HTML
 	if strings.Contains(acceptHeader, "application/json") {
-		if buf, err = fsrv.browseWriteJSON(listing); err != nil {
+		if err := json.NewEncoder(buf).Encode(listing.Items); err != nil {
 			return caddyhttp.Error(http.StatusInternalServerError, err)
 		}
 		w.Header().Set("Content-Type", "application/json; charset=utf-8")
@@ -99,12 +113,11 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 			browseTemplateContext: listing,
 		}
 
-		err = fsrv.makeBrowseTemplate(tplCtx)
+		tpl, err := fsrv.makeBrowseTemplate(tplCtx)
 		if err != nil {
 			return fmt.Errorf("parsing browse template: %v", err)
 		}
-
-		if buf, err = fsrv.browseWriteHTML(tplCtx); err != nil {
+		if err := tpl.Execute(buf, tplCtx); err != nil {
 			return caddyhttp.Error(http.StatusInternalServerError, err)
 		}
 		w.Header().Set("Content-Type", "text/html; charset=utf-8")
@@ -162,7 +175,7 @@ func (fsrv *FileServer) browseApplyQueryParams(w http.ResponseWriter, r *http.Re
 }
 
 // makeBrowseTemplate creates the template to be used for directory listings.
-func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) error {
+func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) (*template.Template, error) {
 	var tpl *template.Template
 	var err error
 
@@ -170,33 +183,17 @@ func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) error {
 		tpl = tplCtx.NewTemplate(path.Base(fsrv.Browse.TemplateFile))
 		tpl, err = tpl.ParseFiles(fsrv.Browse.TemplateFile)
 		if err != nil {
-			return fmt.Errorf("parsing browse template file: %v", err)
+			return nil, fmt.Errorf("parsing browse template file: %v", err)
 		}
 	} else {
 		tpl = tplCtx.NewTemplate("default_listing")
 		tpl, err = tpl.Parse(defaultBrowseTemplate)
 		if err != nil {
-			return fmt.Errorf("parsing default browse template: %v", err)
+			return nil, fmt.Errorf("parsing default browse template: %v", err)
 		}
 	}
 
-	fsrv.Browse.template = tpl
-
-	return nil
-}
-
-func (fsrv *FileServer) browseWriteJSON(listing browseTemplateContext) (*bytes.Buffer, error) {
-	buf := bufPool.Get().(*bytes.Buffer)
-	defer bufPool.Put(buf)
-	err := json.NewEncoder(buf).Encode(listing.Items)
-	return buf, err
-}
-
-func (fsrv *FileServer) browseWriteHTML(tplCtx *templateContext) (*bytes.Buffer, error) {
-	buf := bufPool.Get().(*bytes.Buffer)
-	defer bufPool.Put(buf)
-	err := fsrv.Browse.template.Execute(buf, tplCtx)
-	return buf, err
+	return tpl, nil
 }
 
 // isSymlink return true if f is a symbolic link
@@ -210,7 +207,7 @@ func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool {
 	if !isSymlink(f) {
 		return false
 	}
-	target := sanitizedPathJoin(root, path.Join(urlPath, f.Name()))
+	target := caddyhttp.SanitizedPathJoin(root, path.Join(urlPath, f.Name()))
 	targetInfo, err := os.Stat(target)
 	if err != nil {
 		return false
@@ -225,3 +222,10 @@ type templateContext struct {
 	templates.TemplateContext
 	browseTemplateContext
 }
+
+// bufPool is used to increase the efficiency of file listings.
+var bufPool = sync.Pool{
+	New: func() interface{} {
+		return new(bytes.Buffer)
+	},
+}

modules/caddyhttp/fileserver/browse_test.go

@@ -1,68 +0,0 @@
-// Copyright 2015 Matthew Holt and The Caddy Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package fileserver
-
-import (
-	"testing"
-	"text/template"
-)
-
-func BenchmarkBrowseWriteJSON(b *testing.B) {
-	fsrv := new(FileServer)
-	listing := browseTemplateContext{
-		Name:     "test",
-		Path:     "test",
-		CanGoUp:  false,
-		Items:    make([]fileInfo, 100),
-		NumDirs:  42,
-		NumFiles: 420,
-		Sort:     "",
-		Order:    "",
-		Limit:    42,
-	}
-	b.ResetTimer()
-
-	for n := 0; n < b.N; n++ {
-		fsrv.browseWriteJSON(listing)
-	}
-}
-
-func BenchmarkBrowseWriteHTML(b *testing.B) {
-	fsrv := new(FileServer)
-	fsrv.Browse = &Browse{
-		TemplateFile: "",
-		template:     template.New("test"),
-	}
-	listing := browseTemplateContext{
-		Name:     "test",
-		Path:     "test",
-		CanGoUp:  false,
-		Items:    make([]fileInfo, 100),
-		NumDirs:  42,
-		NumFiles: 420,
-		Sort:     "",
-		Order:    "",
-		Limit:    42,
-	}
-	tplCtx := &templateContext{
-		browseTemplateContext: listing,
-	}
-	fsrv.makeBrowseTemplate(tplCtx)
-	b.ResetTimer()
-
-	for n := 0; n < b.N; n++ {
-		fsrv.browseWriteHTML(tplCtx)
-	}
-}

modules/caddyhttp/fileserver/matcher.go

@@ -185,7 +185,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 		if strings.HasSuffix(file, "/") {
 			suffix += "/"
 		}
-		fullpath = sanitizedPathJoin(root, suffix)
+		fullpath = caddyhttp.SanitizedPathJoin(root, suffix)
 		return
 	}
 

modules/caddyhttp/fileserver/matcher_test.go

@@ -94,7 +94,7 @@ func TestFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}
@@ -197,7 +197,7 @@ func TestPHPFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}

modules/caddyhttp/fileserver/staticfiles.go

@@ -15,16 +15,15 @@
 package fileserver
 
 import (
-	"bytes"
 	"fmt"
 	weakrand "math/rand"
 	"mime"
 	"net/http"
 	"os"
+	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
@@ -162,7 +161,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	filesToHide := fsrv.transformHidePaths(repl)
 
 	root := repl.ReplaceAll(fsrv.Root, ".")
-	filename := sanitizedPathJoin(root, r.URL.Path)
+	filename := caddyhttp.SanitizedPathJoin(root, r.URL.Path)
 
 	fsrv.logger.Debug("sanitized path join",
 		zap.String("site_root", root),
@@ -178,7 +177,6 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 		} else if os.IsPermission(err) {
 			return caddyhttp.Error(http.StatusForbidden, err)
 		}
-		// TODO: treat this as resource exhaustion like with os.Open? Or unnecessary here?
 		return caddyhttp.Error(http.StatusInternalServerError, err)
 	}
 
@@ -187,7 +185,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	var implicitIndexFile bool
 	if info.IsDir() && len(fsrv.IndexNames) > 0 {
 		for _, indexPage := range fsrv.IndexNames {
-			indexPath := sanitizedPathJoin(filename, indexPage)
+			indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage)
 			if fileHidden(indexPath, filesToHide) {
 				// pretend this file doesn't exist
 				fsrv.logger.Debug("hiding index file",
@@ -243,14 +241,26 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	// trailing slash - not enforcing this can break relative hrefs
 	// in HTML (see https://github.com/caddyserver/caddy/issues/2741)
 	if fsrv.CanonicalURIs == nil || *fsrv.CanonicalURIs {
-		oldReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
-
-		if implicitIndexFile && !strings.HasSuffix(oldReq.URL.Path, "/") {
-			fsrv.logger.Debug("redirecting to canonical URI (adding trailing slash for directory)", zap.String("path", oldReq.URL.Path))
-			return redirect(w, r, oldReq.URL.Path+"/")
-		} else if !implicitIndexFile && strings.HasSuffix(oldReq.URL.Path, "/") {
-			fsrv.logger.Debug("redirecting to canonical URI (removing trailing slash for file)", zap.String("path", oldReq.URL.Path))
-			return redirect(w, r, oldReq.URL.Path[:len(oldReq.URL.Path)-1])
+		// Only redirect if the last element of the path (the filename) was not
+		// rewritten; if the admin wanted to rewrite to the canonical path, they
+		// would have, and we have to be very careful not to introduce unwanted
+		// redirects and especially redirect loops!
+		// See https://github.com/caddyserver/caddy/issues/4205.
+		origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
+		if path.Base(origReq.URL.Path) == path.Base(r.URL.Path) {
+			if implicitIndexFile && !strings.HasSuffix(origReq.URL.Path, "/") {
+				to := origReq.URL.Path + "/"
+				fsrv.logger.Debug("redirecting to canonical URI (adding trailing slash for directory)",
+					zap.String("from_path", origReq.URL.Path),
+					zap.String("to_path", to))
+				return redirect(w, r, to)
+			} else if !implicitIndexFile && strings.HasSuffix(origReq.URL.Path, "/") {
+				to := origReq.URL.Path[:len(origReq.URL.Path)-1]
+				fsrv.logger.Debug("redirecting to canonical URI (removing trailing slash for file)",
+					zap.String("from_path", origReq.URL.Path),
+					zap.String("to_path", to))
+				return redirect(w, r, to)
+			}
 		}
 	}
 
@@ -425,42 +435,6 @@ func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string {
 	return hide
 }
 
-// sanitizedPathJoin performs filepath.Join(root, reqPath) that
-// is safe against directory traversal attacks. It uses logic
-// similar to that in the Go standard library, specifically
-// in the implementation of http.Dir. The root is assumed to
-// be a trusted path, but reqPath is not.
-func sanitizedPathJoin(root, reqPath string) string {
-	// TODO: Caddy 1 uses this:
-	// prevent absolute path access on Windows, e.g. http://localhost:5000/C:\Windows\notepad.exe
-	// if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) {
-	// TODO.
-	// }
-
-	// TODO: whereas std lib's http.Dir.Open() uses this:
-	// if filepath.Separator != '/' && strings.ContainsRune(name, filepath.Separator) {
-	// 	return nil, errors.New("http: invalid character in file path")
-	// }
-
-	// TODO: see https://play.golang.org/p/oh77BiVQFti for another thing to consider
-
-	if root == "" {
-		root = "."
-	}
-
-	path := filepath.Join(root, filepath.Clean("/"+reqPath))
-
-	// filepath.Join also cleans the path, and cleaning strips
-	// the trailing slash, so we need to re-add it afterwards.
-	// if the length is 1, then it's a path to the root,
-	// and that should return ".", so we don't append the separator.
-	if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
-		path += separator
-	}
-
-	return path
-}
-
 // fileHidden returns true if filename is hidden according to the hide list.
 // filename must be a relative or absolute file system path, not a request
 // URI path. It is expected that all the paths in the hide list are absolute
@@ -557,12 +531,6 @@ func (wr statusOverrideResponseWriter) WriteHeader(int) {
 
 var defaultIndexNames = []string{"index.html", "index.txt"}
 
-var bufPool = sync.Pool{
-	New: func() interface{} {
-		return new(bytes.Buffer)
-	},
-}
-
 const (
 	minBackoff, maxBackoff = 2, 5
 	separator              = string(filepath.Separator)

modules/caddyhttp/fileserver/staticfiles_test.go

@@ -15,96 +15,12 @@
 package fileserver
 
 import (
-	"net/url"
 	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
 )
 
-func TestSanitizedPathJoin(t *testing.T) {
-	// For easy reference:
-	// %2e = .
-	// %2f = /
-	// %5c = \
-	for i, tc := range []struct {
-		inputRoot string
-		inputPath string
-		expect    string
-	}{
-		{
-			inputPath: "",
-			expect:    ".",
-		},
-		{
-			inputPath: "/",
-			expect:    ".",
-		},
-		{
-			inputPath: "/foo",
-			expect:    "foo",
-		},
-		{
-			inputPath: "/foo/",
-			expect:    "foo" + separator,
-		},
-		{
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("foo", "bar"),
-		},
-		{
-			inputRoot: "/a",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("/", "a", "foo", "bar"),
-		},
-		{
-			inputPath: "/foo/../bar",
-			expect:    "bar",
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/foo/../bar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/..%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b") + separator,
-		},
-		{
-			inputRoot: "C:\\www",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("C:\\www", "foo", "bar"),
-		},
-		// TODO: test more windows paths... on windows... sigh.
-	} {
-		// we don't *need* to use an actual parsed URL, but it
-		// adds some authenticity to the tests since real-world
-		// values will be coming in from URLs; thus, the test
-		// corpus can contain paths as encoded by clients, which
-		// more closely emulates the actual attack vector
-		u, err := url.Parse("http://test:9999" + tc.inputPath)
-		if err != nil {
-			t.Fatalf("Test %d: invalid URL: %v", i, err)
-		}
-		actual := sanitizedPathJoin(tc.inputRoot, u.Path)
-		if actual != tc.expect {
-			t.Errorf("Test %d: [%s %s] => %s (expected %s)",
-				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
-		}
-	}
-}
-
 func TestFileHidden(t *testing.T) {
 	for i, tc := range []struct {
 		inputHide []string

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -978,6 +978,18 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					h.KeepAlive = new(KeepAlive)
 				}
 				h.KeepAlive.MaxIdleConns = num
+
+			case "keepalive_idle_conns_per_host":
+				if !d.NextArg() {
+					return d.ArgErr()
+				}
+				num, err := strconv.Atoi(d.Val())
+				if err != nil {
+					return d.Errf("bad integer value '%s': %v", d.Val(), err)
+				}
+				if h.KeepAlive == nil {
+					h.KeepAlive = new(KeepAlive)
+				}
 				h.KeepAlive.MaxIdleConnsPerHost = num
 
 			case "versions":
@@ -1004,16 +1016,6 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				}
 				h.MaxConnsPerHost = num
 
-			case "max_idle_conns_per_host":
-				if !d.NextArg() {
-					return d.ArgErr()
-				}
-				num, err := strconv.Atoi(d.Val())
-				if err != nil {
-					return d.Errf("bad integer value '%s': %v", d.Val(), err)
-				}
-				h.MaxIdleConnsPerHost = num
-
 			default:
 				return d.Errf("unrecognized subdirective %s", d.Val())
 			}

modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -218,12 +217,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 	}
 
 	// SCRIPT_FILENAME is the absolute path of SCRIPT_NAME
-	scriptFilename := filepath.Join(root, scriptName)
-
-	// Add vhost path prefix to scriptName. Otherwise, some PHP software will
-	// have difficulty discovering its URL.
-	pathPrefix, _ := r.Context().Value(caddy.CtxKey("path_prefix")).(string)
-	scriptName = path.Join(pathPrefix, scriptName)
+	scriptFilename := caddyhttp.SanitizedPathJoin(root, scriptName)
 
 	// Ensure the SCRIPT_NAME has a leading slash for compliance with RFC3875
 	// Info: https://tools.ietf.org/html/rfc3875#section-4.1.13
@@ -236,13 +230,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 	// 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
-	origReq, ok := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
-	if !ok {
-		// some requests, like active health checks, don't add this to
-		// the request context, so we can just use the current URL
-		origReq = *r
-	}
-	reqURL := origReq.URL
+	origReq := r.Context().Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
 
 	requestScheme := "http"
 	if r.TLS != nil {
@@ -285,7 +273,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 		"DOCUMENT_ROOT":   root,
 		"DOCUMENT_URI":    docURI,
 		"HTTP_HOST":       r.Host, // added here, since not always part of headers
-		"REQUEST_URI":     reqURL.RequestURI(),
+		"REQUEST_URI":     origReq.URL.RequestURI(),
 		"SCRIPT_FILENAME": scriptFilename,
 		"SCRIPT_NAME":     scriptName,
 	}
@@ -294,7 +282,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 	// 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(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
+		env["PATH_TRANSLATED"] = caddyhttp.SanitizedPathJoin(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
 	}
 
 	// compliance with the CGI specification requires that

modules/caddyhttp/reverseproxy/httptransport.go

@@ -62,9 +62,6 @@ type HTTPTransport struct {
 	// Maximum number of connections per host. Default: 0 (no limit)
 	MaxConnsPerHost int `json:"max_conns_per_host,omitempty"`
 
-	// Maximum number of idle connections per host. Default: 0 (uses Go's default of 2)
-	MaxIdleConnsPerHost int `json:"max_idle_conns_per_host,omitempty"`
-
 	// How long to wait before timing out trying to connect to
 	// an upstream.
 	DialTimeout caddy.Duration `json:"dial_timeout,omitempty"`
@@ -197,7 +194,6 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 			return conn, nil
 		},
 		MaxConnsPerHost:        h.MaxConnsPerHost,
-		MaxIdleConnsPerHost:    h.MaxIdleConnsPerHost,
 		ResponseHeaderTimeout:  time.Duration(h.ResponseHeaderTimeout),
 		ExpectContinueTimeout:  time.Duration(h.ExpectContinueTimeout),
 		MaxResponseHeaderBytes: h.MaxResponseHeaderSize,
@@ -412,13 +408,13 @@ type KeepAlive struct {
 	// How often to probe for liveness.
 	ProbeInterval caddy.Duration `json:"probe_interval,omitempty"`
 
-	// Maximum number of idle connections.
+	// Maximum number of idle connections. Default: 0, which means no limit.
 	MaxIdleConns int `json:"max_idle_conns,omitempty"`
 
-	// Maximum number of idle connections per upstream host.
+	// Maximum number of idle connections per host. Default: 32.
 	MaxIdleConnsPerHost int `json:"max_idle_conns_per_host,omitempty"`
 
-	// How long connections should be kept alive when idle.
+	// How long connections should be kept alive when idle. Default: 0, which means no timeout.
 	IdleConnTimeout caddy.Duration `json:"idle_timeout,omitempty"`
 }
 

modules/caddyhttp/reverseproxy/reverseproxy.go

@@ -204,7 +204,7 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 			KeepAlive: &KeepAlive{
 				ProbeInterval:       caddy.Duration(30 * time.Second),
 				IdleConnTimeout:     caddy.Duration(2 * time.Minute),
-				MaxIdleConnsPerHost: 32,
+				MaxIdleConnsPerHost: 32, // seems about optimal, see #2805
 			},
 			DialTimeout: caddy.Duration(10 * time.Second),
 		}