Merge pull request #6714 from owncloud/files-newfileinvalidcharsfix

Added extra checks for invalid file chars in newfile.php and newfolder.php

apps/files/ajax/newfile.php

@@ -50,16 +50,22 @@ $l10n = \OC_L10n::get('files');
 $result = array(
 	'success' 	=> false,
 	'data'		=> NULL
-	);
+);
+$trimmedFileName = trim($filename);
 
-if(trim($filename) === '') {
+if($trimmedFileName === '') {
 	$result['data'] = array('message' => (string)$l10n->t('File name cannot be empty.'));
 	OCP\JSON::error($result);
 	exit();
 }
+if($trimmedFileName === '.' || $trimmedFileName === '..') {
+	$result['data'] = array('message' => (string)$l10n->t('"%s" is an invalid file name.', $trimmedFileName));
+	OCP\JSON::error($result);
+	exit();
+}
 
-if(strpos($filename, '/') !== false) {
-	$result['data'] = array('message' => (string)$l10n->t('File name must not contain "/". Please choose a different name.'));
+if(!OCP\Util::isValidFileName($filename)) {
+	$result['data'] = array('message' => (string)$l10n->t("Invalid name, '\\', '/', '<', '>', ':', '\"', '|', '?' and '*' are not allowed."));
 	OCP\JSON::error($result);
 	exit();
 }

apps/files/ajax/newfolder.php

@@ -23,8 +23,8 @@ if(trim($foldername) === '') {
 	exit();
 }
 
-if(strpos($foldername, '/') !== false) {
-	$result['data'] = array('message' => $l10n->t('Folder name must not contain "/". Please choose a different name.'));
+if(!OCP\Util::isValidFileName($foldername)) {
+	$result['data'] = array('message' => (string)$l10n->t("Invalid name, '\\', '/', '<', '>', ':', '\"', '|', '?' and '*' are not allowed."));
 	OCP\JSON::error($result);
 	exit();
 }

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();

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;
 

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('');

lib/private/util.php

@@ -1155,4 +1155,25 @@ class OC_Util {
 		}
 		return $version;
 	}
+
+	/**
+	 * Returns whether the given file name is valid
+	 * @param $file string file name to check
+	 * @return bool true if the file name is valid, false otherwise
+	 */
+	public static function isValidFileName($file) {
+		$trimmed = trim($file);
+		if ($trimmed === '') {
+			return false;
+		}
+		if ($trimmed === '.' || $trimmed === '..') {
+			return false;
+		}
+		foreach (str_split($trimmed) as $char) {
+			if (strpos(\OCP\FILENAME_INVALID_CHARS, $char) !== false) {
+				return false;
+			}
+		}
+		return true;
+	}
 }

lib/public/constants.php

@@ -35,3 +35,6 @@ const PERMISSION_UPDATE = 2;
 const PERMISSION_DELETE = 8;
 const PERMISSION_SHARE = 16;
 const PERMISSION_ALL = 31;
+
+const FILENAME_INVALID_CHARS = "\\/<>:\"|?*\n";
+

lib/public/util.php

@@ -486,4 +486,13 @@ class Util {
 	public static function uploadLimit() {
 		return \OC_Helper::uploadLimit();
 	}
+
+	/**
+	 * Returns whether the given file name is valid
+	 * @param $file string file name to check
+	 * @return bool true if the file name is valid, false otherwise
+	 */
+	public static function isValidFileName($file) {
+		return \OC_Util::isValidFileName($file);
+	}
 }

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
 	 */

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)),
 		);
 	}
 

tests/lib/util.php

@@ -170,4 +170,52 @@ class Test_Util extends PHPUnit_Framework_TestCase {
 			array('442aa682de2a64db1e010f50e60fd9c9', 'local::C:\Users\ADMINI~1\AppData\Local\Temp\2/442aa682de2a64db1e010f50e60fd9c9/')
 		);
 	}
+
+	/**
+	 * @dataProvider filenameValidationProvider
+	 */
+	public function testFilenameValidation($file, $valid) {
+		// private API
+		$this->assertEquals($valid, \OC_Util::isValidFileName($file));
+		// public API
+		$this->assertEquals($valid, \OCP\Util::isValidFileName($file));
+	}
+
+	public function filenameValidationProvider() {
+		return array(
+			// valid names
+			array('boringname', true),
+			array('something.with.extension', true),
+			array('now with spaces', true),
+			array('.a', true),
+			array('..a', true),
+			array('.dotfile', true),
+			array('single\'quote', true),
+			array('  spaces before', true),
+			array('spaces after   ', true),
+			array('allowed chars including the crazy ones $%&_-^@!,()[]{}=;#', true),
+			array('汉字也能用', true),
+			array('und Ümläüte sind auch willkommen', true),
+			// disallowed names
+			array('', false),
+			array('     ', false),
+			array('.', false),
+			array('..', false),
+			array('back\\slash', false),
+			array('sl/ash', false),
+			array('lt<lt', false),
+			array('gt>gt', false),
+			array('col:on', false),
+			array('double"quote', false),
+			array('pi|pe', false),
+			array('dont?ask?questions?', false),
+			array('super*star', false),
+			array('new\nline', false),
+			// better disallow these to avoid unexpected trimming to have side effects
+			array(' ..', false),
+			array('.. ', false),
+			array('. ', false),
+			array(' .', false),
+		);
+	}
 }