Files
pixa/internal/config/config_validation_test.go
clawbot 61f42e6602
All checks were successful
check / check (push) Successful in 4s
feat: validate configuration on startup, fail fast on bad config (closes #52) (#53)
closes #52

Implements startup configuration validation per the plan on #52. Two commits, TDD: the first commit adds the enforcement tests (red — six test functions fail against the lenient behavior) plus a mechanical extraction of `newFromSmartConfig` from `config.New` so construction is testable without fx; the second commit makes them green and carries the `TODO.md` bookkeeping.

## Behavior

- **No silent fallbacks**: a config value that is SET but unparseable or invalid aborts startup with an error naming the key and value. Defaults apply only to OMITTED keys. The old `getString`/`getInt`/`getBool` helpers swallowed every conversion error and returned the default; they are now strict. Fractional ports are rejected, not truncated (smartconfig's `GetInt` would have turned `8080.5` into `8080`).
- **Unknown keys abort**: unknown top-level keys and unknown `metrics` subkeys are fatal, each named in the error (`unknown config keys: whitelist_hosts`). The `env` section stays permitted because smartconfig consumes it for environment injection.
- **Malformed config file aborts**: a config file that exists at a standard location but fails to parse was previously logged as a warning and skipped (the server would start on defaults); it is now fatal.
- **Range/sanity checks**: `port` in 1-65535; `upstream_connections_per_host` at least 1; `signing_key` required, at least 32 characters (keyless mode was never implemented; the stale "leave empty" comment in `config.example.yml` is corrected); `allowlist_hosts` entries must be bare hostnames (leading-dot suffix patterns still allowed; schemes, paths, whitespace, non-string and empty entries rejected); `state_dir` non-empty and verified creatable+writable with a probe file before the listener binds; `sentry_dsn` must be a URL with scheme and host when set; `metrics.username`/`metrics.password` must be set together.

## Verification

- `make check` green on the branch head (all tests, golangci-lint 0 issues, fmt-check clean).
- End-to-end: `./bin/pixad` with `port: banana` exits 1 printing `config key "port": value "banana" is not an integer`; with `whitelist_hosts:` it exits 1 printing `unknown config keys: whitelist_hosts`.

## Notes for review

- `getStringSlice` keeps its lenient signature because the existing tests in `config_test.go` exercise it and modifying existing tests requires explicit approval. Strictness for `allowlist_hosts` is instead enforced up front on the raw value by `validateAllowlistHostsValue`, so nothing is silently skipped; extraction then reuses the existing parser. If you prefer the helper folded into a single strict function, that requires retargeting those three tests — happy to do that as a follow-up with approval.
- `TODO.md` here is edited against current `main`; PR #50 (merge-ready) edits adjacent lines, so whichever merges second will need a trivial rebase of `TODO.md` only.
- The README Configuration section lists keys that have never existed in the code (`access_control_allow_origin`, `upstream_fetch_timeout`, `upstream_max_response_size`, `downstream_timeout`). Under this change a config using them now fails fast instead of silently doing nothing — that is the intended behavior. Implementing them is already tracked as the P2 "add all configuration options from README" item in `TODO.md`.

Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #53
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
2026-08-07 22:39:40 +02:00

548 lines
16 KiB
Go

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)
}
}
})
}
}
// 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)
}
}
})
}
}
// 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())
}
}
// 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:
- 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())
}
}