🐛 bug: reject oversized unknown-length adaptor request bodies#4306
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR updates the net/http-to-fasthttp adaptor middleware to reliably detect oversized request bodies. The implementation now uses io.LimitReader sized to maxBodySize+1 to allow detection of overflow during copying, validates that copied bytes do not exceed maxBodySize, and returns HTTP 413 when exceeded. Test coverage is expanded to include scenarios with unknown content length. ChangesBody Size Limit Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 #4306 +/- ##
==========================================
+ Coverage 91.32% 91.35% +0.02%
==========================================
Files 132 132
Lines 12941 12983 +42
==========================================
+ Hits 11819 11861 +42
Misses 708 708
Partials 414 414
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 enhances the adaptor middleware to correctly enforce body size limits when the content length is unknown. It updates the reading logic to use a slightly larger limit and explicitly checks if the bytes read exceed the maximum allowed size, returning a 413 Request Entity Too Large status if necessary. A review comment identifies a potential integer overflow in the limit calculation and suggests a safer initialization pattern.
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: 5842f01 | Previous: 0041045 | Ratio |
|---|---|---|---|
Benchmark_SetLogger (github.com/gofiber/fiber/v3/log) |
4.817 ns/op 0 B/op 0 allocs/op |
3.179 ns/op 0 B/op 0 allocs/op |
1.52 |
Benchmark_SetLogger (github.com/gofiber/fiber/v3/log) - ns/op |
4.817 ns/op |
3.179 ns/op |
1.52 |
This comment was automatically generated by workflow using github-action-benchmark.
Cap the LimitReader limit at math.MaxInt64 so a BodyLimit of math.MaxInt64 does not overflow to a negative value (which io.LimitReader treats as 0, returning EOF immediately). Addresses review feedback on #4306.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a security/validation gap in the net/http → Fiber adaptor where request bodies with unknown length (e.g., chunked, ContentLength = -1) could previously be truncated to BodyLimit and still processed, rather than being rejected as oversized.
Changes:
- Read request bodies with
io.LimitReader(..., BodyLimit+1)so the adaptor can detect when an unknown-length body exceedsBodyLimit. - Return HTTP 413 as soon as the copied byte count exceeds
BodyLimit, before setting the fasthttpContent-Length. - Add a regression test ensuring unknown-length over-limit bodies are rejected with 413.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/adaptor/adaptor.go | Detects oversized unknown-length request bodies by reading up to BodyLimit+1 and rejecting when exceeded. |
| middleware/adaptor/adaptor_test.go | Adds a test case covering unknown-length (ContentLength = -1) over-limit request bodies returning 413. |
Motivation
BodyLimitand processing the truncated payload instead of rejecting oversized requests.Content-Lengthand bypass application-level validation or corrupt uploads.Description
BodyLimit+1bytes from the incomingr.BodyinhandlerFunc, allowing detection of truncation when the declared length is unknown by usingio.LimitReader(r.Body, maxBodySize+1)andio.Copy.StatusRequestEntityTooLarge) when the copied byte count exceeds the configuredBodyLimitbefore setting the fasthttpContent-Lengthheader.Content-Lengthlarger than the configured limit.Test_FiberHandler_BodyLimitthat submits an over-limit payload withContentLength = -1(unknown-length) and asserts the handler returns 413.