webdav: ignore path and perm errors in PROPFIND

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>

Author: klarose

webdav/webdav.go

@@ -13,6 +13,7 @@ import (
 	"net/url"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 	"time"
 )
@@ -535,13 +536,14 @@ func (h *Handler) handlePropfind(w http.ResponseWriter, r *http.Request) (status
 
 	walkFn := func(reqPath string, info os.FileInfo, err error) error {
 		if err != nil {
-			return err
+			return handlePropfindError(err, info)
 		}
+
 		var pstats []Propstat
 		if pf.Propname != nil {
 			pnames, err := propnames(ctx, h.FileSystem, h.LockSystem, reqPath)
 			if err != nil {
-				return err
+				return handlePropfindError(err, info)
 			}
 			pstat := Propstat{Status: http.StatusOK}
 			for _, xmlname := range pnames {
@@ -554,7 +556,7 @@ func (h *Handler) handlePropfind(w http.ResponseWriter, r *http.Request) (status
 			pstats, err = props(ctx, h.FileSystem, h.LockSystem, reqPath, pf.Prop)
 		}
 		if err != nil {
-			return err
+			return handlePropfindError(err, info)
 		}
 		href := path.Join(h.Prefix, reqPath)
 		if href != "/" && info.IsDir() {
@@ -633,6 +635,33 @@ func makePropstatResponse(href string, pstats []Propstat) *response {
 	return &resp
 }
 
+func handlePropfindError(err error, info os.FileInfo) error {
+	var skipResp error = nil
+	if info != nil && info.IsDir() {
+		skipResp = filepath.SkipDir
+	}
+
+	if errors.Is(err, os.ErrPermission) {
+		// If the server cannot recurse into a directory because it is not allowed,
+		// then there is nothing more to say about it. Just skip sending anything.
+		return skipResp
+	}
+
+	if _, ok := err.(*os.PathError); ok {
+		// If the file is just bad, it couldn't be a proper WebDAV resource. Skip it.
+		return skipResp
+	}
+
+	// We need to be careful with other errors: there is no way to abort the xml stream
+	// part way through while returning a valid PROPFIND response. Returning only half
+	// the data would be misleading, but so would be returning results tainted by errors.
+	// The curent behaviour by returning an error here leads to the stream being aborted,
+	// and the parent http server complaining about writing a spurious header. We should
+	// consider further enhancing this error handling to more gracefully fail, or perhaps
+	// buffer the entire response until we've walked the tree.
+	return err
+}
+
 const (
 	infiniteDepth = -1
 	invalidDepth  = -2