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

x/net/webdav: fails on broken links #16195

Open
enoodle opened this issue Jun 27, 2016 · 4 comments · May be fixed by golang/net#91
Open

x/net/webdav: fails on broken links #16195

enoodle opened this issue Jun 27, 2016 · 4 comments · May be fixed by golang/net#91
Milestone

Comments

@enoodle
Copy link

@enoodle enoodle commented Jun 27, 2016

When trying to list a directory that contains a broken link I get a non informative error:

$ cadaver webdav://localhost:5555/api/v1/content                                                                                                                                         
dav:/api/v1/content/> ls etc
Listing collection `/api/v1/content/etc/': failed:
XML parse error at line 1: junk after document element

This is what I get from the webdav server:

2016/06/27 14:52:42 http: multiple response.WriteHeader calls

The error comes from file.go [1] where it calls stat on the broken link file but I think that maybe it should be handled in webdav.go walkFn [2].

I am not sure myself what should be printed / returned but I would expect to get the full directory listing (ie webdav shouldn't crush on this in my opinion) with this link marked a broken somehow .

[1] https://github.com/golang/net/blob/master/webdav/file.go#L779
[2] https://github.com/golang/net/blob/master/webdav/webdav.go#L527

/cc @simon3z

@enoodle enoodle changed the title x/new/webdav Fails on broken links x/net/webdav Fails on broken linkst Jun 27, 2016
@enoodle enoodle changed the title x/net/webdav Fails on broken linkst x/net/webdav Fails on broken links Jun 27, 2016
@quentinmit quentinmit changed the title x/net/webdav Fails on broken links x/net/webdav: fails on broken links Jun 27, 2016
@quentinmit quentinmit added this to the Unreleased milestone Jun 27, 2016
@skritchz
Copy link

@skritchz skritchz commented Mar 15, 2017

I just encounter this issue to. I fixed it locally by just returning nil when getting an error here https://github.com/golang/net/blob/master/webdav/webdav.go#L557.

I tested the same use case with Apache implementation and in the case of a broken symblink, it is silently discarded.

I don't know if it is a good enough solution but today's behavior is incorrect too.
Here is a wireshark trace of the faulty response

Host: 10.21.59.204
Depth: 1
Content-Type: application/xml
Apply-To-Redirect-Ref: T
Accept-Encoding: gzip, deflate
User-Agent: gvfs/1.28.2
Accept-Language: en-us, en;q=0.9
Connection: Keep-Alive
Content-Length: 235

<?xml version="1.0" encoding="utf-8" ?>
 <D:propfind xmlns:D="DAV:">
  <D:prop>
<D:creationdate/>
<D:displayname/>
<D:getcontentlength/>
<D:getcontenttype/>
<D:getetag/>
<D:getlastmodified/>
<D:resourcetype/>
  </D:prop>
 </D:propfind>HTTP/1.1 207 status code 207
Content-Type: text/xml; charset=utf-8
Date: Wed, 15 Mar 2017 09:00:40 GMT
Content-Length: 610

<?xml version="1.0" encoding="UTF-8"?><D:multistatus xmlns:D="DAV:"><D:response><D:href>/dav</D:href><D:propstat><D:prop><D:displayname></D:displayname><D:getlastmodified>Wed, 15 Mar 2017 08:59:55 GMT</D:getlastmodified><D:resourcetype><D:collection xmlns:D="DAV:"/></D:resourcetype></D:prop><D:status>HTTP/1.1 200 OK</D:status></D:propstat><D:propstat><D:prop><D:creationdate></D:creationdate><D:getcontentlength></D:getcontentlength><D:getcontenttype></D:getcontenttype><D:getetag></D:getetag></D:prop><D:status>HTTP/1.1 404 Not Found</D:status></D:propstat></D:response></D:multistatus>Internal Server Error

In the Webdav Spec it is said that

In the case of allprop and propname, if a principal does not have the
right to know whether a particular property exists then the property
should be silently excluded from the response.

I don't know if this apply in our use case

@bingliu221
Copy link

@bingliu221 bingliu221 commented Jul 24, 2020

I think a 4XX status code should be return in the case of accessing broken links, more specifically, 403 Forbidden for links linked to out of permission targets, and 404 Not Found for links linked to nonexistent targets.

Currently errors returned by FileSystem.OpenFileor File.Stat transfer into http.StatusInternalServerError and cause "http: superfluous response.WriteHeader call from golang.org/x/net/webdav.(*Handler).ServeHTTP (webdav.go:74)"

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 21, 2020

Change https://golang.org/cl/249797 mentions this issue: webdav: ignore os.PathError in PROPFIND

klarose added a commit to Agilicus/golang-net that referenced this issue Jan 20, 2021
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
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285752 mentions this issue: webdav: ignore path and perm errors in PROPFIND

drakkan added a commit to drakkan/net that referenced this issue Mar 24, 2021
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
drakkan added a commit to drakkan/net that referenced this issue Mar 28, 2021
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
drakkan added a commit to drakkan/net that referenced this issue Apr 5, 2021
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
drakkan added a commit to drakkan/net that referenced this issue Apr 7, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants