From 3bcb2cd6e5fe3c21699e708bf1270af3100165dd Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 13:45:15 +0000 Subject: [PATCH 1/2] test: an unknown PIXA_ environment variable aborts startup (closes #133) Tests, written before the change, for the check of PIXA_ names at startup: a PIXA_ variable that is not a setting's variable aborts naming it, PIXA_PORT aborts with a message saying to use PORT, and PIXA_CONFIG_PATH and every setting's variable are accepted. Two tests run New, as the server does at startup: one with a misspelled variable in the environment, one with it in the config file's env section. They call validateKnownEnvVars, which the change adds, so the package does not build until it lands. Model: opus-5-5 --- internal/config/env_internal_test.go | 112 +++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) 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) { -- 2.54.0 From 09c627bf8b8202ff1061d3c0bbdbd8e9ba54cdd9 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 13:45:15 +0000 Subject: [PATCH 2/2] 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 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 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 -- 2.54.0