From e2aaf6dc35b93372ffe3cabf562c73ebeb5980b3 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 and checks the duration settings with time.ParseDuration. Each number and duration must also be finite and above zero. PORT and NEOIRC_HASHCASH_BITS may be zero: port 0 lets the system pick a port, and 0 bits turns proof of work off. A bad value 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 | 110 ++++++++++++++++++++++-- internal/config/config_test.go | 151 ++++++++++++++++++++++++++++++++- 2 files changed, 252 insertions(+), 9 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 23f85b6..f345dac 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,8 +3,12 @@ package config import ( "errors" + "fmt" "log/slog" + "math" "os" + "strconv" + "time" "github.com/spf13/viper" "go.uber.org/fx" @@ -28,6 +32,8 @@ Type /help for available commands.` const DefaultDBURL = "file:///var/lib/neoirc/state.db" + "?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)" +var errOutOfRange = errors.New("setting out of range") + // Params defines the dependencies for creating a Config. type Params struct { fx.In @@ -63,6 +69,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 or is out of range. func New( _ fx.Lifecycle, params Params, ) (*Config, error) { @@ -107,29 +115,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 +150,93 @@ func New( return cfg, nil } + +// parseSettings reads the number and true/false settings into cfg and +// checks the duration settings. Each number and duration must be above +// zero. PORT and NEOIRC_HASHCASH_BITS may also be zero: port 0 lets the +// system pick a port, and 0 bits turns proof of work off. The error +// names each setting that does not parse or is out of range. +func parseSettings(cfg *Config) error { + return errors.Join( + parseInt("PORT", 0, &cfg.Port), + parseInt("MAX_MESSAGE_SIZE", 1, &cfg.MaxMessageSize), + parseInt("NEOIRC_HASHCASH_BITS", 0, &cfg.HashcashBits), + parseInt("LOGIN_RATE_BURST", 1, &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. It +// returns an error when the value does not parse or is below minimum. +func parseInt(key string, minimum int, value *int) error { + raw := viper.GetString(key) + + parsed, err := strconv.Atoi(raw) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + if parsed < minimum { + return fmt.Errorf("%w: %s is %s, but must be at least %d", + errOutOfRange, key, raw, minimum) + } + + *value = parsed + + return nil +} + +// parseFloat sets *value to the setting key read as a floating-point +// number. It returns an error when the value does not parse or is not a +// finite number above zero. +func parseFloat(key string, value *float64) error { + raw := viper.GetString(key) + + parsed, err := strconv.ParseFloat(raw, 64) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + if math.IsNaN(parsed) || math.IsInf(parsed, 0) || parsed <= 0 { + return fmt.Errorf( + "%w: %s is %s, but must be a finite number above zero", + errOutOfRange, key, raw) + } + + *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 Go duration above zero. +func checkDuration(key, value string) error { + duration, err := time.ParseDuration(value) + if err != nil { + return fmt.Errorf("cannot parse %s: %w", key, err) + } + + if duration <= 0 { + return fmt.Errorf("%w: %s is %s, but must be above zero", + errOutOfRange, key, value) + } + + return nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index fc8163a..dc57b38 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,145 @@ 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) { + // Each whole number is the lowest value its setting allows, and 1ns + // is the shortest duration allowed. + t.Setenv("PORT", "0") + t.Setenv("MAX_MESSAGE_SIZE", "1") + t.Setenv("NEOIRC_HASHCASH_BITS", "0") + t.Setenv("LOGIN_RATE_BURST", "1") + t.Setenv("LOGIN_RATE_LIMIT", "0.5") + t.Setenv("DEBUG", "true") + t.Setenv("MAINTENANCE_MODE", "true") + t.Setenv("MESSAGE_MAX_AGE", "1ns") + t.Setenv("QUEUE_MAX_AGE", "1h30m") + t.Setenv("SESSION_IDLE_TIMEOUT", "45m") + + want := settings{ + Port: 0, + MaxMessageSize: 1, + HashcashBits: 0, + LoginRateBurst: 1, + LoginRateLimit: 0.5, + Debug: true, + MaintenanceMode: true, + MessageMaxAge: "1ns", + QueueMaxAge: "1h30m", + SessionIdleTimeout: "45m", + } + + 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 TestBadSettingStopsStartup(t *testing.T) { + // For each setting, the first value does not parse and the others + // parse but are out of range. + tests := []struct { + key string + values []string + }{ + {key: "PORT", values: []string{"80a", "-1"}}, + {key: "MAX_MESSAGE_SIZE", values: []string{"4k", "0", "-4096"}}, + {key: "NEOIRC_HASHCASH_BITS", values: []string{"twenty", "-1"}}, + {key: "LOGIN_RATE_BURST", values: []string{"five", "0", "-5"}}, + {key: "LOGIN_RATE_LIMIT", values: []string{ + "fast", "0", "-1", "NaN", "Inf", "-Inf", + }}, + {key: "DEBUG", values: []string{"yes"}}, + {key: "MAINTENANCE_MODE", values: []string{"on"}}, + {key: "MESSAGE_MAX_AGE", values: []string{"30d", "0", "-1h"}}, + {key: "QUEUE_MAX_AGE", values: []string{"720", "0s", "-24h"}}, + {key: "SESSION_IDLE_TIMEOUT", values: []string{"a month", "0", "-1m"}}, + } + + for _, test := range tests { + for _, value := range test.values { + t.Run(test.key+"="+value, func(t *testing.T) { + t.Setenv(test.key, value) + + _, err := loadConfig(t) + if err == nil { + t.Fatalf("%s=%q: got no error", test.key, value) + } + + msg := err.Error() + if !strings.Contains(msg, test.key) || + !strings.Contains(msg, value) { + t.Errorf("error %q does not name %s and %q", + msg, test.key, value) + } + }) + } + } +} + func TestIRCListenAddrUnsetKeepsDefault(t *testing.T) { // t.Setenv puts back whatever value was there after the test. t.Setenv("IRC_LISTEN_ADDR", "") -- 2.54.0