From e72d08db13f9da3304ee8f63969606cf382dda89 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 2 Oct 2026 13:49:28 +0000 Subject: [PATCH] Isolate config tests from the shell; any out-of-range PORT is ErrInvalidPort (closes #94) Config tests, and the first-boot debug log test that builds a Config, start from an empty environment: config.ClearEnvForTest unsets every variable the process has and restores each when the test ends, so nothing exported in the developer's shell changes a result. TestEnvPositiveInt and TestEnvPort share one table runner, and TestEnvPort checks that the bad value appears in each error. Every out-of-range PORT now wraps ErrInvalidPort: zero, negatives, above 65535, and numbers too large or too small for an int. The README configuration table and the Settings page say that a RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative, fails startup. Model: opus-5-5 --- README.md | 5 +- internal/config/config.go | 29 +++-- internal/config/config_test.go | 61 +++-------- internal/config/dotenv_test.go | 25 ++--- internal/config/env_test.go | 169 +++++++++++++---------------- internal/config/sentry_test.go | 5 +- internal/config/testing.go | 30 +++++ internal/gormlog/firstboot_test.go | 5 +- internal/handlers/settings.go | 3 +- 9 files changed, 160 insertions(+), 172 deletions(-) create mode 100644 internal/config/testing.go diff --git a/README.md b/README.md index 793e67c..9629fb3 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,7 @@ TTY detection, and security headers are always applied. | `METRICS_USERNAME` | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""` | | `SENTRY_DSN` | Sentry error reporting DSN. Unset leaves error reporting off; a value the Sentry SDK cannot parse fails startup rather than serving with reporting silently off | `""` | -| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` | +| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive). A value that does not parse, or is zero or negative, fails startup | `1h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted. A set value replaces the default. If any client can reach webhooker, or the proxy in front of it, from an RFC 1918 source address, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) | @@ -2924,7 +2924,8 @@ webhooker/ │ ├── resetpw/ │ │ └── resetpw.go # `webhooker resetpw`: set an account's password, stopped deployments only │ ├── config/ -│ │ └── config.go # Configuration loading from environment variables +│ │ ├── config.go # Configuration loading from environment variables +│ │ └── testing.go # ClearEnvForTest: an empty environment for one test │ ├── database/ │ │ ├── base_model.go # BaseModel with UUID primary keys │ │ ├── database.go # GORM connection, migrations, admin seed diff --git a/internal/config/config.go b/internal/config/config.go index 386f17a..b2eaf01 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -80,8 +80,7 @@ const ( // process over a Docker network or a private LAN connects from. defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" - // maxPort is the highest valid TCP port number. The lower - // bound (at least 1) is enforced by envPositiveInt. + // maxPort is the highest valid TCP port number. maxPort = 65535 // mappedV4Offset is the number of leading bits an IPv4-mapped @@ -105,7 +104,7 @@ var ErrInvalidEnvironment = errors.New("invalid environment") 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. +// TCP port number is set to a number outside 1 to 65535. var ErrInvalidPort = errors.New("invalid port") // ErrInvalidCIDR is returned when an environment variable holding a @@ -363,17 +362,27 @@ func envPositiveInt( // 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. +// naming the key and the bad value; every out-of-range value wraps +// ErrInvalidPort, including one too large or too small for an int. func envPort(key string, defaultValue int) (int, error) { - port, err := envPositiveInt(key, defaultValue) - if err != nil { - return 0, err + v := os.Getenv(key) + if v == "" { + return defaultValue, nil } - if port > maxPort { + // strconv.ErrRange means a number too large or too small for an + // int, which is outside the port range as well. + port, err := strconv.Atoi(v) + if err != nil && !errors.Is(err, strconv.ErrRange) { return 0, fmt.Errorf( - "%w: %s must be at most %d, got %d", - ErrInvalidPort, key, maxPort, port, + "invalid integer for %s: %q: %w", key, v, err, + ) + } + + if err != nil || port < 1 || port > maxPort { + return 0, fmt.Errorf( + "%w: %s must be from 1 to %d, got %q", + ErrInvalidPort, key, maxPort, v, ) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e5f76b3..8b11f5f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,7 +3,6 @@ package config_test import ( "bytes" "log/slog" - "os" "testing" "time" @@ -71,14 +70,12 @@ func TestEnvironmentConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if tt.envValue != "" { t.Setenv( "WEBHOOKER_ENVIRONMENT", tt.envValue, ) - } else { - require.NoError(t, os.Unsetenv( - "WEBHOOKER_ENVIRONMENT", - )) } for k, v := range tt.envVars { @@ -199,14 +196,11 @@ func TestRetentionSweepInterval(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { t.Setenv("RETENTION_SWEEP_INTERVAL", tt.value) - } else { - require.NoError(t, os.Unsetenv( - "RETENTION_SWEEP_INTERVAL", - )) } if tt.expectError { @@ -341,14 +335,11 @@ func TestSessionIdleTimeout(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { t.Setenv("SESSION_IDLE_TIMEOUT", tt.value) - } else { - require.NoError(t, os.Unsetenv( - "SESSION_IDLE_TIMEOUT", - )) } if tt.expectError { @@ -397,16 +388,12 @@ func TestDefaultDataDir(t *testing.T) { t.Run("env="+name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if env != "" { t.Setenv("WEBHOOKER_ENVIRONMENT", env) - } else { - require.NoError(t, os.Unsetenv( - "WEBHOOKER_ENVIRONMENT", - )) } - require.NoError(t, os.Unsetenv("DATA_DIR")) - var cfg *config.Config app := fxtest.New( @@ -446,9 +433,9 @@ func TestDataDirHelper(t *testing.T) { t.Run(name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - if set == "" { - require.NoError(t, os.Unsetenv("DATA_DIR")) - } else { + config.ClearEnvForTest(t) + + if set != "" { t.Setenv("DATA_DIR", set) } @@ -511,14 +498,11 @@ func TestReceiverRateLimit(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { t.Setenv("RECEIVER_RATE_LIMIT", tt.value) - } else { - require.NoError(t, os.Unsetenv( - "RECEIVER_RATE_LIMIT", - )) } if tt.expectError { @@ -630,12 +614,11 @@ func TestTrustedProxies(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { t.Setenv("TRUSTED_PROXIES", tt.value) - } else { - require.NoError(t, os.Unsetenv("TRUSTED_PROXIES")) } if tt.expectError { @@ -742,14 +725,11 @@ func TestAllowedEgressCIDRs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { t.Setenv("ALLOWED_EGRESS_CIDRS", tt.value) - } else { - require.NoError( - t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"), - ) } if tt.expectError { @@ -817,13 +797,10 @@ func TestEgressAllowlistWarning(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev) - if tt.allowed == "" { - require.NoError( - t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"), - ) - } else { + if tt.allowed != "" { t.Setenv("ALLOWED_EGRESS_CIDRS", tt.allowed) } @@ -956,20 +933,14 @@ func TestMetricsAuthConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if tt.username.set { t.Setenv("METRICS_USERNAME", tt.username.value) - } else { - require.NoError( - t, os.Unsetenv("METRICS_USERNAME"), - ) } if tt.password.set { t.Setenv("METRICS_PASSWORD", tt.password.value) - } else { - require.NoError( - t, os.Unsetenv("METRICS_PASSWORD"), - ) } if tt.expectError { diff --git a/internal/config/dotenv_test.go b/internal/config/dotenv_test.go index 952bb32..b2ccbfa 100644 --- a/internal/config/dotenv_test.go +++ b/internal/config/dotenv_test.go @@ -22,17 +22,6 @@ const malformedDotEnv = "PORT 19615\n" + "this is not = valid ! syntax\n" + "\"unclosed\n" -// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of -// the test and restores it afterwards. t.Setenv registers the restore; -// the Unsetenv that follows is what the test actually needs, because a -// variable set to the empty string is still present in os.Environ and -// godotenv would refuse to overwrite it. -func unsetDotEnvKey(t *testing.T) { - t.Helper() - t.Setenv(dotEnvKey, "placeholder") - require.NoError(t, os.Unsetenv(dotEnvKey)) -} - // writeDotEnv writes contents to a .env file in a fresh temporary // directory and returns its path. func writeDotEnv(t *testing.T, contents string) string { @@ -50,9 +39,9 @@ func writeDotEnv(t *testing.T, contents string) string { // normally rather than be refused for a file it was never meant to // have. // -//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +//nolint:paralleltest // ClearEnvForTest uses t.Setenv. func TestLoadDotEnv_MissingFileIsFine(t *testing.T) { - unsetDotEnvKey(t) + config.ClearEnvForTest(t) absent := filepath.Join(t.TempDir(), config.DotEnvPath) require.NoError(t, config.LoadDotEnvFileForTest(absent)) @@ -65,9 +54,9 @@ func TestLoadDotEnv_MissingFileIsFine(t *testing.T) { // reaches the environment, which is the whole reason the file is read // at all. // -//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +//nolint:paralleltest // ClearEnvForTest uses t.Setenv. func TestLoadDotEnv_AppliesValues(t *testing.T) { - unsetDotEnvKey(t) + config.ClearEnvForTest(t) path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n") @@ -93,9 +82,9 @@ func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) { // reverts to its default; the process used to start that way with no // log line naming the file at all. // -//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +//nolint:paralleltest // ClearEnvForTest uses t.Setenv. func TestLoadDotEnv_MalformedFileAborts(t *testing.T) { - unsetDotEnvKey(t) + config.ClearEnvForTest(t) path := writeDotEnv( t, malformedDotEnv+dotEnvKey+"=from-dot-env\n", @@ -143,7 +132,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) { // //nolint:paralleltest // t.Chdir moves the whole process. func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) { - unsetDotEnvKey(t) + config.ClearEnvForTest(t) dir := t.TempDir() require.NoError(t, os.WriteFile( diff --git a/internal/config/env_test.go b/internal/config/env_test.go index 6978f0f..81448c3 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -1,7 +1,6 @@ package config_test import ( - "os" "testing" "github.com/stretchr/testify/assert" @@ -121,10 +120,10 @@ func TestEnvBool(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if tt.set { t.Setenv(testEnvKey, tt.value) - } else { - require.NoError(t, os.Unsetenv(testEnvKey)) } got, err := config.EnvBoolForTest( @@ -145,17 +144,62 @@ func TestEnvBool(t *testing.T) { } } +// envIntCase is one row of the envPositiveInt and envPort tables. +type envIntCase struct { + name string + set bool + value string + expectError bool + errIs error + expected int +} + +// runEnvIntCases runs each row through parse, which is +// envPositiveInt or envPort, with testEnvKey set to the row's value +// or left unset. +func runEnvIntCases( + t *testing.T, + parse func(key string, defaultValue int) (int, error), + defaultValue int, + tests []envIntCase, +) { + t.Helper() + + 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. + config.ClearEnvForTest(t) + + if tt.set { + t.Setenv(testEnvKey, tt.value) + } + + got, err := parse(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) + }) + } +} + +//nolint:paralleltest // runEnvIntCases uses t.Setenv. func TestEnvPositiveInt(t *testing.T) { const defaultValue = 7 - tests := []struct { - name string - set bool - value string - expectError bool - errIs error - expected int - }{ + runEnvIntCases(t, config.EnvPositiveIntForTest, defaultValue, []envIntCase{ { name: "unset returns the default integer", expected: defaultValue, @@ -192,51 +236,14 @@ func TestEnvPositiveInt(t *testing.T) { 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) - }) - } + }) } +//nolint:paralleltest // runEnvIntCases uses t.Setenv. func TestEnvPort(t *testing.T) { const defaultValue = 8080 - tests := []struct { - name string - set bool - value string - expectError bool - errIs error - expected int - }{ + runEnvIntCases(t, config.EnvPortForTest, defaultValue, []envIntCase{ { name: "unset returns the default port", expected: defaultValue, @@ -264,7 +271,14 @@ func TestEnvPort(t *testing.T) { set: true, value: "0", expectError: true, - errIs: config.ErrNonPositiveValue, + errIs: config.ErrInvalidPort, + }, + { + name: "negative is rejected", + set: true, + value: "-1", + expectError: true, + errIs: config.ErrInvalidPort, }, { name: "above the port range is rejected", @@ -273,37 +287,14 @@ func TestEnvPort(t *testing.T) { 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) - }) - } + { + name: "too large for an int is rejected", + set: true, + value: "99999999999999999999", + expectError: true, + errIs: config.ErrInvalidPort, + }, + }) } // TestEnvBindAddress covers BIND_ADDRESS parsing. @@ -319,10 +310,10 @@ func TestEnvBindAddress(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if tt.set { t.Setenv(testEnvKey, tt.value) - } else { - require.NoError(t, os.Unsetenv(testEnvKey)) } got, err := config.EnvBindAddressForTest( @@ -485,6 +476,7 @@ func TestNewRejectsBadEnvValues(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") t.Setenv(tt.key, tt.value) @@ -646,14 +638,9 @@ func sentryEnvValueCases() []badEnvValueCase { // break the legitimate unset case: absent variables still get their // documented defaults. func TestNewUsesDefaultsWhenUnset(t *testing.T) { + config.ClearEnvForTest(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") - for _, key := range []string{ - envKeyPort, envKeyDebug, envKeyBindAddress, envKeySentryDSN, - } { - require.NoError(t, os.Unsetenv(key)) - } - cfg, err := buildConfig(t) require.NoError(t, err) require.NotNil(t, cfg) diff --git a/internal/config/sentry_test.go b/internal/config/sentry_test.go index 0b475bf..871ecb6 100644 --- a/internal/config/sentry_test.go +++ b/internal/config/sentry_test.go @@ -1,7 +1,6 @@ package config_test import ( - "os" "testing" "github.com/stretchr/testify/assert" @@ -101,10 +100,10 @@ func TestEnvSentryDSN(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. + config.ClearEnvForTest(t) + if tt.set { t.Setenv(envKeySentryDSN, tt.value) - } else { - require.NoError(t, os.Unsetenv(envKeySentryDSN)) } got, err := config.EnvSentryDSNForTest(envKeySentryDSN) diff --git a/internal/config/testing.go b/internal/config/testing.go new file mode 100644 index 0000000..fb79178 --- /dev/null +++ b/internal/config/testing.go @@ -0,0 +1,30 @@ +package config + +import ( + "os" + "strings" + "testing" +) + +// ClearEnvForTest unsets every variable in the process environment +// for the rest of the test and puts each back when the test ends, so +// a test sees only the variables it sets itself, not whatever the +// developer's shell exports. +func ClearEnvForTest(t *testing.T) { + t.Helper() + + for _, entry := range os.Environ() { + key, _, _ := strings.Cut(entry, "=") + + // t.Setenv registers the restore; the Unsetenv after it is + // what makes the key absent, since a key set to the empty + // string is still present, and godotenv will not overwrite a + // present key. + t.Setenv(key, "") + + err := os.Unsetenv(key) + if err != nil { + t.Fatalf("unsetting %s: %v", key, err) + } + } +} diff --git a/internal/gormlog/firstboot_test.go b/internal/gormlog/firstboot_test.go index 8a9dd89..c511eb4 100644 --- a/internal/gormlog/firstboot_test.go +++ b/internal/gormlog/firstboot_test.go @@ -117,8 +117,8 @@ func readFirstBootSecrets( } // bootAtDebug starts and stops the real application graph against -// dataDir with DEBUG=true, and returns everything it wrote to standard -// output. +// dataDir with DEBUG=true and nothing else set, and returns everything +// it wrote to standard output. // // config.New reads DEBUG from the environment exactly as the binary // does, internal/logger builds the handler it builds in production, @@ -128,6 +128,7 @@ func readFirstBootSecrets( func bootAtDebug(t *testing.T, dataDir string) string { t.Helper() + config.ClearEnvForTest(t) t.Setenv("DEBUG", "true") t.Setenv("DATA_DIR", dataDir) diff --git a/internal/handlers/settings.go b/internal/handlers/settings.go index a21f878..508a657 100644 --- a/internal/handlers/settings.go +++ b/internal/handlers/settings.go @@ -77,7 +77,8 @@ func settingRows(cfg *config.Config) []settingRow { { "RETENTION_SWEEP_INTERVAL", "How often the retention reaper and archive sweeper run " + - "(Go duration, must be positive)", + "(Go duration, must be positive). A value that does " + + "not parse, or is zero or negative, fails startup", cfg.RetentionSweepInterval.String(), }, {