Limit the request body kept for logging

internal/runner/options.go

@@ -74,7 +74,7 @@ func ParseOptions() *Options {
 		flagSet.BoolVar(&options.Sandbox, "sandbox", false, "Enable sandbox mode"),
 		flagSet.BoolVar(&options.HTTP1Only, "http1", false, "Enable only HTTP1"),
 		flagSet.IntVar(&options.MaxFileSize, "max-file-size", 50, "Max Upload File Size"),
-		flagSet.IntVar(&options.MaxDumpBodySize, "max-dump-body-size", -1, "Max Dump Body Size"),
+		flagSet.IntVar(&options.MaxDumpBodySize, "max-dump-body-size", -1, "Max Dump Body Size in MB (request bodies are logged up to 1 MB when not set)"),
 		flagSet.BoolVar(&options.Python, "py", false, "Emulate Python Style"),
 		flagSet.BoolVar(&options.CORS, "cors", false, "Enable Cross-Origin Resource Sharing (CORS)"),
 		flagSet.Var(&options.HTTPHeaders, "header", "Add HTTP Response Header (name: value), can be used multiple times"),

pkg/httpserver/loglayer.go

@@ -2,6 +2,7 @@ package httpserver
 
 import (
 	"bytes"
+	"io"
 	"net/http"
 	"net/http/httputil"
 	"time"
@@ -16,17 +17,40 @@ var (
 	EnableLogUA   bool
 )
 
-func (t *HTTPServer) shouldDumpBody(bodysize int64) bool {
-	return t.options.MaxDumpBodySize > 0 && bodysize > t.options.MaxDumpBodySize
+// defaultMaxDumpBodySize bounds the request body kept for logging when
+// -max-dump-body-size is not set.
+const defaultMaxDumpBodySize = 1024 * 1024
+
+func (t *HTTPServer) maxDumpBodySize() int64 {
+	if t.options.MaxDumpBodySize > 0 {
+		return t.options.MaxDumpBodySize
+	}
+	return defaultMaxDumpBodySize
+}
+
+// dumpRequest dumps r with at most maxDumpBodySize bytes of its body. The
+// loglayer runs before authentication, so the size kept must not depend on
+// Content-Length (absent for chunked bodies). The bytes read are put back in
+// front of the body, so the next handlers still read all of it.
+func (t *HTTPServer) dumpRequest(r *http.Request) []byte {
+	dump, _ := httputil.DumpRequest(r, false)
+	if r.Body == nil || r.Body == http.NoBody {
+		return dump
+	}
+	prefix, _ := io.ReadAll(io.LimitReader(r.Body, t.maxDumpBodySize()))
+	r.Body = struct {
+		io.Reader
+		io.Closer
+	}{io.MultiReader(bytes.NewReader(prefix), r.Body), r.Body}
+	return append(dump, prefix...)
 }
 
 func (t *HTTPServer) loglayer(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// the body is read only when a log needs it
 		var fullRequest []byte
-		if t.shouldDumpBody(r.ContentLength) {
-			fullRequest, _ = httputil.DumpRequest(r, false)
-		} else {
-			fullRequest, _ = httputil.DumpRequest(r, true)
+		if EnableVerbose || t.jsonLogger != nil {
+			fullRequest = t.dumpRequest(r)
 		}
 		lrw := newLoggingResponseWriter(w, t.options.MaxDumpBodySize)
 		handler.ServeHTTP(lrw, r)

pkg/httpserver/loglayer_test.go

@@ -0,0 +1,73 @@
+package httpserver
+
+import (
+	"bytes"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+type countingBody struct {
+	r    io.Reader
+	read int64
+}
+
+func (c *countingBody) Read(p []byte) (int, error) {
+	n, err := c.r.Read(p)
+	c.read += int64(n)
+	return n, err
+}
+
+func (c *countingBody) Close() error { return nil }
+
+// TestLogLayerBoundsChunkedBody checks that a body without Content-Length
+// (chunked) is not read past the dump limit before the next handler runs,
+// and that the next handler still gets the whole body.
+func TestLogLayerBoundsChunkedBody(t *testing.T) {
+	EnableVerbose = true
+	defer func() { EnableVerbose = false }()
+
+	const limit = 1024
+	payload := bytes.Repeat([]byte("A"), 1<<20)
+	body := &countingBody{r: bytes.NewReader(payload)}
+
+	server := &HTTPServer{options: &Options{MaxDumpBodySize: limit}}
+	var readBeforeHandler int64
+	var got []byte
+	handler := server.loglayer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		readBeforeHandler = body.read
+		got, _ = io.ReadAll(r.Body)
+		w.WriteHeader(http.StatusUnauthorized)
+	}))
+
+	req := httptest.NewRequest(http.MethodPost, "http://example.com/", nil)
+	req.Body = body
+	req.ContentLength = -1
+	req.TransferEncoding = []string{"chunked"}
+	handler.ServeHTTP(httptest.NewRecorder(), req)
+
+	if readBeforeHandler > limit {
+		t.Fatalf("loglayer read %d bytes before the next handler, limit %d", readBeforeHandler, limit)
+	}
+	if !bytes.Equal(got, payload) {
+		t.Fatalf("next handler got %d bytes, want %d", len(got), len(payload))
+	}
+}
+
+// TestLogLayerSkipsBodyWithoutLog checks that the body is not read when no
+// log needs it.
+func TestLogLayerSkipsBodyWithoutLog(t *testing.T) {
+	body := &countingBody{r: bytes.NewReader(bytes.Repeat([]byte("A"), 4096))}
+	server := &HTTPServer{options: &Options{}}
+	var readBeforeHandler int64 = -1
+	handler := server.loglayer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		readBeforeHandler = body.read
+	}))
+	req := httptest.NewRequest(http.MethodPost, "http://example.com/", nil)
+	req.Body = body
+	handler.ServeHTTP(httptest.NewRecorder(), req)
+	if readBeforeHandler != 0 {
+		t.Fatalf("loglayer read %d bytes with no log enabled", readBeforeHandler)
+	}
+}