Skip to content

Commit a1a317d

Browse files
solracsfbackportbot[bot]
authored andcommitted
fix(files): do not let a failing move hook break the move or hide files
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent 50bf4af commit a1a317d

4 files changed

Lines changed: 113 additions & 1 deletion

File tree

lib/private/Files/Cache/Updater.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,16 @@ public function remove(string $path): void {
121121
#[Override]
122122
public function renameFromStorage(IStorage $sourceStorage, string $source, string $target): void {
123123
$this->copyOrRenameFromStorage($sourceStorage, $source, $target, function (ICache $sourceCache) use ($sourceStorage, $source, $target): void {
124+
$parent = dirname($target);
125+
if ($parent === '.') {
126+
$parent = '';
127+
}
128+
if (!$this->cache->inCache($parent)) {
129+
// scan the parent first, moving the entry below a parent that is not in
130+
// the cache would hide it from folder listings until the next scan
131+
$this->scanner->scan($parent, Scanner::SCAN_SHALLOW, -1, false);
132+
}
133+
124134
// Remove existing cache entry to no reuse the fileId.
125135
if ($this->cache->inCache($target)) {
126136
$this->cache->remove($target);
@@ -179,6 +189,12 @@ private function copyOrRenameFromStorage(IStorage $sourceStorage, string $source
179189

180190
$isDir = $sourceInfo->getMimeType() === FileInfo::MIMETYPE_FOLDER;
181191
} else {
192+
if (!$this->storage->instanceOfStorage(ObjectStoreStorage::class) && !$this->cache->inCache($target)) {
193+
// the source was not in the cache, so the operation could not transfer
194+
// an entry to the target. Scan the target to not leave it invisible
195+
// until the next scan
196+
$this->scanner->scan($target, Scanner::SCAN_SHALLOW, -1, false);
197+
}
182198
$isDir = $this->storage->is_dir($target);
183199
}
184200

lib/private/legacy/OC_Hook.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ public static function emit($signalClass, $signalName, $params = []) {
8585
foreach (self::$registered[$signalClass][$signalName] as $i) {
8686
try {
8787
call_user_func([ $i['class'], $i['name'] ], $params);
88-
} catch (Exception $e) {
88+
} catch (Throwable $e) {
89+
// a failing hook handler must not break the operation that emitted
90+
// the signal, this includes Errors like TypeError
8991
self::$thrownExceptions[] = $e;
9092
Server::get(LoggerInterface::class)->error($e->getMessage(), ['exception' => $e]);
9193
if ($e instanceof HintException) {

tests/lib/Files/Cache/UpdaterTest.php

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,48 @@ public function testMoveCrossStorage(): void {
256256
$this->assertEquals($cached['fileid'], $cachedTarget['fileid']);
257257
}
258258

259+
public function testMoveCrossStorageMissingTargetParent(): void {
260+
$storage2 = new Temporary([]);
261+
$cache2 = $storage2->getCache();
262+
Filesystem::mount($storage2, [], '/bar');
263+
$this->storage->file_put_contents('foo.txt', 'qwerty');
264+
$this->updater->update('foo.txt');
265+
$cached = $this->cache->get('foo.txt');
266+
267+
// the target parent exists on disk but is missing from the cache
268+
$storage2->mkdir('sub');
269+
$this->assertFalse($cache2->inCache('sub'));
270+
271+
$storage2->moveFromStorage($this->storage, 'foo.txt', 'sub/bar.txt');
272+
$storage2->getUpdater()->renameFromStorage($this->storage, 'foo.txt', 'sub/bar.txt');
273+
274+
$this->assertFalse($this->cache->inCache('foo.txt'));
275+
$this->assertTrue($cache2->inCache('sub'));
276+
$this->assertTrue($cache2->inCache('sub/bar.txt'));
277+
278+
// the moved entry is attached to the scanned parent instead of being orphaned
279+
$cachedTarget = $cache2->get('sub/bar.txt');
280+
$this->assertEquals($cache2->getId('sub'), $cachedTarget['parent']);
281+
$this->assertEquals($cached['fileid'], $cachedTarget['fileid']);
282+
}
283+
284+
public function testMoveCrossStorageSourceNotInCache(): void {
285+
$storage2 = new Temporary([]);
286+
$cache2 = $storage2->getCache();
287+
Filesystem::mount($storage2, [], '/bar');
288+
$this->storage->file_put_contents('foo.txt', 'qwerty');
289+
// the source was never scanned into the cache
290+
$this->assertFalse($this->cache->inCache('foo.txt'));
291+
292+
$storage2->moveFromStorage($this->storage, 'foo.txt', 'bar.txt');
293+
$storage2->getUpdater()->renameFromStorage($this->storage, 'foo.txt', 'bar.txt');
294+
295+
// the target still gets a cache entry
296+
$this->assertTrue($cache2->inCache('bar.txt'));
297+
$cachedTarget = $cache2->get('bar.txt');
298+
$this->assertEquals(6, $cachedTarget['size']);
299+
}
300+
259301
public function testMoveFolderCrossStorage(): void {
260302
$storage2 = new Temporary([]);
261303
$cache2 = $storage2->getCache();

tests/lib/LegacyHookTest.php

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace Test;
10+
11+
use OCP\HintException;
12+
13+
class LegacyHookTest extends TestCase {
14+
#[\Override]
15+
protected function setUp(): void {
16+
parent::setUp();
17+
\OC_Hook::clear('LegacyHookTest');
18+
}
19+
20+
#[\Override]
21+
protected function tearDown(): void {
22+
\OC_Hook::clear('LegacyHookTest');
23+
// the exceptions thrown by the handlers below are expected, do not let
24+
// the base test case rethrow them
25+
\OC_Hook::$thrownExceptions = [];
26+
parent::tearDown();
27+
}
28+
29+
public static function throwTypeError(): void {
30+
throw new \TypeError('type error thrown by a hook handler');
31+
}
32+
33+
public static function throwHintException(): void {
34+
throw new HintException('hint exception thrown by a hook handler');
35+
}
36+
37+
public function testEmitDoesNotPropagateThrowable(): void {
38+
\OC_Hook::connect('LegacyHookTest', 'error', self::class, 'throwTypeError');
39+
40+
$this->assertTrue(\OC_Hook::emit('LegacyHookTest', 'error'));
41+
42+
$this->assertCount(1, \OC_Hook::$thrownExceptions);
43+
$this->assertInstanceOf(\TypeError::class, \OC_Hook::$thrownExceptions[0]);
44+
}
45+
46+
public function testEmitRethrowsHintException(): void {
47+
\OC_Hook::connect('LegacyHookTest', 'hint', self::class, 'throwHintException');
48+
49+
$this->expectException(HintException::class);
50+
\OC_Hook::emit('LegacyHookTest', 'hint');
51+
}
52+
}

0 commit comments

Comments
 (0)