fix(trashbin): don't overwrite existing file when restoring to a target (#35974)

[DeepDiver1975]

Summary

Fixes #35974 — restoring a file from the trashbin to an explicit target location could silently overwrite an existing file there, causing data loss.

Trashbin::restore($filename, $targetLocation = null) only resolves a collision-safe unique name on the default path ($targetLocation === null, restore to original location). When an explicit $targetLocation is supplied — the WebDAV "restore to a different place" path (apps/dav/lib/TrashBin/…) — it skipped the unique-name step and called View::rename($source, $target) directly, overwriting whatever already existed at the target.

Change

Add a data-loss guard just before the rename: if an explicit target already exists, refuse the restore and return false instead of clobbering it.

if ($view->file_exists($target)) {
    return false;
}

• The bool return contract is preserved and propagates cleanly through TrashBinManager::restore() → AbstractTrashBinNode::restore().
• The default null-target path already resolves to a unique, non-existing name via getUniqueFilename(), so normal restores are unaffected (the guard is never hit there).
• Failing (rather than auto-renaming) is the conservative choice for the explicit-target case: the caller asked for that exact path, so a clear failure is safer and more predictable than silently restoring to a different name.

Tests

Adds TrashbinTest::testRestoreToExplicitTargetDoesNotOverwriteExisting(): delete a file into the trashbin, create a different existing file at the explicit target, attempt the restore, and assert (a) it returns false and (b) the existing target file's content is unchanged.

Tagged @group DB (existing class). php -l clean; full PHPUnit not run in the preparation environment.

Note: owncloud/core is in maintenance mode; targets installations on classic ownCloud 10.x.

---

🤖 This PR was prepared by the Claude Code review agent from the analysis of #35974. Please review carefully before merging.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

🤖 Automated code review by Claude Code review agent

Overview

This PR adds a data-loss guard in Trashbin::restore(): when an explicit $targetLocation is supplied and a file already exists at the resolved target, it returns false instead of calling View::rename() and clobbering the file. A regression unit test is added. The change is small, well-commented, and the intent (preventing data loss) is excellent.

I traced the change against the actual code paths and the original issue #35974. Unfortunately I found a blocking correctness problem: the guard does not fix the bug as it actually occurs over WebDAV.

Correctness

Placement (good): The guard sits correctly after the source-existence check and before $view->rename($source, $target) (Trashbin.php ~L584–591). Returning false is consistent with the method's documented bool contract.

Null-target path (good, unaffected): When $targetLocation === null, the target is resolved via getUniqueFilename() to a guaranteed non-existing name, so file_exists($target) is always false and the guard never blocks a normal restore. Confirmed.

BLOCKING — the guard never fires on the real WebDAV "restore to a different place" path. The explicit-target caller is the WebDAV MOVE flow: Directory::moveInto() → AbstractTrashBinNode::restore() → TrashBinManager::restore() → Trashbin::restore($path, $targetLocation). But before Trashbin::restore() is ever reached, Sabre's CorePlugin::httpMove() has already handled the destination:

// lib/composer/sabre/dav/lib/DAV/CorePlugin.php  (httpMove)
if ($moveInfo['destinationExists']) {
    $this->server->tree->delete($moveInfo['destination']);   // <-- existing target deleted FIRST
    $this->server->emit('afterUnbind', [$moveInfo['destination']]);
}
$this->server->tree->move($path, $moveInfo['destination']);   // <-- only now does restore() run

Overwrite defaults to T when the header is absent (Server::getCopyAndMoveInfo()), so destinationExists is true and Sabre pre-deletes the existing target before the move. By the time our guard runs, $view->file_exists($target) is already false, so the guard is a no-op and the restore proceeds — exactly the data loss we meant to prevent.

This matches the original issue #35974 verbatim, which reports: "the code-path deletes the existing file (triggering an event that causes it to be saved into the trashbin)." That is Sabre's tree->delete($destination). The reporter also notes the behavior is intermittent — the guard addresses none of that.

With Overwrite: F, getCopyAndMoveInfo() throws PreconditionFailed (412) before restore runs, so the guard is moot there too.

Net: the only path that reaches the guard with a pre-existing target is a direct PHP call to Trashbin::restore($filename, $explicitTarget) that bypasses Sabre's pre-delete — which is precisely (and only) what the new unit test does. The fix passes its own test but does not fix the reported WebDAV bug.

If a fix at this layer is desired, it likely belongs in the DAV layer — e.g. Directory::moveInto() should reject the restore when the destination already exists before Sabre's pre-delete (or httpMove/the trashbin restore endpoint must opt out of the destination pre-delete). A guard inside Trashbin::restore() is too late in the chain.

Tests

The test testRestoreToExplicitTargetDoesNotOverwriteExisting() is internally well-formed and would pass in CI: it follows the established pattern of the neighbouring testRestoreFileDoesNotOverwriteExistingInRoot(), uses the correct trash filename format file1.txt.d<mtime> via $trashedFile->getMtime(), uses real helpers (Helper::getTrashFiles, user folder nodes), and the assertions (assertFalse(restore(...)) + unchanged existing.txt content) are valid for the direct-call path.

However the test gives false confidence: it exercises a direct Trashbin::restore() call, not the WebDAV MOVE flow where the bug lives. It therefore green-lights a change that does not fix #35974 in production. A meaningful regression test would drive the actual DAV trashbin restore endpoint (as the issue's Gherkin scenario does) and assert the destination is untouched and a 4xx/no-overwrite results.

Edge cases

• Direct Trashbin::restore() callers with an explicit target ARE now protected (positive, though no such caller exists in core today besides tests/3rd-party).
• The ajax/undelete.php path only ever passes a null target, so it is unaffected (correct).
• Folder restores via WebDAV hit the same Sabre pre-delete behavior, so they are equally unprotected.
• No new path is broken by the change; the risk is purely that it under-delivers, not that it regresses.

Verdict

changes-requested (blocking). The fix is safe and does not regress anything, but it does not actually fix issue #35974: on the real WebDAV restore-to-target path, Sabre deletes the pre-existing destination before Trashbin::restore() runs, so the new guard never fires. The accompanying test passes only because it bypasses that flow via a direct PHP call, giving false confidence. Please move the protection to the DAV layer (reject/short-circuit the restore when the destination exists, before Sabre's httpMove pre-delete) and add a test that drives the WebDAV MOVE/restore endpoint end-to-end.