Added file name check in webdav connector

- added file name check for the put, rename and setNames() methods which
  throw a "Bad Request" whenever invalid characters are used
- replaced \OC\Filesystem usage with $this->getFS() to be able to write
  unit tests

diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php
index ed27cef440dc7501e33199c5ca5f813cbb609f41..db0726e135cb011c1dbef7e6782b9886d87b81bc 100644 (file)
--- a/lib/private/connector/sabre/file.php
+++ b/lib/private/connector/sabre/file.php
@@ -58,6 +58,11 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements Sabre_D
                        throw new \Sabre_DAV_Exception_ServiceUnavailable();
                }
 
+               $fileName = basename($this->path);
+               if (!\OCP\Util::isValidFileName($fileName)) {
+                       throw new \Sabre_DAV_Exception_BadRequest();
+               }
+
                // chunked handling
                if (isset($_SERVER['HTTP_OC_CHUNKED'])) {
                        return $this->createFileChunked($data);
@@ -142,15 +147,16 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements Sabre_D
         * @throws Sabre_DAV_Exception_Forbidden
         */
        public function delete() {
+               $fs = $this->getFS();
 
                if ($this->path === 'Shared') {
                        throw new \Sabre_DAV_Exception_Forbidden();
                }
 
-               if (!\OC\Files\Filesystem::isDeletable($this->path)) {
+               if (!$fs->isDeletable($this->path)) {
                        throw new \Sabre_DAV_Exception_Forbidden();
                }
-               \OC\Files\Filesystem::unlink($this->path);
+               $fs->unlink($this->path);
 
                // remove properties
                $this->removeProperties();

diff --git a/lib/private/connector/sabre/node.php b/lib/private/connector/sabre/node.php
index 993aa73faeb15b54c0d9040572b98c49626b6d71..bf7a04f5b13f19a6da1ca3354217a2fbfa331513 100644 (file)
--- a/lib/private/connector/sabre/node.php
+++ b/lib/private/connector/sabre/node.php
@@ -85,19 +85,24 @@ abstract class OC_Connector_Sabre_Node implements Sabre_DAV_INode, Sabre_DAV_IPr
         * @return void
         */
        public function setName($name) {
+               $fs = $this->getFS();
 
                // rename is only allowed if the update privilege is granted
-               if (!\OC\Files\Filesystem::isUpdatable($this->path)) {
+               if (!$fs->isUpdatable($this->path)) {
                        throw new \Sabre_DAV_Exception_Forbidden();
                }
 
                list($parentPath, ) = Sabre_DAV_URLUtil::splitPath($this->path);
                list(, $newName) = Sabre_DAV_URLUtil::splitPath($name);
 
+               if (!\OCP\Util::isValidFileName($newName)) {
+                       throw new \Sabre_DAV_Exception_BadRequest();
+               }
+
                $newPath = $parentPath . '/' . $newName;
                $oldPath = $this->path;
 
-               \OC\Files\Filesystem::rename($this->path, $newPath);
+               $fs->rename($this->path, $newPath);
 
                $this->path = $newPath;
 

diff --git a/lib/private/connector/sabre/objecttree.php b/lib/private/connector/sabre/objecttree.php
index d1e179af2ec5eb468f3c92e8ba6780b70ebb5f49..d2fa425b22c4593161d97273f7bd3f58905c7671 100644 (file)
--- a/lib/private/connector/sabre/objecttree.php
+++ b/lib/private/connector/sabre/objecttree.php
@@ -105,6 +105,11 @@ class ObjectTree extends \Sabre_DAV_ObjectTree {
                        }
                }
 
+               $fileName = basename($destinationPath);
+               if (!\OCP\Util::isValidFileName($fileName)) {
+                       throw new \Sabre_DAV_Exception_BadRequest();
+               }
+
                $renameOkay = $fs->rename($sourcePath, $destinationPath);
                if (!$renameOkay) {
                        throw new \Sabre_DAV_Exception_Forbidden('');

diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php
index e1fed0384c6f7b0678f8c5d1c205adc48d99e79c..50b8711a90dc940dbc7402b8ba40fefbe9e7f355 100644 (file)
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@@ -35,6 +35,46 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase {
                $etag = $file->put('test data');
        }
 
+       /**
+        * @expectedException Sabre_DAV_Exception_BadRequest
+        */
+       public function testSimplePutInvalidChars() {
+               // setup
+               $file = new OC_Connector_Sabre_File('/super*star.txt');
+               $file->fileView = $this->getMock('\OC\Files\View', array('file_put_contents'), array(), '', FALSE);
+               $file->fileView->expects($this->any())->method('file_put_contents')->withAnyParameters()->will($this->returnValue(false));
+
+               // action
+               $etag = $file->put('test data');
+       }
+
+       /**
+        * Test setting name with setName()
+        */
+       public function testSetName() {
+               // setup
+               $file = new OC_Connector_Sabre_File('/test.txt');
+               $file->fileView = $this->getMock('\OC\Files\View', array('isUpdatable'), array(), '', FALSE);
+               $file->fileView->expects($this->any())->method('isUpdatable')->withAnyParameters()->will($this->returnValue(true));
+               $etag = $file->put('test data');
+               $file->setName('/renamed.txt');
+               $this->assertTrue($file->fileView->file_exists('/renamed.txt'));
+               // clean up
+               $file->delete();
+       }
+
+       /**
+        * Test setting name with setName() with invalid chars
+        * @expectedException Sabre_DAV_Exception_BadRequest
+        */
+       public function testSetNameInvalidChars() {
+               // setup
+               $file = new OC_Connector_Sabre_File('/test.txt');
+               $file->fileView = $this->getMock('\OC\Files\View', array('isUpdatable'), array(), '', FALSE);
+               $file->fileView->expects($this->any())->method('isUpdatable')->withAnyParameters()->will($this->returnValue(true));
+               $file->setName('/super*star.txt');
+       }
+
        /**
         * @expectedException Sabre_DAV_Exception_Forbidden
         */

diff --git a/tests/lib/connector/sabre/objecttree.php b/tests/lib/connector/sabre/objecttree.php
index e32f2365f9515ff0e1f045ae328bc8e9ae7f3bd4..fb50c736edd871ec26ad7959a24d786d68c1d935 100644 (file)
--- a/tests/lib/connector/sabre/objecttree.php
+++ b/tests/lib/connector/sabre/objecttree.php
@@ -52,6 +52,20 @@ class ObjectTree extends PHPUnit_Framework_TestCase {
                $this->assertTrue(true);
        }
 
+       /**
+        * @dataProvider moveFailedInvalidCharsProvider
+        * @expectedException Sabre_DAV_Exception_BadRequest
+        */
+       public function testMoveFailedInvalidChars($source, $dest, $updatables, $deletables) {
+               $this->moveTest($source, $dest, $updatables, $deletables);
+       }
+
+       function moveFailedInvalidCharsProvider() {
+               return array(
+                       array('a/b', 'a/c*', array('a' => false, 'a/b' => true, 'a/c*' => false), array()),
+               );
+       }
+
        function moveFailedProvider() {
                return array(
                        array('a/b', 'a/c', array('a' => false, 'a/b' => false, 'a/c' => false), array()),
@@ -66,6 +80,8 @@ class ObjectTree extends PHPUnit_Framework_TestCase {
                return array(
                        array('a/b', 'a/c', array('a' => false, 'a/b' => true, 'a/c' => false), array()),
                        array('a/b', 'b/b', array('a' => true, 'a/b' => true, 'b' => true, 'b/b' => false), array('a/b' => true)),
+                       // older files with special chars can still be renamed to valid names
+                       array('a/b*', 'b/b', array('a' => true, 'a/b*' => true, 'b' => true, 'b/b' => false), array('a/b*' => true)),
                );
        }