Fix body limit middleware state contamination with limitedReader pre-check#3032
Open
Kunall7890 wants to merge 1 commit into
Open
Fix body limit middleware state contamination with limitedReader pre-check#3032Kunall7890 wants to merge 1 commit into
Kunall7890 wants to merge 1 commit into
Conversation
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.
Sync.Pool lifecycle: When the \limitedReader\ is reused via \sync.Pool, stale counter values could cause spurious limit failures on otherwise valid requests.
Fix
.read > r.limit, immediately return \ErrStatusRequestEntityTooLarge\ without touching the underlying reader. This prevents any downstream read from accumulating beyond the limit.
Verification
Added \TestBodyLimit_Middleware_BodyRestoration\ which simulates the exact scenario:
The test verifies:
Closes #...