From 2c55babb33b778d1db0794cabc7a5c23702b772c Mon Sep 17 00:00:00 2001
From: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Date: Tue, 20 Aug 2024 20:14:56 +0200
Subject: [PATCH] proxy: handle invalid chunked requests (#3192)

Respond with 400 instead of 500 in case of invalid chunked request body.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
---
 proxy/proxy.go        | 16 +++++++--
 proxy/proxy_test.go   |  7 ++++
 proxy/request_test.go | 81 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 2 deletions(-)
 create mode 100644 proxy/request_test.go

diff --git a/proxy/proxy.go b/proxy/proxy.go
index 5553a2fdd5..0babad8da5 100644
--- a/proxy/proxy.go
+++ b/proxy/proxy.go
@@ -1050,11 +1050,12 @@ func (p *Proxy) makeBackendRequest(ctx *context, requestContext stdlibcontext.Co
 			}
 		}
 
-		ctx.proxySpan.LogKV("event", "error", "message", ensureUTF8(err.Error()))
+		errMessage := err.Error()
+		ctx.proxySpan.LogKV("event", "error", "message", ensureUTF8(errMessage))
+
 		if perr, ok := err.(*proxyError); ok {
 			perr.err = fmt.Errorf("failed to do backend roundtrip to %s: %w", req.URL.Host, perr.err)
 			return nil, perr
-
 		} else if nerr, ok := err.(net.Error); ok {
 			var status int
 			if nerr.Timeout() {
@@ -1067,6 +1068,17 @@ func (p *Proxy) makeBackendRequest(ctx *context, requestContext stdlibcontext.Co
 			return nil, &proxyError{err: fmt.Errorf("net.Error during backend roundtrip to %s: timeout=%v temporary='%v': %w", req.URL.Host, nerr.Timeout(), nerr.Temporary(), err), code: status}
 		}
 
+		switch errMessage {
+		case // net/http/internal/chunked.go
+			"header line too long",
+			"chunked encoding contains too much non-data",
+			"malformed chunked encoding",
+			"empty hex number for chunk length",
+			"invalid byte in chunk length",
+			"http chunk length too large":
+			return nil, &proxyError{code: http.StatusBadRequest, err: fmt.Errorf("failed to do backend roundtrip due to invalid request: %w", err)}
+		}
+
 		return nil, &proxyError{err: fmt.Errorf("unexpected error from Go stdlib net/http package during roundtrip: %w", err)}
 	}
 	p.tracing.setTag(ctx.proxySpan, HTTPStatusCodeTag, uint16(response.StatusCode))
diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
index b9c080b188..21bc5e0c03 100644
--- a/proxy/proxy_test.go
+++ b/proxy/proxy_test.go
@@ -107,6 +107,13 @@ func (l *testLog) String() string {
 	return l.buf.String()
 }
 
+func (l *testLog) Reset() {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	l.buf.Reset()
+}
+
 func (l *testLog) Close() {
 	log.SetOutput(l.oldOut)
 	log.SetLevel(l.oldLevel)
diff --git a/proxy/request_test.go b/proxy/request_test.go
new file mode 100644
index 0000000000..5620553a96
--- /dev/null
+++ b/proxy/request_test.go
@@ -0,0 +1,81 @@
+package proxy_test
+
+import (
+	"bufio"
+	"fmt"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/zalando/skipper/eskip"
+	"github.com/zalando/skipper/proxy"
+	"github.com/zalando/skipper/proxy/proxytest"
+)
+
+func TestRequestInvalidChunkedEncoding(t *testing.T) {
+	testLog := proxy.NewTestLog()
+	defer testLog.Close()
+
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+	defer backend.Close()
+
+	p := proxytest.Config{
+		Routes: eskip.MustParse(fmt.Sprintf(`* -> "%s"`, backend.URL)),
+	}.Create()
+	defer p.Close()
+
+	doChunkedRequest := func(t *testing.T, body string) *http.Response {
+		t.Helper()
+
+		conn, err := net.Dial("tcp", strings.TrimPrefix(p.URL, "http://"))
+		require.NoError(t, err)
+		defer conn.Close()
+
+		_, err = conn.Write([]byte("POST / HTTP/1.1\r\nHost: skipper.test\r\nTransfer-Encoding: chunked\r\n\r\n" + body))
+		require.NoError(t, err)
+
+		resp, err := http.ReadResponse(bufio.NewReader(conn), &http.Request{Method: "POST"})
+		require.NoError(t, err)
+		resp.Body.Close()
+
+		return resp
+	}
+
+	for _, tc := range []struct {
+		body             string
+		expectErrMessage string
+	}{
+		{
+			// "!" is an invalid byte in chunk length
+			body:             "!" + "\r\nabcd\r\n" + "0\r\n\r\n",
+			expectErrMessage: "invalid byte in chunk length",
+		},
+		{
+			// empty chunk length
+			body:             "" + "\r\nabcd\r\n" + "0\r\n\r\n",
+			expectErrMessage: "empty hex number for chunk length",
+		},
+		{
+			// missing \r\n after first chunk
+			body:             "4\r\nabcd" + "0\r\n\r\n",
+			expectErrMessage: "malformed chunked encoding",
+		},
+	} {
+		t.Run(tc.expectErrMessage, func(t *testing.T) {
+			testLog.Reset()
+
+			resp := doChunkedRequest(t, tc.body)
+			assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+
+			err := testLog.WaitFor("failed to do backend roundtrip due to invalid request: "+tc.expectErrMessage, 100*time.Millisecond)
+			if !assert.NoError(t, err) {
+				t.Logf("proxy log: %s", testLog.String())
+			}
+		})
+	}
+}