fix(cli): reject invalid CSV encodings and strengthen header tests

Author: cristim

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 == "" {

cmd/multi_service_csv_header_test.go

@@ -1,57 +1,110 @@
 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)
 		})
 	}
 }
 
-// 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) {