From 363963577c97b6f87df47f424ce6f43c82cadfab Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 10:03:02 +0100 Subject: [PATCH 1/5] Harden files drop * Fail on MKCOL * Only take filename ignore directories * No need to parse query parameters Signed-off-by: Roeland Jago Douma --- .../dav/lib/Files/Sharing/FilesDropPlugin.php | 30 +++++++------------ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/apps/dav/lib/Files/Sharing/FilesDropPlugin.php b/apps/dav/lib/Files/Sharing/FilesDropPlugin.php index 299427b163..3485df09d0 100644 --- a/apps/dav/lib/Files/Sharing/FilesDropPlugin.php +++ b/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); } } From 1f387ad1e6f8f0a14a63bb0c9734c305b1d6ced1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 10:37:06 +0100 Subject: [PATCH 2/5] Add unit tests Signed-off-by: Roeland Jago Douma --- .../Files/Sharing/FilesDropPluginTest.php | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php diff --git a/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php b/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php new file mode 100644 index 0000000000..e2990f27b6 --- /dev/null +++ b/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php @@ -0,0 +1,179 @@ + + * + * @author Roeland Jago Douma + * + * @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 . + * + */ +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); + } +} From 4630ff536ea433d2e81ebe18b04f202bbe6b89ea Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 10:43:11 +0100 Subject: [PATCH 3/5] Add intergration tests Signed-off-by: Roeland Jago Douma --- build/integration/config/behat.yml | 10 +++ .../features/bootstrap/FilesDropContext.php | 72 +++++++++++++++++++ .../filesdrop_features/filesdrop.feature | 59 +++++++++++++++ 3 files changed, 141 insertions(+) create mode 100644 build/integration/features/bootstrap/FilesDropContext.php create mode 100644 build/integration/filesdrop_features/filesdrop.feature diff --git a/build/integration/config/behat.yml b/build/integration/config/behat.yml index 3728e5461d..9ef36f083e 100644 --- a/build/integration/config/behat.yml +++ b/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 diff --git a/build/integration/features/bootstrap/FilesDropContext.php b/build/integration/features/bootstrap/FilesDropContext.php new file mode 100644 index 0000000000..760d76ac5e --- /dev/null +++ b/build/integration/features/bootstrap/FilesDropContext.php @@ -0,0 +1,72 @@ +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(); + } + } +} diff --git a/build/integration/filesdrop_features/filesdrop.feature b/build/integration/filesdrop_features/filesdrop.feature new file mode 100644 index 0000000000..4a8759e241 --- /dev/null +++ b/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" From 2de09927a145f43e5ba4e29e0c7048e3b0a2512f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 13:09:10 +0100 Subject: [PATCH 4/5] Add tests to drone Signed-off-by: Roeland Jago Douma --- .drone.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.drone.yml b/.drone.yml index d5fdcdd746..b0ea2ad6c2 100644 --- a/.drone.yml +++ b/.drone.yml @@ -338,6 +338,14 @@ pipeline: when: matrix: TESTS: integration-setup-features + integration-filesdrop-features: + image: nextcloudci/integration-php7.0:integration-php7.0-2 + commands: + - 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 +390,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 From 9b21b82d186d139cc363b29e674cdeb83fcc5ab5 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 2 Dec 2016 15:48:40 +0100 Subject: [PATCH 5/5] Install instance before running integration tests Signed-off-by: Morris Jobke --- .drone.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.drone.yml b/.drone.yml index b0ea2ad6c2..6174937ad2 100644 --- a/.drone.yml +++ b/.drone.yml @@ -341,6 +341,7 @@ pipeline: 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: