Skip to content

Commit f8f0957

Browse files
solracsfcristianscheid
authored andcommitted
fix(files): do not let a failing move hook break the move or hide files
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> [skip ci]
1 parent 25a12a5 commit f8f0957

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
@@ -167,6 +167,16 @@ public function remove($path) {
167167
*/
168168
public function renameFromStorage(IStorage $sourceStorage, $source, $target) {
169169
$this->copyOrRenameFromStorage($sourceStorage, $source, $target, function (ICache $sourceCache) use ($sourceStorage, $source, $target) {
170+
$parent = dirname($target);
171+
if ($parent === '.') {
172+
$parent = '';
173+
}
174+
if (!$this->cache->inCache($parent)) {
175+
// scan the parent first, moving the entry below a parent that is not in
176+
// the cache would hide it from folder listings until the next scan
177+
$this->scanner->scan($parent, Scanner::SCAN_SHALLOW, -1, false);
178+
}
179+
170180
// Remove existing cache entry to no reuse the fileId.
171181
if ($this->cache->inCache($target)) {
172182
$this->cache->remove($target);
@@ -227,6 +237,12 @@ private function copyOrRenameFromStorage(IStorage $sourceStorage, string $source
227237

228238
$isDir = $sourceInfo->getMimeType() === FileInfo::MIMETYPE_FOLDER;
229239
} else {
240+
if (!$this->storage->instanceOfStorage(ObjectStoreStorage::class) && !$this->cache->inCache($target)) {
241+
// the source was not in the cache, so the operation could not transfer
242+
// an entry to the target. Scan the target to not leave it invisible
243+
// until the next scan
244+
$this->scanner->scan($target, Scanner::SCAN_SHALLOW, -1, false);
245+
}
230246
$isDir = $this->storage->is_dir($target);
231247
}
232248

lib/private/legacy/OC_Hook.php

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

tests/lib/Files/Cache/UpdaterTest.php

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

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