From 9c94f2b70a68814608f098cfebb37c48694eb3cf Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 6 Sep 2026 19:04:04 -0500 Subject: [PATCH] Reject nonregular configuration files before opening them --- docs/config.md | 4 ++- docs/roadmap/remediation.md | 5 +++ imports.go | 12 +++++++ profile.go | 9 +++++ source.go | 20 +++++++---- source_fifo_test.go | 71 +++++++++++++++++++++++++++++++++++++ 6 files changed, 113 insertions(+), 8 deletions(-) create mode 100644 source_fifo_test.go diff --git a/docs/config.md b/docs/config.md index c979d25..c34f707 100644 --- a/docs/config.md +++ b/docs/config.md @@ -490,7 +490,9 @@ source may supply `FilesystemContext.Root` to widen that root but may not supply `BaseDir`. Whenever byte and reader sources supply filesystem context, they must provide both `BaseDir` and `Root` as absolute existing directories; their diagnostic labels, including URI-looking labels, never imply filesystem paths. The base and -selected file must be inside the canonical root. ConfigKit canonicalizes symlinks, +selected file must be inside the canonical root. Nonregular targets are rejected before opening to avoid blocking on FIFOs; +post-open regular-file checks remain in place under the stable-input assumption. +ConfigKit canonicalizes symlinks, checks containment with relative paths, and opens the canonical root-relative target through a library-owned root handle. Contained relative or absolute symlink targets are accepted; escapes are rejected. Canonical paths define identity, so hard links at diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index b220c99..fab42ae 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -50,6 +50,11 @@ references must remain prohibited inside dynamic values. ## 2. P1 — Reject nonregular source files before a blocking open +Status: complete. Selected files, rooted imports and profiles now inspect canonical +targets before opening and retain post-open checks. Deadline-controlled FIFO +subprocess tests cover all three paths. Full tests pass with the issue-8 TMPDIR +workaround; vet, build and whitespace checks pass. + Evidence: [source.go](../../source.go), `readSource`, calls `os.Open` before checking regular-file status. [imports.go](../../imports.go), `readImport`, and [profile.go](../../profile.go), `readProfileFile`, likewise open before checking. diff --git a/imports.go b/imports.go index a128f47..89a06e9 100644 --- a/imports.go +++ b/imports.go @@ -382,6 +382,18 @@ func (s *importTraversal) readImport(relative, sourceID string, optional bool) ( if limit < 0 { limit = 0 } + preflight, err := s.root.Stat(relative) + if err != nil { + if optional && errors.Is(err, fs.ErrNotExist) { + return nil, true, nil + } + value := classifySourceError(err, sourceID) + return nil, false, &value + } + if !preflight.Mode().IsRegular() { + value := sourceDiagnostic(CodeSourceNotRegular, sourceID, "source is not a regular file") + return nil, false, &value + } file, err := s.root.Open(relative) if err != nil { if optional && errors.Is(err, fs.ErrNotExist) { diff --git a/profile.go b/profile.go index 567a962..3c3768e 100644 --- a/profile.go +++ b/profile.go @@ -122,6 +122,15 @@ func readProfileFile(root *os.Root, relative, sourceID string, remaining int64) if limit < 0 { limit = 0 } + preflight, err := root.Stat(relative) + if err != nil { + value := classifySourceError(err, sourceID) + return nil, &value + } + if !preflight.Mode().IsRegular() { + value := sourceDiagnostic(CodeSourceNotRegular, sourceID, "source is not a regular file") + return nil, &value + } file, err := root.Open(relative) if err != nil { value := classifySourceError(err, sourceID) diff --git a/source.go b/source.go index 29d43fc..8b0eecc 100644 --- a/source.go +++ b/source.go @@ -136,7 +136,19 @@ func readSource(source Source, sourceID string, limits sourceLimits) (sourceRead return sourceRead{info: info}, []Diagnostic{sourceDiagnostic(CodeSourceRead, sourceID, "source path could not be resolved")} } info.Path = filepath.Clean(absolute) - file, err := os.Open(source.path) + canonical, err := filepath.EvalSymlinks(info.Path) + if err != nil { + return sourceRead{info: info}, []Diagnostic{classifySourceError(err, sourceID)} + } + info.Path = canonical + preflight, err := os.Stat(canonical) + if err != nil { + return sourceRead{info: info}, []Diagnostic{classifySourceError(err, sourceID)} + } + if !preflight.Mode().IsRegular() { + return sourceRead{info: info}, []Diagnostic{sourceDiagnostic(CodeSourceNotRegular, sourceID, "source is not a regular file")} + } + file, err := os.Open(canonical) if err != nil { return sourceRead{info: info}, []Diagnostic{classifySourceError(err, sourceID)} } @@ -150,12 +162,6 @@ func readSource(source Source, sourceID string, limits sourceLimits) (sourceRead _ = file.Close() return sourceRead{info: info}, []Diagnostic{sourceDiagnostic(CodeSourceNotRegular, sourceID, "source is not a regular file")} } - canonical, err := filepath.EvalSymlinks(info.Path) - if err != nil { - _ = file.Close() - return sourceRead{info: info}, []Diagnostic{classifySourceError(err, sourceID)} - } - info.Path = canonical if stat.Size() > limit { _ = file.Close() return sourceRead{info: info}, []Diagnostic{sourceDiagnostic(CodeSourceLimitExceeded, sourceID, "source exceeds the configured byte limit")} diff --git a/source_fifo_test.go b/source_fifo_test.go new file mode 100644 index 0000000..ca6464d --- /dev/null +++ b/source_fifo_test.go @@ -0,0 +1,71 @@ +//go:build darwin || linux + +package configkit + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "syscall" + "testing" + "time" +) + +func TestNonregularSourcesDoNotBlock(t *testing.T) { + if mode := os.Getenv("CONFIGKIT_FIFO_TEST_MODE"); mode != "" { + root := os.Getenv("CONFIGKIT_FIFO_TEST_ROOT") + source := File(filepath.Join(root, "config.yml")) + opts := Options[map[string]any]{} + switch mode { + case "file": + source = File(filepath.Join(root, "pipe.yml")) + case "import": + opts.Imports = true + case "profile": + opts.Profile = "fifo" + } + _, err := Load(source, opts) + var loadErr *LoadError + if !errors.As(err, &loadErr) { + t.Fatalf("expected safe load error, got %v", err) + } + for _, d := range loadErr.Diagnostics() { + if d.Code == CodeSourceNotRegular { + return + } + } + t.Fatalf("expected nonregular classification, got %v", loadErr.Diagnostics()) + } + for _, mode := range []string{"file", "import", "profile"} { + t.Run(mode, func(t *testing.T) { + root := t.TempDir() + input := "value: 1\n" + if mode == "import" { + input = "_configkit:\n imports:\n - path: pipe.yml\n" + } + if err := os.WriteFile(filepath.Join(root, "config.yml"), []byte(input), 0600); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(root, "profiles"), 0700); err != nil { + t.Fatal(err) + } + fifo := filepath.Join(root, "pipe.yml") + if mode == "profile" { + fifo = filepath.Join(root, "profiles", "fifo.yml") + } + if err := syscall.Mkfifo(fifo, 0600); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestNonregularSourcesDoNotBlock$") + cmd.Env = append(os.Environ(), "CONFIGKIT_FIFO_TEST_MODE="+mode, "CONFIGKIT_FIFO_TEST_ROOT="+root) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("FIFO load failed or blocked: %v, %s", err, out) + } + }) + } +}