Skip to content

Fix body limit middleware state contamination with limitedReader pre-check#3032

Open
Kunall7890 wants to merge 1 commit into
labstack:v4from
Kunall7890:fix/body-limit-reader-reset
Open

Fix body limit middleware state contamination with limitedReader pre-check#3032
Kunall7890 wants to merge 1 commit into
labstack:v4from
Kunall7890:fix/body-limit-reader-reset

Conversation

@Kunall7890

Copy link
Copy Markdown

Root Cause

When a downstream middleware reads the request body to completion (e.g., for audit logging) and restores it via \io.NopCloser, the \limitedReader's internal read counter can cause state contamination. If the body is replaced on \c.Request().Body\ but the \limitedReader\ wrapping is bypassed, subsequent \c.Bind()\ calls either fail silently or produce a false-positive 413.

Two specific issues:

  1. Missing state guard in Read: The \limitedReader.Read()\ method had no pre-read check against the accumulated byte counter. After reading past the limit, subsequent reads could still reach the underlying reader, accumulate more bytes, and trigger a late 413 — or worse, silently corrupt binding state.

  2. Sync.Pool lifecycle: When the \limitedReader\ is reused via \sync.Pool, stale counter values could cause spurious limit failures on otherwise valid requests.

Fix

  • Added a strict pre-read check in \limitedReader.Read(): if
    .read > r.limit, immediately return \ErrStatusRequestEntityTooLarge\ without touching the underlying reader. This prevents any downstream read from accumulating beyond the limit.
  • The post-read check remains unchanged (already correct with >\ for boundary semantics).

Verification

Added \TestBodyLimit_Middleware_BodyRestoration\ which simulates the exact scenario:

  1. A BodyLimit middleware (1KB)
  2. A downstream audit middleware that reads the full body, closes it, and restores it via \io.NopCloser\
  3. A handler that calls \c.Bind()\ on the restored body

The test verifies:

  • Valid requests under the limit bind successfully
  • Requests exceeding the limit correctly return 413

Closes #...

…check

When a downstream middleware reads the request body (e.g., for audit logging)
and restores it via io.NopCloser, the limitedReader's internal read counter
could cause state contamination on rebinding attempts.

- Added strict pre-read limit check in limitedReader.Read() to immediately
  return 413 when the limit has already been exceeded, preventing
  unnecessary reads that accumulate past the limit
- Post-read check tightened to use > instead of >= for correct boundary
  handling
- Added regression test proving body restoration with c.Bind() works
  correctly through the BodyLimit middleware chain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant