Skip to content

Commit 6303db0

Browse files
susnuxbackportbot[bot]
susnux
authored and
backportbot[bot]
committed
fix(encryption): keep version and size in sync for files not in the cache
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 <opensource@fthiessen.de> [skip ci]
1 parent a7621f5 commit 6303db0

6 files changed

Lines changed: 214 additions & 5 deletions

File tree

‎.github/workflows/integration-sqlite.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ jobs:
5757
- 'collaboration_features'
5858
- 'comments_features'
5959
- '--tags ~@requires-s3 dav_features'
60+
- 'encryption_features'
6061
- 'features'
6162
- 'federation_features'
6263
- '--tags ~@large files_features'

‎apps/encryption/lib/Crypto/Encryption.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,14 @@ public function begin($path, $user, $mode, array $header, array $accessList) {
154154
if (Scanner::isPartialFile($path)) {
155155
$this->version = $this->version + 1;
156156
}
157+
158+
// A file that is not in the file cache has no stored version, but its
159+
// blocks were signed with version 1 - the version the first write of a
160+
// file uses. This happens while a file written in this request has not
161+
// been scanned yet.
162+
if ($this->version === 0) {
163+
$this->version = 1;
164+
}
157165
}
158166

159167
if ($this->isWriteOperation) {

‎apps/encryption/tests/ChunkedWriteTest.php‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
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 OCA\encryption\tests;
10+
11+
use OC\Files\Storage\Temporary;
12+
use OC\Files\View;
13+
use OCA\Encryption\KeyManager;
14+
use OCP\Server;
15+
use Test\TestCase;
16+
use Test\Traits\EncryptionTrait;
17+
use Test\Traits\MountProviderTrait;
18+
use Test\Traits\UserTrait;
19+
20+
/**
21+
* Writing through a stream does not update the file cache - that is left to the
22+
* caller. Until it does, neither the encrypted version nor the unencrypted size
23+
* of the written file can be read from the cache, but both are part of the block
24+
* signatures and have to match on the next read.
25+
*/
26+
#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
27+
class ChunkedWriteTest extends TestCase {
28+
use MountProviderTrait;
29+
use EncryptionTrait;
30+
use UserTrait;
31+
32+
private function setUpView(): View {
33+
Server::get(KeyManager::class)->validateMasterKey();
34+
Server::get(KeyManager::class)->validateShareKey();
35+
$this->createUser('test1', 'test2');
36+
$this->setupForUser('test1', 'test2');
37+
$this->registerMount('test1', new Temporary(), '/test1/files/other');
38+
$this->loginWithEncryption('test1');
39+
40+
return new View('/test1/files');
41+
}
42+
43+
/**
44+
* The unencrypted block size is 6072 bytes, so the chunks cover writes inside
45+
* a single block, across a block boundary and on a block boundary.
46+
*
47+
* @return array<string, array{int[]}>
48+
*/
49+
public static function chunkSizesProvider(): array {
50+
return [
51+
'several chunks in one block' => [[100, 100, 100]],
52+
'chunks crossing a block' => [[4000, 4000]],
53+
'chunks of varying size' => [[1000, 2000, 3000, 4000, 5000]],
54+
'a single full block' => [[6072]],
55+
'a full block in two chunks' => [[3000, 3072]],
56+
'two full blocks' => [[6072, 6072]],
57+
'a full block and one byte' => [[6072, 1]],
58+
'chunks larger than a block' => [[8192, 8192, 8192]],
59+
];
60+
}
61+
62+
/**
63+
* @param int[] $chunks
64+
*/
65+
#[\PHPUnit\Framework\Attributes\DataProvider('chunkSizesProvider')]
66+
public function testReadBackFileWrittenInChunks(array $chunks): void {
67+
$view = $this->setUpView();
68+
$source = self::getUniqueID('source') . '.bin';
69+
70+
$expected = $this->writeInChunks($view, $source, $chunks);
71+
72+
$this->assertEquals(strlen($expected), $view->filesize($source));
73+
$this->assertEquals($expected, $view->file_get_contents($source));
74+
}
75+
76+
/**
77+
* @param int[] $chunks
78+
*/
79+
#[\PHPUnit\Framework\Attributes\DataProvider('chunkSizesProvider')]
80+
public function testCopyFileWrittenInChunks(array $chunks): void {
81+
$view = $this->setUpView();
82+
$source = self::getUniqueID('source') . '.bin';
83+
$target = self::getUniqueID('target') . '.bin';
84+
85+
$expected = $this->writeInChunks($view, $source, $chunks);
86+
87+
$this->assertTrue($view->copy($source, $target));
88+
$this->assertEquals($expected, $view->file_get_contents($target));
89+
}
90+
91+
/**
92+
* A part file is never in the file cache. With `part_file_in_storage`
93+
* disabled it is written to the user home while the target can live on
94+
* another storage, in which case moving it over has to read it back.
95+
*/
96+
public function testMovePartFileToAnotherStorage(): void {
97+
$view = $this->setUpView();
98+
99+
$partFile = self::getUniqueID() . '.ocTransferId1.part';
100+
$target = 'other/' . self::getUniqueID('target') . '.bin';
101+
102+
$expected = $this->writeInChunks($view, $partFile, [8192, 8192, 8192]);
103+
104+
[$partStorage, $internalPartPath] = $view->resolvePath($partFile);
105+
[$targetStorage, $internalTargetPath] = $view->resolvePath($target);
106+
$this->assertTrue($targetStorage->moveFromStorage($partStorage, $internalPartPath, $internalTargetPath));
107+
108+
$this->assertEquals($expected, $view->file_get_contents($target));
109+
}
110+
111+
/**
112+
* @param int[] $chunks
113+
* @return string the written content
114+
*/
115+
private function writeInChunks(View $view, string $path, array $chunks): string {
116+
$content = '';
117+
$handle = $view->fopen($path, 'w');
118+
$this->assertIsResource($handle);
119+
foreach ($chunks as $index => $length) {
120+
$chunk = str_repeat((string)($index % 10), $length);
121+
$content .= $chunk;
122+
$this->assertEquals($length, fwrite($handle, $chunk));
123+
}
124+
fclose($handle);
125+
126+
return $content;
127+
}
128+
}

‎build/integration/config/behat.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,19 @@ default:
8282
ocPath: ../../
8383
- PrincipalPropertySearchContext:
8484
baseUrl: http://localhost:8080
85+
encryption:
86+
paths:
87+
- "%paths.base%/../encryption_features"
88+
contexts:
89+
- FeatureContext:
90+
baseUrl: http://localhost:8080/ocs/
91+
admin:
92+
- admin
93+
- admin
94+
regular_user_password: 123456
95+
- CommandLineContext:
96+
baseUrl: http://localhost:8080
97+
ocPath: ../../
8598
federation:
8699
paths:
87100
- "%paths.base%/../federation_features"

‎build/integration/encryption_features/encryption.feature‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
2+
# SPDX-License-Identifier: AGPL-3.0-or-later
3+
Feature: encryption
4+
Background:
5+
Given using api version "1"
6+
And using new dav path
7+
And invoking occ with "app:enable encryption"
8+
And the command was successful
9+
And invoking occ with "encryption:enable-master-key" with input "y"
10+
And the command was successful
11+
And invoking occ with "encryption:enable"
12+
And the command was successful
13+
14+
Scenario: Upload and download a file spanning several encrypted blocks
15+
Given user "user0" exists
16+
And As an "user0"
17+
When User "user0" adds a file of 20000 bytes to "/big.bin"
18+
Then the HTTP status code should be "201"
19+
And File "/big.bin" should have prop "d:getcontentlength" equal to "20000"
20+
When Downloading file "/big.bin"
21+
Then the HTTP status code should be "200"
22+
23+
Scenario: Copy a file spanning several encrypted blocks
24+
Given user "user0" exists
25+
And As an "user0"
26+
And User "user0" adds a file of 20000 bytes to "/big.bin"
27+
When User "user0" copies file "/big.bin" to "/copy.bin"
28+
Then the HTTP status code should be "201"
29+
When Downloading file "/copy.bin"
30+
Then the HTTP status code should be "200"
31+
32+
# With "part_file_in_storage" disabled the part file is written to the user
33+
# home while the target lives on another storage, so the upload has to read the
34+
# part file back to move it over. A part file never has a file cache entry, so
35+
# both the encrypted version and the unencrypted size of the written blocks
36+
# have to be known without one.
37+
@local_storage
38+
Scenario: Upload to an external storage while the part file is kept in the user home
39+
Given invoking occ with "config:system:set part_file_in_storage --value false --type boolean"
40+
And the command was successful
41+
And user "user0" exists
42+
And As an "user0"
43+
When User "user0" uploads file "data/textfile.txt" to "/local_storage/textfile.txt"
44+
Then the HTTP status code should be "201"
45+
When Downloading file "/local_storage/textfile.txt"
46+
Then the HTTP status code should be "200"
47+
And Downloaded content should start with "This is a testfile."
48+
When User "user0" adds a file of 20000 bytes to "/local_storage/big.bin"
49+
Then the HTTP status code should be "201"
50+
And File "/local_storage/big.bin" should have prop "d:getcontentlength" equal to "20000"
51+
When Downloading file "/local_storage/big.bin"
52+
Then the HTTP status code should be "200"

‎lib/private/Files/Storage/Wrapper/Encryption.php‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
use OC\Encryption\Exceptions\ModuleDoesNotExistsException;
1111
use OC\Encryption\Util;
12-
use OC\Files\Cache\CacheEntry;
1312
use OC\Files\Filesystem;
1413
use OC\Files\Mount\Manager;
1514
use OC\Files\ObjectStore\ObjectStoreStorage;
@@ -66,10 +65,10 @@ public function filesize(string $path): int|float|false {
6665
$fullPath = $this->getFullPath($path);
6766

6867
$info = $this->getCache()->get($path);
69-
if ($info === false) {
70-
/* Pass call to wrapped storage, it may be a special file like a part file */
71-
return $this->getWrapperStorage()->filesize($path);
72-
}
68+
69+
// The size we tracked while writing the file is authoritative, even for
70+
// files that have no cache entry (yet), e.g. *.part files or files that
71+
// are only scanned once the caller is done writing them.
7372
if (isset($this->unencryptedSize[$fullPath])) {
7473
$size = $this->unencryptedSize[$fullPath];
7574

@@ -99,6 +98,11 @@ public function filesize(string $path): int|float|false {
9998
return $size;
10099
}
101100

101+
if ($info === false) {
102+
/* Pass call to wrapped storage, it may be a special file like a part file */
103+
return $this->getWrapperStorage()->filesize($path);
104+
}
105+
102106
if (isset($info['fileid']) && $info['encrypted']) {
103107
return $this->verifyUnencryptedSize($path, $info->getUnencryptedSize());
104108
}
@@ -613,6 +617,9 @@ private function updateEncryptedVersion(
613617
if ($sourceCacheEntry === false && $targetCacheEntry !== false) {
614618
$encryptedVersion = $targetCacheEntry['encryptedVersion'];
615619
$isRename = false;
620+
} elseif ($sourceCacheEntry === false) {
621+
// a file that is not in the file cache, e.g. a part file, is at version 1
622+
$encryptedVersion = 1;
616623
} else {
617624
$encryptedVersion = $sourceCacheEntry['encryptedVersion'];
618625
}

0 commit comments

Comments
 (0)