From ce06170604903270ca2eb4f43f5fdec288a70666 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 21:01:03 +0000 Subject: [PATCH] chore: conform post-merge config validation code to v2.12.2 lint config The stricter canonical .golangci.yml surfaced 81 findings in the config validation code merged from main (#53). Fix them all with no behavior change: static sentinel errors wrapped with %w preserving the existing messages (err113), config key name constants (goconst), t.Parallel() throughout except the Setenv/Chdir test (paralleltest), white-box test renamed to config_validation_internal_test.go (testpackage), case tables extracted into builder functions plus a shared runAbortCases helper (funlen/dupl/gochecknoglobals), plain error assignments (noinlineerr), any instead of interface{} and strings.SplitSeq (modernize), slog.DiscardHandler (sloglint), 88-col wrapping (lll), and removal of two stale nolint:gosec directives (nolintlint). --- internal/config/config.go | 229 +++++--- internal/config/config_internal_test.go | 2 +- ....go => config_validation_internal_test.go} | 501 ++++++++++-------- 3 files changed, 417 insertions(+), 315 deletions(-) rename internal/config/{config_validation_test.go => config_validation_internal_test.go} (72%) diff --git a/internal/config/config.go b/internal/config/config.go index f94e0f6..49b5d25 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2,6 +2,7 @@ package config import ( + "errors" "fmt" "log/slog" "math" @@ -25,9 +26,54 @@ const ( DefaultUpstreamConnectionsPerHost = 20 ) +// Configuration key names. +const ( + keyDebug = "debug" + keyMaintenanceMode = "maintenance_mode" + keyPort = "port" + keyStateDir = "state_dir" + keySentryDSN = "sentry_dsn" + keyDBURL = "db_url" + keyMetrics = "metrics" + keyMetricsUsername = "metrics.username" + keyMetricsPassword = "metrics.password" + keySigningKey = "signing_key" + keyAllowlistHosts = "allowlist_hosts" + keyAllowHTTP = "allow_http" + keyUpstreamConnectionsPerHost = "upstream_connections_per_host" +) + +// Static validation errors. Each use site attaches the offending key +// and value by wrapping these with fmt.Errorf and %w. +var ( + errValueRequired = errors.New("a value is required") + errValueEmpty = errors.New("value must not be empty") + errUnknownConfigKeys = errors.New("unknown config keys") + errNotAString = errors.New("not a string") + errNotAnInteger = errors.New("not an integer") + errNotABoolean = errors.New("not a boolean") + errNotAStringList = errors.New("not a list of strings") + errNotAMetricsMap = errors.New("not a map of metrics settings") + errEmptyListEntry = errors.New("list contains an empty entry") + errEmptyEntry = errors.New("contains an empty entry") + errNotAValidURL = errors.New("not a valid URL") + errPortOutOfRange = errors.New("outside the valid port range") + errTooFewConnections = errors.New("must be at least 1") + errValueTooShort = errors.New("value too short") + errMustBeSetTogether = errors.New("must be set together") + errValueNull = errors.New( + "value is null; omit the key entirely to use the default") + errValuesNull = errors.New( + "value is null; omit a key entirely to use its default") + errNotBareHostname = errors.New( + "must be a bare hostname without scheme, path, or whitespace") + errNoHostnameLabels = errors.New("contains no hostname labels") +) + // Params defines dependencies for Config. type Params struct { fx.In + Globals *globals.Globals Logger *logger.Logger } @@ -69,7 +115,8 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { return nil, err } - if err := c.ensureStateDirWritable(); err != nil { + err = c.ensureStateDirWritable() + if err != nil { return nil, err } @@ -87,11 +134,13 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { // to omitted keys, never to invalid explicit values. func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { if sc != nil { - if err := validateKnownKeys(sc); err != nil { + err := validateKnownKeys(sc) + if err != nil { return nil, err } - if err := validateAllowlistHostsValue(sc); err != nil { + err = validateAllowlistHostsValue(sc) + if err != nil { return nil, err } } @@ -99,30 +148,30 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { loader := &strictLoader{sc: sc} c := &Config{ - Debug: loader.boolVal("debug", false), - MaintenanceMode: loader.boolVal("maintenance_mode", false), - Port: loader.intVal("port", DefaultPort), - StateDir: loader.stringVal("state_dir", DefaultStateDir), - SentryDSN: loader.stringVal("sentry_dsn", ""), - MetricsUsername: loader.stringVal("metrics.username", ""), - MetricsPassword: loader.stringVal("metrics.password", ""), - SigningKey: loader.stringVal("signing_key", ""), + Debug: loader.boolVal(keyDebug, false), + MaintenanceMode: loader.boolVal(keyMaintenanceMode, false), + Port: loader.intVal(keyPort, DefaultPort), + StateDir: loader.stringVal(keyStateDir, DefaultStateDir), + SentryDSN: loader.stringVal(keySentryDSN, ""), + MetricsUsername: loader.stringVal(keyMetricsUsername, ""), + MetricsPassword: loader.stringVal(keyMetricsPassword, ""), + SigningKey: loader.stringVal(keySigningKey, ""), AllowlistHosts: getStringSlice(sc), - AllowHTTP: loader.boolVal("allow_http", false), + AllowHTTP: loader.boolVal(keyAllowHTTP, false), UpstreamConnectionsPerHost: loader.intVal( - "upstream_connections_per_host", DefaultUpstreamConnectionsPerHost), + keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), } // Build DBURL from StateDir if not explicitly set. The derived URL // is a default: it applies only when db_url is omitted, never to an // explicitly empty value. - c.DBURL = loader.stringVal("db_url", "") + c.DBURL = loader.stringVal(keyDBURL, "") if c.DBURL == "" && loader.err == nil { if sc != nil { - if _, present := sc.Get("db_url"); present { + if _, present := sc.Get(keyDBURL); present { return nil, fmt.Errorf( - "config key %q: value must not be empty; omit the key to derive it from state_dir", - "db_url") + "config key %q: %w; omit the key to derive it from state_dir", + keyDBURL, errValueEmpty) } } @@ -133,7 +182,8 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { return nil, loader.err } - if err := c.validate(); err != nil { + err := c.validate() + if err != nil { return nil, err } @@ -162,23 +212,22 @@ func validateKnownKeys(sc *smartconfig.Config) error { continue } - if key == "metrics" { - metricsMap, ok := value.(map[string]interface{}) + if key == keyMetrics { + metricsMap, ok := value.(map[string]any) if !ok { - return fmt.Errorf( - "config key %q: value %v is not a map of metrics settings", - "metrics", value) + return fmt.Errorf("config key %q: value %v is %w", + keyMetrics, value, errNotAMetricsMap) } for subkey, subvalue := range metricsMap { if subkey != "username" && subkey != "password" { - unknown = append(unknown, "metrics."+subkey) + unknown = append(unknown, keyMetrics+"."+subkey) continue } if subvalue == nil { - nullKeys = append(nullKeys, "metrics."+subkey) + nullKeys = append(nullKeys, keyMetrics+"."+subkey) } } } @@ -187,7 +236,7 @@ func validateKnownKeys(sc *smartconfig.Config) error { if len(unknown) > 0 { sort.Strings(unknown) - return fmt.Errorf("unknown config keys: %s", strings.Join(unknown, ", ")) + return fmt.Errorf("%w: %s", errUnknownConfigKeys, strings.Join(unknown, ", ")) } if len(nullKeys) > 0 { @@ -197,9 +246,8 @@ func validateKnownKeys(sc *smartconfig.Config) error { return errNullConfigValue(nullKeys[0]) } - return fmt.Errorf( - "config keys %s: value is null; omit a key entirely to use its default", - strings.Join(nullKeys, ", ")) + return fmt.Errorf("config keys %s: %w", + strings.Join(nullKeys, ", "), errValuesNull) } return nil @@ -209,17 +257,16 @@ func validateKnownKeys(sc *smartconfig.Config) error { // null (including the bare "key:" form and the "~" alias). Silently // applying the default would mask a truncated or typo'd config entry. func errNullConfigValue(key string) error { - return fmt.Errorf( - "config key %q: value is null; omit the key entirely to use the default", key) + return fmt.Errorf("config key %q: %w", key, errValueNull) } // isKnownConfigKey reports whether key is a permitted top-level // configuration key. func isKnownConfigKey(key string) bool { switch key { - case "debug", "maintenance_mode", "port", "state_dir", "sentry_dsn", - "db_url", "metrics", "signing_key", "allowlist_hosts", "allow_http", - "upstream_connections_per_host", "env": + case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, + keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, + keyUpstreamConnectionsPerHost, "env": return true } @@ -232,28 +279,30 @@ func isKnownConfigKey(key string) bool { func (c *Config) ensureStateDirWritable() error { const stateDirPerms = 0o750 - if err := os.MkdirAll(c.StateDir, stateDirPerms); err != nil { + err := os.MkdirAll(c.StateDir, stateDirPerms) + if err != nil { return fmt.Errorf("config key %q: cannot create directory %q: %w", - "state_dir", c.StateDir, err) + keyStateDir, c.StateDir, err) } probe, err := os.CreateTemp(c.StateDir, ".startup-write-probe-*") if err != nil { return fmt.Errorf("config key %q: directory %q is not writable: %w", - "state_dir", c.StateDir, err) + keyStateDir, c.StateDir, err) } probePath := probe.Name() - if err := probe.Close(); err != nil { + err = probe.Close() + if err != nil { return fmt.Errorf("config key %q: cannot close probe file %q: %w", - "state_dir", probePath, err) + keyStateDir, probePath, err) } - //nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir - if err := os.Remove(probePath); err != nil { + err = os.Remove(probePath) + if err != nil { return fmt.Errorf("config key %q: cannot remove probe file %q: %w", - "state_dir", probePath, err) + keyStateDir, probePath, err) } return nil @@ -264,33 +313,35 @@ func (c *Config) ensureStateDirWritable() error { func (c *Config) validate() error { // The signing key value is never echoed in error messages. if c.SigningKey == "" { - return fmt.Errorf("config key %q: a value is required", "signing_key") + return fmt.Errorf("config key %q: %w", keySigningKey, errValueRequired) } // Minimum key length for security (32 bytes = 256 bits) const minKeyLength = 32 if len(c.SigningKey) < minKeyLength { - return fmt.Errorf("config key %q: value must be at least %d characters, got %d", - "signing_key", minKeyLength, len(c.SigningKey)) + return fmt.Errorf("config key %q: %w: must be at least %d characters, got %d", + keySigningKey, errValueTooShort, minKeyLength, len(c.SigningKey)) } const maxPort = 65535 if c.Port < 1 || c.Port > maxPort { - return fmt.Errorf("config key %q: value %d is outside the valid port range 1-%d", - "port", c.Port, maxPort) + return fmt.Errorf("config key %q: value %d is %w 1-%d", + keyPort, c.Port, errPortOutOfRange, maxPort) } if c.UpstreamConnectionsPerHost < 1 { - return fmt.Errorf("config key %q: value %d must be at least 1", - "upstream_connections_per_host", c.UpstreamConnectionsPerHost) + return fmt.Errorf("config key %q: value %d %w", + keyUpstreamConnectionsPerHost, c.UpstreamConnectionsPerHost, + errTooFewConnections) } if c.StateDir == "" { - return fmt.Errorf("config key %q: value must not be empty", "state_dir") + return fmt.Errorf("config key %q: %w", keyStateDir, errValueEmpty) } for _, host := range c.AllowlistHosts { - if err := validateAllowlistHost(host); err != nil { + err := validateAllowlistHost(host) + if err != nil { return err } } @@ -298,14 +349,14 @@ func (c *Config) validate() error { if c.SentryDSN != "" { parsed, err := url.Parse(c.SentryDSN) if err != nil || parsed.Scheme == "" || parsed.Host == "" { - return fmt.Errorf("config key %q: value %q is not a valid URL", - "sentry_dsn", c.SentryDSN) + return fmt.Errorf("config key %q: value %q is %w", + keySentryDSN, c.SentryDSN, errNotAValidURL) } } if (c.MetricsUsername == "") != (c.MetricsPassword == "") { - return fmt.Errorf("config keys %q and %q must be set together", - "metrics.username", "metrics.password") + return fmt.Errorf("config keys %q and %q %w", + keyMetricsUsername, keyMetricsPassword, errMustBeSetTogether) } return nil @@ -320,21 +371,20 @@ func (c *Config) validate() error { // disable URL signing. func validateAllowlistHost(host string) error { if strings.Contains(host, "://") || strings.ContainsAny(host, "/ \t") { - return fmt.Errorf( - "config key %q: entry %q must be a bare hostname without scheme, path, or whitespace", - "allowlist_hosts", host) + return fmt.Errorf("config key %q: entry %q %w", + keyAllowlistHosts, host, errNotBareHostname) } if strings.Trim(host, ".") == "" { - return fmt.Errorf( - "config key %q: entry %q contains no hostname labels", - "allowlist_hosts", host) + return fmt.Errorf("config key %q: entry %q %w", + keyAllowlistHosts, host, errNoHostnameLabels) } return nil } -// loadConfigFile loads configuration from PIXA_CONFIG_PATH env var or standard locations. +// loadConfigFile loads configuration from the PIXA_CONFIG_PATH env var +// or standard locations. func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, error) { // Check for explicit config path from environment if envPath := os.Getenv("PIXA_CONFIG_PATH"); envPath != "" { @@ -360,8 +410,9 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro for _, path := range configPaths { cleanPath := filepath.Clean(path) - //nolint:gosec // G703: paths are hardcoded config locations - if _, statErr := os.Stat(cleanPath); statErr == nil { + + _, statErr := os.Stat(cleanPath) + if statErr == nil { // A config file that exists but does not parse is a fatal // startup error, never something to skip over. sc, err := smartconfig.NewFromConfigPath(path) @@ -444,8 +495,8 @@ func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { str, ok := raw.(string) if !ok { - return "", fmt.Errorf("config key %q: value %v (%T) is not a string", - key, raw, raw) + return "", fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotAString) } return str, nil @@ -475,20 +526,22 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { return int(val), nil case float64: if val != math.Trunc(val) { - return 0, fmt.Errorf("config key %q: value %v is not an integer", key, val) + return 0, fmt.Errorf("config key %q: value %v is %w", + key, val, errNotAnInteger) } return int(val), nil case string: parsed, err := strconv.Atoi(strings.TrimSpace(val)) if err != nil { - return 0, fmt.Errorf("config key %q: value %q is not an integer", key, val) + return 0, fmt.Errorf("config key %q: value %q is %w", + key, val, errNotAnInteger) } return parsed, nil default: - return 0, fmt.Errorf("config key %q: value %v (%T) is not an integer", - key, raw, raw) + return 0, fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotAnInteger) } } @@ -516,13 +569,14 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) case string: parsed, err := strconv.ParseBool(strings.TrimSpace(val)) if err != nil { - return false, fmt.Errorf("config key %q: value %q is not a boolean", key, val) + return false, fmt.Errorf("config key %q: value %q is %w", + key, val, errNotABoolean) } return parsed, nil default: - return false, fmt.Errorf("config key %q: value %v (%T) is not a boolean", - key, raw, raw) + return false, fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotABoolean) } } @@ -532,28 +586,27 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) // (or a comma-separated string), a non-string entry, or an empty entry // is an error, never silently skipped. func validateAllowlistHostsValue(sc *smartconfig.Config) error { - const key = "allowlist_hosts" - - raw, ok := sc.Get(key) + raw, ok := sc.Get(keyAllowlistHosts) if !ok { return nil } if raw == nil { - return errNullConfigValue(key) + return errNullConfigValue(keyAllowlistHosts) } switch val := raw.(type) { - case []interface{}: + case []any: for _, item := range val { str, ok := item.(string) if !ok { - return fmt.Errorf( - "config key %q: list entry %v (%T) is not a string", key, item, item) + return fmt.Errorf("config key %q: list entry %v (%T) is %w", + keyAllowlistHosts, item, item, errNotAString) } if strings.TrimSpace(str) == "" { - return fmt.Errorf("config key %q: list contains an empty entry", key) + return fmt.Errorf("config key %q: %w", + keyAllowlistHosts, errEmptyListEntry) } } case string: @@ -561,15 +614,15 @@ func validateAllowlistHostsValue(sc *smartconfig.Config) error { return nil } - for _, part := range strings.Split(val, ",") { + for part := range strings.SplitSeq(val, ",") { if strings.TrimSpace(part) == "" { - return fmt.Errorf( - "config key %q: value %q contains an empty entry", key, val) + return fmt.Errorf("config key %q: value %q %w", + keyAllowlistHosts, val, errEmptyEntry) } } default: - return fmt.Errorf("config key %q: value %v (%T) is not a list of strings", - key, raw, raw) + return fmt.Errorf("config key %q: value %v (%T) is %w", + keyAllowlistHosts, raw, raw, errNotAStringList) } return nil @@ -584,13 +637,13 @@ func getStringSlice(sc *smartconfig.Config) []string { return nil } - val, ok := sc.Get("allowlist_hosts") + val, ok := sc.Get(keyAllowlistHosts) if !ok || val == nil { return nil } // Handle YAML list format - if slice, ok := val.([]interface{}); ok { + if slice, ok := val.([]any); ok { result := make([]string, 0, len(slice)) for _, item := range slice { if str, ok := item.(string); ok { diff --git a/internal/config/config_internal_test.go b/internal/config/config_internal_test.go index bcf957a..de33d40 100644 --- a/internal/config/config_internal_test.go +++ b/internal/config/config_internal_test.go @@ -40,7 +40,7 @@ func checkAllowlistHosts(t *testing.T, configPath string) { t.Errorf("expected 3 hosts, got %d: %v", len(hosts), hosts) } - expected := []string{"static.sneak.cloud", "sneak.berlin", "s3.sneak.cloud"} + expected := []string{"static.sneak.cloud", "sneak.berlin", testHostS3} for i, want := range expected { if i >= len(hosts) { t.Errorf("missing host at index %d: want %q", i, want) diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_internal_test.go similarity index 72% rename from internal/config/config_validation_test.go rename to internal/config/config_validation_internal_test.go index 127c0d7..ca1573e 100644 --- a/internal/config/config_validation_test.go +++ b/internal/config/config_validation_internal_test.go @@ -1,7 +1,6 @@ package config import ( - "io" "log/slog" "os" "path/filepath" @@ -15,6 +14,26 @@ import ( // minimum length requirement in validate(). const validTestSigningKey = "0123456789abcdef0123456789abcdef" +// signingKeyLine is a valid signing_key config line used as the base of +// test config files. +const signingKeyLine = "signing_key: " + validTestSigningKey + "\n" + +// testHostS3 is an allowlist host entry used across the config tests. +const testHostS3 = "s3.sneak.cloud" + +// nullValueText is the substring that error messages about explicitly +// null config values must contain. +const nullValueText = "null" + +// abortCase describes a config file that must abort startup with an +// error mentioning every string in wantErrSubstrings. +type abortCase struct { + name string + yaml string + // wantErrSubstrings must all appear in the error message. + wantErrSubstrings []string +} + // configFromYAML writes yamlContent to a temporary config file, loads it // via smartconfig, and constructs a Config from it using the same code // path the server uses at startup. @@ -24,7 +43,8 @@ func configFromYAML(t *testing.T, yamlContent string) (*Config, error) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "config.yml") - if err := os.WriteFile(configPath, []byte(yamlContent), 0o600); err != nil { + err := os.WriteFile(configPath, []byte(yamlContent), 0o600) + if err != nil { t.Fatalf("failed to write test config: %v", err) } @@ -37,7 +57,9 @@ func configFromYAML(t *testing.T, yamlContent string) (*Config, error) { } func TestOmittedValuesUseDefaults(t *testing.T) { - c, err := configFromYAML(t, "signing_key: "+validTestSigningKey+"\n") + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) if err != nil { t.Fatalf("minimal config should be valid, got error: %v", err) } @@ -78,6 +100,8 @@ func TestOmittedValuesUseDefaults(t *testing.T) { } func TestExplicitValidValuesAreUsed(t *testing.T) { + t.Parallel() + yamlContent := ` port: 9090 debug: true @@ -118,9 +142,10 @@ metrics: t.Errorf("DBURL = %q, want explicit value", c.DBURL) } - if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != "s3.sneak.cloud" || + if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != testHostS3 || c.AllowlistHosts[1] != ".example.com" { - t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud .example.com]", c.AllowlistHosts) + t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud .example.com]", + c.AllowlistHosts) } if c.UpstreamConnectionsPerHost != 5 { @@ -138,8 +163,10 @@ metrics: } func TestCommaSeparatedAllowlistStillSupported(t *testing.T) { - yamlContent := `signing_key: ` + validTestSigningKey + ` -allowlist_hosts: "s3.sneak.cloud, sneak.berlin" + t.Parallel() + + yamlContent := signingKeyLine + + `allowlist_hosts: "s3.sneak.cloud, sneak.berlin" ` c, err := configFromYAML(t, yamlContent) @@ -147,9 +174,160 @@ allowlist_hosts: "s3.sneak.cloud, sneak.berlin" t.Fatalf("comma-separated allowlist should load, got error: %v", err) } - if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != "s3.sneak.cloud" || + if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != testHostS3 || c.AllowlistHosts[1] != "sneak.berlin" { - t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud sneak.berlin]", c.AllowlistHosts) + t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud sneak.berlin]", + c.AllowlistHosts) + } +} + +// runAbortCases asserts that each case's config aborts startup with an +// error message mentioning every expected substring. +func runAbortCases(t *testing.T, cases []abortCase) { + t.Helper() + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, tc.yaml) + if err == nil { + t.Fatalf("config with %s must abort startup, got config: %+v", tc.name, c) + } + + t.Logf("got expected error: %v", err) + + for _, want := range tc.wantErrSubstrings { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err.Error(), want) + } + } + }) + } +} + +// invalidScalarValueCases are configs where a scalar key is explicitly +// set to an unparseable or out-of-range value; each must abort startup +// naming the offending key, never silently fall back to the default. +func invalidScalarValueCases() []abortCase { + return []abortCase{ + { + name: "port not a number", + yaml: signingKeyLine + "port: banana\n", + wantErrSubstrings: []string{keyPort, "banana"}, + }, + { + name: "port zero", + yaml: signingKeyLine + "port: 0\n", + wantErrSubstrings: []string{keyPort, "0"}, + }, + { + name: "port above 65535", + yaml: signingKeyLine + "port: 99999\n", + wantErrSubstrings: []string{keyPort, "99999"}, + }, + { + name: "port fractional", + yaml: signingKeyLine + "port: 8080.5\n", + wantErrSubstrings: []string{keyPort, "8080.5"}, + }, + { + name: "debug not a bool", + yaml: signingKeyLine + "debug: notabool\n", + wantErrSubstrings: []string{keyDebug, "notabool"}, + }, + { + name: "maintenance_mode not a bool", + yaml: signingKeyLine + "maintenance_mode: sometimes\n", + wantErrSubstrings: []string{keyMaintenanceMode, "sometimes"}, + }, + { + name: "allow_http numeric", + yaml: signingKeyLine + "allow_http: 2\n", + wantErrSubstrings: []string{keyAllowHTTP, "2"}, + }, + { + name: "upstream_connections_per_host zero", + yaml: signingKeyLine + "upstream_connections_per_host: 0\n", + wantErrSubstrings: []string{keyUpstreamConnectionsPerHost, "0"}, + }, + { + name: "upstream_connections_per_host negative", + yaml: signingKeyLine + "upstream_connections_per_host: -3\n", + wantErrSubstrings: []string{keyUpstreamConnectionsPerHost, "-3"}, + }, + { + name: "upstream_connections_per_host not a number", + yaml: signingKeyLine + "upstream_connections_per_host: many\n", + wantErrSubstrings: []string{keyUpstreamConnectionsPerHost, "many"}, + }, + } +} + +// invalidHostAndCredentialCases are configs where allowlist_hosts, +// signing_key, state_dir, sentry_dsn, or metrics is explicitly set to +// an invalid value; each must abort startup naming the offending key. +func invalidHostAndCredentialCases() []abortCase { + return []abortCase{ + { + name: "allowlist host with scheme", + yaml: signingKeyLine + "allowlist_hosts:\n - https://example.com\n", + wantErrSubstrings: []string{ + keyAllowlistHosts, "https://example.com", + }, + }, + { + name: "allowlist host with path", + yaml: signingKeyLine + "allowlist_hosts:\n - example.com/images\n", + wantErrSubstrings: []string{ + keyAllowlistHosts, "example.com/images", + }, + }, + { + name: "allowlist host with whitespace", + yaml: signingKeyLine + "allowlist_hosts:\n - \"exa mple.com\"\n", + wantErrSubstrings: []string{keyAllowlistHosts, "exa mple.com"}, + }, + { + name: "allowlist entry not a string", + yaml: signingKeyLine + "allowlist_hosts:\n - 123\n", + wantErrSubstrings: []string{keyAllowlistHosts, "123"}, + }, + { + name: "allowlist not a list", + yaml: signingKeyLine + "allowlist_hosts:\n key: value\n", + wantErrSubstrings: []string{keyAllowlistHosts}, + }, + { + name: "signing_key too short", + yaml: "signing_key: short\n", + wantErrSubstrings: []string{keySigningKey}, + }, + { + name: "signing_key missing", + yaml: "port: 8080\n", + wantErrSubstrings: []string{keySigningKey}, + }, + { + name: "state_dir explicitly empty", + yaml: signingKeyLine + "state_dir: \"\"\n", + wantErrSubstrings: []string{keyStateDir}, + }, + { + name: "sentry_dsn not a URL", + yaml: signingKeyLine + "sentry_dsn: \"not a url\"\n", + wantErrSubstrings: []string{keySentryDSN, "not a url"}, + }, + { + name: "metrics username without password", + yaml: signingKeyLine + "metrics:\n username: bob\n", + wantErrSubstrings: []string{keyMetrics}, + }, + { + name: "metrics password without username", + yaml: signingKeyLine + "metrics:\n password: hunter2\n", + wantErrSubstrings: []string{keyMetrics}, + }, } } @@ -158,140 +336,84 @@ allowlist_hosts: "s3.sneak.cloud, sneak.berlin" // value must produce a startup error naming the offending key, never // silently fall back to the default. func TestSetButInvalidValueAbortsStartup(t *testing.T) { - signingKeyLine := "signing_key: " + validTestSigningKey + "\n" + t.Parallel() - cases := []struct { - name string - yaml string - // wantErrSubstrings must all appear in the error message. - wantErrSubstrings []string - }{ + runAbortCases(t, append( + invalidScalarValueCases(), invalidHostAndCredentialCases()...)) +} + +// explicitNullValueCases are configs where a key is explicitly set to +// null (including the bare "key:" form and the "~" alias); each must +// abort startup naming the key. +func explicitNullValueCases() []abortCase { + return []abortCase{ { - name: "port not a number", - yaml: signingKeyLine + "port: banana\n", - wantErrSubstrings: []string{"port", "banana"}, + name: "port explicit null", + yaml: signingKeyLine + "port: null\n", + wantErrSubstrings: []string{keyPort, nullValueText}, }, { - name: "port zero", - yaml: signingKeyLine + "port: 0\n", - wantErrSubstrings: []string{"port", "0"}, + name: "port bare key no value", + yaml: signingKeyLine + "port:\n", + wantErrSubstrings: []string{keyPort, nullValueText}, }, { - name: "port above 65535", - yaml: signingKeyLine + "port: 99999\n", - wantErrSubstrings: []string{"port", "99999"}, + name: "debug tilde null", + yaml: signingKeyLine + "debug: ~\n", + wantErrSubstrings: []string{keyDebug, nullValueText}, }, { - name: "port fractional", - yaml: signingKeyLine + "port: 8080.5\n", - wantErrSubstrings: []string{"port", "8080.5"}, + name: "maintenance_mode null", + yaml: signingKeyLine + "maintenance_mode: null\n", + wantErrSubstrings: []string{keyMaintenanceMode, nullValueText}, }, { - name: "debug not a bool", - yaml: signingKeyLine + "debug: notabool\n", - wantErrSubstrings: []string{"debug", "notabool"}, + name: "allow_http null", + yaml: signingKeyLine + "allow_http: null\n", + wantErrSubstrings: []string{keyAllowHTTP, nullValueText}, }, { - name: "maintenance_mode not a bool", - yaml: signingKeyLine + "maintenance_mode: sometimes\n", - wantErrSubstrings: []string{"maintenance_mode", "sometimes"}, + name: "state_dir null", + yaml: signingKeyLine + "state_dir: null\n", + wantErrSubstrings: []string{keyStateDir, nullValueText}, }, { - name: "allow_http numeric", - yaml: signingKeyLine + "allow_http: 2\n", - wantErrSubstrings: []string{"allow_http", "2"}, + name: "db_url null", + yaml: signingKeyLine + "db_url: null\n", + wantErrSubstrings: []string{keyDBURL, nullValueText}, }, { - name: "upstream_connections_per_host zero", - yaml: signingKeyLine + "upstream_connections_per_host: 0\n", - wantErrSubstrings: []string{"upstream_connections_per_host", "0"}, + name: "sentry_dsn null", + yaml: signingKeyLine + "sentry_dsn: null\n", + wantErrSubstrings: []string{keySentryDSN, nullValueText}, }, { - name: "upstream_connections_per_host negative", - yaml: signingKeyLine + "upstream_connections_per_host: -3\n", - wantErrSubstrings: []string{"upstream_connections_per_host", "-3"}, + name: "upstream_connections_per_host null", + yaml: signingKeyLine + "upstream_connections_per_host: null\n", + wantErrSubstrings: []string{keyUpstreamConnectionsPerHost, nullValueText}, }, { - name: "upstream_connections_per_host not a number", - yaml: signingKeyLine + "upstream_connections_per_host: many\n", - wantErrSubstrings: []string{"upstream_connections_per_host", "many"}, + name: "allowlist_hosts null", + yaml: signingKeyLine + "allowlist_hosts: null\n", + wantErrSubstrings: []string{keyAllowlistHosts, nullValueText}, }, { - name: "allowlist host with scheme", - yaml: signingKeyLine + "allowlist_hosts:\n - https://example.com\n", + name: "signing_key null", + yaml: "signing_key: null\n", + wantErrSubstrings: []string{keySigningKey, nullValueText}, + }, + { + name: "metrics null", + yaml: signingKeyLine + "metrics: null\n", + wantErrSubstrings: []string{keyMetrics, nullValueText}, + }, + { + name: "metrics subkeys null", + yaml: signingKeyLine + "metrics:\n username: null\n password: null\n", wantErrSubstrings: []string{ - "allowlist_hosts", "https://example.com", + keyMetricsUsername, keyMetricsPassword, nullValueText, }, }, - { - name: "allowlist host with path", - yaml: signingKeyLine + "allowlist_hosts:\n - example.com/images\n", - wantErrSubstrings: []string{ - "allowlist_hosts", "example.com/images", - }, - }, - { - name: "allowlist host with whitespace", - yaml: signingKeyLine + "allowlist_hosts:\n - \"exa mple.com\"\n", - wantErrSubstrings: []string{"allowlist_hosts", "exa mple.com"}, - }, - { - name: "allowlist entry not a string", - yaml: signingKeyLine + "allowlist_hosts:\n - 123\n", - wantErrSubstrings: []string{"allowlist_hosts", "123"}, - }, - { - name: "allowlist not a list", - yaml: signingKeyLine + "allowlist_hosts:\n key: value\n", - wantErrSubstrings: []string{"allowlist_hosts"}, - }, - { - name: "signing_key too short", - yaml: "signing_key: short\n", - wantErrSubstrings: []string{"signing_key"}, - }, - { - name: "signing_key missing", - yaml: "port: 8080\n", - wantErrSubstrings: []string{"signing_key"}, - }, - { - name: "state_dir explicitly empty", - yaml: signingKeyLine + "state_dir: \"\"\n", - wantErrSubstrings: []string{"state_dir"}, - }, - { - name: "sentry_dsn not a URL", - yaml: signingKeyLine + "sentry_dsn: \"not a url\"\n", - wantErrSubstrings: []string{"sentry_dsn", "not a url"}, - }, - { - name: "metrics username without password", - yaml: signingKeyLine + "metrics:\n username: bob\n", - wantErrSubstrings: []string{"metrics"}, - }, - { - name: "metrics password without username", - yaml: signingKeyLine + "metrics:\n password: hunter2\n", - wantErrSubstrings: []string{"metrics"}, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - c, err := configFromYAML(t, tc.yaml) - if err == nil { - t.Fatalf("config with %s must abort startup, got config: %+v", tc.name, c) - } - - t.Logf("got expected error: %v", err) - - for _, want := range tc.wantErrSubstrings { - if !strings.Contains(err.Error(), want) { - t.Errorf("error %q does not mention %q", err.Error(), want) - } - } - }) } } @@ -300,99 +422,9 @@ func TestSetButInvalidValueAbortsStartup(t *testing.T) { // startup naming the key. An explicit null is a SET value: it must // never silently fall back to the default the way an omitted key does. func TestExplicitNullValueAbortsStartup(t *testing.T) { - signingKeyLine := "signing_key: " + validTestSigningKey + "\n" + t.Parallel() - cases := []struct { - name string - yaml string - // wantErrSubstrings must all appear in the error message. - wantErrSubstrings []string - }{ - { - name: "port explicit null", - yaml: signingKeyLine + "port: null\n", - wantErrSubstrings: []string{"port", "null"}, - }, - { - name: "port bare key no value", - yaml: signingKeyLine + "port:\n", - wantErrSubstrings: []string{"port", "null"}, - }, - { - name: "debug tilde null", - yaml: signingKeyLine + "debug: ~\n", - wantErrSubstrings: []string{"debug", "null"}, - }, - { - name: "maintenance_mode null", - yaml: signingKeyLine + "maintenance_mode: null\n", - wantErrSubstrings: []string{"maintenance_mode", "null"}, - }, - { - name: "allow_http null", - yaml: signingKeyLine + "allow_http: null\n", - wantErrSubstrings: []string{"allow_http", "null"}, - }, - { - name: "state_dir null", - yaml: signingKeyLine + "state_dir: null\n", - wantErrSubstrings: []string{"state_dir", "null"}, - }, - { - name: "db_url null", - yaml: signingKeyLine + "db_url: null\n", - wantErrSubstrings: []string{"db_url", "null"}, - }, - { - name: "sentry_dsn null", - yaml: signingKeyLine + "sentry_dsn: null\n", - wantErrSubstrings: []string{"sentry_dsn", "null"}, - }, - { - name: "upstream_connections_per_host null", - yaml: signingKeyLine + "upstream_connections_per_host: null\n", - wantErrSubstrings: []string{"upstream_connections_per_host", "null"}, - }, - { - name: "allowlist_hosts null", - yaml: signingKeyLine + "allowlist_hosts: null\n", - wantErrSubstrings: []string{"allowlist_hosts", "null"}, - }, - { - name: "signing_key null", - yaml: "signing_key: null\n", - wantErrSubstrings: []string{"signing_key", "null"}, - }, - { - name: "metrics null", - yaml: signingKeyLine + "metrics: null\n", - wantErrSubstrings: []string{"metrics", "null"}, - }, - { - name: "metrics subkeys null", - yaml: signingKeyLine + "metrics:\n username: null\n password: null\n", - wantErrSubstrings: []string{ - "metrics.username", "metrics.password", "null", - }, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - c, err := configFromYAML(t, tc.yaml) - if err == nil { - t.Fatalf("config with %s must abort startup, got config: %+v", tc.name, c) - } - - t.Logf("got expected error: %v", err) - - for _, want := range tc.wantErrSubstrings { - if !strings.Contains(err.Error(), want) { - t.Errorf("error %q does not mention %q", err.Error(), want) - } - } - }) - } + runAbortCases(t, explicitNullValueCases()) } // TestExplicitlyEmptyDBURLAbortsStartup verifies that db_url set to an @@ -400,7 +432,9 @@ func TestExplicitNullValueAbortsStartup(t *testing.T) { // a default, and defaults apply only to omitted keys. This matches // state_dir, where an explicitly empty value already aborts. func TestExplicitlyEmptyDBURLAbortsStartup(t *testing.T) { - yamlContent := "signing_key: " + validTestSigningKey + "\ndb_url: \"\"\n" + t.Parallel() + + yamlContent := signingKeyLine + "db_url: \"\"\n" c, err := configFromYAML(t, yamlContent) if err == nil { @@ -409,7 +443,7 @@ func TestExplicitlyEmptyDBURLAbortsStartup(t *testing.T) { t.Logf("got expected error: %v", err) - if !strings.Contains(err.Error(), "db_url") { + if !strings.Contains(err.Error(), keyDBURL) { t.Errorf("error %q does not name the offending key db_url", err.Error()) } } @@ -420,10 +454,12 @@ func TestExplicitlyEmptyDBURLAbortsStartup(t *testing.T) { // host written in FQDN trailing-dot form (e.g. evil.com.) and // effectively disable URL signing with a single character. func TestAllowlistHostsRejectsDotOnlyEntries(t *testing.T) { - signingKeyLine := "signing_key: " + validTestSigningKey + "\n" + t.Parallel() for _, entry := range []string{".", ".."} { t.Run(entry, func(t *testing.T) { + t.Parallel() + yamlContent := signingKeyLine + "allowlist_hosts:\n - \"" + entry + "\"\n" @@ -435,7 +471,7 @@ func TestAllowlistHostsRejectsDotOnlyEntries(t *testing.T) { t.Logf("got expected error: %v", err) - if !strings.Contains(err.Error(), "allowlist_hosts") { + if !strings.Contains(err.Error(), keyAllowlistHosts) { t.Errorf("error %q does not name the offending key allowlist_hosts", err.Error()) } @@ -444,8 +480,9 @@ func TestAllowlistHostsRejectsDotOnlyEntries(t *testing.T) { } func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) { - yamlContent := `signing_key: ` + validTestSigningKey + ` -whitelist_hosts: + t.Parallel() + + yamlContent := signingKeyLine + `whitelist_hosts: - example.com ` @@ -462,8 +499,9 @@ whitelist_hosts: } func TestUnknownMetricsSubkeyAbortsStartup(t *testing.T) { - yamlContent := `signing_key: ` + validTestSigningKey + ` -metrics: + t.Parallel() + + yamlContent := signingKeyLine + `metrics: username: bob password: hunter2 port: 9100 @@ -482,13 +520,17 @@ metrics: } func TestEnvSectionIsPermitted(t *testing.T) { - yamlContent := `signing_key: ` + validTestSigningKey + ` -env: + t.Parallel() + + yamlContent := signingKeyLine + `env: PIXA_TEST_ENV_INJECTION: injected ` - if _, err := configFromYAML(t, yamlContent); err != nil { - t.Fatalf("env section must be permitted (smartconfig consumes it), got error: %v", err) + _, err := configFromYAML(t, yamlContent) + if err != nil { + t.Fatalf( + "env section must be permitted (smartconfig consumes it), got error: %v", + err) } } @@ -496,7 +538,8 @@ func TestMalformedConfigFileAbortsStartup(t *testing.T) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "config.yml") - if err := os.WriteFile(configPath, []byte("port: [unclosed\n"), 0o600); err != nil { + err := os.WriteFile(configPath, []byte("port: [unclosed\n"), 0o600) + if err != nil { t.Fatalf("failed to write malformed config: %v", err) } @@ -505,7 +548,7 @@ func TestMalformedConfigFileAbortsStartup(t *testing.T) { t.Setenv("PIXA_CONFIG_PATH", "") t.Chdir(tmpDir) - log := slog.New(slog.NewTextHandler(io.Discard, nil)) + log := slog.New(slog.DiscardHandler) sc, err := loadConfigFile(log, "pixa-test-nonexistent-app") if err == nil { @@ -516,10 +559,14 @@ func TestMalformedConfigFileAbortsStartup(t *testing.T) { } func TestEnsureStateDirCreatesDirectory(t *testing.T) { + t.Parallel() + stateDir := filepath.Join(t.TempDir(), "nested", "state") c := &Config{StateDir: stateDir} - if err := c.ensureStateDirWritable(); err != nil { + + err := c.ensureStateDirWritable() + if err != nil { t.Fatalf("creatable state_dir must validate, got error: %v", err) } @@ -530,6 +577,8 @@ func TestEnsureStateDirCreatesDirectory(t *testing.T) { } func TestEnsureStateDirFailsOnUncreatablePath(t *testing.T) { + t.Parallel() + // A path below /dev/null can never be created, even when running // as root (as in the Docker build). c := &Config{StateDir: "/dev/null/pixa-state"} @@ -541,7 +590,7 @@ func TestEnsureStateDirFailsOnUncreatablePath(t *testing.T) { t.Logf("got expected error: %v", err) - if !strings.Contains(err.Error(), "state_dir") { + if !strings.Contains(err.Error(), keyStateDir) { t.Errorf("error %q does not name the offending key state_dir", err.Error()) } }