From 0f3700f7f54041c6c9945fd45d66a4d0ca280e1a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 16:12:57 +0200 Subject: [PATCH] Abort startup on an unknown PIXA_ environment variable (closes #133) A variable whose name starts with PIXA_ but is neither a setting's variable, from the list pairing each config key with its variable, nor PIXA_CONFIG_PATH now aborts startup naming it, as an unknown config key does. PIXA_PORT is named with a pointer to PORT. The check runs after the config file loads, so the variables the file's env section sets are checked too. README.md says so under Configuration. Model: opus-5-5 --- README.md | 6 +- TODO.md | 7 ++ internal/config/config.go | 45 +++++++++++ internal/config/env_internal_test.go | 112 +++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 681a624..7e21964 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,11 @@ process was started with and the file's own key. A variable's value is parsed as the same text in the file would be. The three lists take comma-separated entries, with the spaces around each trimmed; an empty variable is an empty list. A value that does not parse or is invalid aborts -startup, naming the variable. +startup, naming the variable. A variable whose name starts with `PIXA_` but +is not in the table below, such as a misspelled one or `PIXA_PORT`, aborts +startup naming it, as an unknown config key does. The one other accepted +name is `PIXA_CONFIG_PATH`, the config file's path (like `--config`). The +variables set by the file's `env:` section are checked the same way. | Variable | Config key | Meaning | | ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- | diff --git a/TODO.md b/TODO.md index 3e8c7b4..e726796 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ exhaustion # Completed Steps +- 2026-09-28 unknown `PIXA_` environment variables abort startup (closes + #133): a variable whose name starts with `PIXA_` but is neither a + setting's variable nor `PIXA_CONFIG_PATH` aborts startup naming it, as + an unknown config key does, and `PIXA_PORT` is named with a pointer to + `PORT`; the check runs after the config file loads, so the variables + the file's `env:` section sets are checked too; documented in + `README.md`. - 2026-09-28 start on a fresh upaas volume (closes #129): the image starts as root only to give `/var/lib/pixa` to `pixad` when `pixad` does not own it (`deploy/docker-entrypoint.sh`), then runs the server diff --git a/internal/config/config.go b/internal/config/config.go index a2e2037..3d948be 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -58,6 +58,7 @@ var ( errValueRequired = errors.New("a value is required") errValueEmpty = errors.New("value must not be empty") errUnknownConfigKeys = errors.New("unknown config keys") + errUnknownEnvVars = errors.New("unknown environment variables") errNotAString = errors.New("not a string") errNotAnInteger = errors.New("not an integer") errNotABoolean = errors.New("not a boolean") @@ -154,6 +155,13 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { return nil, err } + // Loading the config file sets the variables in its env section, + // so this also checks their names. + err = validateKnownEnvVars() + if err != nil { + return nil, err + } + if sc == nil { log.Info("no config file found, using environment variables and defaults") } @@ -377,6 +385,43 @@ func envVarNames() map[string]string { } } +// validateKnownEnvVars rejects environment variables whose names start +// with PIXA_ but that are neither a setting's variable nor +// PIXA_CONFIG_PATH, so a misspelled variable fails at startup instead of +// being silently ignored, as validateKnownKeys does for config file keys. +// New calls it after loading the config file, so the variables the +// file's env section sets are checked too. +func validateKnownEnvVars() error { + known := map[string]bool{"PIXA_CONFIG_PATH": true} + + for _, name := range envVarNames() { + known[name] = true + } + + var unknown []string + + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + + switch { + case !strings.HasPrefix(name, "PIXA_") || known[name]: + continue + case name == "PIXA_PORT": + unknown = append(unknown, name+" (use PORT for the port)") + default: + unknown = append(unknown, name) + } + } + + if len(unknown) > 0 { + sort.Strings(unknown) + + return fmt.Errorf("%w: %s", errUnknownEnvVars, strings.Join(unknown, ", ")) + } + + return nil +} + // lookupValue returns the value set for key and whether one is set. The // key's environment variable wins when it is present, even when empty; // its value is a string, read exactly as the same text quoted in the diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index ea1e361..57f1f15 100644 --- a/internal/config/env_internal_test.go +++ b/internal/config/env_internal_test.go @@ -3,10 +3,14 @@ package config import ( "net/netip" "os" + "path/filepath" "reflect" "slices" "strings" "testing" + + "sneak.berlin/go/pixa/internal/globals" + "sneak.berlin/go/pixa/internal/logger" ) // TestMain unsets PORT and every PIXA_ environment variable before the @@ -95,6 +99,114 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { } } +// TestUnknownPixaVariableAbortsStartup checks that a PIXA_ variable that +// is not a setting's variable, such as a misspelled one, aborts startup +// naming it, as an unknown config key does, instead of being ignored. +func TestUnknownPixaVariableAbortsStartup(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24") + t.Setenv("PIXA_SIGNINGKEY", validTestSigningKey) + + err := validateKnownEnvVars() + wantStartupError(t, err, "PIXA_TRUSTED_PROXY", "PIXA_SIGNINGKEY") +} + +// TestPixaPortAbortsStartupPointingToPort checks that PIXA_PORT aborts +// startup with a message saying to use PORT, which sets the port. +func TestPixaPortAbortsStartupPointingToPort(t *testing.T) { + t.Setenv("PIXA_PORT", "9090") + + err := validateKnownEnvVars() + wantStartupError(t, err, "PIXA_PORT", "use PORT") +} + +// TestSettingVariablesAndConfigPathAreAccepted checks that every +// setting's variable and PIXA_CONFIG_PATH pass the check for unknown +// PIXA_ variables. TestEnvironmentSetsEveryKey pins the names in the list. +func TestSettingVariablesAndConfigPathAreAccepted(t *testing.T) { + // A config file's env section loaded by another test can leave a + // PIXA_ variable set for the whole process, so every one is unset + // here first; t.Setenv restores each when the test ends. + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + if !strings.HasPrefix(name, "PIXA_") { + continue + } + + t.Setenv(name, "") + + err := os.Unsetenv(name) + if err != nil { + t.Fatalf("failed to unset %s: %v", name, err) + } + } + + t.Setenv("PIXA_CONFIG_PATH", "/etc/pixa/config.yml") + + for _, name := range envVarNames() { + t.Setenv(name, "") + } + + err := validateKnownEnvVars() + if err != nil { + t.Fatalf("PIXA_CONFIG_PATH and every setting's variable "+ + "must be accepted: %v", err) + } +} + +// configFromNew writes yamlContent to a temporary config file, points +// PIXA_CONFIG_PATH at it, and runs New, as the server does at startup. +// The state directory is a temporary one and the disk cache is off, so +// New succeeds unless something in the test is wrong. +func configFromNew(t *testing.T, yamlContent string) (*Config, error) { + t.Helper() + + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yml") + + err := os.WriteFile(configPath, []byte(yamlContent), 0o600) + if err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + t.Setenv("PIXA_CONFIG_PATH", configPath) + t.Setenv("PIXA_STATE_DIR", filepath.Join(tmpDir, "state")) + t.Setenv("PIXA_CACHE_MAX_BYTES", "0") + + testLogger, err := logger.New(nil, logger.Params{Globals: &globals.Globals{}}) + if err != nil { + t.Fatalf("failed to create logger: %v", err) + } + + return New(nil, Params{Logger: testLogger}) +} + +// TestUnknownPixaVariableAbortsNew checks that New, which the server +// calls at startup, aborts on a misspelled PIXA_ variable. +func TestUnknownPixaVariableAbortsNew(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24") + + _, err := configFromNew(t, signingKeyLine) + wantStartupError(t, err, "PIXA_TRUSTED_PROXY") +} + +// TestUnknownPixaVariableInEnvSectionAbortsNew checks that New aborts +// on a misspelled PIXA_ name in the config file's env section, which +// loading the file sets as an environment variable. +func TestUnknownPixaVariableInEnvSectionAbortsNew(t *testing.T) { + // The variable must be absent until the file loads. t.Setenv makes + // sure the one the file sets is removed when the test ends. + t.Setenv("PIXA_TRUSTED_PROXY", "") + + err := os.Unsetenv("PIXA_TRUSTED_PROXY") + if err != nil { + t.Fatalf("failed to unset PIXA_TRUSTED_PROXY: %v", err) + } + + _, err = configFromNew(t, signingKeyLine+ + "env:\n PIXA_TRUSTED_PROXY: 192.0.2.0/24\n") + wantStartupError(t, err, "PIXA_TRUSTED_PROXY") +} + // TestPortFromEnvironmentOverridesConfigFile checks that PORT wins over // the port in the config file. func TestPortFromEnvironmentOverridesConfigFile(t *testing.T) {