Verify that destination is not a directory.

Otherwise file_put_contents will fail later. Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
nextcloud · Apr 14, 2020 · 2d61d9e · 2d61d9e
1 parent 09bb8ac
commit 2d61d9e
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 5 deletions.
diff --git a/apps/dav/lib/Upload/ChunkingPlugin.php b/apps/dav/lib/Upload/ChunkingPlugin.php
@@ -23,7 +23,10 @@
 
 namespace OCA\DAV\Upload;
 
+use OCA\DAV\Connector\Sabre\Directory;
 use Sabre\DAV\Exception\BadRequest;
+use Sabre\DAV\Exception\NotFound;
+use Sabre\DAV\INode;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
 
@@ -45,6 +48,9 @@ public function initialize(Server $server) {
 	/**
 	 * @param string $sourcePath source path
 	 * @param string $destination destination path
+	 * @return bool|void
+	 * @throws BadRequest
+	 * @throws NotFound
 	 */
 	public function beforeMove($sourcePath, $destination) {
 		$this->sourceNode = $this->server->tree->getNodeForPath($sourcePath);
@@ -53,6 +59,16 @@ public function beforeMove($sourcePath, $destination) {
 			return;
 		}
 
+		try {
+			/** @var INode $destinationNode */
+			$destinationNode = $this->server->tree->getNodeForPath($destination);
+			if ($destinationNode instanceof Directory) {
+				throw new BadRequest("The given destination $destination is a directory.");
+			}
+		} catch (NotFound $e) {
+			// If the destination does not exist yet it's not a directory either ;)
+		}
+
 		$this->verifySize();
 		return $this->performMove($sourcePath, $destination);
 	}

diff --git a/apps/dav/tests/unit/Upload/ChunkingPluginTest.php b/apps/dav/tests/unit/Upload/ChunkingPluginTest.php
@@ -27,6 +27,7 @@
 use OCA\DAV\Connector\Sabre\Directory;
 use OCA\DAV\Upload\ChunkingPlugin;
 use OCA\DAV\Upload\FutureFile;
+use Sabre\DAV\Exception\NotFound;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
 use Test\TestCase;
@@ -77,7 +78,7 @@ protected function setUp(): void {
 	public function testBeforeMoveFutureFileSkip() {
 		$node = $this->createMock(Directory::class);
 
-		$this->tree->expects($this->any())
+		$this->tree->expects($this->at(0))
 			->method('getNodeForPath')
 			->with('source')
 			->willReturn($node);
@@ -87,16 +88,39 @@ public function testBeforeMoveFutureFileSkip() {
 		$this->assertNull($this->plugin->beforeMove('source', 'target'));
 	}
 
+	public function testBeforeMoveDestinationIsDirectory() {
+		$this->expectException(\Sabre\DAV\Exception\BadRequest::class);
+		$this->expectExceptionMessage('The given destination target is a directory.');
+
+		$sourceNode = $this->createMock(FutureFile::class);
+		$targetNode = $this->createMock(Directory::class);
+
+		$this->tree->expects($this->at(0))
+			->method('getNodeForPath')
+			->with('source')
+			->willReturn($sourceNode);
+		$this->tree->expects($this->at(1))
+			->method('getNodeForPath')
+			->with('target')
+			->willReturn($targetNode);
+
+		$this->assertNull($this->plugin->beforeMove('source', 'target'));
+	}
+
 	public function testBeforeMoveFutureFileSkipNonExisting() {
 		$sourceNode = $this->createMock(FutureFile::class);
 		$sourceNode->expects($this->once())
 			->method('getSize')
 			->willReturn(4);
 
-		$this->tree->expects($this->any())
+		$this->tree->expects($this->at(0))
 			->method('getNodeForPath')
 			->with('source')
 			->willReturn($sourceNode);
+		$this->tree->expects($this->at(1))
+			->method('getNodeForPath')
+			->with('target')
+			->willThrowException(new NotFound());
 		$this->tree->expects($this->any())
 			->method('nodeExists')
 			->with('target')
@@ -117,10 +141,14 @@ public function testBeforeMoveFutureFileMoveIt() {
 			->method('getSize')
 			->willReturn(4);
 
-		$this->tree->expects($this->any())
+		$this->tree->expects($this->at(0))
 			->method('getNodeForPath')
 			->with('source')
 			->willReturn($sourceNode);
+		$this->tree->expects($this->at(1))
+			->method('getNodeForPath')
+			->with('target')
+			->willThrowException(new NotFound());
 		$this->tree->expects($this->any())
 			->method('nodeExists')
 			->with('target')
@@ -143,7 +171,7 @@ public function testBeforeMoveFutureFileMoveIt() {
 		$this->assertFalse($this->plugin->beforeMove('source', 'target'));
 	}
 
-	
+
 	public function testBeforeMoveSizeIsWrong() {
 		$this->expectException(\Sabre\DAV\Exception\BadRequest::class);
 		$this->expectExceptionMessage('Chunks on server do not sum up to 4 but to 3 bytes');
@@ -153,10 +181,14 @@ public function testBeforeMoveSizeIsWrong() {
 			->method('getSize')
 			->willReturn(3);
 
-		$this->tree->expects($this->any())
+		$this->tree->expects($this->at(0))
 			->method('getNodeForPath')
 			->with('source')
 			->willReturn($sourceNode);
+		$this->tree->expects($this->at(1))
+			->method('getNodeForPath')
+			->with('target')
+			->willThrowException(new NotFound());
 		$this->request->expects($this->once())
 			->method('getHeader')
 			->with('OC-Total-Length')