Prevent unbounded request body reads

docs/Webhook-Parameters.md

@@ -23,6 +23,8 @@ Usage of webhook:
         list available TLS cipher suites
   -logfile string
         send log output to a file; implicitly enables verbose logging
+  -max-body-size int
+        maximum request body size in bytes, use 0 for unlimited (default 0)
   -max-multipart-mem int
         maximum memory in bytes for parsing multipart form data before disk caching (default 1048576)
   -nopanic
@@ -57,6 +59,8 @@ Usage of webhook:
 
 Use any of the above specified flags to override their default behavior.
 
+By default, `-max-body-size` is `0`, which preserves historical behavior by allowing unlimited request body reads. For production deployments, set `-max-body-size` to a positive byte value appropriate for your expected webhook payloads. This default may change in a future major release.
+
 # Live reloading hooks
 If you are running an OS that supports the HUP or USR1 signal, you can use it to trigger hooks reload from hooks file, without restarting the webhook instance.
 ```bash

webhook.go

@@ -3,6 +3,7 @@ package main
 import (
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"io/ioutil"
@@ -26,6 +27,8 @@ import (
 
 const (
 	version = "2.8.3"
+	// Keep the default unlimited to preserve existing deployments.
+	defaultMaxBodySize int64 = 0
 )
 
 var (
@@ -47,6 +50,7 @@ var (
 	tlsCipherSuites    = flag.String("cipher-suites", "", "comma-separated list of supported TLS cipher suites")
 	useXRequestID      = flag.Bool("x-request-id", false, "use X-Request-Id header, if present, as request ID")
 	xRequestIDLimit    = flag.Int("x-request-id-limit", 0, "truncate X-Request-Id header to limit; default no limit")
+	maxBodySize        = flag.Int64("max-body-size", defaultMaxBodySize, "maximum request body size in bytes, use 0 for unlimited")
 	maxMultipartMem    = flag.Int64("max-multipart-mem", 1<<20, "maximum memory in bytes for parsing multipart form data before disk caching")
 	httpMethods        = flag.String("http-methods", "", `set default allowed HTTP methods (ie. "POST"); separate methods with comma`)
 	pidPath            = flag.String("pidfile", "", "create PID file at the given path")
@@ -112,6 +116,11 @@ func main() {
 		os.Exit(1)
 	}
 
+	if *maxBodySize < 0 {
+		fmt.Println("error: max-body-size must be greater than or equal to 0")
+		os.Exit(1)
+	}
+
 	if *debug || *logPath != "" {
 		*verbose = true
 	}
@@ -171,6 +180,10 @@ func main() {
 		os.Exit(1)
 	}
 
+	if *maxBodySize == 0 {
+		log.Println("warn: request body size is unlimited; set -max-body-size to a positive byte value to limit request body reads")
+	}
+
 	if !*verbose {
 		log.SetOutput(ioutil.Discard)
 	}
@@ -368,6 +381,10 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set(responseHeader.Name, responseHeader.Value)
 	}
 
+	if *maxBodySize > 0 {
+		r.Body = http.MaxBytesReader(w, r.Body, *maxBodySize)
+	}
+
 	var err error
 
 	// set contentType to IncomingPayloadContentType or header value
@@ -381,7 +398,16 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 	if !isMultipart {
 		req.Body, err = ioutil.ReadAll(r.Body)
 		if err != nil {
+			if isRequestBodyTooLarge(err) {
+				w.WriteHeader(http.StatusRequestEntityTooLarge)
+				fmt.Fprint(w, "Request body too large.")
+				return
+			}
+
 			log.Printf("[%s] error reading the request body: %+v\n", req.ID, err)
+			w.WriteHeader(http.StatusInternalServerError)
+			fmt.Fprint(w, "Error occurred while reading the request body.")
+			return
 		}
 	}
 
@@ -410,6 +436,12 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 	case isMultipart:
 		err = r.ParseMultipartForm(*maxMultipartMem)
 		if err != nil {
+			if isRequestBodyTooLarge(err) {
+				w.WriteHeader(http.StatusRequestEntityTooLarge)
+				fmt.Fprint(w, "Request body too large.")
+				return
+			}
+
 			msg := fmt.Sprintf("[%s] error parsing multipart form: %+v\n", req.ID, err)
 			log.Println(msg)
 			w.WriteHeader(http.StatusInternalServerError)
@@ -654,6 +686,11 @@ func handleHook(h *hook.Hook, r *hook.Request) (string, error) {
 	return string(out), err
 }
 
+func isRequestBodyTooLarge(err error) bool {
+	var maxBytesErr *http.MaxBytesError
+	return errors.As(err, &maxBytesErr)
+}
+
 func writeHttpResponseCode(w http.ResponseWriter, rid, hookId string, responseCode int) {
 	// Check if the given return code is supported by the http package
 	// by testing if there is a StatusText for this code.

webhook_test.go

@@ -198,6 +198,152 @@ func TestWebhook(t *testing.T) {
 	}
 }
 
+func TestWebhookMaxBodySize(t *testing.T) {
+	hookecho, cleanupHookecho := buildHookecho(t)
+	defer cleanupHookecho()
+
+	webhook, cleanupWebhookFn := buildWebhook(t)
+	defer cleanupWebhookFn()
+
+	configPath, cleanupConfigFn := genConfig(t, hookecho, "test/hooks.json.tmpl")
+	defer cleanupConfigFn()
+
+	t.Run("rejects negative limit at startup", func(t *testing.T) {
+		cmd := exec.Command(webhook,
+			fmt.Sprintf("-hooks=%s", configPath),
+			"-max-body-size=-1",
+		)
+		cmd.Env = webhookEnv()
+		cmd.Args[0] = "webhook"
+
+		output, err := cmd.CombinedOutput()
+		if err == nil {
+			t.Fatalf("expected webhook to reject negative max body size, got success\noutput:\n%s", output)
+		}
+
+		if !strings.Contains(string(output), "error: max-body-size must be greater than or equal to 0") {
+			t.Fatalf("expected negative max body size error, got:\n%s", output)
+		}
+	})
+
+	t.Run("default is unlimited and warns", func(t *testing.T) {
+		ip, port := serverAddress(t)
+		authority := fmt.Sprintf("%s:%s", ip, port)
+		logs, stop := startWebhookForMaxBodyTest(t, webhook, configPath, ip, port, "")
+		defer stop()
+
+		body := fmt.Sprintf(`{"payload":%q}`, strings.Repeat("a", 33*1024*1024))
+		req, err := http.NewRequest("POST", fmt.Sprintf("http://%s/hooks/github", authority), strings.NewReader(body))
+		if err != nil {
+			t.Fatalf("New request failed: %s", err)
+		}
+		req.Header.Set("Content-Type", "application/json")
+
+		res, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatalf("client.Do failed: %s\nlogs:\n%s", err, logs)
+		}
+		defer res.Body.Close()
+
+		if res.StatusCode == http.StatusRequestEntityTooLarge {
+			t.Fatalf("expected default max body size to be unlimited, got status %d\nlogs:\n%s", res.StatusCode, logs)
+		}
+
+		if !strings.Contains(logs.String(), "warn: request body size is unlimited") {
+			t.Fatalf("expected unlimited body size warning in logs, got:\n%s", logs)
+		}
+	})
+
+	t.Run("rejects oversized body", func(t *testing.T) {
+		ip, port := serverAddress(t)
+		authority := fmt.Sprintf("%s:%s", ip, port)
+		logs, stop := startWebhookForMaxBodyTest(t, webhook, configPath, ip, port, "16")
+		defer stop()
+
+		req, err := http.NewRequest("POST", fmt.Sprintf("http://%s/hooks/github", authority), strings.NewReader(`{"payload":"too large"}`))
+		if err != nil {
+			t.Fatalf("New request failed: %s", err)
+		}
+		req.Header.Set("Content-Type", "application/json")
+
+		res, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatalf("client.Do failed: %s\nlogs:\n%s", err, logs)
+		}
+		defer res.Body.Close()
+
+		body, err := ioutil.ReadAll(res.Body)
+		if err != nil {
+			t.Fatalf("failed to read body: %s", err)
+		}
+
+		if res.StatusCode != http.StatusRequestEntityTooLarge || string(body) != "Request body too large." {
+			t.Fatalf("expected status %d and body %q, got status %d and body %q\nlogs:\n%s", http.StatusRequestEntityTooLarge, "Request body too large.", res.StatusCode, body, logs)
+		}
+	})
+
+	t.Run("allows body within limit", func(t *testing.T) {
+		ip, port := serverAddress(t)
+		authority := fmt.Sprintf("%s:%s", ip, port)
+		logs, stop := startWebhookForMaxBodyTest(t, webhook, configPath, ip, port, "1048576")
+		defer stop()
+
+		tt := hookHandlerTests[0]
+		req, err := http.NewRequest(tt.method, fmt.Sprintf("http://%s/hooks/%s", authority, tt.id), strings.NewReader(tt.body))
+		if err != nil {
+			t.Fatalf("New request failed: %s", err)
+		}
+		for k, v := range tt.headers {
+			req.Header.Add(k, v)
+		}
+		req.Header.Set("Content-Type", tt.contentType)
+		req.ContentLength = int64(len(tt.body))
+
+		res, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatalf("client.Do failed: %s\nlogs:\n%s", err, logs)
+		}
+		defer res.Body.Close()
+
+		body, err := ioutil.ReadAll(res.Body)
+		if err != nil {
+			t.Fatalf("failed to read body: %s", err)
+		}
+
+		matched, _ := regexp.Match(tt.respBody, body)
+		if res.StatusCode != tt.respStatus || !matched {
+			t.Fatalf("expected status %d and body matching %q, got status %d and body %q\nlogs:\n%s", tt.respStatus, tt.respBody, res.StatusCode, body, logs)
+		}
+	})
+}
+
+func startWebhookForMaxBodyTest(t *testing.T, webhook, configPath, ip, port, maxBodySize string) (*buffer, func()) {
+	t.Helper()
+
+	logs := &buffer{}
+	args := []string{
+		fmt.Sprintf("-hooks=%s", configPath),
+		fmt.Sprintf("-ip=%s", ip),
+		fmt.Sprintf("-port=%s", port),
+	}
+	if maxBodySize != "" {
+		args = append(args, fmt.Sprintf("-max-body-size=%s", maxBodySize))
+	}
+
+	cmd := exec.Command(webhook, args...)
+	cmd.Stderr = logs
+	cmd.Env = webhookEnv()
+	cmd.Args[0] = "webhook"
+
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("failed to start webhook: %s", err)
+	}
+
+	waitForServerReady(t, fmt.Sprintf("%s:%s", ip, port), http.DefaultClient)
+
+	return logs, func() { killAndWait(cmd) }
+}
+
 func buildHookecho(t *testing.T) (binPath string, cleanupFn func()) {
 	tmp, err := ioutil.TempDir("", "hookecho-test-")
 	if err != nil {