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

internal/config/config.go

@@ -44,7 +44,7 @@ var errOutOfRange = errors.New("setting out of range")
 
 var errNeverMatches = errors.New("entry can never match a connection")
 
-var errListOrMap = errors.New("a setting cannot be a list or a map")
+var errListOrMap = errors.New("a list or a map")
 
 // Params defines the dependencies for creating a Config.
 type Params struct {
@@ -138,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()
@@ -169,7 +162,8 @@ func parseSettings(cfg *Config) error {
 		parseString("FEDERATION_KEY", &cfg.FederationKey),
 		parseString("NEOIRC_OPER_NAME", &cfg.OperName),
 		parseString("NEOIRC_OPER_PASSWORD", &cfg.OperPassword),
-		parseString("IRC_LISTEN_ADDR", &cfg.IRCListenAddr),
+		// 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),
@@ -187,13 +181,12 @@ func parseSettings(cfg *Config) error {
 // 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.
+// would return the empty string. The error leaves out the value, which
+// may be a password.
 func getString(key string) (string, error) {
-	value := viper.Get(key)
-
-	raw, err := cast.ToStringE(value)
+	raw, err := cast.ToStringE(viper.Get(key))
 	if err != nil {
-		return "", fmt.Errorf("%w: %s is %v", errListOrMap, key, value)
+		return "", fmt.Errorf("%s is %w", key, errListOrMap)
 	}
 
 	return raw, nil
@@ -211,20 +204,30 @@ func parseString(key string, value *string) error {
 	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 {
-	raw, err := getString("TRUSTED_PROXIES")
-	if err != nil {
-		return err
+// 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
 	}
 
-	fromEnv, set := os.LookupEnv("TRUSTED_PROXIES")
-	if set {
-		raw = fromEnv
+	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

internal/config/config_test.go

@@ -352,6 +352,66 @@ func TestListOrMapInConfigFileStopsStartup(t *testing.T) {
 	}
 }
 
+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")
+
+			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)