From f19da2c02cb697222825b5b9565d2d54667351e6 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 16:31:03 +0000 Subject: [PATCH 01/10] test: add failing startup config validation tests (#52) Encode the required fail-fast behavior as tests ahead of the implementation: a config value that is SET but unparseable or invalid must abort startup (defaults apply only to OMITTED keys), unknown top-level keys and unknown metrics subkeys must abort naming the key, a malformed config file at a standard location must abort instead of being skipped with a warning, and state_dir must be creatable and writable at startup. Mechanically extracts newFromSmartConfig from config.New so the construction path is testable without fx; current lenient behavior is unchanged, so the new enforcement tests fail. --- internal/config/config.go | 31 +- internal/config/config_validation_test.go | 399 ++++++++++++++++++++++ 2 files changed, 426 insertions(+), 4 deletions(-) create mode 100644 internal/config/config_validation_test.go diff --git a/internal/config/config.go b/internal/config/config.go index a2c3778..a36e8ad 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -60,6 +60,26 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { log.Info("no config file found, using defaults") } + c, err := newFromSmartConfig(sc) + if err != nil { + return nil, err + } + + if err := c.ensureStateDirWritable(); err != nil { + return nil, err + } + + if c.Debug { + params.Logger.EnableDebugLogging() + } + + return c, nil +} + +// newFromSmartConfig constructs a Config from a loaded smartconfig +// instance and validates it. A nil sc means no config file was found, +// in which case every option takes its default value. +func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { c := &Config{ Debug: getBool(sc, "debug", false), MaintenanceMode: getBool(sc, "maintenance_mode", false), @@ -80,10 +100,6 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) } - if c.Debug { - params.Logger.EnableDebugLogging() - } - // Validate required configuration if err := c.validate(); err != nil { return nil, err @@ -92,6 +108,13 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { return c, nil } +// ensureStateDirWritable verifies at startup that StateDir can be +// created and written to, so a misconfigured path aborts startup +// instead of failing later at first use. +func (c *Config) ensureStateDirWritable() error { + return nil +} + // validate checks that all required configuration values are set. func (c *Config) validate() error { if c.SigningKey == "" { diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_test.go new file mode 100644 index 0000000..7471c92 --- /dev/null +++ b/internal/config/config_validation_test.go @@ -0,0 +1,399 @@ +package config + +import ( + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + + "git.eeqj.de/sneak/smartconfig" +) + +// validTestSigningKey is a 32-character signing key that satisfies the +// minimum length requirement in validate(). +const validTestSigningKey = "0123456789abcdef0123456789abcdef" + +// 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. +func configFromYAML(t *testing.T, yamlContent string) (*Config, error) { + t.Helper() + + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yml") + + if err := os.WriteFile(configPath, []byte(yamlContent), 0o600); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + sc, err := smartconfig.NewFromConfigPath(configPath) + if err != nil { + t.Fatalf("failed to load test config: %v", err) + } + + return newFromSmartConfig(sc) +} + +func TestOmittedValuesUseDefaults(t *testing.T) { + c, err := configFromYAML(t, "signing_key: "+validTestSigningKey+"\n") + if err != nil { + t.Fatalf("minimal config should be valid, got error: %v", err) + } + + if c.Port != DefaultPort { + t.Errorf("Port = %d, want default %d", c.Port, DefaultPort) + } + + if c.StateDir != DefaultStateDir { + t.Errorf("StateDir = %q, want default %q", c.StateDir, DefaultStateDir) + } + + if c.UpstreamConnectionsPerHost != DefaultUpstreamConnectionsPerHost { + t.Errorf("UpstreamConnectionsPerHost = %d, want default %d", + c.UpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost) + } + + if c.Debug { + t.Error("Debug = true, want default false") + } + + if c.MaintenanceMode { + t.Error("MaintenanceMode = true, want default false") + } + + if c.AllowHTTP { + t.Error("AllowHTTP = true, want default false") + } + + if len(c.AllowlistHosts) != 0 { + t.Errorf("AllowlistHosts = %v, want empty", c.AllowlistHosts) + } + + wantDBURL := "file:" + DefaultStateDir + "/state.sqlite3?_journal_mode=WAL" + if c.DBURL != wantDBURL { + t.Errorf("DBURL = %q, want derived default %q", c.DBURL, wantDBURL) + } +} + +func TestExplicitValidValuesAreUsed(t *testing.T) { + yamlContent := ` +port: 9090 +debug: true +maintenance_mode: true +state_dir: /tmp/pixa-test-state +db_url: "file:/tmp/pixa-test-state/other.sqlite3" +signing_key: ` + validTestSigningKey + ` +allowlist_hosts: + - s3.sneak.cloud + - .example.com +allow_http: true +upstream_connections_per_host: 5 +sentry_dsn: "https://abc123@sentry.example.com/42" +metrics: + username: metricsuser + password: metricspass +` + + c, err := configFromYAML(t, yamlContent) + if err != nil { + t.Fatalf("valid config should load, got error: %v", err) + } + + if c.Port != 9090 { + t.Errorf("Port = %d, want 9090", c.Port) + } + + if !c.Debug || !c.MaintenanceMode || !c.AllowHTTP { + t.Errorf("bool fields = debug %v maintenance %v allow_http %v, want all true", + c.Debug, c.MaintenanceMode, c.AllowHTTP) + } + + if c.StateDir != "/tmp/pixa-test-state" { + t.Errorf("StateDir = %q, want /tmp/pixa-test-state", c.StateDir) + } + + if c.DBURL != "file:/tmp/pixa-test-state/other.sqlite3" { + t.Errorf("DBURL = %q, want explicit value", c.DBURL) + } + + if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != "s3.sneak.cloud" || + c.AllowlistHosts[1] != ".example.com" { + t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud .example.com]", c.AllowlistHosts) + } + + if c.UpstreamConnectionsPerHost != 5 { + t.Errorf("UpstreamConnectionsPerHost = %d, want 5", c.UpstreamConnectionsPerHost) + } + + if c.SentryDSN != "https://abc123@sentry.example.com/42" { + t.Errorf("SentryDSN = %q, want explicit value", c.SentryDSN) + } + + if c.MetricsUsername != "metricsuser" || c.MetricsPassword != "metricspass" { + t.Errorf("metrics = %q/%q, want metricsuser/metricspass", + c.MetricsUsername, c.MetricsPassword) + } +} + +func TestCommaSeparatedAllowlistStillSupported(t *testing.T) { + yamlContent := `signing_key: ` + validTestSigningKey + ` +allowlist_hosts: "s3.sneak.cloud, sneak.berlin" +` + + c, err := configFromYAML(t, yamlContent) + if err != nil { + t.Fatalf("comma-separated allowlist should load, got error: %v", err) + } + + if len(c.AllowlistHosts) != 2 || c.AllowlistHosts[0] != "s3.sneak.cloud" || + c.AllowlistHosts[1] != "sneak.berlin" { + t.Errorf("AllowlistHosts = %v, want [s3.sneak.cloud sneak.berlin]", c.AllowlistHosts) + } +} + +// TestSetButInvalidValueAbortsStartup verifies the no-silent-fallback +// rule: a key that is explicitly set to an unparseable or out-of-range +// 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" + + cases := []struct { + name string + yaml string + // wantErrSubstrings must all appear in the error message. + wantErrSubstrings []string + }{ + { + name: "port not a number", + yaml: signingKeyLine + "port: banana\n", + wantErrSubstrings: []string{"port", "banana"}, + }, + { + name: "port zero", + yaml: signingKeyLine + "port: 0\n", + wantErrSubstrings: []string{"port", "0"}, + }, + { + name: "port above 65535", + yaml: signingKeyLine + "port: 99999\n", + wantErrSubstrings: []string{"port", "99999"}, + }, + { + name: "port fractional", + yaml: signingKeyLine + "port: 8080.5\n", + wantErrSubstrings: []string{"port", "8080.5"}, + }, + { + name: "debug not a bool", + yaml: signingKeyLine + "debug: notabool\n", + wantErrSubstrings: []string{"debug", "notabool"}, + }, + { + name: "maintenance_mode not a bool", + yaml: signingKeyLine + "maintenance_mode: sometimes\n", + wantErrSubstrings: []string{"maintenance_mode", "sometimes"}, + }, + { + name: "allow_http numeric", + yaml: signingKeyLine + "allow_http: 2\n", + wantErrSubstrings: []string{"allow_http", "2"}, + }, + { + name: "upstream_connections_per_host zero", + yaml: signingKeyLine + "upstream_connections_per_host: 0\n", + wantErrSubstrings: []string{"upstream_connections_per_host", "0"}, + }, + { + 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 not a number", + yaml: signingKeyLine + "upstream_connections_per_host: many\n", + wantErrSubstrings: []string{"upstream_connections_per_host", "many"}, + }, + { + name: "allowlist host with scheme", + yaml: signingKeyLine + "allowlist_hosts:\n - https://example.com\n", + wantErrSubstrings: []string{ + "allowlist_hosts", "https://example.com", + }, + }, + { + 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) + } + } + }) + } +} + +func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) { + yamlContent := `signing_key: ` + validTestSigningKey + ` +whitelist_hosts: + - example.com +` + + c, err := configFromYAML(t, yamlContent) + if err == nil { + t.Fatalf("config with unknown key must abort startup, got config: %+v", c) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "whitelist_hosts") { + t.Errorf("error %q does not name the unknown key whitelist_hosts", err.Error()) + } +} + +func TestUnknownMetricsSubkeyAbortsStartup(t *testing.T) { + yamlContent := `signing_key: ` + validTestSigningKey + ` +metrics: + username: bob + password: hunter2 + port: 9100 +` + + c, err := configFromYAML(t, yamlContent) + if err == nil { + t.Fatalf("config with unknown metrics subkey must abort startup, got config: %+v", c) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "metrics.port") { + t.Errorf("error %q does not name the unknown key metrics.port", err.Error()) + } +} + +func TestEnvSectionIsPermitted(t *testing.T) { + yamlContent := `signing_key: ` + validTestSigningKey + ` +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) + } +} + +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 { + t.Fatalf("failed to write malformed config: %v", err) + } + + // loadConfigFile falls through to the relative config.yml candidate; + // the appname is chosen so no /etc or $HOME candidate can exist. + t.Setenv("PIXA_CONFIG_PATH", "") + t.Chdir(tmpDir) + + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + + sc, err := loadConfigFile(log, "pixa-test-nonexistent-app") + if err == nil { + t.Fatalf("malformed config file must abort startup, got config: %v", sc) + } + + t.Logf("got expected error: %v", err) +} + +func TestEnsureStateDirCreatesDirectory(t *testing.T) { + stateDir := filepath.Join(t.TempDir(), "nested", "state") + + c := &Config{StateDir: stateDir} + if err := c.ensureStateDirWritable(); err != nil { + t.Fatalf("creatable state_dir must validate, got error: %v", err) + } + + info, err := os.Stat(stateDir) + if err != nil || !info.IsDir() { + t.Fatalf("state_dir was not created: info=%v err=%v", info, err) + } +} + +func TestEnsureStateDirFailsOnUncreatablePath(t *testing.T) { + // 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"} + + err := c.ensureStateDirWritable() + if err == nil { + t.Fatal("uncreatable state_dir must abort startup, got nil error") + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "state_dir") { + t.Errorf("error %q does not name the offending key state_dir", err.Error()) + } +} -- 2.49.1 From 2fb0801dc615ff37d56f66b512b77718141960ac Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 16:37:30 +0000 Subject: [PATCH 02/10] feat: validate configuration on startup, fail fast on bad config (closes #52) A config value that is set but unparseable or invalid now aborts startup with an error naming the offending key and value; defaults apply only to omitted keys. Unknown top-level config keys and unknown metrics subkeys abort startup naming each unknown key, so typos like whitelist_hosts fail immediately instead of being silently ignored. A config file that exists at a standard location but fails to parse is now a fatal error instead of being skipped with a warning. state_dir is verified creatable and writable with a probe file before the listener binds. Port must be in 1-65535 (fractional values are rejected, not truncated), upstream_connections_per_host must be at least 1, allowlist_hosts entries must be bare hostnames, sentry_dsn must be a valid URL when set, and metrics credentials must be set together. The stale signing_key comment in config.example.yml (keyless mode was never implemented) now states the actual requirement. TODO.md records the completed step per its Workflow section. --- TODO.md | 7 +- config.example.yml | 2 +- internal/config/config.go | 360 ++++++++++++++++++++-- internal/config/config_validation_test.go | 4 +- 4 files changed, 335 insertions(+), 38 deletions(-) diff --git a/TODO.md b/TODO.md index 29359f7..5f6fa30 100644 --- a/TODO.md +++ b/TODO.md @@ -27,6 +27,12 @@ returns 410; logout redirects back to login # Completed Steps +- 2026-08-07 validate configuration on startup, fail fast on bad + config (closes #52): a config value that is set but unparseable or + invalid aborts startup naming the key and value (defaults apply only + to omitted keys), unknown config keys abort startup, a malformed + config file aborts instead of being skipped, and `state_dir` is + verified creatable and writable before the listener binds - 2026-08-07 fix the two remaining gosec findings (G124 in internal/session): session cookies now always carry Secure/HttpOnly/SameSite=Strict on both the set and clear paths; @@ -55,7 +61,6 @@ returns 410; logout redirects back to login - P0: implement cache size management and eviction so the disk cannot fill up -- P0: validate configuration on startup, fail fast on bad config - P1: implement blocked networks configuration to extend SSRF protection - P1: rate limit global concurrent upstream fetches to prevent diff --git a/config.example.yml b/config.example.yml index d4295f6..900a0b6 100644 --- a/config.example.yml +++ b/config.example.yml @@ -9,7 +9,7 @@ maintenance_mode: false state_dir: ./data # Image proxy settings -# HMAC signing key for URL signatures (leave empty to require allowlist for all requests) +# HMAC signing key for URL signatures (required, at least 32 characters) # Generate with: openssl rand -base64 32 signing_key: "CHANGE_ME_generate_with_openssl_rand_base64_32" diff --git a/internal/config/config.go b/internal/config/config.go index a36e8ad..00b1061 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,8 +4,12 @@ package config import ( "fmt" "log/slog" + "math" + "net/url" "os" "path/filepath" + "sort" + "strconv" "strings" "git.eeqj.de/sneak/smartconfig" @@ -78,29 +82,47 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { // newFromSmartConfig constructs a Config from a loaded smartconfig // instance and validates it. A nil sc means no config file was found, -// in which case every option takes its default value. +// in which case every option takes its default value. A key that is +// present but unparseable or invalid is an error: defaults apply only +// to omitted keys, never to invalid explicit values. func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { + if sc != nil { + if err := validateKnownKeys(sc); err != nil { + return nil, err + } + + if err := validateAllowlistHostsValue(sc); err != nil { + return nil, err + } + } + + loader := &strictLoader{sc: sc} + c := &Config{ - Debug: getBool(sc, "debug", false), - MaintenanceMode: getBool(sc, "maintenance_mode", false), - Port: getInt(sc, "port", DefaultPort), - StateDir: getString(sc, "state_dir", DefaultStateDir), - SentryDSN: getString(sc, "sentry_dsn", ""), - MetricsUsername: getString(sc, "metrics.username", ""), - MetricsPassword: getString(sc, "metrics.password", ""), - SigningKey: getString(sc, "signing_key", ""), - AllowlistHosts: getStringSlice(sc, "allowlist_hosts"), - AllowHTTP: getBool(sc, "allow_http", false), - UpstreamConnectionsPerHost: getInt(sc, "upstream_connections_per_host", DefaultUpstreamConnectionsPerHost), + 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", ""), + AllowlistHosts: getStringSlice(sc, "allowlist_hosts"), + AllowHTTP: loader.boolVal("allow_http", false), + UpstreamConnectionsPerHost: loader.intVal( + "upstream_connections_per_host", DefaultUpstreamConnectionsPerHost), } // Build DBURL from StateDir if not explicitly set - c.DBURL = getString(sc, "db_url", "") + c.DBURL = loader.stringVal("db_url", "") if c.DBURL == "" { c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) } - // Validate required configuration + if loader.err != nil { + return nil, loader.err + } + if err := c.validate(); err != nil { return nil, err } @@ -108,14 +130,92 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { return c, nil } +// validateKnownKeys rejects configuration files containing keys the +// application does not understand, so typos fail at startup instead of +// being silently ignored. The env section is permitted because +// smartconfig consumes it for environment variable injection. +func validateKnownKeys(sc *smartconfig.Config) error { + var unknown []string + + for key, value := range sc.Data() { + if !isKnownConfigKey(key) { + unknown = append(unknown, key) + + continue + } + + if key == "metrics" { + metricsMap, ok := value.(map[string]interface{}) + if !ok { + return fmt.Errorf( + "config key %q: value %v is not a map of metrics settings", + "metrics", value) + } + + for subkey := range metricsMap { + if subkey != "username" && subkey != "password" { + unknown = append(unknown, "metrics."+subkey) + } + } + } + } + + if len(unknown) > 0 { + sort.Strings(unknown) + + return fmt.Errorf("unknown config keys: %s", strings.Join(unknown, ", ")) + } + + return nil +} + +// 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": + return true + } + + return false +} + // ensureStateDirWritable verifies at startup that StateDir can be // created and written to, so a misconfigured path aborts startup // instead of failing later at first use. func (c *Config) ensureStateDirWritable() error { + const stateDirPerms = 0o750 + + if err := os.MkdirAll(c.StateDir, stateDirPerms); err != nil { + return fmt.Errorf("config key %q: cannot create directory %q: %w", + "state_dir", 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) + } + + probePath := probe.Name() + + if err := probe.Close(); err != nil { + return fmt.Errorf("config key %q: cannot close probe file %q: %w", + "state_dir", probePath, err) + } + + if err := os.Remove(probePath); err != nil { + return fmt.Errorf("config key %q: cannot remove probe file %q: %w", + "state_dir", probePath, err) + } + return nil } -// validate checks that all required configuration values are set. +// validate checks that all required configuration values are set and +// that every value is within its valid range. func (c *Config) validate() error { if c.SigningKey == "" { return fmt.Errorf("signing_key is required") @@ -124,7 +224,55 @@ func (c *Config) validate() error { // Minimum key length for security (32 bytes = 256 bits) const minKeyLength = 32 if len(c.SigningKey) < minKeyLength { - return fmt.Errorf("signing_key must be at least %d characters", minKeyLength) + return fmt.Errorf("signing_key must be at least %d characters, got %d", + 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) + } + + if c.UpstreamConnectionsPerHost < 1 { + return fmt.Errorf("config key %q: value %d must be at least 1", + "upstream_connections_per_host", c.UpstreamConnectionsPerHost) + } + + if c.StateDir == "" { + return fmt.Errorf("config key %q: value must not be empty", "state_dir") + } + + for _, host := range c.AllowlistHosts { + if err := validateAllowlistHost(host); err != nil { + return err + } + } + + 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) + } + } + + if (c.MetricsUsername == "") != (c.MetricsPassword == "") { + return fmt.Errorf("config keys %q and %q must be set together", + "metrics.username", "metrics.password") + } + + return nil +} + +// validateAllowlistHost checks that an allowlist_hosts entry is a bare +// hostname, optionally with a leading dot for suffix matching. URLs, +// paths, and whitespace indicate a misconfigured entry. +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 nil @@ -158,11 +306,11 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro cleanPath := filepath.Clean(path) //nolint:gosec // G703: paths are hardcoded config locations if _, statErr := os.Stat(cleanPath); 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) if err != nil { - log.Warn("failed to parse config file", "path", path, "error", err) - - continue + return nil, fmt.Errorf("failed to parse config file %s: %w", path, err) } log.Info("loaded config file", "path", path) @@ -174,45 +322,189 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro return nil, nil //nolint:nilnil // nil config is valid (use defaults) } -func getString(sc *smartconfig.Config, key, defaultVal string) string { - if sc == nil { - return defaultVal +// strictLoader accumulates the first error encountered while reading +// typed values out of a smartconfig instance, so Config construction +// can stay a single struct literal. +type strictLoader struct { + sc *smartconfig.Config + err error +} + +func (l *strictLoader) stringVal(key, defaultVal string) string { + if l.err != nil { + return "" } - val, err := sc.GetString(key) + val, err := getString(l.sc, key, defaultVal) if err != nil { - return defaultVal + l.err = err } return val } -func getInt(sc *smartconfig.Config, key string, defaultVal int) int { - if sc == nil { - return defaultVal +func (l *strictLoader) intVal(key string, defaultVal int) int { + if l.err != nil { + return 0 } - val, err := sc.GetInt(key) + val, err := getInt(l.sc, key, defaultVal) if err != nil { - return defaultVal + l.err = err } return val } -func getBool(sc *smartconfig.Config, key string, defaultVal bool) bool { - if sc == nil { - return defaultVal +func (l *strictLoader) boolVal(key string, defaultVal bool) bool { + if l.err != nil { + return false } - val, err := sc.GetBool(key) + val, err := getBool(l.sc, key, defaultVal) if err != nil { - return defaultVal + l.err = err } return val } +// getString returns the string value for key, or defaultVal if the key +// is omitted. A present value that is not a string is an error. +func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { + if sc == nil { + return defaultVal, nil + } + + raw, ok := sc.Get(key) + if !ok || raw == nil { + return defaultVal, nil + } + + str, ok := raw.(string) + if !ok { + return "", fmt.Errorf("config key %q: value %v (%T) is not a string", + key, raw, raw) + } + + return str, nil +} + +// getInt returns the integer value for key, or defaultVal if the key is +// omitted. A present value that is not a whole number is an error; +// fractional values are never truncated. +func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { + if sc == nil { + return defaultVal, nil + } + + raw, ok := sc.Get(key) + if !ok || raw == nil { + return defaultVal, nil + } + + switch val := raw.(type) { + case int: + return val, nil + case int64: + 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 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 parsed, nil + default: + return 0, fmt.Errorf("config key %q: value %v (%T) is not an integer", + key, raw, raw) + } +} + +// getBool returns the boolean value for key, or defaultVal if the key +// is omitted. A present value that is not a boolean (or a ParseBool-able +// string) is an error; numbers are not accepted as booleans. +func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) { + if sc == nil { + return defaultVal, nil + } + + raw, ok := sc.Get(key) + if !ok || raw == nil { + return defaultVal, nil + } + + switch val := raw.(type) { + case bool: + return val, nil + 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 parsed, nil + default: + return false, fmt.Errorf("config key %q: value %v (%T) is not a boolean", + key, raw, raw) + } +} + +// validateAllowlistHostsValue checks the raw shape of the +// allowlist_hosts value before the lenient extraction in getStringSlice +// runs: a value that is not a list of strings (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) + if !ok || raw == nil { + return nil + } + + switch val := raw.(type) { + case []interface{}: + 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) + } + + if strings.TrimSpace(str) == "" { + return fmt.Errorf("config key %q: list contains an empty entry", key) + } + } + case string: + if strings.TrimSpace(val) == "" { + return nil + } + + for _, part := range strings.Split(val, ",") { + if strings.TrimSpace(part) == "" { + return fmt.Errorf( + "config key %q: value %q contains an empty entry", key, val) + } + } + default: + return fmt.Errorf("config key %q: value %v (%T) is not a list of strings", + key, raw, raw) + } + + return nil +} + +// getStringSlice returns the list of strings for key, or nil if the key +// is omitted. It accepts a YAML list of strings or a comma-separated +// string (backwards compatibility). Malformed entries are rejected +// beforehand by validateAllowlistHostsValue. func getStringSlice(sc *smartconfig.Config, key string) []string { if sc == nil { return nil diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_test.go index 7471c92..8b1ef87 100644 --- a/internal/config/config_validation_test.go +++ b/internal/config/config_validation_test.go @@ -231,8 +231,8 @@ func TestSetButInvalidValueAbortsStartup(t *testing.T) { }, }, { - name: "allowlist host with whitespace", - yaml: signingKeyLine + "allowlist_hosts:\n - \"exa mple.com\"\n", + name: "allowlist host with whitespace", + yaml: signingKeyLine + "allowlist_hosts:\n - \"exa mple.com\"\n", wantErrSubstrings: []string{"allowlist_hosts", "exa mple.com"}, }, { -- 2.49.1 From 11e9206c21eccd0d8e6acbe874a87ffb4d40ae88 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:00:20 +0000 Subject: [PATCH 03/10] fix: suppress gosec G703 false positive on state_dir probe removal The pinned CI linter (golangci-lint v2.10.1) flags os.Remove(probePath) in ensureStateDirWritable as G703 path traversal via taint analysis. probePath comes from os.CreateTemp inside the StateDir that the probe just validated, so the taint finding is a false positive; suppress it with a justified nolint comment matching the existing precedent in loadConfigFile. Verified against the pinned linter version via the Dockerfile lint stage (0 issues). --- internal/config/config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/config.go b/internal/config/config.go index 00b1061..6a3af39 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -206,6 +206,7 @@ func (c *Config) ensureStateDirWritable() error { "state_dir", probePath, err) } + //nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir if err := os.Remove(probePath); err != nil { return fmt.Errorf("config key %q: cannot remove probe file %q: %w", "state_dir", probePath, err) -- 2.49.1 From 370545997f25e30721973463194778cb52bf4665 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:01:16 +0000 Subject: [PATCH 04/10] test: explicit null config values must abort startup (PR #53 rework) An explicitly-null key (port: null, bare port:, debug: ~, and every other config key including metrics subkeys) is a SET value under the no-silent-fallback rule and must abort startup naming the key, instead of silently taking the default as it does today. All 13 subtests fail against the current behavior; the fix follows. --- internal/config/config_validation_test.go | 100 ++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_test.go index 8b1ef87..f1a58f6 100644 --- a/internal/config/config_validation_test.go +++ b/internal/config/config_validation_test.go @@ -295,6 +295,106 @@ func TestSetButInvalidValueAbortsStartup(t *testing.T) { } } +// TestExplicitNullValueAbortsStartup verifies that a key explicitly +// set to null (including the bare "key:" form and the "~" alias) aborts +// 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" + + 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) + } + } + }) + } +} + func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) { yamlContent := `signing_key: ` + validTestSigningKey + ` whitelist_hosts: -- 2.49.1 From c0d325156f091965664258d44341d4ef6a86ec4d Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:02:16 +0000 Subject: [PATCH 05/10] fix: abort startup when a config key is explicitly set to null An explicitly-null key (port: null, bare port:, debug: ~, metrics subkeys, and every other known key) previously fell through the ok/nil check in the strict getters and silently took the default, violating the no-silent-fallback rule and contradicting metrics: null which already aborted. validateKnownKeys now collects null-valued keys (top level and metrics subkeys) and aborts naming each one, and the strict getters and validateAllowlistHostsValue error on null instead of defaulting as defense in depth. This also replaces the unhelpful 'value is not a map of metrics settings' rendering for metrics: null with the null-specific message. --- internal/config/config.go | 80 ++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 14 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 6a3af39..269ec8e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -132,10 +132,12 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { // validateKnownKeys rejects configuration files containing keys the // application does not understand, so typos fail at startup instead of -// being silently ignored. The env section is permitted because +// being silently ignored, and rejects keys that are explicitly set to +// null: a null is a SET value, never an omission, so it must not +// silently take the default. The env section is permitted because // smartconfig consumes it for environment variable injection. func validateKnownKeys(sc *smartconfig.Config) error { - var unknown []string + var unknown, nullKeys []string for key, value := range sc.Data() { if !isKnownConfigKey(key) { @@ -144,6 +146,12 @@ func validateKnownKeys(sc *smartconfig.Config) error { continue } + if value == nil { + nullKeys = append(nullKeys, key) + + continue + } + if key == "metrics" { metricsMap, ok := value.(map[string]interface{}) if !ok { @@ -152,9 +160,15 @@ func validateKnownKeys(sc *smartconfig.Config) error { "metrics", value) } - for subkey := range metricsMap { + for subkey, subvalue := range metricsMap { if subkey != "username" && subkey != "password" { unknown = append(unknown, "metrics."+subkey) + + continue + } + + if subvalue == nil { + nullKeys = append(nullKeys, "metrics."+subkey) } } } @@ -166,9 +180,29 @@ func validateKnownKeys(sc *smartconfig.Config) error { return fmt.Errorf("unknown config keys: %s", strings.Join(unknown, ", ")) } + if len(nullKeys) > 0 { + sort.Strings(nullKeys) + + if len(nullKeys) == 1 { + 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 nil } +// errNullConfigValue reports a config key that is explicitly set to +// 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) +} + // isKnownConfigKey reports whether key is a permitted top-level // configuration key. func isKnownConfigKey(key string) bool { @@ -371,17 +405,22 @@ func (l *strictLoader) boolVal(key string, defaultVal bool) bool { } // getString returns the string value for key, or defaultVal if the key -// is omitted. A present value that is not a string is an error. +// is omitted. A present value that is not a string, or is explicitly +// null, is an error. func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { if sc == nil { return defaultVal, nil } raw, ok := sc.Get(key) - if !ok || raw == nil { + if !ok { return defaultVal, nil } + if raw == nil { + return "", errNullConfigValue(key) + } + str, ok := raw.(string) if !ok { return "", fmt.Errorf("config key %q: value %v (%T) is not a string", @@ -392,18 +431,22 @@ func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { } // getInt returns the integer value for key, or defaultVal if the key is -// omitted. A present value that is not a whole number is an error; -// fractional values are never truncated. +// omitted. A present value that is not a whole number, or is explicitly +// null, is an error; fractional values are never truncated. func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { if sc == nil { return defaultVal, nil } raw, ok := sc.Get(key) - if !ok || raw == nil { + if !ok { return defaultVal, nil } + if raw == nil { + return 0, errNullConfigValue(key) + } + switch val := raw.(type) { case int: return val, nil @@ -430,17 +473,22 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { // getBool returns the boolean value for key, or defaultVal if the key // is omitted. A present value that is not a boolean (or a ParseBool-able -// string) is an error; numbers are not accepted as booleans. +// string), or is explicitly null, is an error; numbers are not accepted +// as booleans. func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) { if sc == nil { return defaultVal, nil } raw, ok := sc.Get(key) - if !ok || raw == nil { + if !ok { return defaultVal, nil } + if raw == nil { + return false, errNullConfigValue(key) + } + switch val := raw.(type) { case bool: return val, nil @@ -459,17 +507,21 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) // validateAllowlistHostsValue checks the raw shape of the // allowlist_hosts value before the lenient extraction in getStringSlice -// runs: a value that is not a list of strings (or a comma-separated -// string), a non-string entry, or an empty entry is an error, never -// silently skipped. +// runs: an explicitly null value, a value that is not a list of strings +// (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) - if !ok || raw == nil { + if !ok { return nil } + if raw == nil { + return errNullConfigValue(key) + } + switch val := raw.(type) { case []interface{}: for _, item := range val { -- 2.49.1 From 813ff63153247a5b6844421d0f7975e49cc52868 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:02:44 +0000 Subject: [PATCH 06/10] test: explicitly empty db_url must abort startup (PR #53 rework) db_url: "" currently silently derives the state_dir-based sqlite URL, which is a default applied to a SET value; state_dir: "" already aborts. Failing test first, fix follows. --- internal/config/config_validation_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_test.go index f1a58f6..c5dbc92 100644 --- a/internal/config/config_validation_test.go +++ b/internal/config/config_validation_test.go @@ -395,6 +395,25 @@ func TestExplicitNullValueAbortsStartup(t *testing.T) { } } +// TestExplicitlyEmptyDBURLAbortsStartup verifies that db_url set to an +// empty string aborts startup: the derived file:...state.sqlite3 URL is +// 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" + + c, err := configFromYAML(t, yamlContent) + if err == nil { + t.Fatalf("explicitly empty db_url must abort startup, got config: %+v", c) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "db_url") { + t.Errorf("error %q does not name the offending key db_url", err.Error()) + } +} + func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) { yamlContent := `signing_key: ` + validTestSigningKey + ` whitelist_hosts: -- 2.49.1 From 5297e6033a2c3716ca0aa35a62652928c89b9ced Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:03:12 +0000 Subject: [PATCH 07/10] fix: abort startup on explicitly empty db_url instead of deriving The derived file:...state.sqlite3 URL is a default and defaults apply only to omitted keys: db_url set to an empty string now aborts naming the key, matching the existing behavior of state_dir: "". --- internal/config/config.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 269ec8e..95864fa 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -113,9 +113,19 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { "upstream_connections_per_host", DefaultUpstreamConnectionsPerHost), } - // Build DBURL from StateDir if not explicitly set + // 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", "") - if c.DBURL == "" { + if c.DBURL == "" && loader.err == nil { + if sc != nil { + if _, present := sc.Get("db_url"); present { + return nil, fmt.Errorf( + "config key %q: value must not be empty; omit the key to derive it from state_dir", + "db_url") + } + } + c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) } -- 2.49.1 From 808356f1427b5f9775da001165030f0e163de97c Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:03:32 +0000 Subject: [PATCH 08/10] test: allowlist_hosts must reject dot-only entries (PR #53 rework) A bare "." entry becomes a HasSuffix suffix pattern that matches any upstream written in FQDN trailing-dot form (evil.com.), effectively disabling URL signing with one character. Failing test first, fix follows. --- internal/config/config_validation_test.go | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/config/config_validation_test.go b/internal/config/config_validation_test.go index c5dbc92..127c0d7 100644 --- a/internal/config/config_validation_test.go +++ b/internal/config/config_validation_test.go @@ -414,6 +414,35 @@ func TestExplicitlyEmptyDBURLAbortsStartup(t *testing.T) { } } +// TestAllowlistHostsRejectsDotOnlyEntries verifies that entries with no +// hostname labels are rejected. The allowlist matcher treats a leading +// dot as a suffix pattern, so a bare "." entry would match any upstream +// 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" + + for _, entry := range []string{".", ".."} { + t.Run(entry, func(t *testing.T) { + yamlContent := signingKeyLine + + "allowlist_hosts:\n - \"" + entry + "\"\n" + + c, err := configFromYAML(t, yamlContent) + if err == nil { + t.Fatalf("allowlist entry %q must abort startup, got config: %+v", + entry, c) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "allowlist_hosts") { + t.Errorf("error %q does not name the offending key allowlist_hosts", + err.Error()) + } + }) + } +} + func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) { yamlContent := `signing_key: ` + validTestSigningKey + ` whitelist_hosts: -- 2.49.1 From 83fa22871e2bcd0ba45a008a53c0710e70dd93c1 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:03:50 +0000 Subject: [PATCH 09/10] fix: reject allowlist_hosts entries containing no hostname labels Entries consisting only of dots (".", "..") are now a startup error naming the key and entry. Previously a bare "." passed validation and became a suffix pattern matching every trailing-dot FQDN upstream, bypassing URL signing. --- internal/config/config.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 95864fa..cb5b9b9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -312,7 +312,11 @@ func (c *Config) validate() error { // validateAllowlistHost checks that an allowlist_hosts entry is a bare // hostname, optionally with a leading dot for suffix matching. URLs, -// paths, and whitespace indicate a misconfigured entry. +// paths, and whitespace indicate a misconfigured entry. An entry with +// no hostname labels (such as ".") is rejected: the allowlist matcher +// treats a leading dot as a suffix pattern, so a bare "." would match +// any upstream host written in FQDN trailing-dot form and effectively +// disable URL signing. func validateAllowlistHost(host string) error { if strings.Contains(host, "://") || strings.ContainsAny(host, "/ \t") { return fmt.Errorf( @@ -320,6 +324,12 @@ func validateAllowlistHost(host string) error { "allowlist_hosts", host) } + if strings.Trim(host, ".") == "" { + return fmt.Errorf( + "config key %q: entry %q contains no hostname labels", + "allowlist_hosts", host) + } + return nil } -- 2.49.1 From 22f19c849b52d53d7ae195213ada1dc1596ef11e Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 17:04:08 +0000 Subject: [PATCH 10/10] style: use the config key error-message convention for signing_key The signing_key errors used bare phrasing while every other validation error follows the 'config key %q' convention; align them. The secret value itself is still never echoed. --- internal/config/config.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index cb5b9b9..702f1b8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -262,15 +262,16 @@ func (c *Config) ensureStateDirWritable() error { // validate checks that all required configuration values are set and // that every value is within its valid range. func (c *Config) validate() error { + // The signing key value is never echoed in error messages. if c.SigningKey == "" { - return fmt.Errorf("signing_key is required") + return fmt.Errorf("config key %q: a value is required", "signing_key") } // Minimum key length for security (32 bytes = 256 bits) const minKeyLength = 32 if len(c.SigningKey) < minKeyLength { - return fmt.Errorf("signing_key must be at least %d characters, got %d", - minKeyLength, len(c.SigningKey)) + return fmt.Errorf("config key %q: value must be at least %d characters, got %d", + "signing_key", minKeyLength, len(c.SigningKey)) } const maxPort = 65535 -- 2.49.1