refactor: replace BodyUncompressed() with streaming decoders for request decompression#1973
Merged
Pratham-Mishra04 merged 1 commit intomainfrom Mar 13, 2026
Merged
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements streaming decompression for HTTP request bodies (gzip, deflate, brotli, zstd) with a decompressed-size limit, adding Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MW as RequestDecompressionMiddleware
participant Decoder as requestBodyDecoder
participant App as ApplicationHandler
Client->>MW: POST with Content-Encoding + compressed body
MW->>Decoder: detect encoding, create streaming reader + cleanup
Decoder-->>MW: streaming Reader
MW->>MW: decode stream with limit (decodeRequestBodyWithLimit)
alt valid & within limit
MW->>App: forward request with decompressed body (clear headers)
else decompressed > limit
MW-->>Client: 413 Request Entity Too Large
else decoding error
MW-->>Client: 400 Bad Request (invalid compressed body)
end
MW->>Decoder: invoke cleanup (if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/middlewares.go (1)
159-161: Consider adding a comment explaining the 100MB hard cap.This defensive default prevents unbounded decompression when config is nil or
MaxRequestBodySizeMBis 0. A brief comment would help future maintainers understand this safety net.📝 Suggested documentation
if maxRequestBodyBytes <= 0 { - maxRequestBodyBytes = 100 * 1024 * 1024 // 100 MB hard cap + // Safety net: 100 MB hard cap when config is missing or set to 0. + // Prevents unbounded decompression from exhausting memory. + maxRequestBodyBytes = 100 * 1024 * 1024 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 159 - 161, Add a short explanatory comment above the default assignment to maxRequestBodyBytes that states the 100 * 1024 * 1024 (100MB) hard cap is a defensive default used when MaxRequestBodySizeMB/config is nil or zero to prevent unbounded request body decompression and DoS/resource exhaustion; reference the variable name maxRequestBodyBytes and the surrounding maxRequestBodyBytes <= 0 check so future maintainers know this is an intentional safety limit rather than an arbitrary number.transports/bifrost-http/handlers/middlewares_test.go (1)
1346-1353: Consider making empty body test assertions more specific per encoding.The current test accepts either success or 400 for all encodings. If you want deterministic behavior, you could verify expected outcomes per encoding (e.g., gzip/deflate/brotli typically error on empty input, while zstd may differ).
That said, accepting either behavior is pragmatic if the contract only cares about "no crash, reasonable response."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares_test.go` around lines 1346 - 1353, The test currently treats empty-body behavior as acceptable if either nextCalled is true or the response is 400; make it deterministic by branching on the encoding under test (e.g., the table-driven "encoding" value) and asserting the expected outcome per encoder: for gzip/deflate/brotli assert ctx.Response.StatusCode() == fasthttp.StatusBadRequest, for encoders that allow empty input (e.g., zstd) assert nextCalled is true (or a 2xx response). Update the test logic around nextCalled and ctx.Response.StatusCode() to switch on the encoding string used in the test case so each encoding has a precise expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@transports/bifrost-http/handlers/middlewares_test.go`:
- Around line 1346-1353: The test currently treats empty-body behavior as
acceptable if either nextCalled is true or the response is 400; make it
deterministic by branching on the encoding under test (e.g., the table-driven
"encoding" value) and asserting the expected outcome per encoder: for
gzip/deflate/brotli assert ctx.Response.StatusCode() ==
fasthttp.StatusBadRequest, for encoders that allow empty input (e.g., zstd)
assert nextCalled is true (or a 2xx response). Update the test logic around
nextCalled and ctx.Response.StatusCode() to switch on the encoding string used
in the test case so each encoding has a precise expectation.
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 159-161: Add a short explanatory comment above the default
assignment to maxRequestBodyBytes that states the 100 * 1024 * 1024 (100MB) hard
cap is a defensive default used when MaxRequestBodySizeMB/config is nil or zero
to prevent unbounded request body decompression and DoS/resource exhaustion;
reference the variable name maxRequestBodyBytes and the surrounding
maxRequestBodyBytes <= 0 check so future maintainers know this is an intentional
safety limit rather than an arbitrary number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0065447a-552b-4293-a7ae-6a8e50e0de2b
📒 Files selected for processing (2)
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.go
|
|
danpiths
force-pushed
the
refactor/request-streaming-decompression
branch
from
db5cedd to
e07ec1f
Compare
18 tasks
danpiths
force-pushed
the
refactor/request-streaming-decompression
branch
from
e07ec1f to
b26a802
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
126-133:⚠️ Potential issue | 🟡 MinorCompute one effective max-body limit and reuse it everywhere.
These two paths disagree when
config == nilorMaxRequestBodySizeMB <= 0: the middleware formats a0 byteserror, whiledecodeRequestBodyWithLimit()actually enforces a 100 MB fallback. That makes the client-visible message wrong and can diverge from the server-levelMaxRequestBodySizederived from the same config.Suggested fix
return func(ctx *fasthttp.RequestCtx) { if len(ctx.Request.Header.ContentEncoding()) > 0 { - maxRequestBodyBytes := 0 - if config != nil && config.ClientConfig.MaxRequestBodySizeMB > 0 { - maxRequestBodyBytes = config.ClientConfig.MaxRequestBodySizeMB * 1024 * 1024 - } + maxRequestBodyBytes := configstore.DefaultClientConfig.MaxRequestBodySizeMB * 1024 * 1024 + if config != nil && config.ClientConfig.MaxRequestBodySizeMB > 0 { + maxRequestBodyBytes = config.ClientConfig.MaxRequestBodySizeMB * 1024 * 1024 + } body, err := decodeRequestBodyWithLimit(&ctx.Request, maxRequestBodyBytes) if errors.Is(err, errRequestBodyTooLarge) { SendError(ctx, fasthttp.StatusRequestEntityTooLarge, fmt.Sprintf("decompressed request body exceeds max allowed size of %d bytes", maxRequestBodyBytes)) return @@ func decodeRequestBodyWithLimit(req *fasthttp.Request, maxRequestBodyBytes int) ([]byte, error) { reader, cleanup, err := requestBodyDecoder(req) @@ - if maxRequestBodyBytes <= 0 { - maxRequestBodyBytes = 100 * 1024 * 1024 // 100 MB hard cap - } - limitedReader := &io.LimitedReader{R: reader, N: int64(maxRequestBodyBytes + 1)}Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 126 - 133, Compute and reuse a single effective max-body limit instead of computing maxRequestBodyBytes separately: determine effectiveMaxRequestBodyBytes by checking config != nil and config.ClientConfig.MaxRequestBodySizeMB > 0 else use the same 100*1024*1024 fallback that decodeRequestBodyWithLimit uses, pass that value into decodeRequestBodyWithLimit(&ctx.Request, effectiveMaxRequestBodyBytes), and use effectiveMaxRequestBodyBytes in the SendError message when errRequestBodyTooLarge is encountered (also update the second occurrence at the other location). Ensure references are made to maxRequestBodyBytes (replace with effectiveMaxRequestBodyBytes), decodeRequestBodyWithLimit, errRequestBodyTooLarge, and SendError so both enforcement and the error text agree.
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/middlewares_test.go (1)
1323-1355: Make the empty-body contract explicit.This test currently passes on either
400or success, so it won’t catch a behavior change for any decoder. Pick the intended result for an empty compressed body and assert it per encoding; otherwise this case is effectively untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares_test.go` around lines 1323 - 1355, Update TestRequestDecompressionMiddleware_EmptyBodyWithContentEncoding to make the empty-body contract explicit: replace the current encodings loop with a table-driven test that maps each encoding (e.g., "gzip","deflate","br","zstd") to an expected outcome (status code or success via nextCalled), remove the early return that masks failures, and assert the concrete expectation per-encoding (call RequestDecompressionMiddleware(config)(next) and then check either that nextCalled is true or that ctx.Response.StatusCode() equals the expected status); adjust the table to the intended behavior for RequestDecompressionMiddleware so the test fails if any decoder's behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 188-190: The "deflate" branch currently uses flate.NewReader which
only accepts raw DEFLATE (RFC1951); instead, try zlib.NewReader(bodyReader)
first and if that returns an error fall back to flate.NewReader for
non-conformant servers, ensure you return the chosen reader and a close function
that closes it (e.g., the existing zr variable and its Close), and propagate any
final error; update the case "deflate" handling in
transports/bifrost-http/handlers/middlewares.go to implement this
try-zlib-then-flate pattern and keep the same return signature.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 126-133: Compute and reuse a single effective max-body limit
instead of computing maxRequestBodyBytes separately: determine
effectiveMaxRequestBodyBytes by checking config != nil and
config.ClientConfig.MaxRequestBodySizeMB > 0 else use the same 100*1024*1024
fallback that decodeRequestBodyWithLimit uses, pass that value into
decodeRequestBodyWithLimit(&ctx.Request, effectiveMaxRequestBodyBytes), and use
effectiveMaxRequestBodyBytes in the SendError message when
errRequestBodyTooLarge is encountered (also update the second occurrence at the
other location). Ensure references are made to maxRequestBodyBytes (replace with
effectiveMaxRequestBodyBytes), decodeRequestBodyWithLimit,
errRequestBodyTooLarge, and SendError so both enforcement and the error text
agree.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/middlewares_test.go`:
- Around line 1323-1355: Update
TestRequestDecompressionMiddleware_EmptyBodyWithContentEncoding to make the
empty-body contract explicit: replace the current encodings loop with a
table-driven test that maps each encoding (e.g., "gzip","deflate","br","zstd")
to an expected outcome (status code or success via nextCalled), remove the early
return that masks failures, and assert the concrete expectation per-encoding
(call RequestDecompressionMiddleware(config)(next) and then check either that
nextCalled is true or that ctx.Response.StatusCode() equals the expected
status); adjust the table to the intended behavior for
RequestDecompressionMiddleware so the test fails if any decoder's behavior
changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c924113c-0ace-4e1b-ae3e-7731a675ac20
📒 Files selected for processing (2)
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.go
danpiths
force-pushed
the
refactor/request-streaming-decompression
branch
from
b26a802 to
c529cf3
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
188-190:⚠️ Potential issue | 🔴 CriticalUse a zlib-compatible decoder for HTTP
deflate.
compress/flate.NewReaderonly accepts raw RFC1951 streams, so this branch rejects standards-compliantContent-Encoding: deflatebodies and conflicts with the newdeflateCompress()test helper, which emits the correct zlib-wrapped format.Suggested fix
"bytes" "compress/flate" "compress/gzip" + "compress/zlib" @@ func requestBodyDecoder(req *fasthttp.Request) (io.Reader, func(), error) { - bodyReader := bytes.NewReader(req.Body()) + rawBody := req.Body() + bodyReader := bytes.NewReader(rawBody) encoding := strings.ToLower(strings.TrimSpace(string(req.Header.ContentEncoding()))) if encoding == "" { return bodyReader, func() {}, nil } @@ case "deflate": - zr := flate.NewReader(bodyReader) - return zr, func() { _ = zr.Close() }, nil + zr, err := zlib.NewReader(bytes.NewReader(rawBody)) + if err == nil { + return zr, func() { _ = zr.Close() }, nil + } + // Fallback for non-conformant raw-deflate senders. + fr := flate.NewReader(bytes.NewReader(rawBody)) + return fr, func() { _ = fr.Close() }, nilFor HTTP Content-Encoding: deflate, do RFC 9110 and Go's compress/flate vs compress/zlib require zlib-wrapped DEFLATE? Which reader should decode compliant HTTP deflate bodies?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 188 - 190, The "deflate" branch currently uses flate.NewReader (variable zr) which expects raw RFC1951 streams; replace it with zlib.NewReader to accept zlib-wrapped DEFLATE per HTTP standards, return the reader and a proper closer function that closes the zlib reader (preserving the same return signature used by the function that reads bodyReader), and update any variable names if needed (e.g., zr -> zrZlib) to reflect the change so deflateCompress() test helper and standards-compliant Content-Encoding: deflate bodies decode correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 188-190: The "deflate" branch currently uses flate.NewReader
(variable zr) which expects raw RFC1951 streams; replace it with zlib.NewReader
to accept zlib-wrapped DEFLATE per HTTP standards, return the reader and a
proper closer function that closes the zlib reader (preserving the same return
signature used by the function that reads bodyReader), and update any variable
names if needed (e.g., zr -> zrZlib) to reflect the change so deflateCompress()
test helper and standards-compliant Content-Encoding: deflate bodies decode
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f57eccf-0422-477c-bb5a-1d8b7b7af301
📒 Files selected for processing (2)
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.go
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
danpiths
force-pushed
the
refactor/request-streaming-decompression
branch
from
c529cf3 to
71ddf55
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
188-190:⚠️ Potential issue | 🔴 CriticalUse a zlib decoder for HTTP
Content-Encoding: deflate.
compress/flate.NewReaderexpects a raw RFC 1951 stream, but the HTTPdeflatecontent-coding is zlib-wrapped. The new helper intransports/bifrost-http/handlers/middlewares_test.goalready generates the RFC-correct format, so this branch will reject valid requests and break the new deflate coverage.Suggested fix
- "compress/flate" + "compress/zlib"case "deflate": - zr := flate.NewReader(bodyReader) + zr, err := zlib.NewReader(bodyReader) + if err != nil { + return nil, func() {}, err + } return zr, func() { _ = zr.Close() }, nilBased on learnings: in
transports/bifrost-http/handlers/middlewares.go,requestBodyDecoderfor requestContent-Encoding: deflateshould use onlycompress/zlib.NewReaderwith no raw-flate fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 188 - 190, In requestBodyDecoder handle the "deflate" branch by using compress/zlib.NewReader (not compress/flate.NewReader) and return that reader plus a close wrapper; remove any raw RFC1951 flate fallback so valid HTTP deflate (zlib-wrapped) payloads produced by the test helper are accepted. Update the "deflate" case in requestBodyDecoder to call zlib.NewReader(bodyReader), return the reader and a func that closes it, and return nil error.
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/middlewares_test.go (1)
1323-1355: Make the empty-body expectation deterministic.This test currently passes for two opposite outcomes: either the middleware rejects the payload with
400, or it silently accepts it and callsnext. That makes the suite too weak to catch regressions here. Please pin the intended contract per encoding, and if success is allowed, assert the post-decompression header/body state on that path too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares_test.go` around lines 1323 - 1355, The test TestRequestDecompressionMiddleware_EmptyBodyWithContentEncoding is nondeterministic because it accepts either a 400 response or calling next; update it to assert a deterministic contract per encoding: decide which encodings should return 400 vs succeed (e.g., disallow empty input for "gzip" and "deflate" but allow "br" and "zstd"), and for each subtest compare against the expected outcome; when success is expected assert nextCalled == true and also verify the post-decompression state (e.g., RequestDecompressionMiddleware removed/cleared the Content-Encoding header and ctx.Request.Body() is an empty byte slice), and when failure is expected assert ctx.Response.StatusCode() == fasthttp.StatusBadRequest and nextCalled == false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 188-190: In requestBodyDecoder handle the "deflate" branch by
using compress/zlib.NewReader (not compress/flate.NewReader) and return that
reader plus a close wrapper; remove any raw RFC1951 flate fallback so valid HTTP
deflate (zlib-wrapped) payloads produced by the test helper are accepted. Update
the "deflate" case in requestBodyDecoder to call zlib.NewReader(bodyReader),
return the reader and a func that closes it, and return nil error.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/middlewares_test.go`:
- Around line 1323-1355: The test
TestRequestDecompressionMiddleware_EmptyBodyWithContentEncoding is
nondeterministic because it accepts either a 400 response or calling next;
update it to assert a deterministic contract per encoding: decide which
encodings should return 400 vs succeed (e.g., disallow empty input for "gzip"
and "deflate" but allow "br" and "zstd"), and for each subtest compare against
the expected outcome; when success is expected assert nextCalled == true and
also verify the post-decompression state (e.g., RequestDecompressionMiddleware
removed/cleared the Content-Encoding header and ctx.Request.Body() is an empty
byte slice), and when failure is expected assert ctx.Response.StatusCode() ==
fasthttp.StatusBadRequest and nextCalled == false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1514c00b-5700-4fe5-a694-a8682483ace9
📒 Files selected for processing (2)
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.go
danpiths
force-pushed
the
refactor/request-streaming-decompression
branch
from
71ddf55 to
7e69860
Compare
Pratham-Mishra04
approved these changes
Mar 13, 2026
Collaborator
Merge activity
|
Pratham-Mishra04
force-pushed
the
refactor/request-streaming-decompression
branch
from
7e69860 to
72e294a
Compare
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
4 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.
Summary
Refactors request body decompression to use streaming io.Reader decoders instead of fasthttp's BodyUncompressed(), following the same pattern already used for response-side decompression in DecompressStreamBody(). Size limits are now enforced during decompression via io.LimitedReader rather than after full expansion.
Changes
Type of change
Affected areas
How to test
Validate request decompression middleware handles various compression formats and enforces size limits:
Test with compressed payloads that exceed the configured
MaxRequestBodySizeMBlimit to verify proper 413 responses.Screenshots/Recordings
N/A
Breaking changes
Related issues
Addresses potential memory exhaustion vulnerabilities when processing compressed request bodies.
Security considerations
This change prevents memory exhaustion attacks where malicious clients could send small compressed payloads that expand to extremely large sizes when decompressed. The streaming approach with size limits ensures memory usage is bounded during decompression.
Checklist
docs/contributing/README.mdand followed the guidelines