From 985464dcf9a0b157f6319eba034a7d93ff037421 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 01:45:27 +0000 Subject: [PATCH] Fail loudly on set-but-unparseable env config values (closes #80) The config env helpers silently substituted the documented default whenever a variable was set but could not be parsed, so a typo in an operator-supplied value produced a running daemon with configuration nobody asked for instead of a startup failure. `PORT=eighty` quietly listened on 8080 and `DEBUG=ture` quietly disabled debug logging. Defaults now apply only to variables that are unset or empty. Any variable that is set but unparseable is a hard error that names the key and the offending value and aborts startup through fx. - add `envPositiveInt` with `ErrNonPositiveValue`, copied verbatim from the definition on the unmerged #87 so that rebasing after it lands is a delete-one-copy operation rather than a semantic merge - remove `envInt` entirely; `PORT` is parsed by a new `envPort`, which adds the TCP upper bound (`ErrInvalidPort`, 1..65535) - change `envBool` to return an error and parse with `strconv.ParseBool`, so `yes`, `on`, and typos are rejected rather than silently treated as false; callers are `DEBUG` and `MAINTENANCE_MODE` - move env loading into `loadFromEnv`, with the environment check extracted to `resolveEnvironment` (also as #87 defines it), keeping `New` within the funlen budget `envString` parses nothing and `envDuration` was already fail-loud, so both are unchanged. A repo-wide audit of `os.Getenv`/`os.LookupEnv` found no parse sites outside `internal/config`. Tests cover each helper with a table (unset, valid, set-but-invalid) plus `config.New`-level cases proving a bad `PORT`, `DEBUG`, or `MAINTENANCE_MODE` aborts startup while unset variables still get their defaults. README documents the fail-loud rule and the accepted boolean spellings. --- README.md | 17 ++ TODO.md | 28 +-- internal/config/config.go | 172 +++++++++++--- internal/config/env_test.go | 409 +++++++++++++++++++++++++++++++++ internal/config/export_test.go | 20 ++ 5 files changed, 597 insertions(+), 49 deletions(-) create mode 100644 internal/config/env_test.go create mode 100644 internal/config/export_test.go diff --git a/README.md b/README.md index 1514b61..1961aa9 100644 --- a/README.md +++ b/README.md @@ -89,10 +89,27 @@ TTY detection, and security headers are always applied. | `PORT` | HTTP listen port | `8080` | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | +| `MAINTENANCE_MODE` | Serve the maintenance page | `false` | | `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | +#### Invalid values abort startup + +The defaults above apply **only** to variables that are unset (or set +to an empty string). A variable that is set but cannot be parsed is a +fatal configuration error: webhooker logs the offending variable and +its value and refuses to start, rather than silently running with a +substituted default. `PORT=eighty`, `DEBUG=ture`, and +`RETENTION_SWEEP_INTERVAL=1 hour` all abort startup. `PORT` must +additionally be a number in the range 1–65535. + +Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the +spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, +`true`, `True`, `0`, `f`, `F`, `FALSE`, `false`, `False` — and nothing +else. `yes`, `on`, and `off` are rejected rather than quietly treated +as false. + On first startup, webhooker automatically generates a cryptographically secure session encryption key and stores it in the database. This key persists across restarts — no manual key management is needed. diff --git a/TODO.md b/TODO.md index c869649..5bb3ff5 100644 --- a/TODO.md +++ b/TODO.md @@ -10,24 +10,28 @@ # Status -pre-1.0. No git tags exist. main (afe88c6) is a working webhook proxy +pre-1.0. No git tags exist. main (4f5ecb1) is a working webhook proxy with auth, CSRF/SSRF protections, login rate limiting, Slack target, -policy compliance (#6), and pinned lint tooling (#55). Note: TODO.md was -deliberately deleted from this repo in f9a9569 (2026-03-01, #6); its -content was folded into the README TODO section, which this draft -reconstructs as of 2026-07-06. +policy compliance (#6), pinned lint tooling (#55), a per-webhook event +retention reaper (#63), and fail-loud configuration parsing (#80). Note: +TODO.md was deliberately deleted from this repo in f9a9569 (2026-03-01, +#6); its content was folded into the README TODO section, which this +draft reconstructs as of 2026-07-06. # Next Step -Implement automatic event retention cleanup based on retention_days: a -periodic maintenance job that deletes Events, Deliveries, and -DeliveryResults older than the parent webhook's retention_days from each -per-webhook event database. The field exists on the Webhook model and -the README promises the behavior, but nothing enforces it, so event -databases currently grow without bound. +Manual event redelivery from the web UI (replay is a core promised +capability in the README rationale). # Completed Steps +- 2026-08-09 Configuration parsing fails loudly on set-but-unparseable + environment values: `envInt` removed in favour of `envPositiveInt` + plus a `PORT` range check, `envBool` now parses with + `strconv.ParseBool`, and defaults apply only to unset variables (#80) +- 2026-08-07 Automatic event retention cleanup based on + `retention_days`, deleting expired events, deliveries, and delivery + results from each per-webhook event database (#63) - 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in `Dockerfile`, release-archive sha256 pins in `script/bootstrap`), adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so @@ -56,8 +60,6 @@ databases currently grow without bound. # Future Steps -- Manual event redelivery from the web UI (replay is a core promised - capability in the README rationale) - Delivery status and retry management UI - Per-webhook rate limiting in the receiver handler (per-webhook config plus handler enforcement; global limits must not apply to receiver diff --git a/internal/config/config.go b/internal/config/config.go index 414a3bb..5ba979d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,7 +7,6 @@ import ( "log/slog" "os" "strconv" - "strings" "time" "go.uber.org/fx" @@ -31,12 +30,24 @@ const ( // defaultRetentionSweepInterval is how often the retention // reaper deletes events older than each webhook's RetentionDays. defaultRetentionSweepInterval = time.Hour + + // maxPort is the highest valid TCP port number. The lower + // bound (at least 1) is enforced by envPositiveInt. + maxPort = 65535 ) // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT // contains an unrecognised value. var ErrInvalidEnvironment = errors.New("invalid environment") +// ErrNonPositiveValue is returned when an environment variable that +// requires a positive integer is set to zero or a negative number. +var ErrNonPositiveValue = errors.New("value must be positive") + +// ErrInvalidPort is returned when an environment variable holding a +// TCP port number is set above the valid port range. +var ErrInvalidPort = errors.New("invalid port") + //nolint:revive // ConfigParams is a standard fx naming convention. type ConfigParams struct { fx.In @@ -81,27 +92,81 @@ func envString(key string) string { } // envBool returns the value of the named environment variable -// parsed as a boolean. Returns defaultValue if not set. -func envBool(key string, defaultValue bool) bool { - if v := os.Getenv(key); v != "" { - return strings.EqualFold(v, "true") || v == "1" +// parsed as a boolean. Returns defaultValue if not set. If the +// variable is set but cannot be parsed, it returns a wrapped error +// naming the key and the bad value, so startup fails loudly rather +// than silently falling back to the default. +// +// Parsing is strconv.ParseBool, which accepts 1, t, T, TRUE, true, +// True, 0, f, F, FALSE, false and False. Anything else — "yes", +// "on", or a typo like "ture" — is an error rather than a silent +// false. +func envBool(key string, defaultValue bool) (bool, error) { + v := os.Getenv(key) + if v == "" { + return defaultValue, nil } - return defaultValue + b, err := strconv.ParseBool(v) + if err != nil { + return false, fmt.Errorf( + "invalid boolean for %s: %q: %w", key, v, err, + ) + } + + return b, nil } -// envInt returns the value of the named environment variable -// parsed as an integer. Returns defaultValue if not set or -// unparseable. -func envInt(key string, defaultValue int) int { - if v := os.Getenv(key); v != "" { - i, err := strconv.Atoi(v) - if err == nil { - return i - } +// envPositiveInt returns the value of the named environment variable +// parsed as a positive integer. Returns defaultValue if not set. If +// the variable is set but cannot be parsed, or parses to less than +// one, it returns a wrapped error naming the key and the bad value, +// so startup fails loudly rather than silently falling back to the +// default. +func envPositiveInt( + key string, + defaultValue int, +) (int, error) { + v := os.Getenv(key) + if v == "" { + return defaultValue, nil } - return defaultValue + i, err := strconv.Atoi(v) + if err != nil { + return 0, fmt.Errorf( + "invalid integer for %s: %q: %w", key, v, err, + ) + } + + if i < 1 { + return 0, fmt.Errorf( + "%w: %s must be at least 1, got %q", + ErrNonPositiveValue, key, v, + ) + } + + return i, nil +} + +// envPort returns the value of the named environment variable parsed +// as a TCP port number. Returns defaultValue if not set. A set value +// that is unparseable, below 1, or above maxPort is a hard error +// naming the key and the bad value. +func envPort(key string, defaultValue int) (int, error) { + port, err := envPositiveInt(key, defaultValue) + if err != nil { + return 0, err + } + + if port > maxPort { + return 0, fmt.Errorf( + "%w: %s must be at most %d, got %d", + ErrInvalidPort, key, maxPort, port, + ) + } + + return port, nil } // envDuration returns the value of the named environment variable @@ -128,32 +193,52 @@ func envDuration( return d, nil } -// New creates a Config by reading environment variables. -// -//nolint:revive // lc parameter is required by fx even if unused. -func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { - log := params.Logger.Get() - - // Determine environment from WEBHOOKER_ENVIRONMENT env var, - // default to dev +// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to +// dev, and rejects unrecognised values. +func resolveEnvironment() (string, error) { environment := os.Getenv("WEBHOOKER_ENVIRONMENT") if environment == "" { environment = EnvironmentDev } - // Validate environment if environment != EnvironmentDev && environment != EnvironmentProd { - return nil, fmt.Errorf( + return "", fmt.Errorf( "%w: WEBHOOKER_ENVIRONMENT must be '%s' or '%s', got '%s'", ErrInvalidEnvironment, EnvironmentDev, EnvironmentProd, environment, ) } - // Parse the retention sweep interval; a set-but-unparseable value - // is a hard error so fx aborts startup rather than silently using - // the default. + return environment, nil +} + +// loadFromEnv builds a Config from the environment. Every value that +// needs parsing fails loudly when it is set but unparseable: the +// documented defaults apply only to variables that are unset (or +// empty), never as a substitute for a value the operator actually +// provided. +func loadFromEnv() (*Config, error) { + environment, err := resolveEnvironment() + if err != nil { + return nil, err + } + + port, err := envPort("PORT", defaultPort) + if err != nil { + return nil, err + } + + debug, err := envBool("DEBUG", false) + if err != nil { + return nil, err + } + + maintenanceMode, err := envBool("MAINTENANCE_MODE", false) + if err != nil { + return nil, err + } + retentionSweepInterval, err := envDuration( "RETENTION_SWEEP_INTERVAL", defaultRetentionSweepInterval, @@ -162,21 +247,36 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { return nil, err } - // Load configuration values from environment variables - s := &Config{ + return &Config{ DataDir: envString("DATA_DIR"), - Debug: envBool("DEBUG", false), - MaintenanceMode: envBool("MAINTENANCE_MODE", false), + Debug: debug, + MaintenanceMode: maintenanceMode, Environment: environment, MetricsUsername: envString("METRICS_USERNAME"), MetricsPassword: envString("METRICS_PASSWORD"), - Port: envInt("PORT", defaultPort), + Port: port, SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, - log: log, - params: ¶ms, + }, nil +} + +// New creates a Config by reading environment variables. +// +//nolint:revive // lc parameter is required by fx even if unused. +func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { + log := params.Logger.Get() + + // A set-but-unparseable value anywhere in the environment is a + // hard error, so fx aborts startup rather than running with a + // silently substituted default. + s, err := loadFromEnv() + if err != nil { + return nil, err } + s.log = log + s.params = ¶ms + // Set default DataDir. All SQLite databases (main application // DB and per-webhook event DBs) live here. The same default is // used regardless of environment; override with DATA_DIR if diff --git a/internal/config/env_test.go b/internal/config/env_test.go new file mode 100644 index 0000000..a127abe --- /dev/null +++ b/internal/config/env_test.go @@ -0,0 +1,409 @@ +package config_test + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/globals" + "sneak.berlin/go/webhooker/internal/logger" +) + +// testEnvKey is a throwaway variable name used only by the helper +// tables below, so they cannot disturb real configuration. +const testEnvKey = "WEBHOOKER_TEST_VALUE" + +// Real configuration variables exercised by the config.New tests. +const ( + envKeyPort = "PORT" + envKeyDebug = "DEBUG" + envKeyMaintenanceMode = "MAINTENANCE_MODE" +) + +// envBoolCase is one row of the envBool table. +type envBoolCase struct { + name string + set bool + value string + defaultValue bool + expectError bool + expected bool +} + +// envBoolCases is the envBool table, kept out of the test body so +// the test itself stays readable. +func envBoolCases() []envBoolCase { + return []envBoolCase{ + { + name: "unset uses default false", + defaultValue: false, + expected: false, + }, + { + name: "unset uses default true", + defaultValue: true, + expected: true, + }, + { + name: "empty uses default true", + set: true, + value: "", + defaultValue: true, + expected: true, + }, + { + name: "true is parsed", + set: true, + value: "true", + expected: true, + }, + { + name: "one is parsed", + set: true, + value: "1", + expected: true, + }, + { + name: "False is parsed", + set: true, + value: "False", + defaultValue: true, + expected: false, + }, + { + name: "zero is parsed", + set: true, + value: "0", + defaultValue: true, + expected: false, + }, + { + name: "yes is rejected", + set: true, + value: "yes", + expectError: true, + }, + { + name: "on is rejected", + set: true, + value: "on", + expectError: true, + }, + { + name: "typo is rejected", + set: true, + value: "ture", + expectError: true, + }, + } +} + +func TestEnvBool(t *testing.T) { + for _, tt := range envBoolCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvBoolForTest( + testEnvKey, tt.defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestEnvPositiveInt(t *testing.T) { + const defaultValue = 7 + + tests := []struct { + name string + set bool + value string + expectError bool + errIs error + expected int + }{ + { + name: "unset returns the default integer", + expected: defaultValue, + }, + { + name: "empty returns the default integer", + set: true, + value: "", + expected: defaultValue, + }, + { + name: "positive value is parsed", + set: true, + value: "42", + expected: 42, + }, + { + name: "unparseable value is rejected", + set: true, + value: "not-a-number", + expectError: true, + }, + { + name: "zero is rejected", + set: true, + value: "0", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + { + name: "negative is rejected", + set: true, + value: "-5", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvPositiveIntForTest( + testEnvKey, defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + if tt.errIs != nil { + require.ErrorIs(t, err, tt.errIs) + } + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestEnvPort(t *testing.T) { + const defaultValue = 8080 + + tests := []struct { + name string + set bool + value string + expectError bool + errIs error + expected int + }{ + { + name: "unset returns the default port", + expected: defaultValue, + }, + { + name: "valid port is parsed", + set: true, + value: "9000", + expected: 9000, + }, + { + name: "highest port is accepted", + set: true, + value: "65535", + expected: 65535, + }, + { + name: "unparseable value is rejected", + set: true, + value: "not-a-port", + expectError: true, + }, + { + name: "zero is rejected", + set: true, + value: "0", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + { + name: "above the port range is rejected", + set: true, + value: "65536", + expectError: true, + errIs: config.ErrInvalidPort, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvPortForTest( + testEnvKey, defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + + if tt.errIs != nil { + require.ErrorIs(t, err, tt.errIs) + } + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +// buildConfig constructs a Config through fx exactly as the +// application does, returning the config and any construction error. +func buildConfig(t *testing.T) (*config.Config, error) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + + return cfg, app.Err() +} + +func TestNewRejectsBadEnvValues(t *testing.T) { + tests := []struct { + name string + key string + value string + expectError bool + check func(t *testing.T, cfg *config.Config) + }{ + { + name: "valid PORT is used", + key: envKeyPort, + value: "9001", + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.Equal(t, 9001, cfg.Port) + }, + }, + { + name: "unparseable PORT aborts startup", + key: envKeyPort, + value: "eighty-eighty", + expectError: true, + }, + { + name: "out-of-range PORT aborts startup", + key: envKeyPort, + value: "70000", + expectError: true, + }, + { + name: "valid DEBUG is used", + key: envKeyDebug, + value: "true", + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.True(t, cfg.Debug) + }, + }, + { + name: "unparseable DEBUG aborts startup", + key: envKeyDebug, + value: "ture", + expectError: true, + }, + { + name: "unparseable MAINTENANCE_MODE aborts startup", + key: envKeyMaintenanceMode, + value: "sometimes", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + t.Setenv(tt.key, tt.value) + + cfg, err := buildConfig(t) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + require.NotNil(t, cfg) + tt.check(t, cfg) + }) + } +} + +// TestNewUsesDefaultsWhenUnset proves the fail-loud behaviour did not +// break the legitimate unset case: absent variables still get their +// documented defaults. +func TestNewUsesDefaultsWhenUnset(t *testing.T) { + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + + for _, key := range []string{ + envKeyPort, envKeyDebug, envKeyMaintenanceMode, + } { + require.NoError(t, os.Unsetenv(key)) + } + + cfg, err := buildConfig(t) + require.NoError(t, err) + require.NotNil(t, cfg) + + assert.Equal(t, 8080, cfg.Port) + assert.False(t, cfg.Debug) + assert.False(t, cfg.MaintenanceMode) +} diff --git a/internal/config/export_test.go b/internal/config/export_test.go new file mode 100644 index 0000000..c7fac51 --- /dev/null +++ b/internal/config/export_test.go @@ -0,0 +1,20 @@ +package config + +// This file exposes the unexported environment parsing helpers to +// the external config_test package so each helper can be covered by +// its own table-driven test without weakening the package API. + +// EnvBoolForTest exposes envBool. +func EnvBoolForTest(key string, defaultValue bool) (bool, error) { + return envBool(key, defaultValue) +} + +// EnvPositiveIntForTest exposes envPositiveInt. +func EnvPositiveIntForTest(key string, defaultValue int) (int, error) { + return envPositiveInt(key, defaultValue) +} + +// EnvPortForTest exposes envPort. +func EnvPortForTest(key string, defaultValue int) (int, error) { + return envPort(key, defaultValue) +}