caddyhttp: Address some Go 1.20 features (#6252)

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

8 files changed, 125 insertions, 16 deletions

diff --git a/cmd/x509rootsfallback.go b/cmd/x509rootsfallback.go
new file mode 100644
index 00000000..935a48ec
--- /dev/null
+++ b/cmd/x509rootsfallback.go
@@ -0,0 +1,33 @@
+// 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 caddycmd
+
+import (
+	// 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."
+	//
+	// This is in its own file only because of conflicts
+	// between gci and goimports when in main.go.
+	// See https://github.com/daixiang0/gci/issues/76
+	_ "golang.org/x/crypto/x509roots/fallback"
+)
diff --git a/go.mod b/go.mod
index c3dc1568..6c4fed3c 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 bd298867..14434b24 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 f15aec5e..26b21b54 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 deemed
+// not local according to lexical processing (i.e. ignoring links),
+// 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, filepath.FromSlash(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 84c0271f..4763c383 100644
--- a/modules/caddyhttp/caddyhttp_test.go
+++ b/modules/caddyhttp/caddyhttp_test.go
@@ -3,6 +3,7 @@ package caddyhttp
 import (
 	"net/url"
 	"path/filepath"
+	"runtime"
 	"testing"
 )
 
@@ -12,9 +13,10 @@ func TestSanitizedPathJoin(t *testing.T) {
 	// %2f = /
 	// %5c = \
 	for i, tc := range []struct {
-		inputRoot string
-		inputPath string
-		expect    string
+		inputRoot     string
+		inputPath     string
+		expect        string
+		expectWindows string
 	}{
 		{
 			inputPath: "",
@@ -63,7 +65,7 @@ func TestSanitizedPathJoin(t *testing.T) {
 		{
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b") + separator,
+			expect:    "/a/b", // inputPath fails the IsLocal test so only the root is returned
 		},
 		{
 			inputRoot: "/a/b",
@@ -81,9 +83,16 @@ func TestSanitizedPathJoin(t *testing.T) {
 			expect:    filepath.Join("C:\\www", "foo", "bar"),
 		},
 		{
-			inputRoot: "C:\\www",
-			inputPath: "/D:\\foo\\bar",
-			expect:    filepath.Join("C:\\www", "D:\\foo\\bar"),
+			inputRoot:     "C:\\www",
+			inputPath:     "/D:\\foo\\bar",
+			expect:        filepath.Join("C:\\www", "D:\\foo\\bar"),
+			expectWindows: filepath.Join("C:\\www"), // inputPath fails IsLocal on Windows
+		},
+		{
+			// 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
@@ -96,6 +105,9 @@ func TestSanitizedPathJoin(t *testing.T) {
 			t.Fatalf("Test %d: invalid URL: %v", i, err)
 		}
 		actual := SanitizedPathJoin(tc.inputRoot, u.Path)
+		if runtime.GOOS == "windows" && tc.expectWindows != "" {
+			tc.expect = tc.expectWindows
+		}
 		if actual != tc.expect {
 			t.Errorf("Test %d: SanitizedPathJoin('%s', '%s') =>  '%s' (expected '%s')",
 				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
diff --git a/modules/caddyhttp/requestbody/caddyfile.go b/modules/caddyhttp/requestbody/caddyfile.go
index d2956cae..8378ad7f 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 dfc0fd92..d00455a8 100644
--- a/modules/caddyhttp/requestbody/requestbody.go
+++ b/modules/caddyhttp/requestbody/requestbody.go
@@ -17,6 +17,9 @@ package requestbody
 import (
 	"io"
 	"net/http"
+	"time"
+
+	"go.uber.org/zap"
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
@@ -31,6 +34,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 +52,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 +64,20 @@ 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 {
+		//nolint:bodyclose
+		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 492fcad9..c08ad26a 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)
 				}
 			}
 		})