From fa55e28280e3d83125a019ab6c96d6d6909c877b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 20:05:38 +0200 Subject: [PATCH] fix(cmd): reject NaN and Inf for numeric sizing flags Every comparison with NaN is false, so --target-coverage NaN passed the range check and then read as "unset" in applySizing, silently sizing with --coverage. Add requireFinite and apply it to --coverage, --target-coverage, --min-pool-size and --min-savings-pct, before any API call. Closes #2136 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 7 +++++ cmd/validators.go | 35 ++++++++++++++++++++-- cmd/validators_test.go | 66 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4d9879ff..e94c2e2b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ## [Unreleased] +### Fixed + +- Reject NaN and infinite values for `--coverage`, `--target-coverage`, + `--min-pool-size` and `--min-savings-pct` before any API call. NaN used to + pass validation and silently switch `--target-coverage` runs to + `--coverage` sizing (#2136). + ### Changed - Split the CLI out of the CUDly monorepo into its own module, diff --git a/cmd/validators.go b/cmd/validators.go index 6bf6e58d1..f49d44f35 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -3,6 +3,7 @@ package main import ( "fmt" "log" + "math" "os" "path/filepath" "strings" @@ -87,9 +88,8 @@ func validateRecLookbackPeriod() error { // nil if no flag-source-detection is required (e.g. unit-test paths that // only need the numeric bounds checks). func validateNumericRanges(cmd *cobra.Command) error { - // Validate coverage percentage - if toolCfg.Coverage < 0 || toolCfg.Coverage > 100 { - return fmt.Errorf("coverage percentage must be between 0 and 100, got: %.2f", toolCfg.Coverage) + if err := validateCoverageAndSavingsPct(); err != nil { + return err } if err := validateTargetCoverage(cmd); err != nil { @@ -126,9 +126,35 @@ func validateNumericRanges(cmd *cobra.Command) error { return nil } +// requireFinite rejects NaN and +/-Inf. Every ordered comparison with NaN is +// false, so range checks like `v < 0 || v > 100` and "is set" guards like +// `v > 0` silently treat NaN as valid and as unset. +func requireFinite(name string, v float64) error { + if math.IsNaN(v) || math.IsInf(v, 0) { + return fmt.Errorf("%s must be a finite number, got: %v", name, v) + } + return nil +} + +// validateCoverageAndSavingsPct checks --coverage (finite, 0-100) and that +// --min-savings-pct is finite. target-coverage and min-pool-size call +// requireFinite in their own validators. +func validateCoverageAndSavingsPct() error { + if err := requireFinite("coverage", toolCfg.Coverage); err != nil { + return err + } + if toolCfg.Coverage < 0 || toolCfg.Coverage > 100 { + return fmt.Errorf("coverage percentage must be between 0 and 100, got: %.2f", toolCfg.Coverage) + } + return requireFinite("min-savings-pct", toolCfg.MinSavingsPct) +} + // validateMinPoolSize checks that --min-pool-size is a non-negative whole number. // Instance counts are integers, so a fractional threshold is nonsensical. func validateMinPoolSize() error { + if err := requireFinite("min-pool-size", toolCfg.MinPoolSize); err != nil { + return err + } if toolCfg.MinPoolSize < 0 { return fmt.Errorf("min-pool-size must be 0 (disabled) or a positive whole number, got: %.2f", toolCfg.MinPoolSize) } @@ -143,6 +169,9 @@ func validateMinPoolSize() error { // --coverage (target wins). Split out of validateNumericRanges to keep // the parent under gocyclo's complexity threshold. func validateTargetCoverage(cmd *cobra.Command) error { + if err := requireFinite("target-coverage", toolCfg.TargetCoverage); err != nil { + return err + } if toolCfg.TargetCoverage < 0 || toolCfg.TargetCoverage > 100 { return fmt.Errorf("target-coverage percentage must be between 0 and 100, got: %.2f", toolCfg.TargetCoverage) } diff --git a/cmd/validators_test.go b/cmd/validators_test.go index c3e74ffac..a008ece77 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -761,3 +761,69 @@ func TestCheckDuplicates_HonorsIdempotencyWindow(t *testing.T) { t.Errorf("adjustRecsForDuplicates (CSV path) kept %d instance(s), err=%v; want 0", CalculateTotalInstances(got), err) } } + +// TestValidateNumericFlagsRejectNonFinite drives the real flag set through +// rootCmd.ParseFlags (cobra/pflag parse "NaN" and "Inf" as floats) and then +// the validators. Nothing here calls runTool, so no AWS client is built. +func TestValidateNumericFlagsRejectNonFinite(t *testing.T) { + saved := toolCfg + reset := func() { + toolCfg = Config{} + for _, name := range []string{"coverage", "target-coverage", "min-pool-size", "min-savings-pct"} { + f := rootCmd.Flags().Lookup(name) + if f == nil { + t.Fatalf("flag %q missing", name) + } + _ = f.Value.Set(f.DefValue) + f.Changed = false + } + } + t.Cleanup(func() { reset(); toolCfg = saved }) + + tests := []struct { + name string + args []string + parseErr bool + wantErr string + }{ + {name: "target NaN", args: []string{"--target-coverage", "NaN"}, wantErr: "target-coverage must be a finite number, got: NaN"}, + {name: "target nan lowercase", args: []string{"--target-coverage", "nan"}, wantErr: "target-coverage must be a finite number, got: NaN"}, + {name: "target +Inf", args: []string{"--target-coverage", "+Inf"}, wantErr: "target-coverage must be a finite number, got: +Inf"}, + {name: "target -Inf", args: []string{"--target-coverage", "-Inf"}, wantErr: "target-coverage must be a finite number, got: -Inf"}, + {name: "coverage NaN", args: []string{"--coverage", "NaN"}, wantErr: "coverage must be a finite number, got: NaN"}, + {name: "coverage Inf", args: []string{"--coverage", "Inf"}, wantErr: "coverage must be a finite number, got: +Inf"}, + {name: "min-pool-size NaN", args: []string{"--min-pool-size", "NaN"}, wantErr: "min-pool-size must be a finite number, got: NaN"}, + {name: "min-savings-pct NaN", args: []string{"--min-savings-pct", "NaN"}, wantErr: "min-savings-pct must be a finite number, got: NaN"}, + {name: "min-savings-pct Inf", args: []string{"--min-savings-pct", "Inf"}, wantErr: "min-savings-pct must be a finite number, got: +Inf"}, + {name: "overflow rejected by the flag parser", args: []string{"--target-coverage", "1e999"}, parseErr: true}, + {name: "explicit target 0 stays valid", args: []string{"--target-coverage", "0"}}, + {name: "valid values pass", args: []string{"--target-coverage", "95", "--coverage", "50", "--min-pool-size", "2", "--min-savings-pct", "5"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reset() + toolCfg.CoverageLookbackDays = 30 + err := rootCmd.ParseFlags(tt.args) + if tt.parseErr { + if err == nil { + t.Fatal("expected a flag parse error") + } + return + } + if err != nil { + t.Fatalf("ParseFlags: %v", err) + } + toolCfg.CoverageLookbackDays = 30 + err = validateNumericRanges(rootCmd) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil || err.Error() != tt.wantErr { + t.Fatalf("error = %v, want %q", err, tt.wantErr) + } + }) + } +}