diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 89e5553..627804c 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Shared validation foundation remediation -Status: Remediation in progress; finding 1 addressed, findings 2–6 pending. +Status: Remediation in progress; findings 1–2 addressed, findings 3–6 pending. Reviewed committed implementation `f53354b`, covering the eight foundation commits after `0c7129a`, against the [implementation plan](implementation.md), @@ -63,6 +63,13 @@ and bounded even when testing the broken behavior. ## 2. P2: Path cleanup changes symlink traversal and checkout classification +**Resolution:** Addressed. Destination components and symlink targets are +traversed in order, with parent traversal applied to resolved directories. +Missing entries and regular-file traversal remain observable. Regression tests +cover both destination and symlink-target parent components and confirm that +neighboring files do not change classification. +`go test ./internal/validation` passed. + **Owner:** [candidate destination resolver](../../internal/validation/tree.go), initial destination construction and construction after symlink expansion. diff --git a/internal/validation/destination_test.go b/internal/validation/destination_test.go index f8349a4..6b14514 100644 --- a/internal/validation/destination_test.go +++ b/internal/validation/destination_test.go @@ -174,3 +174,59 @@ func TestResolveDestinationIgnoresNeighboringFiles(t *testing.T) { t.Fatalf("neighbor changed resolution: absent=%#v present=%#v", gotWithoutNeighbor, gotWithNeighbor) } } + +func TestResolveDestinationPreservesParentTraversalOrder(t *testing.T) { + repository := newTreeRepository(t) + for _, name := range []string{"README.md", "docs/file.md", "nested/file.md", "nested/inner/item.txt"} { + writeTreeFile(t, repository, name, []byte("captured\n")) + } + for name, target := range map[string]string{ + "docs/link": "../nested/inner", + "docs/parent-link": "link/../file.md", + "escape": "../neighbor", + } { + if err := os.Symlink(target, filepath.Join(repository, filepath.FromSlash(name))); err != nil { + t.Fatal(err) + } + } + gitTree(t, repository, "add", "--all") + gitTree(t, repository, "commit", "-m", "capture parent traversal") + tree, err := ReadCandidateTree(t.Context(), repository, strings.TrimSpace(gitTree(t, repository, "rev-parse", "HEAD"))) + if err != nil { + t.Fatal(err) + } + for _, test := range []struct { + destination string + kind DestinationKind + resolved string + }{ + {"docs/link/../file.md", DestinationEntry, "nested/file.md"}, + {"docs/parent-link", DestinationEntry, "nested/file.md"}, + {"escape/../README.md", DestinationOutsideCheckout, ""}, + {"missing/../README.md", DestinationMissing, ""}, + {"README.md/../README.md", DestinationMissing, ""}, + {"README.md/.", DestinationMissing, ""}, + } { + t.Run(test.destination, func(t *testing.T) { + got, err := tree.ResolveDestination(t.Context(), "README.md", test.destination) + if err != nil || got.Kind != test.kind || (test.resolved != "" && got.Path != test.resolved) { + t.Fatalf("resolution = %#v, %v; want %s %q", got, err, test.kind, test.resolved) + } + }) + } + before, err := tree.ResolveDestination(t.Context(), "README.md", "escape/../README.md") + if err != nil { + t.Fatal(err) + } + neighbor := filepath.Join(filepath.Dir(repository), "neighbor") + if err := os.MkdirAll(neighbor, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(neighbor, "file.md"), []byte("neighbor"), 0o644); err != nil { + t.Fatal(err) + } + after, err := tree.ResolveDestination(t.Context(), "README.md", "escape/../README.md") + if err != nil || before != after { + t.Fatalf("neighbor changed resolution: %#v -> %#v, %v", before, after, err) + } +} diff --git a/internal/validation/tree.go b/internal/validation/tree.go index 2675170..949facd 100644 --- a/internal/validation/tree.go +++ b/internal/validation/tree.go @@ -106,34 +106,36 @@ func (tree *CandidateTree) ResolveDestination(ctx context.Context, sourcePath, d if strings.HasPrefix(destination, "/") { return Destination{Kind: DestinationOutsideCheckout, Path: path.Clean(destination)}, nil } - resolved := path.Clean(path.Join(path.Dir(sourcePath), destination)) - return tree.resolveDestination(ctx, resolved, make(map[string]bool)) + return tree.resolveDestination(ctx, path.Dir(sourcePath), strings.Split(destination, "/"), make(map[string]bool)) } -func (tree *CandidateTree) resolveDestination(ctx context.Context, resolved string, active map[string]bool) (Destination, error) { - if err := ctx.Err(); err != nil { - return Destination{}, err - } - if outsideCheckout(resolved) { - return Destination{Kind: DestinationOutsideCheckout, Path: resolved}, nil - } - components := strings.Split(resolved, "/") - partial := "" +func (tree *CandidateTree) resolveDestination(ctx context.Context, directory string, components []string, active map[string]bool) (Destination, error) { for index, component := range components { if err := ctx.Err(); err != nil { return Destination{}, err } - partial = path.Join(partial, component) - remaining := strings.Join(components[index+1:], "/") + remaining := components[index+1:] + switch component { + case "", ".": + continue + case "..": + if directory == "." { + return Destination{Kind: DestinationOutsideCheckout, Path: unresolvedPath("..", remaining)}, nil + } + directory = path.Dir(directory) + continue + } + partial := path.Join(directory, component) entry, exists := tree.Entry(partial) if !exists { - if _, directory := tree.directories[partial]; !directory { - return Destination{Kind: DestinationMissing, Path: resolved}, nil + if _, isDirectory := tree.directories[partial]; !isDirectory { + return Destination{Kind: DestinationMissing, Path: unresolvedPath(partial, remaining)}, nil } + directory = partial continue } if entry.Mode == "160000" { - return Destination{Kind: DestinationSubmodule, Path: path.Join(partial, remaining)}, nil + return Destination{Kind: DestinationSubmodule, Path: unresolvedPath(partial, remaining)}, nil } if entry.Mode == "120000" { if active[partial] { @@ -145,30 +147,38 @@ func (tree *CandidateTree) resolveDestination(ctx context.Context, resolved stri } target := string(linkText) if strings.HasPrefix(target, "/") { - return Destination{Kind: DestinationOutsideCheckout, Path: target}, nil + return Destination{Kind: DestinationOutsideCheckout, Path: unresolvedPath(target, remaining)}, nil } active[partial] = true - destination, err := tree.resolveDestination(ctx, path.Join(path.Dir(partial), target), active) + destination, err := tree.resolveDestination(ctx, directory, strings.Split(target, "/"), active) delete(active, partial) - if err != nil || remaining == "" || destination.Kind != DestinationDirectory { - if err == nil && remaining != "" && destination.Kind == DestinationEntry { - return Destination{Kind: DestinationMissing, Path: path.Join(destination.Path, remaining)}, nil - } + if err != nil || len(remaining) == 0 { return destination, err } - return tree.resolveDestination(ctx, path.Join(destination.Path, remaining), active) + switch destination.Kind { + case DestinationDirectory: + return tree.resolveDestination(ctx, destination.Path, remaining, active) + case DestinationEntry: + destination.Kind = DestinationMissing + case DestinationSymlinkCycle: + return destination, nil + } + destination.Path = unresolvedPath(destination.Path, remaining) + return destination, nil } - if remaining != "" { - return Destination{Kind: DestinationMissing, Path: resolved}, nil + if len(remaining) != 0 { + return Destination{Kind: DestinationMissing, Path: unresolvedPath(partial, remaining)}, nil } return Destination{Kind: DestinationEntry, Path: partial}, nil } - if _, directory := tree.directories[resolved]; directory { - return Destination{Kind: DestinationDirectory, Path: resolved}, nil - } - return Destination{Kind: DestinationMissing, Path: resolved}, nil + return Destination{Kind: DestinationDirectory, Path: directory}, nil } -func outsideCheckout(name string) bool { - return name == ".." || strings.HasPrefix(name, "../") +// unresolvedPath preserves components that could not be traversed. Cleaning +// them would hide missing entries, non-directories, or checkout boundaries. +func unresolvedPath(base string, remaining []string) string { + if len(remaining) == 0 { + return base + } + return base + "/" + strings.Join(remaining, "/") }