Skip to content

[code-scanning-fix] Fix go/incorrect-integer-conversion: unsafe int64 to int conversion (CWE-190) #40599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 21, 2026
+22 −6
Merged

[code-scanning-fix] Fix go/incorrect-integer-conversion: unsafe int64 to int conversion (CWE-190) #40599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
848d237
fix(security): replace unsafe int64→int casts with strconv.Atoi (CWE-…
github-actions[bot] Jun 21, 2026
171bafa
test: add overflow regression test for parsePositiveIntEnvVar (CWE-190)
Copilot Jun 21, 2026
6447928
test: clarify overflow comment — 2^31 is one above MaxInt32
Copilot Jun 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions pkg/workflow/compilerenv/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const (
// otherwise returns the parsed override as a string.
func ResolveDefaultMaxTurns(fallback string) string {
if parsed, ok := parsePositiveIntEnvVar(DefaultMaxTurns); ok {
return strconv.FormatInt(parsed, 10)
return strconv.Itoa(parsed)
}
return fallback
}
Expand All @@ -62,7 +62,7 @@ func ResolveDefaultMaxTurns(fallback string) string {
// otherwise returns the parsed override.
func ResolveDefaultTimeoutMinutes(fallback int) int {
if parsed, ok := parsePositiveIntEnvVar(DefaultTimeoutMinutes); ok {
return int(parsed)
return parsed
}
return fallback
}
Expand All @@ -71,7 +71,7 @@ func ResolveDefaultTimeoutMinutes(fallback int) int {
// otherwise returns the parsed override.
func ResolveDefaultMaxTurnCacheMisses(fallback int) int {
if parsed, ok := parsePositiveIntEnvVar(DefaultMaxTurnCacheMisses); ok {
return int(parsed)
return parsed
}
return fallback
}
Expand All @@ -98,15 +98,15 @@ func ResolveDefaultUTC(fallback string) string {
return raw
}

// parsePositiveIntEnvVar parses an environment variable as a base-10 positive int64.
// parsePositiveIntEnvVar parses an environment variable as a base-10 positive int.
// It returns (value, true) when the variable is set to a valid value > 0.
// For unset, empty, non-numeric, or non-positive values, it returns (0, false).
func parsePositiveIntEnvVar(name string) (int64, bool) {
func parsePositiveIntEnvVar(name string) (int, bool) {
raw := strings.TrimSpace(os.Getenv(name))
if raw == "" {
return 0, false
}
parsed, err := strconv.ParseInt(raw, 10, 64)
parsed, err := strconv.Atoi(raw)

@github-actions github-actions Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

if err != nil || parsed <= 0 {

@github-actions github-actions Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

return 0, false
Comment on lines +109 to 111
}
Expand Down
16 changes: 16 additions & 0 deletions pkg/workflow/compilerenv/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package compilerenv

import (
"strconv"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -106,6 +107,21 @@ func TestResolveDefaultMaxTurnCacheMisses(t *testing.T) {
})
}

func TestParsePositiveIntEnvVar_OverflowRegression(t *testing.T) {
// 2^31 = 2147483648, one above MaxInt32 (2^31-1): fits in int64 but overflows int32.
// On 32-bit platforms (strconv.IntSize == 32) strconv.Atoi rejects this
// value, so the function must fall back to the default — the original
// CWE-190 silent-overflow scenario. On 64-bit platforms it parses
// successfully, proving no over-restriction.
const bigVal = "2147483648"
t.Setenv(DefaultTimeoutMinutes, bigVal)
if strconv.IntSize == 32 {
assert.Equal(t, 20, ResolveDefaultTimeoutMinutes(20), "overflow value must fall back on 32-bit")
} else {
assert.Equal(t, 2147483648, ResolveDefaultTimeoutMinutes(20), "value fits on 64-bit, must parse")
}
}

func TestResolveDefaultDetectionModel(t *testing.T) {
t.Run("unset uses fallback", func(t *testing.T) {
t.Setenv(DefaultDetectionModel, "")
Expand Down
Loading