🐛 fix: Enforce BodyLimit on request decompression and multipart form parsing#4213
Conversation
WalkthroughThe changes implement body size limiting for decompressed request bodies and multipart form parsing. The app's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4213 +/- ##
==========================================
- Coverage 91.22% 91.19% -0.04%
==========================================
Files 123 123
Lines 11892 11910 +18
==========================================
+ Hits 10849 10861 +12
- Misses 655 660 +5
- Partials 388 389 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request ensures that the configured BodyLimit is enforced during request body decompression and multipart form parsing by utilizing fasthttp's limit-aware helper methods. Documentation and tests have been updated to reflect these changes. Feedback suggests extending this enforcement to other multipart-related methods like FormFile() and FormValue(), and improving error handling in Ctx.Body() to return a 413 Request Entity Too Large status code instead of a 200 OK when the limit is exceeded.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ctx_test.go (1)
1027-1051: Consider extending BodyLimit decode tests to non-gzip encodings.This new test covers gzip well, but the changed implementation also added limit-aware paths for
br,deflate, andzstd. A small table-driven extension here would better protect against regressions in those branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ctx_test.go` around lines 1027 - 1051, Extend Test_Ctx_Body_With_Compression_BodyLimitExceeded into a table-driven test that iterates over content-encoding values ("gzip", "br", "deflate", "zstd") and for each case sets c.Request().Header.Set(HeaderContentEncoding, enc) and supplies a compressed payload exceeding the configured BodyLimit so you exercise the limit-aware branches added for brotli/deflate/zstd; reuse the existing setup (New(Config{BodyLimit: 8}), AcquireCtx -> (*DefaultCtx), set Request body) and assertions (expect fasthttp.ErrBodyTooLarge in c.Body(), original compressedBody remains in c.Request().Body(), and StatusOK) while generating the compressed payload using the appropriate compressor for each encoding in the table entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ctx_test.go`:
- Around line 1027-1051: Extend Test_Ctx_Body_With_Compression_BodyLimitExceeded
into a table-driven test that iterates over content-encoding values ("gzip",
"br", "deflate", "zstd") and for each case sets
c.Request().Header.Set(HeaderContentEncoding, enc) and supplies a compressed
payload exceeding the configured BodyLimit so you exercise the limit-aware
branches added for brotli/deflate/zstd; reuse the existing setup
(New(Config{BodyLimit: 8}), AcquireCtx -> (*DefaultCtx), set Request body) and
assertions (expect fasthttp.ErrBodyTooLarge in c.Body(), original compressedBody
remains in c.Request().Body(), and StatusOK) while generating the compressed
payload using the appropriate compressor for each encoding in the table entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 579e01c5-c7bf-46c9-827d-97953256b677
📒 Files selected for processing (10)
bind.gobinder/form.gobinder/form_test.goctx_test.godocs/api/ctx.mddocs/api/fiber.mddocs/middleware/compress.mddocs/whats_new.mdmiddleware/compress/compress_test.goreq.go
|
@copilot review this pull request. |
Reviewed the PR. Here's a summary of what was checked: Code correctness:
Tests:
Docs: |
|
@copilot Ctx.Body() returns 200 OK even when the body limit is exceeded during decompression. It would be better to return 413 Request Entity Too Large in this scenario to accurately reflect the failure to the client |
Fixed in |
|
@copilot You forgot to update the tests for the compress middleware. |
…ceeded Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/76b45b30-ff6b-4e33-b479-aa68389efec5 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in |
|
@copilot While MultipartForm() now correctly enforces the BodyLimit, other multipart-related methods in req.go such as FormFile() and FormValue() still call the underlying fasthttp methods directly. Those methods use a default 8MB limit, effectively bypassing the application's configured BodyLimit. Consider updating them to call this MultipartForm() method first to ensure consistent limit enforcement. |
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 97efe65 | Previous: b08638a | Ratio |
|---|---|---|---|
Benchmark_ShutdownServices/no-services (github.com/gofiber/fiber/v3) |
3889 ns/op 3912 B/op 19 allocs/op |
2535 ns/op 3912 B/op 19 allocs/op |
1.53 |
Benchmark_ShutdownServices/no-services (github.com/gofiber/fiber/v3) - ns/op |
3889 ns/op |
2535 ns/op |
1.53 |
Benchmark_Compress/Zstd (github.com/gofiber/fiber/v3/middleware/compress) - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/5b86c6bd-c9af-4bf0-a36b-f5d170af9c47 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in
Tests |
|
@copilot The FormValue function in req.go needs 100% test coverage. |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/1de7bad0-0284-44e5-b05f-ce7837e40ca5 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in
|
Motivation
BodyLimitto avoid unbounded memory usage and DoS vectors.BodyLimitinto form binders and request decoding paths so all entry points use the same limit.Description
BodyGunzipWithLimit,BodyUnbrotliWithLimit,BodyInflateWithLimit, andBodyUnzstdWithLimitand passingr.c.app.config.BodyLimitfromtryDecodeBodyInOrder.MultipartForm()withMultipartFormWithLimit(r.c.app.config.BodyLimit)inMultipartForm()and inFormBinding.bindMultipartcallreq.MultipartFormWithLimit(b.MaxBodySize).MaxBodySize inttoFormBinding, wire it fromBind.Form()viabind.MaxBodySize = b.ctx.App().config.BodyLimit, and reset it inFormBinding.Reset().BodyLimitboundsCtx.Body()decompression andCtx.MultipartForm()parsing, and update middleware docs and changelog to reflect the change.