[stable35] fix(files_sharing): normalize share target on parent folder rename

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]

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],

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 |

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,
+			),
 		];
 	}
 

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,
 	) {
 	}
 
@@ -87,6 +90,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');
+		}
+	}
+
 	#[\Override]
 	public function run(IOutput $output) {
 		$ocVersionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0');
@@ -95,5 +143,6 @@ public function run(IOutput $output) {
 		}
 
 		$this->removeSharesNonExistingParent($output);
+		$this->removeTrailingSlashFromFileTarget($output);
 	}
 }

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]
@@ -127,6 +132,80 @@ 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
+	 */
+	private function addShareWithTarget(string $fileTarget): 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();
+	}
+
+	private function getSingleFileTarget(): string {
+		$results = $this->connection->getQueryBuilder()
+			->select('file_target')
+			->from('share')
+			->executeQuery()
+			->fetchAllAssociative();
+
+		$this->assertCount(1, $results);
+
+		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 {
 		return [
 			// unchanged for folder