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,