[code-scanning-fix] Fix go/incorrect-integer-conversion: unsafe int64 to int conversion (CWE-190)#40599
Conversation
…190) parsePositiveIntEnvVar previously parsed env vars as int64 and callers cast the result to int with int(parsed), risking integer overflow on 32-bit platforms when the supplied value exceeded math.MaxInt32 (CWE-190 / CWE-681, CodeQL rule go/incorrect-integer-conversion, alerts #634 and #609). Change parsePositiveIntEnvVar to return int directly via strconv.Atoi, which uses the platform-native integer width and rejects values that would overflow. Update callers accordingly: - ResolveDefaultMaxTurns: strconv.FormatInt → strconv.Itoa - ResolveDefaultTimeoutMinutes: remove int() cast - ResolveDefaultMaxTurnCacheMisses: remove int() cast Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great catch on the integer-conversion issue in One thing that would strengthen the PR before it's marked ready:
If you'd like an agent to draft that:
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only changed file is pkg/workflow/compilerenv/manager.go (a production code fix). Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40599 does not have the 'implementation' label and has only 6 new lines of code in business logic directories (threshold is 100). |
There was a problem hiding this comment.
Pull request overview
This PR addresses a CodeQL go/incorrect-integer-conversion finding (CWE-190/CWE-681) by eliminating an unsafe int64 -> int cast when parsing positive integer environment variables used for workflow defaults.
Changes:
- Switched env var parsing from
strconv.ParseInt(..., 64)tostrconv.Atoito ensure out-of-range values forintare rejected on 32-bit platforms. - Updated callers to return the parsed
intdirectly (removing unsafe casts) and adjusted formatting tostrconv.Itoa. - Updated the helper’s return type and documentation to reflect
int-native parsing.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compilerenv/manager.go |
Uses strconv.Atoi and int return types to avoid overflow-prone integer conversions when resolving default env overrides. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| parsed, err := strconv.Atoi(raw) | ||
| if err != nil || parsed <= 0 { | ||
| return 0, false |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with two minor test-coverage suggestions.
📋 Key Themes & Highlights
Key Themes
- Missing overflow regression test: The fix's primary behavioral change (rejecting platform-overflowing values) isn't directly tested. A boundary test pinning the rejection is worth adding.
- Negative-value test gap: The
parsed <= 0guard covers negative inputs, but no test case explicitly documents that"-1"is treated as absent.
Positive Highlights
- ✅ Root cause correctly addressed:
strconv.Atoiusesstrconv.IntSize(platform-native width) so out-of-range values are rejected at parse time rather than wrapping silently. - ✅ All three unsafe
int64→intcast sites are cleaned up consistently. - ✅
strconv.ItoainResolveDefaultMaxTurnsis the correct symmetric counterpart toAtoi— no intermediateint64string formatting needed. - ✅ Existing test suites cover unset, zero, non-numeric, and valid paths for all three Resolve* functions.
- ✅ Minimal, surgical change — only 6 lines; no unrelated drift.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 54.7 AIC · ⌖ 7.49 AIC · ⊞ 6.9K
| return 0, false | ||
| } | ||
| parsed, err := strconv.ParseInt(raw, 10, 64) | ||
| parsed, err := strconv.Atoi(raw) |
There was a problem hiding this comment.
[/diagnose] The CWE-190 fix is correct, but there is no regression test exercising the overflow boundary that the fix specifically addresses.
💡 Suggested regression test
On a 32-bit platform, the old code silently wrapped any value > math.MaxInt32 (e.g. "2147483648"). The new strconv.Atoi correctly rejects it. On a 64-bit CI machine the boundary lies at math.MaxInt, so you can exercise rejection portably with a value that exceeds math.MaxInt64:
t.Run("overflow value uses fallback", func(t *testing.T) {
t.Setenv(DefaultTimeoutMinutes, "9223372036854775808") // MaxInt64+1
assert.Equal(t, 20, ResolveDefaultTimeoutMinutes(20))
})This locks in the behavioral change and prevents silent regression back to an unchecked cast.
| } | ||
| parsed, err := strconv.ParseInt(raw, 10, 64) | ||
| parsed, err := strconv.Atoi(raw) | ||
| if err != nil || parsed <= 0 { |
There was a problem hiding this comment.
[/tdd] Negative integer inputs (e.g. "-1") are correctly rejected by the parsed <= 0 guard, but there is no test case covering this path — only "abc" (non-numeric) and "0" (zero) are tested.
💡 Suggested test case
t.Run("negative value uses fallback", func(t *testing.T) {
t.Setenv(DefaultTimeoutMinutes, "-1")
assert.Equal(t, 20, ResolveDefaultTimeoutMinutes(20))
})Adding this makes the test suite serve as a complete specification: it documents that non-positive values (zero and negative) are all treated as absent.
There was a problem hiding this comment.
Fix is correct and targeted
The replacement of strconv.ParseInt(raw, 10, 64) with strconv.Atoi correctly delegates range-checking to the platform-native int width, eliminating the silent int64 → int narrowing at call sites. All three callers (ResolveDefaultMaxTurns, ResolveDefaultTimeoutMinutes, ResolveDefaultMaxTurnCacheMisses) are correctly updated: unnecessary int(parsed) casts removed, FormatInt → Itoa in the string path.
Review notes
- The behavioral change for out-of-range inputs on 32-bit (now falls back to default instead of silently overflowing) is the intended security outcome.
strconv.Atoi=ParseInt(s, 10, strconv.IntSize)— rejects values outside[math.MinInt, math.MaxInt]rather than wrapping; this is the correct fix for CWE-190.- An existing review comment already flags the missing overflow regression test; that concern stands as a non-blocking follow-up.
🔎 Code quality review by PR Code Quality Reviewer · 57.3 AIC · ⌖ 7.13 AIC · ⊞ 5.1K
|
Please add the overflow-boundary regression test and then summarize any remaining blockers.
|
|
@copilot review all comments and address unresolved review feedback on the int-conversion boundary coverage.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added |
pelikhan
deleted the
security/fix-incorrect-integer-conversion-634-6be7eb2cc5ff2066
branch
"body": "## Summary\n\nFixes an unsafe integer-conversion bug (CWE-190) in
pkg/workflow/compilerenv/manager.go.\nThe internal helperparsePositiveIntEnvVarwas previously written againstint64and then cast down tointvia Go's unchecked numeric conversion — silently truncating values on 32-bit platforms and risking integer-overflow misuse in callers. The fix replaces the conversion chain with a directstrconv.Atoicall, which produces a platform-nativeintand returns an error on overflow, eliminating the unsafe narrowing cast entirely. The threeResolve*callers are simplified accordingly (no more explicitint64→intcasts). A regression test is added to document and verify the correct 32-bit/64-bit boundary behaviour.\n\n## Changed Files\n\n| File | Change type | Summary |\n|---|---|---|\n|pkg/workflow/compilerenv/manager.go| modified | Replacestrconv.ParseInt+int64→intcast withstrconv.Atoi; remove downstream casts in threeResolve*callers. |\n|pkg/workflow/compilerenv/manager_test.go| modified | AddTestParsePositiveIntEnvVar_OverflowRegression— verifies fallback on 32-bit for values >MaxInt32, success on 64-bit. |\n\n## Motivation & Risk\n\nSecurity / correctness: Go'sint64→intnumeric conversion is unchecked; on a 32-bit runtime a value such as2147483648(231, one aboveMaxInt32) silently wraps to-2147483648. This is the integer-overflow class tracked as CWE-190 and reported by thego/incorrect-integer-conversionCodeQL query. Usingstrconv.Atoiinstead lets the standard library reject out-of-range values before they ever reach the rest of the resolver logic.\n\nBreaking changes: None. The public API ofcompilerenvis unchanged; only the internal helper's type signature and its callers are affected.\n\nImpact: Medium for the production code (correctness fix, no API change); Low for the test addition (documentation/regression only).\n\n## Commits\n\n| Hash | Message |\n|---|---|\n|848d23731|fix(security): replace unsafe int64→int casts with strconv.Atoi (CWE-190)|\n|171bafaee|test: add overflow regression test for parsePositiveIntEnvVar (CWE-190)|\n|644792886|test: clarify overflow comment — 2^31 is one above MaxInt32|\n\n## Testing\n\n- Existing unit tests for theResolve*functions continue to exercise normal-path behaviour.\n- NewTestParsePositiveIntEnvVar_OverflowRegressionverifies:\n - On 64-bit platforms: a value of2147483648(MaxInt32 + 1) is parsed successfully.\n - On 32-bit platforms: the same value exceedsstrconv.Atoi's range and is rejected with an error (no silent truncation).\n\n## References\n\n- CWE-190: Integer Overflow or Wraparound — (cwe.mitre.org/redacted) CodeQL query:go/incorrect-integer-conversion\n- PR: #40599"}