From 7eae917b02a3ad2c9ba9afdcbc64856482b63f7c Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Wed, 29 Apr 2026 11:58:31 -0400 Subject: [PATCH 1/5] fix(encryption): Refactor EncryptionWrapper with HomeMountPoint support Rewrite conditional flow to use early-return guards: skip IDisableEncryptionStorage, skip the root mount, respect encryptHomeStorage for HomeMountPoints. Uses IAppConfig for the encryptHomeStorage setting with a legacy string fallback for the upgrade window. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Stephen Cuppett --- lib/private/Encryption/EncryptionWrapper.php | 86 ++++++++++++++------ 1 file changed, 62 insertions(+), 24 deletions(-) diff --git a/lib/private/Encryption/EncryptionWrapper.php b/lib/private/Encryption/EncryptionWrapper.php index 68d2efd8b8c0b..355257218e806 100644 --- a/lib/private/Encryption/EncryptionWrapper.php +++ b/lib/private/Encryption/EncryptionWrapper.php @@ -8,14 +8,17 @@ namespace OC\Encryption; use OC\Files\Filesystem; +use OC\Files\Mount\HomeMountPoint; use OC\Files\Storage\Wrapper\Encryption; use OC\Files\View; use OC\Memcache\ArrayCache; use OCP\Encryption\IFile; use OCP\Encryption\Keys\IStorage as EncryptionKeysStorage; +use OCP\Exceptions\AppConfigTypeConflictException; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage\IDisableEncryptionStorage; use OCP\Files\Storage\IStorage; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IGroupManager; use OCP\IUserManager; @@ -57,32 +60,67 @@ public function wrapStorage(string $mountPoint, IStorage $storage, IMountPoint $ 'mount' => $mount ]; - if ($force || (!$storage->instanceOfStorage(IDisableEncryptionStorage::class) && $mountPoint !== '/')) { - $user = Server::get(IUserSession::class)->getUser(); - $mountManager = Filesystem::getMountManager(); - $uid = $user ? $user->getUID() : null; - $fileHelper = Server::get(IFile::class); - $keyStorage = Server::get(EncryptionKeysStorage::class); + // Only evaluate other conditions if not forced + if (!$force) { + // If a disabled storage medium, return basic storage + if ($storage->instanceOfStorage(IDisableEncryptionStorage::class)) { + return $storage; + } - $util = new Util( - new View(), - Server::get(IUserManager::class), - Server::get(IGroupManager::class), - Server::get(IConfig::class) - ); - return new Encryption( - $parameters, - $this->manager, - $util, - $this->logger, - $fileHelper, - $uid, - $keyStorage, - $mountManager, - $this->arrayCache + // Root mount point handling: skip encryption wrapper + if ($mountPoint === '/') { + return $storage; + } + + // Skip encryption for home mounts if encryptHomeStorage is disabled + if ($mount instanceof HomeMountPoint && !$this->shouldEncryptHomeStorage()) { + return $storage; + } + } + + // Apply encryption wrapper + $user = Server::get(IUserSession::class)->getUser(); + $mountManager = Filesystem::getMountManager(); + $uid = $user ? $user->getUID() : null; + $fileHelper = Server::get(IFile::class); + $keyStorage = Server::get(EncryptionKeysStorage::class); + + $util = new Util( + new View(), + Server::get(IUserManager::class), + Server::get(IGroupManager::class), + Server::get(IConfig::class) + ); + return new Encryption( + $parameters, + $this->manager, + $util, + $this->logger, + $fileHelper, + $uid, + $keyStorage, + $mountManager, + $this->arrayCache + ); + } + + private function shouldEncryptHomeStorage(): bool { + $appConfig = Server::get(IAppConfig::class); + try { + return $appConfig->getValueBool('encryption', 'encryptHomeStorage', true); + } catch (AppConfigTypeConflictException) { + // Stored as VALUE_STRING from a pre-upgrade installation. + // RetypeEncryptionConfigKeys repair step will fix the type on occ upgrade. + return $this->parseLegacyBoolString( + $appConfig->getValueString('encryption', 'encryptHomeStorage', '1') ); - } else { - return $storage; + } catch (\Throwable) { + // DB not ready (e.g. oc_appconfig does not yet exist during install). + return true; } } + + private function parseLegacyBoolString(string $value): bool { + return in_array(strtolower(trim($value)), ['1', 'true', 'yes', 'on'], true); + } } From 14f16f1e1b8678d41371842975b535a238b2dc21 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Wed, 29 Apr 2026 11:58:52 -0400 Subject: [PATCH 2/5] fix(encryption): Correctly report size for zero-byte encrypted files Files with 0 bytes no longer incorrectly report as 8192 bytes. Widens unencryptedSize to ?int, fixes verifyUnencryptedSize to compare against header size instead of 0, and corrects Scanner to populate unencrypted_size on initial upload. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Stephen Cuppett --- lib/private/Files/Cache/CacheEntry.php | 2 +- lib/private/Files/Cache/Scanner.php | 15 ++++++++++++--- lib/private/Files/FileInfo.php | 2 +- lib/private/Files/Storage/Wrapper/Encryption.php | 1 + lib/private/Files/Stream/Encryption.php | 2 +- 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/private/Files/Cache/CacheEntry.php b/lib/private/Files/Cache/CacheEntry.php index 74af7d0a59800..5f20bfdd3e613 100644 --- a/lib/private/Files/Cache/CacheEntry.php +++ b/lib/private/Files/Cache/CacheEntry.php @@ -140,7 +140,7 @@ public function __clone() { #[\Override] public function getUnencryptedSize(): int { - if ($this->data['encrypted'] && isset($this->data['unencrypted_size']) && $this->data['unencrypted_size'] > 0) { + if ($this->data['encrypted'] && isset($this->data['unencrypted_size'])) { return $this->data['unencrypted_size']; } else { return $this->data['size'] ?? 0; diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php index 36c8c02a8bfca..df73ccd79d94d 100644 --- a/lib/private/Files/Cache/Scanner.php +++ b/lib/private/Files/Cache/Scanner.php @@ -183,8 +183,12 @@ public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = } } - // we only updated unencrypted_size if it's already set - if (isset($cacheData['unencrypted_size']) && $cacheData['unencrypted_size'] === 0) { + // Only skip updating unencrypted_size if both cached and new values are 0 + // This allows updating from incorrect cached 0 to correct non-zero value + // while avoiding unnecessary updates when both are legitimately 0 + if (isset($cacheData['unencrypted_size']) + && $cacheData['unencrypted_size'] === 0 + && (!isset($data['unencrypted_size']) || $data['unencrypted_size'] === 0)) { unset($data['unencrypted_size']); } @@ -202,7 +206,12 @@ public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = $data['etag_changed'] = true; } } else { - unset($data['unencrypted_size']); + // For new files, preserve unencrypted_size only when the file is encrypted + // and the key is present; otherwise clear it so it won't be written stale. + if (!isset($data['encrypted']) || !$data['encrypted'] + || !isset($data['unencrypted_size'])) { + unset($data['unencrypted_size']); + } $newData = $data; $fileId = -1; } diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 65280b0f31580..37d96ba27ff2b 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -173,7 +173,7 @@ public function getSize($includeMounts = true) { if ($includeMounts) { $this->updateEntryFromSubMounts(); - if ($this->isEncrypted() && isset($this->data['unencrypted_size']) && $this->data['unencrypted_size'] > 0) { + if ($this->isEncrypted() && isset($this->data['unencrypted_size'])) { return $this->data['unencrypted_size']; } else { return isset($this->data['size']) ? 0 + $this->data['size'] : 0; diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 8edf25e9a5111..a256dfaa63d1d 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -400,6 +400,7 @@ protected function verifyUnencryptedSize(string $path, int $unencryptedSize): in if ($unencryptedSize < 0 || ($size > 0 && $unencryptedSize === $size) || $unencryptedSize > $size + || ($unencryptedSize === 0 && $size > $this->util->getHeaderSize()) ) { // check if we already calculate the unencrypted size for the // given path to avoid recursions diff --git a/lib/private/Files/Stream/Encryption.php b/lib/private/Files/Stream/Encryption.php index 1c662aa023c6c..7b83ce0592a80 100644 --- a/lib/private/Files/Stream/Encryption.php +++ b/lib/private/Files/Stream/Encryption.php @@ -28,7 +28,7 @@ class Encryption extends Wrapper { protected string $cache; protected ?int $size = null; protected int $position; - protected ?int $unencryptedSize = null; + protected int|float|null $unencryptedSize = null; protected int $headerSize; protected int $unencryptedBlockSize; protected array $header; From eef0a39c100b3dae5ff3eb7298041b9fa746421c Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Wed, 29 Apr 2026 11:59:02 -0400 Subject: [PATCH 3/5] test(encryption): Add S3 encryption test suite for primary object storage Comprehensive tests covering encryption with S3 as primary storage backend, including upload/download, multipart, migration detection, and key validation. EncryptionTrait updated to use IAppConfig and validate share/master keys on setup. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Stephen Cuppett --- .../ObjectStore/S3EncryptionMigrationTest.php | 217 ++++++++ .../Files/ObjectStore/S3EncryptionTest.php | 482 ++++++++++++++++++ tests/lib/Traits/EncryptionTrait.php | 25 +- 3 files changed, 716 insertions(+), 8 deletions(-) create mode 100644 tests/lib/Files/ObjectStore/S3EncryptionMigrationTest.php create mode 100644 tests/lib/Files/ObjectStore/S3EncryptionTest.php diff --git a/tests/lib/Files/ObjectStore/S3EncryptionMigrationTest.php b/tests/lib/Files/ObjectStore/S3EncryptionMigrationTest.php new file mode 100644 index 0000000000000..8abbe35eba73d --- /dev/null +++ b/tests/lib/Files/ObjectStore/S3EncryptionMigrationTest.php @@ -0,0 +1,217 @@ +getSystemValue('objectstore'); + if (!is_array($config) || ($config['class'] ?? null) !== S3::class) { + self::markTestSkipped('S3 primary storage not configured'); + } + } + + protected function setUp(): void { + parent::setUp(); + + $s3Config = Server::get(IConfig::class)->getSystemValue('objectstore'); + $this->bucket = $s3Config['arguments']['bucket'] ?? 'nextcloud'; + $this->objectStore = new S3($s3Config['arguments']); + + if (!$this->userManager->userExists(self::TEST_USER)) { + $this->createUser(self::TEST_USER, self::TEST_PASSWORD); + } + + $this->setupForUser(self::TEST_USER, self::TEST_PASSWORD); + $this->loginWithEncryption(self::TEST_USER); + + $this->userFolder = \OC::$server->getUserFolder(self::TEST_USER); + $this->view = new \OC\Files\View('/' . self::TEST_USER . '/files'); + } + + protected function tearDown(): void { + try { + if ($this->view) { + // Clean up test files + $testFiles = $this->view->getDirectoryContent(''); + foreach ($testFiles as $file) { + if (str_starts_with($file->getName(), 'migration-test-')) { + $this->view->unlink($file->getName()); + } + } + } + } catch (\Exception $e) { + // Ignore + } + + parent::tearDown(); + } + + /** + * Test that isEncrypted() correctly identifies file state + */ + public function testIsEncryptedFlag(): void { + $testFile = 'migration-test-encrypted-flag.txt'; + $content = 'Test content for encryption flag'; + + // Write file with encryption wrapper (should be encrypted) + $this->view->file_put_contents($testFile, $content); + + // Get file info via node + $node = $this->userFolder->get($testFile); + + // Verify encrypted flag is set via node + $this->assertTrue($node->isEncrypted(), + 'File should be marked as encrypted in database after write'); + + // Verify content is accessible + $readContent = $this->view->file_get_contents($testFile); + $this->assertEquals($content, $readContent, + 'Content should be readable after encryption'); + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test database query to detect unencrypted files + */ + public function testDetectUnencryptedFilesQuery(): void { + // Create encrypted file + $this->view->file_put_contents('migration-test-encrypted.txt', 'encrypted'); + + // Query database for unencrypted files + $db = Server::get(\OCP\IDBConnection::class); + + // Resolve object-store storage IDs first + $storageQuery = $db->getQueryBuilder(); + $storageQuery->select('numeric_id') + ->from('storages') + ->where($storageQuery->expr()->like('id', $storageQuery->createNamedParameter('object::%'))); + $storageResult = $storageQuery->executeQuery(); + $objectStoreIds = array_column($storageResult->fetchAllAssociative(), 'numeric_id'); + $storageResult->closeCursor(); + + $query = $db->getQueryBuilder(); + $query->select($query->func()->count('*', 'total')) + ->from('filecache') + ->where($query->expr()->eq('encrypted', $query->createNamedParameter(0))); + + if (!empty($objectStoreIds)) { + $query->andWhere($query->expr()->in('storage', + $query->createNamedParameter($objectStoreIds, \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT_ARRAY))); + } + + $result = $query->executeQuery(); + $row = $result->fetch(); + $unencryptedCount = $row['total'] ?? 0; + + // Verify the test file actually landed in filecache (query is hitting the right table). + $fileQuery = $db->getQueryBuilder(); + $fileQuery->select('fileid', 'encrypted') + ->from('filecache') + ->where($fileQuery->expr()->like('path', $fileQuery->createNamedParameter('%migration-test-encrypted%'))); + $fileResult = $fileQuery->executeQuery(); + $fileRow = $fileResult->fetch(); + $fileResult->closeCursor(); + + $this->assertNotFalse($fileRow, 'Test file should appear in filecache after write'); + + // The detection query must return a valid numeric result. + $this->assertIsNumeric($unencryptedCount, + 'Should be able to query unencrypted file count'); + + // Clean up + $this->view->unlink('migration-test-encrypted.txt'); + } + + /** + * Test size consistency after simulated migration + */ + public function testSizeConsistencyAfterEncryption(): void { + $testFile = 'migration-test-size-check.bin'; + $size = 50 * 1024; // 50KB + $data = random_bytes($size); + + // Write encrypted file + $this->view->file_put_contents($testFile, $data); + + // Verify size in database + $node = $this->userFolder->get($testFile); + $dbSize = $node->getSize(); + + // Verify actual content size + $readData = $this->view->file_get_contents($testFile); + $actualSize = strlen($readData); + + // Verify S3 size (should be larger) + $fileId = $node->getId(); + $urn = 'urn:oid:' . $fileId; + $s3Result = $this->objectStore->getConnection()->headObject([ + 'Bucket' => $this->bucket, + 'Key' => $urn, + ]); + $s3Size = $s3Result['ContentLength']; + + // Assertions + $this->assertEquals($size, $dbSize, + 'Database should have unencrypted size'); + $this->assertEquals($size, $actualSize, + 'Read content should match original size'); + $this->assertGreaterThan($size, $s3Size, + 'S3 should have encrypted size (larger)'); + + // Clean up + $this->view->unlink($testFile); + } +} diff --git a/tests/lib/Files/ObjectStore/S3EncryptionTest.php b/tests/lib/Files/ObjectStore/S3EncryptionTest.php new file mode 100644 index 0000000000000..ff95fa89e662c --- /dev/null +++ b/tests/lib/Files/ObjectStore/S3EncryptionTest.php @@ -0,0 +1,482 @@ +getSystemValue('objectstore'); + if (!is_array($config) || ($config['class'] ?? null) !== S3::class) { + self::markTestSkipped('S3 primary storage not configured. Configure objectstore in config.php to run these tests.'); + } + } + + protected function setUp(): void { + parent::setUp(); + + // Get S3 config from system config + $this->s3Config = Server::get(IConfig::class)->getSystemValue('objectstore'); + $this->bucket = $this->s3Config['arguments']['bucket'] ?? 'nextcloud'; + + // Create S3 object store + $this->objectStore = new S3($this->s3Config['arguments']); + + // Create test user + if (!$this->userManager->userExists(self::TEST_USER)) { + $this->createUser(self::TEST_USER, self::TEST_PASSWORD); + } + + // Set up encryption for user + $this->setupForUser(self::TEST_USER, self::TEST_PASSWORD); + $this->loginWithEncryption(self::TEST_USER); + + // Get user folder (this will have encryption wrapper applied) + $this->userFolder = \OC::$server->getUserFolder(self::TEST_USER); + + // Get the view for the user + $this->view = new \OC\Files\View('/' . self::TEST_USER . '/files'); + } + + protected function tearDown(): void { + // Clean up test files + try { + if ($this->view) { + $this->cleanupTestFiles(); + } + } catch (\Exception $e) { + // Ignore cleanup errors + } + + parent::tearDown(); + } + + private function cleanupTestFiles(): void { + // Clean up any test files that match our patterns + $patterns = ['test-size-*', 'test-roundtrip-*', 'test-integrity-*', + 'test-partial-read*', 'test-seek*', 'test-multipart*', 'test.txt']; + + foreach ($patterns as $pattern) { + try { + $files = $this->view->getDirectoryContent(''); + foreach ($files as $file) { + $name = $file->getName(); + if (fnmatch($pattern, $name)) { + $this->view->unlink($name); + } + } + } catch (\Exception $e) { + // Ignore + } + } + } + + /** + * Get the S3 URN for a path in the user's files + */ + private function getObjectUrn(string $path): string { + // Get file info from user folder + try { + $node = $this->userFolder->get($path); + $fileId = $node->getId(); + // URN format: urn:oid:{fileId} + return 'urn:oid:' . $fileId; + } catch (\Exception $e) { + throw new \Exception("File not found: $path - " . $e->getMessage(), 0, $e); + } + } + + /** + * Data provider for file sizes + */ + public static function dataFileSizes(): array { + $sizes = [ + '0 bytes (empty file)' => [0], + '1KB' => [1024], + '1MB' => [1024 * 1024], + '5MB (multipart threshold)' => [5 * 1024 * 1024], + '16MB (historical issue)' => [16 * 1024 * 1024], + ]; + if (getenv('RUN_HEAVY_S3_TESTS')) { + $sizes['64MB (historical issue)'] = [64 * 1024 * 1024]; + $sizes['100MB (stress test)'] = [100 * 1024 * 1024]; + } + return $sizes; + } + + /** + * CRITICAL SIZE VALIDATION TEST + * + * This test validates size consistency across three sources: + * 1. Database (filecache) - should store unencrypted size + * 2. S3 Object (headObject) - will be larger (encrypted) + * 3. Actual content - should match original unencrypted size + * + * Known issues exist with size mismatches between these sources. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataFileSizes')] + public function testSizeConsistencyAcrossSources(int $originalSize): void { + $testFile = 'test-size-' . ($originalSize / 1024) . 'kb.bin'; + + // 1. Write file of known size using View (encryption wrapper applied) + $data = $originalSize > 0 ? random_bytes($originalSize) : ''; + $expectedHash = hash('sha256', $data); + $bytesWritten = $this->view->file_put_contents($testFile, $data); + unset($data); + + $this->assertEquals($originalSize, $bytesWritten, + 'file_put_contents should return original size written'); + + // 2. Get database size (from filecache via userFolder) + $node = $this->userFolder->get($testFile); + $dbSize = $node->getSize(); + + // 3. Get S3 object size (encrypted size) directly from S3 + $urn = $this->getObjectUrn($testFile); + $s3Result = $this->objectStore->getConnection()->headObject([ + 'Bucket' => $this->bucket, + 'Key' => $urn, + ]); + $s3Size = $s3Result['ContentLength']; + + // 4. Get actual content size (after decryption via View) + $content = $this->view->file_get_contents($testFile); + $actualSize = strlen($content); + + // ASSERTIONS - Critical size relationships + // After fixing CacheEntry.php getUnencryptedSize() bug, all sizes should be correct + $this->assertEquals($originalSize, $dbSize, + "Database should store unencrypted size (original: $originalSize, db: $dbSize)"); + + $this->assertEquals($originalSize, $actualSize, + "Actual content should match original size after decryption (original: $originalSize, actual: $actualSize)"); + + if ($originalSize === 0) { + // Zero-byte files still get encryption header in S3 + $this->assertGreaterThan(0, $s3Size, + 'S3 should have encryption header even for empty files'); + } else { + $this->assertGreaterThan($originalSize, $s3Size, + "S3 size should be larger than original due to encryption overhead (original: $originalSize, s3: $s3Size)"); + } + + // Verify content integrity via hash to avoid holding two large buffers simultaneously + $this->assertEquals($expectedHash, hash('sha256', $content), + 'Content should be identical after encrypt/decrypt cycle - corruption detected!'); + unset($content); + + // Validate encryption overhead is reasonable + // Binary signed format: Header (8192 bytes) + data blocks + // Each encrypted block is 8192 bytes, holds 8096 bytes unencrypted + // Overhead: ~2% for large files, more for small files due to header + + // Special case for zero-byte files + if ($originalSize === 0) { + // Zero-byte files still get encryption header + $this->assertGreaterThan(0, $s3Size, + 'Even empty files should have encryption header in S3'); + $this->assertLessThanOrEqual(8192, $s3Size, + 'Empty file should only have header block'); + } else { + $overheadPercent = (($s3Size - $originalSize) / $originalSize) * 100; + + // Sanity checks for overhead + if ($originalSize < 10240) { // < 10KB + // Small files have large relative overhead due to 8KB header + $this->assertLessThan(1000, $overheadPercent, + "Encryption overhead should be reasonable even for small files (got: {$overheadPercent}%)"); + } else { + // Larger files should have ~1-3% overhead + $this->assertGreaterThan(0.5, $overheadPercent, + "Should have some encryption overhead (got: {$overheadPercent}%)"); + $this->assertLessThan(5, $overheadPercent, + "Encryption overhead should be under 5% for files > 10KB (got: {$overheadPercent}%)"); + } + } + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test encrypted file round trip - write and read back + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataFileSizes')] + public function testEncryptedFileRoundTrip(int $size): void { + $testFile = 'test-roundtrip-' . ($size / 1024) . 'kb.bin'; + $data = $size > 0 ? random_bytes($size) : ''; + $expectedHash = hash('sha256', $data); + + // Write + $written = $this->view->file_put_contents($testFile, $data); + $this->assertEquals($size, $written); + unset($data); + + // Verify exists + $this->assertTrue($this->view->file_exists($testFile)); + + // Read back + $readData = $this->view->file_get_contents($testFile); + + // Verify size + $this->assertEquals($size, strlen($readData)); + + // Verify content via hash to avoid holding two large buffers in memory + $this->assertEquals($expectedHash, hash('sha256', $readData), 'Content mismatch after round trip'); + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test encrypted file integrity with streaming reads + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataFileSizes')] + public function testEncryptedFileIntegrity(int $size): void { + $testFile = 'test-integrity-' . ($size / 1024) . 'kb.bin'; + $data = $size > 0 ? random_bytes($size) : ''; + $expectedHash = hash('sha256', $data); + + // Write + $this->view->file_put_contents($testFile, $data); + unset($data); + + // Stream read using incremental hashing to avoid large in-memory buffers + $handle = $this->view->fopen($testFile, 'r'); + $this->assertIsResource($handle); + + $ctx = hash_init('sha256'); + $totalRead = 0; + $chunkSize = 8192; + while (!feof($handle)) { + $chunk = fread($handle, $chunkSize); + $totalRead += strlen($chunk); + hash_update($ctx, $chunk); + } + fclose($handle); + + // Verify + $this->assertEquals($size, $totalRead, 'Size mismatch in streaming read'); + $this->assertEquals($expectedHash, hash_final($ctx), 'Content mismatch in streaming read'); + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test partial reads (seeking) on encrypted files + */ + public function testEncryptedFilePartialRead(): void { + $testFile = 'test-partial-read.bin'; + $size = 1024 * 100; // 100KB + $data = random_bytes($size); + + // Write + $this->view->file_put_contents($testFile, $data); + + // Test partial reads at various offsets + $testCases = [ + ['offset' => 0, 'length' => 100], + ['offset' => 1000, 'length' => 500], + ['offset' => 50000, 'length' => 1000], + ['offset' => $size - 100, 'length' => 100], // End of file + ]; + + foreach ($testCases as $test) { + $offset = $test['offset']; + $length = $test['length']; + + $handle = $this->view->fopen($testFile, 'r'); + fseek($handle, $offset); + $chunk = fread($handle, $length); + fclose($handle); + + $expected = substr($data, $offset, $length); + $this->assertEquals($expected, $chunk, + "Partial read mismatch at offset $offset, length $length"); + } + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test seeking within encrypted files + */ + public function testEncryptedFileSeek(): void { + $testFile = 'test-seek.bin'; + $size = 1024 * 50; // 50KB + $data = random_bytes($size); + + // Write + $this->view->file_put_contents($testFile, $data); + + $handle = $this->view->fopen($testFile, 'r'); + + // Test SEEK_SET + fseek($handle, 1000, SEEK_SET); + $this->assertEquals(1000, ftell($handle)); + $chunk = fread($handle, 100); + $this->assertEquals(substr($data, 1000, 100), $chunk); + + // Test SEEK_CUR + fseek($handle, 500, SEEK_CUR); + $this->assertEquals(1600, ftell($handle)); + + // Test SEEK_END + fseek($handle, -100, SEEK_END); + $this->assertEquals($size - 100, ftell($handle)); + $chunk = fread($handle, 100); + $this->assertEquals(substr($data, -100), $chunk); + + fclose($handle); + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test that multipart upload works correctly with encryption + */ + public function testEncryptedMultipartUpload(): void { + if (!getenv('RUN_HEAVY_S3_TESTS')) { + $this->markTestSkipped('Set RUN_HEAVY_S3_TESTS=1 to run large-file multipart tests'); + } + + $testFile = 'test-multipart.bin'; + // 110 MiB file to exceed the multipart upload threshold (~100 MiB); see S3 backend source. + $size = 110 * 1024 * 1024; + $data = random_bytes($size); + $expectedHash = hash('sha256', $data); + + // Write (should use multipart upload) + $written = $this->view->file_put_contents($testFile, $data); + $this->assertEquals($size, $written); + unset($data); + + // Verify file was created + $this->assertTrue($this->view->file_exists($testFile)); + + // Verify size in database + $node = $this->userFolder->get($testFile); + $dbSize = $node->getSize(); + $this->assertEquals($size, $dbSize, + 'Database should have unencrypted size even for multipart upload'); + + // Verify content via streaming hash to avoid loading 110MiB into memory + $stream = $this->view->fopen($testFile, 'rb'); + $this->assertNotFalse($stream, 'Failed to open multipart encrypted upload for reading'); + $hashCtx = hash_init('sha256'); + while (!feof($stream)) { + $chunk = fread($stream, 65536); + if ($chunk !== false && $chunk !== '') { + hash_update($hashCtx, $chunk); + } + } + fclose($stream); + $this->assertEquals($expectedHash, hash_final($hashCtx), + 'Content mismatch for multipart encrypted upload'); + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test that file size tracking works correctly during writes + */ + public function testEncryptedFileSizeTracking(): void { + $testFile = 'test-size-tracking.bin'; + $sizes = [1024, 10240, 102400]; // 1KB, 10KB, 100KB + + foreach ($sizes as $size) { + $data = random_bytes($size); + + // Write + $this->view->file_put_contents($testFile, $data); + + // Check filesize() returns unencrypted size + $reportedSize = $this->view->filesize($testFile); + $this->assertEquals($size, $reportedSize, + "filesize() should return unencrypted size (expected: $size, got: $reportedSize)"); + + // Check stat() returns unencrypted size via userFolder + $node = $this->userFolder->get($testFile); + $nodeSize = $node->getSize(); + $this->assertEquals($size, $nodeSize, + "Node size should return unencrypted size (expected: $size, got: $nodeSize)"); + } + + // Clean up + $this->view->unlink($testFile); + } + + /** + * Test mime type handling with encryption + */ + public function testEncryptedFileMimeType(): void { + $testFile = 'test.txt'; + $data = 'This is a text file'; + + // Write + $this->view->file_put_contents($testFile, $data); + + // Get mime type via userFolder node + $node = $this->userFolder->get($testFile); + $mimeType = $node->getMimetype(); + + // Should detect as text/plain + $this->assertEquals('text/plain', $mimeType, + 'MIME type detection should work on encrypted files'); + + // Clean up + $this->view->unlink($testFile); + } +} diff --git a/tests/lib/Traits/EncryptionTrait.php b/tests/lib/Traits/EncryptionTrait.php index 2dc9213ff2431..42d04376a53f8 100644 --- a/tests/lib/Traits/EncryptionTrait.php +++ b/tests/lib/Traits/EncryptionTrait.php @@ -34,7 +34,7 @@ abstract protected function registerStorageWrapper($name, $wrapper); abstract protected static function markTestSkipped(string $message = ''): void; abstract protected static function assertTrue($condition, string $message = ''): void; - private $encryptionWasEnabled; + private bool $encryptionWasEnabled = false; private $originalEncryptionModule; @@ -109,18 +109,27 @@ protected function setUpEncryptionTrait() { $this->encryptionApp = new Application([], $isReady); $this->config = Server::get(IConfig::class); - $this->encryptionWasEnabled = $this->config->getAppValue('core', 'encryption_enabled', 'no'); - $this->originalEncryptionModule = $this->config->getAppValue('core', 'default_encryption_module'); - $this->config->setAppValue('core', 'default_encryption_module', Encryption::ID); - $this->config->setAppValue('core', 'encryption_enabled', 'yes'); + $appConfig = Server::get(\OCP\IAppConfig::class); + $this->encryptionWasEnabled = $appConfig->getValueBool('core', 'encryption_enabled', false); + $this->originalEncryptionModule = $appConfig->getValueString('core', 'default_encryption_module', ''); + $appConfig->setValueString('core', 'default_encryption_module', Encryption::ID); + $appConfig->setValueBool('core', 'encryption_enabled', true); $this->assertTrue(Server::get(\OCP\Encryption\IManager::class)->isEnabled()); + + // Ensure system-wide share/master keys exist in the keystore. + // Setup::setupSystem() is cache-gated on 'keys-validated' and may no-op + // when the cache was primed while encryption was disabled. + $keyManager = Server::get(KeyManager::class); + $keyManager->validateShareKey(); + $keyManager->validateMasterKey(); } protected function tearDownEncryptionTrait() { if ($this->config) { - $this->config->setAppValue('core', 'encryption_enabled', $this->encryptionWasEnabled); - $this->config->setAppValue('core', 'default_encryption_module', $this->originalEncryptionModule); - $this->config->deleteAppValue('encryption', 'useMasterKey'); + $appConfig = Server::get(\OCP\IAppConfig::class); + $appConfig->setValueBool('core', 'encryption_enabled', $this->encryptionWasEnabled); + $appConfig->setValueString('core', 'default_encryption_module', $this->originalEncryptionModule); + $appConfig->deleteKey('encryption', 'useMasterKey'); } } } From e34918ae9dc4dcdfcbf26d53c6645956dac287fa Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Wed, 29 Apr 2026 11:59:09 -0400 Subject: [PATCH 4/5] fix(tests): Fix encryption test isolation between test runs Add global encryption teardown to TestCase base class so encryption state does not leak between test suites regardless of which tests ran earlier. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Stephen Cuppett --- tests/lib/TestCase.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php index aed919a1e6fe0..b9e5dfc5500e3 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -27,6 +27,7 @@ use OCP\Command\IBus; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\IRootFolder; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\IUserManager; @@ -195,6 +196,22 @@ protected function tearDown(): void { call_user_func([$this, $methodName]); } } + + // Clean up encryption state to prevent test pollution + // This ensures encryption_enabled is reset after each test, preventing + // MultiKeyEncryptException failures in subsequent tests when encryption + // is left enabled but user keys don't exist + try { + $appConfig = Server::get(IAppConfig::class); + $currentValue = $appConfig->getValueBool('core', 'encryption_enabled', false); + if ($currentValue) { + $appConfig->setValueBool('core', 'encryption_enabled', false); + $appConfig->deleteKey('core', 'default_encryption_module'); + $appConfig->deleteKey('encryption', 'useMasterKey'); + } + } catch (\Throwable $e) { + // Ignore - may be called before bootstrap completes + } } /** From d99a8c6e1926406aa6be285c7636d6735f228b01 Mon Sep 17 00:00:00 2001 From: Stephen Cuppett Date: Wed, 29 Apr 2026 11:59:33 -0400 Subject: [PATCH 5/5] refactor(encryption): Migrate appconfig keys to typed bool IAppConfig with repair step Switch all encryption config reads/writes from deprecated string-typed IConfig to bool-typed IAppConfig (getValueBool/setValueBool). Adds RetypeEncryptionConfigKeys repair step to retype existing string values to bool on upgrade. Includes lazy IAppConfig resolution in Manager and AppConfigTypeConflictException fallbacks throughout for safety during the upgrade window. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Stephen Cuppett --- apps/encryption/lib/Settings/Admin.php | 1 + apps/encryption/lib/Util.php | 17 +-- apps/encryption/tests/Settings/AdminTest.php | 6 +- apps/encryption/tests/UtilTest.php | 29 ++--- .../lib/Controller/AppConfigController.php | 3 +- .../Controller/AppConfigControllerTest.php | 6 ++ build/psalm-baseline.xml | 15 --- core/Command/Encryption/DecryptAll.php | 26 +++-- core/Command/Encryption/Disable.php | 20 +++- core/Command/Encryption/Enable.php | 22 +++- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Encryption/Manager.php | 15 ++- lib/private/Repair.php | 2 + .../Repair/RetypeEncryptionConfigKeys.php | 55 ++++++++++ .../Command/Encryption/DecryptAllTest.php | 6 +- tests/Core/Command/Encryption/DisableTest.php | 30 +++--- tests/Core/Command/Encryption/EnableTest.php | 51 +++++---- tests/lib/Encryption/ManagerTest.php | 14 ++- .../Repair/RetypeEncryptionConfigKeysTest.php | 101 ++++++++++++++++++ 20 files changed, 311 insertions(+), 110 deletions(-) create mode 100644 lib/private/Repair/RetypeEncryptionConfigKeys.php create mode 100644 tests/lib/Repair/RetypeEncryptionConfigKeysTest.php diff --git a/apps/encryption/lib/Settings/Admin.php b/apps/encryption/lib/Settings/Admin.php index 422d338d45e03..13cd9a4017e1a 100644 --- a/apps/encryption/lib/Settings/Admin.php +++ b/apps/encryption/lib/Settings/Admin.php @@ -53,6 +53,7 @@ public function getForm() { $crypt, $this->userSession, $this->config, + $this->appConfig, $this->userManager); // Check if an adminRecovery account is enabled for recovering files after lost pwd diff --git a/apps/encryption/lib/Util.php b/apps/encryption/lib/Util.php index ccbdcdcb242b1..bc6d50b86aa09 100644 --- a/apps/encryption/lib/Util.php +++ b/apps/encryption/lib/Util.php @@ -11,6 +11,7 @@ use OC\Files\View; use OCA\Encryption\Crypto\Crypt; use OCP\Files\Storage\IStorage; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IUser; use OCP\IUserManager; @@ -25,6 +26,7 @@ public function __construct( private Crypt $crypt, IUserSession $userSession, private IConfig $config, + private IAppConfig $appConfig, private IUserManager $userManager, ) { $this->user = $userSession->isLoggedIn() ? $userSession->getUser() : false; @@ -51,13 +53,7 @@ public function isRecoveryEnabledForUser($uid) { * @return bool */ public function shouldEncryptHomeStorage() { - $encryptHomeStorage = $this->config->getAppValue( - 'encryption', - 'encryptHomeStorage', - '1' - ); - - return ($encryptHomeStorage === '1'); + return $this->appConfig->getValueBool('encryption', 'encryptHomeStorage', true); } /** @@ -66,12 +62,7 @@ public function shouldEncryptHomeStorage() { * @param bool $encryptHomeStorage */ public function setEncryptHomeStorage($encryptHomeStorage) { - $value = $encryptHomeStorage ? '1' : '0'; - $this->config->setAppValue( - 'encryption', - 'encryptHomeStorage', - $value - ); + $this->appConfig->setValueBool('encryption', 'encryptHomeStorage', (bool)$encryptHomeStorage); } /** diff --git a/apps/encryption/tests/Settings/AdminTest.php b/apps/encryption/tests/Settings/AdminTest.php index 10d2a61c5279e..f81dfe3e87115 100644 --- a/apps/encryption/tests/Settings/AdminTest.php +++ b/apps/encryption/tests/Settings/AdminTest.php @@ -62,7 +62,8 @@ public function testGetForm(): void { $this->appConfig ->method('getValueBool') ->willReturnMap([ - ['encryption', 'recoveryAdminEnabled', true] + ['encryption', 'recoveryAdminEnabled', true], + ['encryption', 'encryptHomeStorage', true, true], ]); $this->config ->method('getAppValue') @@ -70,9 +71,6 @@ public function testGetForm(): void { if ($app === 'encryption' && $key === 'recoveryAdminEnabled' && $default === '0') { return '1'; } - if ($app === 'encryption' && $key === 'encryptHomeStorage' && $default === '1') { - return '1'; - } return $default; }); diff --git a/apps/encryption/tests/UtilTest.php b/apps/encryption/tests/UtilTest.php index ed274f74ab5ca..5f42f18ab4632 100644 --- a/apps/encryption/tests/UtilTest.php +++ b/apps/encryption/tests/UtilTest.php @@ -14,6 +14,7 @@ use OCA\Encryption\Util; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage\IStorage; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IUser; use OCP\IUserManager; @@ -26,6 +27,7 @@ class UtilTest extends TestCase { protected Util $instance; protected static $tempStorage = []; + protected IAppConfig&MockObject $appConfigMock; protected IConfig&MockObject $configMock; protected View&MockObject $filesMock; protected IUserManager&MockObject $userManagerMock; @@ -78,6 +80,7 @@ protected function setUp(): void { ->willReturn(true); $this->configMock = $this->createMock(IConfig::class); + $this->appConfigMock = $this->createMock(IAppConfig::class); $this->configMock->expects($this->any()) ->method('getUserValue') @@ -87,7 +90,7 @@ protected function setUp(): void { ->method('setUserValue') ->willReturnCallback([$this, 'setValueTester']); - $this->instance = new Util($this->filesMock, $cryptMock, $userSessionMock, $this->configMock, $this->userManagerMock); + $this->instance = new Util($this->filesMock, $cryptMock, $userSessionMock, $this->configMock, $this->appConfigMock, $this->userManagerMock); } /** @@ -136,13 +139,13 @@ public static function dataTestIsMasterKeyEnabled(): array { } /** - * @param string $returnValue return value from getAppValue() + * @param bool $returnValue return value from getValueBool() * @param bool $expected */ #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestShouldEncryptHomeStorage')] - public function testShouldEncryptHomeStorage($returnValue, $expected): void { - $this->configMock->expects($this->once())->method('getAppValue') - ->with('encryption', 'encryptHomeStorage', '1') + public function testShouldEncryptHomeStorage(bool $returnValue, bool $expected): void { + $this->appConfigMock->expects($this->once())->method('getValueBool') + ->with('encryption', 'encryptHomeStorage', true) ->willReturn($returnValue); $this->assertSame($expected, @@ -151,26 +154,26 @@ public function testShouldEncryptHomeStorage($returnValue, $expected): void { public static function dataTestShouldEncryptHomeStorage(): array { return [ - ['1', true], - ['0', false] + [true, true], + [false, false] ]; } /** - * @param $value - * @param $expected + * @param bool $value + * @param bool $expected */ #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestSetEncryptHomeStorage')] - public function testSetEncryptHomeStorage($value, $expected): void { - $this->configMock->expects($this->once())->method('setAppValue') + public function testSetEncryptHomeStorage(bool $value, bool $expected): void { + $this->appConfigMock->expects($this->once())->method('setValueBool') ->with('encryption', 'encryptHomeStorage', $expected); $this->instance->setEncryptHomeStorage($value); } public static function dataTestSetEncryptHomeStorage(): array { return [ - [true, '1'], - [false, '0'] + [true, true], + [false, false] ]; } diff --git a/apps/provisioning_api/lib/Controller/AppConfigController.php b/apps/provisioning_api/lib/Controller/AppConfigController.php index 0c53e3c009110..374d74ea9cd24 100644 --- a/apps/provisioning_api/lib/Controller/AppConfigController.php +++ b/apps/provisioning_api/lib/Controller/AppConfigController.php @@ -203,7 +203,8 @@ protected function verifyConfigKey(string $app, string $key, string $value) { throw new \InvalidArgumentException('The given key can not be set'); } - if ($app === 'core' && $key === 'encryption_enabled' && $value !== 'yes') { + if ($app === 'core' && $key === 'encryption_enabled' + && !in_array(strtolower(trim($value)), ['yes', '1', 'true', 'on'], true)) { throw new \InvalidArgumentException('The given key can not be set'); } diff --git a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php index 55885aabf0a42..a1253411871b0 100644 --- a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php +++ b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php @@ -367,6 +367,10 @@ public static function dataVerifyConfigKey(): array { ['dav', 'public_route', ''], ['files', 'remote_route', ''], ['core', 'encryption_enabled', 'yes'], + ['core', 'encryption_enabled', '1'], + ['core', 'encryption_enabled', 'true'], + ['core', 'encryption_enabled', 'YES'], + ['core', 'encryption_enabled', 'on'], ]; } @@ -384,6 +388,8 @@ public static function dataVerifyConfigKeyThrows(): array { ['contacts', 'types', ''], ['core', 'encryption_enabled', 'no'], ['core', 'encryption_enabled', ''], + ['core', 'encryption_enabled', '0'], + ['core', 'encryption_enabled', 'false'], ['core', 'public_files', ''], ['core', 'public_dav', ''], ['core', 'remote_files', ''], diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 4b83ed5e23a90..2f8be2212060f 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1300,10 +1300,8 @@ - - @@ -3023,19 +3021,6 @@ - - - - - - - - - - - - - diff --git a/core/Command/Encryption/DecryptAll.php b/core/Command/Encryption/DecryptAll.php index 85a70b49cec24..56fd22d3b2a6b 100644 --- a/core/Command/Encryption/DecryptAll.php +++ b/core/Command/Encryption/DecryptAll.php @@ -9,6 +9,7 @@ namespace OC\Core\Command\Encryption; use OCP\App\IAppManager; +use OCP\Exceptions\AppConfigTypeConflictException; use OCP\IAppConfig; use OCP\IConfig; use Symfony\Component\Console\Command\Command; @@ -91,11 +92,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int return 1; } - $originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled'); + try { + $originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false); + } catch (AppConfigTypeConflictException) { + $raw = $this->appConfig->getValueString('core', 'encryption_enabled', 'no'); + $originallyEnabled = in_array(strtolower(trim($raw)), ['1', 'true', 'yes', 'on'], true); + } try { if ($originallyEnabled) { $output->write('Disable server side encryption... '); - $this->appConfig->setValueBool('core', 'encryption_enabled', false); + $this->writeEncryptionEnabled(false); $output->writeln('done.'); } else { $output->writeln('Server side encryption not enabled. Nothing to do.'); @@ -123,18 +129,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln(' aborted.'); if ($originallyEnabled) { $output->writeln('Server side encryption remains enabled'); - $this->appConfig->setValueBool('core', 'encryption_enabled', true); + $this->writeEncryptionEnabled(true); } } elseif (($uid !== '') && $originallyEnabled) { $output->writeln('Server side encryption remains enabled'); - $this->appConfig->setValueBool('core', 'encryption_enabled', true); + $this->writeEncryptionEnabled(true); } $this->resetMaintenanceAndTrashbin(); return 0; } if ($originallyEnabled) { $output->write('Enable server side encryption... '); - $this->appConfig->setValueBool('core', 'encryption_enabled', true); + $this->writeEncryptionEnabled(true); $output->writeln('done.'); } $output->writeln('aborted'); @@ -142,10 +148,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int } catch (\Exception $e) { // enable server side encryption again if something went wrong if ($originallyEnabled) { - $this->appConfig->setValueBool('core', 'encryption_enabled', true); + $this->writeEncryptionEnabled(true); } $this->resetMaintenanceAndTrashbin(); throw $e; } } + + private function writeEncryptionEnabled(bool $enabled): void { + try { + $this->appConfig->setValueBool('core', 'encryption_enabled', $enabled); + } catch (AppConfigTypeConflictException) { + $this->appConfig->setValueString('core', 'encryption_enabled', $enabled ? 'yes' : 'no'); + } + } } diff --git a/core/Command/Encryption/Disable.php b/core/Command/Encryption/Disable.php index fe280daa111b4..0f69be38f1011 100644 --- a/core/Command/Encryption/Disable.php +++ b/core/Command/Encryption/Disable.php @@ -9,14 +9,15 @@ */ namespace OC\Core\Command\Encryption; -use OCP\IConfig; +use OCP\Exceptions\AppConfigTypeConflictException; +use OCP\IAppConfig; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; class Disable extends Command { public function __construct( - protected IConfig $config, + protected IAppConfig $appConfig, ) { parent::__construct(); } @@ -31,10 +32,21 @@ protected function configure() { #[\Override] protected function execute(InputInterface $input, OutputInterface $output): int { - if ($this->config->getAppValue('core', 'encryption_enabled', 'no') !== 'yes') { + try { + $isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false); + } catch (AppConfigTypeConflictException) { + $raw = $this->appConfig->getValueString('core', 'encryption_enabled', 'no'); + $isEnabled = in_array(strtolower(trim($raw)), ['1', 'true', 'yes', 'on'], true); + } + + if (!$isEnabled) { $output->writeln('Encryption is already disabled'); } else { - $this->config->setAppValue('core', 'encryption_enabled', 'no'); + try { + $this->appConfig->setValueBool('core', 'encryption_enabled', false); + } catch (AppConfigTypeConflictException) { + $this->appConfig->setValueString('core', 'encryption_enabled', 'no'); + } $output->writeln('Encryption disabled'); } return 0; diff --git a/core/Command/Encryption/Enable.php b/core/Command/Encryption/Enable.php index 02c610250011c..66d30032f5842 100644 --- a/core/Command/Encryption/Enable.php +++ b/core/Command/Encryption/Enable.php @@ -10,14 +10,15 @@ namespace OC\Core\Command\Encryption; use OCP\Encryption\IManager; -use OCP\IConfig; +use OCP\Exceptions\AppConfigTypeConflictException; +use OCP\IAppConfig; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; class Enable extends Command { public function __construct( - protected IConfig $config, + protected IAppConfig $appConfig, protected IManager $encryptionManager, ) { parent::__construct(); @@ -33,10 +34,21 @@ protected function configure() { #[\Override] protected function execute(InputInterface $input, OutputInterface $output): int { - if ($this->config->getAppValue('core', 'encryption_enabled', 'no') === 'yes') { + try { + $isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false); + } catch (AppConfigTypeConflictException) { + $raw = $this->appConfig->getValueString('core', 'encryption_enabled', 'no'); + $isEnabled = in_array(strtolower(trim($raw)), ['1', 'true', 'yes', 'on'], true); + } + + if ($isEnabled) { $output->writeln('Encryption is already enabled'); } else { - $this->config->setAppValue('core', 'encryption_enabled', 'yes'); + try { + $this->appConfig->setValueBool('core', 'encryption_enabled', true); + } catch (AppConfigTypeConflictException) { + $this->appConfig->setValueString('core', 'encryption_enabled', 'yes'); + } $output->writeln('Encryption enabled'); } $output->writeln(''); @@ -46,7 +58,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln('No encryption module is loaded'); return 1; } - $defaultModule = $this->config->getAppValue('core', 'default_encryption_module'); + $defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module', ''); if ($defaultModule === '') { $output->writeln('No default module is set'); return 1; diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 6d57f1e487842..de2f29d2535f4 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2092,6 +2092,7 @@ 'OC\\Repair\\RepairInvalidShares' => $baseDir . '/lib/private/Repair/RepairInvalidShares.php', 'OC\\Repair\\RepairLogoDimension' => $baseDir . '/lib/private/Repair/RepairLogoDimension.php', 'OC\\Repair\\RepairMimeTypes' => $baseDir . '/lib/private/Repair/RepairMimeTypes.php', + 'OC\\Repair\\RetypeEncryptionConfigKeys' => $baseDir . '/lib/private/Repair/RetypeEncryptionConfigKeys.php', 'OC\\RichObjectStrings\\RichTextFormatter' => $baseDir . '/lib/private/RichObjectStrings/RichTextFormatter.php', 'OC\\RichObjectStrings\\Validator' => $baseDir . '/lib/private/RichObjectStrings/Validator.php', 'OC\\Route\\CachingRouter' => $baseDir . '/lib/private/Route/CachingRouter.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 564995dbfa757..1584ff0333dfa 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2133,6 +2133,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Repair\\RepairInvalidShares' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairInvalidShares.php', 'OC\\Repair\\RepairLogoDimension' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairLogoDimension.php', 'OC\\Repair\\RepairMimeTypes' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairMimeTypes.php', + 'OC\\Repair\\RetypeEncryptionConfigKeys' => __DIR__ . '/../../..' . '/lib/private/Repair/RetypeEncryptionConfigKeys.php', 'OC\\RichObjectStrings\\RichTextFormatter' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/RichTextFormatter.php', 'OC\\RichObjectStrings\\Validator' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/Validator.php', 'OC\\Route\\CachingRouter' => __DIR__ . '/../../..' . '/lib/private/Route/CachingRouter.php', diff --git a/lib/private/Encryption/Manager.php b/lib/private/Encryption/Manager.php index ac27f0911b8da..f8b278d3d3b9e 100644 --- a/lib/private/Encryption/Manager.php +++ b/lib/private/Encryption/Manager.php @@ -16,10 +16,12 @@ use OC\ServiceUnavailableException; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; +use OCP\Exceptions\AppConfigTypeConflictException; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage\IStorage; use OCP\IConfig; use OCP\IL10N; +use OCP\Server; use Psr\Log\LoggerInterface; class Manager implements IManager { @@ -48,8 +50,17 @@ public function isEnabled() { return false; } - $enabled = $this->config->getAppValue('core', 'encryption_enabled', 'no'); - return $enabled === 'yes'; + try { + return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false); + } catch (AppConfigTypeConflictException) { + // Stored as VALUE_STRING from a pre-upgrade installation. + // RetypeEncryptionConfigKeys repair step will fix the type on occ upgrade. + $raw = Server::get(\OCP\IAppConfig::class)->getValueString('core', 'encryption_enabled', 'no'); + return in_array(strtolower(trim($raw)), ['1', 'true', 'yes', 'on'], true); + } catch (\Throwable) { + // DB not ready (e.g. oc_appconfig does not yet exist during install). + return false; + } } /** diff --git a/lib/private/Repair.php b/lib/private/Repair.php index 90e209cfe9085..3bc5827e4c44e 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -56,6 +56,7 @@ use OC\Repair\RepairInvalidShares; use OC\Repair\RepairLogoDimension; use OC\Repair\RepairMimeTypes; +use OC\Repair\RetypeEncryptionConfigKeys; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; @@ -157,6 +158,7 @@ public function addStep(IRepairStep|string $repairStep, bool $includeExpensive = */ public static function getRepairSteps(bool $includeExpensive = false): array { $repairSteps = [ + Server::get(RetypeEncryptionConfigKeys::class), new Collation(Server::get(IConfig::class), Server::get(LoggerInterface::class), Server::get(IDBConnection::class), false), Server::get(CleanTags::class), Server::get(RepairInvalidShares::class), diff --git a/lib/private/Repair/RetypeEncryptionConfigKeys.php b/lib/private/Repair/RetypeEncryptionConfigKeys.php new file mode 100644 index 0000000000000..48f0ce17c12c6 --- /dev/null +++ b/lib/private/Repair/RetypeEncryptionConfigKeys.php @@ -0,0 +1,55 @@ +appConfig->getValueType($app, $key); + } catch (AppConfigUnknownKeyException) { + continue; + } + + if (($type & IAppConfig::VALUE_BOOL) === IAppConfig::VALUE_BOOL) { + $output->info("$app.$key is already typed as boolean, skipping."); + continue; + } + + $raw = strtolower(trim($this->appConfig->getValueString($app, $key, $defaultBool ? '1' : '0'))); + $bool = in_array($raw, ['1', 'true', 'yes', 'on'], true); + + $this->appConfig->deleteKey($app, $key); + $this->appConfig->setValueBool($app, $key, $bool); + $output->info("Re-typed $app.$key from string '$raw' to boolean " . ($bool ? 'true' : 'false') . '.'); + } + } +} diff --git a/tests/Core/Command/Encryption/DecryptAllTest.php b/tests/Core/Command/Encryption/DecryptAllTest.php index 009eb36eb7fe2..74ccf0563bfbc 100644 --- a/tests/Core/Command/Encryption/DecryptAllTest.php +++ b/tests/Core/Command/Encryption/DecryptAllTest.php @@ -103,7 +103,7 @@ public function testExecute($encryptionEnabled, $continue): void { $this->appConfig->expects($this->once()) ->method('getValueBool') - ->with('core', 'encryption_enabled') + ->with('core', 'encryption_enabled', false) ->willReturn($encryptionEnabled); $this->consoleInput->expects($this->any()) @@ -168,7 +168,7 @@ public function testExecuteFailure(): void { ['core', 'encryption_enabled', true, false], ]; $this->appConfig->expects($this->exactly(2)) - ->method('setValuebool') + ->method('setValueBool') ->willReturnCallback(function () use (&$calls): bool { $expected = array_shift($calls); $this->assertEquals($expected, func_get_args()); @@ -176,7 +176,7 @@ public function testExecuteFailure(): void { }); $this->appConfig->expects($this->once()) ->method('getValueBool') - ->with('core', 'encryption_enabled') + ->with('core', 'encryption_enabled', false) ->willReturn(true); $this->consoleInput->expects($this->any()) diff --git a/tests/Core/Command/Encryption/DisableTest.php b/tests/Core/Command/Encryption/DisableTest.php index 96310a6c75ba3..b2ef86c2921f2 100644 --- a/tests/Core/Command/Encryption/DisableTest.php +++ b/tests/Core/Command/Encryption/DisableTest.php @@ -9,14 +9,14 @@ namespace Tests\Core\Command\Encryption; use OC\Core\Command\Encryption\Disable; -use OCP\IConfig; +use OCP\IAppConfig; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Test\TestCase; class DisableTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject */ - protected $config; + protected $appConfig; /** @var \PHPUnit\Framework\MockObject\MockObject */ protected $consoleInput; /** @var \PHPUnit\Framework\MockObject\MockObject */ @@ -29,35 +29,35 @@ class DisableTest extends TestCase { protected function setUp(): void { parent::setUp(); - $config = $this->config = $this->getMockBuilder(IConfig::class) + $appConfig = $this->appConfig = $this->getMockBuilder(IAppConfig::class) ->disableOriginalConstructor() ->getMock(); $this->consoleInput = $this->getMockBuilder(InputInterface::class)->getMock(); $this->consoleOutput = $this->getMockBuilder(OutputInterface::class)->getMock(); - /** @var IConfig $config */ - $this->command = new Disable($config); + /** @var IAppConfig $appConfig */ + $this->command = new Disable($appConfig); } public static function dataDisable(): array { return [ - ['yes', true, 'Encryption disabled'], - ['no', false, 'Encryption is already disabled'], + [true, true, 'Encryption disabled'], + [false, false, 'Encryption is already disabled'], ]; } /** * - * @param string $oldStatus + * @param bool $oldStatus * @param bool $isUpdating * @param string $expectedString */ #[\PHPUnit\Framework\Attributes\DataProvider('dataDisable')] - public function testDisable($oldStatus, $isUpdating, $expectedString): void { - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('core', 'encryption_enabled', $this->anything()) + public function testDisable(bool $oldStatus, bool $isUpdating, string $expectedString): void { + $this->appConfig->expects($this->once()) + ->method('getValueBool') + ->with('core', 'encryption_enabled', false) ->willReturn($oldStatus); $this->consoleOutput->expects($this->once()) @@ -65,9 +65,9 @@ public function testDisable($oldStatus, $isUpdating, $expectedString): void { ->with($this->stringContains($expectedString)); if ($isUpdating) { - $this->config->expects($this->once()) - ->method('setAppValue') - ->with('core', 'encryption_enabled', 'no'); + $this->appConfig->expects($this->once()) + ->method('setValueBool') + ->with('core', 'encryption_enabled', false); } self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]); diff --git a/tests/Core/Command/Encryption/EnableTest.php b/tests/Core/Command/Encryption/EnableTest.php index 234984b1c0954..2acc1458e81aa 100644 --- a/tests/Core/Command/Encryption/EnableTest.php +++ b/tests/Core/Command/Encryption/EnableTest.php @@ -10,14 +10,14 @@ use OC\Core\Command\Encryption\Enable; use OCP\Encryption\IManager; -use OCP\IConfig; +use OCP\IAppConfig; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Test\TestCase; class EnableTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject */ - protected $config; + protected $appConfig; /** @var \PHPUnit\Framework\MockObject\MockObject */ protected $manager; /** @var \PHPUnit\Framework\MockObject\MockObject */ @@ -32,7 +32,7 @@ class EnableTest extends TestCase { protected function setUp(): void { parent::setUp(); - $config = $this->config = $this->getMockBuilder(IConfig::class) + $appConfig = $this->appConfig = $this->getMockBuilder(IAppConfig::class) ->disableOriginalConstructor() ->getMock(); $manager = $this->manager = $this->getMockBuilder(IManager::class) @@ -41,47 +41,44 @@ protected function setUp(): void { $this->consoleInput = $this->getMockBuilder(InputInterface::class)->getMock(); $this->consoleOutput = $this->getMockBuilder(OutputInterface::class)->getMock(); - /** @var \OCP\IConfig $config */ + /** @var \OCP\IAppConfig $appConfig */ /** @var \OCP\Encryption\IManager $manager */ - $this->command = new Enable($config, $manager); + $this->command = new Enable($appConfig, $manager); } public static function dataEnable(): array { return [ - ['no', '', [], true, 'Encryption enabled', 'No encryption module is loaded'], - ['yes', '', [], false, 'Encryption is already enabled', 'No encryption module is loaded'], - ['no', '', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'No default module is set'], - ['no', 'OC_NO_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'The current default module does not exist: OC_NO_MODULE'], - ['no', 'OC_TEST_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'Default module: OC_TEST_MODULE'], + [false, '', [], true, 'Encryption enabled', 'No encryption module is loaded'], + [true, '', [], false, 'Encryption is already enabled', 'No encryption module is loaded'], + [false, '', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'No default module is set'], + [false, 'OC_NO_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'The current default module does not exist: OC_NO_MODULE'], + [false, 'OC_TEST_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'Default module: OC_TEST_MODULE'], ]; } #[\PHPUnit\Framework\Attributes\DataProvider('dataEnable')] - public function testEnable(string $oldStatus, ?string $defaultModule, array $availableModules, bool $isUpdating, string $expectedString, string $expectedDefaultModuleString): void { + public function testEnable(bool $oldStatus, ?string $defaultModule, array $availableModules, bool $isUpdating, string $expectedString, string $expectedDefaultModuleString): void { if ($isUpdating) { - $this->config->expects($this->once()) - ->method('setAppValue') - ->with('core', 'encryption_enabled', 'yes'); + $this->appConfig->expects($this->once()) + ->method('setValueBool') + ->with('core', 'encryption_enabled', true); } $this->manager->expects($this->atLeastOnce()) ->method('getEncryptionModules') ->willReturn($availableModules); - if (empty($availableModules)) { - $this->config->expects($this->once()) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'encryption_enabled', 'no', $oldStatus], - ]); - } else { - $this->config->expects($this->exactly(2)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'encryption_enabled', 'no', $oldStatus], - ['core', 'default_encryption_module', '', $defaultModule], - ]); + $this->appConfig->expects($this->once()) + ->method('getValueBool') + ->with('core', 'encryption_enabled', false) + ->willReturn($oldStatus); + + if (!empty($availableModules)) { + $this->appConfig->expects($this->once()) + ->method('getValueString') + ->with('core', 'default_encryption_module', '') + ->willReturn((string)$defaultModule); } $calls = [ diff --git a/tests/lib/Encryption/ManagerTest.php b/tests/lib/Encryption/ManagerTest.php index 5f3d1987dc3dc..2b414f0c64f91 100644 --- a/tests/lib/Encryption/ManagerTest.php +++ b/tests/lib/Encryption/ManagerTest.php @@ -14,8 +14,10 @@ use OC\Files\View; use OC\Memcache\ArrayCache; use OCP\Encryption\IEncryptionModule; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IL10N; +use OCP\Server; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -73,10 +75,18 @@ public function testManagerIsDisabledIfDisabledButModules(): void { $this->assertFalse($this->manager->isEnabled()); } + /** + * @group DB + */ public function testManagerIsEnabled(): void { + $appConfig = Server::get(IAppConfig::class); + $appConfig->setValueBool('core', 'encryption_enabled', true); + $this->config->expects($this->any())->method('getSystemValueBool')->willReturn(true); - $this->config->expects($this->any())->method('getAppValue')->willReturn('yes'); - $this->assertTrue($this->manager->isEnabled()); + $result = $this->manager->isEnabled(); + + $appConfig->deleteKey('core', 'encryption_enabled'); + $this->assertTrue($result); } public function testModuleRegistration() { diff --git a/tests/lib/Repair/RetypeEncryptionConfigKeysTest.php b/tests/lib/Repair/RetypeEncryptionConfigKeysTest.php new file mode 100644 index 0000000000000..48bea2cb97739 --- /dev/null +++ b/tests/lib/Repair/RetypeEncryptionConfigKeysTest.php @@ -0,0 +1,101 @@ +appConfig = Server::get(IAppConfig::class); + $this->output = $this->createMock(IOutput::class); + $this->repair = new RetypeEncryptionConfigKeys($this->appConfig); + // Clean slate in case previous test runs or occ invocations left residue + $this->appConfig->deleteKey('core', 'encryption_enabled'); + $this->appConfig->deleteKey('encryption', 'encryptHomeStorage'); + } + + protected function tearDown(): void { + $this->appConfig->deleteKey('core', 'encryption_enabled'); + $this->appConfig->deleteKey('encryption', 'encryptHomeStorage'); + parent::tearDown(); + } + + public function testAbsentKeyIsNoOp(): void { + $this->output->expects($this->never())->method('info'); + $this->repair->run($this->output); + // No exception, no write + $this->assertTrue(true); + } + + public static function dataStringValues(): array { + return [ + ['yes', true], + ['no', false], + ['1', true], + ['0', false], + ['true', true], + ['false', false], + ['on', true], + ['YES', true], + ]; + } + + /** + * @dataProvider dataStringValues + */ + public function testEncryptionEnabledIsRetypedFromString(string $raw, bool $expected): void { + $this->appConfig->setValueString('core', 'encryption_enabled', $raw); + + $this->repair->run($this->output); + + $this->assertSame(IAppConfig::VALUE_BOOL, $this->appConfig->getValueType('core', 'encryption_enabled')); + $this->assertSame($expected, $this->appConfig->getValueBool('core', 'encryption_enabled', !$expected)); + } + + /** + * @dataProvider dataStringValues + */ + public function testEncryptHomeStorageIsRetypedFromString(string $raw, bool $expected): void { + $this->appConfig->setValueString('encryption', 'encryptHomeStorage', $raw); + + $this->repair->run($this->output); + + $this->assertSame(IAppConfig::VALUE_BOOL, $this->appConfig->getValueType('encryption', 'encryptHomeStorage')); + $this->assertSame($expected, $this->appConfig->getValueBool('encryption', 'encryptHomeStorage', !$expected)); + } + + public function testAlreadyBoolIsNoOp(): void { + $this->appConfig->setValueBool('core', 'encryption_enabled', true); + $this->appConfig->setValueBool('encryption', 'encryptHomeStorage', false); + + // Should log "already typed" messages but not re-write + $this->output->expects($this->exactly(2)) + ->method('info') + ->with($this->stringContains('already typed')); + + $this->repair->run($this->output); + + $this->assertTrue($this->appConfig->getValueBool('core', 'encryption_enabled', false)); + $this->assertFalse($this->appConfig->getValueBool('encryption', 'encryptHomeStorage', true)); + } +}