fix(cli): validate CSV headers, encoding and numeric bounds

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,9 +48,14 @@ 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
-	colIdx := buildColumnIndexMap(header)
+	colIdx, err := buildColumnIndexMap(header)
+	if err != nil {
+		return nil, err
+	}
 
 	// Parse all records
 	parsed, err := parseCSVRecords(reader, colIdx)
@@ -60,13 +66,36 @@ 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 {
+// Minimal CSVs require these columns; service-specific columns stay optional.
+var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count"}
+
+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")
+	}
 	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.
@@ -81,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
@@ -165,9 +198,7 @@ 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.
+// 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]
@@ -180,17 +211,16 @@ 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).
+// 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 == "" {
@@ -199,11 +229,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
 }

cmd/multi_service_csv_header_test.go

@@ -0,0 +1,148 @@
+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"
+)
+
+func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) {
+	tests := []struct {
+		name        string
+		header      string
+		row         string
+		errContains string
+	}{
+		{"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+tt.row)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "CSV header missing required columns")
+			assert.Contains(t, err.Error(), tt.errContains)
+		})
+	}
+}
+
+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) {
+		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)
+	})
+}

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"},

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)
 			},
 		},