Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drive: rewrite LOCK paths #12137

Merged
merged 1 commit into from
May 16, 2024
Merged

drive: rewrite LOCK paths #12137

merged 1 commit into from
May 16, 2024

Conversation

oxtoacart
Copy link
Contributor

@oxtoacart oxtoacart commented May 15, 2024

Fixes #12097

Just like we rewrite paths for PROPFIND responses to reflect the unified filesystem structure of <tailnet>/<peer>/<share>/<file>, we also have to rewrite paths used in LOCK requests and responses.

@@ -93,8 +93,17 @@ var cacheInvalidatingMethods = map[string]bool{

// ServeHTTP implements http.Handler.
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if r.Method == "PROPFIND" {
h.handlePROPFIND(w, r)
pathComponents := shared.CleanAndSplit(r.URL.Path)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: do this only once, then pass results to the methods that need them.

@oxtoacart oxtoacart force-pushed the percy/webdav_locks branch 4 times, most recently from af3d20d to 6f94023 Compare May 15, 2024 01:33
@@ -0,0 +1,112 @@
// Copyright (c) Tailscale Inc & AUTHORS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file replaces propfind.go with all rewriting logic, including for PROPFIND and LOCK.

share11 = `sha re$%11`
share12 = `_sha re$%12`
file111 = `fi le$%111.txt`
remote1 = `rem ote$%<>1`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: we weren't properly escaping paths for inclusion in XML, this tests for that.

h.delegate(mpl, pathComponents[mpl-1:], bw, r)

// Fixup paths to add the requested path as a prefix, escaped for inclusion in XML.
pp := shared.EscapeForXML(shared.Join(pathComponents[0:mpl]...))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: properly escape paths for inclusion in XML.

case "1":
return 1
case "infinity":
return math.MaxInt16 // a really large number, but not infinity (avoids wrapping when we do arithmetic with this)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: this used to be MaxInt, which wrapped when we added +1 to it.

drive/driveimpl/compositedav/rewriting.go Dismissed Show dismissed Hide dismissed
)

var (
responseHrefRegex = regexp.MustCompile(`(?s)(<D:(response|lockroot)>)<D:href>/?([^<]*)/?</D:href>`)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<D:href> appears in various contexts, but we're only interested when it's used with <D:response> or <D:lockroot>.

@oxtoacart oxtoacart changed the title drive: properly rewrite LOCK paths drive: rewrite LOCK paths May 15, 2024
Copy link
Member

@soniaappasamy soniaappasamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I don't think I know enough about WebDAV to really review for accuracy, but the restructure of code lgtm.

http.Error(w, "locking of top level directories is not allowed", http.StatusMethodNotAllowed)
}

func shouldDelegateToChild(r *http.Request, pathComponents []string, mpl int) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe add some docs to this function? a little confusing to decipher for someone new

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return bw.buf.Write(p)
}

func rewriteIfHeader(r *http.Request, pathComponents []string, mpl int) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: docs for this one too pls!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Fixes #12097

Signed-off-by: Percy Wegmann <[email protected]>
@oxtoacart
Copy link
Contributor Author

Thanks for the review @soniaappasamy

@oxtoacart oxtoacart merged commit 59848fe into main May 16, 2024
48 checks passed
@oxtoacart oxtoacart deleted the percy/webdav_locks branch May 16, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

taildrive: writes do not arrive on a writeable mount
2 participants