fix(cli): validate CSV headers, encoding and numeric bounds by cristim · Pull Request #2145 · LeanerCloud/cloud-commitments-cli · GitHub
Skip to content
6 changes: 6 additions & 0 deletions CHANGELOG.md
2 changes: 1 addition & 1 deletion cmd/cancellation_prepurchase_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestFilterAndAdjustRecommendations_CancellationAndDeadlineAreTerminal(t *te
func TestPrepareCSVPurchaseRun_CanceledContextStopsBeforeConfirmationAndPurchase(t *testing.T) {
dir := t.TempDir()
csvPath := filepath.Join(dir, "in.csv")
require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count,Engine\nrds,us-east-1,db.t3.small,2,mysql\n"), 0o600))
require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count,Engine,Term,PaymentOption\nrds,us-east-1,db.t3.small,2,mysql,1yr,no-upfront\n"), 0o600))
cfg := Config{CSVInput: csvPath, AuditLog: filepath.Join(dir, "audit.jsonl")}

for _, dry := range []bool{true, false} {
Expand Down
104 changes: 86 additions & 18 deletions cmd/multi_service_csv.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"strconv"
"strings"
"time"
"unicode/utf8"

"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
"github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations"
Expand Down Expand Up @@ -48,9 +49,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)
Expand All @@ -61,13 +67,60 @@ 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.
// Term and PaymentOption are required because the purchase path reads them per
// row and must never guess a commitment's term or payment option.
var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count", "Term", "PaymentOption"}

// Accepted Term values match fetchRecommendationsForRegion; accepted
// PaymentOption values match validatePaymentAndTerm.
var (
validCSVTerms = []string{"1yr", "3yr"}
validCSVPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"}
)

// parseCSVEnum requires the cell to be exactly one of allowed.
func parseCSVEnum(record []string, colIdx map[string]int, fieldName string, allowed []string) (string, error) {
col := colIdx[fieldName] + 1
value := getCSVField(record, colIdx, fieldName)
if strings.TrimSpace(value) == "" {
return "", fmt.Errorf("column %d %q: value is required (one of: %s)", col, fieldName, strings.Join(allowed, ", "))
}
for _, a := range allowed {
if value == a {
return value, nil
}
}
return "", fmt.Errorf("column %d %q: invalid value %q (must be one of: %s)", col, fieldName, value, strings.Join(allowed, ", "))
}

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.
Expand All @@ -82,6 +135,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
Expand Down Expand Up @@ -113,14 +170,25 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati
rec.ResourceType = getCSVField(record, colIdx, "ResourceType")
rec.Account = getCSVField(record, colIdx, "Account")
rec.AccountName = getCSVField(record, colIdx, "AccountName")
rec.Term = getCSVField(record, colIdx, "Term")
rec.PaymentOption = getCSVField(record, colIdx, "PaymentOption")

if err := parseCSVCount(record, colIdx, &rec.Count); err != nil {
term, err := parseCSVEnum(record, colIdx, "Term", validCSVTerms)
if err != nil {
return rec, err
}
rec.Term = term
payment, err := parseCSVEnum(record, colIdx, "PaymentOption", validCSVPaymentOptions)
if err != nil {
return rec, err
}
rec.PaymentOption = payment

err = parseCSVCount(record, colIdx, &rec.Count)
if err != nil {
return rec, err
}

if err := parseCSVFloat(record, colIdx, "EstimatedSavings", &rec.EstimatedSavings); err != nil {
err = parseCSVFloat(record, colIdx, "EstimatedSavings", &rec.EstimatedSavings)
if err != nil {
return rec, err
}

Expand All @@ -143,9 +211,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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
func parseCSVCount(record []string, colIdx map[string]int, target *int) error {
const fieldName = "Count"
idx, ok := colIdx[fieldName]
Expand All @@ -158,17 +224,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 == "" {
Expand All @@ -177,11 +242,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
}
Expand Down
151 changes: 151 additions & 0 deletions cmd/multi_service_csv_header_test.go
Loading
Loading