From 54915b5b06c0a8836ff9ad295da1d866b9197623 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 26 Aug 2024 19:31:43 +0200 Subject: [PATCH] fix(dav): Validate target path before doing a MOVE or COPY Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 107 ++++--- apps/dav/lib/Connector/Sabre/Node.php | 5 +- .../dav/lib/Connector/Sabre/ServerFactory.php | 2 + apps/dav/lib/Server.php | 14 +- .../unit/Connector/Sabre/FilesPluginTest.php | 273 ++++++++++-------- .../Connector/Sabre/FilesReportPluginTest.php | 61 ++-- lib/private/Files/Node/Node.php | 2 +- 7 files changed, 263 insertions(+), 201 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index d4c996df145a9..7fbf174a89a24 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -8,8 +8,11 @@ namespace OCA\DAV\Connector\Sabre; use OC\AppFramework\Http\Request; +use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCP\Constants; use OCP\Files\ForbiddenException; +use OCP\Files\IFilenameValidator; +use OCP\Files\InvalidPathException; use OCP\Files\StorageNotAvailableException; use OCP\FilesMetadata\Exceptions\FilesMetadataException; use OCP\FilesMetadata\Exceptions\FilesMetadataNotFoundException; @@ -19,6 +22,7 @@ use OCP\IPreview; use OCP\IRequest; use OCP\IUserSession; +use OCP\L10N\IFactory; use Sabre\DAV\Exception\Forbidden; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\IFile; @@ -65,33 +69,27 @@ class FilesPlugin extends ServerPlugin { /** Reference to main server object */ private ?Server $server = null; - private Tree $tree; - private IUserSession $userSession; /** - * Whether this is public webdav. - * If true, some returned information will be stripped off. + * @param Tree $tree + * @param IConfig $config + * @param IRequest $request + * @param IPreview $previewManager + * @param IUserSession $userSession + * @param bool $isPublic Whether this is public WebDAV. If true, some returned information will be stripped off. + * @param bool $downloadAttachment + * @return void */ - private bool $isPublic; - private bool $downloadAttachment; - private IConfig $config; - private IRequest $request; - private IPreview $previewManager; - - public function __construct(Tree $tree, - IConfig $config, - IRequest $request, - IPreview $previewManager, - IUserSession $userSession, - bool $isPublic = false, - bool $downloadAttachment = true) { - $this->tree = $tree; - $this->config = $config; - $this->request = $request; - $this->userSession = $userSession; - $this->isPublic = $isPublic; - $this->downloadAttachment = $downloadAttachment; - $this->previewManager = $previewManager; + public function __construct( + private Tree $tree, + private IConfig $config, + private IRequest $request, + private IPreview $previewManager, + private IUserSession $userSession, + private IFilenameValidator $validator, + private bool $isPublic = false, + private bool $downloadAttachment = true, + ) { } /** @@ -142,6 +140,46 @@ public function initialize(Server $server) { } }); $this->server->on('beforeMove', [$this, 'checkMove']); + $this->server->on('beforeCopy', [$this, 'checkCopy']); + } + + /** + * Plugin that checks if a copy can actually be performed. + * + * @param string $source source path + * @param string $destination destination path + * @throws NotFound If the source does not exist + * @throws InvalidPath If the destination is invalid + */ + public function checkCopy($source, $destination) { + $sourceNode = $this->tree->getNodeForPath($source); + if (!$sourceNode instanceof Node) { + return; + } + + // Ensure source exists + $sourceNodeFileInfo = $sourceNode->getFileInfo(); + if ($sourceNodeFileInfo === null) { + throw new NotFound($source . ' does not exist'); + } + // Ensure the target name is valid + try { + [$targetPath, $targetName] = \Sabre\Uri\split($destination); + $this->validator->validateFilename($targetName); + } catch (InvalidPathException $e) { + throw new InvalidPath($e->getMessage(), false); + } + // Ensure the target path is valid + try { + $segments = array_slice(explode('/', $targetPath), 2); + foreach ($segments as $segment) { + $this->validator->validateFilename($segment); + } + } catch (InvalidPathException) { + $l = \OCP\Server::get(IFactory::class)->get('dav'); + throw new InvalidPath($l->t('Invalid target path')); + } + } /** @@ -149,26 +187,23 @@ public function initialize(Server $server) { * * @param string $source source path * @param string $destination destination path - * @throws Forbidden - * @throws NotFound + * @throws Forbidden If the source is not deletable + * @throws NotFound If the source does not exist + * @throws InvalidPath If the destination name is invalid */ public function checkMove($source, $destination) { $sourceNode = $this->tree->getNodeForPath($source); if (!$sourceNode instanceof Node) { return; } - [$sourceDir,] = \Sabre\Uri\split($source); - [$destinationDir,] = \Sabre\Uri\split($destination); - if ($sourceDir !== $destinationDir) { - $sourceNodeFileInfo = $sourceNode->getFileInfo(); - if ($sourceNodeFileInfo === null) { - throw new NotFound($source . ' does not exist'); - } + // First check copyable (move only needs additional delete permission) + $this->checkCopy($source, $destination); - if (!$sourceNodeFileInfo->isDeletable()) { - throw new Forbidden($source . ' cannot be deleted'); - } + // The source needs to be deletable for moving + $sourceNodeFileInfo = $sourceNode->getFileInfo(); + if (!$sourceNodeFileInfo->isDeletable()) { + throw new Forbidden($source . ' cannot be deleted'); } } diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index 973d5ca6f0ea9..ab07f1e73b01f 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -117,8 +117,8 @@ public function getPath() { * @throws \Sabre\DAV\Exception\Forbidden */ public function setName($name) { - // rename is only allowed if the update privilege is granted - if (!($this->info->isUpdateable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) { + // rename is only allowed if the delete privilege is granted + if (!($this->info->isDeletable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) { throw new \Sabre\DAV\Exception\Forbidden(); } @@ -129,7 +129,6 @@ public function setName($name) { // verify path of the target $this->verifyPath($newPath); - if (!$this->fileView->rename($this->path, $newPath)) { throw new \Sabre\DAV\Exception('Failed to rename '. $this->path . ' to ' . $newPath); } diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 89d9b86becc83..fc35d78a2f754 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -13,6 +13,7 @@ use OCA\DAV\Files\BrowserErrorPagePlugin; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; +use OCP\Files\IFilenameValidator; use OCP\Files\Mount\IMountManager; use OCP\IConfig; use OCP\IDBConnection; @@ -129,6 +130,7 @@ public function createServer(string $baseUri, $this->request, $this->previewManager, $this->userSession, + \OCP\Server::get(IFilenameValidator::class), false, !$this->config->getSystemValue('debug', false) ) diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 4fdd70d05c738..449b8ef0ceb81 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -53,9 +53,13 @@ use OCP\AppFramework\Http\Response; use OCP\Diagnostics\IEventLogger; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\IFilenameValidator; use OCP\FilesMetadata\IFilesMetadataManager; use OCP\ICacheFactory; +use OCP\IConfig; +use OCP\IPreview; use OCP\IRequest; +use OCP\IUserSession; use OCP\Profiler\IProfiler; use OCP\SabrePluginEvent; use Psr\Log\LoggerInterface; @@ -236,15 +240,17 @@ public function __construct(IRequest $request, string $baseUri) { $user = $userSession->getUser(); if ($user !== null) { $view = \OC\Files\Filesystem::getView(); + $config = \OCP\Server::get(IConfig::class); $this->server->addPlugin( new FilesPlugin( $this->server->tree, - \OC::$server->getConfig(), + $config, $this->request, - \OC::$server->getPreviewManager(), - \OC::$server->getUserSession(), + \OCP\Server::get(IPreview::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(IFilenameValidator::class), false, - !\OC::$server->getConfig()->getSystemValue('debug', false) + $config->getSystemValueBool('debug', false) === false, ) ); $this->server->addPlugin(new ChecksumUpdatePlugin()); diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index 144b8af76598f..9d8f4e8d4c4f7 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -9,10 +9,13 @@ use OC\User\User; use OCA\DAV\Connector\Sabre\Directory; +use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCA\DAV\Connector\Sabre\File; use OCA\DAV\Connector\Sabre\FilesPlugin; use OCA\DAV\Connector\Sabre\Node; use OCP\Files\FileInfo; +use OCP\Files\IFilenameValidator; +use OCP\Files\InvalidPathException; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; use OCP\IPreview; @@ -32,51 +35,15 @@ * @group DB */ class FilesPluginTest extends TestCase { - public const GETETAG_PROPERTYNAME = FilesPlugin::GETETAG_PROPERTYNAME; - public const FILEID_PROPERTYNAME = FilesPlugin::FILEID_PROPERTYNAME; - public const INTERNAL_FILEID_PROPERTYNAME = FilesPlugin::INTERNAL_FILEID_PROPERTYNAME; - public const SIZE_PROPERTYNAME = FilesPlugin::SIZE_PROPERTYNAME; - public const PERMISSIONS_PROPERTYNAME = FilesPlugin::PERMISSIONS_PROPERTYNAME; - public const LASTMODIFIED_PROPERTYNAME = FilesPlugin::LASTMODIFIED_PROPERTYNAME; - public const CREATIONDATE_PROPERTYNAME = FilesPlugin::CREATIONDATE_PROPERTYNAME; - public const DOWNLOADURL_PROPERTYNAME = FilesPlugin::DOWNLOADURL_PROPERTYNAME; - public const OWNER_ID_PROPERTYNAME = FilesPlugin::OWNER_ID_PROPERTYNAME; - public const OWNER_DISPLAY_NAME_PROPERTYNAME = FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME; - public const DATA_FINGERPRINT_PROPERTYNAME = FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME; - public const HAS_PREVIEW_PROPERTYNAME = FilesPlugin::HAS_PREVIEW_PROPERTYNAME; - /** - * @var \Sabre\DAV\Server | \PHPUnit\Framework\MockObject\MockObject - */ - private $server; - - /** - * @var \Sabre\DAV\Tree | \PHPUnit\Framework\MockObject\MockObject - */ - private $tree; - - /** - * @var FilesPlugin - */ - private $plugin; - - /** - * @var \OCP\IConfig | \PHPUnit\Framework\MockObject\MockObject - */ - private $config; - - /** - * @var \OCP\IRequest | \PHPUnit\Framework\MockObject\MockObject - */ - private $request; - - /** - * @var \OCP\IPreview | \PHPUnit\Framework\MockObject\MockObject - */ - private $previewManager; - - /** @var IUserSession|MockObject */ - private $userSession; + private Tree&MockObject $tree; + private Server&MockObject $server; + private IConfig&MockObject $config; + private IRequest&MockObject $request; + private IPreview&MockObject $previewManager; + private IUserSession&MockObject $userSession; + private IFilenameValidator&MockObject $filenameValidator; + private FilesPlugin $plugin; protected function setUp(): void { parent::setUp(); @@ -89,13 +56,15 @@ protected function setUp(): void { $this->request = $this->createMock(IRequest::class); $this->previewManager = $this->createMock(IPreview::class); $this->userSession = $this->createMock(IUserSession::class); + $this->filenameValidator = $this->createMock(IFilenameValidator::class); $this->plugin = new FilesPlugin( $this->tree, $this->config, $this->request, $this->previewManager, - $this->userSession + $this->userSession, + $this->filenameValidator, ); $response = $this->getMockBuilder(ResponseInterface::class) @@ -160,16 +129,16 @@ public function testGetPropertiesForFile(): void { $propFind = new PropFind( '/dummyPath', [ - self::GETETAG_PROPERTYNAME, - self::FILEID_PROPERTYNAME, - self::INTERNAL_FILEID_PROPERTYNAME, - self::SIZE_PROPERTYNAME, - self::PERMISSIONS_PROPERTYNAME, - self::DOWNLOADURL_PROPERTYNAME, - self::OWNER_ID_PROPERTYNAME, - self::OWNER_DISPLAY_NAME_PROPERTYNAME, - self::DATA_FINGERPRINT_PROPERTYNAME, - self::CREATIONDATE_PROPERTYNAME, + FilesPlugin::GETETAG_PROPERTYNAME, + FilesPlugin::FILEID_PROPERTYNAME, + FilesPlugin::INTERNAL_FILEID_PROPERTYNAME, + FilesPlugin::SIZE_PROPERTYNAME, + FilesPlugin::PERMISSIONS_PROPERTYNAME, + FilesPlugin::DOWNLOADURL_PROPERTYNAME, + FilesPlugin::OWNER_ID_PROPERTYNAME, + FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME, + FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME, + FilesPlugin::CREATIONDATE_PROPERTYNAME, ], 0 ); @@ -197,16 +166,16 @@ public function testGetPropertiesForFile(): void { $node ); - $this->assertEquals('"abc"', $propFind->get(self::GETETAG_PROPERTYNAME)); - $this->assertEquals('00000123instanceid', $propFind->get(self::FILEID_PROPERTYNAME)); - $this->assertEquals('123', $propFind->get(self::INTERNAL_FILEID_PROPERTYNAME)); - $this->assertEquals('1973-11-29T21:33:09+00:00', $propFind->get(self::CREATIONDATE_PROPERTYNAME)); - $this->assertEquals(0, $propFind->get(self::SIZE_PROPERTYNAME)); - $this->assertEquals('DWCKMSR', $propFind->get(self::PERMISSIONS_PROPERTYNAME)); - $this->assertEquals('http://example.com/', $propFind->get(self::DOWNLOADURL_PROPERTYNAME)); - $this->assertEquals('foo', $propFind->get(self::OWNER_ID_PROPERTYNAME)); - $this->assertEquals('M. Foo', $propFind->get(self::OWNER_DISPLAY_NAME_PROPERTYNAME)); - $this->assertEquals('my_fingerprint', $propFind->get(self::DATA_FINGERPRINT_PROPERTYNAME)); + $this->assertEquals('"abc"', $propFind->get(FilesPlugin::GETETAG_PROPERTYNAME)); + $this->assertEquals('00000123instanceid', $propFind->get(FilesPlugin::FILEID_PROPERTYNAME)); + $this->assertEquals('123', $propFind->get(FilesPlugin::INTERNAL_FILEID_PROPERTYNAME)); + $this->assertEquals('1973-11-29T21:33:09+00:00', $propFind->get(FilesPlugin::CREATIONDATE_PROPERTYNAME)); + $this->assertEquals(0, $propFind->get(FilesPlugin::SIZE_PROPERTYNAME)); + $this->assertEquals('DWCKMSR', $propFind->get(FilesPlugin::PERMISSIONS_PROPERTYNAME)); + $this->assertEquals('http://example.com/', $propFind->get(FilesPlugin::DOWNLOADURL_PROPERTYNAME)); + $this->assertEquals('foo', $propFind->get(FilesPlugin::OWNER_ID_PROPERTYNAME)); + $this->assertEquals('M. Foo', $propFind->get(FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME)); + $this->assertEquals('my_fingerprint', $propFind->get(FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME)); $this->assertEquals([], $propFind->get404Properties()); } @@ -217,7 +186,7 @@ public function testGetPropertiesStorageNotAvailable(): void { $propFind = new PropFind( '/dummyPath', [ - self::DOWNLOADURL_PROPERTYNAME, + FilesPlugin::DOWNLOADURL_PROPERTYNAME, ], 0 ); @@ -231,25 +200,29 @@ public function testGetPropertiesStorageNotAvailable(): void { $node ); - $this->assertEquals(null, $propFind->get(self::DOWNLOADURL_PROPERTYNAME)); + $this->assertEquals(null, $propFind->get(FilesPlugin::DOWNLOADURL_PROPERTYNAME)); } public function testGetPublicPermissions(): void { + /** @var IRequest&MockObject */ + $request = $this->getMockBuilder(IRequest::class) + ->disableOriginalConstructor() + ->getMock(); $this->plugin = new FilesPlugin( $this->tree, $this->config, - $this->getMockBuilder(IRequest::class) - ->disableOriginalConstructor() - ->getMock(), + $request, $this->previewManager, $this->userSession, - true); + $this->filenameValidator, + true, + ); $this->plugin->initialize($this->server); $propFind = new PropFind( '/dummyPath', [ - self::PERMISSIONS_PROPERTYNAME, + FilesPlugin::PERMISSIONS_PROPERTYNAME, ], 0 ); @@ -265,7 +238,7 @@ public function testGetPublicPermissions(): void { $node ); - $this->assertEquals('DWCKR', $propFind->get(self::PERMISSIONS_PROPERTYNAME)); + $this->assertEquals('DWCKR', $propFind->get(FilesPlugin::PERMISSIONS_PROPERTYNAME)); } public function testGetPropertiesForDirectory(): void { @@ -275,12 +248,12 @@ public function testGetPropertiesForDirectory(): void { $propFind = new PropFind( '/dummyPath', [ - self::GETETAG_PROPERTYNAME, - self::FILEID_PROPERTYNAME, - self::SIZE_PROPERTYNAME, - self::PERMISSIONS_PROPERTYNAME, - self::DOWNLOADURL_PROPERTYNAME, - self::DATA_FINGERPRINT_PROPERTYNAME, + FilesPlugin::GETETAG_PROPERTYNAME, + FilesPlugin::FILEID_PROPERTYNAME, + FilesPlugin::SIZE_PROPERTYNAME, + FilesPlugin::PERMISSIONS_PROPERTYNAME, + FilesPlugin::DOWNLOADURL_PROPERTYNAME, + FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME, ], 0 ); @@ -294,13 +267,13 @@ public function testGetPropertiesForDirectory(): void { $node ); - $this->assertEquals('"abc"', $propFind->get(self::GETETAG_PROPERTYNAME)); - $this->assertEquals('00000123instanceid', $propFind->get(self::FILEID_PROPERTYNAME)); - $this->assertEquals(1025, $propFind->get(self::SIZE_PROPERTYNAME)); - $this->assertEquals('DWCKMSR', $propFind->get(self::PERMISSIONS_PROPERTYNAME)); - $this->assertEquals(null, $propFind->get(self::DOWNLOADURL_PROPERTYNAME)); - $this->assertEquals('my_fingerprint', $propFind->get(self::DATA_FINGERPRINT_PROPERTYNAME)); - $this->assertEquals([self::DOWNLOADURL_PROPERTYNAME], $propFind->get404Properties()); + $this->assertEquals('"abc"', $propFind->get(FilesPlugin::GETETAG_PROPERTYNAME)); + $this->assertEquals('00000123instanceid', $propFind->get(FilesPlugin::FILEID_PROPERTYNAME)); + $this->assertEquals(1025, $propFind->get(FilesPlugin::SIZE_PROPERTYNAME)); + $this->assertEquals('DWCKMSR', $propFind->get(FilesPlugin::PERMISSIONS_PROPERTYNAME)); + $this->assertEquals(null, $propFind->get(FilesPlugin::DOWNLOADURL_PROPERTYNAME)); + $this->assertEquals('my_fingerprint', $propFind->get(FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME)); + $this->assertEquals([FilesPlugin::DOWNLOADURL_PROPERTYNAME], $propFind->get404Properties()); } public function testGetPropertiesForRootDirectory(): void { @@ -322,7 +295,7 @@ public function testGetPropertiesForRootDirectory(): void { $propFind = new PropFind( '/', [ - self::DATA_FINGERPRINT_PROPERTYNAME, + FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME, ], 0 ); @@ -332,7 +305,7 @@ public function testGetPropertiesForRootDirectory(): void { $node ); - $this->assertEquals('my_fingerprint', $propFind->get(self::DATA_FINGERPRINT_PROPERTYNAME)); + $this->assertEquals('my_fingerprint', $propFind->get(FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME)); } public function testGetPropertiesWhenNoPermission(): void { @@ -358,7 +331,7 @@ public function testGetPropertiesWhenNoPermission(): void { $propFind = new PropFind( '/test', [ - self::DATA_FINGERPRINT_PROPERTYNAME, + FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME, ], 0 ); @@ -392,9 +365,9 @@ public function testUpdateProps(): void { // properties to set $propPatch = new PropPatch([ - self::GETETAG_PROPERTYNAME => 'newetag', - self::LASTMODIFIED_PROPERTYNAME => $testDate, - self::CREATIONDATE_PROPERTYNAME => $testCreationDate, + FilesPlugin::GETETAG_PROPERTYNAME => 'newetag', + FilesPlugin::LASTMODIFIED_PROPERTYNAME => $testDate, + FilesPlugin::CREATIONDATE_PROPERTYNAME => $testCreationDate, ]); @@ -408,19 +381,19 @@ public function testUpdateProps(): void { $this->assertEmpty($propPatch->getRemainingMutations()); $result = $propPatch->getResult(); - $this->assertEquals(200, $result[self::LASTMODIFIED_PROPERTYNAME]); - $this->assertEquals(200, $result[self::GETETAG_PROPERTYNAME]); - $this->assertEquals(200, $result[self::CREATIONDATE_PROPERTYNAME]); + $this->assertEquals(200, $result[FilesPlugin::LASTMODIFIED_PROPERTYNAME]); + $this->assertEquals(200, $result[FilesPlugin::GETETAG_PROPERTYNAME]); + $this->assertEquals(200, $result[FilesPlugin::CREATIONDATE_PROPERTYNAME]); } public function testUpdatePropsForbidden(): void { $propPatch = new PropPatch([ - self::OWNER_ID_PROPERTYNAME => 'user2', - self::OWNER_DISPLAY_NAME_PROPERTYNAME => 'User Two', - self::FILEID_PROPERTYNAME => 12345, - self::PERMISSIONS_PROPERTYNAME => 'C', - self::SIZE_PROPERTYNAME => 123, - self::DOWNLOADURL_PROPERTYNAME => 'http://example.com/', + FilesPlugin::OWNER_ID_PROPERTYNAME => 'user2', + FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME => 'User Two', + FilesPlugin::FILEID_PROPERTYNAME => 12345, + FilesPlugin::PERMISSIONS_PROPERTYNAME => 'C', + FilesPlugin::SIZE_PROPERTYNAME => 123, + FilesPlugin::DOWNLOADURL_PROPERTYNAME => 'http://example.com/', ]); $this->plugin->handleUpdateProperties( @@ -433,16 +406,16 @@ public function testUpdatePropsForbidden(): void { $this->assertEmpty($propPatch->getRemainingMutations()); $result = $propPatch->getResult(); - $this->assertEquals(403, $result[self::OWNER_ID_PROPERTYNAME]); - $this->assertEquals(403, $result[self::OWNER_DISPLAY_NAME_PROPERTYNAME]); - $this->assertEquals(403, $result[self::FILEID_PROPERTYNAME]); - $this->assertEquals(403, $result[self::PERMISSIONS_PROPERTYNAME]); - $this->assertEquals(403, $result[self::SIZE_PROPERTYNAME]); - $this->assertEquals(403, $result[self::DOWNLOADURL_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::OWNER_ID_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::FILEID_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::PERMISSIONS_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::SIZE_PROPERTYNAME]); + $this->assertEquals(403, $result[FilesPlugin::DOWNLOADURL_PROPERTYNAME]); } /** - * Testcase from https://github.com/owncloud/core/issues/5251 + * Test case from https://github.com/owncloud/core/issues/5251 * * |-FolderA * |-text.txt @@ -466,11 +439,12 @@ public function testMoveSrcNotDeletable(): void { $node = $this->getMockBuilder(Node::class) ->disableOriginalConstructor() ->getMock(); - $node->expects($this->once()) + $node->expects($this->atLeastOnce()) ->method('getFileInfo') ->willReturn($fileInfoFolderATestTXT); - $this->tree->expects($this->once())->method('getNodeForPath') + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') ->willReturn($node); $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); @@ -487,17 +461,17 @@ public function testMoveSrcDeletable(): void { $node = $this->getMockBuilder(Node::class) ->disableOriginalConstructor() ->getMock(); - $node->expects($this->once()) + $node->expects($this->atLeastOnce()) ->method('getFileInfo') ->willReturn($fileInfoFolderATestTXT); - $this->tree->expects($this->once())->method('getNodeForPath') + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') ->willReturn($node); $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); } - public function testMoveSrcNotExist(): void { $this->expectException(\Sabre\DAV\Exception\NotFound::class); $this->expectExceptionMessage('FolderA/test.txt does not exist'); @@ -505,16 +479,81 @@ public function testMoveSrcNotExist(): void { $node = $this->getMockBuilder(Node::class) ->disableOriginalConstructor() ->getMock(); - $node->expects($this->once()) + $node->expects($this->atLeastOnce()) ->method('getFileInfo') ->willReturn(null); - $this->tree->expects($this->once())->method('getNodeForPath') + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') ->willReturn($node); $this->plugin->checkMove('FolderA/test.txt', 'test.txt'); } + public function testMoveDestinationInvalid(): void { + $this->expectException(InvalidPath::class); + $this->expectExceptionMessage('Mocked exception'); + + $fileInfoFolderATestTXT = $this->createMock(FileInfo::class); + $fileInfoFolderATestTXT->expects(self::any()) + ->method('isDeletable') + ->willReturn(true); + + $node = $this->createMock(Node::class); + $node->expects($this->atLeastOnce()) + ->method('getFileInfo') + ->willReturn($fileInfoFolderATestTXT); + + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') + ->willReturn($node); + + $this->filenameValidator->expects(self::once()) + ->method('validateFilename') + ->with('invalid\\path.txt') + ->willThrowException(new InvalidPathException('Mocked exception')); + + $this->plugin->checkMove('FolderA/test.txt', 'invalid\\path.txt'); + } + + public function testCopySrcNotExist(): void { + $this->expectException(\Sabre\DAV\Exception\NotFound::class); + $this->expectExceptionMessage('FolderA/test.txt does not exist'); + + $node = $this->createMock(Node::class); + $node->expects($this->atLeastOnce()) + ->method('getFileInfo') + ->willReturn(null); + + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') + ->willReturn($node); + + $this->plugin->checkCopy('FolderA/test.txt', 'test.txt'); + } + + public function testCopyDestinationInvalid(): void { + $this->expectException(InvalidPath::class); + $this->expectExceptionMessage('Mocked exception'); + + $fileInfoFolderATestTXT = $this->createMock(FileInfo::class); + $node = $this->createMock(Node::class); + $node->expects($this->atLeastOnce()) + ->method('getFileInfo') + ->willReturn($fileInfoFolderATestTXT); + + $this->tree->expects($this->atLeastOnce()) + ->method('getNodeForPath') + ->willReturn($node); + + $this->filenameValidator->expects(self::once()) + ->method('validateFilename') + ->with('invalid\\path.txt') + ->willThrowException(new InvalidPathException('Mocked exception')); + + $this->plugin->checkCopy('FolderA/test.txt', 'invalid\\path.txt'); + } + public function downloadHeadersProvider() { return [ [ @@ -581,7 +620,7 @@ public function testHasPreview(): void { $propFind = new PropFind( '/dummyPath', [ - self::HAS_PREVIEW_PROPERTYNAME + FilesPlugin::HAS_PREVIEW_PROPERTYNAME ], 0 ); @@ -595,6 +634,6 @@ public function testHasPreview(): void { $node ); - $this->assertEquals('false', $propFind->get(self::HAS_PREVIEW_PROPERTYNAME)); + $this->assertEquals('false', $propFind->get(FilesPlugin::HAS_PREVIEW_PROPERTYNAME)); } } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 95fd79fa5f1f0..76a70a93e1313 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -14,6 +14,7 @@ use OCP\Files\File; use OCP\Files\FileInfo; use OCP\Files\Folder; +use OCP\Files\IFilenameValidator; use OCP\IConfig; use OCP\IGroupManager; use OCP\IPreview; @@ -32,43 +33,20 @@ use Sabre\HTTP\ResponseInterface; class FilesReportPluginTest extends \Test\TestCase { - /** @var \Sabre\DAV\Server|MockObject */ - private $server; - /** @var \Sabre\DAV\Tree|MockObject */ - private $tree; - - /** @var ISystemTagObjectMapper|MockObject */ - private $tagMapper; - - /** @var ISystemTagManager|MockObject */ - private $tagManager; - - /** @var ITags|MockObject */ - private $privateTags; - - private ITagManager|MockObject $privateTagManager; - - /** @var \OCP\IUserSession */ - private $userSession; - - /** @var FilesReportPluginImplementation */ - private $plugin; - - /** @var View|MockObject * */ - private $view; - - /** @var IGroupManager|MockObject * */ - private $groupManager; - - /** @var Folder|MockObject * */ - private $userFolder; - - /** @var IPreview|MockObject * */ - private $previewManager; - - /** @var IAppManager|MockObject * */ - private $appManager; + private \Sabre\DAV\Server&MockObject $server; + private Tree&MockObject $tree; + private ISystemTagObjectMapper&MockObject $tagMapper; + private ISystemTagManager&MockObject $tagManager; + private ITags&MockObject $privateTags; + private ITagManager&MockObject $privateTagManager; + private IUserSession&MockObject $userSession; + private FilesReportPluginImplementation $plugin; + private View&MockObject $view; + private IGroupManager&MockObject $groupManager; + private Folder&MockObject $userFolder; + private IPreview&MockObject $previewManager; + private IAppManager&MockObject $appManager; protected function setUp(): void { parent::setUp(); @@ -82,7 +60,7 @@ protected function setUp(): void { $this->server = $this->getMockBuilder('\Sabre\DAV\Server') ->setConstructorArgs([$this->tree]) - ->setMethods(['getRequestUri', 'getBaseUri']) + ->onlyMethods(['getRequestUri', 'getBaseUri']) ->getMock(); $this->server->expects($this->any()) @@ -311,7 +289,7 @@ public function testFindNodesByFileIdsRoot(): void { $filesNode2, ); - /** @var \OCA\DAV\Connector\Sabre\Directory|MockObject $reportTargetNode */ + /** @var \OCA\DAV\Connector\Sabre\Directory&MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -364,7 +342,7 @@ public function testFindNodesByFileIdsSubDir(): void { $filesNode2, ); - /** @var \OCA\DAV\Connector\Sabre\Directory|MockObject $reportTargetNode */ + /** @var \OCA\DAV\Connector\Sabre\Directory&MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -409,13 +387,16 @@ public function testPrepareResponses(): void { ->disableOriginalConstructor() ->getMock(); + $validator = $this->createMock(IFilenameValidator::class); + $this->server->addPlugin( new \OCA\DAV\Connector\Sabre\FilesPlugin( $this->tree, $config, $this->createMock(IRequest::class), $this->previewManager, - $this->createMock(IUserSession::class) + $this->createMock(IUserSession::class), + $validator, ) ); $this->plugin->initialize($this->server); diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index 962d1bb6d4400..5dbdc4054bf33 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -463,7 +463,7 @@ public function move($targetPath) { $this->path = $targetPath; return $targetNode; } else { - throw new NotPermittedException('No permission to move to path ' . $targetPath . ($parent instanceof Folder ? 'true':'faöse'). ($this->isValidPath($targetPath) ? 'true':'false')); + throw new NotPermittedException('No permission to move to path ' . $targetPath); } }