From 97cb868afdd85a2c0b2742f2b8aea6cc161da6e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:36:19 +0200 Subject: [PATCH 1/3] fix(cli): reject CSV headers missing required columns loadRecommendationsFromCSV silently tolerated headers it did not recognize: a missing Service/Region/ResourceType column decoded every row to empty values, and the TEST-02 fixture shape (Instance Type / Instance Count) only surfaced as a misleading missing-Count error after rows had already decoded wrong. Validate the header up front against the required set (Service, Region, ResourceType, Count) and name the missing columns; strip a UTF-8 BOM so Excel exports parse. Closes #1327 --- cmd/multi_service_csv.go | 38 +++++++++-- cmd/multi_service_csv_header_test.go | 95 ++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 cmd/multi_service_csv_header_test.go diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 553747d23..bef32f9d2 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -48,8 +48,15 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) return nil, fmt.Errorf("failed to read CSV header: %w", err) } - // Build column index map - colIdx := buildColumnIndexMap(header) + // Build column index map, rejecting headers that lack the columns a + // row cannot be meaningful without (#1327). Before this check a header + // the parser did not recognize (e.g. "Instance Type" / "Instance Count") + // decoded every row to an empty service and was only caught by the + // Count cell validation, if at all. + colIdx, err := buildColumnIndexMap(header) + if err != nil { + return nil, err + } // Parse all records parsed, err := parseCSVRecords(reader, colIdx) @@ -60,13 +67,34 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) return parsed, nil } -// buildColumnIndexMap creates a map from column names to indices. -func buildColumnIndexMap(header []string) map[string]int { +// requiredCSVColumns are the header columns a recommendation row cannot be +// meaningful without. Term, PaymentOption, Account, Engine and the derived +// columns stay optional: minimal CSVs and Savings Plans rows legitimately +// omit them, and their values are validated downstream where they are used. +var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count"} + +// buildColumnIndexMap creates a map from column names to indices, failing +// loudly when a required column is missing. A UTF-8 BOM on the first header +// cell is stripped so Excel-exported CSVs parse; other encodings (UTF-16, +// Latin-1) still fail the required-column check with a clear error. +func buildColumnIndexMap(header []string) (map[string]int, error) { + if len(header) > 0 { + header[0] = strings.TrimPrefix(header[0], "\ufeff") + } colIdx := make(map[string]int) for i, col := range header { colIdx[col] = i } - return colIdx + var missing []string + for _, req := range requiredCSVColumns { + if _, ok := colIdx[req]; !ok { + missing = append(missing, req) + } + } + if len(missing) > 0 { + return nil, fmt.Errorf("CSV header missing required columns: %s", strings.Join(missing, ", ")) + } + return colIdx, nil } // parseCSVRecords reads and parses all CSV records. diff --git a/cmd/multi_service_csv_header_test.go b/cmd/multi_service_csv_header_test.go new file mode 100644 index 000000000..95d0b4e0d --- /dev/null +++ b/cmd/multi_service_csv_header_test.go @@ -0,0 +1,95 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Header validation (#1327): before this, a header the parser did not +// recognize silently decoded every row to an empty Service/Region/ +// ResourceType, and only the Count cell check (added in #1944) caught +// anything at all, with a misleading "missing Count column" message when +// the real problem was a wrong header. The run must fail loudly up front +// and name the missing columns. +func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) { + tests := []struct { + name string + header string + errContains string + }{ + {"missing Service", "Region,ResourceType,Count\n", "Service"}, + {"missing Region", "Service,ResourceType,Count\n", "Region"}, + {"missing ResourceType", "Service,Region,Count\n", "ResourceType"}, + {"missing Count", "Service,Region,ResourceType\n", "Count"}, + {"multiple missing", "Service,Count\n", "Region, ResourceType"}, + { + "legacy TEST-02 headers rejected", + "Service,Region,Instance Type,Instance Count\n", + "ResourceType, Count", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := loadCSVContent(t, tt.header+"rds,us-east-1,db.t3.micro,2\n") + require.Error(t, err) + assert.Contains(t, err.Error(), "CSV header missing required columns") + assert.Contains(t, err.Error(), tt.errContains) + }) + } +} + +// A UTF-8 BOM is stripped before header matching so Excel-exported CSVs +// parse instead of failing the required-column check on the BOM-prefixed +// first header cell. +func TestLoadRecommendationsFromCSV_BOMPrefixed_1327(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "\ufeffService,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "us-east-1", recs[0].Region) + assert.Equal(t, 2, recs[0].Count) +} + +// Lock in encoding/csv behaviors the purchase path relies on. +func TestLoadRecommendationsFromCSV_EdgeCases_1327(t *testing.T) { + t.Run("CRLF line endings", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\r\nrds,us-east-1,db.t3.micro,2\r\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, 2, recs[0].Count) + }) + + t.Run("quoted commas and newlines inside fields", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,\"db.t3.micro, burstable\",2,\"prod\naccount\"\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "db.t3.micro, burstable", recs[0].ResourceType) + assert.Equal(t, "prod\naccount", recs[0].AccountName) + }) + + t.Run("headers only loads zero recs", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\n")) + require.NoError(t, err) + assert.Empty(t, recs) + }) + + t.Run("wrong field count on a data row", func(t *testing.T) { + err := loadCSVContent(t, + "Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro\n") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to read CSV record") + }) + + t.Run("TOTAL row still skipped with full header", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\nTOTAL,,,2\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + }) +} From 41d10244f29f9dee80d3367132dd4fb6645050db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:40:48 +0200 Subject: [PATCH 2/3] fix(cli): reject Count=0 and negative EstimatedSavings in CSV input An explicit Count of 0 bought nothing but still triggered a live purchase call that only the EC2 provider zero guard caught; require at least 1 (the tool's own CSVs never emit 0, the Savings Plans client sets Count to 1). Reject a negative EstimatedSavings as a malformed row, and stop printing the bad value twice in parse errors (the wrapped strconv.NumError already carries it). Closes #2116 --- cmd/multi_service_csv.go | 27 ++++++++++++++++++--------- cmd/multi_service_csv_strict_test.go | 2 ++ cmd/multi_service_csv_test.go | 6 +++--- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index bef32f9d2..90f24863c 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -193,9 +193,12 @@ func getCSVField(record []string, colIdx map[string]int, fieldName string) strin return "" } -// parseCSVCount parses the required Count field as a whole non-negative -// integer. It drives purchase quantities, so a blank, missing, fractional or -// otherwise malformed cell is an error rather than a truncated or zero value. +// parseCSVCount parses the required Count field as a whole positive +// integer. It drives purchase quantities, so a blank, missing, zero, +// fractional or otherwise malformed cell is an error rather than a +// truncated or zero value. Zero in particular bought nothing but still +// triggered a live purchase call (#2116); the tool's own CSVs never +// emit 0 because the Savings Plans client sets Count to 1. func parseCSVCount(record []string, colIdx map[string]int, target *int) error { const fieldName = "Count" idx, ok := colIdx[fieldName] @@ -208,17 +211,20 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error { } n, err := strconv.Atoi(value) if err != nil { - return fmt.Errorf("column %d %q: invalid integer %q: %w", idx+1, fieldName, value, err) + return fmt.Errorf("column %d %q: invalid integer: %w", idx+1, fieldName, err) } - if n < 0 { - return fmt.Errorf("column %d %q: must not be negative, got %d", idx+1, fieldName, n) + if n < 1 { + return fmt.Errorf("column %d %q: must be at least 1, got %d", idx+1, fieldName, n) } *target = n return nil } -// parseCSVFloat parses a finite float field from a CSV record. A blank or -// absent cell leaves target untouched (see requireRankingSignal). +// parseCSVFloat parses a finite, non-negative float field from a CSV +// record. A blank or absent cell leaves target untouched (see +// requireRankingSignal). Negative values are rejected: EstimatedSavings +// is the only caller, and a negative savings figure is a malformed row, +// not a signal to buy (#2116). func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, target *float64) error { value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { @@ -227,11 +233,14 @@ func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, tar col := colIdx[fieldName] + 1 f, err := strconv.ParseFloat(value, 64) if err != nil { - return fmt.Errorf("column %d %q: invalid number %q: %w", col, fieldName, value, err) + return fmt.Errorf("column %d %q: invalid number: %w", col, fieldName, err) } if math.IsNaN(f) || math.IsInf(f, 0) { return fmt.Errorf("column %d %q: invalid number %q: must be finite", col, fieldName, value) } + if f < 0 { + return fmt.Errorf("column %d %q: must not be negative, got %g", col, fieldName, f) + } *target = f return nil } diff --git a/cmd/multi_service_csv_strict_test.go b/cmd/multi_service_csv_strict_test.go index ac0ed3fba..b965ee728 100644 --- a/cmd/multi_service_csv_strict_test.go +++ b/cmd/multi_service_csv_strict_test.go @@ -26,6 +26,7 @@ func TestLoadRecommendationsFromCSV_StrictCount(t *testing.T) { {"trailing garbage", "3abc"}, {"trailing unit", "12 units"}, {"negative", "-1"}, + {"zero", "0"}, {"blank", ""}, {"whitespace only", " "}, {"overflows int64", "99999999999999999999"}, @@ -62,6 +63,7 @@ func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { name string cell string }{ + {"negative", "-100"}, {"trailing currency", "1000 USD"}, {"trailing garbage", "12.5abc"}, {"NaN", "NaN"}, diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index fdbfcc5da..32222b0d1 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -767,13 +767,13 @@ rds,us-east-1,db.t3.micro,5,1234.5678`, }, }, { - name: "CSV with zero values", + name: "CSV with zero EstimatedSavings stays valid", csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,0,0`, +rds,us-east-1,db.t3.micro,5,0`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) - assert.Equal(t, 0, recs[0].Count) + assert.Equal(t, 5, recs[0].Count) assert.Equal(t, float64(0), recs[0].EstimatedSavings) }, }, From d75df92f7beec70a02801ee5eb50b31bf5497f76 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 18:02:08 +0200 Subject: [PATCH 3/3] fix(cli): reject invalid CSV encodings and strengthen header tests --- cmd/multi_service_csv.go | 44 +++++++-------- cmd/multi_service_csv_header_test.go | 83 +++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 39 deletions(-) diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 90f24863c..39eb4dbd0 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" @@ -47,12 +48,10 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) if err != nil { return nil, fmt.Errorf("failed to read CSV header: %w", err) } + if err = validateCSVUTF8(header); err != nil { + return nil, fmt.Errorf("CSV header: %w", err) + } - // Build column index map, rejecting headers that lack the columns a - // row cannot be meaningful without (#1327). Before this check a header - // the parser did not recognize (e.g. "Instance Type" / "Instance Count") - // decoded every row to an empty service and was only caught by the - // Count cell validation, if at all. colIdx, err := buildColumnIndexMap(header) if err != nil { return nil, err @@ -67,16 +66,18 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) return parsed, nil } -// requiredCSVColumns are the header columns a recommendation row cannot be -// meaningful without. Term, PaymentOption, Account, Engine and the derived -// columns stay optional: minimal CSVs and Savings Plans rows legitimately -// omit them, and their values are validated downstream where they are used. +// Minimal CSVs require these columns; service-specific columns stay optional. var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count"} -// buildColumnIndexMap creates a map from column names to indices, failing -// loudly when a required column is missing. A UTF-8 BOM on the first header -// cell is stripped so Excel-exported CSVs parse; other encodings (UTF-16, -// Latin-1) still fail the required-column check with a clear error. +func validateCSVUTF8(fields []string) error { + for i, field := range fields { + if !utf8.ValidString(field) { + return fmt.Errorf("column %d: invalid UTF-8 encoding", i+1) + } + } + return nil +} + func buildColumnIndexMap(header []string) (map[string]int, error) { if len(header) > 0 { header[0] = strings.TrimPrefix(header[0], "\ufeff") @@ -109,6 +110,10 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm if err != nil { return nil, fmt.Errorf("failed to read CSV record: %w", err) } + if err = validateCSVUTF8(record); err != nil { + line, _ := reader.FieldPos(0) + return nil, fmt.Errorf("CSV line %d: %w", line, err) + } // Skip the trailing TOTAL summary row that writeMultiServiceCSVReport // emits (label in the Service column). Without this, feeding the tool @@ -193,12 +198,7 @@ func getCSVField(record []string, colIdx map[string]int, fieldName string) strin return "" } -// parseCSVCount parses the required Count field as a whole positive -// integer. It drives purchase quantities, so a blank, missing, zero, -// fractional or otherwise malformed cell is an error rather than a -// truncated or zero value. Zero in particular bought nothing but still -// triggered a live purchase call (#2116); the tool's own CSVs never -// emit 0 because the Savings Plans client sets Count to 1. +// Savings Plans exports use Count=1, preserving positive-count round trips. func parseCSVCount(record []string, colIdx map[string]int, target *int) error { const fieldName = "Count" idx, ok := colIdx[fieldName] @@ -220,11 +220,7 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error { return nil } -// parseCSVFloat parses a finite, non-negative float field from a CSV -// record. A blank or absent cell leaves target untouched (see -// requireRankingSignal). Negative values are rejected: EstimatedSavings -// is the only caller, and a negative savings figure is a malformed row, -// not a signal to buy (#2116). +// Blank savings stay absent-as-zero for requireRankingSignal. func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, target *float64) error { value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { diff --git a/cmd/multi_service_csv_header_test.go b/cmd/multi_service_csv_header_test.go index 95d0b4e0d..225b36a68 100644 --- a/cmd/multi_service_csv_header_test.go +++ b/cmd/multi_service_csv_header_test.go @@ -1,38 +1,37 @@ package main import ( + "encoding/binary" "testing" + "unicode/utf16" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// Header validation (#1327): before this, a header the parser did not -// recognize silently decoded every row to an empty Service/Region/ -// ResourceType, and only the Count cell check (added in #1944) caught -// anything at all, with a misleading "missing Count column" message when -// the real problem was a wrong header. The run must fail loudly up front -// and name the missing columns. func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) { tests := []struct { name string header string + row string errContains string }{ - {"missing Service", "Region,ResourceType,Count\n", "Service"}, - {"missing Region", "Service,ResourceType,Count\n", "Region"}, - {"missing ResourceType", "Service,Region,Count\n", "ResourceType"}, - {"missing Count", "Service,Region,ResourceType\n", "Count"}, - {"multiple missing", "Service,Count\n", "Region, ResourceType"}, + {"missing Service", "Region,ResourceType,Count\n", "us-east-1,db.t3.micro,2\n", "Service"}, + {"missing Region", "Service,ResourceType,Count\n", "rds,db.t3.micro,2\n", "Region"}, + {"missing ResourceType", "Service,Region,Count\n", "rds,us-east-1,2\n", "ResourceType"}, + {"missing Count", "Service,Region,ResourceType\n", "rds,us-east-1,db.t3.micro\n", "Count"}, + {"multiple missing", "Service,Count\n", "rds,2\n", "Region, ResourceType"}, { "legacy TEST-02 headers rejected", "Service,Region,Instance Type,Instance Count\n", + "rds,us-east-1,db.t3.micro,2\n", "ResourceType, Count", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := loadCSVContent(t, tt.header+"rds,us-east-1,db.t3.micro,2\n") + err := loadCSVContent(t, tt.header+tt.row) require.Error(t, err) assert.Contains(t, err.Error(), "CSV header missing required columns") assert.Contains(t, err.Error(), tt.errContains) @@ -40,18 +39,72 @@ func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) { } } -// A UTF-8 BOM is stripped before header matching so Excel-exported CSVs -// parse instead of failing the required-column check on the BOM-prefixed -// first header cell. func TestLoadRecommendationsFromCSV_BOMPrefixed_1327(t *testing.T) { recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, "\ufeffService,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\n")) require.NoError(t, err) require.Len(t, recs, 1) + assert.Equal(t, common.ServiceRDS, recs[0].Service) assert.Equal(t, "us-east-1", recs[0].Region) assert.Equal(t, 2, recs[0].Count) } +func TestLoadRecommendationsFromCSV_Encoding_1327(t *testing.T) { + t.Run("UTF-16", func(t *testing.T) { + units := utf16.Encode([]rune("Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\n")) + for _, tt := range []struct { + name string + bom []byte + order binary.ByteOrder + }{ + {"little endian", []byte{0xff, 0xfe}, binary.LittleEndian}, + {"big endian", []byte{0xfe, 0xff}, binary.BigEndian}, + } { + t.Run(tt.name, func(t *testing.T) { + encoded := make([]byte, 2+2*len(units)) + copy(encoded, tt.bom) + for i, unit := range units { + tt.order.PutUint16(encoded[2+2*i:], unit) + } + err := loadCSVContent(t, string(encoded)) + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 1: invalid UTF-8 encoding") + }) + } + }) + + t.Run("Latin-1 optional header", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Caf\xe9\n"+ + "rds,us-east-1,db.t3.micro,2,prod\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 5: invalid UTF-8 encoding") + }) + + t.Run("Latin-1 AccountName", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,db.t3.micro,2,Caf\xe9\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 2: column 5: invalid UTF-8 encoding") + }) + + t.Run("invalid ignored field in TOTAL after multiline record", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Ignored\n"+ + "rds,us-east-1,db.t3.micro,2,\"prod\naccount\"\n"+ + "TOTAL,,,2,\xff\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 4: column 5: invalid UTF-8 encoding") + }) + + t.Run("valid non-ASCII UTF-8", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,db.t3.micro,2,Café 東京\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "Café 東京", recs[0].AccountName) + }) +} + // Lock in encoding/csv behaviors the purchase path relies on. func TestLoadRecommendationsFromCSV_EdgeCases_1327(t *testing.T) { t.Run("CRLF line endings", func(t *testing.T) {