From 8eb9cd7604ea23a3e4b8e3f3f3893c1a1506ef6c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 26 Jan 2026 15:40:14 +0100 Subject: [PATCH 1/4] fix: improve share mount conflict resolution logic Signed-off-by: Robin Appelman --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/files_sharing/lib/MountProvider.php | 16 +- .../lib/ShareTargetValidator.php | 140 ++++++++++++++++++ apps/files_sharing/lib/SharedMount.php | 79 ---------- 5 files changed, 152 insertions(+), 85 deletions(-) create mode 100644 apps/files_sharing/lib/ShareTargetValidator.php diff --git a/apps/files_sharing/composer/composer/autoload_classmap.php b/apps/files_sharing/composer/composer/autoload_classmap.php index f087686c4ca50..6de418dce0101 100644 --- a/apps/files_sharing/composer/composer/autoload_classmap.php +++ b/apps/files_sharing/composer/composer/autoload_classmap.php @@ -96,6 +96,7 @@ 'OCA\\Files_Sharing\\Settings\\Personal' => $baseDir . '/../lib/Settings/Personal.php', 'OCA\\Files_Sharing\\ShareBackend\\File' => $baseDir . '/../lib/ShareBackend/File.php', 'OCA\\Files_Sharing\\ShareBackend\\Folder' => $baseDir . '/../lib/ShareBackend/Folder.php', + 'OCA\\Files_Sharing\\ShareTargetValidator' => $baseDir . '/../lib/ShareTargetValidator.php', 'OCA\\Files_Sharing\\SharedMount' => $baseDir . '/../lib/SharedMount.php', 'OCA\\Files_Sharing\\SharedStorage' => $baseDir . '/../lib/SharedStorage.php', 'OCA\\Files_Sharing\\SharesReminderJob' => $baseDir . '/../lib/SharesReminderJob.php', diff --git a/apps/files_sharing/composer/composer/autoload_static.php b/apps/files_sharing/composer/composer/autoload_static.php index c39bd4d677a83..fb0da77fb4095 100644 --- a/apps/files_sharing/composer/composer/autoload_static.php +++ b/apps/files_sharing/composer/composer/autoload_static.php @@ -111,6 +111,7 @@ class ComposerStaticInitFiles_Sharing 'OCA\\Files_Sharing\\Settings\\Personal' => __DIR__ . '/..' . '/../lib/Settings/Personal.php', 'OCA\\Files_Sharing\\ShareBackend\\File' => __DIR__ . '/..' . '/../lib/ShareBackend/File.php', 'OCA\\Files_Sharing\\ShareBackend\\Folder' => __DIR__ . '/..' . '/../lib/ShareBackend/Folder.php', + 'OCA\\Files_Sharing\\ShareTargetValidator' => __DIR__ . '/..' . '/../lib/ShareTargetValidator.php', 'OCA\\Files_Sharing\\SharedMount' => __DIR__ . '/..' . '/../lib/SharedMount.php', 'OCA\\Files_Sharing\\SharedStorage' => __DIR__ . '/..' . '/../lib/SharedStorage.php', 'OCA\\Files_Sharing\\SharesReminderJob' => __DIR__ . '/..' . '/../lib/SharesReminderJob.php', diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index cc7ab6e827ce6..005bf9caf4212 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -11,7 +11,6 @@ use InvalidArgumentException; use OC\Files\View; use OCA\Files_Sharing\Event\ShareMountedEvent; -use OCP\Cache\CappedMemoryCache; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Config\IMountProvider; use OCP\Files\Config\IPartialMountProvider; @@ -29,6 +28,7 @@ use function count; class MountProvider implements IMountProvider, IPartialMountProvider { + /** * @param IConfig $config * @param IManager $shareManager @@ -41,6 +41,7 @@ public function __construct( protected IEventDispatcher $eventDispatcher, protected ICacheFactory $cacheFactory, protected IMountManager $mountManager, + protected ShareTargetValidator $shareTargetValidator, ) { } @@ -270,8 +271,6 @@ private function getMountsFromSuperShares( $view = new View('/' . $userId . '/files'); $ownerViews = []; $sharingDisabledForUser = $this->shareManager->sharingDisabledForUser($userId); - /** @var CappedMemoryCache $folderExistCache */ - $foldersExistCache = new CappedMemoryCache(); $validShareCache = $this->cacheFactory->createLocal('share-valid-mountpoint-max'); $maxValidatedShare = $validShareCache->get($userId) ?? 0; @@ -292,10 +291,17 @@ private function getMountsFromSuperShares( if (!isset($ownerViews[$owner])) { $ownerViews[$owner] = new View('/' . $owner . '/files'); } + $shareId = (int)$parentShare->getId(); + $absMountPoint = '/' . $user->getUID() . '/files/' . trim($parentShare->getTarget(), '/') . '/'; + + // after the mountpoint is verified for the first time, only new mountpoints (e.g. groupfolders can overwrite the target) + if ($shareId > $maxValidatedShare || isset($allMounts[$absMountPoint])) { + $this->shareTargetValidator->verifyMountPoint($user, $parentShare, $allMounts, $groupedShares); + } + $mount = new SharedMount( '\OCA\Files_Sharing\SharedStorage', - $allMounts, [ 'user' => $userId, // parent share @@ -307,10 +313,8 @@ private function getMountsFromSuperShares( ], $loader, $view, - $foldersExistCache, $this->eventDispatcher, $user, - $shareId <= $maxValidatedShare, ); $newMaxValidatedShare = max($shareId, $newMaxValidatedShare); diff --git a/apps/files_sharing/lib/ShareTargetValidator.php b/apps/files_sharing/lib/ShareTargetValidator.php new file mode 100644 index 0000000000000..6a1113bb07d0e --- /dev/null +++ b/apps/files_sharing/lib/ShareTargetValidator.php @@ -0,0 +1,140 @@ +folderExistsCache = new CappedMemoryCache(); + } + + private function getViewForUser(IUser $user): View { + /** + * @psalm-suppress InternalClass + * @psalm-suppress InternalMethod + */ + return new View('/' . $user->getUID() . '/files'); + } + + /** + * check if the parent folder exists otherwise move the mount point up + * + * @param array $allCachedMounts Other mounts for the user, indexed by path + * @param IShare[] $childShares + * @return string + */ + public function verifyMountPoint( + IUser $user, + IShare &$share, + array $allCachedMounts, + array $childShares, + ): string { + $mountPoint = basename($share->getTarget()); + $parent = dirname($share->getTarget()); + + $recipientView = $this->getViewForUser($user); + $event = new VerifyMountPointEvent($share, $recipientView, $parent); + $this->eventDispatcher->dispatchTyped($event); + $parent = $event->getParent(); + + /** @psalm-suppress InternalMethod */ + $absoluteParent = $recipientView->getAbsolutePath($parent); + $this->setupManager->setupForPath($absoluteParent); + $parentMount = $this->mountManager->find($absoluteParent); + + $cached = $this->folderExistsCache->get($parent); + if ($cached !== null) { + $parentExists = $cached; + } else { + $parentCache = $parentMount->getStorage()->getCache(); + $parentExists = $parentCache->inCache($parentMount->getInternalPath($absoluteParent)); + $this->folderExistsCache->set($parent, $parentExists); + } + if (!$parentExists) { + $parent = Helper::getShareFolder($recipientView, $user->getUID()); + /** @psalm-suppress InternalMethod */ + $absoluteParent = $recipientView->getAbsolutePath($parent); + } + + $newAbsoluteMountPoint = $this->generateUniqueTarget( + Filesystem::normalizePath($absoluteParent . '/' . $mountPoint), + $parentMount, + $allCachedMounts, + ); + + /** @psalm-suppress InternalMethod */ + $newMountPoint = $recipientView->getRelativePath($newAbsoluteMountPoint); + if ($newMountPoint === null) { + return $share->getTarget(); + } + + if ($newMountPoint !== $share->getTarget()) { + $this->updateFileTarget($user, $newMountPoint, $share, $childShares); + } + + return $newMountPoint; + } + + + /** + * @param IMountPoint[] $allCachedMounts + */ + private function generateUniqueTarget(string $absolutePath, IMountPoint $parentMount, array $allCachedMounts): string { + $pathInfo = pathinfo($absolutePath); + $ext = isset($pathInfo['extension']) ? '.' . $pathInfo['extension'] : ''; + $name = $pathInfo['filename']; + $dir = $pathInfo['dirname']; + + $i = 2; + $parentCache = $parentMount->getStorage()->getCache(); + $internalPath = $parentMount->getInternalPath($absolutePath); + while ($parentCache->inCache($internalPath) || isset($allCachedMounts[$absolutePath . '/'])) { + $absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext); + $internalPath = $parentMount->getInternalPath($absolutePath); + $i++; + } + + return $absolutePath; + } + + /** + * update fileTarget in the database if the mount point changed + * + * @param IShare[] $childShares + */ + private function updateFileTarget(IUser $user, string $newPath, IShare &$share, array $childShares) { + $share->setTarget($newPath); + + foreach ($childShares as $tmpShare) { + $tmpShare->setTarget($newPath); + $this->shareManager->moveShare($tmpShare, $user->getUID()); + } + } +} diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index 7dc95533d7497..4dff3bcf3b75b 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -13,14 +13,12 @@ use OC\Files\Mount\MoveableMount; use OC\Files\View; use OCA\Files_Sharing\Exceptions\BrokenPath; -use OCP\Cache\CappedMemoryCache; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\InvalidateMountCacheEvent; use OCP\Files\Storage\IStorageFactory; use OCP\IDBConnection; use OCP\IUser; use OCP\Server; -use OCP\Share\Events\VerifyMountPointEvent; use OCP\Share\IShare; use Psr\Log\LoggerInterface; @@ -41,73 +39,20 @@ class SharedMount extends MountPoint implements MoveableMount, ISharedMountPoint public function __construct( $storage, - array $mountpoints, $arguments, IStorageFactory $loader, private View $recipientView, - CappedMemoryCache $folderExistCache, private IEventDispatcher $eventDispatcher, private IUser $user, - bool $alreadyVerified, ) { $this->superShare = $arguments['superShare']; $this->groupedShares = $arguments['groupedShares']; $absMountPoint = '/' . $user->getUID() . '/files/' . trim($this->superShare->getTarget(), '/') . '/'; - // after the mountpoint is verified for the first time, only new mountpoints (e.g. groupfolders can overwrite the target) - if (!$alreadyVerified || isset($mountpoints[$absMountPoint])) { - $newMountPoint = $this->verifyMountPoint($this->superShare, $mountpoints, $folderExistCache); - $absMountPoint = '/' . $user->getUID() . '/files/' . trim($newMountPoint, '/') . '/'; - } - parent::__construct($storage, $absMountPoint, $arguments, $loader, null, null, MountProvider::class); } - /** - * check if the parent folder exists otherwise move the mount point up - * - * @param IShare $share - * @param SharedMount[] $mountpoints - * @param CappedMemoryCache $folderExistCache - * @return string - */ - private function verifyMountPoint( - IShare $share, - array $mountpoints, - CappedMemoryCache $folderExistCache, - ) { - $mountPoint = basename($share->getTarget()); - $parent = dirname($share->getTarget()); - - $event = new VerifyMountPointEvent($share, $this->recipientView, $parent); - $this->eventDispatcher->dispatchTyped($event); - $parent = $event->getParent(); - - $cached = $folderExistCache->get($parent); - if ($cached) { - $parentExists = $cached; - } else { - $parentExists = $this->recipientView->is_dir($parent); - $folderExistCache->set($parent, $parentExists); - } - if (!$parentExists) { - $parent = Helper::getShareFolder($this->recipientView, $this->user->getUID()); - } - - $newMountPoint = $this->generateUniqueTarget( - Filesystem::normalizePath($parent . '/' . $mountPoint), - $this->recipientView, - $mountpoints - ); - - if ($newMountPoint !== $share->getTarget()) { - $this->updateFileTarget($newMountPoint, $share); - } - - return $newMountPoint; - } - /** * update fileTarget in the database if the mount point changed * @@ -126,30 +71,6 @@ private function updateFileTarget($newPath, &$share) { $this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($this->user)); } - - /** - * @param string $path - * @param View $view - * @param SharedMount[] $mountpoints - * @return mixed - */ - private function generateUniqueTarget($path, $view, array $mountpoints) { - $pathinfo = pathinfo($path); - $ext = isset($pathinfo['extension']) ? '.' . $pathinfo['extension'] : ''; - $name = $pathinfo['filename']; - $dir = $pathinfo['dirname']; - - $i = 2; - $absolutePath = $this->recipientView->getAbsolutePath($path) . '/'; - while ($view->file_exists($path) || isset($mountpoints[$absolutePath])) { - $path = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext); - $absolutePath = $this->recipientView->getAbsolutePath($path) . '/'; - $i++; - } - - return $path; - } - /** * Format a path to be relative to the /user/files/ directory * From 810a0b253d4ba2c3f1dcc54bc61aed1fb3dca686 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 27 Jan 2026 17:26:26 +0100 Subject: [PATCH 2/4] test: adjust tests Signed-off-by: Robin Appelman --- apps/files_sharing/tests/MountProviderTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 82c503f4232af..090e2e353dd6b 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -10,6 +10,7 @@ use OC\Share20\Share; use OCA\Files_Sharing\MountProvider; use OCA\Files_Sharing\SharedMount; +use OCA\Files_Sharing\ShareTargetValidator; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Config\ICachedMountInfo; @@ -54,8 +55,9 @@ protected function setUp(): void { $cacheFactory = $this->createMock(ICacheFactory::class); $cacheFactory->method('createLocal')->willReturn($this->cache); $mountManager = $this->createMock(IMountManager::class); + $shareValidator = $this->createMock(ShareTargetValidator::class); - $this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory, $mountManager); + $this->provider = new MountProvider($this->config, $this->shareManager, $this->logger, $eventDispatcher, $cacheFactory, $mountManager, $shareValidator); } private function makeMockShareAttributes($attrs) { From 6b841417544752d7af1cc72186c4c431f16d3a51 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 14 Jan 2026 16:59:54 +0100 Subject: [PATCH 3/4] fix: attempt to make share conflict resolution more resilient to false positives Signed-off-by: Robin Appelman --- .../lib/ShareTargetValidator.php | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/ShareTargetValidator.php b/apps/files_sharing/lib/ShareTargetValidator.php index 6a1113bb07d0e..27c79962a324c 100644 --- a/apps/files_sharing/lib/ShareTargetValidator.php +++ b/apps/files_sharing/lib/ShareTargetValidator.php @@ -84,6 +84,7 @@ public function verifyMountPoint( } $newAbsoluteMountPoint = $this->generateUniqueTarget( + $share, Filesystem::normalizePath($absoluteParent . '/' . $mountPoint), $parentMount, $allCachedMounts, @@ -106,7 +107,12 @@ public function verifyMountPoint( /** * @param IMountPoint[] $allCachedMounts */ - private function generateUniqueTarget(string $absolutePath, IMountPoint $parentMount, array $allCachedMounts): string { + private function generateUniqueTarget( + IShare $share, + string $absolutePath, + IMountPoint $parentMount, + array $allCachedMounts, + ): string { $pathInfo = pathinfo($absolutePath); $ext = isset($pathInfo['extension']) ? '.' . $pathInfo['extension'] : ''; $name = $pathInfo['filename']; @@ -115,7 +121,7 @@ private function generateUniqueTarget(string $absolutePath, IMountPoint $parentM $i = 2; $parentCache = $parentMount->getStorage()->getCache(); $internalPath = $parentMount->getInternalPath($absolutePath); - while ($parentCache->inCache($internalPath) || isset($allCachedMounts[$absolutePath . '/'])) { + while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($share, $allCachedMounts, $absolutePath)) { $absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext); $internalPath = $parentMount->getInternalPath($absolutePath); $i++; @@ -124,6 +130,23 @@ private function generateUniqueTarget(string $absolutePath, IMountPoint $parentM return $absolutePath; } + /** + * @param IMountPoint[] $allCachedMounts + */ + private function hasConflictingMount(IShare $share, array $allCachedMounts, string $absolutePath): bool { + if (!isset($allCachedMounts[$absolutePath . '/'])) { + return false; + } + + $mount = $allCachedMounts[$absolutePath . '/']; + if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $share->getNodeId()) { + // "conflicting" mount is a mount for the current share + return false; + } + + return true; + } + /** * update fileTarget in the database if the mount point changed * From d0d38e49383b42fb063f7319dfcd6f9e72a6a6ce Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 27 Jan 2026 17:27:48 +0100 Subject: [PATCH 4/4] chore: update psalm baseline Signed-off-by: Robin Appelman --- build/psalm-baseline.xml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 84f4b71a65515..371f575b9a25e 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1663,12 +1663,6 @@ - - - - - -