Repository navigation
fix: reject values at or above 2^63 in int parameter parsing - #3493
Open
xiao115255 wants to merge 1 commit into
Open
xiao115255 wants to merge 1 commit into
xiao115255 wants to merge 1 commit into
Conversation
toInt's upper bound compares against math.MaxInt, which is 2^(strconv.IntSize-1) - 1 and cannot be represented as a float64: on 64-bit targets it rounds up to exactly 2^63, so a value that parses to 2^63 is not greater than the bound and passed the check. It then reached int(f), and converting an out-of-range float64 to int is implementation-defined - MinInt on amd64 - so the parameter was accepted and a large negative number was sent to the API instead of failing the range check. That covered every caller of RequiredInt, OptionalIntParam, OptionalIntParamWithDefault and OptionalPaginationParams, including the string "9223372036854775807", which ParseFloat rounds to 2^63. Compare against math.Ldexp(1, strconv.IntSize-1) instead. It is the exact power of two on both 32- and 64-bit targets, so the bound holds everywhere and the conversion is never reached with an out-of-range value - unlike inspecting the result of the conversion, which only catches the overflow on targets that do not saturate.
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
1 participant
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
toIntbounded its upper range withmath.MaxInt, which a float64 cannot represent: it rounds up to exactly 2^63, so a value that parses to 2^63 was not greater than the bound and passed. It then reachedint(f), and that conversion is implementation-defined for an out-of-range value (MinInt on amd64). Compare against the exact power of two instead, before converting.Why
Fixes #3489
What changed
pkg/github/params.go: the range check intoIntnow usesmath.Ldexp(1, strconv.IntSize-1)and runs before the conversion, so the bound is exact on 32- and 64-bit targets andint(f)is never reached with an out-of-range value.pkg/github/params_test.go: boundary cases throughOptionalIntParam— 2^63 and"9223372036854775807"(whichParseFloatrounds to 2^63) are rejected, as a string and as a float64, whilemath.MaxInt32is still accepted.MCP impact
Prompts tested (tool changes only)
Security / limits
Rejected values used to pass locally and reach the API as large negative integers (
issue_number,pull_number,page,perPage). They now fail the range check before the request is built. The error message is unchanged.Tool renaming
Lint & tests
./script/lint./script/testgo test ./pkg/githubpasses the changed tests, and the three new rejection cases fail with the fix reverted.gofmt -s -landgo vetreport nothing on either file. I did not runscript/lint(golangci-lint is not installed on this machine) or the fullscript/test; on this Windows checkout the whole./pkg/githubpackage reports 14 failures in tool-snapshot tests (icon\rmismatches) that are identical on the parent commit.Docs
The same
parsed > float64(math.MaxInt)check is duplicated insecurity_outputs.goandtyped_read_normalizers.go. Happy to send that as a separate PR.