upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-04-22 23:03:00 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 78

Merge pull request #47185 from nextcloud/fix/filename-validation

Browse source 
fix: Filename validation should only forbid `create` and `update`
This commit is contained in:
Andy Scherzinger 2024-08-28 18:39:16 +02:00 committed by GitHub
commit ffde0c993e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 313 additions and 225 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
apps/dav/lib/Connector/Sabre/Directory.php
View file

@ -173,7 +173,7 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol
$path = $this->path . '/' . $name;
if (is_null($info)) {
try {
$this->fileView->verifyPath($this->path, $name);
$this->fileView->verifyPath($this->path, $name, true);
$info = $this->fileView->getFileInfo($path);
} catch (\OCP\Files\StorageNotAvailableException $e) {
throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage(), 0, $e);

108
apps/dav/lib/Connector/Sabre/FilesPlugin.php
View file

@ -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\IConfig;
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;
@ -64,33 +68,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,
) {
}
/**
@ -140,33 +138,67 @@ class FilesPlugin extends ServerPlugin {
}
});
$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 $target target path
* @throws NotFound If the source does not exist
* @throws InvalidPath If the target is invalid
*/
public function checkCopy($source, $target): void {
$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($target);
$this->validator->validateFilename($targetName);
} catch (InvalidPathException $e) {
throw new InvalidPath($e->getMessage(), false);
}
// Ensure the target path is valid
$segments = array_slice(explode('/', $targetPath), 2);
foreach ($segments as $segment) {
if ($this->validator->isFilenameValid($segment) === false) {
$l = \OCP\Server::get(IFactory::class)->get('dav');
throw new InvalidPath($l->t('Invalid target path'));
}
}
}
/**
* Plugin that checks if a move can actually be performed.
*
* @param string $source source path
* @param string $destination destination path
* @throws Forbidden
* @throws NotFound
* @param string $target target path
* @throws Forbidden If the source is not deletable
* @throws NotFound If the source does not exist
* @throws InvalidPath If the target name is invalid
*/
public function checkMove($source, $destination) {
public function checkMove(string $source, string $target): void {
$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, $target);
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');
}
}

6
apps/dav/lib/Connector/Sabre/Node.php
View file

@ -117,8 +117,9 @@ abstract class Node implements \Sabre\DAV\INode {
* @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
// (basically rename is a copy with delete of the original node)
if (!($this->info->isDeletable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) {
throw new \Sabre\DAV\Exception\Forbidden();
}
@ -129,7 +130,6 @@ abstract class Node implements \Sabre\DAV\INode {
// 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);
}

2
apps/dav/lib/Connector/Sabre/ServerFactory.php
View file

@ -13,6 +13,7 @@ use OCA\DAV\DAV\ViewOnlyPlugin;
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 @@ class ServerFactory {
$this->request,
$this->previewManager,
$this->userSession,
\OCP\Server::get(IFilenameValidator::class),
false,
!$this->config->getSystemValue('debug', false)
)

14
apps/dav/lib/Server.php
View file

@ -53,9 +53,13 @@ use OCA\DAV\Upload\ChunkingV2Plugin;
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 @@ class Server {
$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());

273
apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php
View file

@ -9,10 +9,13 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre;
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 @@ use Test\TestCase;
* @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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$propFind = new PropFind(
'/dummyPath',
[
self::DOWNLOADURL_PROPERTYNAME,
FilesPlugin::DOWNLOADURL_PROPERTYNAME,
],
0
);
@ -231,25 +200,29 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$propFind = new PropFind(
'/',
[
self::DATA_FINGERPRINT_PROPERTYNAME,
FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME,
],
0
);
@ -332,7 +305,7 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$propFind = new PropFind(
'/test',
[
self::DATA_FINGERPRINT_PROPERTYNAME,
FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME,
],
0
);
@ -392,9 +365,9 @@ class FilesPluginTest extends TestCase {
// 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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$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 @@ class FilesPluginTest extends TestCase {
$propFind = new PropFind(
'/dummyPath',
[
self::HAS_PREVIEW_PROPERTYNAME
FilesPlugin::HAS_PREVIEW_PROPERTYNAME
],
0
);
@ -595,6 +634,6 @@ class FilesPluginTest extends TestCase {
$node
);
$this->assertEquals('false', $propFind->get(self::HAS_PREVIEW_PROPERTYNAME));
$this->assertEquals('false', $propFind->get(FilesPlugin::HAS_PREVIEW_PROPERTYNAME));
}
}

61
apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php
View file

@ -14,6 +14,7 @@ use OCP\App\IAppManager;
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\DAV\Tree;
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 @@ class FilesReportPluginTest extends \Test\TestCase {
$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 @@ class FilesReportPluginTest extends \Test\TestCase {
$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 @@ class FilesReportPluginTest extends \Test\TestCase {
$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 @@ class FilesReportPluginTest extends \Test\TestCase {
->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);

2
apps/files/src/actions/moveOrCopyActionUtils.ts
View file

@ -38,7 +38,7 @@ export type MoveCopyResult = {
export const canMove = (nodes: Node[]) => {
const minPermission = nodes.reduce((min, node) => Math.min(min, node.permissions), Permission.ALL)
return (minPermission & Permission.UPDATE) !== 0
return Boolean(minPermission & Permission.DELETE)
}
export const canDownload = (nodes: Node[]) => {

4
apps/files/src/actions/renameAction.spec.ts
View file

@ -30,14 +30,14 @@ describe('Rename action enabled tests', () => {
source: 'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
owner: 'admin',
mime: 'text/plain',
permissions: Permission.UPDATE,
permissions: Permission.UPDATE | Permission.DELETE,
})
expect(action.enabled).toBeDefined()
expect(action.enabled!([file], view)).toBe(true)
})
test('Disabled for node without UPDATE permission', () => {
test('Disabled for node without DELETE permission', () => {
const file = new File({
id: 1,
source: 'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',

2
apps/files/src/actions/renameAction.ts
View file

@ -17,7 +17,7 @@ export const action = new FileAction({
enabled: (nodes: Node[]) => {
return nodes.length > 0 && nodes
.map(node => node.permissions)
.every(permission => (permission & Permission.UPDATE) !== 0)
.every(permission => Boolean(permission & Permission.DELETE))
},
async exec(node: Node) {

4
dist/files-init.js vendored
View file

File diff suppressed because one or more lines are too long

2
dist/files-init.js.map vendored
View file

File diff suppressed because one or more lines are too long

4
dist/files-main.js vendored
View file

File diff suppressed because one or more lines are too long

2
dist/files-main.js.map vendored
View file

File diff suppressed because one or more lines are too long

6
lib/private/Files/FilenameValidator.php
View file

@ -25,6 +25,8 @@ use Psr\Log\LoggerInterface;
*/
class FilenameValidator implements IFilenameValidator {
public const INVALID_FILE_TYPE = 100;
private IL10N $l10n;
/**
@ -269,12 +271,12 @@ class FilenameValidator implements IFilenameValidator {
*/
protected function checkForbiddenExtension(string $filename): void {
$filename = mb_strtolower($filename);
// Check for forbidden filename exten<sions
// Check for forbidden filename extensions
$forbiddenExtensions = $this->getForbiddenExtensions();
foreach ($forbiddenExtensions as $extension) {
if (str_ends_with($filename, $extension)) {
if (str_starts_with($extension, '.')) {
throw new InvalidPathException($this->l10n->t('"%1$s" is a forbidden file type.', [$extension]));
throw new InvalidPathException($this->l10n->t('"%1$s" is a forbidden file type.', [$extension]), self::INVALID_FILE_TYPE);
} else {
throw new InvalidPathException($this->l10n->t('Filenames must not end with "%1$s".', [$extension]));
}

13
lib/private/Files/Node/Node.php
View file

@ -432,11 +432,14 @@ class Node implements INode {
$targetPath = $this->normalizePath($targetPath);
$parent = $this->root->get(dirname($targetPath));
if (
$parent instanceof Folder and
$this->isValidPath($targetPath) and
(
$parent->isCreatable() ||
($parent->getInternalPath() === '' && $parent->getMountPoint() instanceof MoveableMount)
($parent instanceof Folder)
&& $this->isValidPath($targetPath)
&& (
$parent->isCreatable()
|| (
$parent->getInternalPath() === ''
&& ($parent->getMountPoint() instanceof MoveableMount)
)
)
) {
$nonExisting = $this->createNonExistingNode($targetPath);

14
lib/private/Files/Storage/Common.php
View file

@ -13,6 +13,7 @@ use OC\Files\Cache\Propagator;
use OC\Files\Cache\Scanner;
use OC\Files\Cache\Updater;
use OC\Files\Cache\Watcher;
use OC\Files\FilenameValidator;
use OC\Files\Filesystem;
use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Wrapper\Wrapper;
@ -494,7 +495,18 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
$this->getFilenameValidator()
->validateFilename($fileName);
// NOTE: $path will remain unverified for now
// verify also the path is valid
if ($path && $path !== '/' && $path !== '.') {
try {
$this->verifyPath(dirname($path), basename($path));
} catch (InvalidPathException $e) {
// Ignore invalid file type exceptions on directories
if ($e->getCode() !== FilenameValidator::INVALID_FILE_TYPE) {
$l = \OCP\Util::getL10N('lib');
throw new InvalidPathException($l->t('Invalid parent path'), previous: $e);
}
}
}
}
/**

13
lib/private/Files/View.php
View file

@ -1826,15 +1826,26 @@ class View {
/**
* @param string $path
* @param string $fileName
* @param bool $readonly Check only if the path is allowed for read-only access
* @throws InvalidPathException
*/
public function verifyPath($path, $fileName): void {
public function verifyPath($path, $fileName, $readonly = false): void {
// All of the view's functions disallow '..' in the path so we can short cut if the path is invalid
if (!Filesystem::isValidPath($path ?: '/')) {
$l = \OCP\Util::getL10N('lib');
throw new InvalidPathException($l->t('Path contains invalid segments'));
}
// Short cut for read-only validation
if ($readonly) {
$validator = \OCP\Server::get(FilenameValidator::class);
if ($validator->isForbidden($fileName)) {
$l = \OCP\Util::getL10N('lib');
throw new InvalidPathException($l->t('Filename is a reserved word'));
}
return;
}
try {
/** @type \OCP\Files\Storage $storage */
[$storage, $internalPath] = $this->resolvePath($path);

6
tests/lib/Files/Node/IntegrationTest.php
View file

@ -12,6 +12,7 @@ use OC\Files\Storage\Temporary;
use OC\Files\View;
use OC\Memcache\ArrayCache;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Config\IUserMountCache;
use OCP\Files\Mount\IMountManager;
use OCP\ICacheFactory;
use OCP\IUserManager;
@ -46,8 +47,7 @@ class IntegrationTest extends \Test\TestCase {
protected function setUp(): void {
parent::setUp();
/** @var IMountManager $manager */
$manager = \OC::$server->get(IMountManager::class);
$manager = \OCP\Server::get(IMountManager::class);
\OC_Hook::clear('OC_Filesystem');
@ -64,7 +64,7 @@ class IntegrationTest extends \Test\TestCase {
$manager,
$this->view,
$user,
\OC::$server->getUserMountCache(),
\OCP\Server::get(IUserMountCache::class),
$this->createMock(LoggerInterface::class),
$this->createMock(IUserManager::class),
$this->createMock(IEventDispatcher::class),