From 6fc436c95de3e5163fe8c575e7c443d0d515322d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 30 Sep 2026 01:10:17 +0200 Subject: [PATCH] fix(server): reject malformed startup environment defaults Return parse errors for term, coverage and analytics enablement before initializing external dependencies. Preserve unset defaults and valid values, including disabled analytics and zero coverage. Closes #236 --- internal/server/analytics_collect.go | 31 +++--- internal/server/analytics_collect_test.go | 58 ++++++++++- internal/server/app.go | 54 ++++++----- internal/server/app_test.go | 113 ++++++++++++++-------- 4 files changed, 172 insertions(+), 84 deletions(-) diff --git a/internal/server/analytics_collect.go b/internal/server/analytics_collect.go index ddb0f44be..b1558b55b 100644 --- a/internal/server/analytics_collect.go +++ b/internal/server/analytics_collect.go @@ -40,16 +40,22 @@ const ( analyticsDDLTimeout = 5 * time.Minute ) -// LoadAnalyticsConfig reads the collector knobs from env, falling back to -// defaults for unset/blank values. Out-of-range or unparseable values are -// preserved as-is here so Validate can reject them with a clear message at -// startup (fail-fast at the boundary) rather than being silently clamped. -func LoadAnalyticsConfig() AnalyticsConfig { +// LoadAnalyticsConfig defaults unset/blank values and rejects malformed booleans. +// Integer validation remains in Validate. +func LoadAnalyticsConfig() (AnalyticsConfig, error) { + enabled := true + if raw := os.Getenv("ANALYTICS_COLLECTION_ENABLED"); raw != "" { + value, err := strconv.ParseBool(raw) + if err != nil { + return AnalyticsConfig{}, fmt.Errorf("invalid ANALYTICS_COLLECTION_ENABLED: %w", err) + } + enabled = value + } return AnalyticsConfig{ - Enabled: getEnvBool("ANALYTICS_COLLECTION_ENABLED", true), + Enabled: enabled, RetentionMonths: loadAnalyticsInt("ANALYTICS_RETENTION_MONTHS", defaultAnalyticsRetentionMonths), PartitionsAhead: loadAnalyticsInt("ANALYTICS_PARTITIONS_AHEAD", defaultAnalyticsPartitionsAhead), - } + }, nil } // loadAnalyticsInt reads an integer collector knob from env. An unset/blank @@ -91,17 +97,6 @@ func withDDLTimeout(ctx context.Context, fn func(context.Context, int) error, ar return fn(stepCtx, arg) } -// getEnvBool parses a boolean env var, returning defaultVal when unset or -// unparseable. Accepts the strconv.ParseBool truth set (1/t/true/...). -func getEnvBool(key string, defaultVal bool) bool { - if val := os.Getenv(key); val != "" { - if result, err := strconv.ParseBool(val); err == nil { - return result - } - } - return defaultVal -} - // newAnalyticsCollector builds the savings-snapshot collector against the live // DB connection. Kept here (not inline in reinitializeAfterConnect) so the // app.go footprint is one localized call (eases the in-flight app.go rebase). diff --git a/internal/server/analytics_collect_test.go b/internal/server/analytics_collect_test.go index ba971e6c8..9cc5b0ce2 100644 --- a/internal/server/analytics_collect_test.go +++ b/internal/server/analytics_collect_test.go @@ -2,11 +2,57 @@ package server import ( "context" + "os" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" ) +func TestLoadAnalyticsConfigEnabled(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") + for _, tc := range []struct { + name, value string + wantEnabled bool + wantError bool + }{ + {"unset", "", true, false}, + {"empty", "", true, false}, + {"true", "true", true, false}, + {"TRUE", "TRUE", true, false}, + {"1", "1", true, false}, + {"false", "false", false, false}, + {"FALSE", "FALSE", false, false}, + {"0", "0", false, false}, + {"no", "no", false, true}, + {"off", "off", false, true}, + {"trailing space", "False ", false, true}, + {"disabled", "disabled", false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("ANALYTICS_COLLECTION_ENABLED", tc.value) + if tc.name == "unset" { + if err := os.Unsetenv("ANALYTICS_COLLECTION_ENABLED"); err != nil { + t.Fatal(err) + } + } + cfg, err := LoadAnalyticsConfig() + if tc.wantError { + if err == nil || !strings.Contains(err.Error(), "ANALYTICS_COLLECTION_ENABLED") { + t.Fatalf("expected enabled parse error, got %v", err) + } + return + } + if err != nil { + t.Fatal(err) + } + testutil.AssertEqual(t, tc.wantEnabled, cfg.Enabled) + testutil.AssertNoError(t, cfg.Validate()) + }) + } +} + // TestLoadAnalyticsConfig_FailFastOnMalformedInt is the CR #1049 regression: // a set-but-unparseable ANALYTICS_RETENTION_MONTHS / ANALYTICS_PARTITIONS_AHEAD // must produce a config that Validate() rejects (fail-fast at startup) instead @@ -16,7 +62,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("unset uses defaults and validates", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, defaultAnalyticsRetentionMonths, cfg.RetentionMonths) testutil.AssertEqual(t, defaultAnalyticsPartitionsAhead, cfg.PartitionsAhead) testutil.AssertNoError(t, cfg.Validate()) @@ -25,7 +72,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("valid override is parsed", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "12") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "6") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 12, cfg.RetentionMonths) testutil.AssertEqual(t, 6, cfg.PartitionsAhead) testutil.AssertNoError(t, cfg.Validate()) @@ -34,7 +82,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("malformed retention is rejected by Validate", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "not-a-number") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "3") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 0, cfg.RetentionMonths) // sentinel testutil.AssertTrue(t, cfg.Validate() != nil, "malformed retention must fail Validate") }) @@ -42,7 +91,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("malformed partitions-ahead is rejected by Validate", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "24") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "12x") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 0, cfg.PartitionsAhead) // sentinel testutil.AssertTrue(t, cfg.Validate() != nil, "malformed partitions-ahead must fail Validate") }) diff --git a/internal/server/app.go b/internal/server/app.go index d1f6d7cfd..b6a94656a 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -258,18 +258,38 @@ func decorateSenderWithMute(sender email.SenderInterface, mc email.MuteChecker, } // LoadApplicationConfig reads all configuration from environment variables. -func LoadApplicationConfig() ApplicationConfig { +func LoadApplicationConfig() (ApplicationConfig, error) { version := os.Getenv("VERSION") if version == "" { version = "dev" } + term := 3 + if raw := os.Getenv("DEFAULT_TERM"); raw != "" { + value, err := strconv.Atoi(raw) + if err != nil { + return ApplicationConfig{}, fmt.Errorf("invalid DEFAULT_TERM: %w", err) + } + term = value + } + coverage := 80.0 + if raw := os.Getenv("DEFAULT_COVERAGE"); raw != "" { + value, err := strconv.ParseFloat(raw, 64) + if err != nil { + return ApplicationConfig{}, fmt.Errorf("invalid DEFAULT_COVERAGE: %w", err) + } + coverage = value + } + analyticsConfig, err := LoadAnalyticsConfig() + if err != nil { + return ApplicationConfig{}, err + } return ApplicationConfig{ Version: version, NotificationDaysBefore: getEnvInt("NOTIFICATION_DAYS_BEFORE", 3), - DefaultTerm: getEnvInt("DEFAULT_TERM", 3), + DefaultTerm: term, DefaultPaymentOption: os.Getenv("DEFAULT_PAYMENT_OPTION"), - DefaultCoverage: getEnvFloat("DEFAULT_COVERAGE", 80), + DefaultCoverage: coverage, DefaultRampSchedule: os.Getenv("DEFAULT_RAMP_SCHEDULE"), APIKeySecretARN: os.Getenv("API_KEY_SECRET_ARN"), EnableDashboard: os.Getenv("ENABLE_DASHBOARD") == "true", @@ -283,8 +303,8 @@ func LoadApplicationConfig() ApplicationConfig { ScheduledTaskSecret: os.Getenv("SCHEDULED_TASK_SECRET"), ScheduledTaskSecretName: os.Getenv("SCHEDULED_TASK_SECRET_NAME"), IsLambda: runtime.IsLambda(), - Analytics: LoadAnalyticsConfig(), - } + Analytics: analyticsConfig, + }, nil } // validateAppConfigEnvDefaults validates the money-moving env-sourced defaults @@ -532,12 +552,15 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext // can pass the ldflags-stamped value directly instead of round-tripping // through os.Setenv / os.Getenv (04-N1). Pass "" to fall back to the env. func NewApplication(ctx context.Context, version string) (*Application, error) { - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + if err != nil { + return nil, err + } if version != "" { cfg.Version = version } - if err := cfg.Analytics.Validate(); err != nil { + if err = cfg.Analytics.Validate(); err != nil { return nil, fmt.Errorf("invalid analytics configuration: %w", err) } @@ -969,23 +992,6 @@ func getEnvInt(key string, defaultVal int) int { return defaultVal } -// getEnvFloat mirrors getEnvInt for float-valued env vars. defaultVal is kept -// parameterized (rather than inlined) to stay symmetric with getEnvInt even -// though every current caller passes the same coverage default. -// -//nolint:unparam // general-purpose env parser; default kept parameterized for symmetry with getEnvInt -func getEnvFloat(key string, defaultVal float64) float64 { - if val := os.Getenv(key); val != "" { - result, err := strconv.ParseFloat(val, 64) - if err != nil { - log.Printf("WARNING: %s=%q is not a valid float; using default %g", key, val, defaultVal) // #nosec G706 -- env var value is operator-controlled configuration; logged for diagnostics - return defaultVal - } - return result - } - return defaultVal -} - // authServiceAdapter adapts auth.Service to api.AuthServiceInterface. type authServiceAdapter struct { service *auth.Service diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 9a7046603..f6e6ff0d9 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -73,22 +73,6 @@ func TestGetEnvInt(t *testing.T) { testutil.AssertEqual(t, 42, getEnvInt(key, 42)) } -func TestGetEnvFloat(t *testing.T) { - key := "TEST_GET_ENV_FLOAT_CUDLY" - defer os.Unsetenv(key) - - // Default value - testutil.AssertEqual(t, 80.0, getEnvFloat(key, 80.0)) - - // Valid float - os.Setenv(key, "95.5") - testutil.AssertEqual(t, 95.5, getEnvFloat(key, 80.0)) - - // Invalid float - returns default - os.Setenv(key, "not-a-float") - testutil.AssertEqual(t, 80.0, getEnvFloat(key, 80.0)) -} - func TestHttpToLambdaRequest_XForwardedFor(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), "GET", "/api/test", nil) req.Header.Set("X-Forwarded-For", "1.2.3.4, 5.6.7.8") @@ -336,7 +320,8 @@ func TestLoadApplicationConfig(t *testing.T) { testutil.SetEnv(t, key, "") } - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "dev", cfg.Version) testutil.AssertEqual(t, 3, cfg.NotificationDaysBefore) @@ -362,7 +347,8 @@ func TestLoadApplicationConfig(t *testing.T) { testutil.SetEnv(t, "CORS_ALLOWED_ORIGIN", "https://example.com") testutil.SetEnv(t, "AWS_LAMBDA_RUNTIME_API", "localhost:9001") - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "1.2.3", cfg.Version) testutil.AssertEqual(t, 7, cfg.NotificationDaysBefore) @@ -379,6 +365,77 @@ func TestLoadApplicationConfig(t *testing.T) { }) } +func TestNewApplicationRejectsMalformedEnvDefaults(t *testing.T) { + for _, key := range []string{"DEFAULT_TERM", "DEFAULT_COVERAGE", "ANALYTICS_COLLECTION_ENABLED", "ANALYTICS_RETENTION_MONTHS", "ANALYTICS_PARTITIONS_AHEAD", "SQS_REPORT_BATCH_ITEM_FAILURES", "DB_HOST"} { + t.Setenv(key, "") + } + for _, tc := range []struct{ key, value string }{ + {"DEFAULT_TERM", "3y"}, + {"DEFAULT_TERM", "999999999999999999999999999"}, + {"DEFAULT_COVERAGE", "80%"}, + {"DEFAULT_COVERAGE", "1e999"}, + {"ANALYTICS_COLLECTION_ENABLED", "no"}, + {"ANALYTICS_COLLECTION_ENABLED", "off"}, + {"ANALYTICS_COLLECTION_ENABLED", "False "}, + {"ANALYTICS_COLLECTION_ENABLED", "disabled"}, + } { + t.Run(tc.key+"="+tc.value, func(t *testing.T) { + t.Setenv(tc.key, tc.value) + if _, err := LoadApplicationConfig(); err == nil || !strings.Contains(err.Error(), tc.key) { + t.Fatalf("expected %s load error, got %v", tc.key, err) + } + app, err := NewApplication(context.Background(), "test") + if app != nil || err == nil || !strings.Contains(err.Error(), tc.key) { + t.Fatalf("expected %s startup error and nil application, got app=%v err=%v", tc.key, app, err) + } + }) + } +} + +func TestLoadApplicationConfigNumericDefaults(t *testing.T) { + t.Setenv("ANALYTICS_COLLECTION_ENABLED", "false") + t.Setenv("ANALYTICS_RETENTION_MONTHS", "") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") + for _, tc := range []struct { + name, term, coverage string + wantTerm int + wantCoverage float64 + }{ + {"unset", "", "", 3, 80}, + {"empty", "", "", 3, 80}, + {"one year", "1", "95.5", 1, 95.5}, + {"zero coverage", "3", "0", 3, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("DEFAULT_TERM", tc.term) + t.Setenv("DEFAULT_COVERAGE", tc.coverage) + if tc.name == "unset" { + if err := os.Unsetenv("DEFAULT_TERM"); err != nil { + t.Fatal(err) + } + if err := os.Unsetenv("DEFAULT_COVERAGE"); err != nil { + t.Fatal(err) + } + } + cfg, err := LoadApplicationConfig() + if err != nil { + t.Fatal(err) + } + t.Setenv("DEFAULT_TERM", "3y") + t.Setenv("DEFAULT_COVERAGE", "80%") + t.Setenv("ANALYTICS_COLLECTION_ENABLED", "off") + testutil.AssertEqual(t, tc.wantTerm, cfg.DefaultTerm) + testutil.AssertEqual(t, tc.wantCoverage, cfg.DefaultCoverage) + testutil.AssertNoError(t, validateAppConfigEnvDefaults(cfg)) + testutil.AssertNoError(t, cfg.Analytics.Validate()) + app := &Application{appConfig: cfg} + result, err := app.handleCollectAnalytics(context.Background()) + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "disabled", result["status"]) + }) + } +} + func TestNewApplicationFromDeps(t *testing.T) { ctx := testutil.TestContext(t) @@ -575,26 +632,6 @@ func TestGetEnvIntLogsOnBadValue(t *testing.T) { "Expected key name in warning log, got: "+logged) } -// TestGetEnvFloatLogsOnBadValue mirrors TestGetEnvIntLogsOnBadValue for floats. -func TestGetEnvFloatLogsOnBadValue(t *testing.T) { - testutil.SetEnv(t, "TEST_ENV_FLOAT_BAD", "eighty") - - var logged string - orig := log.Writer() - var buf strings.Builder - log.SetOutput(&buf) - t.Cleanup(func() { log.SetOutput(orig) }) - - result := getEnvFloat("TEST_ENV_FLOAT_BAD", 80.0) - logged = buf.String() - - testutil.AssertEqual(t, 80.0, result) - testutil.AssertTrue(t, strings.Contains(logged, "WARNING"), - "Expected WARNING log for bad float env var, got: "+logged) - testutil.AssertTrue(t, strings.Contains(logged, "TEST_ENV_FLOAT_BAD"), - "Expected key name in warning log, got: "+logged) -} - func TestInitConfigStore(t *testing.T) { t.Run("missing DB_HOST returns error", func(t *testing.T) { testutil.SetEnv(t, "DB_HOST", "")