From e2453d78c0a2e6fcdfa3c826cb231ab3865c4cf8 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 24 Mar 2015 11:21:58 +0100 Subject: [PATCH] Properly catch whether a share is `null` Despite it's PHPDoc the function might return `null` which was not properly catched and thus in some situations the share was resolved to the sharing users root directory. To test this perform the following steps: * Share file in owncloud 7 (7.0.4.2) * Delete the parent folder of the shared file * The share stays is in the DB and the share via the sharelink is inaccessible. (which is good) * Upgrade to owncloud 8 (8.0.2) (This step is crucial. The bug is not reproduceable without upgrading from 7 to 8. It seems like the old tokens are handled different than the newer ones) * Optional Step: Logout, Reset Browser Session, etc. * Access the share via the old share url: almost empty page, but there is a dowload button which adds a "/download" to the URL. * Upon clicking, a download.zip is downloaded which contains EVERYTHING from the owncloud directory (of the user who shared the file) * No exception is thrown and no error is logged. This will add a check whether the share is a valid one and also adds unit tests to prevent further regressions in the future. Needs to be backported to ownCloud 8. Adding a proper clean-up of the orphaned shares is out-of-scope and would probably require some kind of FK or so. Fixes https://github.com/owncloud/core/issues/15097 --- apps/files_sharing/application.php | 5 +- .../lib/controllers/sharecontroller.php | 38 ++++++------ .../lib/middleware/sharingcheckmiddleware.php | 3 +- .../tests/controller/sharecontroller.php | 62 ++++++++++++++++++- .../appframework/http/notfoundresponse.php | 43 +++++++++++++ 5 files changed, 128 insertions(+), 23 deletions(-) create mode 100644 lib/public/appframework/http/notfoundresponse.php diff --git a/apps/files_sharing/application.php b/apps/files_sharing/application.php index 3302848106..e23960cf2b 100644 --- a/apps/files_sharing/application.php +++ b/apps/files_sharing/application.php @@ -42,7 +42,7 @@ class Application extends App { $server->getAppConfig(), $server->getConfig(), $c->query('URLGenerator'), - $server->getUserManager(), + $c->query('UserManager'), $server->getLogger(), $server->getActivityManager() ); @@ -65,6 +65,9 @@ class Application extends App { $container->registerService('URLGenerator', function(SimpleContainer $c) use ($server){ return $server->getUrlGenerator(); }); + $container->registerService('UserManager', function(SimpleContainer $c) use ($server){ + return $server->getUserManager(); + }); $container->registerService('IsIncomingShareEnabled', function(SimpleContainer $c) { return Helper::isIncomingServer2serverShareEnabled(); }); diff --git a/apps/files_sharing/lib/controllers/sharecontroller.php b/apps/files_sharing/lib/controllers/sharecontroller.php index b0d7781515..d48e7671cf 100644 --- a/apps/files_sharing/lib/controllers/sharecontroller.php +++ b/apps/files_sharing/lib/controllers/sharecontroller.php @@ -17,12 +17,12 @@ use OC_Files; use OC_Util; use OCP; use OCP\Template; -use OCP\JSON; use OCP\Share; use OCP\AppFramework\Controller; use OCP\IRequest; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Http\RedirectResponse; +use OCP\AppFramework\Http\NotFoundResponse; use OC\URLGenerator; use OC\AppConfig; use OCP\ILogger; @@ -60,7 +60,7 @@ class ShareController extends Controller { * @param AppConfig $appConfig * @param OCP\IConfig $config * @param URLGenerator $urlGenerator - * @param OC\User\Manager $userManager + * @param OCP\IUserManager $userManager * @param ILogger $logger * @param OCP\Activity\IManager $activityManager */ @@ -70,7 +70,7 @@ class ShareController extends Controller { AppConfig $appConfig, OCP\IConfig $config, URLGenerator $urlGenerator, - OC\User\Manager $userManager, + OCP\IUserManager $userManager, ILogger $logger, OCP\Activity\IManager $activityManager) { parent::__construct($appName, $request); @@ -113,7 +113,7 @@ class ShareController extends Controller { public function authenticate($token, $password = '') { $linkItem = Share::getShareByToken($token, false); if($linkItem === false) { - return new TemplateResponse('core', '404', array(), 'guest'); + return new NotFoundResponse(); } $authenticate = Helper::authenticate($linkItem, $password); @@ -139,18 +139,11 @@ class ShareController extends Controller { // Check whether share exists $linkItem = Share::getShareByToken($token, false); if($linkItem === false) { - return new TemplateResponse('core', '404', array(), 'guest'); + return new NotFoundResponse(); } $shareOwner = $linkItem['uid_owner']; - $originalSharePath = null; - $rootLinkItem = OCP\Share::resolveReShare($linkItem); - if (isset($rootLinkItem['uid_owner'])) { - OCP\JSON::checkUserExists($rootLinkItem['uid_owner']); - OC_Util::tearDownFS(); - OC_Util::setupFS($rootLinkItem['uid_owner']); - $originalSharePath = Filesystem::getPath($linkItem['file_source']); - } + $originalSharePath = $this->getPath($token); // Share is password protected - check whether the user is permitted to access the share if (isset($linkItem['share_with']) && !Helper::authenticate($linkItem)) { @@ -165,7 +158,7 @@ class ShareController extends Controller { $file = basename($originalSharePath); - $shareTmpl = array(); + $shareTmpl = []; $shareTmpl['displayName'] = User::getDisplayName($shareOwner); $shareTmpl['filename'] = $file; $shareTmpl['directory_path'] = $linkItem['file_target']; @@ -289,22 +282,29 @@ class ShareController extends Controller { } /** - * @param $token - * @return null|string + * @param string $token + * @return string Resolved file path of the token + * @throws \Exception In case share could not get properly resolved */ private function getPath($token) { $linkItem = Share::getShareByToken($token, false); - $path = null; if (is_array($linkItem) && isset($linkItem['uid_owner'])) { // seems to be a valid share $rootLinkItem = Share::resolveReShare($linkItem); if (isset($rootLinkItem['uid_owner'])) { - JSON::checkUserExists($rootLinkItem['uid_owner']); + if(!$this->userManager->userExists($rootLinkItem['uid_owner'])) { + throw new \Exception('Owner of the share does not exist anymore'); + } OC_Util::tearDownFS(); OC_Util::setupFS($rootLinkItem['uid_owner']); $path = Filesystem::getPath($linkItem['file_source']); + + if(!empty($path) && Filesystem::isReadable($path)) { + return $path; + } } } - return $path; + + throw new \Exception('No file found belonging to file.'); } } diff --git a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php index 3508407f2a..3e7cdf4aa3 100644 --- a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php +++ b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php @@ -11,6 +11,7 @@ namespace OCA\Files_Sharing\Middleware; use OCP\App\IAppManager; +use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; @@ -59,7 +60,7 @@ class SharingCheckMiddleware extends Middleware { * @return TemplateResponse */ public function afterException($controller, $methodName, \Exception $exception){ - return new TemplateResponse('core', '404', array(), 'guest'); + return new NotFoundResponse(); } /** diff --git a/apps/files_sharing/tests/controller/sharecontroller.php b/apps/files_sharing/tests/controller/sharecontroller.php index 189fb57653..204422853d 100644 --- a/apps/files_sharing/tests/controller/sharecontroller.php +++ b/apps/files_sharing/tests/controller/sharecontroller.php @@ -12,6 +12,7 @@ namespace OCA\Files_Sharing\Controllers; use OC\Files\Filesystem; use OCA\Files_Sharing\Application; +use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\IAppContainer; use OCP\Files; use OCP\AppFramework\Http\RedirectResponse; @@ -49,6 +50,8 @@ class ShareControllerTest extends \Test\TestCase { ->disableOriginalConstructor()->getMock(); $this->container['URLGenerator'] = $this->getMockBuilder('\OC\URLGenerator') ->disableOriginalConstructor()->getMock(); + $this->container['UserManager'] = $this->getMockBuilder('\OCP\IUserManager') + ->disableOriginalConstructor()->getMock(); $this->urlGenerator = $this->container['URLGenerator']; $this->shareController = $this->container['ShareController']; @@ -115,7 +118,7 @@ class ShareControllerTest extends \Test\TestCase { public function testAuthenticate() { // Test without a not existing token $response = $this->shareController->authenticate('ThisTokenShouldHopefullyNeverExistSoThatTheUnitTestWillAlwaysPass :)'); - $expectedResponse = new TemplateResponse('core', '404', array(), 'guest'); + $expectedResponse = new NotFoundResponse(); $this->assertEquals($expectedResponse, $response); // Test with a valid password @@ -130,9 +133,14 @@ class ShareControllerTest extends \Test\TestCase { } public function testShowShare() { + $this->container['UserManager']->expects($this->exactly(2)) + ->method('userExists') + ->with($this->user) + ->will($this->returnValue(true)); + // Test without a not existing token $response = $this->shareController->showShare('ThisTokenShouldHopefullyNeverExistSoThatTheUnitTestWillAlwaysPass :)'); - $expectedResponse = new TemplateResponse('core', '404', array(), 'guest'); + $expectedResponse = new NotFoundResponse(); $this->assertEquals($expectedResponse, $response); // Test with a password protected share and no authentication @@ -176,4 +184,54 @@ class ShareControllerTest extends \Test\TestCase { array('token' => $this->token))); $this->assertEquals($expectedResponse, $response); } + + /** + * @expectedException \Exception + * @expectedExceptionMessage No file found belonging to file. + */ + public function testShowShareWithDeletedFile() { + $this->container['UserManager']->expects($this->once()) + ->method('userExists') + ->with($this->user) + ->will($this->returnValue(true)); + + $view = new View('/'. $this->user . '/files'); + $view->unlink('file1.txt'); + $linkItem = Share::getShareByToken($this->token, false); + \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']); + $this->shareController->showShare($this->token); + } + + /** + * @expectedException \Exception + * @expectedExceptionMessage No file found belonging to file. + */ + public function testDownloadShareWithDeletedFile() { + $this->container['UserManager']->expects($this->once()) + ->method('userExists') + ->with($this->user) + ->will($this->returnValue(true)); + + $view = new View('/'. $this->user . '/files'); + $view->unlink('file1.txt'); + $linkItem = Share::getShareByToken($this->token, false); + \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']); + $this->shareController->downloadShare($this->token); + } + + /** + * @expectedException \Exception + * @expectedExceptionMessage Owner of the share does not exist anymore + */ + public function testShowShareWithNotExistingUser() { + $this->container['UserManager']->expects($this->once()) + ->method('userExists') + ->with($this->user) + ->will($this->returnValue(false)); + + $linkItem = Share::getShareByToken($this->token, false); + \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']); + $this->shareController->showShare($this->token); + } + } diff --git a/lib/public/appframework/http/notfoundresponse.php b/lib/public/appframework/http/notfoundresponse.php new file mode 100644 index 0000000000..21f0461f5e --- /dev/null +++ b/lib/public/appframework/http/notfoundresponse.php @@ -0,0 +1,43 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 OCP\AppFramework\Http; + +use OCP\AppFramework\Http; +use OCP\Template; + +/** + * A generic 404 response showing an 404 error page as well to the end-user + */ +class NotFoundResponse extends Response { + + public function __construct() { + $this->setStatus(404); + } + + /** + * @return string + */ + public function render() { + $template = new Template('core', '404', 'guest'); + return $template->fetchPage(); + } +}