fix(conv): correct bounds check in ToInt to resolve CodeQL integer-conversion alert - #166
Merged
Merged
Conversation
…nversion The guard in ToInt used `||`, making the condition always true (any int64 is either <= math.MaxInt or >= math.MinInt), so `int(i)` was performed without an effective range check. On a 32-bit platform this allows a value outside the int range to be truncated instead of returning the intended sentinel. Use `&&` so the value is verified to lie within [math.MinInt, math.MaxInt] before the conversion. On 64-bit platforms behaviour is unchanged; on 32-bit platforms out-of-range values now correctly return -1. Fixes the CodeQL go/incorrect-integer-conversion (CWE-681) alert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRikukPomDzH3VYribFNPU
BenchstatBase: 4 minor regression(s) (all within 5% threshold)
1 improvement(s)
Full benchstat output |
WalkthroughChangesInteger conversion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
conv/conv.go (1)
225-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a 32-bit regression test for this branch.
The current
conv/conv_test.gocoverage does not exercise values just outside the 32-bit range. Add architecture-aware tests formath.MaxInt32+1andmath.MinInt32-1, and run them withGOARCH=386 go test ./conv/...; otherwise the corrected condition is not directly protected by the test suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conv/conv.go` around lines 225 - 226, Add architecture-aware regression coverage in conv/conv_test.go for ToInt64 conversion values just beyond the 32-bit boundaries, specifically math.MaxInt32+1 and math.MinInt32-1, asserting the expected behavior on 32-bit builds. Run the tests with GOARCH=386 go test ./conv/... to verify the branch guarded by the int-range condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conv/conv.go`:
- Around line 225-226: Update ToInt to handle uint64 inputs without first
converting them to int64; validate the original unsigned value against the int
range before narrowing, and return -1 for values outside that range. Preserve
the existing conversion behavior for values that safely fit in int.
---
Nitpick comments:
In `@conv/conv.go`:
- Around line 225-226: Add architecture-aware regression coverage in
conv/conv_test.go for ToInt64 conversion values just beyond the 32-bit
boundaries, specifically math.MaxInt32+1 and math.MinInt32-1, asserting the
expected behavior on 32-bit builds. Run the tests with GOARCH=386 go test
./conv/... to verify the branch guarded by the int-range condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+225
to
226
| if i := ToInt64(in); i >= math.MinInt && i <= math.MaxInt { | ||
| return int(i) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== conv/conv.go excerpt ==\n'
sed -n '1,320p' conv/conv.go | nl -ba | sed -n '200,250p'
printf '\n== conv/conv_test.go excerpt ==\n'
sed -n '1,260p' conv/conv_test.go | nl -ba | sed -n '1,260p'
printf '\n== search for ToInt64 and ToInt usage ==\n'
rg -n "func ToInt64|func ToInt|ToInt64\\(|ToInt\\(" convRepository: flanksource/gomplate
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== conv/conv.go around ToInt/ToInt64 ==\n'
sed -n '200,260p' conv/conv.go | cat -n
printf '\n== conv/conv_test.go relevant excerpts ==\n'
sed -n '1,260p' conv/conv_test.go | cat -n
printf '\n== direct definitions/search ==\n'
rg -n "func ToInt64|func ToInt|ToInt64\\(|ToInt\\(" convRepository: flanksource/gomplate
Length of output: 13698
Preserve uint64 values until after the int range check
Large uint64s can wrap to small negative int64s here (for example math.MaxUint64-1 becomes -2), so ToInt returns a bogus int instead of -1. Check the unsigned range before narrowing or keep the value as uint64 through validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conv/conv.go` around lines 225 - 226, Update ToInt to handle uint64 inputs
without first converting them to int64; validate the original unsigned value
against the int range before narrowing, and return -1 for values outside that
range. Preserve the existing conversion behavior for values that safely fit in
int.
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
2 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
Fixes the single open CodeQL code-scanning alert on
main:go/incorrect-integer-conversion(CWE-681) —conv/conv.go:225(ToInt)The problem
ToIntguarded itsint64 → intconversion with a tautology:Every
int64value satisfies at least one side of that||, so the condition is always true andint(i)was executed with no effective range check. The tainted value flows fromstrconv.ParseInt(strToInt64) →ToInt64→ToInt, which is exactly the path CodeQL flags. On a 32-bit platform an out-of-range value would be silently truncated instead of returning the intended-1sentinel described by the comment just below.The fix
Using
&&verifies the value lies within[math.MinInt, math.MaxInt]before converting.MinInt == MinInt64,MaxInt == MaxInt64) behaviour is unchanged.return -1.Verification
Because GitHub scans this repo with the default CodeQL query suite (the workflow specifies no custom queries), I reproduced the alert locally with the matching CodeQL bundle (
v2.26.1) and thego-code-scanning.qlssuite:go/incorrect-integer-conversionatconv/conv.go:226.go test ./conv/...passes.🤖 Generated with Claude Code
https://claude.ai/code/session_01LRikukPomDzH3VYribFNPU
Generated by Claude Code
Summary by CodeRabbit