From fd9475dab4b0e1145e6570bacc31823020d23a06 Mon Sep 17 00:00:00 2001 From: silver Date: Wed, 2 Sep 2026 14:50:15 +0200 Subject: [PATCH 1/4] fix(files_sharing): normalize share target on parent folder rename When a recipient moved an incoming share into one of their own folders and later renamed that folder, Updater::renameChildren passed the mount point to SharedMount::moveMount. Mount points always end in a slash, and stripUserFilesPath did not normalize its result, so the slash was stored in share.file_target. PROPFIND on such a share then returns 500. Normalize the stripped path so no caller can write a trailing slash, and repair rows that are already affected. Signed-off-by: silver Assisted-by: ClaudeCode:claude-opus-5 --- apps/files_sharing/lib/SharedMount.php | 5 ++- apps/files_sharing/tests/SharedMountTest.php | 3 ++ lib/private/Repair/RepairInvalidShares.php | 40 +++++++++++++++++ tests/lib/Repair/RepairInvalidSharesTest.php | 47 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index fddd81d5a7001..e72e09711fca8 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -73,6 +73,9 @@ protected function updateFileTarget($newPath, &$share) { /** * Format a path to be relative to the /user/files/ directory * + * The result is normalized, so it never carries a trailing slash. Callers may + * pass mount points, which always end in a slash. + * * @param string $path the absolute path * @return string e.g. turns '/admin/files/test.txt' into '/test.txt' * @throws BrokenPath @@ -91,7 +94,7 @@ protected function stripUserFilesPath(string $path): string { $sliced = array_slice($split, 2); $relPath = implode('/', $sliced); - return '/' . $relPath; + return Filesystem::normalizePath('/' . $relPath); } #[Override] diff --git a/apps/files_sharing/tests/SharedMountTest.php b/apps/files_sharing/tests/SharedMountTest.php index 22f42b050fa93..1e86969fe084c 100644 --- a/apps/files_sharing/tests/SharedMountTest.php +++ b/apps/files_sharing/tests/SharedMountTest.php @@ -166,6 +166,9 @@ public static function dataProviderTestStripUserFilesPath() { return [ ['/user/files/foo.txt', '/foo.txt', false], ['/user/files/folder/foo.txt', '/folder/foo.txt', false], + ['/user/files/foo.txt/', '/foo.txt', false], + ['/user/files/folder/foo.txt/', '/folder/foo.txt', false], + ['/user/files/folder//foo.txt', '/folder/foo.txt', false], ['/data/user/files/foo.txt', null, true], ['/data/user/files/', null, true], ['/files/foo.txt', null, true], diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php index f3a4cbef724e3..3fd97c11f5c8c 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -87,6 +87,45 @@ private function removeSharesNonExistingParent(IOutput $output): void { } } + /** + * Strip trailing slashes that leaked into the share target when a parent folder + * of a moved incoming share was renamed + */ + private function removeTrailingSlashFromFileTarget(IOutput $output): void { + $updatedEntries = 0; + + $query = $this->connection->getQueryBuilder(); + $query->select('id', 'file_target') + ->from('share') + ->where($query->expr()->like('file_target', $query->createNamedParameter('%/'))) + ->andWhere($query->expr()->neq('file_target', $query->createNamedParameter('/'))) + ->setMaxResults(self::CHUNK_SIZE); + + $updateQuery = $this->connection->getQueryBuilder(); + $updateQuery->update('share') + ->set('file_target', $updateQuery->createParameter('file_target')) + ->where($updateQuery->expr()->eq('id', $updateQuery->createParameter('id'))); + + $rowsInLastChunk = self::CHUNK_SIZE; + while ($rowsInLastChunk === self::CHUNK_SIZE) { + $result = $query->executeQuery(); + $rows = $result->fetchAllAssociative(); + $result->closeCursor(); + $rowsInLastChunk = count($rows); + + foreach ($rows as $row) { + $updatedEntries += $updateQuery + ->setParameter('file_target', rtrim($row['file_target'], '/')) + ->setParameter('id', (int)$row['id']) + ->executeStatement(); + } + } + + if ($updatedEntries > 0) { + $output->info('Removed trailing slashes from the target of ' . $updatedEntries . ' shares'); + } + } + #[\Override] public function run(IOutput $output) { $ocVersionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0'); @@ -95,5 +134,6 @@ public function run(IOutput $output) { } $this->removeSharesNonExistingParent($output); + $this->removeTrailingSlashFromFileTarget($output); } } diff --git a/tests/lib/Repair/RepairInvalidSharesTest.php b/tests/lib/Repair/RepairInvalidSharesTest.php index 615247ab01140..369559af78c98 100644 --- a/tests/lib/Repair/RepairInvalidSharesTest.php +++ b/tests/lib/Repair/RepairInvalidSharesTest.php @@ -127,6 +127,53 @@ public function testSharesNonExistingParent(): void { $result->closeCursor(); } + public static function trailingSlashProvider(): array { + return [ + // trailing slash left behind by renaming a parent folder of a moved share + ['/rename_folder/First_share_the_file.odt/', '/rename_folder/First_share_the_file.odt'], + ['/shared_folder/', '/shared_folder'], + // unchanged + ['/rename_folder/First_share_the_file.odt', '/rename_folder/First_share_the_file.odt'], + ['/', '/'], + ]; + } + + /** + * Test stripping trailing slashes from the share target + */ + #[\PHPUnit\Framework\Attributes\DataProvider('trailingSlashProvider')] + public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string $expectedFileTarget): void { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_USER), + 'share_with' => $qb->expr()->literal('recipientuser1'), + 'uid_owner' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('folder'), + 'item_source' => $qb->expr()->literal(123), + 'item_target' => $qb->expr()->literal('/123'), + 'file_source' => $qb->expr()->literal(123), + 'file_target' => $qb->expr()->literal($fileTarget), + 'permissions' => $qb->expr()->literal(31), + 'stime' => $qb->expr()->literal(time()), + ]) + ->executeStatement(); + + /** @var IOutput|\PHPUnit\Framework\MockObject\MockObject $outputMock */ + $outputMock = $this->createMock(IOutput::class); + + $this->repair->run($outputMock); + + $results = $this->connection->getQueryBuilder() + ->select('file_target') + ->from('share') + ->executeQuery() + ->fetchAllAssociative(); + + $this->assertCount(1, $results); + $this->assertSame($expectedFileTarget, $results[0]['file_target']); + } + public static function fileSharePermissionsProvider(): array { return [ // unchanged for folder From 215fc834f8a6f4117afafb5dde1271d9fb64c1af Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 3 Sep 2026 21:52:36 +0200 Subject: [PATCH 2/4] test(sharing): add regression test for moving shares Signed-off-by: Ferdinand Thiessen --- .../sharing_features/sharing-v1-part4.feature | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index 08ab1a1daed3c..7ba1234a6500e 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -555,3 +555,25 @@ Scenario: User added/removed to group share with marking | permissions | 1 | Then the OCS status code should be "100" And the HTTP status code should be "200" + + # Renaming the parent folder moves the share mount through Updater::renameChildren, + # which passes a mount point instead of a path. Mount points carry a trailing slash, + # which used to end up in the share target. + Scenario: Renaming a folder that contains a received share keeps the share target normalized + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "content" to "/moved-share.txt" + And file "/moved-share.txt" of user "user0" is shared with user "user1" with permissions 19 + And user "user1" accepts last share + And user "user1" created a folder "/received" + And User "user1" moves file "/moved-share.txt" to "/received/moved-share.txt" + Then the HTTP status code should be "201" + When User "user1" moves file "/received" to "/archive" + Then the HTTP status code should be "201" + And As an "user1" + And Getting info of last share + And the OCS status code should be "100" + And Share fields of last share match with + | file_target | /archive/moved-share.txt | + And user "user1" should see following elements + | /archive/moved-share.txt | From 85444faa6179dc51bdce4d00742c45f5ae49dffa Mon Sep 17 00:00:00 2001 From: silver Date: Mon, 7 Sep 2026 10:52:10 +0200 Subject: [PATCH 3/4] perf(core): only run the trailing slash share repair once perf(core): only run the trailing slash share repair once Signed-off-by: silver Assisted-by: ClaudeCode:claude-opus-5 [skip ci] --- lib/private/Repair/RepairInvalidShares.php | 9 ++++ tests/lib/Repair/RepairInvalidSharesTest.php | 50 ++++++++++++++++---- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php index 3fd97c11f5c8c..d2d1b04fe773d 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -8,7 +8,9 @@ namespace OC\Repair; +use OC\Core\AppInfo\ConfigLexicon; use OCP\Constants; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -23,6 +25,7 @@ class RepairInvalidShares implements IRepairStep { public function __construct( protected IConfig $config, protected IDBConnection $connection, + protected IAppConfig $appConfig, ) { } @@ -92,6 +95,10 @@ private function removeSharesNonExistingParent(IOutput $output): void { * of a moved incoming share was renamed */ private function removeTrailingSlashFromFileTarget(IOutput $output): void { + if ($this->appConfig->getValueBool('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, lazy: true)) { + return; + } + $updatedEntries = 0; $query = $this->connection->getQueryBuilder(); @@ -121,6 +128,8 @@ private function removeTrailingSlashFromFileTarget(IOutput $output): void { } } + $this->appConfig->setValueBool('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, true, lazy: true); + if ($updatedEntries > 0) { $output->info('Removed trailing slashes from the target of ' . $updatedEntries . ' shares'); } diff --git a/tests/lib/Repair/RepairInvalidSharesTest.php b/tests/lib/Repair/RepairInvalidSharesTest.php index 369559af78c98..9b665c5f66e55 100644 --- a/tests/lib/Repair/RepairInvalidSharesTest.php +++ b/tests/lib/Repair/RepairInvalidSharesTest.php @@ -8,8 +8,10 @@ namespace Test\Repair; +use OC\Core\AppInfo\ConfigLexicon; use OC\Repair\RepairInvalidShares; use OCP\Constants; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -28,6 +30,7 @@ class RepairInvalidSharesTest extends TestCase { private RepairInvalidShares $repair; private IDBConnection $connection; + private IAppConfig&\PHPUnit\Framework\MockObject\MockObject $appConfig; #[\Override] protected function setUp(): void { @@ -44,7 +47,9 @@ protected function setUp(): void { $this->connection = Server::get(IDBConnection::class); $this->deleteAllShares(); - $this->repair = new RepairInvalidShares($config, $this->connection); + $this->appConfig = $this->createMock(IAppConfig::class); + + $this->repair = new RepairInvalidShares($config, $this->connection, $this->appConfig); } #[\Override] @@ -141,8 +146,7 @@ public static function trailingSlashProvider(): array { /** * Test stripping trailing slashes from the share target */ - #[\PHPUnit\Framework\Attributes\DataProvider('trailingSlashProvider')] - public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string $expectedFileTarget): void { + private function addShareWithTarget(string $fileTarget): void { $qb = $this->connection->getQueryBuilder(); $qb->insert('share') ->values([ @@ -158,12 +162,9 @@ public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string 'stime' => $qb->expr()->literal(time()), ]) ->executeStatement(); + } - /** @var IOutput|\PHPUnit\Framework\MockObject\MockObject $outputMock */ - $outputMock = $this->createMock(IOutput::class); - - $this->repair->run($outputMock); - + private function getSingleFileTarget(): string { $results = $this->connection->getQueryBuilder() ->select('file_target') ->from('share') @@ -171,7 +172,38 @@ public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string ->fetchAllAssociative(); $this->assertCount(1, $results); - $this->assertSame($expectedFileTarget, $results[0]['file_target']); + + return $results[0]['file_target']; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('trailingSlashProvider')] + public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string $expectedFileTarget): void { + $this->addShareWithTarget($fileTarget); + + $this->appConfig->method('getValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, false, true) + ->willReturn(false); + $this->appConfig->expects($this->once()) + ->method('setValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, true, true); + + $this->repair->run($this->createMock(IOutput::class)); + + $this->assertSame($expectedFileTarget, $this->getSingleFileTarget()); + } + + public function testRemoveTrailingSlashFromFileTargetSkippedWhenAlreadyRun(): void { + $this->addShareWithTarget('/rename_folder/First_share_the_file.odt/'); + + $this->appConfig->method('getValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, false, true) + ->willReturn(true); + $this->appConfig->expects($this->never()) + ->method('setValueBool'); + + $this->repair->run($this->createMock(IOutput::class)); + + $this->assertSame('/rename_folder/First_share_the_file.odt/', $this->getSingleFileTarget()); } public static function fileSharePermissionsProvider(): array { From 592768b74beb3d983a7f74d57049fc2ef84535f5 Mon Sep 17 00:00:00 2001 From: silver Date: Tue, 8 Sep 2026 16:06:12 +0200 Subject: [PATCH 4/4] fix(configlexikon): add missing config key Signed-off-by: silver --- core/AppInfo/ConfigLexicon.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/AppInfo/ConfigLexicon.php b/core/AppInfo/ConfigLexicon.php index f99c44e43a61f..cb49a42893ede 100644 --- a/core/AppInfo/ConfigLexicon.php +++ b/core/AppInfo/ConfigLexicon.php @@ -28,6 +28,7 @@ class ConfigLexicon implements ILexicon { public const SHARE_LINK_EXPIRE_DATE_ENFORCED = 'shareapi_enforce_expire_date'; public const USER_LANGUAGE = 'lang'; public const OCM_DISCOVERY_ENABLED = 'ocm_discovery_enabled'; + public const SHARE_REPAIR_REMOVED_TRAILING_SLASHES = 'share_repair_removed_trailing_slashes'; public const USER_LOCALE = 'locale'; public const USER_TIMEZONE = 'timezone'; @@ -113,6 +114,13 @@ public function getAppConfigs(): array { definition: 'Show the app store link in the app menu to accounts without admin rights', note: 'When this key is not set, the link is also hidden while a valid subscription is available or while "appstoreenabled" is disabled. Setting this key explicitly takes precedence over both.', ), + new Entry( + key: self::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, + type: ValueType::BOOL, + defaultRaw: false, + definition: 'Whether the repair step stripping trailing slashes from share targets has already been run.', + lazy: true, + ), ]; }