From ad30316cd11c426b99e6847a76bc9b75009ae516 Mon Sep 17 00:00:00 2001 From: Robert Niederreiter Date: Mon, 10 Aug 2026 15:47:03 +0200 Subject: [PATCH 1/2] fix(files_external): propagate child copy failures in AmazonS3::copy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When copying a directory, copy() discarded the return values of its recursive calls and always returned true. rename() relies on that value, so a failed copy still led to rmdir() on the source: every file in the directory was deleted, the destination stayed empty, and the UI reported success. This is reachable whenever a provider rejects CopyObject for objects it otherwise serves — Hetzner Object Storage answers 501 NotImplemented for SSE-C encrypted objects, which makes every single child copy fail. Signed-off-by: Robert Niederreiter --- .../lib/Lib/Storage/AmazonS3.php | 7 +- .../tests/Storage/Amazons3CopyTest.php | 135 ++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 apps/files_external/tests/Storage/Amazons3CopyTest.php diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 9986270a931cc..360a04c856739 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -583,7 +583,12 @@ public function copy(string $source, string $target, ?bool $isFile = null): bool foreach ($this->getDirectoryContent($source) as $item) { $childSource = $source . '/' . $item['name']; $childTarget = $target . '/' . $item['name']; - $this->copy($childSource, $childTarget, $item['mimetype'] !== FileInfo::MIMETYPE_FOLDER); + // Propagate copy failure + if ($this->copy($childSource, $childTarget, $item['mimetype'] !== FileInfo::MIMETYPE_FOLDER) === false) { + // Cache is stale because the target was already partially written + $this->invalidateCache($target); + return false; + } } } diff --git a/apps/files_external/tests/Storage/Amazons3CopyTest.php b/apps/files_external/tests/Storage/Amazons3CopyTest.php new file mode 100644 index 0000000000000..6f6e6296458a9 --- /dev/null +++ b/apps/files_external/tests/Storage/Amazons3CopyTest.php @@ -0,0 +1,135 @@ +getMockBuilder(AmazonS3::class) + ->disableOriginalConstructor() + ->onlyMethods($methods) + ->getMock(); + + // The constructor is disabled, so the properties copy() relies on are uninitialised + $this->invokePrivate($storage, 'initCaches'); + $this->invokePrivate($storage, 'storageClass', ['STANDARD']); + // invokePrivate() cannot reach these two: $logger is private, so it is invisible on + // the mock subclass, and 'test' resolves to the test() method before the property + (new \ReflectionProperty(AmazonS3::class, 'logger'))->setValue($storage, new NullLogger()); + (new \ReflectionProperty(AmazonS3::class, 'test'))->setValue($storage, false); + + return $storage; + } + + private function failingCopyException(): S3Exception { + return new S3Exception('NotImplemented', new Command('CopyObject')); + } + + /** + * A directory copy must report failure when copying one of its children fails. + */ + public function testCopyDirectoryReportsFailureWhenChildCopyFails(): void { + $storage = $this->getStorageMock(['is_file', 'remove', 'mkdir', 'getDirectoryContent', 'copyObject']); + + $storage->method('is_file')->willReturn(false); + $storage->method('remove')->willReturn(true); + $storage->method('mkdir')->willReturn(true); + $storage->method('getDirectoryContent')->willReturn(new \ArrayIterator([ + ['name' => 'child.txt', 'mimetype' => 'text/plain'], + ])); + $storage->method('copyObject')->willThrowException($this->failingCopyException()); + + $this->assertFalse( + $storage->copy('source', 'target'), + 'copy() must report failure when copying a child object fails' + ); + } + + /** + * Renaming a directory must not remove the source when the copy failed. + */ + public function testRenameDirectoryKeepsSourceWhenCopyFails(): void { + $storage = $this->getStorageMock(['is_file', 'remove', 'mkdir', 'getDirectoryContent', 'copyObject', 'rmdir', 'unlink']); + + $storage->method('is_file')->willReturn(false); + $storage->method('remove')->willReturn(true); + $storage->method('mkdir')->willReturn(true); + $storage->method('getDirectoryContent')->willReturn(new \ArrayIterator([ + ['name' => 'child.txt', 'mimetype' => 'text/plain'], + ])); + $storage->method('copyObject')->willThrowException($this->failingCopyException()); + + $storage->expects($this->never())->method('rmdir'); + $storage->expects($this->never())->method('unlink'); + + $this->assertFalse( + $storage->rename('source', 'target'), + 'rename() must report failure when the underlying copy failed' + ); + } + + /** + * A failing grandchild must propagate through nested directories as well. + */ + public function testCopyDirectoryReportsFailureFromNestedDirectory(): void { + $storage = $this->getStorageMock(['is_file', 'remove', 'mkdir', 'getDirectoryContent', 'copyObject']); + + $storage->method('is_file')->willReturn(false); + $storage->method('remove')->willReturn(true); + $storage->method('mkdir')->willReturn(true); + $storage->method('getDirectoryContent')->willReturnCallback( + function (string $directory): \Traversable { + if ($directory === 'source') { + return new \ArrayIterator([ + ['name' => 'nested', 'mimetype' => \OC\Files\FileInfo::MIMETYPE_FOLDER], + ]); + } + return new \ArrayIterator([ + ['name' => 'child.txt', 'mimetype' => 'text/plain'], + ]); + } + ); + $storage->method('copyObject')->willThrowException($this->failingCopyException()); + + $this->assertFalse( + $storage->copy('source', 'target'), + 'copy() must propagate a failure from a nested directory' + ); + } + + /** + * The success path must keep working: a directory whose children all copy fine + * still reports true. + */ + public function testCopyDirectoryReportsSuccessWhenAllChildrenSucceed(): void { + $storage = $this->getStorageMock(['is_file', 'remove', 'mkdir', 'getDirectoryContent', 'copyObject']); + + $storage->method('is_file')->willReturn(false); + $storage->method('remove')->willReturn(true); + $storage->method('mkdir')->willReturn(true); + $storage->method('getDirectoryContent')->willReturn(new \ArrayIterator([ + ['name' => 'child.txt', 'mimetype' => 'text/plain'], + ])); + $storage->method('copyObject')->willReturn(null); + + $this->assertTrue( + $storage->copy('source', 'target'), + 'copy() must still report success when every child copies fine' + ); + } +} From bce2e225357c233571efb406986095de24992ddf Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 1 Oct 2026 23:27:29 -0400 Subject: [PATCH 2/2] test(files_external): initialize caches in S3 copy tests For the backport to the older internal API Signed-off-by: Josh --- apps/files_external/tests/Storage/Amazons3CopyTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_external/tests/Storage/Amazons3CopyTest.php b/apps/files_external/tests/Storage/Amazons3CopyTest.php index 6f6e6296458a9..8cfa7541aaca9 100644 --- a/apps/files_external/tests/Storage/Amazons3CopyTest.php +++ b/apps/files_external/tests/Storage/Amazons3CopyTest.php @@ -26,7 +26,7 @@ private function getStorageMock(array $methods) { ->getMock(); // The constructor is disabled, so the properties copy() relies on are uninitialised - $this->invokePrivate($storage, 'initCaches'); + $this->invokePrivate($storage, 'clearCache'); $this->invokePrivate($storage, 'storageClass', ['STANDARD']); // invokePrivate() cannot reach these two: $logger is private, so it is invisible on // the mock subclass, and 'test' resolves to the test() method before the property