From 39cbb34ccc723b1101859b58c5fe5377cf4e0d56 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 27 Oct 2026 21:43:30 +0000 Subject: [PATCH] Stop startup when a setting does not parse (closes #219) config.New read the number and true/false settings with viper's GetInt, GetFloat64 and GetBool. These return zero or false for a value that does not parse, so a typo silently changed how the server ran. config.New now parses these settings with strconv. It also checks that MESSAGE_MAX_AGE, QUEUE_MAX_AGE and SESSION_IDLE_TIMEOUT are valid Go durations. A value that does not parse makes config.New return an error naming the setting and its value, and startup stops. Unset settings keep their defaults. Model: opus-5-5 --- internal/config/config.go | 82 +++++++++++++++++-- internal/config/config_test.go | 143 ++++++++++++++++++++++++++++++++- 2 files changed, 216 insertions(+), 9 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 23f85b6..7f5671d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,8 +3,11 @@ package config import ( "errors" + "fmt" "log/slog" "os" + "strconv" + "time" "github.com/spf13/viper" "go.uber.org/fx" @@ -63,6 +66,8 @@ type Config struct { } // New creates a new Config by reading from files and environment variables. +// It returns an error when a number, true/false or duration setting does +// not parse. func New( _ fx.Lifecycle, params Params, ) (*Config, error) { @@ -107,29 +112,27 @@ func New( cfg := &Config{ DBURL: viper.GetString("DBURL"), - Debug: viper.GetBool("DEBUG"), - Port: viper.GetInt("PORT"), SentryDSN: viper.GetString("SENTRY_DSN"), - MaintenanceMode: viper.GetBool("MAINTENANCE_MODE"), MetricsUsername: viper.GetString("METRICS_USERNAME"), MetricsPassword: viper.GetString("METRICS_PASSWORD"), MessageMaxAge: viper.GetString("MESSAGE_MAX_AGE"), - MaxMessageSize: viper.GetInt("MAX_MESSAGE_SIZE"), QueueMaxAge: viper.GetString("QUEUE_MAX_AGE"), MOTD: viper.GetString("MOTD"), ServerName: viper.GetString("SERVER_NAME"), FederationKey: viper.GetString("FEDERATION_KEY"), SessionIdleTimeout: viper.GetString("SESSION_IDLE_TIMEOUT"), - HashcashBits: viper.GetInt("NEOIRC_HASHCASH_BITS"), OperName: viper.GetString("NEOIRC_OPER_NAME"), OperPassword: viper.GetString("NEOIRC_OPER_PASSWORD"), - LoginRateLimit: viper.GetFloat64("LOGIN_RATE_LIMIT"), - LoginRateBurst: viper.GetInt("LOGIN_RATE_BURST"), IRCListenAddr: viper.GetString("IRC_LISTEN_ADDR"), log: log, params: ¶ms, } + err = parseSettings(cfg) + if err != nil { + return nil, err + } + // Viper counts an empty environment variable as unset, but an // empty IRC_LISTEN_ADDR must turn the IRC listener off. ircListenAddr, set := os.LookupEnv("IRC_LISTEN_ADDR") @@ -144,3 +147,68 @@ func New( return cfg, nil } + +// parseSettings reads the number and true/false settings into cfg and +// checks that each duration setting is a valid Go duration. Its error +// names each setting whose value does not parse. +func parseSettings(cfg *Config) error { + return errors.Join( + parseInt("PORT", &cfg.Port), + parseInt("MAX_MESSAGE_SIZE", &cfg.MaxMessageSize), + parseInt("NEOIRC_HASHCASH_BITS", &cfg.HashcashBits), + parseInt("LOGIN_RATE_BURST", &cfg.LoginRateBurst), + parseFloat("LOGIN_RATE_LIMIT", &cfg.LoginRateLimit), + parseBool("DEBUG", &cfg.Debug), + parseBool("MAINTENANCE_MODE", &cfg.MaintenanceMode), + checkDuration("MESSAGE_MAX_AGE", cfg.MessageMaxAge), + checkDuration("QUEUE_MAX_AGE", cfg.QueueMaxAge), + checkDuration("SESSION_IDLE_TIMEOUT", cfg.SessionIdleTimeout), + ) +} + +// parseInt sets *value to the setting key read as a whole number. +func parseInt(key string, value *int) error { + parsed, err := strconv.Atoi(viper.GetString(key)) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + *value = parsed + + return nil +} + +// parseFloat sets *value to the setting key read as a decimal number. +func parseFloat(key string, value *float64) error { + parsed, err := strconv.ParseFloat(viper.GetString(key), 64) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + *value = parsed + + return nil +} + +// parseBool sets *value to the setting key read as true or false. +func parseBool(key string, value *bool) error { + parsed, err := strconv.ParseBool(viper.GetString(key)) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + *value = parsed + + return nil +} + +// checkDuration returns an error naming the setting key when value is +// not a valid Go duration. +func checkDuration(key, value string) error { + _, err := time.ParseDuration(value) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + return nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index fc8163a..a1af1ca 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config_test import ( "os" + "strings" "testing" "time" @@ -10,7 +11,8 @@ import ( "sneak.berlin/go/neoirc/internal/logger" ) -func newConfig(t *testing.T) *config.Config { +// loadConfig runs config.New and returns its result. +func loadConfig(t *testing.T) (*config.Config, error) { t.Helper() globs := &globals.Globals{ @@ -24,9 +26,15 @@ func newConfig(t *testing.T) *config.Config { t.Fatal(err) } - cfg, err := config.New( + return config.New( nil, config.Params{Globals: globs, Logger: log}, ) +} + +func newConfig(t *testing.T) *config.Config { + t.Helper() + + cfg, err := loadConfig(t) if err != nil { t.Fatal(err) } @@ -34,6 +42,137 @@ func newConfig(t *testing.T) *config.Config { return cfg } +func TestUnsetSettingsKeepDefaults(t *testing.T) { + // Viper counts an empty environment variable as unset. + t.Setenv("PORT", "") + t.Setenv("MAX_MESSAGE_SIZE", "") + t.Setenv("NEOIRC_HASHCASH_BITS", "") + t.Setenv("LOGIN_RATE_BURST", "") + t.Setenv("LOGIN_RATE_LIMIT", "") + t.Setenv("DEBUG", "") + t.Setenv("MAINTENANCE_MODE", "") + t.Setenv("MESSAGE_MAX_AGE", "") + t.Setenv("QUEUE_MAX_AGE", "") + t.Setenv("SESSION_IDLE_TIMEOUT", "") + + // Each duration setting defaults to 30 days. + const thirtyDays = "720h" + + want := settings{ + Port: 8080, + MaxMessageSize: 4096, + HashcashBits: 20, + LoginRateBurst: 5, + LoginRateLimit: 1, + Debug: false, + MaintenanceMode: false, + MessageMaxAge: thirtyDays, + QueueMaxAge: thirtyDays, + SessionIdleTimeout: thirtyDays, + } + + got := settingsOf(newConfig(t)) + if got != want { + t.Errorf("settings = %+v, want %+v", got, want) + } +} + +func TestValidSettingsAreUsed(t *testing.T) { + t.Setenv("PORT", "9000") + t.Setenv("MAX_MESSAGE_SIZE", "1024") + t.Setenv("NEOIRC_HASHCASH_BITS", "0") + t.Setenv("LOGIN_RATE_BURST", "10") + t.Setenv("LOGIN_RATE_LIMIT", "0.5") + t.Setenv("DEBUG", "true") + t.Setenv("MAINTENANCE_MODE", "true") + t.Setenv("MESSAGE_MAX_AGE", "24h") + t.Setenv("QUEUE_MAX_AGE", "1h30m") + t.Setenv("SESSION_IDLE_TIMEOUT", "0") + + want := settings{ + Port: 9000, + MaxMessageSize: 1024, + HashcashBits: 0, + LoginRateBurst: 10, + LoginRateLimit: 0.5, + Debug: true, + MaintenanceMode: true, + MessageMaxAge: "24h", + QueueMaxAge: "1h30m", + SessionIdleTimeout: "0", + } + + got := settingsOf(newConfig(t)) + if got != want { + t.Errorf("settings = %+v, want %+v", got, want) + } +} + +// settings holds the Config fields whose settings config.New parses. +type settings struct { + Port int + MaxMessageSize int + HashcashBits int + LoginRateBurst int + LoginRateLimit float64 + Debug bool + MaintenanceMode bool + MessageMaxAge string + QueueMaxAge string + SessionIdleTimeout string +} + +func settingsOf(cfg *config.Config) settings { + return settings{ + Port: cfg.Port, + MaxMessageSize: cfg.MaxMessageSize, + HashcashBits: cfg.HashcashBits, + LoginRateBurst: cfg.LoginRateBurst, + LoginRateLimit: cfg.LoginRateLimit, + Debug: cfg.Debug, + MaintenanceMode: cfg.MaintenanceMode, + MessageMaxAge: cfg.MessageMaxAge, + QueueMaxAge: cfg.QueueMaxAge, + SessionIdleTimeout: cfg.SessionIdleTimeout, + } +} + +func TestUnparseableSettingStopsStartup(t *testing.T) { + tests := []struct { + key string + value string + }{ + {key: "PORT", value: "80a"}, + {key: "MAX_MESSAGE_SIZE", value: "4k"}, + {key: "NEOIRC_HASHCASH_BITS", value: "twenty"}, + {key: "LOGIN_RATE_BURST", value: "five"}, + {key: "LOGIN_RATE_LIMIT", value: "fast"}, + {key: "DEBUG", value: "yes"}, + {key: "MAINTENANCE_MODE", value: "on"}, + {key: "MESSAGE_MAX_AGE", value: "30d"}, + {key: "QUEUE_MAX_AGE", value: "720"}, + {key: "SESSION_IDLE_TIMEOUT", value: "a month"}, + } + + for _, test := range tests { + t.Run(test.key, func(t *testing.T) { + t.Setenv(test.key, test.value) + + _, err := loadConfig(t) + if err == nil { + t.Fatalf("%s=%q: got no error", test.key, test.value) + } + + msg := err.Error() + if !strings.Contains(msg, test.key) || + !strings.Contains(msg, test.value) { + t.Errorf("error %q does not name %s and %q", + msg, test.key, test.value) + } + }) + } +} + func TestIRCListenAddrUnsetKeepsDefault(t *testing.T) { // t.Setenv puts back whatever value was there after the test. t.Setenv("IRC_LISTEN_ADDR", "")