webdav: ignore path and perm errors in PROPFIND #91
Conversation
PROPFIND can walk through directories, retrieving information about each file. Unfortunately, the filesystem may deny access to the WebDAV server for various reasons, such as the file truly not being readable (e.g. a broken symlink), or because the server does not have permission to read the file. PROPFIND should ignore these. The current behaviour of the WebDAV server when encountering such issues is to immediately stop its walk, and output an http 500. This leads to poor behaviour with the builtin golang server, since the walk has likely already written out its status header as part of serving the previously walked files' properties. The server closes the response, also emitting an error log. While the error log is noisy, the closed response is truly an issue: the xml returned to the client is invalid, which means that the response is useless. It is not unreasonable to expect that a directory shared using WebDAV has files which cannot be read for the reasons given above. The shared directory becomes useless with the current behavior. Rather than making directories with unreadable files useless, skip over anything that is bad. A more nuanced solution to this problem could likely involve indicating that the requested properties have problems, or buffering the response prior to returning the failure. However, this change is simple and a move in the right direction. Fixes golang/go#16195 Fixes golang/go#43782
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
re the CLA failing, we just signed the corporate CLA. I'm in the group, so it probably just needs to be accepted (at least according to the troubleshooting docs). |
|
What corporation? That is, what name was used to sign the corporate CLA? |
It would have been Agilicus (or some variation thereof. My boss filled out the form). |
|
@googlebot I signed it! |
|
This PR (HEAD: d56917e) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/285752 to see it. Tip: You can toggle comments from me using the |
|
@ianlancetaylor I don't like poking people like this, but I have a series of webdav reviews that have been open for ~5 months with no feedback at all. Is there anything I can do to expedite them? I'd rather close them off while the issues are still somewhat fresh in my mind, and the codebase hasn't wandered away too much. |
|
Poking people is fine. Poking on the Gerritt CL is better (https://golang.org/cl/285752). I know nothing about the webdav code, I'll see if I can find someone who does. |
|
Message from Ian Lance Taylor: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/285752. |
|
Message from Ian Lance Taylor: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/285752. |
It seems really difficult to get a patch accepted in the net and crypto repositories. I stopped trying, if anyone is interested I am maintaining a fork of net and crypto with the necessary patch for SFTPGo. For webdav I included some patches here and added lock discovery support https://github.com/drakkan/net/commits/sftpgo I periodically rebase my branches against master |
|
Any updates for this patch? |
I haven't been pushing it from my side. I'd like for it to be integrated to the x/net repo, but I don't really have time to wrangle the reviewers, so unfortunately, I've just left it hanging. |
|
I'd like to say, that this is definitely a step in the right direction and is a bit sad to see this trivial and obvious problem existing for around 6 years in the golang standard library now. Even more so, when someone took the time to research it, fix it, test it, upload it, legalise it and publish it. Is there something that can be done to speed this up? |
PROPFIND can walk through directories, retrieving information about each file. Unfortunately, the filesystem may deny access to the WebDAV server for various reasons, such as the file truly not being readable (e.g. a broken symlink), or because the server does not have permission to read the file. PROPFIND should ignore these. The current behaviour of the WebDAV server when encountering such issues is to immediately stop its walk, and output an http 500. This leads to poor behaviour with the builtin golang server, since the walk has likely already written out its status header as part of serving the previously walked files' properties. The server closes the response, also emitting an error log. While the error log is noisy, the closed response is truly an issue: the xml returned to the client is invalid, which means that the response is useless. It is not unreasonable to expect that a directory shared using WebDAV has files which cannot be read for the reasons given above. The shared directory becomes useless with the current behavior. Rather than making directories with unreadable files useless, skip over anything that is bad. A more nuanced solution to this problem could likely involve indicating that the requested properties have problems, or buffering the response prior to returning the failure. However, this change is simple and a move in the right direction. Fixes golang/go#16195 Fixes golang/go#43782 Change-Id: I065e4c90f7ef797709e5e81e7318c3eafae6db71 GitHub-Last-Rev: d56917e GitHub-Pull-Request: #91 Reviewed-on: https://go-review.googlesource.com/c/net/+/285752 Reviewed-by: Nigel Tao <nigeltao@golang.org> Reviewed-by: Nigel Tao (INACTIVE; USE @golang.org INSTEAD) <nigeltao@google.com> Run-TryBot: Nigel Tao <nigeltao@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Matthew Holt <matthew.holt@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
PROPFIND can walk through directories, retrieving information about each file. Unfortunately, the filesystem may deny access to the WebDAV server for various reasons, such as the file truly not being readable (e.g. a broken symlink), or because the server does not have permission to read the file. PROPFIND should ignore these. The current behaviour of the WebDAV server when encountering such issues is to immediately stop its walk, and output an http 500. This leads to poor behaviour with the builtin golang server, since the walk has likely already written out its status header as part of serving the previously walked files' properties. The server closes the response, also emitting an error log. While the error log is noisy, the closed response is truly an issue: the xml returned to the client is invalid, which means that the response is useless. It is not unreasonable to expect that a directory shared using WebDAV has files which cannot be read for the reasons given above. The shared directory becomes useless with the current behavior. Rather than making directories with unreadable files useless, skip over anything that is bad. A more nuanced solution to this problem could likely involve indicating that the requested properties have problems, or buffering the response prior to returning the failure. However, this change is simple and a move in the right direction. Fixes golang/go#16195 Fixes golang/go#43782 Change-Id: I065e4c90f7ef797709e5e81e7318c3eafae6db71 GitHub-Last-Rev: d56917e02885fb4151c0d6d8303be3e70dd4aa7a GitHub-Pull-Request: golang/net#91 Reviewed-on: https://go-review.googlesource.com/c/net/+/285752 Reviewed-by: Nigel Tao <nigeltao@golang.org> Reviewed-by: Nigel Tao (INACTIVE; USE @golang.org INSTEAD) <nigeltao@google.com> Run-TryBot: Nigel Tao <nigeltao@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Matthew Holt <matthew.holt@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
PROPFIND can walk through directories, retrieving information about
each file. Unfortunately, the filesystem may deny access to the WebDAV
server for various reasons, such as the file truly not being readable
(e.g. a broken symlink), or because the server does not have permission
to read the file. PROPFIND should ignore these.
The current behaviour of the WebDAV server when encountering such issues
is to immediately stop its walk, and output an http 500. This leads to
poor behaviour with the builtin golang server, since the walk has likely
already written out its status header as part of serving the previously
walked files' properties. The server closes the response, also emitting
an error log.
While the error log is noisy, the closed response is truly an issue: the
xml returned to the client is invalid, which means that the response is
useless. It is not unreasonable to expect that a directory shared using
WebDAV has files which cannot be read for the reasons given above. The
shared directory becomes useless with the current behavior.
Rather than making directories with unreadable files useless, skip over
anything that is bad. A more nuanced solution to this problem could
likely involve indicating that the requested properties have problems,
or buffering the response prior to returning the failure. However, this
change is simple and a move in the right direction.
Fixes golang/go#16195
Fixes golang/go#43782