Merge pull request #2471 from nextcloud/harden_files_drop

Harden files drop

.drone.yml

@@ -338,6 +338,15 @@ pipeline:
     when:
       matrix:
         TESTS: integration-setup-features
+  integration-filesdrop-features:
+    image: nextcloudci/integration-php7.0:integration-php7.0-2
+    commands:
+      - ./occ maintenance:install --admin-pass=admin
+      - cd build/integration
+      - ./run.sh filesdrop_features/filesdrop.feature
+    when:
+      matrix:
+        TESTS: integration-filesdrop-features
   nodb-codecov:
     image: nextcloudci/php7.0:php7.0-6
     commands:
@@ -382,6 +391,7 @@ matrix:
     - TESTS: integration-webdav-related
     - TESTS: integration-sharees-features
     - TESTS: integration-setup-features
+    - TESTS: integration-filesdrop-features
     - TESTS: jsunit
     - TESTS: check-autoloader
     - TESTS: app-check-code

apps/dav/lib/Files/Sharing/FilesDropPlugin.php

@@ -23,6 +23,7 @@
 namespace OCA\DAV\Files\Sharing;
 
 use OC\Files\View;
+use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
@@ -56,6 +57,7 @@ class FilesDropPlugin extends ServerPlugin {
 	 * @param \Sabre\DAV\Server $server Sabre server
 	 *
 	 * @return void
+	 * @throws MethodNotAllowed
 	 */
 	public function initialize(\Sabre\DAV\Server $server) {
 		$server->on('beforeMethod', [$this, 'beforeMethod'], 999);
@@ -64,31 +66,19 @@ class FilesDropPlugin extends ServerPlugin {
 
 	public function beforeMethod(RequestInterface $request, ResponseInterface $response){
 
-		if (!$this->enabled || $request->getMethod() !== 'PUT') {
+		if (!$this->enabled) {
 			return;
 		}
 
-		$path = $request->getPath();
-
-		if ($this->view->file_exists($path)) {
-			$newName = \OC_Helper::buildNotExistingFileNameForView('/', $path, $this->view);
-
-			$url = $request->getBaseUrl() . $newName . '?';
-			$parms = $request->getQueryParameters();
-			$first = true;
-			foreach ($parms as $k => $v) {
-				if ($first) {
-					$url .= '?';
-					$first = false;
-				} else {
-					$url .= '&';
-				}
-				$url .= $k . '=' . $v;
-			}
-
-			$request->setUrl($url);
+		if ($request->getMethod() !== 'PUT') {
+			throw new MethodNotAllowed('Only PUT is allowed on files drop');
 		}
 
+		$path = explode('/', $request->getPath());
+		$path = array_pop($path);
 
+		$newName = \OC_Helper::buildNotExistingFileNameForView('/', $path, $this->view);
+		$url = $request->getBaseUrl() . $newName;
+		$request->setUrl($url);
 	}
 }

apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php

@@ -0,0 +1,179 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016, Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\DAV\Tests\Files\Sharing;
+
+use OC\Files\View;
+use OCA\DAV\Files\Sharing\FilesDropPlugin;
+use Sabre\DAV\Exception\MethodNotAllowed;
+use Sabre\DAV\Server;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+use Test\TestCase;
+
+class FilesDropPluginTest extends TestCase {
+
+	/** @var View|\PHPUnit_Framework_MockObject_MockObject */
+	private $view;
+
+	/** @var Server|\PHPUnit_Framework_MockObject_MockObject */
+	private $server;
+
+	/** @var FilesDropPlugin */
+	private $plugin;
+
+	/** @var RequestInterface|\PHPUnit_Framework_MockObject_MockObject */
+	private $request;
+
+	/** @var ResponseInterface|\PHPUnit_Framework_MockObject_MockObject */
+	private $response;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->view = $this->createMock(View::class);
+		$this->server = $this->createMock(Server::class);
+		$this->plugin = new FilesDropPlugin();
+
+		$this->request = $this->createMock(RequestInterface::class);
+		$this->response = $this->createMock(ResponseInterface::class);
+
+		$this->response->expects($this->never())
+			->method($this->anything());
+	}
+
+	public function testInitialize() {
+		$this->server->expects($this->once())
+			->method('on')
+			->with(
+				$this->equalTo('beforeMethod'),
+				$this->equalTo([$this->plugin, 'beforeMethod']),
+				$this->equalTo(999)
+			);
+
+		$this->plugin->initialize($this->server);
+	}
+
+	public function testNotEnabled() {
+		$this->view->expects($this->never())
+			->method($this->anything());
+
+		$this->request->expects($this->never())
+			->method($this->anything());
+
+		$this->plugin->beforeMethod($this->request, $this->response);
+	}
+
+	public function testValid() {
+		$this->plugin->enable();
+		$this->plugin->setView($this->view);
+
+		$this->request->method('getMethod')
+			->willReturn('PUT');
+
+		$this->request->method('getPath')
+			->willReturn('file.txt');
+
+		$this->request->method('getBaseUrl')
+			->willReturn('https://example.com');
+
+		$this->view->method('file_exists')
+			->with('/file.txt')
+			->willReturn(false);
+
+		$this->request->expects($this->once())
+			->method('setUrl')
+			->with('https://example.com/file.txt');
+
+		$this->plugin->beforeMethod($this->request, $this->response);
+	}
+
+	public function testFileAlreadyExistsValid() {
+		$this->plugin->enable();
+		$this->plugin->setView($this->view);
+
+		$this->request->method('getMethod')
+			->willReturn('PUT');
+
+		$this->request->method('getPath')
+			->willReturn('file.txt');
+
+		$this->request->method('getBaseUrl')
+			->willReturn('https://example.com');
+
+		$this->view->method('file_exists')
+			->will($this->returnCallback(function($path) {
+				if ($path === 'file.txt' || $path === '/file.txt') {
+					return true;
+				} else {
+					return false;
+				}
+			}));
+
+		$this->request->expects($this->once())
+			->method('setUrl')
+			->with($this->equalTo('https://example.com/file (2).txt'));
+
+		$this->plugin->beforeMethod($this->request, $this->response);
+	}
+
+	public function testNoMKCOL() {
+		$this->plugin->enable();
+		$this->plugin->setView($this->view);
+
+		$this->request->method('getMethod')
+			->willReturn('MKCOL');
+
+		$this->expectException(MethodNotAllowed::class);
+
+		$this->plugin->beforeMethod($this->request, $this->response);
+	}
+
+	public function testNoSubdirPut() {
+		$this->plugin->enable();
+		$this->plugin->setView($this->view);
+
+		$this->request->method('getMethod')
+			->willReturn('PUT');
+
+		$this->request->method('getPath')
+			->willReturn('folder/file.txt');
+
+		$this->request->method('getBaseUrl')
+			->willReturn('https://example.com');
+
+		$this->view->method('file_exists')
+			->will($this->returnCallback(function($path) {
+				if ($path === 'file.txt' || $path === '/file.txt') {
+					return true;
+				} else {
+					return false;
+				}
+			}));
+
+		$this->request->expects($this->once())
+			->method('setUrl')
+			->with($this->equalTo('https://example.com/file (2).txt'));
+
+		$this->plugin->beforeMethod($this->request, $this->response);
+	}
+}

build/integration/config/behat.yml

@@ -62,6 +62,16 @@ default:
               - admin
               - admin
             regular_user_password: 123456
+    filesdrop:
+      paths:
+        - %paths.base%/../filesdrop_features
+      contexts:
+        - FilesDropContext:
+            baseUrl: http://localhost:8080
+            admin:
+              - admin
+              - admin
+            regular_user_password: 123456
 
 
 

build/integration/features/bootstrap/FilesDropContext.php

@@ -0,0 +1,72 @@
+<?php
+
+use Behat\Behat\Context\Context;
+use Behat\Behat\Context\SnippetAcceptingContext;
+use Behat\Behat\Tester\Exception\PendingException;
+use GuzzleHttp\Client;
+
+require __DIR__ . '/../../vendor/autoload.php';
+
+class FilesDropContext implements Context, SnippetAcceptingContext {
+	use WebDav;
+
+	/**
+	 * @When Dropping file :path with :content
+	 */
+	public function droppingFileWith($path, $content) {
+		$client = new Client();
+		$options = [];
+		if (count($this->lastShareData->data->element) > 0){
+			$token = $this->lastShareData->data[0]->token;
+		} else {
+			$token = $this->lastShareData->data[0]->token;
+		}
+
+		$base = substr($this->baseUrl, 0, -4);
+		$fullUrl = $base . '/public.php/webdav' . $path;
+
+		$options['auth'] = [$token, ''];
+		$options['headers'] = [
+			'X-REQUESTED-WITH' => 'XMLHttpRequest'
+		];
+
+		$request = $client->createRequest('PUT', $fullUrl, $options);
+		$file = \GuzzleHttp\Stream\Stream::factory($content);
+		$request->setBody($file);
+
+		try {
+			$this->response = $client->send($request);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			$this->response = $e->getResponse();
+		}
+	}
+
+	/**
+	 * @When Creating folder :folder in drop
+	 */
+	public function creatingFolderInDrop($folder) {
+		$client = new Client();
+		$options = [];
+		if (count($this->lastShareData->data->element) > 0){
+			$token = $this->lastShareData->data[0]->token;
+		} else {
+			$token = $this->lastShareData->data[0]->token;
+		}
+
+		$base = substr($this->baseUrl, 0, -4);
+		$fullUrl = $base . '/public.php/webdav/' . $folder;
+
+		$options['auth'] = [$token, ''];
+		$options['headers'] = [
+			'X-REQUESTED-WITH' => 'XMLHttpRequest'
+		];
+
+		$request = $client->createRequest('MKCOL', $fullUrl, $options);
+
+		try {
+			$this->response = $client->send($request);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			$this->response = $e->getResponse();
+		}
+	}
+}

build/integration/filesdrop_features/filesdrop.feature

@@ -0,0 +1,59 @@
+Feature: FilesDrop
+
+  Scenario: Put file via files drop
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 3 |
+      | publicUpload | true |
+    And Updating last share with
+      | permissions | 4 |
+    When Dropping file "/a.txt" with "abc"
+    And Downloading file "/drop/a.txt"
+    Then Downloaded content should be "abc"
+
+  Scenario: Put file same file multiple times via files drop
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 3 |
+      | publicUpload | true |
+    And Updating last share with
+      | permissions | 4 |
+    When Dropping file "/a.txt" with "abc"
+    And Dropping file "/a.txt" with "def"
+    And Downloading file "/drop/a.txt"
+    Then Downloaded content should be "abc"
+    And Downloading file "/drop/a (2).txt"
+    Then Downloaded content should be "def"
+
+  Scenario: Files drop ignores directory
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 3 |
+      | publicUpload | true |
+    And Updating last share with
+      | permissions | 4 |
+    When Dropping file "/folder/a.txt" with "abc"
+    And Downloading file "/drop/a.txt"
+    Then Downloaded content should be "abc"
+
+  Scenario: Files drop forbis MKCOL
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 3 |
+      | publicUpload | true |
+    And Updating last share with
+      | permissions | 4 |
+    When Creating folder "folder" in drop
+    Then the HTTP status code should be "405"