Skip to content

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

Merged
aldas merged 1 commit into
labstack:v4from
Kunall7890:fix/body-limit-reader-reset
Jul 6, 2026
Merged

Fix body limit middleware state contamination with limitedReader pre-check#3032
aldas merged 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
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.53%. Comparing base (2714c07) to head (3854786).
⚠️ Report is 1 commits behind head on v4.

Files with missing lines Patch % Lines
middleware/body_limit.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##               v4    #3032   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files          42       42           
  Lines        4299     4301    +2     
=======================================
+ Hits         4021     4023    +2     
+ Misses        159      158    -1     
- Partials      119      120    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aldas aldas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aldas aldas merged commit 4f2f975 into labstack:v4 Jul 6, 2026
9 of 10 checks passed
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.

2 participants