From 7e01b0727be57e9764d46a9e86bde9b6a0e8bfc6 Mon Sep 17 00:00:00 2001 From: "Wayne E. Seguin" Date: Thu, 16 Jul 2026 12:36:09 -0400 Subject: [PATCH] Scope dav recursive list and delete to the requested prefix The dav backend's List started its PROPFIND walk at the blobstore endpoint root and filtered by prefix only at the leaves, so DeleteRecursive traversed every directory in the store to delete one prefix. Against a large partitioned WebDAV blobstore this issues thousands of PROPFIND round trips per delete and the operation times out. Bound the walk to the prefix's subtree: - List now starts the PROPFIND at the deepest directory the prefix fully names (the portion up to its last slash), matching how the s3 backend pushes the prefix to the server. Directories that would escape the endpoint through dot-dot segments are rejected and fall back to the endpoint-root walk, and the starting URL is slash-terminated so servers that redirect directory URLs to their canonical form are not triggered. - listRecursive now skips collections whose paths cannot contain blob IDs matching the prefix, which prunes sibling partitions when the prefix does not name a directory. Blob IDs are still matched as string prefixes at the leaves, so the observable results of List and DeleteRecursive are unchanged; only the traversal is narrower. A missing prefix directory still yields an empty list via the existing 404 handling. Add a dav integration spec covering multi-segment prefixes, which the existing single-segment specs never exercised. Fixes cloudfoundry/storage-cli#129 --- dav/client/storage_client.go | 47 +++- dav/client/storage_client_list_test.go | 327 +++++++++++++++++++++++++ dav/integration/assertions.go | 58 +++++ dav/integration/general_dav_test.go | 10 + 4 files changed, 441 insertions(+), 1 deletion(-) create mode 100644 dav/client/storage_client_list_test.go diff --git a/dav/client/storage_client.go b/dav/client/storage_client.go index 1d3e17b..9cdc828 100644 --- a/dav/client/storage_client.go +++ b/dav/client/storage_client.go @@ -336,8 +336,32 @@ func (c *storageClient) List(prefix string) ([]string, error) { if !strings.HasPrefix(rootURL.Path, "/") { rootURL.Path = "/" + rootURL.Path } + endpointPath := rootURL.Path - return c.listRecursive(rootURL.String(), rootURL.Path, prefix) + // Start the walk at the deepest directory the prefix fully names, so the + // traversal is bounded by the prefix's subtree instead of the whole store. + // Blob IDs are matched as string prefixes, so only the portion of the + // prefix up to its last "/" is guaranteed to be a directory. The trailing + // slash marks the URL as a collection, matching the slash-terminated hrefs + // the recursion follows and avoiding directory redirects from servers that + // insist on the canonical form. + if dir := prefixDir(prefix); dir != "" { + rootURL.Path = path.Join(rootURL.Path, dir) + "/" + } + + return c.listRecursive(rootURL.String(), endpointPath, prefix) +} + +// prefixDir returns the deepest directory a blob-ID prefix fully names, or "" +// when the prefix does not name one ("" or a single path segment) or would +// escape the endpoint through ".." segments, in which case the walk starts at +// the endpoint root as it always did. +func prefixDir(prefix string) string { + dir := path.Dir(strings.TrimPrefix(prefix, "/")) + if dir == "." || dir == "/" || dir == ".." || strings.HasPrefix(dir, "../") { + return "" + } + return dir } func (c *storageClient) listRecursive(dirURL, endpointPath, prefix string) ([]string, error) { @@ -395,6 +419,9 @@ func (c *storageClient) listRecursive(dirURL, endpointPath, prefix string) ([]st } if response.isCollection() { + if !collectionMayContainPrefix(response.Href, endpointPath, prefix) { + continue + } subURL := hrefURL.String() if !hrefURL.IsAbs() { subURL = parsedDirURL.ResolveReference(hrefURL).String() @@ -420,6 +447,24 @@ func (c *storageClient) listRecursive(dirURL, endpointPath, prefix string) ([]st return blobs, nil } +// collectionMayContainPrefix reports whether a collection can hold blob IDs +// matching the prefix. Every blob under a collection with relative path rel +// has an ID starting with rel+"/", so the collection is worth descending into +// only when rel+"/" and the prefix are string prefixes of one another. +func collectionMayContainPrefix(href, endpointPath, prefix string) bool { + if prefix == "" { + return true + } + rel, err := blobIDFromHref(href, endpointPath) + if err != nil { + // Can't map the href to a relative path; descend rather than risk + // skipping blobs. + return true + } + rel = strings.TrimSuffix(rel, "/") + "/" + return strings.HasPrefix(prefix, rel) || strings.HasPrefix(rel, prefix) +} + // blobIDFromHref extracts the blob ID from a WebDAV href // Returns the path relative to the endpoint func blobIDFromHref(href, endpointPath string) (string, error) { diff --git a/dav/client/storage_client_list_test.go b/dav/client/storage_client_list_test.go new file mode 100644 index 0000000..a3ec3fc --- /dev/null +++ b/dav/client/storage_client_list_test.go @@ -0,0 +1,327 @@ +package client + +import ( + "fmt" + "net/http" + "net/http/httptest" + "sort" + "strings" + "sync" + "testing" + + davconf "github.com/cloudfoundry/storage-cli/dav/config" +) + +// fakeDavServer serves a static directory tree over PROPFIND/DELETE and +// records which paths were hit, so tests can assert how far a walk went. +type fakeDavServer struct { + mu sync.Mutex + propfinds []string + deletes []string + + // dirs maps a directory path (relative to the endpoint, "" for the + // root) to its child entries. + dirs map[string][]fakeEntry +} + +type fakeEntry struct { + name string + isDir bool +} + +func (s *fakeDavServer) handler(endpointPath string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + rel := strings.Trim(strings.TrimPrefix(r.URL.Path, endpointPath), "/") + + switch r.Method { + case "PROPFIND": + s.mu.Lock() + s.propfinds = append(s.propfinds, rel) + s.mu.Unlock() + + children, ok := s.dirs[rel] + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + + var b strings.Builder + b.WriteString(``) + writeResponse := func(href string, isDir bool) { + resourceType := "" + if isDir { + resourceType = "" + } + fmt.Fprintf(&b, + `%s%s`, + href, resourceType) + } + + self := endpointPath + "/" + if rel != "" { + self = endpointPath + "/" + rel + "/" + } + writeResponse(self, true) + for _, child := range children { + href := endpointPath + "/" + child.name + if rel != "" { + href = endpointPath + "/" + rel + "/" + child.name + } + if child.isDir { + href += "/" + } + writeResponse(href, child.isDir) + } + b.WriteString(``) + + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(http.StatusMultiStatus) + w.Write([]byte(b.String())) //nolint:errcheck + + case "DELETE": + s.mu.Lock() + s.deletes = append(s.deletes, rel) + s.mu.Unlock() + w.WriteHeader(http.StatusNoContent) + + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + } +} + +// newPartitionedStore models a CC-style blobstore: two levels of partition +// directories with blobs at the leaves, plus sibling partitions that a +// correctly scoped walk must never touch. +func newPartitionedStore() *fakeDavServer { + return &fakeDavServer{ + dirs: map[string][]fakeEntry{ + "": { + {name: "aa", isDir: true}, + {name: "ab", isDir: true}, + {name: "zz", isDir: true}, + }, + "aa": {{name: "bb", isDir: true}}, + "aa/bb": {{name: "aabb-other-guid", isDir: false}}, + "ab": {{name: "cd", isDir: true}, {name: "zz", isDir: true}}, + "ab/zz": {{name: "abzz-unrelated", isDir: false}}, + "ab/cd": {{name: "abcd-target-guid", isDir: true}, {name: "abcd-target-guid-file", isDir: false}, {name: "abcd-unrelated", isDir: false}}, + "ab/cd/abcd-target-guid": {{name: "cflinuxfs4", isDir: false}}, + "zz": {{name: "yy", isDir: true}}, + "zz/yy": {{name: "zzyy-other", isDir: false}}, + }, + } +} + +func newTestStorageClient(t *testing.T, store *fakeDavServer) (*storageClient, func()) { + t.Helper() + server := httptest.NewServer(store.handler("/dav")) + c := NewStorageClient(davconf.Config{Endpoint: server.URL + "/dav"}, http.DefaultClient) + return c.(*storageClient), server.Close +} + +func sorted(in []string) []string { + out := append([]string{}, in...) + sort.Strings(out) + return out +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func TestListScopesWalkToPrefix(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + blobs, err := c.List("ab/cd/abcd-target-guid") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + wantBlobs := []string{"ab/cd/abcd-target-guid-file", "ab/cd/abcd-target-guid/cflinuxfs4"} + if !equalStrings(sorted(blobs), wantBlobs) { + t.Fatalf("unexpected blobs: %v, want %v", sorted(blobs), wantBlobs) + } + + wantPropfinds := []string{"ab/cd", "ab/cd/abcd-target-guid"} + if !equalStrings(sorted(store.propfinds), wantPropfinds) { + t.Fatalf("walk was not scoped to the prefix: PROPFINDs hit %v, want %v", sorted(store.propfinds), wantPropfinds) + } +} + +func TestListEmptyPrefixWalksWholeStore(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + blobs, err := c.List("") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + wantBlobs := []string{ + "aa/bb/aabb-other-guid", + "ab/cd/abcd-target-guid-file", + "ab/cd/abcd-target-guid/cflinuxfs4", + "ab/cd/abcd-unrelated", + "ab/zz/abzz-unrelated", + "zz/yy/zzyy-other", + } + if !equalStrings(sorted(blobs), wantBlobs) { + t.Fatalf("unexpected blobs: %v, want %v", sorted(blobs), wantBlobs) + } +} + +func TestListMissingPrefixDirectoryReturnsEmpty(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + blobs, err := c.List("no/such/prefix") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(blobs) != 0 { + t.Fatalf("expected no blobs, got %v", blobs) + } +} + +func TestListSingleSegmentPrefixPrunesSiblings(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + blobs, err := c.List("ab") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + wantBlobs := []string{ + "ab/cd/abcd-target-guid-file", + "ab/cd/abcd-target-guid/cflinuxfs4", + "ab/cd/abcd-unrelated", + "ab/zz/abzz-unrelated", + } + if !equalStrings(sorted(blobs), wantBlobs) { + t.Fatalf("unexpected blobs: %v, want %v", sorted(blobs), wantBlobs) + } + + for _, p := range store.propfinds { + if strings.HasPrefix(p, "aa") || strings.HasPrefix(p, "zz") { + t.Fatalf("walk descended into sibling partition %q; PROPFINDs: %v", p, store.propfinds) + } + } +} + +func TestDeleteRecursiveDeletesOnlyPrefixedBlobs(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + if err := c.DeleteRecursive("ab/cd/abcd-target-guid"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + wantDeletes := []string{"ab/cd/abcd-target-guid-file", "ab/cd/abcd-target-guid/cflinuxfs4"} + if !equalStrings(sorted(store.deletes), wantDeletes) { + t.Fatalf("unexpected deletes: %v, want %v", sorted(store.deletes), wantDeletes) + } + + wantPropfinds := []string{"ab/cd", "ab/cd/abcd-target-guid"} + if !equalStrings(sorted(store.propfinds), wantPropfinds) { + t.Fatalf("walk was not scoped to the prefix: PROPFINDs hit %v, want %v", sorted(store.propfinds), wantPropfinds) + } +} + +func TestListEscapingPrefixStaysInsideEndpoint(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + for _, prefix := range []string{"../", "../../..", "../etc/passwd"} { + store.propfinds = nil + + blobs, err := c.List(prefix) + if err != nil { + t.Fatalf("List(%q): unexpected error: %v", prefix, err) + } + if len(blobs) != 0 { + t.Fatalf("List(%q): expected no blobs, got %v", prefix, blobs) + } + for _, p := range store.propfinds { + if strings.Contains(p, "..") { + t.Fatalf("List(%q): PROPFIND escaped the endpoint: %v", prefix, store.propfinds) + } + } + } +} + +func TestDeleteRecursiveMissingPrefixIsNoop(t *testing.T) { + store := newPartitionedStore() + c, cleanup := newTestStorageClient(t, store) + defer cleanup() + + if err := c.DeleteRecursive("no/such/prefix"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(store.deletes) != 0 { + t.Fatalf("expected no deletes, got %v", store.deletes) + } +} + +func TestPrefixDir(t *testing.T) { + cases := []struct { + prefix string + want string + }{ + {prefix: "", want: ""}, + {prefix: "abc", want: ""}, + {prefix: "ab/", want: "ab"}, + {prefix: "ab/cd", want: "ab"}, + {prefix: "ab/cd/", want: "ab/cd"}, + {prefix: "ab/cd/guid", want: "ab/cd"}, + {prefix: "/ab/cd/guid", want: "ab/cd"}, + {prefix: "..", want: ""}, + {prefix: "../", want: ""}, + {prefix: "../etc", want: ""}, + {prefix: "../../../etc", want: ""}, + {prefix: "a/../../b", want: ""}, + {prefix: "foo/../bar/x", want: "bar"}, + } + for _, tc := range cases { + if got := prefixDir(tc.prefix); got != tc.want { + t.Errorf("prefixDir(%q) = %q, want %q", tc.prefix, got, tc.want) + } + } +} + +func TestCollectionMayContainPrefix(t *testing.T) { + cases := []struct { + href string + prefix string + want bool + }{ + {href: "/dav/ab/", prefix: "", want: true}, + {href: "/dav/ab/", prefix: "ab/cd/guid", want: true}, + {href: "/dav/ab/cd/", prefix: "ab/cd/guid", want: true}, + {href: "/dav/ab/cd/guid-dir/", prefix: "ab/cd/guid", want: true}, + {href: "/dav/ab/cd/other/", prefix: "ab/cd/guid", want: false}, + {href: "/dav/aa/", prefix: "ab/cd/guid", want: false}, + {href: "/dav/ab/zz/", prefix: "ab/cd/guid", want: false}, + } + for _, tc := range cases { + if got := collectionMayContainPrefix(tc.href, "/dav", tc.prefix); got != tc.want { + t.Errorf("collectionMayContainPrefix(%q, %q) = %v, want %v", tc.href, tc.prefix, got, tc.want) + } + } +} diff --git a/dav/integration/assertions.go b/dav/integration/assertions.go index 82f1825..de72109 100644 --- a/dav/integration/assertions.go +++ b/dav/integration/assertions.go @@ -160,6 +160,64 @@ func AssertOnListDeleteLifecycle(cliPath string, cfg *config.Config) { } } +func AssertOnNestedListDeleteLifecycle(cliPath string, cfg *config.Config) { + guid := GenerateRandomString() + prefix := "ab/cd/" + guid + + configPath := MakeConfigFile(cfg) + defer os.Remove(configPath) //nolint:errcheck + + targets := []string{ + prefix + "/droplet", + prefix + "-extra", + } + bystanders := []string{ + "ab/cd/" + GenerateRandomString(), + "zz/yy/" + GenerateRandomString(), + } + + var contentFiles []string + defer func() { + for _, f := range contentFiles { + os.Remove(f) //nolint:errcheck + } + }() + + for _, blobName := range append(append([]string{}, targets...), bystanders...) { + contentFile := MakeContentFile(GenerateRandomString()) + contentFiles = append(contentFiles, contentFile) + session, err := RunCli(cliPath, configPath, storageType, "put", contentFile, blobName) + Expect(err).ToNot(HaveOccurred()) + Expect(session.ExitCode()).To(BeZero()) + } + + session, err := RunCli(cliPath, configPath, storageType, "list", prefix) + Expect(err).ToNot(HaveOccurred()) + Expect(session.ExitCode()).To(BeZero()) + for _, blobName := range targets { + Expect(session.Out.Contents()).To(ContainSubstring(blobName)) + } + for _, blobName := range bystanders { + Expect(session.Out.Contents()).ToNot(ContainSubstring(blobName)) + } + + session, err = RunCli(cliPath, configPath, storageType, "delete-recursive", prefix) + Expect(err).ToNot(HaveOccurred()) + Expect(session.ExitCode()).To(BeZero()) + + for _, blobName := range targets { + session, err := RunCli(cliPath, configPath, storageType, "exists", blobName) + Expect(err).ToNot(HaveOccurred()) + Expect(session.ExitCode()).ToNot(BeZero()) + } + + for _, blobName := range bystanders { + session, err := RunCli(cliPath, configPath, storageType, "exists", blobName) + Expect(err).ToNot(HaveOccurred()) + Expect(session.ExitCode()).To(BeZero()) + } +} + func AssertListNonexistentPrefixReturnsEmpty(cliPath string, cfg *config.Config) { nonExistentPrefix := GenerateRandomString() diff --git a/dav/integration/general_dav_test.go b/dav/integration/general_dav_test.go index c6526b7..9d91808 100644 --- a/dav/integration/general_dav_test.go +++ b/dav/integration/general_dav_test.go @@ -61,6 +61,16 @@ var _ = Describe("General testing for DAV", func() { integration.AssertOnListDeleteLifecycle(cliPath, cfg) }) + It("Blobstore list and delete-recursive lifecycle works with nested prefixes", func() { + cfg := &config.Config{ + Endpoint: davEndpoint, + User: davUser, + Password: davPassword, + TLS: config.TLS{Cert: config.Cert{CA: davCA}}, + } + integration.AssertOnNestedListDeleteLifecycle(cliPath, cfg) + }) + It("Invoking `list` on non-existent prefix returns empty list", func() { cfg := &config.Config{ Endpoint: davEndpoint,