From 760f25293632a1f54af4000b683bf826dfa3a86f Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Sat, 28 Feb 2026 10:10:17 -0800 Subject: [PATCH] fix: improve WalkVersions() ancestor-directory guard for prefix filtering Replace the length-comparison condition with an explicit predicate that is both more readable and correctly scoped: skip only when the visited directory is a strict ancestor of the specified prefix (not a descendant and not when prefix is empty). Adds tests from the original bug report (#1864) to verify the fix and guard against future regressions. --- backend/walk.go | 11 ++++- backend/walk_test.go | 109 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/backend/walk.go b/backend/walk.go index 5670cfd4..2e394c4a 100644 --- a/backend/walk.go +++ b/backend/walk.go @@ -389,8 +389,15 @@ func WalkVersions(ctx context.Context, fileSystem fs.FS, prefix, delimiter, keyM return fs.SkipDir } - // Skip parents of specified prefix - if len(path+"/") < len(prefix) { + // Skip ancestor directories of the specified prefix; only process + // the directory that exactly matches the prefix. + // At this point we know strings.HasPrefix(prefix, path+"/") holds + // (i.e. path is an ancestor of the prefix directory). Skip it + // unless it is the exact prefix directory. + // Note: WalkVersions always walks from "." (unlike Walk, which + // narrows the root) because versioning marker semantics require + // visiting all entries in order, so this guard is needed instead. + if prefix != "" && strings.HasPrefix(prefix, path+"/") && path+"/" != prefix { return nil } diff --git a/backend/walk_test.go b/backend/walk_test.go index 9d800961..489a4d4e 100644 --- a/backend/walk_test.go +++ b/backend/walk_test.go @@ -20,6 +20,7 @@ import ( "encoding/hex" "fmt" "io/fs" + "strings" "sync" "testing" "testing/fstest" @@ -1098,3 +1099,111 @@ func TestWalkVersionsTruncated(t *testing.T) { t.Errorf("total versions mismatch: expected %d, got %d", expectedTotal, totalVersions) } } + +// TestWalkVersionsPrefixSkipsParents reproduces the bug (#1864) where +// ListObjectVersions with a deep prefix and delimiter="/" was incorrectly +// returning version entries for ancestor directories (e.g. "vendor/", +// "vendor/Backup/") in addition to the the prefix directory itself. +// Only "vendor/Backup/vendor/Clients/" should appear in ObjectVersions; all +// deeper entries should become CommonPrefixes. +func TestWalkVersionsPrefixSkipsParents(t *testing.T) { + fsys := fstest.MapFS{ + "vendor": {Mode: fs.ModeDir}, + "vendor/Backup": {Mode: fs.ModeDir}, + "vendor/Backup/vendor": {Mode: fs.ModeDir}, + "vendor/Backup/vendor/Clients": {Mode: fs.ModeDir}, + "vendor/Backup/vendor/Clients/abc": {Mode: fs.ModeDir}, + "vendor/Backup/vendor/Clients/abc/backup.vbm": {}, + } + + prefix := "vendor/Backup/vendor/Clients/" + delimiter := "/" + + res, err := backend.WalkVersions(context.Background(), fsys, prefix, delimiter, "", "", 1000, getVersionsTestFunc, []string{}) + if err != nil { + t.Fatalf("WalkVersions: %v", err) + } + + // Only the exact prefix directory should appear as an ObjectVersion. + expectedVersionKeys := []string{"vendor/Backup/vendor/Clients/"} + if !compareObjectVersionsOrdered(res.ObjectVersions, makeObjectVersions(expectedVersionKeys)) { + t.Errorf("unexpected ObjectVersions: got %v, want %v", + printVersionObjects(res.ObjectVersions), expectedVersionKeys) + } + + // The child directory should be a CommonPrefix. + expectedPrefixes := []string{"vendor/Backup/vendor/Clients/abc/"} + if !comparePrefixesUnordered(res.CommonPrefixes, expectedPrefixes) { + t.Errorf("unexpected CommonPrefixes: got %v, want %v", + printCommonPrefixes(res.CommonPrefixes), expectedPrefixes) + } +} + +// TestWalkVersionsPrefixNoDelimiterSkipsParents verifies that even without a +// delimiter, ancestor directories of the prefix are not returned as versions. +func TestWalkVersionsPrefixNoDelimiterSkipsParents(t *testing.T) { + fsys := fstest.MapFS{ + "vendor": {Mode: fs.ModeDir}, + "vendor/Backup": {Mode: fs.ModeDir}, + "vendor/Backup/vendor": {Mode: fs.ModeDir}, + "vendor/Backup/vendor/Clients": {Mode: fs.ModeDir}, + "vendor/Backup/vendor/Clients/file.vbm": {}, + } + + prefix := "vendor/Backup/vendor/Clients/" + + res, err := backend.WalkVersions(context.Background(), fsys, prefix, "", "", "", 1000, getVersionsTestFunc, []string{}) + if err != nil { + t.Fatalf("WalkVersions: %v", err) + } + + // Ancestor keys must not appear. every returned key must start with prefix. + for _, ov := range res.ObjectVersions { + if ov.Key == nil { + t.Error("nil key in ObjectVersions") + continue + } + if !strings.HasPrefix(*ov.Key, prefix) { + t.Errorf("ancestor key leaked into results: %q", *ov.Key) + } + } + + // The prefix dir itself and its file should be the only versions. + expectedVersionKeys := []string{ + "vendor/Backup/vendor/Clients/", + "vendor/Backup/vendor/Clients/file.vbm", + } + if !compareObjectVersionsOrdered(res.ObjectVersions, makeObjectVersions(expectedVersionKeys)) { + t.Errorf("unexpected ObjectVersions: got %v, want %v", + printVersionObjects(res.ObjectVersions), expectedVersionKeys) + } +} + +// makeObjectVersions builds a []s3response.ObjectVersion slice with just keys +// set, for use in test comparisons. +func makeObjectVersions(keys []string) []s3response.ObjectVersion { + ovs := make([]s3response.ObjectVersion, len(keys)) + for i, k := range keys { + key := k + ovs[i] = s3response.ObjectVersion{Key: &key} + } + return ovs +} + +// comparePrefixesUnordered checks that got contains exactly the expected +// prefix strings regardless of order. +func comparePrefixesUnordered(got []types.CommonPrefix, want []string) bool { + if len(got) != len(want) { + return false + } + wantSet := make(map[string]bool, len(want)) + for _, w := range want { + wantSet[w] = true + } + for _, cp := range got { + if cp.Prefix == nil || !wantSet[*cp.Prefix] { + return false + } + } + return true +}