Implement Copy, List, DeleteRecursive, Properties for DAV#103
Conversation
Replaces the stubs left from the two-layer refactor (#98). Adds PROPFIND and COPY support via WebDAV verbs and Basic Auth. EnsureStorageExists is a no-op, matching the existing Ruby DavClient — WebDAV has no bucket concept, the blobstore VM provisions the root directory, and nginx auto-creates per-resource subdirs on first PUT.
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
serdarozerr
left a comment
serdarozerr
left a comment
There was a problem hiding this comment.
looking good some small suggestions
| func (c *storageClient) Put(path string, content io.ReadCloser, contentLength int64) error { | ||
| defer content.Close() //nolint:errcheck | ||
|
|
||
| if err := validateBlobID(path); err != nil { |
There was a problem hiding this comment.
How about calling validateBlogID in client.go, for example in this put we already open file and send content , if we already checked in here https://github.com/cloudfoundry/storage-cli/blob/main/dav/client/client.go#L54 as first condition then we could have skip all these file operations.
| return fmt.Errorf("fetching properties of %q: status %d", blobPath, resp.StatusCode) | ||
| } | ||
|
|
||
| props := BlobProperties{ContentLength: resp.ContentLength} |
There was a problem hiding this comment.
Is this always returns some value, why not null check or something on this ContentLength
| srcBlob, dstBlob, copyResp.StatusCode, c.readAndTruncateBody(copyResp)) | ||
| } | ||
|
|
||
| func (c *storageClient) List(prefix string) ([]string, error) { |
There was a problem hiding this comment.
why there is no validateBlobID for List command ?
There was a problem hiding this comment.
List and DeleteRecursive take a prefix, not a blob ID — they use validatePrefix in client.go
| return hrefPath, nil | ||
| } | ||
|
|
||
| func (c *storageClient) DeleteRecursive(prefix string) error { |
There was a problem hiding this comment.
same as in list function no validateBlobID ?
There was a problem hiding this comment.
List and DeleteRecursive take a prefix, not a blob ID — they use validatePrefix in client.go
| // validateBlobID rejects blob IDs that are unsafe to splice into a request | ||
| // path. The rules are intentionally strict: blob IDs come from external | ||
| // callers (e.g. CCNG, Diego) and a malformed value can confuse path joining, | ||
| // produce ambiguous URLs, or — worst case — escape the configured endpoint | ||
| // via path traversal. We refuse: | ||
| // | ||
| // - empty strings | ||
| // - leading or trailing slashes (the path joiner adds them itself) | ||
| // - empty path segments ("//") | ||
| // - "." or ".." segments (traversal) | ||
| // - control characters (CRLF / NUL injection into headers and URLs) |
There was a problem hiding this comment.
Please shorten those AI comments
There was a problem hiding this comment.
I will ask the AI to do so
| // validatePrefix is the more lenient sibling of validateBlobID, used for List. | ||
| // A directory-style prefix may legitimately end in "/" (e.g. "cc-droplets/"), | ||
| // but everything else still applies. An empty prefix is allowed at the caller | ||
| // level — List uses "" to mean "no filtering" — so this helper is only invoked | ||
| // when a non-empty prefix was supplied. |
There was a problem hiding this comment.
Please shorten those AI comments
| return nil | ||
| } | ||
|
|
||
| // RFC 4918 §9.6.1: 207 Multi-Status means at least one child could not be |
There was a problem hiding this comment.
Do we know when this multi status is raised? Can this happen if a user wants to delete a whole directory but another user concurrently deletes a specific file in that that directory? Recursive delete should be idempotent
There was a problem hiding this comment.
Good point — since we switched DeleteRecursive to list-then-delete on individual files, 207 won't actually be
triggered there anymore. Removing it.
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
3 participants
Replaces the stubs left from the two-layer refactor (#98). Adds PROPFIND and COPY support via WebDAV verbs and Basic Auth.
EnsureStorageExists is a no-op, matching the existing Ruby DavClient — WebDAV has no bucket concept, the blobstore VM provisions the root directory, and nginx auto-creates per-resource subdirs on first PUT.