diff --git a/internal/proxy/rest_backend_caller_tool_test.go b/internal/proxy/rest_backend_caller_tool_test.go index d261accc8..b5d855814 100644 --- a/internal/proxy/rest_backend_caller_tool_test.go +++ b/internal/proxy/rest_backend_caller_tool_test.go @@ -79,6 +79,19 @@ func TestRestBackendCaller_ExtractOwnerRepoNumber(t *testing.T) { wantRepo: "myrepo", wantNumber: "7", }, + { + name: "json.Number number", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": json.Number("99"), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantOwner: "myorg", + wantRepo: "myrepo", + wantNumber: "99", + }, { name: "missing values", args: map[string]interface{}{ @@ -89,6 +102,61 @@ func TestRestBackendCaller_ExtractOwnerRepoNumber(t *testing.T) { toolName: "pull_request_read", wantErr: "pull_request_read: missing owner/repo/pullNumber", }, + { + name: "float64 non-integer returns invalid error", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": float64(1.5), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantErr: "issue_read: invalid issue_number (out of range or not an integer)", + }, + { + name: "float64 negative returns invalid error", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": float64(-1), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantErr: "issue_read: invalid issue_number (must be non-negative)", + }, + { + name: "float64 out-of-range returns invalid error", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": float64(1e20), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantErr: "issue_read: invalid issue_number (out of range or not an integer)", + }, + { + name: "json.Number non-integer returns invalid error", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": json.Number("1.5"), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantErr: "issue_read: invalid issue_number (out of range or not an integer)", + }, + { + name: "json.Number negative returns invalid error", + args: map[string]interface{}{ + "owner": "myorg", + "repo": "myrepo", + "issue_number": json.Number("-1"), + }, + numberKey: "issue_number", + toolName: "issue_read", + wantErr: "issue_read: invalid issue_number (must be non-negative)", + }, } for _, tt := range tests { diff --git a/internal/proxy/router.go b/internal/proxy/router.go index e44dd52e7..ca63f0f40 100644 --- a/internal/proxy/router.go +++ b/internal/proxy/router.go @@ -1,6 +1,7 @@ package proxy import ( + "encoding/json" "fmt" "regexp" "strings" @@ -77,21 +78,22 @@ func extractOwnerRepoNumber(argsMap map[string]interface{}, ownerKey, repoKey, n repo = strutil.GetStringFromMap(argsMap, repoKey) number = strutil.GetStringFromMap(argsMap, numberKey) if number == "" { - if n, ok := argsMap[numberKey].(float64); ok { - logRouter.Printf("extractOwnerRepoNumber: %s provided as float64=%v, parsing as integer for tool=%s", numberKey, n, toolName) - const maxInt64AsFloat = float64(int64(^uint64(0) >> 1)) - if n < 0 || n > maxInt64AsFloat { - logRouter.Printf("extractOwnerRepoNumber: %s float64=%v out of int64 range [0,%v] for tool=%s", numberKey, n, maxInt64AsFloat, toolName) - err = fmt.Errorf("%s: invalid %s (out of range)", toolName, numberKey) + rawVal := argsMap[numberKey] + switch rawVal.(type) { + case float64, json.Number: + s, ok := strutil.InterfaceToIntString(rawVal) + if !ok { + logRouter.Printf("extractOwnerRepoNumber: %s=%v is not a valid integer for tool=%s", numberKey, rawVal, toolName) + err = fmt.Errorf("%s: invalid %s (out of range or not an integer)", toolName, numberKey) return } - i := int64(n) - if n != float64(i) { - logRouter.Printf("extractOwnerRepoNumber: %s float64=%v is not a whole number for tool=%s", numberKey, n, toolName) - err = fmt.Errorf("%s: invalid %s (expected integer)", toolName, numberKey) + if len(s) > 0 && s[0] == '-' { + logRouter.Printf("extractOwnerRepoNumber: %s=%v is negative for tool=%s", numberKey, rawVal, toolName) + err = fmt.Errorf("%s: invalid %s (must be non-negative)", toolName, numberKey) return } - number = fmt.Sprintf("%d", i) + logRouter.Printf("extractOwnerRepoNumber: %s provided as numeric=%v, parsing as integer for tool=%s", numberKey, rawVal, toolName) + number = s } } if owner == "" || repo == "" || number == "" { diff --git a/internal/server/difc_log.go b/internal/server/difc_log.go index b558474d3..db1b4b0b0 100644 --- a/internal/server/difc_log.go +++ b/internal/server/difc_log.go @@ -65,13 +65,8 @@ func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredIt } } entry.HTMLURL = strutil.GetStringFromMap(m, "html_url", "htmlUrl") - if n, ok := m["number"]; ok { - switch v := n.(type) { - case float64: - entry.Number = fmt.Sprintf("%d", int64(v)) - case json.Number: - entry.Number = v.String() - } + if s, ok := strutil.InterfaceToIntString(m["number"]); ok { + entry.Number = s } entry.SHA = strutil.GetStringFromMap(m, "sha") logDifcLog.Printf("Filtered item metadata: author=%s, number=%s, url=%s", entry.AuthorLogin, entry.Number, entry.HTMLURL) diff --git a/internal/server/difc_log_test.go b/internal/server/difc_log_test.go index 777f9c949..565a24f96 100644 --- a/internal/server/difc_log_test.go +++ b/internal/server/difc_log_test.go @@ -334,7 +334,7 @@ func TestBuildFilteredItemLogEntry_NumberEdgeCases(t *testing.T) { wantNumber string }{ {name: "zero float64", data: map[string]interface{}{"number": float64(0)}, wantNumber: "0"}, - {name: "decimal float64 truncates", data: map[string]interface{}{"number": float64(123.9)}, wantNumber: "123"}, + {name: "decimal float64 returns empty", data: map[string]interface{}{"number": float64(123.9)}, wantNumber: ""}, {name: "nil number returns empty", data: map[string]interface{}{"number": nil}, wantNumber: ""}, {name: "int number returns empty", data: map[string]interface{}{"number": 42}, wantNumber: ""}, } diff --git a/internal/strutil/numbers.go b/internal/strutil/numbers.go new file mode 100644 index 000000000..f392ee1ab --- /dev/null +++ b/internal/strutil/numbers.go @@ -0,0 +1,38 @@ +package strutil + +import ( + "encoding/json" + "fmt" + "math" +) + +// InterfaceToIntString attempts to convert a JSON-decoded numeric interface value +// (float64 or json.Number) to its decimal integer string representation. +// Returns ("", false) if the value is not a numeric type or is non-integer. +func InterfaceToIntString(v interface{}) (string, bool) { + switch n := v.(type) { + case float64: + // Explicitly guard against out-of-range values before conversion, since + // converting an out-of-range float64 to int64 is implementation-defined in Go. + // float64(math.MaxInt64) rounds up to 9.223372036854776e18, so use >= + // for the upper bound. float64(math.MinInt64) = -(2^63) is exactly + // representable, so < is appropriate for the lower bound. + if n < float64(math.MinInt64) || n >= float64(math.MaxInt64) { + return "", false // out of int64 range + } + i := int64(n) + if n != float64(i) { + return "", false // non-integer float + } + return fmt.Sprintf("%d", i), true + case json.Number: + // Validate that the json.Number represents a valid integer and convert to + // a canonical decimal string (avoids non-canonical forms like "00123"). + i, err := n.Int64() + if err != nil { + return "", false // non-integer or out-of-range json.Number + } + return fmt.Sprintf("%d", i), true + } + return "", false +} diff --git a/internal/strutil/numbers_test.go b/internal/strutil/numbers_test.go new file mode 100644 index 000000000..8d8b63fbd --- /dev/null +++ b/internal/strutil/numbers_test.go @@ -0,0 +1,111 @@ +package strutil + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestInterfaceToIntString(t *testing.T) { + t.Parallel() + + t.Run("float64 integer", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(float64(42)) + assert.True(t, ok) + assert.Equal(t, "42", s) + }) + + t.Run("float64 zero", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(float64(0)) + assert.True(t, ok) + assert.Equal(t, "0", s) + }) + + t.Run("float64 negative integer", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(float64(-7)) + assert.True(t, ok) + assert.Equal(t, "-7", s) + }) + + t.Run("float64 non-integer returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(float64(1.5)) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("float64 truncatable decimal returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(float64(123.9)) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("float64 out of int64 range returns false", func(t *testing.T) { + t.Parallel() + // 1e20 exceeds int64 max; explicit out-of-range guard rejects it + s, ok := InterfaceToIntString(float64(1e20)) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("json.Number integer", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(json.Number("999")) + assert.True(t, ok) + assert.Equal(t, "999", s) + }) + + t.Run("json.Number leading zeros canonicalized", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(json.Number("00123")) + assert.True(t, ok) + assert.Equal(t, "123", s) + }) + + t.Run("json.Number large value within int64", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(json.Number("9223372036854775807")) + assert.True(t, ok) + assert.Equal(t, "9223372036854775807", s) + }) + + t.Run("json.Number decimal returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(json.Number("123.45")) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("json.Number out of int64 range returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(json.Number("99999999999999999999")) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("string returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString("42") + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("int returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(42) + assert.False(t, ok) + assert.Equal(t, "", s) + }) + + t.Run("nil returns false", func(t *testing.T) { + t.Parallel() + s, ok := InterfaceToIntString(nil) + assert.False(t, ok) + assert.Equal(t, "", s) + }) +}