Skip to content

fix(server): reject malformed startup environment defaults #454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Sep 29, 2026
Merged

fix(server): reject malformed startup environment defaults #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 13 additions & 18 deletions internal/server/analytics_collect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Expand Down
58 changes: 54 additions & 4 deletions internal/server/analytics_collect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -34,15 +82,17 @@ 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")
})

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")
})
Expand Down
54 changes: 30 additions & 24 deletions internal/server/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
Expand Down
113 changes: 75 additions & 38 deletions internal/server/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)

Expand Down Expand Up @@ -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", "")
Expand Down
Loading