From dc19d0d7d156b70a6095396c910142105f34b7b2 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 10 Nov 2026 21:54:09 +0000 Subject: [PATCH] Stop startup on a list or map setting in neoirc.yaml (closes #268) Every setting is now read through one helper that uses cast.ToStringE, the conversion viper.GetString wraps, keeping its error instead of returning the empty string. TRUSTED_PROXIES uses the same helper. A config file that does not parse makes New return an error naming the file instead of panicking. Model: opus-5-5 --- README.md | 3 +- go.mod | 2 +- internal/config/config.go | 133 +++++++++++++++++++++------------ internal/config/config_test.go | 128 ++++++++++++++++++++----------- 4 files changed, 175 insertions(+), 91 deletions(-) diff --git a/README.md b/README.md index 85155a9..2b3e501 100644 --- a/README.md +++ b/README.md @@ -1722,7 +1722,8 @@ exceptions: A `neoirc.yaml` that is not valid YAML stops the server at startup. A setting that does not parse or is out of range also stops the server at startup, and the -error names the setting. +error names the setting. The same holds for a setting given a list or a map in +`neoirc.yaml`. - `PORT` (default `8080`): the HTTP port. `0` lets the system pick a free port. - `IRC_LISTEN_ADDR` (default `:6667`): the address of the IRC listener. The diff --git a/go.mod b/go.mod index 73baceb..49cc678 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/prometheus/client_golang v1.23.2 github.com/rivo/tview v0.42.0 github.com/slok/go-http-metrics v0.13.0 + github.com/spf13/cast v1.10.0 github.com/spf13/viper v1.21.0 go.uber.org/fx v1.24.0 go4.org/netipx v0.0.0-20260823151212-3075585bcbeb @@ -41,7 +42,6 @@ require ( github.com/sagikazarmark/locafero v0.11.0 // indirect github.com/sourcegraph/conc v0.3.1-0.20240121214520-5f936abd7ae8 // indirect github.com/spf13/afero v1.15.0 // indirect - github.com/spf13/cast v1.10.0 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/subosito/gotenv v1.6.0 // indirect go.uber.org/dig v1.19.0 // indirect diff --git a/internal/config/config.go b/internal/config/config.go index 01c47a5..c604954 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/spf13/cast" "github.com/spf13/viper" "go.uber.org/fx" "go4.org/netipx" @@ -43,7 +44,7 @@ var errOutOfRange = errors.New("setting out of range") var errNeverMatches = errors.New("entry can never match a connection") -var errNotString = errors.New("not a string") +var errListOrMap = errors.New("a setting cannot be a list or a map") // Params defines the dependencies for creating a Config. type Params struct { @@ -81,8 +82,9 @@ type Config struct { } // New creates a new Config by reading from files and environment variables. -// It returns an error when a number, true/false, duration or address -// setting does not parse or is out of range. +// It returns an error when the config file does not parse, when the file +// gives a setting a list or a map, and when a number, true/false, +// duration or address setting does not parse or is out of range. func New( _ fx.Lifecycle, params Params, ) (*Config, error) { @@ -121,27 +123,14 @@ func New( if err != nil { var notFound viper.ConfigFileNotFoundError if !errors.As(err, ¬Found) { - log.Error("config file malformed", "error", err) - panic(err) + return nil, fmt.Errorf("cannot read %s: %w", + viper.ConfigFileUsed(), err) } } cfg := &Config{ - DBURL: viper.GetString("DBURL"), - SentryDSN: viper.GetString("SENTRY_DSN"), - MetricsUsername: viper.GetString("METRICS_USERNAME"), - MetricsPassword: viper.GetString("METRICS_PASSWORD"), - MessageMaxAge: viper.GetString("MESSAGE_MAX_AGE"), - 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"), - OperName: viper.GetString("NEOIRC_OPER_NAME"), - OperPassword: viper.GetString("NEOIRC_OPER_PASSWORD"), - IRCListenAddr: viper.GetString("IRC_LISTEN_ADDR"), - log: log, - params: ¶ms, + log: log, + params: ¶ms, } err = parseSettings(cfg) @@ -164,13 +153,23 @@ func New( return cfg, nil } -// parseSettings reads the number, true/false and address 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. +// parseSettings reads every setting into cfg. 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 is a list or a map, does not parse or +// is out of range. func parseSettings(cfg *Config) error { return errors.Join( + parseString("DBURL", &cfg.DBURL), + parseString("SENTRY_DSN", &cfg.SentryDSN), + parseString("METRICS_USERNAME", &cfg.MetricsUsername), + parseString("METRICS_PASSWORD", &cfg.MetricsPassword), + parseString("MOTD", &cfg.MOTD), + parseString("SERVER_NAME", &cfg.ServerName), + parseString("FEDERATION_KEY", &cfg.FederationKey), + parseString("NEOIRC_OPER_NAME", &cfg.OperName), + parseString("NEOIRC_OPER_PASSWORD", &cfg.OperPassword), + parseString("IRC_LISTEN_ADDR", &cfg.IRCListenAddr), parseInt("PORT", 0, &cfg.Port), parseInt("MAX_MESSAGE_SIZE", 1, &cfg.MaxMessageSize), parseInt("NEOIRC_HASHCASH_BITS", 0, &cfg.HashcashBits), @@ -178,39 +177,61 @@ func parseSettings(cfg *Config) error { 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), + parseDuration("MESSAGE_MAX_AGE", &cfg.MessageMaxAge), + parseDuration("QUEUE_MAX_AGE", &cfg.QueueMaxAge), + parseDuration("SESSION_IDLE_TIMEOUT", &cfg.SessionIdleTimeout), parseTrustedProxies(&cfg.TrustedProxies), ) } +// getString returns the setting key as a string. A number or a true/false +// value in the config file becomes its text, as with viper.GetString. A +// list or a map in the config file is an error, where viper.GetString +// would return the empty string. +func getString(key string) (string, error) { + value := viper.Get(key) + + raw, err := cast.ToStringE(value) + if err != nil { + return "", fmt.Errorf("%w: %s is %v", errListOrMap, key, value) + } + + return raw, nil +} + +// parseString sets *value to the setting key. +func parseString(key string, value *string) error { + raw, err := getString(key) + if err != nil { + return err + } + + *value = raw + + return nil +} + // parseTrustedProxies sets *value to the addresses in TRUSTED_PROXIES, a // comma-separated list of networks such as 10.0.0.0/8 and single // addresses such as 192.0.2.1. The empty string trusts nothing. Viper // counts an empty environment variable as unset, so the environment is // read directly as well. func parseTrustedProxies(value **netipx.IPSet) error { - setting := viper.Get("TRUSTED_PROXIES") + raw, err := getString("TRUSTED_PROXIES") + if err != nil { + return err + } fromEnv, set := os.LookupEnv("TRUSTED_PROXIES") if set { - setting = fromEnv - } - - // A YAML list or map in the config file is not a string, and - // viper.GetString would turn it into the empty string. - raw, isString := setting.(string) - if !isString { - return fmt.Errorf("cannot parse TRUSTED_PROXIES: %v is %w", - setting, errNotString) + raw = fromEnv } var builder netipx.IPSetBuilder if strings.TrimSpace(raw) != "" { for entry := range strings.SplitSeq(raw, ",") { - err := addTrustedProxy(&builder, strings.TrimSpace(entry)) + err = addTrustedProxy(&builder, strings.TrimSpace(entry)) if err != nil { return fmt.Errorf("cannot parse TRUSTED_PROXIES: %w", err) } @@ -267,7 +288,10 @@ func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error { // 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) + raw, err := getString(key) + if err != nil { + return err + } parsed, err := strconv.Atoi(raw) if err != nil { @@ -288,7 +312,10 @@ func parseInt(key string, minimum int, value *int) error { // 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) + raw, err := getString(key) + if err != nil { + return err + } parsed, err := strconv.ParseFloat(raw, 64) if err != nil { @@ -308,7 +335,12 @@ func parseFloat(key string, value *float64) error { // 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)) + raw, err := getString(key) + if err != nil { + return err + } + + parsed, err := strconv.ParseBool(raw) if err != nil { return fmt.Errorf("cannot parse %s: %w", key, err) } @@ -318,18 +350,25 @@ func parseBool(key string, value *bool) error { 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) +// parseDuration sets *value to the setting key. It returns an error when +// the value is not a Go duration above zero. +func parseDuration(key string, value *string) error { + raw, err := getString(key) + if err != nil { + return err + } + + duration, err := time.ParseDuration(raw) 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) + errOutOfRange, key, raw) } + *value = raw + return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9ab5dfd..f7d2802 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -287,49 +287,93 @@ func TestTrustedProxies(t *testing.T) { } } -func TestTrustedProxiesNotAStringInYAMLStopsStartup(t *testing.T) { - // YAML reads the first value as a list and the second as a map. - values := []string{"[10.0.0.1]", "{proxy: 10.0.0.1}"} +// unsetEnv removes the environment variable key for the rest of the +// test, so that config.New reads key from the config file. +func unsetEnv(t *testing.T, key string) { + t.Helper() - for _, value := range values { - t.Run(value, func(t *testing.T) { - // t.Setenv puts back whatever value was there after the - // test, including when Unsetenv removes it. - t.Setenv("TRUSTED_PROXIES", "") + // t.Setenv puts back whatever value was there after the test, + // including when Unsetenv removes it. + t.Setenv(key, "") - err := os.Unsetenv("TRUSTED_PROXIES") - if err != nil { - t.Fatal(err) - } - - // loadConfig names the app neoirc-test, so config.New reads - // $HOME/.config/neoirc-test/neoirc-test.yaml. - home := t.TempDir() - t.Setenv("HOME", home) - - dir := filepath.Join(home, ".config", "neoirc-test") - - err = os.MkdirAll(dir, 0o700) - if err != nil { - t.Fatal(err) - } - - err = os.WriteFile( - filepath.Join(dir, "neoirc-test.yaml"), - []byte("TRUSTED_PROXIES: "+value+"\n"), 0o600, - ) - if err != nil { - t.Fatal(err) - } - - // Viper keeps the values it read from this file after the - // file is gone. Reset clears them for later tests. - t.Cleanup(viper.Reset) - - _, err = loadConfig(t) - if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") { - t.Errorf("error = %v, want one naming TRUSTED_PROXIES", err) - } - }) + err := os.Unsetenv(key) + if err != nil { + t.Fatal(err) + } +} + +// writeConfigFile writes content to the config file config.New reads +// when HOME is home, and returns the file's path. +func writeConfigFile(t *testing.T, home, content string) string { + t.Helper() + + // loadConfig names the app neoirc-test, so config.New reads + // $HOME/.config/neoirc-test/neoirc-test.yaml. + dir := filepath.Join(home, ".config", "neoirc-test") + + err := os.MkdirAll(dir, 0o700) + if err != nil { + t.Fatal(err) + } + + path := filepath.Join(dir, "neoirc-test.yaml") + + err = os.WriteFile(path, []byte(content), 0o600) + if err != nil { + t.Fatal(err) + } + + // Viper keeps the file and the values it read from it after the + // file is gone. Reset clears them for later tests. + t.Cleanup(viper.Reset) + + return path +} + +func TestListOrMapInConfigFileStopsStartup(t *testing.T) { + // YAML reads the first value as a list and the second as a map. + values := []string{`[":6667"]`, `{addr: ":6667"}`} + keys := []string{"IRC_LISTEN_ADDR", "MOTD", "TRUSTED_PROXIES"} + + for _, key := range keys { + for _, value := range values { + t.Run(key+"="+value, func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + unsetEnv(t, key) + writeConfigFile(t, home, key+": "+value+"\n") + + _, err := loadConfig(t) + if err == nil || !strings.Contains(err.Error(), key) { + t.Errorf("error = %v, want one naming %s", err, key) + } + }) + } + } +} + +func TestSingleValuesInConfigFileAreUsed(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + unsetEnv(t, "PORT") + unsetEnv(t, "DEBUG") + unsetEnv(t, "MOTD") + writeConfigFile(t, home, "PORT: 9000\nDEBUG: true\nMOTD: hello\n") + + cfg := newConfig(t) + if cfg.Port != 9000 || !cfg.Debug || cfg.MOTD != "hello" { + t.Errorf("Port, Debug, MOTD = %d, %t, %q, want 9000, true, %q", + cfg.Port, cfg.Debug, cfg.MOTD, "hello") + } +} + +func TestConfigFileThatDoesNotParseStopsStartup(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := writeConfigFile(t, home, "MOTD: [unclosed\n") + + _, err := loadConfig(t) + if err == nil || !strings.Contains(err.Error(), path) { + t.Errorf("error = %v, want one naming %s", err, path) } }