From 72d8f2c12b2e103dba9e8a79fa9e26e2e92ee26f Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 11 Nov 2026 00:07:58 +0100 Subject: [PATCH] Stop startup on a list or map setting in neoirc.yaml (closes #268) viper.GetString returns the empty string for a list or a map in neoirc.yaml, so IRC_LISTEN_ADDR: [":6667"] turned the IRC listener off and MOTD or SERVER_NAME went blank without an error. Every setting is now read through getString in internal/config, which uses cast.ToStringE, the conversion viper.GetString wraps. A list or a map is an error naming the setting, without its value, since the value may be a password. An environment value, the empty string included, still wins over the file. TRUSTED_PROXIES uses the same helper. A config file that does not parse makes New return an error naming the file instead of calling panic. Model: opus-5-5 --- README.md | 3 +- go.mod | 2 +- internal/config/config.go | 164 +++++++++++++++++++------------ internal/config/config_test.go | 174 ++++++++++++++++++++++++++------- 4 files changed, 245 insertions(+), 98 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..d040f4e 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 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) @@ -149,13 +138,6 @@ func New( 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") - if set { - cfg.IRCListenAddr = ircListenAddr - } - if cfg.Debug { params.Logger.EnableDebugLogging() cfg.log = params.Logger.Get() @@ -164,13 +146,24 @@ 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), + // The empty string turns the IRC listener off. + parseStringAllowEmptyEnv("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 +171,70 @@ 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), ) } -// 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") - - fromEnv, set := os.LookupEnv("TRUSTED_PROXIES") - if set { - setting = fromEnv +// 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. The error leaves out the value, which +// may be a password. +func getString(key string) (string, error) { + raw, err := cast.ToStringE(viper.Get(key)) + if err != nil { + return "", fmt.Errorf("%s is %w", key, errListOrMap) } - // 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) + 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 +} + +// parseStringAllowEmptyEnv sets *value to the setting key, as parseString +// does, but an environment variable set to the empty string counts as +// set. Viper counts such a variable as unset and uses the config file or +// the default instead. +func parseStringAllowEmptyEnv(key string, value *string) error { + fromEnv, set := os.LookupEnv(key) + if set { + *value = fromEnv + + return nil + } + + return parseString(key, value) +} + +// 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. +func parseTrustedProxies(value **netipx.IPSet) error { + var raw string + + err := parseStringAllowEmptyEnv("TRUSTED_PROXIES", &raw) + if err != nil { + return err } 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 +291,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 +315,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 +338,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 +353,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..6c361cf 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -287,49 +287,153 @@ func TestTrustedProxies(t *testing.T) { } } -func TestTrustedProxiesNotAStringInYAMLStopsStartup(t *testing.T) { +// 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() + + // t.Setenv puts back whatever value was there after the test, + // including when Unsetenv removes it. + t.Setenv(key, "") + + 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{"[10.0.0.1]", "{proxy: 10.0.0.1}"} + values := []string{`[":6667"]`, `{addr: ":6667"}`} + keys := []string{"IRC_LISTEN_ADDR", "MOTD", "TRUSTED_PROXIES"} - 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", "") + 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 := os.Unsetenv("TRUSTED_PROXIES") - if err != nil { - t.Fatal(err) - } + _, err := loadConfig(t) + if err == nil || !strings.Contains(err.Error(), key) { + t.Errorf("error = %v, want one naming %s", err, key) + } + }) + } + } +} - // loadConfig names the app neoirc-test, so config.New reads - // $HOME/.config/neoirc-test/neoirc-test.yaml. +func TestListOrMapErrorLeavesOutValue(t *testing.T) { + const key = "NEOIRC_OPER_PASSWORD" + + home := t.TempDir() + t.Setenv("HOME", home) + unsetEnv(t, key) + // YAML reads an unquoted {hunter2} as a map. + writeConfigFile(t, home, key+": {hunter2}\n") + + _, err := loadConfig(t) + if err == nil || !strings.Contains(err.Error(), key) { + t.Fatalf("error = %v, want one naming %s", err, key) + } + + if strings.Contains(err.Error(), "hunter2") { + t.Errorf("error %q shows the password", err) + } +} + +func TestEmptyEnvironmentValueWinsOverConfigFile(t *testing.T) { + const ( + ircListenAddr = "IRC_LISTEN_ADDR" + trustedProxies = "TRUSTED_PROXIES" + ) + + // Each setting is the empty string in the environment and a list in + // the config file. The environment wins, so the list is never read. + tests := []struct { + key string + empty func(cfg *config.Config) bool + }{ + { + key: ircListenAddr, + empty: func(cfg *config.Config) bool { + return cfg.IRCListenAddr == "" + }, + }, + { + key: trustedProxies, + empty: func(cfg *config.Config) bool { + return len(cfg.TrustedProxies.Prefixes()) == 0 + }, + }, + } + + for _, test := range tests { + t.Run(test.key, func(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) + t.Setenv(test.key, "") + writeConfigFile(t, home, test.key+": [\":6667\"]\n") - 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) + cfg := newConfig(t) + if !test.empty(cfg) { + t.Errorf("%s is not the empty string", test.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) + } +}