From 0dbdeaa3cff41dd59dacc5637df7e1383f1134cc Mon Sep 17 00:00:00 2001 From: silver Date: Wed, 2 Sep 2026 14:50:15 +0200 Subject: [PATCH 1/5] 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 | 1 + tests/lib/Repair/RepairInvalidSharesTest.php | 47 ++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index b936d2f6a2100..d7bc61d39dc30 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -72,6 +72,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 @@ -90,7 +93,7 @@ protected function stripUserFilesPath($path) { $sliced = array_slice($split, 2); $relPath = implode('/', $sliced); - return '/' . $relPath; + return Filesystem::normalizePath('/' . $relPath); } /** diff --git a/apps/files_sharing/tests/SharedMountTest.php b/apps/files_sharing/tests/SharedMountTest.php index 851dd7168e4a5..07c9d109aaad9 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 627ecf6659d06..3984b3a2bd313 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -91,5 +91,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 3ffb1726e19a1..da4d021ad459e 100644 --- a/tests/lib/Repair/RepairInvalidSharesTest.php +++ b/tests/lib/Repair/RepairInvalidSharesTest.php @@ -125,6 +125,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 95de0d0eb58a4b330db7e1427f779a11cd4ed9d7 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 3 Sep 2026 21:52:36 +0200 Subject: [PATCH 2/5] test(sharing): add regression test for moving shares Signed-off-by: Ferdinand Thiessen From 648fce7e49a0b4916c428ae8b8e6eca07820ad39 Mon Sep 17 00:00:00 2001 From: silver Date: Mon, 7 Sep 2026 10:52:10 +0200 Subject: [PATCH 3/5] 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 | 1 + tests/lib/Repair/RepairInvalidSharesTest.php | 50 ++++++++++++++++---- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php index 3984b3a2bd313..984a35b5c6f4b 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -21,6 +21,7 @@ class RepairInvalidShares implements IRepairStep { public function __construct( protected IConfig $config, protected IDBConnection $connection, + protected IAppConfig $appConfig, ) { } diff --git a/tests/lib/Repair/RepairInvalidSharesTest.php b/tests/lib/Repair/RepairInvalidSharesTest.php index da4d021ad459e..e3de78af1f0cf 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; protected function setUp(): void { parent::setUp(); @@ -43,7 +46,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); } protected function tearDown(): void { @@ -139,8 +144,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([ @@ -156,12 +160,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') @@ -169,7 +170,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 2b8f24273f4df7c5764714fc54d6e43ef1b95fab Mon Sep 17 00:00:00 2001 From: silver Date: Tue, 8 Sep 2026 16:06:12 +0200 Subject: [PATCH 4/5] fix(configlexikon): add missing config key fix(configlexikon): add missing config key Signed-off-by: silver [skip ci] --- core/AppInfo/ConfigLexicon.php | 1 + 1 file changed, 1 insertion(+) diff --git a/core/AppInfo/ConfigLexicon.php b/core/AppInfo/ConfigLexicon.php index 1813961dd4e9f..c4a58a0c29852 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'; From f42fa9f193d9a17ed8e6140c519c8897de668f78 Mon Sep 17 00:00:00 2001 From: silver Date: Wed, 9 Sep 2026 14:56:12 +0200 Subject: [PATCH 5/5] fix(backport): add missing code from stable34 Signed-off-by: silver Assisted-by:ClaudeCode:claude-opus-5 --- .../sharing_features/sharing-v1-part4.feature | 22 +++++++++ core/AppInfo/ConfigLexicon.php | 7 +++ lib/private/Repair.php | 2 +- lib/private/Repair/RepairInvalidShares.php | 47 +++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index 4409ebb1116be..989423b8b236c 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -537,3 +537,25 @@ Scenario: User added/removed to group share with marking When User "user0" moves file "/textfile0 (2).txt" to "/target.txt" Then Share mounts for "user0" match | /user0/files/target.txt/ | + + # 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 | diff --git a/core/AppInfo/ConfigLexicon.php b/core/AppInfo/ConfigLexicon.php index c4a58a0c29852..ce8dd2b9c117b 100644 --- a/core/AppInfo/ConfigLexicon.php +++ b/core/AppInfo/ConfigLexicon.php @@ -100,6 +100,13 @@ public function getAppConfigs(): array { defaultRaw: true, definition: 'Whether on demand preview migration is enabled.' ), + 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, + ), ]; } diff --git a/lib/private/Repair.php b/lib/private/Repair.php index 700e769dbb7f3..67480e370ef87 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -159,7 +159,7 @@ public static function getRepairSteps(): array { return [ new Collation(\OC::$server->getConfig(), \OC::$server->get(LoggerInterface::class), \OC::$server->getDatabaseConnection(), false), new CleanTags(\OC::$server->getDatabaseConnection(), \OC::$server->getUserManager()), - new RepairInvalidShares(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()), + Server::get(RepairInvalidShares::class), new MoveUpdaterStepFile(\OC::$server->getConfig()), new MoveAvatars( \OC::$server->getJobList(), diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php index 984a35b5c6f4b..5f49e6f564452 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -7,6 +7,8 @@ */ namespace OC\Repair; +use OC\Core\AppInfo\ConfigLexicon; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -85,6 +87,51 @@ 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 { + if ($this->appConfig->getValueBool('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, lazy: true)) { + return; + } + + $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(); + } + } + + $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'); + } + } + public function run(IOutput $output) { $ocVersionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0'); if (version_compare($ocVersionFromBeforeUpdate, '12.0.0.11', '<')) {