diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index eee93bca7524..b61f50548ee2 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -101,6 +101,7 @@ public function createServer($baseUri, // Load plugins $server->addPlugin(new \OCA\DAV\Connector\Sabre\MaintenancePlugin($this->config)); + $server->addPlugin(new \OCA\DAV\Connector\Sabre\ValidateRequestPlugin('webdav')); $server->addPlugin(new \OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin($this->config)); $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend)); // FIXME: The following line is a workaround for legacy components relying on being able to send a GET to / diff --git a/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php b/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php new file mode 100644 index 000000000000..f26b50c3d44a --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php @@ -0,0 +1,101 @@ + + * + * @copyright Copyright (c) 2017, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\DAV\Connector\Sabre; + +use Sabre\DAV\Exception\ServiceUnavailable; +use Sabre\DAV\ServerPlugin; + +/** + * Class ValidateRequestPlugin is a plugin which examines correctness of + * the webdav request, checking if accessing a service e.g. "dav" + * the request has proper headers/structure. + * + * @see https://github.com/owncloud/core/issues/28200 + * @package OCA\DAV\Connector\Sabre + */ +class ValidateRequestPlugin extends ServerPlugin { + + /** + * Reference to main server object + * + * @var \Sabre\DAV\Server + */ + private $server; + + /** + * Service type as decided by resolveService($service) in remote.php + * + * @var string + */ + private $service; + + /** + * This plugin ensures that all request directed to specific + * services (type $service as decided by resolveService($service) in remote.php) + * contain correct headers and their structure is correct + * + * Currently supported: + * 'webdav' + * 'dav' + * + * @var string $service + */ + public function __construct($service) { + $this->service = $service; + } + + /** + * This initializes the plugin. + * + * This function is called by \Sabre\DAV\Server, after + * addPlugin is called. + * + * This method should set up the required event subscriptions. + * + * @param \Sabre\DAV\Server $server + * @return void + */ + public function initialize(\Sabre\DAV\Server $server) { + $this->server = $server; + $this->server->on('beforeMethod:PUT', [$this, 'checkValidityPut'], 100); + } + + /** + * This method is called before any HTTP method and returns http status code 503 + * in case the request is incorrect + * + * @throws ServiceUnavailable + * @return bool + */ + public function checkValidityPut() { + $request = $this->server->httpRequest; + + // Verify if optional OC headers are routed in the proper endpoint + if (($request->hasHeader('HTTP_OC_CHUNKED') || $request->hasHeader('Oc-Chunked')) + && ($this->service !== 'webdav')) { + // Headers not allowed in new dav endpoint + throw new ServiceUnavailable('Specified header (HTTP_OC_CHUNKED/OC-Chunked header) is allowed only in webdav endpoint'); + } + + return true; + } +} diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 14d285f5de77..e6807c8d73d4 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -50,6 +50,7 @@ use OCA\DAV\Connector\Sabre\TagsPlugin; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\Connector\Sabre\MaintenancePlugin; +use OCA\DAV\Connector\Sabre\ValidateRequestPlugin; class Server { @@ -80,6 +81,7 @@ public function __construct(IRequest $request, $baseUri) { $config = \OC::$server->getConfig(); $this->server->addPlugin(new MaintenancePlugin($config)); + $this->server->addPlugin(new ValidateRequestPlugin('dav')); $this->server->addPlugin(new BlockLegacyClientPlugin($config)); $authPlugin = new Plugin(); $authPlugin->addBackend(new PublicAuth()); diff --git a/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php new file mode 100644 index 000000000000..964fda2bd6f9 --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php @@ -0,0 +1,74 @@ + + * + * @copyright Copyright (c) 2017, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\DAV\Tests\unit\Connector\Sabre; + +use OCA\DAV\Connector\Sabre\ValidateRequestPlugin; +use Test\TestCase; +use OCP\IConfig; + +/** + * Class ValidateRequestPluginTest + * + * @package OCA\DAV\Tests\unit\Connector\Sabre + */ +class ValidateRequestPluginTest extends TestCase { + /** @var \Sabre\DAV\Server */ + private $server; + + /** @var ValidateRequestPlugin */ + private $plugin; + + public function setUp() { + parent::setUp(); + + $this->server = new \Sabre\DAV\Server(); + } + + /** + * @expectedException \Sabre\DAV\Exception\ServiceUnavailable + * @expectedExceptionMessage Specified header (HTTP_OC_CHUNKED/OC-Chunked header) is allowed only in webdav endpoint + */ + public function testOldChunkingInNewEndpoint1() { + $headers['HTTP_OC_CHUNKED'] = 1; + + $this->plugin = new ValidateRequestPlugin('dav'); + $this->server->httpRequest = new \Sabre\HTTP\Request('PUT', 'http://server/root_url/remote.php/dav/files/user/testnew.txt', $headers); + $this->plugin->initialize($this->server); + + $this->plugin->checkValidityPut(); + } + + /** + * @expectedException \Sabre\DAV\Exception\ServiceUnavailable + * @expectedExceptionMessage Specified header (HTTP_OC_CHUNKED/OC-Chunked header) is allowed only in webdav endpoint + */ + public function testOldChunkingInNewEndpoint2() { + $headers['OC-Chunked'] = 1; + + $this->plugin = new ValidateRequestPlugin('dav'); + $this->server->httpRequest = new \Sabre\HTTP\Request('PUT', 'http://server/root_url/remote.php/dav/files/user/testnew.txt', $headers); + $this->plugin->initialize($this->server); + + $this->plugin->checkValidityPut(); + } + +} diff --git a/tests/integration/features/bootstrap/Checksums.php b/tests/integration/features/bootstrap/Checksums.php index 76f3b911ae26..41f92aeb5ac9 100644 --- a/tests/integration/features/bootstrap/Checksums.php +++ b/tests/integration/features/bootstrap/Checksums.php @@ -159,14 +159,18 @@ public function theOcChecksumHeaderShouldNotBeThere() * @param string $checksum */ public function userUploadsChunkFileOfWithToWithChecksum($user, $num, $total, $data, $destination, $checksum) { - $num -= 1; - $data = \GuzzleHttp\Stream\Stream::factory($data); - $file = $destination . '-chunking-42-' . $total . '-' . $num; - $this->response = $this->makeDavRequest($user, - 'PUT', - $file, - ['OC-Checksum' => $checksum, 'OC-Chunked' => '1'], - $data, - "files"); + try { + $num -= 1; + $data = \GuzzleHttp\Stream\Stream::factory($data); + $file = $destination . '-chunking-42-' . $total . '-' . $num; + $this->response = $this->makeDavRequest($user, + 'PUT', + $file, + ['OC-Checksum' => $checksum, 'OC-Chunked' => '1'], + $data, + "files"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } } diff --git a/tests/integration/features/bootstrap/WebDav.php b/tests/integration/features/bootstrap/WebDav.php index 10c67dda5d63..1cb375232c96 100644 --- a/tests/integration/features/bootstrap/WebDav.php +++ b/tests/integration/features/bootstrap/WebDav.php @@ -680,19 +680,6 @@ public function uploadWithAllMechanisms($user, $source, $destination, $overwrite } } if ($dav === 'new') { - // old chunking style applied to new endpoint 🙈 - if (!$overwriteMode) { - $suffix = '-' . $dav . 'dav-oldchunking'; - } - try { - // FIXME: prepending new dav path because the chunking utility functions are messed up - $this->userUploadsAFileToWithChunks($user, $source, '/files/' . $user . '/' . ltrim($destination, '/') . $suffix, 'old'); - $responses[] = $this->response; - } catch (ServerException $e) { - $responses[] = $e->getResponse(); - } - - // new chunking style applied to new endpoint if (!$overwriteMode) { $suffix = '-' . $dav . 'dav-newchunking'; } @@ -834,10 +821,14 @@ public function userCreatedAFolder($user, $destination){ */ public function userUploadsChunkedFile($user, $num, $total, $data, $destination) { - $num -= 1; - $data = \GuzzleHttp\Stream\Stream::factory($data); - $file = $destination . '-chunking-42-' . $total . '-' . $num; - $this->makeDavRequest($user, 'PUT', $file, ['OC-Chunked' => '1'], $data, "uploads"); + try { + $num -= 1; + $data = \GuzzleHttp\Stream\Stream::factory($data); + $file = $destination . '-chunking-42-' . $total . '-' . $num; + $this->makeDavRequest($user, 'PUT', $file, ['OC-Chunked' => '1'], $data, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -845,8 +836,12 @@ public function userUploadsChunkedFile($user, $num, $total, $data, $destination) */ public function userCreatesANewChunkingUploadWithId($user, $id) { - $destination = '/uploads/'.$user.'/'.$id; - $this->makeDavRequest($user, 'MKCOL', $destination, [], null, "uploads"); + try { + $destination = '/uploads/'.$user.'/'.$id; + $this->makeDavRequest($user, 'MKCOL', $destination, [], null, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -854,9 +849,13 @@ public function userCreatesANewChunkingUploadWithId($user, $id) */ public function userUploadsNewChunkFileOfWithToId($user, $num, $data, $id) { - $data = \GuzzleHttp\Stream\Stream::factory($data); - $destination = '/uploads/'. $user .'/'. $id .'/' . $num; - $this->makeDavRequest($user, 'PUT', $destination, [], $data, "uploads"); + try { + $data = \GuzzleHttp\Stream\Stream::factory($data); + $destination = '/uploads/'. $user .'/'. $id .'/' . $num; + $this->makeDavRequest($user, 'PUT', $destination, [], $data, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -885,11 +884,15 @@ public function userUploadsNewChunkFileOfWithToIdWithChecksum($user, $num, $data */ public function userMovesNewChunkFileWithIdToMychunkedfile($user, $id, $dest) { - $source = '/uploads/' . $user . '/' . $id . '/.file'; - $destination = $this->baseUrlWithoutOCSAppendix() . $this->getDavFilesPath($user) . $dest; - $this->makeDavRequest($user, 'MOVE', $source, [ - 'Destination' => $destination - ], null, "uploads"); + try { + $source = '/uploads/' . $user . '/' . $id . '/.file'; + $destination = $this->baseUrlWithoutOCSAppendix() . $this->getDavFilesPath($user) . $dest; + $this->makeDavRequest($user, 'MOVE', $source, [ + 'Destination' => $destination + ], null, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index 1c178a3fea94..50618cd0b166 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -115,24 +115,6 @@ Feature: checksums And user "user0" downloads the file "/myChecksumFileCopy.txt" Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" - Scenario: Uploading a chunked file with checksum should return the checksum in the propfind using new dav path - Given using new dav path - And user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" - - Scenario: Uploading a chunked file with checksum should return the checksum in the download header using new dav path - Given using new dav path - And user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8" - @local_storage Scenario: Downloading a file from local storage has correct checksum using new dav path Given using new dav path @@ -183,3 +165,10 @@ Feature: checksums Then The following headers should be set | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | + ## Validation Plugin or Old Endpoint Specific + + Scenario: Uploading a old method chunked file with checksum should fail using new dav path + Given using new dav path + And user "user0" exists + When user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + Then the HTTP status code should be "503" diff --git a/tests/integration/features/webdav-related-new-endpoint.feature b/tests/integration/features/webdav-related-new-endpoint.feature index df2510cca4ca..3c65b636b3f6 100644 --- a/tests/integration/features/webdav-related-new-endpoint.feature +++ b/tests/integration/features/webdav-related-new-endpoint.feature @@ -548,3 +548,35 @@ Feature: webdav-related-new-endpoint And as "user1" the folder "/folderB/ONE/TWO" exists And User "user1" checks id of file "/folderB/ONE" + ## Validation Plugin or Old Endpoint Specific + + Scenario: New chunked upload MKDIR using old dav path should fail + Given using old dav path + And user "user0" exists + When user "user0" creates a new chunking upload with id "chunking-42" + Then the HTTP status code should be "409" + + Scenario: New chunked upload PUT using old dav path should fail + Given using new dav path + And user "user0" exists + And user "user0" creates a new chunking upload with id "chunking-42" + When using old dav path + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + Then the HTTP status code should be "404" + + Scenario: New chunked upload MOVE using old dav path should fail + Given using new dav path + And user "user0" exists + And user "user0" creates a new chunking upload with id "chunking-42" + And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42" + And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + When using old dav path + And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" + Then the HTTP status code should be "404" + + Scenario: Upload to new dav path using old way should fail + Given using new dav path + And user "user0" exists + When user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChunkedFile.txt" + Then the HTTP status code should be "503" \ No newline at end of file