From 7f8ad81065f879d2edf484c4222b3853d8f26486 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 20 Oct 2026 00:26:30 +0000 Subject: [PATCH] Track active symlink expansions precisely --- docs/roadmap/remediation.md | 11 +++++- facade_import_root_test.go | 29 +++++++++++++++ internal/engine/filesystem_links.go | 41 +++++++++++++++------ internal/engine/filesystem_links_test.go | 47 ++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 14 deletions(-) create mode 100644 internal/engine/filesystem_links_test.go diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index e8ca5aa..76f2f11 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Import Discovery and Scheduling Review Findings -Status: in progress; issues 1–3 complete, three findings remain. +Status: in progress; issues 1–4 complete, two findings remain. Reviewed implementation: `aa93aed..15943e2`, ending at `15943e233ca3cd1286c03c0e7085871db344cd4b`, against the accepted @@ -120,7 +120,7 @@ undergo their existing containment checks. Public directory/glob regressions cover ordinary and aliased roots plus escaping links. All four valid cases failed before the fix. The full everyday check passed. -## 4. [ ] P2 — Distinguish finite symlink reuse from actual cycles +## 4. [x] P2 — Distinguish finite symlink reuse from actual cycles Owners: [resolveHostImportPath and resolveNativeImportPath](../../internal/engine/filesystem_links.go), especially the `activeLinks` sets around lines 115–134 and 172–194. @@ -143,6 +143,13 @@ separate active-path directory-cycle rule. Add finite-reuse and real-loop regressions for host and link-aware native access; do not replace this defect with an arbitrary new link or path-depth restriction. +Completed: host and native resolvers now release active links when each target +expansion completes, using explicit frames rather than retaining a whole-path +visited set. Added resolver regressions for finite reuse, self loops, and mutual +loops, plus public host/native directory/glob loading coverage. Finite cases +failed before the change; all pass now alongside existing traversal-cycle and +containment tests. The full everyday check passed; no new limit was added. + ## 5. [ ] P2 — Validate the complete depth token before saturating its value Owner: [parseImportDepth](../../internal/engine/import_spec.go), around lines diff --git a/facade_import_root_test.go b/facade_import_root_test.go index c9113a0..c605c5a 100644 --- a/facade_import_root_test.go +++ b/facade_import_root_test.go @@ -50,3 +50,32 @@ func TestFacadeNativeDiscoveryWithNestedRoot(t *testing.T) { } } } + +func TestFacadeDiscoveryAllowsFiniteLinkReuse(t *testing.T) { + root := t.TempDir() + if err := os.Mkdir(filepath.Join(root, "parts"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "parts/value.yml"), []byte("value: 1\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink("..", filepath.Join(root, "parts/up")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + filesystem := configkit.NewFilesystem("finite", os.DirFS(root)) + for _, native := range []bool{false, true} { + for _, selector := range []string{"dir: parts/up/parts/up/parts\n pattern: '*.yml'", "glob: 'parts/up/parts/up/parts/*.yml'"} { + context := &configkit.FilesystemContext{Root: root, BaseDir: root} + if native { + context = &configkit.FilesystemContext{FS: &filesystem, Root: ".", BaseDir: "."} + } + result, err := configkit.Load(configkit.Bytes("main", []byte("configkit:\n imports:\n - "+selector+"\n")), configkit.Options[map[string]any]{Imports: true, Filesystem: context}) + if err != nil { + t.Fatalf("native=%v selector=%s: %v", native, selector, err) + } + if result.Config["value"] != int64(1) { + t.Fatalf("configuration = %#v", result.Config) + } + } + } +} diff --git a/internal/engine/filesystem_links.go b/internal/engine/filesystem_links.go index 25ff005..05aa712 100644 --- a/internal/engine/filesystem_links.go +++ b/internal/engine/filesystem_links.go @@ -109,13 +109,26 @@ func (a *filesystemAccess) importEntryTarget(name string, entry fs.DirEntry, fol return importEntryTarget{symlink: symlink, dir: info.IsDir(), identity: name}, nil } +// A link stays active until its target has resolved, not until the entire +// original pathname has resolved. Remaining segments may reuse a finished link. +type importLinkFrame struct { + pending []string + link string +} + func resolveHostImportPath(access *filesystemAccess, name string) (string, error) { - pending := strings.Split(filepath.ToSlash(name), "/") + frames := []importLinkFrame{{pending: strings.Split(filepath.ToSlash(name), "/")}} resolved := "" activeLinks := make(map[string]struct{}) - for len(pending) > 0 { - segment := pending[0] - pending = pending[1:] + for len(frames) > 0 { + frame := &frames[len(frames)-1] + if len(frame.pending) == 0 { + delete(activeLinks, frame.link) + frames = frames[:len(frames)-1] + continue + } + segment := frame.pending[0] + frame.pending = frame.pending[1:] candidate := path.Join(resolved, segment) if candidate == "" { candidate = "." @@ -149,8 +162,7 @@ func resolveHostImportPath(access *filesystemAccess, name string) (string, error if !withinPortableRoot(targetPath) { return "", errImportPathOutsideRoot } - remaining := append([]string(nil), pending...) - pending = append(strings.Split(targetPath, "/"), remaining...) + frames = append(frames, importLinkFrame{pending: strings.Split(targetPath, "/"), link: candidate}) resolved = "" } if resolved == "" { @@ -167,12 +179,18 @@ func resolveNativeImportPath(access *filesystemAccess, filesystem fs.ReadLinkFS, if !fs.ValidPath(name) || !access.contains(name) { return "", fs.ErrInvalid } - pending := strings.Split(name, "/") + frames := []importLinkFrame{{pending: strings.Split(name, "/")}} resolved := "" activeLinks := make(map[string]struct{}) - for len(pending) > 0 { - segment := pending[0] - pending = pending[1:] + for len(frames) > 0 { + frame := &frames[len(frames)-1] + if len(frame.pending) == 0 { + delete(activeLinks, frame.link) + frames = frames[:len(frames)-1] + continue + } + segment := frame.pending[0] + frame.pending = frame.pending[1:] candidate := path.Join(resolved, segment) if candidate == "" { candidate = "." @@ -205,8 +223,7 @@ func resolveNativeImportPath(access *filesystemAccess, filesystem fs.ReadLinkFS, if !fs.ValidPath(targetPath) || !access.contains(targetPath) { return "", errImportPathOutsideRoot } - remaining := append([]string(nil), pending...) - pending = append(strings.Split(targetPath, "/"), remaining...) + frames = append(frames, importLinkFrame{pending: strings.Split(targetPath, "/"), link: candidate}) resolved = "" } if resolved == "" { diff --git a/internal/engine/filesystem_links_test.go b/internal/engine/filesystem_links_test.go new file mode 100644 index 0000000..36a470d --- /dev/null +++ b/internal/engine/filesystem_links_test.go @@ -0,0 +1,47 @@ +package engine + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "testing" +) + +func TestImportLinkResolutionTracksActiveExpansions(t *testing.T) { + root := t.TempDir() + if err := os.Mkdir(filepath.Join(root, "parts"), 0o700); err != nil { + t.Fatal(err) + } + for name, target := range map[string]string{"parts/up": "..", "self": "self", "first": "second", "second": "first"} { + if err := os.Symlink(target, filepath.Join(root, filepath.FromSlash(name))); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + } + host, err := openHostAccess(root) + if err != nil { + t.Fatal(err) + } + defer host.close() + filesystem := os.DirFS(root) + links, ok := filesystem.(fs.ReadLinkFS) + if !ok { + t.Fatal("native directory filesystem does not expose link inspection") + } + native := newNativeAccess(filesystem, ".", &namespaceToken{}) + for name, resolve := range map[string]func(string) (string, error){ + "host": func(name string) (string, error) { return resolveHostImportPath(host, name) }, + "native": func(name string) (string, error) { return resolveNativeImportPath(native, links, name) }, + } { + t.Run(name, func(t *testing.T) { + if got, err := resolve("parts/up/parts/up/parts"); err != nil || got != "parts" { + t.Errorf("finite link reuse = %q, %v", got, err) + } + for _, cycle := range []string{"self", "first"} { + if _, err := resolve(cycle); !errors.Is(err, errImportDirectoryLinkCycle) { + t.Errorf("%s cycle = %v", cycle, err) + } + } + }) + } +}