fix(integrity): clear stale per app results and use the local cache tier

DeepDiver1975 · claude · DeepDiver1975 · commit e37ca833095a · 2026-07-27T16:32:14.000+02:00
storeResults() writes one cache entry per checked scope in addition to the entry
holding the combined results, but cleanResults() only removed the latter. The per
app entries had no TTL either, so a verdict about an app was cached forever and
kept being served through getVerifiedAppsFromCache() even after the app had been
repaired or replaced. cleanResults() now clears the whole prefix - ICache::clear()
is prefix scoped in every backend - and the entries expire.

The results describe the files on disk of the host that produced them, so they
also move to the host local cache tier. getResults() already falls back to
appconfig, so a check run through occ stays visible to the web requests.

Co-Authored-By: Claude Opus 5 &lt;noreply@anthropic.com&gt;
Signed-off-by: Thomas Müller &lt;1005065+DeepDiver1975@users.noreply.github.com&gt;
diff --git a/changelog/unreleased/41735 b/changelog/unreleased/41735
@@ -0,0 +1,11 @@
+Bugfix: Clear stale integrity check results when rescanning
+
+The code integrity checker stores one cache entry per checked scope, but a
+rescan only removed the entry holding the combined results. The per app entries
+had no expiry either, so a verdict about an app was cached indefinitely and was
+served even after the app had been repaired or replaced. Rescanning now clears
+all of them, the entries expire, and the results are kept in the host local
+cache tier - they describe the files on disk of one host and are of no use to
+another.
+
+https://github.com/owncloud/core/pull/41735
diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php
@@ -55,6 +55,14 @@
  */
 class Checker implements OnDiskHasher {
 	public const CACHE_KEY = 'oc.integritycheck.checker';
+
+	/**
+	 * The results describe the files on disk of this instance, so cached entries
+	 * have to expire for a node that never runs a check itself to notice a
+	 * repaired or replaced installation.
+	 */
+	public const CACHE_TTL = 24 * 3600;
+
 	/** @var EnvironmentHelper */
 	private $environmentHelper;
 	/** @var AppLocator */
@@ -104,7 +112,9 @@ public function __construct(
 		$this->fileAccessHelper = $fileAccessHelper;
 		$this->appLocator = $appLocator;
 		$this->config = $config;
-		$this->cache = $cacheFactory ? $cacheFactory->create(self::CACHE_KEY) : new \OC\Memcache\NullCache();
+		$this->cache = $cacheFactory
+			? \OC\Memcache\LocalCacheFactory::create($cacheFactory, self::CACHE_KEY)
+			: new \OC\Memcache\NullCache();
 		$this->appManager = $appManager;
 		$this->tempManager = $tempManager;
 		$this->verifier = $verifier;
@@ -400,17 +410,21 @@ private function storeResults($scope, array $result) {
 
 		$this->setAppValue(self::CACHE_KEY, \json_encode($resultArray));
 		//Set cache for each app
-		$this->cache->set($scope, \json_encode($resultArray));
-		$this->cache->set(self::CACHE_KEY, \json_encode($resultArray));
+		$this->cache->set($scope, \json_encode($resultArray), self::CACHE_TTL);
+		$this->cache->set(self::CACHE_KEY, \json_encode($resultArray), self::CACHE_TTL);
 	}
 
 	/**
+	 * Clean previous results for a proper rescanning. Otherwise a stale verdict
+	 * would be served instead of the one the rescan is about to produce.
 	 *
-	 * Clean previous results for a proper rescanning. Otherwise
+	 * storeResults() writes one entry per scope in addition to CACHE_KEY, so the
+	 * whole prefix is cleared - removing CACHE_KEY alone left every per app entry
+	 * behind.
 	 */
 	private function cleanResults() {
 		$this->deleteAppValue(self::CACHE_KEY);
-		$this->cache->remove(self::CACHE_KEY);
+		$this->cache->clear();
 	}
 
 	/**
diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php
@@ -27,11 +27,12 @@
 use OC\IntegrityCheck\Helpers\FileAccessHelper;
 use OC\IntegrityCheck\Verifier\Verifier;
 use OC\IntegrityCheck\Verifier\VerificationResult;
+use OC\Memcache\ArrayCache;
 use OC\Memcache\NullCache;
 use OC\Memcache\Redis;
 use OCP\App\IAppManager;
-use OCP\ICacheFactory;
 use OCP\IConfig;
+use Test\Memcache\FixedCacheFactory;
 use Test\TestCase;
 
 /**
@@ -48,7 +49,7 @@ class CheckerTest extends TestCase {
 	private $fileAccessHelper;
 	/** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */
 	private $config;
-	/** @var ICacheFactory | \PHPUnit\Framework\MockObject\MockObject */
+	/** @var FixedCacheFactory */
 	private $cacheFactory;
 	/** @var IAppManager | \PHPUnit\Framework\MockObject\MockObject */
 	private $appManager;
@@ -61,7 +62,7 @@ public function setUp(): void {
 		$this->fileAccessHelper = $this->createMock(FileAccessHelper::class);
 		$this->appLocator = $this->createMock(AppLocator::class);
 		$this->config = $this->createMock(IConfig::class);
-		$this->cacheFactory = $this->createMock(ICacheFactory::class);
+		$this->cacheFactory = new FixedCacheFactory(new NullCache());
 		$this->appManager = $this->createMock(IAppManager::class);
 		$this->verifier = $this->createMock(Verifier::class);
 
@@ -87,12 +88,6 @@ public function setUp(): void {
 			->method('getAllApps')
 			->willReturn([]);
 
-		$this->cacheFactory
-			->expects($this->any())
-			->method('create')
-			->with('oc.integritycheck.checker')
-			->willReturn(new NullCache());
-
 		$this->checker = new Checker(
 			$this->environmentHelper,
 			$this->fileAccessHelper,
@@ -103,6 +98,69 @@ public function setUp(): void {
 			\OC::$server->getTempManager(),
 			$this->verifier
 		);
+
+		$this->assertSame([Checker::CACHE_KEY], $this->cacheFactory->getRequestedPrefixes());
+	}
+
+	/**
+	 * The results describe the files on disk of this host, so they belong in the
+	 * host local cache tier - not in a distributed one where another host could
+	 * hand back a verdict about an installation it cannot see.
+	 */
+	public function testUsesTheLocalCacheTier() {
+		$cacheFactory = $this->createMock(FixedCacheFactory::class);
+		$cacheFactory->expects($this->once())
+			->method('createLocal')
+			->with(Checker::CACHE_KEY)
+			->willReturn(new NullCache());
+		$cacheFactory->expects($this->never())->method('createDistributed');
+		$cacheFactory->expects($this->never())->method('create');
+
+		new Checker(
+			$this->environmentHelper,
+			$this->fileAccessHelper,
+			$this->appLocator,
+			$this->config,
+			$cacheFactory,
+			$this->appManager,
+			\OC::$server->getTempManager(),
+			$this->verifier
+		);
+	}
+
+	/**
+	 * storeResults() writes one entry per scope next to CACHE_KEY, and a rescan
+	 * has to invalidate all of them - removing CACHE_KEY alone left every per app
+	 * verdict cached forever.
+	 */
+	public function testRescanningDropsThePerAppResults() {
+		$cache = new ArrayCache();
+		$checker = new Checker(
+			$this->environmentHelper,
+			$this->fileAccessHelper,
+			$this->appLocator,
+			$this->config,
+			new FixedCacheFactory($cache),
+			$this->appManager,
+			\OC::$server->getTempManager(),
+			$this->verifier
+		);
+
+		$this->environmentHelper->method('getChannel')->willReturn('stable');
+		$this->environmentHelper->method('getServerRoot')->willReturn(\OC::$SERVERROOT);
+		$this->verifier->method('verify')->willReturn(VerificationResult::passed());
+
+		$cache->set('SomeApp', '{"SomeApp":[]}');
+		$cache->set('SomeOtherApp', '{"SomeOtherApp":[]}');
+		$cache->set(Checker::CACHE_KEY, '{"SomeApp":[]}');
+
+		$checker->runInstanceVerification();
+
+		// only the results of this run are left - the verification passed, so
+		// there is nothing to report
+		$this->assertNull($cache->get('SomeApp'));
+		$this->assertNull($cache->get('SomeOtherApp'));
+		$this->assertSame('[]', $cache->get(Checker::CACHE_KEY));
 	}
 
 	public function testIgnoredAppSignatureWithoutSignatureData() {
@@ -630,12 +688,7 @@ public function testVerifyCachedAppSignatureCheck() {
 		$redisObj->method('get')
 			->with('SomeApp')
 			->willReturn('[]');
-		$cacheFactory = $this->createMock(ICacheFactory::class);
-		$cacheFactory
-			->expects($this->any())
-			->method('create')
-			->with('oc.integritycheck.checker')
-			->will($this->returnValue($redisObj));
+		$cacheFactory = new FixedCacheFactory($redisObj);
 		$checker = new Checker(
 			$this->environmentHelper,
 			$this->fileAccessHelper,
@@ -654,12 +707,7 @@ public function testAppNotCachedSignatureCheck() {
 		$redisObj->method('get')
 			->with('SomeApp')
 			->willReturn(null);
-		$cacheFactory = $this->createMock(ICacheFactory::class);
-		$cacheFactory
-			->expects($this->any())
-			->method('create')
-			->with('oc.integritycheck.checker')
-			->will($this->returnValue($redisObj));
+		$cacheFactory = new FixedCacheFactory($redisObj);
 		$checker = new Checker(
 			$this->environmentHelper,
 			$this->fileAccessHelper,
@@ -702,10 +750,7 @@ public function testHasPassedCheckWithExceptionResult() {
 				]
 			]));
 
-		$cacheFactory = $this->createMock(ICacheFactory::class);
-		$cacheFactory->expects($this->any())
-			->method('create')
-			->willReturn(new NullCache());
+		$cacheFactory = new FixedCacheFactory(new NullCache());
 
 		$checker = new Checker(
 			$this->environmentHelper,
@@ -732,10 +777,7 @@ public function testHasPassedCheckWithEmptyResults() {
 			->with('core', 'oc.integritycheck.checker', '{}')
 			->willReturn('{}');
 
-		$cacheFactory = $this->createMock(ICacheFactory::class);
-		$cacheFactory->expects($this->any())
-			->method('create')
-			->willReturn(new NullCache());
+		$cacheFactory = new FixedCacheFactory(new NullCache());
 
 		$checker = new Checker(
 			$this->environmentHelper,
@@ -766,10 +808,7 @@ public function testHasPassedCheckWithFileMissing() {
 				]
 			]));
 
-		$cacheFactory = $this->createMock(ICacheFactory::class);
-		$cacheFactory->expects($this->any())
-			->method('create')
-			->willReturn(new NullCache());
+		$cacheFactory = new FixedCacheFactory(new NullCache());
 
 		$checker = new Checker(
 			$this->environmentHelper,
