fix: Harden files scanner for invalid null access

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

4 files changed, 131 insertions, 132 deletions

diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index 49ce3c7b054..e64c28f074d 100644
--- a/build/psalm-baseline.xml
+++ b/build/psalm-baseline.xml
@@ -1796,12 +1796,6 @@
     <InvalidArgument>
       <code><![CDATA[self::SCAN_RECURSIVE_INCOMPLETE]]></code>
     </InvalidArgument>
-    <InvalidReturnStatement>
-      <code><![CDATA[$existingChildren]]></code>
-    </InvalidReturnStatement>
-    <InvalidReturnType>
-      <code><![CDATA[array[]]]></code>
-    </InvalidReturnType>
   </file>
   <file src="lib/private/Files/Cache/Storage.php">
     <InvalidNullableReturnType>
diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php
index cb841755efd..5dcce6d8bbb 100644
--- a/lib/private/Files/Cache/Cache.php
+++ b/lib/private/Files/Cache/Cache.php
@@ -1009,8 +1009,8 @@ class Cache implements ICache {
 			}
 
 			// only set unencrypted size for a folder if any child entries have it set, or the folder is empty
-			$shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || $entry['unencrypted_size'] > 0;
-			if ($entry['size'] !== $totalSize || ($entry['unencrypted_size'] !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) {
+			$shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || ($entry['unencrypted_size'] ?? 0) > 0;
+			if ($entry['size'] !== $totalSize || (($entry['unencrypted_size'] ?? 0) !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) {
 				if ($shouldWriteUnEncryptedSize) {
 					// if all children have an unencrypted size of 0, just set the folder unencrypted size to 0 instead of summing the sizes
 					if ($unencryptedMax === 0) {
diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php
index 3bd674f79e2..2bf1203955e 100644
--- a/lib/private/Files/Cache/Scanner.php
+++ b/lib/private/Files/Cache/Scanner.php
@@ -83,7 +83,7 @@ class Scanner extends BasicEmitter implements IScanner {
 	 *
 	 * @param bool $useTransactions
 	 */
-	public function setUseTransactions($useTransactions) {
+	public function setUseTransactions($useTransactions): void {
 		$this->useTransactions = $useTransactions;
 	}
 
@@ -110,7 +110,7 @@ class Scanner extends BasicEmitter implements IScanner {
 	 * @param int $parentId
 	 * @param array|null|false $cacheData existing data in the cache for the file to be scanned
 	 * @param bool $lock set to false to disable getting an additional read lock during scanning
-	 * @param null $data the metadata for the file, as returned by the storage
+	 * @param array|null $data the metadata for the file, as returned by the storage
 	 * @return array|null an array of metadata of the scanned file
 	 * @throws \OCP\Lock\LockedException
 	 */
@@ -122,139 +122,139 @@ class Scanner extends BasicEmitter implements IScanner {
 				return null;
 			}
 		}
+
 		// only proceed if $file is not a partial file, blacklist is handled by the storage
-		if (!self::isPartialFile($file)) {
-			// acquire a lock
+		if (self::isPartialFile($file)) {
+			return null;
+		}
+
+		// acquire a lock
+		if ($lock) {
+			if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+			}
+		}
+
+		try {
+			$data = $data ?? $this->getData($file);
+		} catch (ForbiddenException $e) {
 			if ($lock) {
 				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-					$this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+					$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 				}
 			}
 
-			try {
-				$data = $data ?? $this->getData($file);
-			} catch (ForbiddenException $e) {
-				if ($lock) {
-					if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-						$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					}
-				}
+			return null;
+		}
 
-				return null;
-			}
+		try {
+			if ($data === null) {
+				$this->removeFromCache($file);
+			} else {
+				// pre-emit only if it was a file. By that we avoid counting/treating folders as files
+				if ($data['mimetype'] !== 'httpd/unix-directory') {
+					$this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]);
+					\OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]);
+				}
 
-			try {
-				if ($data) {
-					// pre-emit only if it was a file. By that we avoid counting/treating folders as files
-					if ($data['mimetype'] !== 'httpd/unix-directory') {
-						$this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]);
-						\OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]);
-					}
+				$parent = dirname($file);
+				if ($parent === '.' || $parent === '/') {
+					$parent = '';
+				}
+				if ($parentId === -1) {
+					$parentId = $this->cache->getParentId($file);
+				}
 
-					$parent = dirname($file);
-					if ($parent === '.' || $parent === '/') {
-						$parent = '';
-					}
-					if ($parentId === -1) {
-						$parentId = $this->cache->getParentId($file);
+				// scan the parent if it's not in the cache (id -1) and the current file is not the root folder
+				if ($file && $parentId === -1) {
+					$parentData = $this->scanFile($parent);
+					if ($parentData === null) {
+						return null;
 					}
 
-					// scan the parent if it's not in the cache (id -1) and the current file is not the root folder
-					if ($file && $parentId === -1) {
-						$parentData = $this->scanFile($parent);
-						if (!$parentData) {
-							return null;
-						}
-						$parentId = $parentData['fileid'];
-					}
-					if ($parent) {
-						$data['parent'] = $parentId;
-					}
-					if (is_null($cacheData)) {
-						/** @var CacheEntry $cacheData */
-						$cacheData = $this->cache->get($file);
-					}
-					if ($cacheData && $reuseExisting && isset($cacheData['fileid'])) {
-						// prevent empty etag
-						$etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag'];
-						$fileId = $cacheData['fileid'];
-						$data['fileid'] = $fileId;
-						// only reuse data if the file hasn't explicitly changed
-						$mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime'];
-						// if the folder is marked as unscanned, never reuse etags
-						if ($mtimeUnchanged && $cacheData['size'] !== -1) {
-							$data['mtime'] = $cacheData['mtime'];
-							if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) {
-								$data['size'] = $cacheData['size'];
-							}
-							if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) {
-								$data['etag'] = $etag;
-							}
-						}
+					$parentId = $parentData['fileid'];
+				}
+				if ($parent) {
+					$data['parent'] = $parentId;
+				}
 
-						// we only updated unencrypted_size if it's already set
-						if ($cacheData['unencrypted_size'] === 0) {
-							unset($data['unencrypted_size']);
+				$cacheData = $cacheData ?? $this->cache->get($file);
+				if ($reuseExisting && $cacheData !== false && isset($cacheData['fileid'])) {
+					// prevent empty etag
+					$etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag'];
+					$fileId = $cacheData['fileid'];
+					$data['fileid'] = $fileId;
+					// only reuse data if the file hasn't explicitly changed
+					$mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime'];
+					// if the folder is marked as unscanned, never reuse etags
+					if ($mtimeUnchanged && $cacheData['size'] !== -1) {
+						$data['mtime'] = $cacheData['mtime'];
+						if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) {
+							$data['size'] = $cacheData['size'];
 						}
-
-						// Only update metadata that has changed
-						// i.e. get all the values in $data that are not present in the cache already
-						$newData = $this->array_diff_assoc_multi($data, $cacheData->getData());
-						
-						// make it known to the caller that etag has been changed and needs propagation
-						if (isset($newData['etag'])) {
-							$data['etag_changed'] = true;
+						if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) {
+							$data['etag'] = $etag;
 						}
-					} else {
-						// we only updated unencrypted_size if it's already set
-						unset($data['unencrypted_size']);
-						$newData = $data;
-						$fileId = -1;
-					}
-					if (!empty($newData)) {
-						// Reset the checksum if the data has changed
-						$newData['checksum'] = '';
-						$newData['parent'] = $parentId;
-						$data['fileid'] = $this->addToCache($file, $newData, $fileId);
 					}
 
-					$data['oldSize'] = ($cacheData && isset($cacheData['size'])) ? $cacheData['size'] : 0;
-
-					if ($cacheData && isset($cacheData['encrypted'])) {
-						$data['encrypted'] = $cacheData['encrypted'];
+					// we only updated unencrypted_size if it's already set
+					if (isset($cacheData['unencrypted_size']) && $cacheData['unencrypted_size'] === 0) {
+						unset($data['unencrypted_size']);
 					}
 
-					// post-emit only if it was a file. By that we avoid counting/treating folders as files
-					if ($data['mimetype'] !== 'httpd/unix-directory') {
-						$this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]);
-						\OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]);
+					/**
+					 * Only update metadata that has changed.
+					 * i.e. get all the values in $data that are not present in the cache already
+					 *
+					 * We need the OC implementation for usage of "getData" method below.
+				 	 * @var \OC\Files\Cache\CacheEntry $cacheData
+					 */
+					$newData = $this->array_diff_assoc_multi($data, $cacheData->getData());
+					
+					// make it known to the caller that etag has been changed and needs propagation
+					if (isset($newData['etag'])) {
+						$data['etag_changed'] = true;
 					}
 				} else {
-					$this->removeFromCache($file);
+					unset($data['unencrypted_size']);
+					$newData = $data;
+					$fileId = -1;
 				}
-			} catch (\Exception $e) {
-				if ($lock) {
-					if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-						$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					}
+				if (!empty($newData)) {
+					// Reset the checksum if the data has changed
+					$newData['checksum'] = '';
+					$newData['parent'] = $parentId;
+					$data['fileid'] = $this->addToCache($file, $newData, $fileId);
+				}
+
+				if ($cacheData !== false) {
+					$data['oldSize'] = $cacheData['size'] ?? 0;
+					$data['encrypted'] = $cacheData['encrypted'] ?? false;
 				}
-				throw $e;
-			}
 
-			// release the acquired lock
+				// post-emit only if it was a file. By that we avoid counting/treating folders as files
+				if ($data['mimetype'] !== 'httpd/unix-directory') {
+					$this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]);
+					\OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]);
+				}
+			}
+		} catch (\Exception $e) {
 			if ($lock) {
 				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
 					$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 				}
 			}
+			throw $e;
+		}
 
-			if ($data && !isset($data['encrypted'])) {
-				$data['encrypted'] = false;
+		// release the acquired lock
+		if ($lock) {
+			if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 			}
-			return $data;
 		}
 
-		return null;
+		return $data;
 	}
 
 	protected function removeFromCache($path) {
@@ -319,29 +319,26 @@ class Scanner extends BasicEmitter implements IScanner {
 		if ($reuse === -1) {
 			$reuse = ($recursive === self::SCAN_SHALLOW) ? self::REUSE_ETAG | self::REUSE_SIZE : self::REUSE_ETAG;
 		}
-		if ($lock) {
-			if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-				$this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
-				$this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-			}
+
+		if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) {
+			$this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
+			$this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 		}
+
 		try {
-			try {
-				$data = $this->scanFile($path, $reuse, -1, null, $lock);
-				if ($data && $data['mimetype'] === 'httpd/unix-directory') {
-					$size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']);
-					$data['size'] = $size;
-				}
-			} catch (NotFoundException $e) {
-				$this->removeFromCache($path);
-				return null;
+			$data = $this->scanFile($path, $reuse, -1, null, $lock);
+
+			if ($data !== null && $data['mimetype'] === 'httpd/unix-directory') {
+				$size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']);
+				$data['size'] = $size;
 			}
+		} catch (NotFoundException $e) {
+			$this->removeFromCache($path);
+			return null;
 		} finally {
-			if ($lock) {
-				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-					$this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					$this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
-				}
+			if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+				$this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
 			}
 		}
 		return $data;
@@ -395,9 +392,9 @@ class Scanner extends BasicEmitter implements IScanner {
 	 * Get the children currently in the cache
 	 *
 	 * @param int $folderId
-	 * @return array[]
+	 * @return array<string, \OCP\Files\Cache\ICacheEntry>
 	 */
-	protected function getExistingChildren($folderId) {
+	protected function getExistingChildren($folderId): array {
 		$existingChildren = [];
 		$children = $this->cache->getFolderContentsById($folderId);
 		foreach ($children as $child) {
diff --git a/tests/lib/Files/Storage/Wrapper/EncodingTest.php b/tests/lib/Files/Storage/Wrapper/EncodingTest.php
index f52e3689155..d8b03a891c2 100644
--- a/tests/lib/Files/Storage/Wrapper/EncodingTest.php
+++ b/tests/lib/Files/Storage/Wrapper/EncodingTest.php
@@ -240,4 +240,12 @@ class EncodingTest extends \Test\Files\Storage\Storage {
 		$entry = $this->instance->getMetaData('/test/' . self::NFD_NAME);
 		$this->assertEquals(self::NFC_NAME, $entry['name']);
 	}
+
+	/**
+	 * Regression test of https://github.com/nextcloud/server/issues/50431
+	 */
+	public function testNoMetadata() {
+		$this->assertNull($this->instance->getMetaData('/test/null'));
+	}
+
 }