From 48f21d4ecf1d0a2b300444a6742600ca5c211205 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 18:24:43 +0200 Subject: [PATCH] Abort startup on a config file pixa cannot read (closes #176) Of the places pixa looks for its config file on its own, it passed over any place where os.Stat failed, so a file in a directory pixa may not enter was skipped without a word and pixa started on a later file or on the environment and defaults. Now only a path that does not exist, or that runs through a file (such as under a HOME of /dev/null), is passed over; any other error aborts startup naming the file, as a file that does not parse already did. README.md says so where it gives the search order. A config.yml that links to itself tests this as root too. Model: opus-5-5 --- README.md | 8 +- TODO.md | 5 + internal/config/config.go | 32 +++-- .../config/config_validation_internal_test.go | 110 ++++++++++++++++++ 4 files changed, 140 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 54de083..6e3e2d1 100644 --- a/README.md +++ b/README.md @@ -413,10 +413,10 @@ pixa finds: `/etc/pixa/config.yml`, `/etc/pixa/config.yaml`, `~/.config/pixa/config.yml`, `~/.config/pixa/config.yaml`, then `config.yml` and `config.yaml` in the working directory. A named file that does not exist, cannot be read or does not parse aborts startup. Of the files pixa looks for on -its own, one it finds but cannot read or parse aborts startup; one it cannot -find, for any reason, is passed over without a message, even when the file is -there in a directory pixa may not enter. With no file, pixa uses the environment -and the defaults. +its own, only one that does not exist is passed over, without a message. One +that pixa cannot read or parse aborts startup, naming the file. So does one in a +directory pixa may not enter, whether or not it is there, since pixa cannot +tell. With no file, pixa uses the environment and the defaults. | Variable | Config key | Meaning | | ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- | diff --git a/TODO.md b/TODO.md index 8a0673a..84e896f 100644 --- a/TODO.md +++ b/TODO.md @@ -48,6 +48,11 @@ P2: security: referer blacklist files and the eviction pass after it evicts them. No other test in `internal/imgcache` inserts a row by hand after starting the evictor. Test only. +- 2026-10-04 a config file pixa cannot read aborts startup (closes #176): of the + places pixa looks for its config file on its own, only one where the file does + not exist is passed over; any other error, such as a directory on the path + that pixa may not enter, aborts startup naming the file, as a file that does + not parse already did. - 2026-10-04 deployment guide and example Caddy config (closes #89): "Deployment" in `README.md` says what the reverse proxy in front of pixa must do (terminate TLS; pass `Host`, `Origin` and `Referer` on unchanged; set diff --git a/internal/config/config.go b/internal/config/config.go index b8e041b..a9e583f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "errors" "fmt" + "io/fs" "log/slog" "math" "net/netip" @@ -14,6 +15,7 @@ import ( "sort" "strconv" "strings" + "syscall" "time" "git.eeqj.de/sneak/smartconfig" @@ -778,19 +780,27 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro for _, path := range configPaths { cleanPath := filepath.Clean(path) + // Only a config file that does not exist is skipped, including + // one whose path runs through a file, such as under a HOME of + // /dev/null. One that cannot be read or does not parse is a + // fatal startup error. _, statErr := os.Stat(cleanPath) - if statErr == nil { - // A config file that exists but does not parse is a fatal - // startup error, never something to skip over. - sc, err := smartconfig.NewFromConfigPath(path) - if err != nil { - return nil, fmt.Errorf("failed to parse config file %s: %w", path, err) - } - - log.Info("loaded config file", "path", path) - - return sc, nil + if errors.Is(statErr, fs.ErrNotExist) || errors.Is(statErr, syscall.ENOTDIR) { + continue } + + if statErr != nil { + return nil, fmt.Errorf("failed to read config file %s: %w", path, statErr) + } + + sc, err := smartconfig.NewFromConfigPath(path) + if err != nil { + return nil, fmt.Errorf("failed to parse config file %s: %w", path, err) + } + + log.Info("loaded config file", "path", path) + + return sc, nil } return nil, nil //nolint:nilnil // nil config is valid (use defaults) diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index c269a38..1800ab3 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -564,6 +564,116 @@ func TestMalformedConfigFileAbortsStartup(t *testing.T) { t.Logf("got expected error: %v", err) } +// TestConfigFileInDirectoryPixaMayNotEnterAbortsStartup checks that a +// config file pixa cannot read because it may not enter its directory +// aborts startup instead of being passed over. +func TestConfigFileInDirectoryPixaMayNotEnterAbortsStartup(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root may enter any directory") + } + + home := t.TempDir() + configDir := filepath.Join(home, ".config", "pixa-test-nonexistent-app") + configPath := filepath.Join(configDir, "config.yml") + + err := os.MkdirAll(configDir, 0o700) + if err != nil { + t.Fatalf("failed to create config directory: %v", err) + } + + err = os.WriteFile(configPath, []byte(signingKeyLine), 0o600) + if err != nil { + t.Fatalf("failed to write config: %v", err) + } + + err = os.Chmod(configDir, 0) + if err != nil { + t.Fatalf("failed to remove the config directory's permissions: %v", err) + } + + // Give the directory back its permissions so t.TempDir can remove it. + t.Cleanup(func() { + //nolint:gosec // G302: a directory needs its execute bit to be removed + _ = os.Chmod(configDir, 0o700) + }) + + // The ~/.config candidate is the only one that exists: the appname + // rules out /etc, and the working directory is empty. + t.Setenv("PIXA_CONFIG_PATH", "") + t.Setenv("HOME", home) + t.Chdir(t.TempDir()) + + log := slog.New(slog.DiscardHandler) + + sc, err := loadConfigFile(log, "pixa-test-nonexistent-app") + if err == nil { + t.Fatalf("config file pixa cannot read must abort startup, got config: %v", + sc) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), configPath) { + t.Errorf("error %q does not name the config file %s", err.Error(), configPath) + } +} + +// TestConfigFileLinkingToItselfAbortsStartup checks that a config file +// pixa cannot read for a reason other than not existing aborts startup, +// as root too: a symbolic link to itself fails with "too many levels of +// symbolic links". +func TestConfigFileLinkingToItselfAbortsStartup(t *testing.T) { + workDir := t.TempDir() + + err := os.Symlink("config.yml", filepath.Join(workDir, "config.yml")) + if err != nil { + t.Fatalf("failed to create symbolic link: %v", err) + } + + // Only the working directory's config.yml is there: the appname rules + // out /etc, and HOME is empty. + t.Setenv("PIXA_CONFIG_PATH", "") + t.Setenv("HOME", t.TempDir()) + t.Chdir(workDir) + + log := slog.New(slog.DiscardHandler) + + sc, err := loadConfigFile(log, "pixa-test-nonexistent-app") + if err == nil { + t.Fatalf("config file pixa cannot read must abort startup, got config: %v", + sc) + } + + t.Logf("got expected error: %v", err) + + if !strings.Contains(err.Error(), "config.yml") { + t.Errorf("error %q does not name the config file config.yml", err.Error()) + } +} + +// TestConfigPathThroughFileIsPassedOver checks that a config file path +// that runs through a file, such as one under a HOME of /dev/null, is +// passed over like one that does not exist, since no file can be there. +func TestConfigPathThroughFileIsPassedOver(t *testing.T) { + // No config file is there: the appname rules out /etc, HOME is + // /dev/null, and the working directory is empty. + t.Setenv("PIXA_CONFIG_PATH", "") + t.Setenv("HOME", os.DevNull) + t.Chdir(t.TempDir()) + + log := slog.New(slog.DiscardHandler) + + sc, err := loadConfigFile(log, "pixa-test-nonexistent-app") + if err != nil { + t.Fatalf("a config path through a file must be passed over, got error: %v", + err) + } + + if sc != nil { + t.Errorf("expected no config file, got config: %v", sc) + } +} + func TestEnsureStateDirCreatesDirectory(t *testing.T) { t.Parallel()