From 78dafe45d7de678b63dff56895beb108e0686be4 Mon Sep 17 00:00:00 2001 From: Anas Khan <83116240+anxkhn@users.noreply.github.com> Date: Mon, 29 Jun 2026 04:49:37 +0530 Subject: [PATCH] test: make gzip decompression-bomb regression test actually bite TestParseProtoReader_GzipDecompressionBomb only asserted that an error was returned, which passes whether or not the io.LimitReader cap on gzip output is present. The compressed input is already bounded by an outer LimitReader and a truncated bomb is rejected by the deferred size check regardless, so neither the returned error nor the bytes read from the source distinguish the two cases. The only real signal is how much is decompressed into memory. Replace the test with a white-box TestDecompressRequest_GzipDecompressionBomb that calls decompressRequest directly and asserts the buffered body never exceeds maxSize+1. This is deterministic and parallel-safe, unlike the process-wide, monotonic runtime.MemStats.TotalAlloc, which concurrent goroutines or parallel tests would inflate. Removing the cap inflates the bomb to ~4 MB in the buffer, blowing past maxSize+1 and failing the test. Also add TestParseProtoReader_Gzip covering the under-cap decode and over-cap rejection through the public API, which had no gzip coverage. Fixes #7581. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> --- pkg/util/http_internal_test.go | 41 ++++++++++++++++++++++++++++++++++ pkg/util/http_test.go | 41 ++++++++++++++++++++++------------ 2 files changed, 68 insertions(+), 14 deletions(-) create mode 100644 pkg/util/http_internal_test.go diff --git a/pkg/util/http_internal_test.go b/pkg/util/http_internal_test.go new file mode 100644 index 0000000000..156749c027 --- /dev/null +++ b/pkg/util/http_internal_test.go @@ -0,0 +1,41 @@ +package util + +import ( + "bytes" + "compress/gzip" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestDecompressRequest_GzipDecompressionBomb guards the +// io.LimitReader(gzReader, maxSize+1) cap that decompressFromReader puts on gzip +// output. It feeds a tiny gzip payload that inflates to far more than maxSize (a +// "zip bomb") and asserts the buffered body never exceeds maxSize+1. +// +// This is a white-box test on purpose. The only signal that distinguishes the +// cap being present from absent is how many bytes are decompressed into memory, +// and that is not observable through ParseProtoReader: both paths return an +// error, and the compressed input is already bounded by an outer LimitReader, so +// counting bytes read from the source cannot tell them apart. Asserting on +// len(body) is deterministic and parallel-safe, unlike reading the process-wide, +// monotonic runtime.MemStats.TotalAlloc, which concurrent goroutines or parallel +// tests would inflate. Removing the cap lets the reader inflate the bomb to +// megabytes, so len(body) blows past maxSize+1 and this test fails, which is what +// makes it a real regression guard rather than a no-op. +func TestDecompressRequest_GzipDecompressionBomb(t *testing.T) { + const maxSize = 4096 // 4 KB limit on decompressed output + uncompressed := make([]byte, 32<<20) // 32 MB of zeros, compresses to a few KB + require.Greater(t, len(uncompressed), maxSize) + + var compressed bytes.Buffer + gzw := gzip.NewWriter(&compressed) + _, err := gzw.Write(uncompressed) + require.NoError(t, err) + require.NoError(t, gzw.Close()) + + body, _ := decompressRequest(bytes.NewReader(compressed.Bytes()), 0, maxSize, Gzip, nil) + + require.LessOrEqual(t, len(body), maxSize+1, + "gzip decompression buffered %d bytes, exceeding the maxSize+1 cap; the io.LimitReader guard is missing", len(body)) +} diff --git a/pkg/util/http_test.go b/pkg/util/http_test.go index d2a5e128d4..7e1668e581 100644 --- a/pkg/util/http_test.go +++ b/pkg/util/http_test.go @@ -222,25 +222,38 @@ func TestIsRequestBodyTooLargeRegression(t *testing.T) { assert.True(t, util.IsRequestBodyTooLarge(err)) } -func TestParseProtoReader_GzipDecompressionBomb(t *testing.T) { - // Create a gzip payload where decompressed size far exceeds maxSize. - const maxSize = 4096 // 4 KB limit on decompressed output - uncompressed := make([]byte, 1<<20) // 1 MB of zeros +func TestParseProtoReader_Gzip(t *testing.T) { + req := &cortexpb.PreallocWriteRequest{ + WriteRequest: cortexpb.WriteRequest{ + Timeseries: []cortexpb.PreallocTimeseries{ + {TimeSeries: &cortexpb.TimeSeries{ + Labels: []cortexpb.LabelAdapter{{Name: "foo", Value: "bar"}}, + Samples: []cortexpb.Sample{{Value: 10, TimestampMs: 1}}, + Exemplars: []cortexpb.Exemplar{}, + Histograms: []cortexpb.WrappedHistogram{}, + }}, + }, + }, + } + data, err := req.Marshal() + require.NoError(t, err) var compressed bytes.Buffer gzw := gzip.NewWriter(&compressed) - _, err := gzw.Write(uncompressed) + _, err = gzw.Write(data) require.NoError(t, err) require.NoError(t, gzw.Close()) - // The compressed payload is small enough to pass the compressed-size limit, - // but decompresses to far more than maxSize. - require.Less(t, compressed.Len(), maxSize) - + // Under-cap payload decodes successfully. maxSize must clear both the + // compressed input and the decompressed body; gzip framing makes a tiny + // message larger compressed, so size off the compressed length. + maxSize := compressed.Len() var fromWire cortexpb.PreallocWriteRequest - err = util.ParseProtoReader(context.Background(), io.NopCloser(&compressed), 0, maxSize, &fromWire, util.Gzip) - // The decompressed output should be limited to maxSize+1 bytes, causing a - // proto unmarshal error (not an OOM). The key assertion is that we don't - // allocate 1 MB of memory. - assert.NotNil(t, err) + err = util.ParseProtoReader(context.Background(), io.NopCloser(bytes.NewReader(compressed.Bytes())), 0, maxSize, &fromWire, util.Gzip) + require.NoError(t, err) + require.Equal(t, req.Timeseries, fromWire.Timeseries) + + // Capping the decompressed size below the message rejects it. + err = util.ParseProtoReader(context.Background(), io.NopCloser(bytes.NewReader(compressed.Bytes())), 0, len(data)-1, &fromWire, util.Gzip) + assert.Error(t, err) }