fix(middleware): stop BodyLimit handing out more than the limit

limitedReader.Read passed the caller's buffer to the source untouched and
only looked at the running total afterwards, and the refusal did not stick.
io.Reader asks callers to process the n>0 bytes of a read before treating
its error as fatal, so a caller following that advice — encoding/json's
Decoder among them — kept getting real data on every call after the limit
had already been passed. With a 5 byte limit and a 50 byte body, 50 bytes
came through.

The read is now capped at one byte past the limit, which is all it takes to
know the body is too large; that byte is not handed to the caller; and once
the limit is passed the reader stays refused without touching the source
again.

Fixes #3071

Author: Rohilalala

middleware/body_limit.go

@@ -81,9 +81,30 @@ func (config BodyLimitConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
 }
 
 func (r *limitedReader) Read(b []byte) (n int, err error) {
+	// Once the limit is known to be exceeded, stay refused. io.Reader's
+	// contract invites callers to process the n>0 bytes of a failed read and
+	// carry on, so a reader that keeps serving data after the first refusal
+	// hands out the whole body to anyone following that advice.
+	if r.read > r.LimitBytes {
+		return 0, echo.ErrStatusRequestEntityTooLarge
+	}
+
+	// Never read further than one byte past the limit: that byte is what
+	// proves the body is too large, and anything beyond it is data the caller
+	// asked for but is not allowed to have.
+	if max := r.LimitBytes - r.read + 1; int64(len(b)) > max {
+		b = b[:max]
+	}
+
 	n, err = r.reader.Read(b)
 	r.read += int64(n)
 	if r.read > r.LimitBytes {
+		// Hand back only what fits. The byte past the limit was read to prove
+		// the body is too large, not to be delivered, and a caller that
+		// processes n>0 before the error must not receive it.
+		if over := int(r.read - r.LimitBytes); over <= n {
+			n -= over
+		}
 		return n, echo.ErrStatusRequestEntityTooLarge
 	}
 	return

middleware/body_limit_test.go

@@ -93,6 +93,89 @@ func TestBodyLimitAfterDecompressUsesDecodedSize(t *testing.T) {
 	assert.Equal(t, body, rec.Body.String())
 }
 
+func TestBodyLimitReaderStopsAtTheLimit(t *testing.T) {
+	const limit = 5
+	body := bytes.Repeat([]byte("x"), 10*limit)
+
+	reader := &limitedReader{
+		BodyLimitConfig: BodyLimitConfig{Skipper: DefaultSkipper, LimitBytes: limit},
+		reader:          io.NopCloser(bytes.NewReader(body)),
+	}
+
+	// io.Reader asks callers to process the n>0 bytes of a read before
+	// treating its error as fatal, so keep reading the way such a caller
+	// would. No more than the limit may be handed over however long it goes on.
+	buf := make([]byte, 64)
+	var total int
+	for range 20 {
+		n, err := reader.Read(buf)
+		total += n
+		if n == 0 && err != nil {
+			break
+		}
+	}
+
+	assert.Equal(t, limit, total)
+}
+
+// countingReader records how much was asked of the source, which is not
+// visible from what the caller receives.
+type countingReader struct {
+	io.Reader
+	read  int64
+	calls int
+}
+
+func (c *countingReader) Read(b []byte) (int, error) {
+	c.calls++
+	n, err := c.Reader.Read(b)
+	c.read += int64(n)
+	return n, err
+}
+
+func (c *countingReader) Close() error { return nil }
+
+func TestBodyLimitReaderDoesNotOverdrawTheSource(t *testing.T) {
+	const limit = 5
+	src := &countingReader{Reader: bytes.NewReader(bytes.Repeat([]byte("x"), 1<<20))}
+
+	reader := &limitedReader{
+		BodyLimitConfig: BodyLimitConfig{Skipper: DefaultSkipper, LimitBytes: limit},
+		reader:          src,
+	}
+
+	buf := make([]byte, 64*1024)
+	_, _ = reader.Read(buf)
+	callsAtRefusal := src.calls
+
+	// One byte past the limit is enough to know the body is too large; a
+	// megabyte of it should never be pulled off the wire to find that out.
+	assert.LessOrEqual(t, src.read, int64(limit+1))
+
+	// And once refused, the source must not be touched again.
+	for range 5 {
+		_, _ = reader.Read(buf)
+	}
+	assert.Equal(t, callsAtRefusal, src.calls)
+}
+
+func TestBodyLimitReaderStaysRefused(t *testing.T) {
+	reader := &limitedReader{
+		BodyLimitConfig: BodyLimitConfig{Skipper: DefaultSkipper, LimitBytes: 2},
+		reader:          io.NopCloser(bytes.NewReader([]byte("Hello, World!"))),
+	}
+
+	_, err := io.ReadAll(reader)
+	he := err.(echo.HTTPStatusCoder)
+	assert.Equal(t, http.StatusRequestEntityTooLarge, he.StatusCode())
+
+	// Reading on after the refusal must not produce more of the body.
+	n, err := reader.Read(make([]byte, 8))
+	assert.Equal(t, 0, n)
+	he = err.(echo.HTTPStatusCoder)
+	assert.Equal(t, http.StatusRequestEntityTooLarge, he.StatusCode())
+}
+
 func TestBodyLimitReader(t *testing.T) {
 	hw := []byte("Hello, World!")