[18.0.x] WebDAV MOVE: file_put_contents() is a directory

[killerbees19]

Steps to reproduce:

• Create an empty directory with Nextcloud.
• Use the chunk upload feature via WebDAV API.
• Try to move the chunks to the existing directory…

MOVE /remote.php/dav/uploads/username/chunk/.file
Destination: https://hostname/remote.php/dav/files/username/directory

Result:

• No HTTP/WebDAV error
• Chunk files deleted
• Uploaded file lost
• PHP warning…

file_put_contents(data/username/files/directory): failed to open stream: Is a directory
  at lib/private/Files/Storage/Local.php#503

I know, it's a bad idea to move an upload to a directory, but THAT shouldn't happen? 😕

I would expect a HTTP/WebDAV error, without a PHP warning and without all chunks getting lost.

[kesselb]

Good suggestion 👍

server/apps/dav/lib/Upload/ChunkingPlugin.php

Lines 49 to 58 in fd372a3

	function beforeMove($sourcePath, $destination) {
	$this->sourceNode = $this->server->tree->getNodeForPath($sourcePath);
	if (!$this->sourceNode instanceof FutureFile) {
	// skip handling as the source is not a chunked FutureFile
	return;
	}
	$this->verifySize();
	return $this->performMove($sourcePath, $destination);
	}

Here is the code in question. A check if the destination is a valid path to a file (and not a directory) is indeed missing.

		if ($this->server->tree->nodeExists($destination)) {
			/** @var INode $destinationNode */
			$destinationNode = $this->server->tree->getNodeForPath($destination);
			if ($destinationNode instanceof Directory) {
				throw new BadRequest("The given destination $destination is a directory.");
			}
		}

Probably a check like that.

cc @rullzer @icewind1991

[rullzer]

Looks sane @kesselb please shoot in a PR

[kesselb]

I'm not able to reproduce the PHP warning with Nextcloud 19 and PHP 7.4. Moving the chunked files to previously created folder returns 500er status and the message below:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception</s:exception>
  <s:message>Error while copying file to target location (copied bytes: 0, expected filesize: 1936016 )</s:message>
</d:error>

I'm using the script below to upload a image in chunks.

<?php

define('CHUNK_SIZE', 2000000);

$davUrl = 'https://nextcloud.test/remote.php/dav/';
$data = file_get_contents(__DIR__ . '/4k-wallpaper-background-beautiful-853199.jpg');
$name = 'test-upload-' . random_int(1000, 2000);

$ch = curl_init();

// folder for chunks
curl_setopt($ch, CURLOPT_URL, $davUrl . 'uploads/admin/' . $name);
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'MKCOL');
curl_setopt($ch, CURLOPT_USERPWD, 'admin:admin');
curl_exec($ch);

// destination folder we use later
curl_setopt($ch, CURLOPT_URL, $davUrl . 'files/admin/destination-' . $name);
curl_exec($ch);

$chunks = str_split($data, CHUNK_SIZE);
$start = 0;
$end = CHUNK_SIZE;

foreach ($chunks as $key => $chunk) {
	// upload chunks
	curl_setopt($ch, CURLOPT_URL, $davUrl . 'uploads/admin/' . $name . '/' . $start . '-' . $end);
	curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'PUT');
	curl_setopt($ch, CURLOPT_POSTFIELDS, $chunk);
	curl_exec($ch);
	$start = 1 + $end;
	$end = CHUNK_SIZE + $start;
}

// try to move the chunk and use the folder as destination
curl_setopt($ch, CURLOPT_URL, $davUrl . 'uploads/admin/' . $name . '/.file');
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'MOVE');
curl_setopt($ch, CURLOPT_HTTPHEADER, [
	'Destination:' . $davUrl . 'files/admin/destination-' . $name,
	//'Destination:' . $davUrl . 'files/admin/' . $name . '.jpg', // that should work
]);
curl_exec($ch);

@killerbees19 Any suggestion how to trigger the file_put_contents warning? 🤔

[killerbees19]

Maybe it's already fixed in v19? 🤔

I'll try to reproduce it with a simple test script. Please be patient, my free time is currently very limited and I'm still recovering from a short disease.

[killerbees19]

Ok, I've tested it again with a simple shell script.

🗸 No HTTP/WebDAV error: Nope, HTTP error 500 and WebDAV error message as XML.
🗸 Chunk files deleted / Uploaded file lost: Nope, looks like a bug in my real application.
⍻ PHP warning: Still visible at nextcloud.log, but that's not a real problem…

I'm unable to fully reproduce it at the moment. Sorry for the noise! :-(

/closed

[kesselb]

Hmm. I would like to keep this open ;) We should (if possible) check that the destination is not a directory. Would you mind to use my patch and check if the php warning is gone? Also the error message should be different. Thanks in advance 👍

[killerbees19]

@kesselb With a small modification… YES! 🥳

--- a/apps/dav/lib/Upload/ChunkingPlugin.php	2020-03-24 16:33:43.393789565 +0100
+++ b/apps/dav/lib/Upload/ChunkingPlugin.php	2020-04-14 03:23:16.309796205 +0200
@@ -53,6 +53,14 @@ class ChunkingPlugin extends ServerPlugi
 			return;
 		}
 
+		if ($this->server->tree->nodeExists($destination)) {
+			/** @var INode $destinationNode */
+			$destinationNode = $this->server->tree->getNodeForPath($destination);
+			if ($destinationNode instanceof \OCA\DAV\Connector\Sabre\Directory) {
+				throw new BadRequest("The given destination $destination is a directory.");
+			}
+		}
+
 		$this->verifySize();
 		return $this->performMove($sourcePath, $destination);
 	}

Because Directory is in a different namespace, we're in OCA\DAV\Upload. I know, it'd be much better to import the class with an use statement at the top, but it was easier to test it directly.

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\BadRequest</s:exception>
  <s:message>The given destination files/user/example is a directory.</s:message>
</d:error>

The only log entry is about the BadRequest exception.