fix: math.MaxUint32 overflows int and test cases#3469
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
""" WalkthroughThis update modifies test cases in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware (etag)
participant Response
Client->>Middleware (etag): Send request
Middleware (etag)->>Response: Process response body
alt Body length > Max allowed size (uint64)
Middleware (etag)->>Client: Return 413 status
else Body length <= Max allowed size
Middleware (etag)->>Client: Return normal response with ETag
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3469 +/- ##
=======================================
Coverage 84.45% 84.45%
=======================================
Files 120 120
Lines 12157 12157
=======================================
Hits 10267 10267
Misses 1462 1462
Partials 428 428
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:
|
|
@viralkansarav thx for the code |
|
@ReneWerner87 The check if uint64(bodyLength) > uint64(math.MaxUint32) ensures that bodyLength will always fit into a uint32 (i.e., it’s ≤ 4,294,967,295). After this check, it’s safe to cast bodyLength to uint32 for appendUint, which expects a uint32 argument. |
There was a problem hiding this comment.
Pull Request Overview
Fixes potential integer overflow when generating ETags by rejecting response bodies larger than math.MaxUint32 and updates tests to use the correct formatting functions for int64 and uint32 boundaries.
- Use
uint64casts to reliably comparebodyLengthagainstMaxUint32 - Introduce a
uint32cast forbodyLengthbefore appending to the ETag buffer - Replace
strconv.Itoawithstrconv.FormatInt/FormatUintin tests for extreme values
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/etag/etag.go | Added overflow check with uint64 casts and renamed cast result |
| ctx_test.go | Switched from Itoa to FormatInt/FormatUint for boundary tests |
Comments suppressed due to low confidence (1)
middleware/etag/etag.go:67
- [nitpick] The variable name 'bodylength32' mixes casing and is harder to read; consider renaming it to 'bodyLength32' to follow camelCase conventions.
bodylength32 := uint32(bodyLength)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| bodyLength := len(body) | ||
| if bodyLength > math.MaxUint32 { | ||
| if uint64(bodyLength) > uint64(math.MaxUint32) { |
| { | ||
| value: int64(math.MaxInt64), | ||
| str: strconv.Itoa(math.MaxInt64), | ||
| str: strconv.FormatInt(math.MaxInt64, 10), |
There was a problem hiding this comment.
This is not needed, golang already does this inside Itoa
|
@viralkansarav These changes are not needed. We already check for max u32. The itoa function in golang is an alias to the FormatInt one you added. |
|
thank you for your work on the project, but due to the findings we decided against the adjustment in the PR |
Description
Issue fixed:
This check prevents generating an ETag for response bodies larger than the maximum value of a 32-bit unsigned integer. Since the ETag calculation uses a uint32 for the body length, allowing larger bodies could cause integer overflows or incorrect ETag values. By returning a 413 Request Entity Too Large status, the middleware safely handles oversized responses.
Fixes # (issue)
avoids potential bugs or vulnerabilities related to integer overflow.