Skip to content

fix(storage/local): stop deleting prior to forbidden items check #57421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshtrichards wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(storage/local): stop deleting prior to forbidden items check #57421

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2aebf02
fix(storage/local): check for forbidden items before deleting target …
joshtrichards Jan 8, 2026
42b48ca
fix(storage/local): avoid unsafe fallback on case-only rename
joshtrichards Jan 8, 2026
ee8cbe3
chore: fixup for lint
joshtrichards Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions lib/private/Files/Storage/Local.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,46 +325,68 @@ private function checkTreeForForbiddenItems(string $path): void {
}

public function rename(string $source, string $target): bool {
$srcParent = dirname($source);
$dstParent = dirname($target);
if (!$this->file_exists($source)) {
$this->logRenameError('file does not exist', $source);
return false;
}

$srcParent = dirname($source);
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);
Copy link
Collaborator

@Altahrim Altahrim Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also rename this variable $targetParent

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)); // may throw
}

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)) {
Copy link
Collaborator

@Altahrim Altahrim Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's current behaviour, I am not sure we need to remove the target when it's a file.
It should be overwritten by the rename. Not sure if it's dependant from filesystem…

$this->logRenameError('pre-existing target is not removable', $target);
return false;
}
}

if ($this->is_dir($source)) {
$this->checkTreeForForbiddenItems($this->getSourcePath($source));
}
$renameSuccess = @rename($this->getSourcePath($source), $this->getSourcePath($target));
$isCaseOnly = $this->caseInsensitive && mb_strtolower($target) === mb_strtolower($source);

if (@rename($this->getSourcePath($source), $this->getSourcePath($target))) {
if ($this->caseInsensitive) {
if (mb_strtolower($target) === mb_strtolower($source) && !$this->file_exists($target)) {
return false;
}
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) {
Comment on lines +379 to +380
Copy link
Collaborator

@Altahrim Altahrim Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
if ($copySuccess && !$unlinkSuccess) {
} elseif (!$unlinkSuccess) {

Or maybe with early returns?

$this->logRenameError('fallback copy() succeeded but unlink() failed', $source);
}
return $copySuccess && $unlinkSuccess;
}

private function logRenameError(string $reason, string $path, string $level = 'error'): void {
Server::get(LoggerInterface::class)->$level(
"unable to rename, {$reason}: {$path}", ['app' => 'core']
);
}

public function copy(string $source, string $target): bool {
Expand Down
Loading