Opened 13 years ago

Closed 13 years ago

Last modified 3 years ago

#1807 enhancement closed fixed (fixed)

Exception handling improvements in web2/dav

Reported by: troy@… Owned by:
Priority: normal Milestone:
Component: web2.dav Keywords:
Cc: Branch:
Author:

Description

The following patch improves the handling of common exceptions in web2/dav. The current implementation will return an HTTP status 500 ("Internal server error") on virtually any exception. This patch allows the DAV server code to better handle "pathological" DAV requests (eg. deleting a file or directory not owned by the DAV server's userid) by returning the appropriate HTTP codes (eg. 403/FORBIDDEN for attempts to delete other users files or 404/NOT_FOUND to accesses to nonexistent files).


Index: twisted/web2/dav/fileop.py
===================================================================
--- twisted/web2/dav/fileop.py  (revision 17118)
+++ twisted/web2/dav/fileop.py  (working copy)
@@ -114,8 +114,21 @@
         for dir, subdirs, files in os.walk(filepath.path, topdown=False):
             for filename in files:
                 path = os.path.join(dir, filename)
-                try: os.remove(path)
-                except: errors.add(path, Failure())
+                try:
+                    os.remove(path)
+                except (IOError, OSError), (error,msg):
+                    # this is likely a perm. denied
+                    # it should return a HTTP code 403, *NOT* a 500
+                    import errno
+                    print "%s (errno=%d) on remove(%s)" % msg,error,filepath.path
+                    if error==errno.EPERM or error==errno.EACCES:
+                        return responsecode.FORBIDDEN
+                    elif error==errno.ENOENT:
+                        return responsecode.NOT_FOUND
+                    else:
+                        errors.add(path, Failure())
+                except:
+                    errors.add(path, Failure())

             for subdir in subdirs:
                 path = os.path.join(dir, subdir)
@@ -126,7 +139,19 @@
                     try: os.rmdir(path)
                     except: errors.add(path, Failure())

-        try: os.rmdir(filepath.path)
+        try:
+            os.rmdir(filepath.path)
+        except (IOError,OSError), (error,msg):
+            # this is likely a perm. denied
+            # it should return a HTTP code 403, *NOT* a 500
+            import errno
+            print "%s (errno=%d) on rmdir(%s)" % (msg,error,filepath.path)
+            if error==errno.EPERM or error==errno.EACCES:
+                return responsecode.FORBIDDEN
+            elif error==errno.ENOENT:
+                return responsecode.NOT_FOUND
+            else:
+                return statusForFailure(Failure(), "deleting directory: %s" % (filepath.path,))
         except:
             return statusForFailure(Failure(), "deleting directory: %s" % (filepath.path,))

@@ -139,6 +164,17 @@
         log.msg("Deleting file %s" % (filepath.path,))
         try:
             os.remove(filepath.path)
+        except (IOError,OSError), (error,msg):
+            # this is likely a perm. denied
+            # it should return a HTTP code 403, *NOT* a 500
+            import errno
+            print "%s (errno=%d) on remove(%s)" % (msg,error,filepath.path)
+            if error==errno.EPERM or error==errno.EACCES:
+                return responsecode.FORBIDDEN
+            elif error==errno.ENOENT:
+                return responsecode.NOT_FOUND
+            else:
+                return statusForFailure(Failure(), "deleting directory: %s" % (filepath.path,))
         except:
             return statusForFailure(Failure(), "deleting file: %s" % (filepath.path,))

@@ -426,6 +462,17 @@
         os.mkdir(filepath.path)
         # Restat filepath because we modified it
         filepath.restat(False)
+    except (IOError,OSError), (error,msg):
+        # this is likely a perm. denied
+        # it should return a HTTP code 403, *NOT* a 500
+        import errno
+        print "%s (errno=%d) on mkdir(%s)" % (msg,error,filepath.path)
+        if error==errno.EPERM or error==errno.EACCES:
+            return responsecode.FORBIDDEN
+        elif error==errno.ENOENT:
+            return responsecode.NOT_FOUND
+        else:
+            return statusForFailure(Failure(), "creating directory in MKCOL: %s" % (filepath.path,))
     except:
         return statusForFailure(Failure(), "creating directory in MKCOL: %s" %
(filepath.path,))

Index: twisted/web2/static.py
===================================================================
--- twisted/web2/static.py      (revision 17118)
+++ twisted/web2/static.py      (working copy)
@@ -322,7 +322,12 @@
         """
         children = self.putChildren.keys()
         if self.fp.isdir():
-            children += [c for c in self.fp.listdir() if c not in children]
+            try:
+                children += [c for c in self.fp.listdir() if c not in children]+            except OSError, errmsg:
+                # this is likely a perm. denied on traversing a directory
+                # it can be ignored
+                print "\"%s\"" % errmsg
         return children

     def locateChild(self, req, segments):

Attachments (2)

dav-fileop-fixes-update.diff (5.1 KB) - added by troy@… 13 years ago.
updated patch
status.patch (459 bytes) - added by rfmohr@… 13 years ago.
statusForFailure function patch

Download all attachments as: .zip

Change History (13)

comment:1 Changed 13 years ago by David Reid

Owner: changed from jknight to Wilfredo Sánchez Vega

It's not an issue of a particular user not being able to perform these actions, it's the user the server is running as not being able to perform these actions. I think a 500 is the correct response, I'll assign this to wsanchez and ask him what he thinks.

comment:2 Changed 13 years ago by Glyph

Either way this behavior is supposed to go, some unit tests would be nice.

comment:3 Changed 13 years ago by troy@…

According to the WebDAV RFC, an MKCOL should return a 403/FORBIDDEN if the server does not allow the creation of a collection in the specified location in the namespace; however, in the current implementation, a 500 would be returned instead. I submit that DELETE and RMCOL actions should have similar behavior. (Furthermore, I would argue that a server should fail gracefully rather than dump a stack trace in its log file and send the user an internal server error response every time it's asked to perform an action it's not allowed to do. Currently, the latter happens.)

To let you know the motivation of these changes, I work in a scientific computing center, and we have a need to give users remote access to their existing files via a protocol such as DAV over HTTPS. It is not possible in such an environment for all the files being served by DAV to be owned by a single user, and we'd certainly like to have a DAV server that's well behaved in such an environment. The patch I submitted isn't perfect (it doesn't mark read-only or unreadable collections as such for one thing), but it's a start.

As to unit tests, I'm currently working through how to implement one that doesn't require an external tool like cadaver. Basically, the test I used was to fire up a simple DAV server as a non-root user exporting the root filesystem of a Linux box:

# To run:  twistd -noy DAVservice.py
from twisted.web2.dav import static
from twisted.web2 import server, channel
from twisted.application import service, strports

root=static.DAVFile("/")
site = server.Site(root)

DAVService = strports.service("tcp:8765",channel.HTTPFactory(site))
DAVService.setServiceParent(application)

Then I connected to it with cadaver and did things like the following:

cadaver http://localhost:8765/
ls
rm /etc/passwd
rmcol /dev
mkcol /this_should_not_work

All these should give something like a 403/FORBIDDEN message; however, with the current code they will all give 500/internal server error messages.

comment:4 Changed 13 years ago by troy@…

Small correction to the DAVservice.py script above:

# To run:  twistd -noy DAVservice.py
from twisted.web2.dav import static
from twisted.web2 import server, channel
from twisted.application import service, strports

root=static.DAVFile("/")
site = server.Site(root)

application = service.Application("DAVinci")

DAVService = strports.service("tcp:8765",channel.HTTPFactory(site))
DAVService.setServiceParent(application)

Changed 13 years ago by troy@…

updated patch

comment:5 Changed 13 years ago by rfmohr@…

I did some poking around, and it looks like most (maybe all?) of the exceptions can be handled properly just by modifiying the statusForFailure function in twisted.web2.dav.http. This function catches IOError's and tries to translate them into reasonable return codes like "FORBIDDEN" or "NOT_FOUND". The problem is that all the exceptions that Troy mentioned as the result of OSError exceptions. Catching OSError along with IOError and checking for EPERM errors seems to do the trick. (At least in all the cases I tested.)

Here are the changes:

Index: twisted/web2/dav/http.py
===================================================================
--- twisted/web2/dav/http.py    (revision 42)
+++ twisted/web2/dav/http.py    (working copy)
@@ -262,9 +262,9 @@
         if what is not None:
             log.msg("%s while %s" % (err, what))

-    if failure.check(IOError):
+    if failure.check(IOError, OSError):
         e = failure.value[0]
-        if e == errno.EACCES:
+        if e == errno.EACCES or e == errno.EPERM:
             msg("Permission denied")
             return responsecode.FORBIDDEN
         elif e == errno.ENOSPC:

I have also attached this as a patch.

Changed 13 years ago by rfmohr@…

Attachment: status.patch added

statusForFailure function patch

comment:6 Changed 13 years ago by Wilfredo Sánchez Vega

Keywords: review added
Status: newassigned

comment:7 Changed 13 years ago by Wilfredo Sánchez Vega

Component: web2web2.dav

comment:8 Changed 13 years ago by Wilfredo Sánchez Vega

The patch in the problem description is incorrect, I think. You don't want to return an error code; you want to add the error code to the list of error codes which will be contained in a multi status response to be returned at the end.

statusForFailure() should be converting system errors into the correct status code, and is the way that I avoid having to duplicate this logic in many places.

Are errno.EPERM and errno.EACCES not always the same thing?

rfmohr's patch looks about right

comment:9 Changed 13 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: assignedclosed

(In [18104]) Some systems raise an OSError with EPERM rather than an IOError with EACCES for filesystem permissions errors. Submitted by rfmohr@… Reviewed by wsanchez Fixes #1807

comment:10 Changed 8 years ago by <automation>

Owner: Wilfredo Sánchez Vega deleted

comment:11 Changed 3 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.