diff --git a/changes/improve-body-validation b/changes/improve-body-validation new file mode 100644 index 0000000000..c62adbe474 --- /dev/null +++ b/changes/improve-body-validation @@ -0,0 +1 @@ +* Improved body parsing validation by using http.MaxBytesReader and wrapping gzip decode output too. diff --git a/server/platform/endpointer/endpoint_utils.go b/server/platform/endpointer/endpoint_utils.go index 5bdc224df5..b013edd3ef 100644 --- a/server/platform/endpointer/endpoint_utils.go +++ b/server/platform/endpointer/endpoint_utils.go @@ -32,17 +32,6 @@ import ( "github.com/gorilla/mux" ) -// We use our own wrapper here, to preserve the Close method of the original io.ReadCloser -// But also allows us to modify the limit at a laterp oint. -type LimitedReadCloser struct { - *io.LimitedReader - Closer io.Closer -} - -func (lrc *LimitedReadCloser) Close() error { - return lrc.Closer.Close() -} - type HandlerRoutesFunc func(r *mux.Router, opts []kithttp.ServerOption) // ParseTag parses a `url` tag and whether it's optional or not, which is an optional part of the tag @@ -501,23 +490,7 @@ type requestValidator interface { // The customQueryDecoder parameter allows services to inject domain-specific // query parameter decoding logic. // -// If adding a new way to parse/decode the requset, make sure to wrap the body in a limited reader with the maxRequestBodySize - -// limitExhaustedBody returns true when the LimitedReader was the cause of an -// unexpected EOF — i.e. the underlying body actually had more data beyond the -// limit. It does this by attempting a single-byte read from the underlying -// reader (limitedReader.R) which bypasses the limit wrapper. A successful read -// means the body exceeded the limit; an immediate EOF means the body ended -// exactly at the limit (malformed JSON, not an oversized payload). -// -// This is used to avoid false-positive PayloadTooLargeError responses for -// bodies whose JSON is malformed and happen to be exactly maxRequestBodySize -// bytes long. -func limitExhaustedBody(limitedReader *io.LimitedReader) bool { - var peek [1]byte - n, _ := limitedReader.R.Read(peek[:]) - return n > 0 -} +// If adding a new way to parse/decode the request, make sure to wrap the body with http.MaxBytesReader using the maxRequestBodySize func MakeDecoder( iface interface{}, @@ -537,18 +510,48 @@ func MakeDecoder( } if rd, ok := iface.(RequestDecoder); ok { return func(ctx context.Context, r *http.Request) (interface{}, error) { - var limitedReader *io.LimitedReader if maxRequestBodySize != -1 { - limitedReader = io.LimitReader(r.Body, maxRequestBodySize).(*io.LimitedReader) - - r.Body = &LimitedReadCloser{ - LimitedReader: limitedReader, - Closer: r.Body, + r.Body = http.MaxBytesReader(nil, r.Body, maxRequestBodySize) + } + // + // We take care of gzip encoding here to prevent any future DecodeRequest + // implementations from missing gzip bomb checks. + // + gzipped := false + if strings.EqualFold(r.Header.Get("content-encoding"), "gzip") { + gzipped = true + gzr, err := gzip.NewReader(r.Body) + if err != nil { + return nil, BadRequestErr("gzip decoder error", err) } + defer gzr.Close() + if maxRequestBodySize != -1 { + // Limit decompressed bytes to prevent gzip bombs from bypassing + // the raw body size limit applied above. + r.Body = http.MaxBytesReader(nil, gzr, maxRequestBodySize) + } else { + r.Body = io.NopCloser(gzr) + } + // Clear so implementations don't try to decompress again. + r.Header.Del("Content-Encoding") } ret, err := rd.DecodeRequest(ctx, r) - if err != nil && errors.Is(err, io.ErrUnexpectedEOF) && limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) { - return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize} + + // Some DecodeRequest implementations (like getHostSoftwareRequest) + // themselves return platform_http.PayloadTooLargeError. + if inner, isPayloadTooLargeError := errors.AsType[platform_http.PayloadTooLargeError](err); isPayloadTooLargeError { + // Preserve the inner error's MaxRequestSize and ContentLength + // (it knows the actual limit that was hit), only add Gzipped. + inner.Gzipped = gzipped + return nil, inner + } + + if _, isMaxBytesError := errors.AsType[*http.MaxBytesError](err); isMaxBytesError { + return nil, platform_http.PayloadTooLargeError{ + ContentLength: r.Header.Get("Content-Length"), + MaxRequestSize: maxRequestBodySize, + Gzipped: gzipped, + } } return ret, err } @@ -564,28 +567,30 @@ func MakeDecoder( nilBody := false var rewriter *JSONKeyRewriteReader - var limitedReader *io.LimitedReader if maxRequestBodySize != -1 { - limitedReader = io.LimitReader(r.Body, maxRequestBodySize).(*io.LimitedReader) - - r.Body = &LimitedReadCloser{ - LimitedReader: limitedReader, - Closer: r.Body, - } + r.Body = http.MaxBytesReader(nil, r.Body, maxRequestBodySize) } buf := bufio.NewReader(r.Body) var body io.Reader = buf + gzipped := false if _, err := buf.Peek(1); err == io.EOF { nilBody = true } else { - if r.Header.Get("content-encoding") == "gzip" { + if strings.EqualFold(r.Header.Get("content-encoding"), "gzip") { + gzipped = true gzr, err := gzip.NewReader(buf) if err != nil { return nil, BadRequestErr("gzip decoder error", err) } defer gzr.Close() - body = gzr + if maxRequestBodySize != -1 { + // Limit decompressed bytes to prevent gzip bombs from bypassing + // the raw body size limit applied above. + body = http.MaxBytesReader(nil, gzr, maxRequestBodySize) + } else { + body = gzr + } } // Insert the JSON key rewriter into the reader pipeline @@ -610,11 +615,13 @@ func MakeDecoder( InternalErr: ace, } } - - if errors.Is(err, io.ErrUnexpectedEOF) && limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) { - return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize} + if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + return nil, platform_http.PayloadTooLargeError{ + ContentLength: r.Header.Get("Content-Length"), + MaxRequestSize: maxRequestBodySize, + Gzipped: gzipped, + } } - return nil, BadRequestErr("json decoder error", err) } v = reflect.ValueOf(req) @@ -682,10 +689,14 @@ func MakeDecoder( } } - if errors.Is(err, io.ErrUnexpectedEOF) { - if limitedReader != nil && limitedReader.N == 0 && limitExhaustedBody(limitedReader) { - return nil, platform_http.PayloadTooLargeError{ContentLength: r.Header.Get("Content-Length"), MaxRequestSize: maxRequestBodySize} + if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + return nil, platform_http.PayloadTooLargeError{ + ContentLength: r.Header.Get("Content-Length"), + MaxRequestSize: maxRequestBodySize, + Gzipped: gzipped, } + } + if errors.Is(err, io.ErrUnexpectedEOF) { return nil, BadRequestErr("json decoder error", err) } return nil, err diff --git a/server/platform/endpointer/endpoint_utils_test.go b/server/platform/endpointer/endpoint_utils_test.go index da6c17bcf2..05229a4347 100644 --- a/server/platform/endpointer/endpoint_utils_test.go +++ b/server/platform/endpointer/endpoint_utils_test.go @@ -1,6 +1,8 @@ package endpointer import ( + "bytes" + "compress/gzip" "context" "encoding/json" "errors" @@ -9,6 +11,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "reflect" "strings" "testing" @@ -276,9 +279,9 @@ func TestMakeDecoderRequestDecoderFalsePositive(t *testing.T) { t.Run("malformed JSON exactly at limit returns decode error, not 413", func(t *testing.T) { // Build a body of exactly `limit` bytes that is malformed JSON (no closing - // brace). The LimitedReader is exhausted (N==0), but a peek at the - // underlying reader returns EOF — the body ended at the limit, it was not - // cut short. Must not produce PayloadTooLargeError. + // brace). The MaxBytesReader allows the full read (body fits within limit), + // so the JSON decoder sees io.ErrUnexpectedEOF — not *http.MaxBytesError. + // Must not produce PayloadTooLargeError. prefix := `{"data":"` body := strings.NewReader(prefix + strings.Repeat("x", limit-len(prefix))) // exactly limit bytes, no closing r := httptest.NewRequest("POST", "/", body) @@ -310,3 +313,161 @@ func TestMakeDecoderRequestDecoderFalsePositive(t *testing.T) { assert.Equal(t, "hello", rd.Data) }) } + +func TestMakeDecoderRequestDecoderGzipBomb(t *testing.T) { + const limit = 100 + + makeDecoder := func() kithttp.DecodeRequestFunc { + return MakeDecoder(&testRequestDecoderType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit) + } + + gzipBody := func(data string) *bytes.Buffer { + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + _, err := gw.Write([]byte(data)) + require.NoError(t, err) + require.NoError(t, gw.Close()) + return &buf + } + + t.Run("gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) { + big := `{"data":"` + strings.Repeat("x", limit*10) + `"}` + r := httptest.NewRequest("POST", "/", gzipBody(big)) + r.Header.Set("Content-Encoding", "gzip") + _, err := makeDecoder()(context.Background(), r) + require.Error(t, err) + var ple platform_http.PayloadTooLargeError + require.True(t, errors.As(err, &ple), "gzip bomb via RequestDecoder must produce PayloadTooLargeError, got: %v", err) + assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set") + }) + + t.Run("valid gzip body within limit is decoded successfully", func(t *testing.T) { + r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`)) + r.Header.Set("Content-Encoding", "gzip") + result, err := makeDecoder()(context.Background(), r) + require.NoError(t, err) + rd, ok := result.(*testRequestDecoderType) + require.True(t, ok) + assert.Equal(t, "hi", rd.Data) + }) + + t.Run("Content-Encoding header is cleared after decompression", func(t *testing.T) { + r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`)) + r.Header.Set("Content-Encoding", "gzip") + _, err := makeDecoder()(context.Background(), r) + require.NoError(t, err) + assert.Empty(t, r.Header.Get("Content-Encoding"), "Content-Encoding should be cleared after framework decompression") + }) +} + +type testGzipRequestType struct { + Data string `json:"data"` +} + +// testRequestDecoderPayloadTooLargeType implements RequestDecoder and returns +// a PayloadTooLargeError directly from DecodeRequest (simulating implementations +// like getHostSoftwareRequest that enforce their own size limits). +type testRequestDecoderPayloadTooLargeType struct { + Data string `json:"data"` +} + +func (d *testRequestDecoderPayloadTooLargeType) DecodeRequest(_ context.Context, r *http.Request) (any, error) { + return nil, platform_http.PayloadTooLargeError{ + ContentLength: r.Header.Get("Content-Length"), + MaxRequestSize: 42, + } +} + +type testGzipBodyDecoderType struct { + Data string `json:"data"` +} + +func TestMakeDecoderGzipBomb(t *testing.T) { + const limit = 100 + + makeDecoder := func() kithttp.DecodeRequestFunc { + return MakeDecoder(testGzipRequestType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit) + } + + gzipBody := func(data string) *bytes.Buffer { + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + _, err := gw.Write([]byte(data)) + require.NoError(t, err) + require.NoError(t, gw.Close()) + return &buf + } + + t.Run("gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) { + // Compressed payload is small but decompresses well beyond the limit. + big := `{"data":"` + strings.Repeat("x", limit*10) + `"}` + r := httptest.NewRequest("POST", "/", gzipBody(big)) + r.Header.Set("Content-Encoding", "gzip") + _, err := makeDecoder()(context.Background(), r) + require.Error(t, err) + var ple platform_http.PayloadTooLargeError + require.True(t, errors.As(err, &ple), "gzip bomb must produce PayloadTooLargeError, got: %v", err) + assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set") + }) + + t.Run("valid gzip body within limit is decoded successfully", func(t *testing.T) { + r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`)) + r.Header.Set("Content-Encoding", "gzip") + result, err := makeDecoder()(context.Background(), r) + require.NoError(t, err) + rd, ok := result.(*testGzipRequestType) + require.True(t, ok) + assert.Equal(t, "hi", rd.Data) + }) + + // Sub-tests for the bodyDecoder (DecodeBody) code path, where isBodyDecoder + // returns true and decodeBody is called instead of jsonUnmarshal. + isBodyDecoder := func(v reflect.Value) bool { + _, ok := v.Interface().(*testGzipBodyDecoderType) + return ok + } + decodeBodyFn := func(_ context.Context, _ *http.Request, v reflect.Value, body io.Reader) error { + bd := v.Interface().(*testGzipBodyDecoderType) + return json.NewDecoder(body).Decode(bd) + } + makeBodyDecoder := func() kithttp.DecodeRequestFunc { + return MakeDecoder(testGzipBodyDecoderType{}, defaultJSONUnmarshal, nil, isBodyDecoder, decodeBodyFn, nil, limit) + } + + t.Run("DecodeBody gzip bomb exceeding decompressed limit returns 413", func(t *testing.T) { + big := `{"data":"` + strings.Repeat("x", limit*10) + `"}` + r := httptest.NewRequest("POST", "/", gzipBody(big)) + r.Header.Set("Content-Encoding", "gzip") + _, err := makeBodyDecoder()(context.Background(), r) + require.Error(t, err) + var ple platform_http.PayloadTooLargeError + require.True(t, errors.As(err, &ple), "gzip bomb via DecodeBody must produce PayloadTooLargeError, got: %v", err) + assert.True(t, ple.Gzipped, "PayloadTooLargeError from gzip bomb must have Gzipped set") + }) + + t.Run("DecodeBody valid gzip body within limit is decoded successfully", func(t *testing.T) { + r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`)) + r.Header.Set("Content-Encoding", "gzip") + result, err := makeBodyDecoder()(context.Background(), r) + require.NoError(t, err) + rd, ok := result.(*testGzipBodyDecoderType) + require.True(t, ok) + assert.Equal(t, "hi", rd.Data) + }) + + // Sub-test for the RequestDecoder code path where DecodeRequest itself + // returns a PayloadTooLargeError (covers lines 545-546). + t.Run("DecodeRequest returning PayloadTooLargeError preserves inner fields and sets Gzipped", func(t *testing.T) { + makePayloadDecoder := func() kithttp.DecodeRequestFunc { + return MakeDecoder(&testRequestDecoderPayloadTooLargeType{}, defaultJSONUnmarshal, nil, nil, nil, nil, limit) + } + r := httptest.NewRequest("POST", "/", gzipBody(`{"data":"hi"}`)) + r.Header.Set("Content-Encoding", "gzip") + _, err := makePayloadDecoder()(context.Background(), r) + require.Error(t, err) + var ple platform_http.PayloadTooLargeError + require.True(t, errors.As(err, &ple), "DecodeRequest returning PayloadTooLargeError must propagate, got: %v", err) + assert.True(t, ple.Gzipped, "Gzipped must be set when the request was gzip-encoded") + assert.Equal(t, int64(42), ple.MaxRequestSize, "MaxRequestSize from inner error must be preserved") + }) +} diff --git a/server/platform/http/errors.go b/server/platform/http/errors.go index 5957f74204..6de89fd76f 100644 --- a/server/platform/http/errors.go +++ b/server/platform/http/errors.go @@ -101,6 +101,7 @@ func (e *BadRequestError) IsClientError() bool { type PayloadTooLargeError struct { ContentLength string MaxRequestSize int64 + Gzipped bool } func (e PayloadTooLargeError) Error() string { @@ -117,7 +118,11 @@ func (e PayloadTooLargeError) Internal() string { // We don't care if we failed to parse the number, only if we were successful size = units.HumanSize(contentLengthAsNumber) } - msg += fmt.Sprintf(", Incoming Content-Length: %s", size) + label := "Incoming Content-Length" + if e.Gzipped { + label = "Incoming Content-Length (compressed)" + } + msg += fmt.Sprintf(", %s: %s", label, size) } return msg }