From 2aebf02a08fc55c5ac432e1e9cd515bc5ce5033a Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 7 Jan 2026 20:30:04 -0500 Subject: [PATCH 1/3] fix(storage/local): check for forbidden items before deleting target in rename Signed-off-by: Josh --- lib/private/Files/Storage/Local.php | 35 ++++++++++++++++------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 260f9218a88e2..9eace736c8a27 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -325,36 +325,35 @@ private function checkTreeForForbiddenItems(string $path): void { } public function rename(string $source, string $target): bool { + if (!$this->file_exists($source)) { + $this->logRenameError('file does not exist', $source); + return false; + } + $srcParent = dirname($source); - $dstParent = dirname($target); - if (!$this->isUpdatable($srcParent)) { - Server::get(LoggerInterface::class)->error('unable to rename, source directory is not writable : ' . $srcParent, ['app' => 'core']); + $this->logRenameError('source directory is not writable', $srcParent); return false; } + $dstParent = dirname($target); if (!$this->isUpdatable($dstParent)) { - Server::get(LoggerInterface::class)->error('unable to rename, destination directory is not writable : ' . $dstParent, ['app' => 'core']); + $this->logRenameError('destination directory is not writable', $dstParent); return false; } - if (!$this->file_exists($source)) { - Server::get(LoggerInterface::class)->error('unable to rename, file does not exists : ' . $source, ['app' => 'core']); - return false; + if ($this->is_dir($source)) { + $this->checkTreeForForbiddenItems($this->getSourcePath($source)); + // throws } if ($this->file_exists($target)) { - if ($this->is_dir($target)) { - $this->rmdir($target); - } elseif ($this->is_file($target)) { - $this->unlink($target); + if (!$this->remove($target)) { + $this->logRenameError('pre-existing target is not removable', $target); + return false; } } - if ($this->is_dir($source)) { - $this->checkTreeForForbiddenItems($this->getSourcePath($source)); - } - if (@rename($this->getSourcePath($source), $this->getSourcePath($target))) { if ($this->caseInsensitive) { if (mb_strtolower($target) === mb_strtolower($source) && !$this->file_exists($target)) { @@ -367,6 +366,12 @@ public function rename(string $source, string $target): bool { return $this->copy($source, $target) && $this->unlink($source); } + private function logRenameError(string $reason, string $path): void { + Server::get(LoggerInterface::class)->error( + "unable to rename, {$reason}: {$path}", ['app' => 'core'] + ); + } + public function copy(string $source, string $target): bool { if ($this->is_dir($source)) { return parent::copy($source, $target); From 42b48cafac7d43104e4f689e2f9f2c85843f4ed6 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 7 Jan 2026 22:33:55 -0500 Subject: [PATCH 2/3] fix(storage/local): avoid unsafe fallback on case-only rename On case-insensitive filesystems, the copy+unlink fallback shouldn't be attempted since source/destination can "overlap". Also adds some additional logging for error conditions + one at debug level for a more common scenario. Otherwise just some additional tidying for readability. Signed-off-by: Josh --- lib/private/Files/Storage/Local.php | 39 +++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 9eace736c8a27..46a1d9ccfcac5 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -329,7 +329,7 @@ public function rename(string $source, string $target): bool { $this->logRenameError('file does not exist', $source); return false; } - + $srcParent = dirname($source); if (!$this->isUpdatable($srcParent)) { $this->logRenameError('source directory is not writable', $srcParent); @@ -343,8 +343,7 @@ public function rename(string $source, string $target): bool { } if ($this->is_dir($source)) { - $this->checkTreeForForbiddenItems($this->getSourcePath($source)); - // throws + $this->checkTreeForForbiddenItems($this->getSourcePath($source)); // may throw } if ($this->file_exists($target)) { @@ -354,20 +353,38 @@ public function rename(string $source, string $target): bool { } } - if (@rename($this->getSourcePath($source), $this->getSourcePath($target))) { - if ($this->caseInsensitive) { - if (mb_strtolower($target) === mb_strtolower($source) && !$this->file_exists($target)) { - return false; - } + $renameSuccess = @rename($this->getSourcePath($source), $this->getSourcePath($target)); + $isCaseOnly = $this->caseInsensitive && mb_strtolower($target) === mb_strtolower($source); + + if ($renameSuccess) { + if ($isCaseOnly && !$this->file_exists($target)) { + $this->logRenameError('case-only rename succeeded but target does not exist', $target); + return false; } return true; } - return $this->copy($source, $target) && $this->unlink($source); + if ($isCaseOnly) { + $this->logRenameError('OS-level rename syscall failed (fallback unavailable for case-only)', "$source -> $target"); + // Avoid unsafe fallback on case-insensitive filesystems to avoid data loss + return false; + } + + $this->logRenameError('OS-level rename syscall failed (attempting fallback copy+unlink)', "$source -> $target", 'debug'); // debug since not necessarily an error + $copySuccess = $this->copy($source, $target); + $unlinkSuccess = $copySuccess ? $this->unlink($source) : false; + + if (!$copySuccess) { + $this->logRenameError('fallback copy() failed', "$source -> $target"); + } + if ($copySuccess && !$unlinkSuccess) { + $this->logRenameError('fallback copy() succeeded but unlink() failed', $source); + } + return $copySuccess && $unlinkSuccess; } - private function logRenameError(string $reason, string $path): void { - Server::get(LoggerInterface::class)->error( + private function logRenameError(string $reason, string $path, string $level = "error"): void { + Server::get(LoggerInterface::class)->$level( "unable to rename, {$reason}: {$path}", ['app' => 'core'] ); } From ee8cbe30586b73a3fcae8525698f3450cdfac64d Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 8 Jan 2026 10:55:22 -0500 Subject: [PATCH 3/3] chore: fixup for lint Signed-off-by: Josh --- lib/private/Files/Storage/Local.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index 46a1d9ccfcac5..c4218ef16fbe0 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -383,7 +383,7 @@ public function rename(string $source, string $target): bool { return $copySuccess && $unlinkSuccess; } - private function logRenameError(string $reason, string $path, string $level = "error"): void { + private function logRenameError(string $reason, string $path, string $level = 'error'): void { Server::get(LoggerInterface::class)->$level( "unable to rename, {$reason}: {$path}", ['app' => 'core'] );