caddyhttp: Address some Go 1.20 features (#5288)

8 files changed, 90 insertions, 10 deletions

diff --git a/cmd/main.go b/cmd/main.go
index 31a121aa6..85b62fcc4 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -40,6 +40,18 @@ import (
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig"
+
+	// For running in minimal environments, this can ease
+	// headaches related to establishing TLS connections.
+	// "Package fallback embeds a set of fallback X.509 trusted
+	// roots in the application by automatically invoking
+	// x509.SetFallbackRoots. This allows the application to
+	// work correctly even if the operating system does not
+	// provide a verifier or system roots pool. ... It's
+	// recommended that only binaries, and not libraries,
+	// import this package. This package must be kept up to
+	// date for security and compatibility reasons."
+	_ "golang.org/x/crypto/x509roots/fallback"
 )
 
 func init() {
diff --git a/go.mod b/go.mod
index c3dc1568f..6c4fed3c4 100644
--- a/go.mod
+++ b/go.mod
@@ -36,6 +36,7 @@ require (
 	go.uber.org/zap v1.27.0
 	go.uber.org/zap/exp v0.2.0
 	golang.org/x/crypto v0.22.0
+	golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8
 	golang.org/x/net v0.24.0
 	golang.org/x/sync v0.7.0
 	golang.org/x/term v0.19.0
diff --git a/go.sum b/go.sum
index bd298867e..14434b24a 100644
--- a/go.sum
+++ b/go.sum
@@ -507,6 +507,8 @@ golang.org/x/crypto v0.3.0/go.mod h1:hebNnKkNXi2UzZN1eVRvBB7co0a+JxK6XbPiWVs/3J4
 golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU=
 golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30=
 golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M=
+golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8 h1:WsZ1vq1wEPrhLWtbblOEshIKDAbG0sYGgbYCFInT0QM=
+golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8/go.mod h1:kNa9WdvYnzFwC79zRpLRMJbdEFlhyM5RPFBBZp/wWH8=
 golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 h1:aAcj0Da7eBAtrTp03QXWvm88pSyOt+UgdZw2BFZ+lEw=
 golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8/go.mod h1:CQ1k9gNrJ50XIzaKCRR2hssIjF07kZFEiieALBM/ARQ=
 golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index f15aec5ed..f4dc5f37a 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -226,13 +226,22 @@ func StatusCodeMatches(actual, configured int) bool {
 // 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.
+// with the local file system. If root is empty, the current
+// directory is assumed. If the cleaned request path is not
+// lexically local, it will be rejected as unsafe and only
+// the root will be returned.
 func SanitizedPathJoin(root, reqPath string) string {
 	if root == "" {
 		root = "."
 	}
 
-	path := filepath.Join(root, path.Clean("/"+reqPath))
+	relPath := path.Clean("/" + reqPath)[1:] // clean path and trim the leading /
+	if !filepath.IsLocal(relPath) {
+		// path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885)
+		return root
+	}
+
+	path := filepath.Join(root, relPath)
 
 	// filepath.Join also cleans the path, and cleaning strips
 	// the trailing slash, so we need to re-add it afterwards.
diff --git a/modules/caddyhttp/caddyhttp_test.go b/modules/caddyhttp/caddyhttp_test.go
index 84c0271f1..8c55fcb7a 100644
--- a/modules/caddyhttp/caddyhttp_test.go
+++ b/modules/caddyhttp/caddyhttp_test.go
@@ -63,7 +63,7 @@ func TestSanitizedPathJoin(t *testing.T) {
 		{
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b") + separator,
+			expect:    filepath.Join("/", "a", "b"),
 		},
 		{
 			inputRoot: "/a/b",
@@ -85,6 +85,12 @@ func TestSanitizedPathJoin(t *testing.T) {
 			inputPath: "/D:\\foo\\bar",
 			expect:    filepath.Join("C:\\www", "D:\\foo\\bar"),
 		},
+		{
+			// https://github.com/golang/go/issues/56336#issuecomment-1416214885
+			inputRoot: "root",
+			inputPath: "/a/b/../../c",
+			expect:    filepath.Join("root", "c"),
+		},
 	} {
 		// we don't *need* to use an actual parsed URL, but it
 		// adds some authenticity to the tests since real-world
diff --git a/modules/caddyhttp/requestbody/caddyfile.go b/modules/caddyhttp/requestbody/caddyfile.go
index d2956cae0..8378ad7f4 100644
--- a/modules/caddyhttp/requestbody/caddyfile.go
+++ b/modules/caddyhttp/requestbody/caddyfile.go
@@ -15,6 +15,8 @@
 package requestbody
 
 import (
+	"time"
+
 	"github.com/dustin/go-humanize"
 
 	"github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile"
@@ -44,8 +46,30 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 			}
 			rb.MaxSize = int64(size)
 
+		case "read_timeout":
+			var timeoutStr string
+			if !h.AllArgs(&timeoutStr) {
+				return nil, h.ArgErr()
+			}
+			timeout, err := time.ParseDuration(timeoutStr)
+			if err != nil {
+				return nil, h.Errf("parsing read_timeout: %v", err)
+			}
+			rb.ReadTimeout = timeout
+
+		case "write_timeout":
+			var timeoutStr string
+			if !h.AllArgs(&timeoutStr) {
+				return nil, h.ArgErr()
+			}
+			timeout, err := time.ParseDuration(timeoutStr)
+			if err != nil {
+				return nil, h.Errf("parsing write_timeout: %v", err)
+			}
+			rb.WriteTimeout = timeout
+
 		default:
-			return nil, h.Errf("unrecognized servers option '%s'", h.Val())
+			return nil, h.Errf("unrecognized request_body subdirective '%s'", h.Val())
 		}
 	}
 
diff --git a/modules/caddyhttp/requestbody/requestbody.go b/modules/caddyhttp/requestbody/requestbody.go
index dfc0fd928..3cea7b4f9 100644
--- a/modules/caddyhttp/requestbody/requestbody.go
+++ b/modules/caddyhttp/requestbody/requestbody.go
@@ -17,9 +17,11 @@ package requestbody
 import (
 	"io"
 	"net/http"
+	"time"
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
+	"go.uber.org/zap"
 )
 
 func init() {
@@ -31,6 +33,14 @@ type RequestBody struct {
 	// The maximum number of bytes to allow reading from the body by a later handler.
 	// If more bytes are read, an error with HTTP status 413 is returned.
 	MaxSize int64 `json:"max_size,omitempty"`
+
+	// EXPERIMENTAL. Subject to change/removal.
+	ReadTimeout time.Duration `json:"read_timeout,omitempty"`
+
+	// EXPERIMENTAL. Subject to change/removal.
+	WriteTimeout time.Duration `json:"write_timeout,omitempty"`
+
+	logger *zap.Logger
 }
 
 // CaddyModule returns the Caddy module information.
@@ -41,6 +51,11 @@ func (RequestBody) CaddyModule() caddy.ModuleInfo {
 	}
 }
 
+func (rb *RequestBody) Provision(ctx caddy.Context) error {
+	rb.logger = ctx.Logger()
+	return nil
+}
+
 func (rb RequestBody) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
 	if r.Body == nil {
 		return next.ServeHTTP(w, r)
@@ -48,6 +63,19 @@ func (rb RequestBody) ServeHTTP(w http.ResponseWriter, r *http.Request, next cad
 	if rb.MaxSize > 0 {
 		r.Body = errorWrapper{http.MaxBytesReader(w, r.Body, rb.MaxSize)}
 	}
+	if rb.ReadTimeout > 0 || rb.WriteTimeout > 0 {
+		rc := http.NewResponseController(w)
+		if rb.ReadTimeout > 0 {
+			if err := rc.SetReadDeadline(time.Now().Add(rb.ReadTimeout)); err != nil {
+				rb.logger.Error("could not set read deadline", zap.Error(err))
+			}
+		}
+		if rb.WriteTimeout > 0 {
+			if err := rc.SetWriteDeadline(time.Now().Add(rb.WriteTimeout)); err != nil {
+				rb.logger.Error("could not set write deadline", zap.Error(err))
+			}
+		}
+	}
 	return next.ServeHTTP(w, r)
 }
 
diff --git a/modules/caddyhttp/responsewriter_test.go b/modules/caddyhttp/responsewriter_test.go
index 492fcad9e..c08ad26a4 100644
--- a/modules/caddyhttp/responsewriter_test.go
+++ b/modules/caddyhttp/responsewriter_test.go
@@ -2,7 +2,6 @@ package caddyhttp
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 	"net/http"
 	"strings"
@@ -75,20 +74,19 @@ func TestResponseWriterWrapperReadFrom(t *testing.T) {
 			// take precedence over our ReadFrom.
 			src := struct{ io.Reader }{strings.NewReader(srcData)}
 
-			fmt.Println(name)
 			if _, err := io.Copy(wrapped, src); err != nil {
-				t.Errorf("Copy() err = %v", err)
+				t.Errorf("%s: Copy() err = %v", name, err)
 			}
 
 			if got := tt.responseWriter.Written(); got != srcData {
-				t.Errorf("data = %q, want %q", got, srcData)
+				t.Errorf("%s: data = %q, want %q", name, got, srcData)
 			}
 
 			if tt.responseWriter.CalledReadFrom() != tt.wantReadFrom {
 				if tt.wantReadFrom {
-					t.Errorf("ReadFrom() should have been called")
+					t.Errorf("%s: ReadFrom() should have been called", name)
 				} else {
-					t.Errorf("ReadFrom() should not have been called")
+					t.Errorf("%s: ReadFrom() should not have been called", name)
 				}
 			}
 		})