From 4abda56f5ec5b3866eefd9645a7e23b994c8011f Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 10 Sep 2026 09:54:21 +0200 Subject: [PATCH 1/3] fix(files_sharing): log when a share is disabled by invalid source permissions `SharedStorage::isValid()` returns false when the share source lost its share permission, which makes every permission check return 0 while reading keeps working. For the recipient that looks like arbitrary breakage: uploads fail with 403, renames get reverted by the sync client, and the file list still shows the folder as writable. Nothing was logged, so there was no way to tell this apart from real corruption. Log a warning once per storage instance naming the share, the owner and the source file id plus its cached permissions, so the source that needs repairing can be found. Assisted-By: ClaudeCode:claude-opus-5 Signed-off-by: Marcel Klehr --- apps/files_sharing/lib/SharedStorage.php | 43 +++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index 7a2909c3ad839..aa27e3b2244dc 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -63,6 +63,8 @@ class SharedStorage extends Jail implements LegacyISharedStorage, ISharedStorage private $initialized = false; + private bool $invalidSourceLogged = false; + /** * @var ICacheEntry */ @@ -237,7 +239,46 @@ public function getShareId() { } private function isValid(): bool { - return $this->getSourceRootInfo() && ($this->getSourceRootInfo()->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE; + $sourceRootInfo = $this->getSourceRootInfo(); + if ($sourceRootInfo instanceof ICacheEntry + && ($sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE) { + return true; + } + + $this->logInvalidSource($sourceRootInfo); + + return false; + } + + /** + * An invalid share is disabled entirely: every permission check returns 0 while + * reading keeps working, which is easy to mistake for arbitrary breakage such as + * failing uploads or renames that revert. Leave a trace naming the share and the + * source that has to be repaired. + * + * @param ICacheEntry|false|null $sourceRootInfo + */ + private function logInvalidSource($sourceRootInfo): void { + if ($this->invalidSourceLogged) { + return; + } + $this->invalidSourceLogged = true; + + if (!($sourceRootInfo instanceof ICacheEntry) || $sourceRootInfo->getId() < 0) { + // The source could not be resolved at all, in which case `init()` has + // already swapped in a `FailedStorage` and `FailedCache` and the root info + // is only a read-only placeholder. That is a different problem from a + // source whose cached permissions are wrong, so don't report it as one. + return; + } + + $this->logger->warning('Share {shareId} is disabled because its source is missing the share permission. Repair the cached permissions of {shareOwner} with "occ files:scan", or check whether the source is masked by a read-only mount or access control rule.', [ + 'app' => 'files_sharing', + 'shareId' => $this->superShare->getId(), + 'shareOwner' => $this->superShare->getShareOwner(), + 'sourceFileId' => $sourceRootInfo->getId(), + 'sourcePermissions' => $sourceRootInfo->getPermissions(), + ]); } #[\Override] From 3900bc3039d07156187bd18fb37efa91ba1e6827 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 10 Sep 2026 09:56:45 +0200 Subject: [PATCH 2/3] fix(files_sharing): don't advertise permissions an invalid share denies When the source of a share lost its share permission in the file cache, `SharedStorage::getPermissions()` returns 0 and every write is refused. The shared cache never took part in that decision: it reported `filecache.permissions & share.permissions`, so the file list showed the folder and its contents as writable while every upload, move and rename was answered with a 403. The web UI turns that into "Operation is blocked by access control" and sync clients revert renames a few seconds later, which makes a wrong permission value look like arbitrary corruption. Mask the reported permissions down to read when the source is invalid, so what the cache advertises is what the storage grants. Reading is left untouched because it keeps working: `fopen()` in read mode bypasses the permission checks. The validity check moves into `SharedStorage::hasValidSourcePermissions()` so the storage and its cache share one definition. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Marcel Klehr --- apps/files_sharing/lib/Cache.php | 9 ++++ apps/files_sharing/lib/SharedStorage.php | 15 ++++++- .../files_sharing/tests/SharedStorageTest.php | 41 +++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php index 08a48c398f07f..23fad70b0909a 100644 --- a/apps/files_sharing/lib/Cache.php +++ b/apps/files_sharing/lib/Cache.php @@ -15,6 +15,7 @@ use OC\Files\Search\SearchComparison; use OC\Files\Storage\Wrapper\Jail; use OC\User\DisplayNameCache; +use OCP\Constants; use OCP\Files\Cache\ICache; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Search\ISearchBinaryOperator; @@ -154,6 +155,14 @@ protected function formatCacheEntry($entry, $path = null) { $entry['permissions'] = $this->storage->getPermissions($entry['path']); } + if (!SharedStorage::hasValidSourcePermissions($this->sourceRootInfo)) { + // SharedStorage disables a share whose source lost the share permission: + // every write is refused while reading still works. Report the same here, + // otherwise the file list advertises write permissions that every + // operation then denies with a 403. + $entry['permissions'] &= Constants::PERMISSION_READ; + } + if ($this->share->getNodeId() === $entry['fileid']) { $entry['name'] = basename($this->share->getTarget()); } diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index aa27e3b2244dc..608eac24af739 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -238,10 +238,21 @@ public function getShareId() { return $this->superShare->getId(); } + /** + * A share only works as long as its source still carries the share permission. + * Both this storage and its cache have to agree on that, so keep the check in + * one place. + * + * @param ICacheEntry|false|null $sourceRootInfo + */ + public static function hasValidSourcePermissions($sourceRootInfo): bool { + return $sourceRootInfo instanceof ICacheEntry + && ($sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE; + } + private function isValid(): bool { $sourceRootInfo = $this->getSourceRootInfo(); - if ($sourceRootInfo instanceof ICacheEntry - && ($sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE) { + if (self::hasValidSourcePermissions($sourceRootInfo)) { return true; } diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index 26e1e0a5e81c0..fcc1da200c622 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -443,6 +443,47 @@ public function testOwnerPermissions(): void { $this->shareManager->deleteShare($share); } + public function testInvalidSourcePermissionsAreReportedAsReadOnly(): void { + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + + $share = $this->share( + IShare::TYPE_USER, + $this->folder, + self::TEST_FILES_SHARING_API_USER1, + self::TEST_FILES_SHARING_API_USER2, + Constants::PERMISSION_ALL + ); + + // Drop the share permission of the source in the file cache, the way a scan + // through a permission mask used to persist masked permissions. This disables + // the share in `SharedStorage::isValid()`. + $sourceInfo = $this->view->getFileInfo($this->folder); + $sourceCache = $sourceInfo->getStorage()->getCache(); + $sourceCache->update($sourceInfo->getId(), [ + 'permissions' => Constants::PERMISSION_ALL & ~Constants::PERMISSION_SHARE, + ]); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER2); + $user2View = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + + // the storage refuses every write, so the cache must not advertise more than read + $folderInfo = $user2View->getFileInfo($this->folder); + $this->assertNotFalse($folderInfo); + $this->assertSame(0, $folderInfo->getPermissions() & ~Constants::PERMISSION_READ); + + $fileInfo = $user2View->getFileInfo($this->folder . $this->filename); + $this->assertNotFalse($fileInfo); + $this->assertSame(0, $fileInfo->getPermissions() & ~Constants::PERMISSION_READ); + + // and what it does advertise still holds + $this->assertSame('file in subfolder', $user2View->file_get_contents($this->folder . $this->filename)); + $this->assertFalse($user2View->fopen($this->folder . '/blocked.txt', 'w')); + + self::loginHelper(self::TEST_FILES_SHARING_API_USER1); + $sourceCache->update($sourceInfo->getId(), ['permissions' => Constants::PERMISSION_ALL]); + $this->shareManager->deleteShare($share); + } + public function testInitWithNonExistingUser(): void { $share = $this->createMock(IShare::class); $share->method('getShareOwner')->willReturn('unexist'); From b163e1f0265091c3d56cef82d1494752ea030015 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 10 Sep 2026 12:24:22 +0200 Subject: [PATCH 3/3] fix(SharedStorageTest): Fix test Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Marcel Klehr --- apps/files_sharing/tests/SharedStorageTest.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index fcc1da200c622..39ee3c1069c73 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -466,14 +466,20 @@ public function testInvalidSourcePermissionsAreReportedAsReadOnly(): void { self::loginHelper(self::TEST_FILES_SHARING_API_USER2); $user2View = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); - // the storage refuses every write, so the cache must not advertise more than read + // the storage refuses every write, so the cache must not advertise more. + // The mount point keeps its delete permission: `View::getFileInfo()` adds that + // for every movable mount, so the recipient can always remove the share from + // their own view. $folderInfo = $user2View->getFileInfo($this->folder); $this->assertNotFalse($folderInfo); - $this->assertSame(0, $folderInfo->getPermissions() & ~Constants::PERMISSION_READ); + $this->assertTrue($folderInfo->isReadable()); + $this->assertFalse($folderInfo->isCreatable()); + $this->assertFalse($folderInfo->isUpdateable()); + $this->assertFalse($folderInfo->isShareable()); $fileInfo = $user2View->getFileInfo($this->folder . $this->filename); $this->assertNotFalse($fileInfo); - $this->assertSame(0, $fileInfo->getPermissions() & ~Constants::PERMISSION_READ); + $this->assertSame(Constants::PERMISSION_READ, $fileInfo->getPermissions()); // and what it does advertise still holds $this->assertSame('file in subfolder', $user2View->file_get_contents($this->folder . $this->filename));