From 63ebd2b49e4e88b5deff8f12cd1a68403f6fe18a Mon Sep 17 00:00:00 2001 From: Francois Prunayre Date: Thu, 12 Jun 2025 08:56:22 +0200 Subject: [PATCH 1/3] fix(storage): Unlink symlink instead of deleting target content and link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nextcloud allow following symlink (https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#localstorage-allowsymlinks) but removal of symlink remove files in the target of the symlink and fail to remove the link returning an error. Using the following structure: ``` . ├── afolder │   └── test.txt ├── alink -> afolder └── welcome.txt ``` created with: ```bash mkdir afolder touch afolder/test.txt ln -s afolder alink ``` After deletion of `alink` symbolic link, the content of `afolder` is removed, the link also and an error is reported. ```json { "method":"DELETE", "url":"/remote.php/dav/files/admin/alink", "message":"rmdir(/data/dev/nextcloud/data/admin/files/alink): Not a directory at /data/dev/nextcloud/lib/private/Files/Storage/Local.php#128" } ``` Results is: ``` . ├── afolder └── welcome.txt ``` If the resource to be deleted is a link, unlink it (and preserve link target content). ``` . ├── afolder │   └── test.txt └── welcome.txt ``` Signed-off-by: Francois Prunayre --- lib/private/Files/Storage/Local.php | 53 ++++++++++++++++------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index a4f0d1cc9bf91..2d3bf5971ce55 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -100,35 +100,40 @@ public function rmdir(string $path): bool { return false; } try { - $it = new \RecursiveIteratorIterator( - new \RecursiveDirectoryIterator($this->getSourcePath($path)), - \RecursiveIteratorIterator::CHILD_FIRST - ); - /** - * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach - * This bug is fixed in PHP 5.5.9 or before - * See #8376 - */ - $it->rewind(); - while ($it->valid()) { + if (is_link($this->getSourcePath($path))) { + clearstatcache(true, $this->getSourcePath($path)); + return unlink($this->getSourcePath($path)); + } else { + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->getSourcePath($path)), + \RecursiveIteratorIterator::CHILD_FIRST + ); /** - * @var \SplFileInfo $file + * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach + * This bug is fixed in PHP 5.5.9 or before + * See #8376 */ - $file = $it->current(); - clearstatcache(true, $file->getRealPath()); - if (in_array($file->getBasename(), ['.', '..'])) { + $it->rewind(); + while ($it->valid()) { + /** + * @var \SplFileInfo $file + */ + $file = $it->current(); + clearstatcache(true, $file->getRealPath()); + if (in_array($file->getBasename(), ['.', '..'])) { + $it->next(); + continue; + } elseif ($file->isFile() || $file->isLink()) { + unlink($file->getPathname()); + } elseif ($file->isDir()) { + rmdir($file->getPathname()); + } $it->next(); - continue; - } elseif ($file->isFile() || $file->isLink()) { - unlink($file->getPathname()); - } elseif ($file->isDir()) { - rmdir($file->getPathname()); } - $it->next(); + unset($it); // Release iterator and thereby its potential directory lock (e.g. in case of VirtualBox shared folders) + clearstatcache(true, $this->getSourcePath($path)); + return rmdir($this->getSourcePath($path)); } - unset($it); // Release iterator and thereby its potential directory lock (e.g. in case of VirtualBox shared folders) - clearstatcache(true, $this->getSourcePath($path)); - return rmdir($this->getSourcePath($path)); } catch (\UnexpectedValueException $e) { return false; } From d6747c3d937af2c72c73083aad7edcef2ba5eb7f Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 25 Aug 2026 22:25:32 -0400 Subject: [PATCH 2/3] refactor(storage): simplify symlink-safe directory removal Resolve the source path once, clear the stat cache before checking whether it is a symlink, and use an early return to avoid nesting the recursive directory removal logic. Signed-off-by: Josh --- lib/private/Files/Storage/Local.php | 72 ++++++++++++++++------------- 1 file changed, 41 insertions(+), 31 deletions(-) diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 2d3bf5971ce55..8b69d650ba827 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -99,41 +99,51 @@ public function rmdir(string $path): bool { if (!$this->isDeletable($path)) { return false; } + + $sourcePath = $this->getSourcePath($path); + clearstatcache(true, $sourcePath); + + if (is_link($sourcePath)) { + return unlink($sourcePath); + } + try { - if (is_link($this->getSourcePath($path))) { - clearstatcache(true, $this->getSourcePath($path)); - return unlink($this->getSourcePath($path)); - } else { - $it = new \RecursiveIteratorIterator( - new \RecursiveDirectoryIterator($this->getSourcePath($path)), - \RecursiveIteratorIterator::CHILD_FIRST - ); - /** - * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach - * This bug is fixed in PHP 5.5.9 or before - * See #8376 - */ - $it->rewind(); - while ($it->valid()) { - /** - * @var \SplFileInfo $file - */ - $file = $it->current(); - clearstatcache(true, $file->getRealPath()); - if (in_array($file->getBasename(), ['.', '..'])) { - $it->next(); - continue; - } elseif ($file->isFile() || $file->isLink()) { - unlink($file->getPathname()); - } elseif ($file->isDir()) { - rmdir($file->getPathname()); - } + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($sourcePath), + \RecursiveIteratorIterator::CHILD_FIRST + ); + + /** + * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach. + * This bug is fixed in PHP 5.5.9 or before. + * See #8376. + */ + $it->rewind(); + while ($it->valid()) { + /** @var \SplFileInfo $file */ + $file = $it->current(); + clearstatcache(true, $file->getRealPath()); + + if (in_array($file->getBasename(), ['.', '..'], true)) { $it->next(); + continue; + } + + if ($file->isFile() || $file->isLink()) { + unlink($file->getPathname()); + } elseif ($file->isDir()) { + rmdir($file->getPathname()); } - unset($it); // Release iterator and thereby its potential directory lock (e.g. in case of VirtualBox shared folders) - clearstatcache(true, $this->getSourcePath($path)); - return rmdir($this->getSourcePath($path)); + + $it->next(); } + + // Release the iterator and its potential directory lock, + // for example on VirtualBox shared folders. + unset($it); + + clearstatcache(true, $sourcePath); + return rmdir($sourcePath); } catch (\UnexpectedValueException $e) { return false; } From 2200f6ab2933daaba3186e1a456ec5f3d45557dd Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 25 Aug 2026 22:32:27 -0400 Subject: [PATCH 3/3] test(storage): cover removal of directory symlinks Verify that deleting a directory symlink through rmdir or unlink removes only the link while preserving the target directory and its contents. Assisted-by: Copilot:gpt-5.6-sol Signed-off-by: Josh --- tests/lib/Files/Storage/LocalTest.php | 44 +++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/lib/Files/Storage/LocalTest.php b/tests/lib/Files/Storage/LocalTest.php index e676e6718ac31..6aa44e894b2c1 100644 --- a/tests/lib/Files/Storage/LocalTest.php +++ b/tests/lib/Files/Storage/LocalTest.php @@ -102,6 +102,50 @@ public function testDisallowSymlinksInsideDatadir(): void { $this->addToAssertionCount(1); } + public static function directorySymlinkRemovalProvider(): array { + return [ + 'rmdir' => ['rmdir'], + 'unlink' => ['unlink'], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('directorySymlinkRemovalProvider')] + public function testRemovingDirectorySymlinkPreservesTarget(string $operation): void { + $targetPath = $this->tmpDir . 'target'; + $linkPath = $this->tmpDir . 'link'; + + $this->assertTrue($this->instance->mkdir('target')); + $this->assertSame( + strlen('target contents'), + $this->instance->file_put_contents('target/file.txt', 'target contents'), + ); + + if (!@symlink($targetPath, $linkPath)) { + $this->markTestSkipped('Failed to create directory symlink'); + } + + $this->assertTrue(is_link($linkPath)); + $this->assertTrue($this->instance->$operation('link')); + + $this->assertFalse( + is_link($linkPath), + 'The symbolic link should be removed', + ); + $this->assertDirectoryExists( + $targetPath, + 'The target directory should be preserved', + ); + $this->assertFileExists( + $targetPath . '/file.txt', + 'Files inside the target directory should be preserved', + ); + $this->assertSame( + 'target contents', + file_get_contents($targetPath . '/file.txt'), + 'The target file contents should remain unchanged', + ); + } + public function testWriteUmaskFilePutContents(): void { $oldMask = umask(0333); $this->instance->file_put_contents('test.txt', 'sad');