Merge pull request #187 from basecamp/chunked-encoding-404s

Report malformed chunked requests as 400, not 500

Author: kevinmcconnell

internal/server/errors.go

@@ -0,0 +1,21 @@
+package server
+
+func isChunkedEncodingError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// The chunked encoding support in the stdlib returns these failures as plain
+	// errors using errors.New, so matching them means string matching on the
+	// error message, unfortunately.
+	switch err.Error() {
+	case "invalid byte in chunk length",
+		"http chunk length too large",
+		"malformed chunked encoding",
+		"trailer header without chunked transfer encoding",
+		"too many trailers":
+		return true
+	}
+
+	return false
+}

internal/server/request_buffer_middleware.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"errors"
 	"log/slog"
 	"net/http"
 )
@@ -22,11 +23,14 @@ func WithRequestBufferMiddleware(maxMemBytes, maxBytes int64, next http.Handler)
 func (h *RequestBufferMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	requestBuffer, err := NewBufferedReadCloser(r.Body, h.maxBytes, h.maxMemBytes)
 	if err != nil {
-		if err == ErrMaximumSizeExceeded {
-			http.Error(w, "Request too large", http.StatusRequestEntityTooLarge)
+		if errors.Is(err, ErrMaximumSizeExceeded) {
+			SetErrorResponse(w, r, http.StatusRequestEntityTooLarge, nil)
+		} else if isChunkedEncodingError(err) {
+			slog.Info("Malformed chunked request", "path", r.URL.Path, "error", err)
+			SetErrorResponse(w, r, http.StatusBadRequest, nil)
 		} else {
 			slog.Error("Error buffering request", "path", r.URL.Path, "error", err)
-			http.Error(w, "Internal Server Error", http.StatusInternalServerError)
+			SetErrorResponse(w, r, http.StatusInternalServerError, nil)
 		}
 		return
 	}

internal/server/request_buffer_middleware_test.go

@@ -1,12 +1,15 @@
 package server
 
 import (
+	"bufio"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestRequestBufferMiddleware(t *testing.T) {
@@ -35,3 +38,31 @@ func TestRequestBufferMiddleware(t *testing.T) {
 		assert.Equal(t, http.StatusRequestEntityTooLarge, w.Result().StatusCode)
 	})
 }
+
+func TestRequestBufferMiddleware_MalformedChunkedEncoding(t *testing.T) {
+	middleware := WithRequestBufferMiddleware(1024, 4096, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte("ok"))
+	}))
+
+	server := httptest.NewServer(middleware)
+	t.Cleanup(server.Close)
+
+	conn, err := net.Dial("tcp", server.Listener.Addr().String())
+	require.NoError(t, err)
+	defer conn.Close()
+
+	rawRequest := "POST / HTTP/1.1\r\n" +
+		"Host: example.com\r\n" +
+		"Transfer-Encoding: chunked\r\n" +
+		"\r\n" +
+		"ZZ\r\n"
+
+	_, err = conn.Write([]byte(rawRequest))
+	require.NoError(t, err)
+
+	resp, err := http.ReadResponse(bufio.NewReader(conn), nil)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+}

internal/server/response_buffer_middleware.go

@@ -2,6 +2,7 @@ package server
 
 import (
 	"bufio"
+	"errors"
 	"log/slog"
 	"net"
 	"net/http"
@@ -31,12 +32,12 @@ func (h *ResponseBufferMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Requ
 
 	err := responseWriter.Send()
 	if err != nil {
-		if err == ErrMaximumSizeExceeded {
+		if errors.Is(err, ErrMaximumSizeExceeded) {
 			slog.Info("Response exceeded max response limit", "path", r.URL.Path)
-			http.Error(w, "Internal Server Error", http.StatusInternalServerError)
+			SetErrorResponse(w, r, http.StatusInternalServerError, nil)
 		} else {
 			slog.Error("Error sending response", "path", r.URL.Path, "error", err)
-			http.Error(w, "Internal Server Error", http.StatusInternalServerError)
+			SetErrorResponse(w, r, http.StatusInternalServerError, nil)
 		}
 		return
 	}

internal/server/target.go

@@ -382,6 +382,12 @@ func (t *Target) handleProxyError(w http.ResponseWriter, r *http.Request, err er
 		return
 	}
 
+	if isChunkedEncodingError(err) {
+		slog.Info("Malformed request", "target", t.Address(), "path", r.URL.Path, "error", err)
+		SetErrorResponse(w, r, http.StatusBadRequest, nil)
+		return
+	}
+
 	slog.Error("Error while proxying", "target", t.Address(), "path", r.URL.Path, "error", err)
 	SetErrorResponse(w, r, http.StatusBadGateway, nil)
 }

internal/server/target_test.go

@@ -543,6 +543,43 @@ func TestTarget_EnforceMaxBodySizes(t *testing.T) {
 	})
 }
 
+func TestTarget_MalformedChunkedEncodingWithoutBuffering(t *testing.T) {
+	targetOptions := TargetOptions{
+		BufferRequests:    false,
+		HealthCheckConfig: defaultHealthCheckConfig,
+	}
+
+	target := testTargetWithOptions(t, targetOptions, func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte("ok"))
+	})
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		r, err := target.StartRequest(r)
+		require.NoError(t, err)
+		target.SendRequest(w, r)
+	}))
+	t.Cleanup(server.Close)
+
+	conn, err := net.Dial("tcp", server.Listener.Addr().String())
+	require.NoError(t, err)
+	defer conn.Close()
+
+	rawRequest := "POST / HTTP/1.1\r\n" +
+		"Host: example.com\r\n" +
+		"Transfer-Encoding: chunked\r\n" +
+		"\r\n" +
+		"ZZ\r\n"
+
+	_, err = conn.Write([]byte(rawRequest))
+	require.NoError(t, err)
+
+	resp, err := http.ReadResponse(bufio.NewReader(conn), nil)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+}
+
 func TestTarget_buildHealthCheckURL(t *testing.T) {
 	tests := []struct {
 		name            string