Isolate config tests from the shell; any out-of-range PORT is ErrInvalidPort (closes #94)

Config tests that unset a variable now restore it when they end, and
every test that builds a Config first unsets every variable the config
reads, so a value exported in the developer's shell cannot change the
result. TestEnvPort checks that the bad value appears in its errors.

A PORT of zero or below now wraps ErrInvalidPort, as one above 65535
already did.

The README configuration table and the Settings page now say that a
RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative,
fails startup.

Model: opus-5-5

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) |

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
+	// maxPort is the highest valid TCP port number.
-	// bound (at least 1) is enforced by envPositiveInt.
 	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,25 @@ 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; the two out-of-range cases both
+// wrap ErrInvalidPort.
 func envPort(key string, defaultValue int) (int, error) {
-	port, err := envPositiveInt(key, defaultValue)
+	v := os.Getenv(key)
-	if err != nil {
+	if v == "" {
-		return 0, err
+		return defaultValue, nil
 	}
 
-	if port > maxPort {
+	port, err := strconv.Atoi(v)
+	if err != nil {
 		return 0, fmt.Errorf(
-			"%w: %s must be at most %d, got %d",
+			"invalid integer for %s: %q: %w", key, v, err,
-			ErrInvalidPort, key, maxPort, port,
+		)
+	}
+
+	if port < 1 || port > maxPort {
+		return 0, fmt.Errorf(
+			"%w: %s must be from 1 to %d, got %q",
+			ErrInvalidPort, key, maxPort, v,
 		)
 	}
 

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.
+			unsetEnv(t, configEnvKeys()...)
+
 			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.
+			unsetEnv(t, configEnvKeys()...)
 			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.
+			unsetEnv(t, configEnvKeys()...)
 			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.
+			unsetEnv(t, configEnvKeys()...)
+
 			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(
@@ -447,7 +434,7 @@ func TestDataDirHelper(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"))
+				unsetEnv(t, "DATA_DIR")
 			} else {
 				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.
+			unsetEnv(t, configEnvKeys()...)
 			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.
+			unsetEnv(t, configEnvKeys()...)
 			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.
+			unsetEnv(t, configEnvKeys()...)
 			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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev)
 
-			if tt.allowed == "" {
+			if tt.allowed != "" {
-				require.NoError(
-					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
-				)
-			} else {
 				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.
+			unsetEnv(t, configEnvKeys()...)
+
 			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 {

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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_AppliesValues(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	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)
+	unsetEnv(t, dotEnvKey)
 
 	dir := t.TempDir()
 	require.NoError(t, os.WriteFile(

internal/config/env_test.go

@@ -38,6 +38,41 @@ const (
 	bindAddressSample = "10.1.2.3"
 )
 
+// configEnvKeys is every variable config.New reads. Its tests unset
+// all of them before setting the ones under test, so a variable
+// exported in the developer's shell cannot change their outcome.
+func configEnvKeys() []string {
+	return []string{
+		"WEBHOOKER_ENVIRONMENT",
+		envKeyPort,
+		envKeyBindAddress,
+		"DATA_DIR",
+		envKeyDebug,
+		"METRICS_USERNAME",
+		"METRICS_PASSWORD",
+		envKeySentryDSN,
+		"RETENTION_SWEEP_INTERVAL",
+		"SESSION_IDLE_TIMEOUT",
+		"RECEIVER_RATE_LIMIT",
+		"TRUSTED_PROXIES",
+		"ALLOWED_EGRESS_CIDRS",
+	}
+}
+
+// unsetEnv makes each key absent for the rest of the test and puts
+// back whatever it held when the test ends. t.Setenv registers that
+// 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.
+func unsetEnv(t *testing.T, keys ...string) {
+	t.Helper()
+
+	for _, key := range keys {
+		t.Setenv(key, "")
+		require.NoError(t, os.Unsetenv(key))
+	}
+}
+
 // envBoolCase is one row of the envBool table.
 type envBoolCase struct {
 	name         string
@@ -124,7 +159,7 @@ func TestEnvBool(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -194,6 +229,7 @@ func TestEnvPositiveInt(t *testing.T) {
 		},
 	}
 
+	//nolint:dupl // TestEnvPort makes the same checks, on purpose.
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
@@ -201,7 +237,7 @@ func TestEnvPositiveInt(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvPositiveIntForTest(
@@ -264,7 +300,7 @@ func TestEnvPort(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
-			errIs:       config.ErrNonPositiveValue,
+			errIs:       config.ErrInvalidPort,
 		},
 		{
 			name:        "above the port range is rejected",
@@ -275,6 +311,7 @@ func TestEnvPort(t *testing.T) {
 		},
 	}
 
+	//nolint:dupl // TestEnvPositiveInt makes the same checks, on purpose.
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
@@ -282,7 +319,7 @@ func TestEnvPort(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvPortForTest(
@@ -292,6 +329,7 @@ func TestEnvPort(t *testing.T) {
 			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)
@@ -322,7 +360,7 @@ func TestEnvBindAddress(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -485,6 +523,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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -646,14 +685,9 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
+	unsetEnv(t, configEnvKeys()...)
 	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)

internal/config/sentry_test.go

@@ -1,7 +1,6 @@
 package config_test
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -104,7 +103,7 @@ func TestEnvSentryDSN(t *testing.T) {
 			if tt.set {
 				t.Setenv(envKeySentryDSN, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(envKeySentryDSN))
+				unsetEnv(t, envKeySentryDSN)
 			}
 
 			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)

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(),
 		},
 		{