From 9f6a49eef590eaf27d2b9f05a4bd364ce5f2937d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:54:10 +0200 Subject: [PATCH] fix(integrity): clear stale per app results and use the local cache tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41735 | 11 +++ lib/private/IntegrityCheck/Checker.php | 24 ++++-- tests/lib/IntegrityCheck/CheckerTest.php | 105 ++++++++++++++++------- 3 files changed, 102 insertions(+), 38 deletions(-) create mode 100644 changelog/unreleased/41735 diff --git a/changelog/unreleased/41735 b/changelog/unreleased/41735 new file mode 100644 index 00000000000..599c250e4d1 --- /dev/null +++ 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 index edd9a3b018c..143fe54c360 100644 --- 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 index 8cc07152f47..3a28796c8e9 100644 --- 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,