*: fix request body not rewindable when forwarding HTTP requests#10589
Open
iosmanthus wants to merge 2 commits intotikv:masterfrom
Open
*: fix request body not rewindable when forwarding HTTP requests#10589iosmanthus wants to merge 2 commits intotikv:masterfrom
iosmanthus wants to merge 2 commits intotikv:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduced Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReverseProxy
participant APIUtil as EnsureRewindableBody
participant Backend
Client->>ReverseProxy: HTTP request (with possible Body)
ReverseProxy->>APIUtil: EnsureRewindableBody(r)
alt EnsureRewindableBody succeeds
APIUtil-->>ReverseProxy: nil (r.GetBody set / Body rewindable)
ReverseProxy->>Backend: Forward request (Transport may call r.GetBody on retry)
Backend-->>ReverseProxy: Response
ReverseProxy-->>Client: Response
else EnsureRewindableBody fails
APIUtil-->>ReverseProxy: error
ReverseProxy-->>Client: 500 Internal Server Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
bot
added
the
size/S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/utils/apiutil/apiutil.go (1)
492-503: Bound request-body buffering to avoid unbounded allocations.This path reads the full body into memory for every forwarded request with
GetBody == nil. Add a hard cap and return413when exceeded to avoid memory pressure under large payloads.💡 Proposed bounded-buffering change
+const maxRewindBodyBytes int64 = 8 << 20 // 8 MiB func (p *customReverseProxies) ServeHTTP(w http.ResponseWriter, r *http.Request) { if r.Body != nil && r.Body != http.NoBody && r.GetBody == nil { - bodyBytes, err := io.ReadAll(r.Body) + lr := &io.LimitedReader{R: r.Body, N: maxRewindBodyBytes + 1} + bodyBytes, err := io.ReadAll(lr) r.Body.Close() if err != nil { log.Error("failed to read request body", zap.Error(err)) http.Error(w, err.Error(), http.StatusInternalServerError) return } + if int64(len(bodyBytes)) > maxRewindBodyBytes { + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) + return + } r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) r.GetBody = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(bodyBytes)), nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/apiutil/apiutil.go` around lines 492 - 503, The code unconditionally reads the entire request body into memory when r.GetBody == nil; change that to use a bounded read (e.g., io.LimitedReader or similar) with a configurable MAX_BODY_BYTES and if the body exceeds the limit return http.Error(w, "request entity too large", http.StatusRequestEntityTooLarge) (413). Ensure you still close the original r.Body, set r.Body to a new io.NopCloser over the buffered bytes and implement r.GetBody to return a fresh reader for the same buffered bytes (use the truncated/allowed bytes only), and preserve existing error logging via log.Error when reading fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/apiutil/apiutil.go`:
- Around line 495-498: Read handler currently logs the internal error via
log.Error but writes err.Error() to the response using http.Error(w,
err.Error(), ...); change this to return a stable client-facing message and
appropriate status code (e.g., http.Error(w, "invalid request body",
http.StatusBadRequest)) while keeping the detailed error only in the server log
(log.Error(..., zap.Error(err))). Update the branch that checks err (the block
using log.Error, http.Error, w, err) to not expose err.Error() to clients and
ensure payload validation is applied before processing.
---
Nitpick comments:
In `@pkg/utils/apiutil/apiutil.go`:
- Around line 492-503: The code unconditionally reads the entire request body
into memory when r.GetBody == nil; change that to use a bounded read (e.g.,
io.LimitedReader or similar) with a configurable MAX_BODY_BYTES and if the body
exceeds the limit return http.Error(w, "request entity too large",
http.StatusRequestEntityTooLarge) (413). Ensure you still close the original
r.Body, set r.Body to a new io.NopCloser over the buffered bytes and implement
r.GetBody to return a fresh reader for the same buffered bytes (use the
truncated/allowed bytes only), and preserve existing error logging via log.Error
when reading fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdde20d7-c34d-4392-9c95-5d37062e55f8
📒 Files selected for processing (2)
pkg/utils/apiutil/apiutil.gopkg/utils/requestutil/request_info.go
pkg/utils/apiutil/apiutil.go
Outdated
Comment on lines
+495
to
+498
| if err != nil { | ||
| log.Error("failed to read request body", zap.Error(err)) | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return |
There was a problem hiding this comment.
Avoid returning raw internal error text to clients.
Returning err.Error() directly can expose internal details; use a stable message and an appropriate client-facing status code.
🔧 Proposed response handling adjustment
if err != nil {
log.Error("failed to read request body", zap.Error(err))
- http.Error(w, err.Error(), http.StatusInternalServerError)
+ http.Error(w, "failed to read request body", http.StatusBadRequest)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err != nil { | |
| log.Error("failed to read request body", zap.Error(err)) | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| if err != nil { | |
| log.Error("failed to read request body", zap.Error(err)) | |
| http.Error(w, "failed to read request body", http.StatusBadRequest) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/apiutil/apiutil.go` around lines 495 - 498, Read handler currently
logs the internal error via log.Error but writes err.Error() to the response
using http.Error(w, err.Error(), ...); change this to return a stable
client-facing message and appropriate status code (e.g., http.Error(w, "invalid
request body", http.StatusBadRequest)) while keeping the detailed error only in
the server log (log.Error(..., zap.Error(err))). Update the branch that checks
err (the block using log.Error, http.Error, w, err) to not expose err.Error() to
clients and ensure payload validation is applied before processing.
JmPotato
reviewed
Apr 9, 2026
Member
JmPotato
left a comment
JmPotato
left a comment
There was a problem hiding this comment.
Could you add a corresponding test?
Member
Author
Sure! |
iosmanthus
force-pushed
the
iosmanthus/fix-cannot-rewind-body
branch
from
98ace34 to
c85ce09
Compare
ti-chi-bot
bot
added
size/M
When audit or rate-limit middleware is enabled, `getBodyParam` replaces `r.Body` with an `io.NopCloser` wrapping a `bytes.Buffer` but does not set `r.GetBody`. This turns `http.NoBody` into a non-nil, non-NoBody reader without a rewind function. Later, when `customReverseProxies` forwards the request via `http.Client.Do`, Go's transport may need to retry on a new connection (e.g. stale keep-alive, HTTP/2 GOAWAY). The transport probes the body (setting didRead=true), but cannot rewind it because `GetBody` is nil, resulting in: "net/http: cannot rewind body after connection loss". This commit fixes both the root cause and adds a defensive guard: 1. `getBodyParam`: skip `http.NoBody`, and set both `Body` and `GetBody` when replacing the body so that the transport can rewind on retry. 2. `customReverseProxies.ServeHTTP`: before forwarding, ensure any non-nil body without `GetBody` is made rewindable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
iosmanthus
force-pushed
the
iosmanthus/fix-cannot-rewind-body
branch
from
c85ce09 to
c71d947
Compare
ti-chi-bot
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/utils/apiutil/apiutil.go (1)
525-528:⚠️ Potential issue | 🟠 MajorDo not expose raw internal errors in HTTP responses.
Line [527] returns
err.Error()to clients, which can leak internal details. Return a stable message and a client-appropriate status instead.🔧 Suggested response handling
if err := EnsureRewindableBody(r); err != nil { log.Error("failed to read request body", zap.Error(err)) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "invalid request body", http.StatusBadRequest) return }As per coding guidelines, "HTTP handlers must validate payloads and return proper status codes; avoid panics".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/apiutil/apiutil.go` around lines 525 - 528, The handler currently returns raw err.Error() to clients after calling EnsureRewindableBody, which can leak internals; update the response to log the detailed error with log.Error(zap.Error(err)) but send a stable, client-safe message instead (e.g., http.Error(w, "failed to read request body", http.StatusBadRequest) or http.StatusInternalServerError as appropriate for the failure type). Modify the code around the EnsureRewindableBody call in apiutil.go so it does not return err.Error() to the client but uses a fixed message and appropriate status while preserving the detailed zap.Error(err) in server logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/apiutil/apiutil.go`:
- Around line 501-521: The code that calls io.ReadAll(r.Body) must enforce a
max-body-size to prevent OOM: change the read to use a bounded reader (e.g.,
buf, err := io.ReadAll(io.LimitReader(r.Body, maxBodySize+1))) and check if
len(buf) > maxBodySize then return a specific sentinel error (e.g.,
ErrRequestBodyTooLarge) instead of buffering unbounded data; restore
r.Body/GetBody from the buffered bytes only when within the limit. In the
ServeHTTP reverse proxy handler, do not propagate err.Error() to clients—handle
ErrRequestBodyTooLarge by responding with a generic 413 Payload Too Large
message (or use http.MaxBytesReader at the handler boundary as an alternative)
and for other errors return a sanitized message. Ensure the symbols referenced
are the io.ReadAll call and r.Body / r.GetBody assignments and the ServeHTTP
reverse proxy handler.
---
Duplicate comments:
In `@pkg/utils/apiutil/apiutil.go`:
- Around line 525-528: The handler currently returns raw err.Error() to clients
after calling EnsureRewindableBody, which can leak internals; update the
response to log the detailed error with log.Error(zap.Error(err)) but send a
stable, client-safe message instead (e.g., http.Error(w, "failed to read request
body", http.StatusBadRequest) or http.StatusInternalServerError as appropriate
for the failure type). Modify the code around the EnsureRewindableBody call in
apiutil.go so it does not return err.Error() to the client but uses a fixed
message and appropriate status while preserving the detailed zap.Error(err) in
server logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf78e3d4-8d96-4aa6-8898-f7d82721eee8
📒 Files selected for processing (3)
pkg/utils/apiutil/apiutil.gopkg/utils/apiutil/apiutil_test.gopkg/utils/requestutil/request_info.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/utils/requestutil/request_info.go
Comment on lines
+501
to
+521
| buf, err := io.ReadAll(r.Body) | ||
| _ = r.Body.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Restore NoBody semantics for empty payloads so that Transport's | ||
| // outgoingLength returns 0 and no body probing happens. | ||
| if len(buf) == 0 { | ||
| r.Body = http.NoBody | ||
| r.GetBody = func() (io.ReadCloser, error) { return http.NoBody, nil } | ||
| r.ContentLength = 0 | ||
| return nil | ||
| } | ||
| r.Body = io.NopCloser(bytes.NewReader(buf)) | ||
| r.GetBody = func() (io.ReadCloser, error) { | ||
| return io.NopCloser(bytes.NewReader(buf)), nil | ||
| } | ||
| // We now know the exact length; set it so the transport can pick | ||
| // Content-Length framing over chunked encoding when forwarding. | ||
| r.ContentLength = int64(len(buf)) | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current helper has unbounded body buffering and where it is used.
rg -n 'func EnsureRewindableBody|io.ReadAll\(r.Body\)|LimitedReader|maxRewindableBodyBytes' pkg/utils/apiutil/apiutil.go
rg -n 'EnsureRewindableBody\(' pkg/utils/apiutil/apiutil.go pkg/utils/requestutil/request_info.goRepository: tikv/pd
Length of output: 425
🏁 Script executed:
# Examine the ServeHTTP handler context where EnsureRewindableBody is called
sed -n '520,535p' pkg/utils/apiutil/apiutil.go
# Check for any request size limit configuration in the server setup
rg -n 'MaxHeaderBytes|ContentLength|MaxBytesReader|LimitedReader|maxBody|size.*limit' pkg/utils/apiutil/ pkg/utils/requestutil/ -A 2 -B 2 | head -50
# Check the request_info.go usage context
sed -n '65,85p' pkg/utils/requestutil/request_info.goRepository: tikv/pd
Length of output: 2553
🏁 Script executed:
# Check for server configuration and request size limits
rg -n 'http\.Server|MaxHeaderBytes|MaxBytesReader|ListenAndServe|NewServer' pkg/ cmd/ --type go -B 2 -A 3 | head -60
# Check if there's any middleware that validates request size
rg -n 'ContentLength.*<|request.*size|body.*limit|ReadLimitCloser' pkg/ cmd/ --type go | head -20
# Check what the error message would look like
sed -n '497,530p' pkg/utils/apiutil/apiutil.goRepository: tikv/pd
Length of output: 4467
Add a max body-size guard before buffering.
Line 501 reads the full request body into memory with no upper bound. When called in the ServeHTTP reverse proxy handler (line 525), this creates a memory-exhaustion vector—an attacker can send large payloads to exhaust memory and trigger OOM without any server-side size limit configured.
Additionally, line 528 exposes the raw error message to the client via http.Error(w, err.Error(), ...), which should not leak internal details.
🔧 Suggested fix
+const maxRewindableBodyBytes = 8 << 20 // 8 MiB, tune as needed
+
func EnsureRewindableBody(r *http.Request) error {
if r.Body == nil || r.Body == http.NoBody || r.GetBody != nil {
return nil
}
- buf, err := io.ReadAll(r.Body)
+ if r.ContentLength > maxRewindableBodyBytes {
+ _ = r.Body.Close()
+ return errors.Errorf("request body too large: %d", r.ContentLength)
+ }
+ lr := &io.LimitedReader{R: r.Body, N: maxRewindableBodyBytes + 1}
+ buf, err := io.ReadAll(lr)
_ = r.Body.Close()
if err != nil {
return err
}
+ if int64(len(buf)) > maxRewindableBodyBytes {
+ return errors.Errorf("request body too large")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/apiutil/apiutil.go` around lines 501 - 521, The code that calls
io.ReadAll(r.Body) must enforce a max-body-size to prevent OOM: change the read
to use a bounded reader (e.g., buf, err := io.ReadAll(io.LimitReader(r.Body,
maxBodySize+1))) and check if len(buf) > maxBodySize then return a specific
sentinel error (e.g., ErrRequestBodyTooLarge) instead of buffering unbounded
data; restore r.Body/GetBody from the buffered bytes only when within the limit.
In the ServeHTTP reverse proxy handler, do not propagate err.Error() to
clients—handle ErrRequestBodyTooLarge by responding with a generic 413 Payload
Too Large message (or use http.MaxBytesReader at the handler boundary as an
alternative) and for other errors return a sanitized message. Ensure the symbols
referenced are the io.ReadAll call and r.Body / r.GetBody assignments and the
ServeHTTP reverse proxy handler.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10589 +/- ##
========================================
Coverage 78.96% 78.97%
========================================
Files 532 532
Lines 71883 71987 +104
========================================
+ Hits 56766 56852 +86
- Misses 11093 11109 +16
- Partials 4024 4026 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
bot
added
release-note
Move `require.New(t)` into each subtest so that failures are attributed to the correct sub-`*testing.T`, and apply minor lint fixes (unused param, intrange). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Member
Author
|
/retest |
Contributor
|
@iosmanthus: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
What problem does this PR solve?
Issue Number: close #10590
When audit or rate-limit middleware is enabled,
requestutil.getBodyParamreplaces
r.Bodywithio.NopCloser(bytes.NewBuffer(buf))without wiringup
r.GetBody. A request that originally hadhttp.NoBodyends up with anon-nil body reader and no rewind function.
When such a request is forwarded by
customReverseProxiesviahttp.Client.Do, Go's Transport may need to retry on a new connection(stale keep-alive, HTTP/2 GOAWAY). The Transport probes the body
(
didRead=true) and on retry cannot rewind becauseGetBodyis nil,failing the forward with:
What is changed and how does it work?
apiutil.EnsureRewindableBody(r)helper. It is a no-op when the bodyis
nil,http.NoBody, orr.GetBodyis already set. Otherwise itdrains the body once, restores
http.NoBodysemantics for empty payloads(so
Transport.outgoingLengthreturns 0 and no body probing happens),wires
r.GetBodyto return a fresh reader over the buffered bytes, andsets
r.ContentLengthto the exact length.customReverseProxies.ServeHTTPbefore callinghttp.Client.Do, so any forwarded request is rewindable.requestutil.getBodyParamto reuse the helper, then read thecached body via
r.GetBody()for audit logging. This fixes the rootcause while keeping existing audit behavior.
TestEnsureRewindableBodycovering nil/NoBody/existing-GetBodyno-op paths, empty-body
NoBodyrestoration, non-empty rewindabilityacross multiple
GetBody()invocations, and read-error propagation.Check List
Tests
Side effects
forwarded request that has a body — same as the pre-existing audit path)
Related changes
Release note
Summary by CodeRabbit
Bug Fixes
Tests