From 363963577c97b6f87df47f424ce6f43c82cadfab Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 10:03:02 +0100 Subject: Harden files drop * Fail on MKCOL * Only take filename ignore directories * No need to parse query parameters Signed-off-by: Roeland Jago Douma --- apps/dav/lib/Files/Sharing/FilesDropPlugin.php | 30 +++++++++----------------- 1 file changed, 10 insertions(+), 20 deletions(-) (limited to 'apps/dav') diff --git a/apps/dav/lib/Files/Sharing/FilesDropPlugin.php b/apps/dav/lib/Files/Sharing/FilesDropPlugin.php index 299427b1634..3485df09d0f 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); } } -- cgit v1.2.3 From 1f387ad1e6f8f0a14a63bb0c9734c305b1d6ced1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Dec 2016 10:37:06 +0100 Subject: Add unit tests Signed-off-by: Roeland Jago Douma --- .../unit/Files/Sharing/FilesDropPluginTest.php | 179 +++++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php (limited to 'apps/dav') 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 00000000000..e2990f27b60 --- /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); + } +} -- cgit v1.2.3