mholt / caddy-webdav

WebDAV handler module for Caddy
Apache License 2.0
222 stars 23 forks source link

Permissions error with one file prevents listing the whole directory #16

Closed mholt closed 1 year ago

mholt commented 3 years ago

I think this is a bug or limitation in the upstream webdav library, but I noticed that when listing the contents of a directory, the whole request returns an error if just one of the files has a permissions error. Instead, that file should not be in the list, and the request should still succeed, listing the rest of the files in the directory.

occlu commented 3 years ago

I encountered the same problem under Windows 10. My webdav using caddy works perfectly fine until it try to gain permission on a file/dir that has a high permission requirement.

For exemple, I cannot point root to my second internal hard drive D:\ because it does not have permission to read D:\System Volume Information which is a protected OS file present at the root of every mounted drive.

ERROR http.handlers.webdav internal handler error {"error": "open D:\\System Volume Information: Access is denied.", ...}}}

But if I point root to D:\Sharing everything works fine because there are no files that require special permissions.

sillikk commented 2 years ago

Same issue with Windows 11: the "System Volume Information" folder prevents the directory listing when pointing to a drive. I think the default behavior should be as in Windows explorer: the upper folder listing should work, but it should return an error only if you try to open the system folder for which you don't have the right. It seems the Webdav module tries to aaccess the sub-folder content or some other property for the upper folder listing when all it needs is the name, or if name is not accessible, just ignore it. What I get for the moment, is a partial listing of the folder, ending at the previous sub-folder (sorted alphabetically).

mholt commented 2 years ago

I just upgraded the webdav package and our other dependencies for the first time in a couple years; someone want to try and see if things have been fixed?

sillikk commented 2 years ago

I tried downloading Caddy with the webdav plugin again from the Caddy website and it still doesn't seem to work. How can I see which version I'm using? I'm not sure this is the proper way to get the latest update?

mholt commented 2 years ago

@sillikk If the version is implicit (not specified in the URL), then the build is probably cached on an old commit. (That's a known issue that I'm not quite sure how to solve at the moment.)

You can either specify the latest version (branch name like master or a commit SHA), or use xcaddy to build from source yourself.

Edit: I just cleared the build cache anyway, so you could just try the download again.

sillikk commented 2 years ago

I tried entering "master" or the latest build SHA into the version field on the caddy download page, but the download file is exactly the same. So it seems it doesn't change anything. I'll look into xcaddy when I find the time but that seems a bit more complicated...

mholt commented 2 years ago

@sillikk Odd, that should be impossible. It worked for me. Make sure you're running the newly downloaded binary.

If you run ./caddy list-modules --versions you should see in the output:

http.handlers.webdav v0.0.0-20220728174539-14bd56b1afc9

  Non-standard modules: 1

Note the version string is a commit from yesterday.

sillikk commented 2 years ago

I have the same: http.handlers.webdav v0.0.0-20220728174539-14bd56b1afc9

So it seems the latest version doesn't fix the issue, I'm afraid.

mholt commented 2 years ago

@sillikk The SHA of the binary should definitely be different. Can you confirm the two binaries have different checksums, and then verify you are performing the test using the binary with the new checksum?

(Again, it's quite possible the upstream hasn't fixed this, but there seems to be something non-sequitur here that I want to make sure we are clear on first, so we can be precise.)

sillikk commented 2 years ago

I'm not sure how to get the checksum of the binary. But I'm definitely using the latest version I just downloaded, with the version I just showed above.

mholt commented 2 years ago

I'm confused, you said:

the download file is exactly the same

how do you know that without hashing it?

sillikk commented 2 years ago

Sorry, it is exactly the same size but I didn't hash it.

mholt commented 2 years ago

Ok. In that case I am not sure then. Still seems like this bug should be reported upstream.

sillikk commented 2 years ago

Where is upstream? On the Caddy repository? Will you report the bug or should I?

mholt commented 2 years ago

@sillikk With Go, since I believe the bug is in https://pkg.go.dev/golang.org/x/net/webdav.

I only scanned the code very quickly, but I believe the error would be somewhere around here: https://cs.opensource.google/go/x/net/+/c7608f3a:webdav/file.go;l=791;drc=c7608f3a8462fd23f95e21be49b40c7b9c20c74a;bpv=0;bpt=1 -- or really, any of the Open/Read/Stat/Readdir operations (etc) that might return a permissions error are possible causes of this behavior.

I haven't had a chance to file a bug myself, so it might be faster if you do it :sweat_smile:

sillikk commented 2 years ago

Well I couldn't find a way to file a bug on the two links you mentioned. Besides, I think you will probably be better informed than me to put a correct description I guess. All I could do is point to this issue on Github. Could you file the bug when you have time? Thank you.

mholt commented 2 years ago

Sure, but I won't have time for a while. Bug report link is here: golang/go/issues/new/choose

heimoshuiyu commented 2 years ago

There is a PR https://github.com/golang/net/pull/91 but still under review, see also https://go-review.googlesource.com/c/net/+/285752/

I cherry-pick that commit to my fork. Add one line replace golang.org/x/net => github.com/heimoshuiyu/net v0.0.0-20220813162333-9dad79f931f2 to the end of caddy's (not caddy-webdav) go.mod can temporarily solve this problem

mholt commented 2 years ago

Ah that's great news!! Thanks for the update!

mholt commented 2 years ago

@heimoshuiyu Well, while we wait for Go to review and merge the CL, I've gone ahead and pushed the replacement to our go.mod in the meantime.