From 1af7fe8b0b5a491ddf167034d95bf06acef97371 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 15 Sep 2026 18:12:54 +0200 Subject: [PATCH 1/3] fix(encryption): keep version and size in sync for files not in the cache A file written through a stream has no file cache entry until the caller scans it, but both inputs of the block signature are read from that entry: stream_close() can only bump `encryptedVersion` if the entry exists, while the reader got version 0 instead of the 1 the blocks were signed with, and filesize() returned the wrapped storage's ciphertext size, which moved the 'end' position marker to the wrong block. Reading such a file back - e.g. moving a part file to a target on another storage - failed with "Bad Signature". Treat a missing version as 1 on read, and let the size tracked while writing win over the wrapped storage's size even without a cache entry. Also stop reading `encryptedVersion` off a missing source entry when updating the encrypted version of a copy or rename. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- .github/workflows/integration-sqlite.yml | 1 + apps/encryption/lib/Crypto/Encryption.php | 8 ++ apps/encryption/tests/ChunkedWriteTest.php | 128 ++++++++++++++++++ build/integration/config/behat.yml | 13 ++ .../encryption_features/encryption.feature | 52 +++++++ .../Files/Storage/Wrapper/Encryption.php | 34 ++--- 6 files changed, 217 insertions(+), 19 deletions(-) create mode 100644 apps/encryption/tests/ChunkedWriteTest.php create mode 100644 build/integration/encryption_features/encryption.feature diff --git a/.github/workflows/integration-sqlite.yml b/.github/workflows/integration-sqlite.yml index 75cde735234ba..b8acc65506dda 100644 --- a/.github/workflows/integration-sqlite.yml +++ b/.github/workflows/integration-sqlite.yml @@ -52,6 +52,7 @@ jobs: - 'collaboration_features' - 'comments_features' - '--tags ~@requires-s3 dav_features' + - 'encryption_features' - 'features' - 'federation_features' - '--tags ~@large files_features' diff --git a/apps/encryption/lib/Crypto/Encryption.php b/apps/encryption/lib/Crypto/Encryption.php index 1d6950a5ab776..fc1d55ab7b480 100644 --- a/apps/encryption/lib/Crypto/Encryption.php +++ b/apps/encryption/lib/Crypto/Encryption.php @@ -157,6 +157,14 @@ public function begin($path, $user, $mode, array $header, array $accessList) { if (Scanner::isPartialFile($path)) { $this->version = $this->version + 1; } + + // A file that is not in the file cache has no stored version, but its + // blocks were signed with version 1 - the version the first write of a + // file uses. This happens while a file written in this request has not + // been scanned yet. + if ($this->version === 0) { + $this->version = 1; + } } if ($this->isWriteOperation) { diff --git a/apps/encryption/tests/ChunkedWriteTest.php b/apps/encryption/tests/ChunkedWriteTest.php new file mode 100644 index 0000000000000..79a918e6e1808 --- /dev/null +++ b/apps/encryption/tests/ChunkedWriteTest.php @@ -0,0 +1,128 @@ +validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + $this->registerMount('test1', new Temporary(), '/test1/files/other'); + $this->loginWithEncryption('test1'); + + return new View('/test1/files'); + } + + /** + * The unencrypted block size is 6072 bytes, so the chunks cover writes inside + * a single block, across a block boundary and on a block boundary. + * + * @return array + */ + public static function chunkSizesProvider(): array { + return [ + 'several chunks in one block' => [[100, 100, 100]], + 'chunks crossing a block' => [[4000, 4000]], + 'chunks of varying size' => [[1000, 2000, 3000, 4000, 5000]], + 'a single full block' => [[6072]], + 'a full block in two chunks' => [[3000, 3072]], + 'two full blocks' => [[6072, 6072]], + 'a full block and one byte' => [[6072, 1]], + 'chunks larger than a block' => [[8192, 8192, 8192]], + ]; + } + + /** + * @param int[] $chunks + */ + #[\PHPUnit\Framework\Attributes\DataProvider('chunkSizesProvider')] + public function testReadBackFileWrittenInChunks(array $chunks): void { + $view = $this->setUpView(); + $source = self::getUniqueID('source') . '.bin'; + + $expected = $this->writeInChunks($view, $source, $chunks); + + $this->assertEquals(strlen($expected), $view->filesize($source)); + $this->assertEquals($expected, $view->file_get_contents($source)); + } + + /** + * @param int[] $chunks + */ + #[\PHPUnit\Framework\Attributes\DataProvider('chunkSizesProvider')] + public function testCopyFileWrittenInChunks(array $chunks): void { + $view = $this->setUpView(); + $source = self::getUniqueID('source') . '.bin'; + $target = self::getUniqueID('target') . '.bin'; + + $expected = $this->writeInChunks($view, $source, $chunks); + + $this->assertTrue($view->copy($source, $target)); + $this->assertEquals($expected, $view->file_get_contents($target)); + } + + /** + * A part file is never in the file cache. With `part_file_in_storage` + * disabled it is written to the user home while the target can live on + * another storage, in which case moving it over has to read it back. + */ + public function testMovePartFileToAnotherStorage(): void { + $view = $this->setUpView(); + + $partFile = self::getUniqueID() . '.ocTransferId1.part'; + $target = 'other/' . self::getUniqueID('target') . '.bin'; + + $expected = $this->writeInChunks($view, $partFile, [8192, 8192, 8192]); + + [$partStorage, $internalPartPath] = $view->resolvePath($partFile); + [$targetStorage, $internalTargetPath] = $view->resolvePath($target); + $this->assertTrue($targetStorage->moveFromStorage($partStorage, $internalPartPath, $internalTargetPath)); + + $this->assertEquals($expected, $view->file_get_contents($target)); + } + + /** + * @param int[] $chunks + * @return string the written content + */ + private function writeInChunks(View $view, string $path, array $chunks): string { + $content = ''; + $handle = $view->fopen($path, 'w'); + $this->assertIsResource($handle); + foreach ($chunks as $index => $length) { + $chunk = str_repeat((string)($index % 10), $length); + $content .= $chunk; + $this->assertEquals($length, fwrite($handle, $chunk)); + } + fclose($handle); + + return $content; + } +} diff --git a/build/integration/config/behat.yml b/build/integration/config/behat.yml index 4781b32497131..6071badadb089 100644 --- a/build/integration/config/behat.yml +++ b/build/integration/config/behat.yml @@ -82,6 +82,19 @@ default: ocPath: ../../ - PrincipalPropertySearchContext: baseUrl: http://localhost:8080 + encryption: + paths: + - "%paths.base%/../encryption_features" + contexts: + - FeatureContext: + baseUrl: http://localhost:8080/ocs/ + admin: + - admin + - admin + regular_user_password: 123456 + - CommandLineContext: + baseUrl: http://localhost:8080 + ocPath: ../../ federation: paths: - "%paths.base%/../federation_features" diff --git a/build/integration/encryption_features/encryption.feature b/build/integration/encryption_features/encryption.feature new file mode 100644 index 0000000000000..15704609d79df --- /dev/null +++ b/build/integration/encryption_features/encryption.feature @@ -0,0 +1,52 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +Feature: encryption + Background: + Given using api version "1" + And using new dav path + And invoking occ with "app:enable encryption" + And the command was successful + And invoking occ with "encryption:enable-master-key" with input "y" + And the command was successful + And invoking occ with "encryption:enable" + And the command was successful + + Scenario: Upload and download a file spanning several encrypted blocks + Given user "user0" exists + And As an "user0" + When User "user0" adds a file of 20000 bytes to "/big.bin" + Then the HTTP status code should be "201" + And File "/big.bin" should have prop "d:getcontentlength" equal to "20000" + When Downloading file "/big.bin" + Then the HTTP status code should be "200" + + Scenario: Copy a file spanning several encrypted blocks + Given user "user0" exists + And As an "user0" + And User "user0" adds a file of 20000 bytes to "/big.bin" + When User "user0" copies file "/big.bin" to "/copy.bin" + Then the HTTP status code should be "201" + When Downloading file "/copy.bin" + Then the HTTP status code should be "200" + + # With "part_file_in_storage" disabled the part file is written to the user + # home while the target lives on another storage, so the upload has to read the + # part file back to move it over. A part file never has a file cache entry, so + # both the encrypted version and the unencrypted size of the written blocks + # have to be known without one. + @local_storage + Scenario: Upload to an external storage while the part file is kept in the user home + Given invoking occ with "config:system:set part_file_in_storage --value false --type boolean" + And the command was successful + And user "user0" exists + And As an "user0" + When User "user0" uploads file "data/textfile.txt" to "/local_storage/textfile.txt" + Then the HTTP status code should be "201" + When Downloading file "/local_storage/textfile.txt" + Then the HTTP status code should be "200" + And Downloaded content should start with "This is a testfile." + When User "user0" adds a file of 20000 bytes to "/local_storage/big.bin" + Then the HTTP status code should be "201" + And File "/local_storage/big.bin" should have prop "d:getcontentlength" equal to "20000" + When Downloading file "/local_storage/big.bin" + Then the HTTP status code should be "200" diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 4a1fa9135a46d..911489268a8ba 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -10,7 +10,6 @@ use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\Encryption\Util; -use OC\Files\Cache\CacheEntry; use OC\Files\Filesystem; use OC\Files\Mount\Manager; use OC\Files\ObjectStore\ObjectStoreStorage; @@ -23,7 +22,6 @@ use OCP\Encryption\IFile; use OCP\Encryption\IManager; use OCP\Encryption\Keys\IStorage; -use OCP\Files\Cache\ICacheEntry; use OCP\Files\GenericFileException; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage; @@ -66,28 +64,18 @@ public function filesize(string $path): int|float|false { $fullPath = $this->getFullPath($path); $info = $this->getCache()->get($path); - if ($info === false) { - /* Pass call to wrapped storage, it may be a special file like a part file */ - return $this->getWrapperStorage()->filesize($path); - } + + // The size we tracked while writing the file is authoritative, even for + // files that have no cache entry (yet), e.g. *.part files or files that + // are only scanned once the caller is done writing them. if (isset($this->unencryptedSize[$fullPath])) { $size = $this->unencryptedSize[$fullPath]; // Update file cache (only if file is already cached). // Certain files are not cached (e.g. *.part). - if (isset($info['fileid'])) { - if ($info instanceof ICacheEntry) { - $info['encrypted'] = $info['encryptedVersion']; - } else { - /** - * @psalm-suppress RedundantCondition - */ - if (!is_array($info)) { - $info = []; - } - $info['encrypted'] = true; - $info = new CacheEntry($info); - } + if ($info !== false && isset($info['fileid'])) { + $isEncryptedInCache = !empty($info['encrypted']); + $info['encrypted'] = $info['encryptedVersion']; if ($size !== $info->getUnencryptedSize()) { $this->getCache()->update($info->getId(), [ @@ -99,6 +87,11 @@ public function filesize(string $path): int|float|false { return $size; } + if ($info === false) { + /* Pass call to wrapped storage, it may be a special file like a part file */ + return $this->getWrapperStorage()->filesize($path); + } + if (isset($info['fileid']) && $info['encrypted']) { return $this->verifyUnencryptedSize($path, $info->getUnencryptedSize()); } @@ -612,6 +605,9 @@ private function updateEncryptedVersion( if ($sourceCacheEntry === false && $targetCacheEntry !== false) { $encryptedVersion = $targetCacheEntry['encryptedVersion']; $isRename = false; + } elseif ($sourceCacheEntry === false) { + // a file that is not in the file cache, e.g. a part file, is at version 1 + $encryptedVersion = 1; } else { $encryptedVersion = $sourceCacheEntry['encryptedVersion']; } From 58c9b86f7ded2a612375b82b353ec0845f16843c Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 15 Sep 2026 19:00:49 +0200 Subject: [PATCH 2/3] fix(encryption): keep the encrypted version the copy was written with A copy re-encrypts the target, so the signature of its blocks is keyed on the version of the target - the version of the file it overwrites plus one - and not on the version of the source. Two writers overwrite that value after the stream recorded it: updateEncryptedVersion() resets it to 1 for every copy, and Cache::copyFromCache() then puts the source's version on the target. Reading the copy back fails with "Bad Signature" whenever those differ, which is the case for every copy of a file that was written more than once and for every copy onto an existing file. Take the version the stream recorded for the target instead of resetting it, and let it win over the source's version when the cache entry of the copy is written. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- apps/encryption/tests/EncryptedCopyTest.php | 139 ++++++++++++++++++ .../encryption_features/encryption.feature | 22 +++ lib/private/Files/Cache/Cache.php | 8 + .../Files/Storage/Wrapper/Encryption.php | 25 +++- .../Files/Storage/Wrapper/EncryptionTest.php | 69 +++++++++ 5 files changed, 257 insertions(+), 6 deletions(-) create mode 100644 apps/encryption/tests/EncryptedCopyTest.php diff --git a/apps/encryption/tests/EncryptedCopyTest.php b/apps/encryption/tests/EncryptedCopyTest.php new file mode 100644 index 0000000000000..6aa44509af277 --- /dev/null +++ b/apps/encryption/tests/EncryptedCopyTest.php @@ -0,0 +1,139 @@ +validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + $this->registerMount('test1', new Temporary(), '/test1/files/other'); + $this->loginWithEncryption('test1'); + + return new View('/test1/files'); + } + + /** + * The version the target ends up at depends on the storage: a copy unlinks the + * target first, which keeps the cache entry - and with it the version to bump - + * on a local storage but drops it on an object store, where the copy therefore + * starts over at version 1. Reading the target back is what shows that the + * recorded version is the one its blocks were signed with. + */ + public function testCopyOverExistingFile(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + /** + * Every write bumps the version of the source, while the copy of it starts over + * at the version of the target. + */ + public function testCopyFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('c', 20000); + + $view->file_put_contents('source.bin', str_repeat('a', 20000)); + $view->file_put_contents('source.bin', str_repeat('b', 20000)); + $view->file_put_contents('source.bin', $source); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyOverExistingFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + $view->file_put_contents('target.bin', str_repeat('c', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyFolderOverExistingFolder(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->mkdir('source'); + $view->file_put_contents('source/file.bin', $source); + $view->mkdir('target'); + $view->file_put_contents('target/file.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source', 'target')); + + $this->assertEquals($source, $view->file_get_contents('target/file.bin')); + } + + public function testMoveOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->rename('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + } + + /** + * The target is not unlinked when it lives on another storage, so it keeps its + * cache entry and the copy is signed with the version that follows the one of + * the file it overwrites. + */ + public function testCopyOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + $this->assertEquals( + 2, + $view->getFileInfo('other/target.bin')->getEncryptedVersion(), + 'the version of the overwritten file was not bumped' + ); + } +} diff --git a/build/integration/encryption_features/encryption.feature b/build/integration/encryption_features/encryption.feature index 15704609d79df..d64d429e72ce9 100644 --- a/build/integration/encryption_features/encryption.feature +++ b/build/integration/encryption_features/encryption.feature @@ -29,6 +29,28 @@ Feature: encryption When Downloading file "/copy.bin" Then the HTTP status code should be "200" + Scenario: Copy a file over an existing file + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the source content" to "/source.txt" + And User "user0" uploads file with content "the target content" to "/target.txt" + When User "user0" copies file "/source.txt" to "/target.txt" + Then the HTTP status code should be "204" + When Downloading file "/target.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the source content" + + Scenario: Copy a file that was written several times + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the first content" to "/source.txt" + And User "user0" uploads file with content "the second content" to "/source.txt" + When User "user0" copies file "/source.txt" to "/copy.txt" + Then the HTTP status code should be "201" + When Downloading file "/copy.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the second content" + # With "part_file_in_storage" disabled the part file is written to the user # home while the target lives on another storage, so the upload has to read the # part file back to move it over. A part file never has a file cache entry, so diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index eec26ecc96608..3e22e274ae325 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -1281,6 +1281,14 @@ public function copyFromCache(ICache $sourceCache, ICacheEntry $sourceEntry, str // normalizeData() prefers 'encryptedVersion' over 'encrypted' when both are // set, so it has to be cleared too or the mark above gets ignored unset($data['encryptedVersion']); + } elseif (isset($data['encryptedVersion'])) { + // The storage re-encrypts the content it writes to the target, so the target + // is at its own version - the one recorded for it while it was written - and + // not at the version of the source. + $targetEntry = $this->get($targetPath); + if ($targetEntry !== false && !empty($targetEntry['encryptedVersion'])) { + $data['encryptedVersion'] = $targetEntry['encryptedVersion']; + } } $fileId = $this->put($targetPath, $data); diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 911489268a8ba..bb0658fb784d7 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -603,13 +603,26 @@ private function updateEncryptedVersion( // Rename of the cache already happened, so we do the cleanup on the target if ($sourceCacheEntry === false && $targetCacheEntry !== false) { - $encryptedVersion = $targetCacheEntry['encryptedVersion']; $isRename = false; - } elseif ($sourceCacheEntry === false) { - // a file that is not in the file cache, e.g. a part file, is at version 1 - $encryptedVersion = 1; + } + + if ($keepEncryptionVersion) { + // a 1:1 copy reuses the keys and the ciphertext of the source, so the + // target stays at the version of the source + if ($sourceCacheEntry !== false) { + $encryptedVersion = (int)($sourceCacheEntry['encryptedVersion'] ?? 0); + } elseif ($targetCacheEntry !== false) { + $encryptedVersion = (int)($targetCacheEntry['encryptedVersion'] ?? 0); + } else { + // a file that is not in the file cache, e.g. a part file, is at version 1 + $encryptedVersion = 1; + } } else { - $encryptedVersion = $sourceCacheEntry['encryptedVersion']; + // The target was written through the encryption stream, which signs the + // blocks with the version that follows the version of the file they + // replaced and records it on the target's cache entry. A target that has + // no cache entry was written at version 1. + $encryptedVersion = $targetCacheEntry === false ? 1 : (int)($targetCacheEntry['encryptedVersion'] ?? 0); } // In case of a move operation from an unencrypted to an encrypted @@ -617,7 +630,7 @@ private function updateEncryptedVersion( // correct value would be "1". Thus we manually set the value to "1" // for those cases. // See also https://github.com/owncloud/core/issues/23078 - if ($encryptedVersion === 0 || !$keepEncryptionVersion) { + if ($encryptedVersion === 0) { $encryptedVersion = 1; } diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 5f7e157d67cf0..36315ce9fe1c3 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -721,6 +721,75 @@ public static function dataCopyBetweenStorage(): array { ]; } + public static function dataUpdateEncryptedVersion(): array { + return [ + // the target is written through the encryption stream, which signs its blocks + // with the version that follows the version of the file they replace + 'copy onto an existing file' => [['encryptedVersion' => 4], ['encryptedVersion' => 3], false, 3], + 'copy onto a new file' => [['encryptedVersion' => 4], false, false, 1], + 'copy onto a file that is not encrypted yet' => [['encryptedVersion' => 4], ['encryptedVersion' => 0], false, 1], + // a 1:1 copy reuses the keys and the ciphertext of the source + '1:1 copy' => [['encryptedVersion' => 5], false, true, 5], + '1:1 copy of a file that is not encrypted yet' => [['encryptedVersion' => 0], false, true, 1], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataUpdateEncryptedVersion')] + public function testUpdateEncryptedVersion( + array|false $sourceCacheEntry, + array|false $targetCacheEntry, + bool $keepEncryptionVersion, + int $expectedVersion, + ): void { + $sourceCache = $this->createMock(ICache::class); + $sourceCache->method('get') + ->with('source.txt') + ->willReturn($sourceCacheEntry); + $sourceStorage = $this->createMock(\OC\Files\Storage\Storage::class); + $sourceStorage->method('getCache') + ->willReturn($sourceCache); + + $targetCache = $this->createMock(ICache::class); + $targetCache->method('get') + ->with('target.txt') + ->willReturn($targetCacheEntry); + $targetCache->expects($this->once()) + ->method('put') + ->with('target.txt', ['encrypted' => true, 'encryptedVersion' => $expectedVersion]); + + $instance = $this->getMockBuilder(Encryption::class) + ->setConstructorArgs( + [ + [ + 'storage' => $this->sourceStorage, + 'root' => 'foo', + 'mountPoint' => '/', + 'mount' => $this->mount + ], + $this->encryptionManager, + $this->util, + $this->logger, + $this->file, + null, + $this->keyStore, + $this->mountManager, + $this->arrayCache + ] + ) + ->onlyMethods(['getCache', 'getEncryptionModule']) + ->getMock(); + $instance->method('getCache')->willReturn($targetCache); + $instance->method('getEncryptionModule')->willReturn($this->encryptionModule); + + $this->encryptionManager->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); + global $mockedMountPointEncryptionEnabled; + $mockedMountPointEncryptionEnabled = true; + + $this->invokePrivate($instance, 'updateEncryptedVersion', [$sourceStorage, 'source.txt', 'target.txt', false, $keepEncryptionVersion]); + } + public function testCopyBetweenStorageMinimumEncryptedVersion(): void { $storage2 = $this->createMock(\OC\Files\Storage\Storage::class); From af7fbdc8535c83f52ab63764efd5bff56e47b33f Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 15 Sep 2026 19:04:06 +0200 Subject: [PATCH 3/3] fix(files): carry the unencrypted size over when copying a cache entry The size of an encrypted file lives in `unencrypted_size`, and both CacheEntry::getUnencryptedSize() and FileInfo::getSize() prefer it over `size`. Cache::copyFromCache() copied the `encrypted` mark without it, so the copy of an encrypted file was marked encrypted with an unencrypted size of 0 and reported as empty - in the web UI, to clients and for quota - until something rescanned it. Copy the unencrypted size alongside the encrypted version, and reset it when the mark is dropped for a target that is not encrypted, where the size is read from `size`. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- apps/encryption/tests/CopySizeTest.php | 69 +++++++++++++++++++ .../encryption_features/encryption.feature | 1 + lib/private/Files/Cache/Cache.php | 12 +++- tests/lib/Files/Cache/CacheTest.php | 22 +++++- 4 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 apps/encryption/tests/CopySizeTest.php diff --git a/apps/encryption/tests/CopySizeTest.php b/apps/encryption/tests/CopySizeTest.php new file mode 100644 index 0000000000000..b04aecbd6f449 --- /dev/null +++ b/apps/encryption/tests/CopySizeTest.php @@ -0,0 +1,69 @@ +validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + $this->registerMount('test1', new Temporary(), '/test1/files/other'); + $this->loginWithEncryption('test1'); + + return new View('/test1/files'); + } + + public function testCopyKeepsTheSize(): void { + $view = $this->setUpView(); + + $view->file_put_contents('source.bin', str_repeat('a', 20000)); + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals(20000, $view->getFileInfo('target.bin')->getSize()); + } + + public function testCopyToAnotherStorageKeepsTheSize(): void { + $view = $this->setUpView(); + + $view->file_put_contents('source.bin', str_repeat('a', 20000)); + $this->assertTrue($view->copy('source.bin', 'other/target.bin')); + + $this->assertEquals(20000, $view->getFileInfo('other/target.bin')->getSize()); + } + + public function testCopyOfAFolderKeepsTheSizes(): void { + $view = $this->setUpView(); + + $view->mkdir('source'); + $view->file_put_contents('source/file.bin', str_repeat('a', 20000)); + $this->assertTrue($view->copy('source', 'target')); + + $this->assertEquals(20000, $view->getFileInfo('target/file.bin')->getSize()); + } +} diff --git a/build/integration/encryption_features/encryption.feature b/build/integration/encryption_features/encryption.feature index d64d429e72ce9..fed221e1a72ad 100644 --- a/build/integration/encryption_features/encryption.feature +++ b/build/integration/encryption_features/encryption.feature @@ -26,6 +26,7 @@ Feature: encryption And User "user0" adds a file of 20000 bytes to "/big.bin" When User "user0" copies file "/big.bin" to "/copy.bin" Then the HTTP status code should be "201" + And File "/copy.bin" should have prop "d:getcontentlength" equal to "20000" When Downloading file "/copy.bin" Then the HTTP status code should be "200" diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index 3e22e274ae325..08810cef87694 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -1281,6 +1281,8 @@ public function copyFromCache(ICache $sourceCache, ICacheEntry $sourceEntry, str // normalizeData() prefers 'encryptedVersion' over 'encrypted' when both are // set, so it has to be cleared too or the mark above gets ignored unset($data['encryptedVersion']); + // without the `encrypted` mark the size of the copy is read from `size` + $data['unencrypted_size'] = 0; } elseif (isset($data['encryptedVersion'])) { // The storage re-encrypts the content it writes to the target, so the target // is at its own version - the one recorded for it while it was written - and @@ -1323,8 +1325,14 @@ private function cacheEntryToArray(ICacheEntry $entry): array { $data['permissions'] = $entry['scan_permissions']; } - if ($entry->isEncrypted() && isset($entry['encryptedVersion'])) { - $data['encryptedVersion'] = $entry['encryptedVersion']; + if ($entry->isEncrypted()) { + // the size of an encrypted file is stored in its own column, which every + // reader prefers over `size`, so the copy is reported as empty without it + $data['unencrypted_size'] = $entry->getUnencryptedSize(); + + if (isset($entry['encryptedVersion'])) { + $data['encryptedVersion'] = $entry['encryptedVersion']; + } } return $data; diff --git a/tests/lib/Files/Cache/CacheTest.php b/tests/lib/Files/Cache/CacheTest.php index e58f41344f24c..da3978ee105a8 100644 --- a/tests/lib/Files/Cache/CacheTest.php +++ b/tests/lib/Files/Cache/CacheTest.php @@ -726,10 +726,25 @@ public function testCopyFromCachePreservesEncryptedVersion(): void { $this->assertSame(3, $targetEntry['encryptedVersion']); } + public function testCopyFromCachePreservesUnencryptedSize(): void { + $data = [ + 'size' => 128, 'mtime' => 50, 'mimetype' => 'foo/bar', + 'encrypted' => true, 'encryptedVersion' => 3, 'unencrypted_size' => 100, + ]; + $this->cache->put('source', $data); + $sourceEntry = $this->cache->get('source'); + $this->assertEquals(100, $sourceEntry->getUnencryptedSize()); + + $this->cache->copyFromCache($this->cache, $sourceEntry, 'target'); + + $targetEntry = $this->cache->get('target'); + $this->assertEquals(100, $targetEntry->getUnencryptedSize()); + } + public function testCopyFromCacheClearsEncryptedVersionWhenCopyingToNonEncryptedStorage(): void { $data = [ - 'size' => 100, 'mtime' => 50, 'mimetype' => 'foo/bar', - 'encrypted' => true, 'encryptedVersion' => 3, + 'size' => 128, 'mtime' => 50, 'mimetype' => 'foo/bar', + 'encrypted' => true, 'encryptedVersion' => 3, 'unencrypted_size' => 100, ]; $this->cache2->put('source', $data); $sourceEntry = $this->cache2->get('source'); @@ -751,6 +766,9 @@ public function testCopyFromCacheClearsEncryptedVersionWhenCopyingToNonEncrypted $targetEntry = $targetCache->get('target'); $this->assertFalse($targetEntry->isEncrypted()); $this->assertSame(0, $targetEntry['encryptedVersion']); + // the target is not marked as encrypted, so its size is read from `size` + $this->assertEquals(0, $targetEntry['unencrypted_size']); + $this->assertEquals(128, $targetEntry->getSize()); } public function testGetIncomplete(): void {