From 8aba43da3877a19a918f3fb06f4a890fc820ff42 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 21:33:36 +0000 Subject: [PATCH] Preserve directory traversal failure phases --- docs/roadmap/implementation.md | 8 +- internal/engine/filesystem_access.go | 33 +++- internal/engine/glob.go | 2 +- internal/engine/import_selection.go | 2 +- internal/engine/native_filesystem_test.go | 77 +++++++-- internal/engine/resource_context_test.go | 189 ++++++++++++++++++++++ 6 files changed, 287 insertions(+), 24 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 799f014..1ae58d6 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Audit Remediation Implementation Plan -Status: ready for implementation; all stages pending. +Status: in progress; completion is recorded per stage below. Prepared: 2026-10-25. Planning baseline: `4a5316b7fc2da57dcced33190cb6499c9a1beb5c`. Audience: a Luna coding agent implementing the stages sequentially. @@ -229,6 +229,12 @@ Finding: **C01**, including its integrated test improvements. Owners: `internal/engine/filesystem_access.go`, `import_selection.go`, `glob.go`, `native_filesystem_test.go`, and `resource_context_test.go`. +Status: **complete** (C01). Prior completed commits: `2736ddf`, `deb9aa5`, +`ae3408a`, `ea27f17`, `08a8c25`, `877c497`. Validation passed: +`env GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -run 'Directory|Glob|Filesystem|Resource' -count=1`, +`env GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -count=1`, +and `git diff --check`. + 1. Return private directory failure information distinguishing unsuccessful initial open from enumeration/close failures. Preserve error unwrapping for classification; do not expose backing error text. diff --git a/internal/engine/filesystem_access.go b/internal/engine/filesystem_access.go index 064a593..0319ae3 100644 --- a/internal/engine/filesystem_access.go +++ b/internal/engine/filesystem_access.go @@ -141,40 +141,59 @@ func (a *filesystemAccess) open(name string) (fs.File, error) { return a.root.Open(relative) } +type directoryReadError struct { + opened bool + err error +} + +func (e *directoryReadError) Error() string { return "directory could not be read" } +func (e *directoryReadError) Unwrap() error { return e.err } + +func missingDirectoryOpen(err error) bool { + var failure *directoryReadError + return errors.As(err, &failure) && !failure.opened && errors.Is(err, fs.ErrNotExist) +} + func (a *filesystemAccess) readDirectory(name string, visit func(fs.DirEntry) bool) error { file, err := a.open(name) if err != nil { - return err + return &directoryReadError{err: err} + } + failure := func(err error) error { + if err == nil { + return nil + } + return &directoryReadError{opened: true, err: err} } if reader, ok := file.(fs.ReadDirFile); ok { for { entries, readErr := reader.ReadDir(globReadBatch) for _, entry := range entries { if !visit(entry) { - return file.Close() + return failure(file.Close()) } } if errors.Is(readErr, io.EOF) { - return file.Close() + return failure(file.Close()) } if readErr != nil { _ = file.Close() - return readErr + return failure(readErr) } if len(entries) == 0 { _ = file.Close() - return io.ErrNoProgress + return failure(io.ErrNoProgress) } } } if err := file.Close(); err != nil { - return err + return failure(err) } // fs.ReadDir provides the standard fallback for implementations whose opened // files do not implement fs.ReadDirFile. It may allocate all entries itself. entries, err := fs.ReadDir(a.native, name) if err != nil { - return err + return failure(err) } for _, entry := range entries { if !visit(entry) { diff --git a/internal/engine/glob.go b/internal/engine/glob.go index 1f4e612..e1b8298 100644 --- a/internal/engine/glob.go +++ b/internal/engine/glob.go @@ -191,7 +191,7 @@ func readGlobEntries(access *filesystemAccess, directory string, location Locati if errors.Is(err, errImportDirectoryEntryLimit) { return nil, false, []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "glob traversal exceeds the directory-entry visit limit", &location)} } - if errors.Is(err, fs.ErrNotExist) { + if missingDirectoryOpen(err) { return nil, true, nil } if err != nil { diff --git a/internal/engine/import_selection.go b/internal/engine/import_selection.go index 45ed589..8faf30d 100644 --- a/internal/engine/import_selection.go +++ b/internal/engine/import_selection.go @@ -299,7 +299,7 @@ func importDirectoryEntries(access *filesystemAccess, directory string, site imp if errors.Is(err, errImportDirectoryEntryLimit) { return nil, false, importSelectionDiagnostic(site, CodeImportLimitExceeded, "directory search exceeds the directory-entry visit limit") } - if errors.Is(err, fs.ErrNotExist) { + if missingDirectoryOpen(err) { return nil, true, nil } if err != nil { diff --git a/internal/engine/native_filesystem_test.go b/internal/engine/native_filesystem_test.go index 7ff839f..a76c929 100644 --- a/internal/engine/native_filesystem_test.go +++ b/internal/engine/native_filesystem_test.go @@ -246,22 +246,36 @@ func TestNativeFilesystemDetectsRepeatedAndCyclicImports(t *testing.T) { } type observedFilesystem struct { - files fstest.MapFS - opened int - closed int - fallback bool - failRead string - failStat string - failDir string + files fstest.MapFS + opened int + closed int + fallback bool + failOpen string + failRead string + failStat string + failDir string + failReadDir string + readDirErr error + failNoProgress string + failClose string + closeErr error + failFallbackDir string + fallbackErr error + fallbackBeforeClose bool + handles []*observedFile } func (f *observedFilesystem) Open(name string) (fs.File, error) { + if name == f.failOpen { + return nil, fs.ErrNotExist + } inner, err := f.files.Open(name) if err != nil { return nil, err } f.opened++ file := &observedFile{File: inner, owner: f, name: name} + f.handles = append(f.handles, file) if directory, ok := inner.(fs.ReadDirFile); ok && !f.fallback { return &observedDirectoryFile{observedFile: file, directory: directory}, nil } @@ -272,20 +286,36 @@ func (f *observedFilesystem) ReadDir(name string) ([]fs.DirEntry, error) { if name == f.failDir { return nil, errors.New("tracking failure canary") } - return fs.ReadDir(f.files, name) + entries, err := fs.ReadDir(f.files, name) + if name == f.failFallbackDir { + for _, handle := range f.handles { + if handle.name == name && !handle.closed { + f.fallbackBeforeClose = true + } + } + return entries, f.fallbackErr + } + return entries, err } func (f *observedFilesystem) assertClosed(t *testing.T) { t.Helper() - if f.opened == 0 || f.opened != f.closed { - t.Fatalf("filesystem opens=%d closes=%d", f.opened, f.closed) + if f.opened != f.closed || len(f.handles) != f.opened { + t.Fatalf("filesystem opens=%d closes=%d handles=%d", f.opened, f.closed, len(f.handles)) + } + for _, handle := range f.handles { + if !handle.closed || handle.closeCalls != 1 { + t.Errorf("filesystem handle %q closed=%t close calls=%d", handle.name, handle.closed, handle.closeCalls) + } } } type observedFile struct { fs.File - owner *observedFilesystem - name string + owner *observedFilesystem + name string + closed bool + closeCalls int } func (f *observedFile) Read(data []byte) (int, error) { @@ -304,7 +334,16 @@ func (f *observedFile) Stat() (fs.FileInfo, error) { func (f *observedFile) Close() error { f.owner.closed++ - return f.File.Close() + f.closeCalls++ + f.closed = true + err := f.File.Close() + if f.name == f.owner.failClose { + if f.owner.closeErr != nil { + return f.owner.closeErr + } + return fs.ErrNotExist + } + return err } type observedDirectoryFile struct { @@ -316,5 +355,15 @@ func (f *observedDirectoryFile) ReadDir(count int) ([]fs.DirEntry, error) { if f.name == f.owner.failDir { return nil, errors.New("tracking failure canary") } - return f.directory.ReadDir(count) + if f.name == f.owner.failNoProgress { + return nil, nil + } + entries, err := f.directory.ReadDir(count) + if f.name == f.owner.failReadDir { + if f.owner.readDirErr != nil { + return entries, f.owner.readDirErr + } + return entries, fs.ErrNotExist + } + return entries, err } diff --git a/internal/engine/resource_context_test.go b/internal/engine/resource_context_test.go index 4834f9a..7ac7460 100644 --- a/internal/engine/resource_context_test.go +++ b/internal/engine/resource_context_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "testing/fstest" ) @@ -181,3 +182,191 @@ func TestLoadingClosesNativeFilesAcrossPipelineOutcomes(t *testing.T) { }) } } + +func TestDirectorySelectorsFailOnPostOpenNotExistErrors(t *testing.T) { + const canary = "DIRECTORY_FAILURE_CANARY" + readFailure := errors.Join(fs.ErrNotExist, errors.New(canary)) + closeFailure := errors.Join(fs.ErrNotExist, errors.New(canary)) + for _, test := range []struct { + name string + selector string + fallback bool + readError bool + noProgress bool + closeError bool + fallbackErr bool + }{ + {name: "directory batched read with entries", selector: "directory", readError: true}, + {name: "directory close", selector: "directory", closeError: true}, + {name: "directory fallback with entries", selector: "directory", fallback: true, fallbackErr: true}, + {name: "directory no progress", selector: "directory", noProgress: true}, + {name: "glob batched read with entries", selector: "glob", readError: true}, + {name: "glob close", selector: "glob", closeError: true}, + {name: "glob fallback with entries", selector: "glob", fallback: true, fallbackErr: true}, + {name: "glob no progress", selector: "glob", noProgress: true}, + } { + t.Run(test.name, func(t *testing.T) { + main := "configkit:\n imports:\n" + if test.selector == "directory" { + main += " - dir: [broken, good]\n pattern: '*.yml'\n" + } else { + main += " - glob: 'broken/*.yml'\n - glob: 'good/*.yml'\n" + } + filesystem := &observedFilesystem{ + files: fstest.MapFS{ + "main.yml": &fstest.MapFile{Data: []byte(main)}, + "broken": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "broken/bad.yml": &fstest.MapFile{Data: []byte("value: bad\n")}, + "good": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "good/good.yml": &fstest.MapFile{Data: []byte("value: good\n")}, + }, + fallback: test.fallback, + failReadDir: "broken", + readDirErr: readFailure, + failNoProgress: "broken", + failClose: "broken", + closeErr: closeFailure, + failFallbackDir: "broken", + fallbackErr: readFailure, + } + if !test.readError { + filesystem.failReadDir = "" + } + if !test.noProgress { + filesystem.failNoProgress = "" + } + if !test.closeError { + filesystem.failClose = "" + } + if !test.fallbackErr { + filesystem.failFallbackDir = "" + } + + result, err := testLoad[map[string]any](filesystemFile(newNativeNamespace("bundle", filesystem), "main.yml"), Options[map[string]any]{Imports: true}) + if result != nil { + t.Fatalf("load returned a result after directory failure: %#v", result) + } + var loadError *LoadError + if !errors.As(err, &loadError) { + t.Fatalf("load error = %#v, want structured LoadError", err) + } + diagnostics := loadError.Diagnostics() + if len(diagnostics) != 1 { + t.Fatalf("diagnostics = %#v, want one directory failure", diagnostics) + } + diagnostic := diagnostics[0] + wantCode := CodeImportInvalid + if test.selector == "glob" { + wantCode = CodeImportRead + } + if diagnostic.Code != wantCode || diagnostic.Primary == nil || diagnostic.Primary.SourceID != "source-1" { + t.Fatalf("directory diagnostic = %#v, want %q at source-1", diagnostic, wantCode) + } + var rendered strings.Builder + if err := WriteDiagnostics(&rendered, diagnostics); err != nil { + t.Fatal(err) + } + if strings.Contains(err.Error()+diagnostic.Message+rendered.String(), canary) { + t.Fatalf("backing error canary escaped: error=%q diagnostic=%#v rendered=%q", err, diagnostic, rendered.String()) + } + if filesystem.fallbackBeforeClose { + t.Fatal("fallback directory enumeration began before the opened handle was closed") + } + filesystem.assertClosed(t) + }) + } +} + +func TestReadDirectoryPreservesReadErrorOverCloseError(t *testing.T) { + readFailure := errors.New("read failure") + closeFailure := errors.New("close failure") + filesystem := &observedFilesystem{ + files: fstest.MapFS{ + "broken": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "broken/entry": &fstest.MapFile{Data: []byte("entry")}, + }, + failReadDir: "broken", + readDirErr: readFailure, + failClose: "broken", + closeErr: closeFailure, + } + access := newNativeAccess(filesystem, ".", &namespaceToken{}) + visited := 0 + err := access.readDirectory("broken", func(fs.DirEntry) bool { + visited++ + return true + }) + if !errors.Is(err, readFailure) || errors.Is(err, closeFailure) || visited != 1 { + t.Fatalf("directory read = %v, visited %d; want entries plus primary read failure", err, visited) + } + if missingDirectoryOpen(err) { + t.Fatal("post-open read failure was classified as missing-on-open") + } + filesystem.assertClosed(t) +} + +func TestReadDirectoryVisitStopCloseNotExistIsPostOpen(t *testing.T) { + filesystem := &observedFilesystem{ + files: fstest.MapFS{ + "broken": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "broken/entry": &fstest.MapFile{Data: []byte("entry")}, + }, + failClose: "broken", + } + access := newNativeAccess(filesystem, ".", &namespaceToken{}) + err := access.readDirectory("broken", func(fs.DirEntry) bool { return false }) + if !errors.Is(err, fs.ErrNotExist) || missingDirectoryOpen(err) { + t.Fatalf("visit-stop close error = %v, missing-on-open=%t", err, missingDirectoryOpen(err)) + } + filesystem.assertClosed(t) +} + +func TestDirectorySelectorsKeepMissingRootsOptional(t *testing.T) { + for _, selector := range []string{"directory", "glob"} { + t.Run(selector, func(t *testing.T) { + main := "configkit:\n imports:\n" + if selector == "directory" { + main += " - dir: [missing, good]\n pattern: '*.yml'\n required: true\n" + } else { + main += " - glob: 'missing/*.yml'\n - glob: 'good/*.yml'\n required: true\n" + } + filesystem := &observedFilesystem{files: fstest.MapFS{ + "main.yml": &fstest.MapFile{Data: []byte(main)}, + "good": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "good/good.yml": &fstest.MapFile{Data: []byte("value: good\n")}, + }} + result, err := testLoad[map[string]any](filesystemFile(newNativeNamespace("bundle", filesystem), "main.yml"), Options[map[string]any]{Imports: true}) + if err != nil || result == nil { + t.Fatalf("load with missing search root = %#v, %v", result, err) + } + filesystem.assertClosed(t) + }) + } +} + +func TestDirectorySelectorsTreatMissingOnOpenAsOptional(t *testing.T) { + for _, selector := range []string{"directory", "glob"} { + t.Run(selector, func(t *testing.T) { + main := "configkit:\n imports:\n" + if selector == "directory" { + main += " - dir: [broken, good]\n pattern: '*.yml'\n required: true\n" + } else { + main += " - glob: 'broken/*.yml'\n - glob: 'good/*.yml'\n required: true\n" + } + filesystem := &observedFilesystem{ + files: fstest.MapFS{ + "main.yml": &fstest.MapFile{Data: []byte(main)}, + "broken": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "good": &fstest.MapFile{Mode: fs.ModeDir | 0o700}, + "good/good.yml": &fstest.MapFile{Data: []byte("value: good\n")}, + }, + failOpen: "broken", + } + result, err := testLoad[map[string]any](filesystemFile(newNativeNamespace("bundle", filesystem), "main.yml"), Options[map[string]any]{Imports: true}) + if err != nil || result == nil { + t.Fatalf("load after missing-on-open root = %#v, %v", result, err) + } + filesystem.assertClosed(t) + }) + } +}