From 34651cc44c491ebf559741d9b641cc55bec508fe Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 10 Aug 2026 16:28:01 -0400 Subject: [PATCH 1/8] feat(appstore): use bounded stale cache when refresh fails Fall back to stale but otherwise valid, same-version App Store cache data when a refresh fails, provided it is no older than seven days. Continue accepting valid empty responses as fresh data while rejecting missing, incompatible, or overly stale cache entries. Improves the user experience by preserving access to App Store metadata during temporary retrieval failures, allowing users to continue browsing the App Store. Signed-off-by: Josh --- lib/private/App/AppStore/Fetcher/Fetcher.php | 59 ++++++++++++++++++-- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index 7aec5f3251efd..d1dbd595babf7 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -32,6 +32,7 @@ abstract class Fetcher { public const INVALIDATE_AFTER_SECONDS = 3600; public const INVALIDATE_AFTER_SECONDS_UNSTABLE = 900; public const RETRY_AFTER_FAILURE_SECONDS = 300; + public const MAX_STALE_SECONDS = 604800; // 7 days public const APP_STORE_URL = 'https://apps.nextcloud.com/api/v1'; /** @var IAppData */ @@ -142,6 +143,35 @@ public function get($allowUnstable = false): array { $ETag = ''; $content = ''; + $sameVersionCachedData = null; + $sameVersionCacheTimestamp = null; + + $useCachedData = function () use (&$sameVersionCachedData, &$sameVersionCacheTimestamp): array { + $now = $this->timeFactory->getTime(); + + if ($sameVersionCachedData === null || $sameVersionCacheTimestamp === null) { + return []; + } + + if ($sameVersionCacheTimestamp >= ($now - self::MAX_STALE_SECONDS)) { + $this->logger->warning( + 'Could not refresh appstore cache, using stale data', + ['app' => 'appstoreFetcher'] + ); + + return $sameVersionCachedData; + } + + $this->logger->warning( + 'Could not refresh appstore cache and cached data is too old', + [ + 'app' => 'appstoreFetcher', + 'cacheAge' => $now - $sameVersionCacheTimestamp, + ] + ); + + return []; + }; try { // File does already exists @@ -151,6 +181,14 @@ public function get($allowUnstable = false): array { if (is_array($jsonBlob)) { // No caching when the version has been updated if (isset($jsonBlob['ncversion']) && $jsonBlob['ncversion'] === $this->getVersion()) { + if (isset($jsonBlob['data']) && is_array($jsonBlob['data'])) { + $sameVersionCachedData = $jsonBlob['data']; + } + + if (isset($jsonBlob['timestamp']) && is_numeric($jsonBlob['timestamp'])) { + $sameVersionCacheTimestamp = (int)$jsonBlob['timestamp']; + } + // If the timestamp is older than 3600 seconds request the files new $invalidateAfterSeconds = self::INVALIDATE_AFTER_SECONDS; @@ -158,8 +196,13 @@ public function get($allowUnstable = false): array { $invalidateAfterSeconds = self::INVALIDATE_AFTER_SECONDS_UNSTABLE; } - if ((int)$jsonBlob['timestamp'] > ($this->timeFactory->getTime() - $invalidateAfterSeconds)) { - return $jsonBlob['data']; + if ( + $sameVersionCachedData !== null + && $sameVersionCacheTimestamp !== null + && $sameVersionCacheTimestamp > ($this->timeFactory->getTime() - $invalidateAfterSeconds) + ) { + $this->logger->debug('Using still fresh appstore cache file', ['app' => 'appstoreFetcher']); + return $sameVersionCachedData; } if (isset($jsonBlob['ETag'])) { @@ -186,21 +229,25 @@ public function get($allowUnstable = false): array { try { $responseJson = $this->fetch($ETag, $content, $allowUnstable); - if (empty($responseJson) || empty($responseJson['data'])) { - return []; + // On refresh failure, fallback to the stale but otherwise valid, + // same-version cached data, provided it is no older than + // MAX_STALE_SECONDS. An empty data array is valid and must be + // written to the cache. + if (!isset($responseJson['data']) || !is_array($responseJson['data'])) { + return $useCachedData(); } $file->putContent(json_encode($responseJson)); return json_decode($file->getContent(), true)['data']; } catch (ConnectException $e) { $this->logger->warning('Could not connect to appstore: ' . $e->getMessage(), ['app' => 'appstoreFetcher']); - return []; + return $useCachedData(); } catch (\Exception $e) { $this->logger->warning($e->getMessage(), [ 'exception' => $e, 'app' => 'appstoreFetcher', ]); - return []; + return $useCachedData(); } } From 2ca48f2bb203056dec8760a8d18f2443f4da8c3f Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 10 Aug 2026 16:34:32 -0400 Subject: [PATCH 2/8] perf(AppStore): avoid unnecessary data load+decode in fetchers Signed-off-by: Josh --- lib/private/App/AppStore/Fetcher/Fetcher.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index d1dbd595babf7..19bd65952ac21 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -238,7 +238,7 @@ public function get($allowUnstable = false): array { } $file->putContent(json_encode($responseJson)); - return json_decode($file->getContent(), true)['data']; + return $responseJson['data']; } catch (ConnectException $e) { $this->logger->warning('Could not connect to appstore: ' . $e->getMessage(), ['app' => 'appstoreFetcher']); return $useCachedData(); From 683fa0d0e2569f07b948440e3276dace5e14e635 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 10 Aug 2026 21:53:30 -0400 Subject: [PATCH 3/8] test(appstore): cover bounded stale cache fallback Verify that same-version cached data is used after refresh failures while within the seven-day stale limit, rejected after the limit, and returned during the recent-failure cooldown. Also verify that valid empty refresh responses replace stale data. Signed-off-by: Josh --- .../lib/App/AppStore/Fetcher/FetcherBase.php | 332 +++++++++++++++++- 1 file changed, 329 insertions(+), 3 deletions(-) diff --git a/tests/lib/App/AppStore/Fetcher/FetcherBase.php b/tests/lib/App/AppStore/Fetcher/FetcherBase.php index 3a0d30a32653a..52e98cb48c36f 100644 --- a/tests/lib/App/AppStore/Fetcher/FetcherBase.php +++ b/tests/lib/App/AppStore/Fetcher/FetcherBase.php @@ -392,9 +392,22 @@ public function testGetWithAlreadyExistingFileAndOutdatedVersion(): void { public function testGetWithExceptionInClient(): void { $this->config->method('getSystemValueString') - ->willReturnArgument(1); + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + $this->config->method('getSystemValueBool') - ->willReturnArgument(1); + ->willReturnArgument(1); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['settings', 'appstore-fetcher-lastFailure', '0', '0'], + ['settings', 'appstore-timeout', '120', '120'], + ]); $folder = $this->createMock(ISimpleFolder::class); $file = $this->createMock(ISimpleFile::class); @@ -411,7 +424,21 @@ public function testGetWithExceptionInClient(): void { $file ->expects($this->once()) ->method('getContent') - ->willReturn('{"timestamp":1200,"data":{"MyApp":{"id":"MyApp"}}}'); + ->willReturn(json_encode([ + 'timestamp' => 1200, + 'data' => [ + ['id' => 'MyApp'], + ], + 'ncversion' => '11.0.0.2', + ])); + + // First call checks whether the cache is fresh; the second call is + // made by the stale-cache fallback closure. + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturnOnConsecutiveCalls(4801, 4801); + $client = $this->createMock(IClient::class); $this->clientService ->expects($this->once()) @@ -423,6 +450,305 @@ public function testGetWithExceptionInClient(): void { ->with($this->endpoint) ->willThrowException(new \Exception()); + $this->assertSame([['id' => 'MyApp']], $this->fetcher->get()); + } + + public function testGetUsesStaleCacheWithinMaximumStaleAge(): void { + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['settings', 'appstore-fetcher-lastFailure', '0', '0'], + ['settings', 'appstore-timeout', '120', '120'], + ]); + + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + + $file + ->expects($this->once()) + ->method('getContent') + ->willReturn(json_encode([ + 'timestamp' => 1000, + 'data' => [ + ['id' => 'MyApp'], + ], + 'ncversion' => '11.0.0.2', + ])); + + $now = 1000 + Fetcher::MAX_STALE_SECONDS - 1; + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturnOnConsecutiveCalls($now, $now); + + $client = $this->createMock(IClient::class); + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->willReturn($client); + + $client + ->expects($this->once()) + ->method('get') + ->with($this->endpoint, [ + 'timeout' => 120, + ]) + ->willThrowException(new \Exception('temporary failure')); + + $this->assertSame([['id' => 'MyApp']], $this->fetcher->get()); + } + + public function testGetDoesNotUseStaleCacheOlderThanMaximumStaleAge(): void { + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['settings', 'appstore-fetcher-lastFailure', '0', '0'], + ['settings', 'appstore-timeout', '120', '120'], + ]); + + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + + $file + ->expects($this->once()) + ->method('getContent') + ->willReturn(json_encode([ + 'timestamp' => 1000, + 'data' => [ + ['id' => 'MyApp'], + ], + 'ncversion' => '11.0.0.2', + ])); + + $now = 1000 + Fetcher::MAX_STALE_SECONDS + 1; + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturnOnConsecutiveCalls($now, $now); + + $client = $this->createMock(IClient::class); + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->willReturn($client); + + $client + ->expects($this->once()) + ->method('get') + ->with($this->endpoint, [ + 'timeout' => 120, + ]) + ->willThrowException(new \Exception('temporary failure')); + + $this->assertSame([], $this->fetcher->get()); + } + + public function testGetUsesStaleCacheDuringFailureCooldown(): void { + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config->method('getAppValue') + ->willReturnCallback(function (string $app, string $key, string $default): string { + if ($key === 'appstore-fetcher-lastFailure') { + return (string)time(); + } + + return $default; + }); + + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + + $file + ->expects($this->once()) + ->method('getContent') + ->willReturn(json_encode([ + 'timestamp' => 1000, + 'data' => [ + ['id' => 'MyApp'], + ], + 'ncversion' => '11.0.0.2', + ])); + + $now = 1000 + Fetcher::MAX_STALE_SECONDS - 1; + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturnOnConsecutiveCalls($now, $now); + + $this->clientService + ->expects($this->never()) + ->method('newClient'); + + $this->assertSame([['id' => 'MyApp']], $this->fetcher->get()); + } + + public function testGetAcceptsValidEmptyRefreshResponse(): void { + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['settings', 'appstore-fetcher-lastFailure', '0', '0'], + ['settings', 'appstore-timeout', '120', '120'], + ]); + + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + + $oldData = json_encode([ + 'timestamp' => 1000, + 'data' => [ + ['id' => 'MyApp'], + ], + 'ncversion' => '11.0.0.2', + ]); + + $newData = json_encode([ + 'timestamp' => 2000, + 'data' => [], + 'ncversion' => '11.0.0.2', + 'ETag' => '"newETag"', + ]); + + $file + ->expects($this->exactly(2)) + ->method('getContent') + ->willReturnOnConsecutiveCalls($oldData, $newData); + + $file + ->expects($this->once()) + ->method('putContent') + ->with($newData); + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturnOnConsecutiveCalls(4801, 2000); + + $client = $this->createMock(IClient::class); + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->willReturn($client); + + $response = $this->createMock(IResponse::class); + + $client + ->expects($this->once()) + ->method('get') + ->with($this->endpoint, [ + 'timeout' => 120, + ]) + ->willReturn($response); + + $response + ->expects($this->once()) + ->method('getStatusCode') + ->willReturn(200); + + $response + ->expects($this->once()) + ->method('getBody') + ->willReturn('[]'); + + $response + ->expects($this->once()) + ->method('getHeader') + ->with('ETag') + ->willReturn('"newETag"'); + $this->assertSame([], $this->fetcher->get()); } From dbe50eaad4548591537d7bff23864629f87e68de Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 10 Aug 2026 21:57:13 -0400 Subject: [PATCH 4/8] test(appstore): cover stale cache filtering in AppFetcher Verify that stale app data still respects the App Store allowlist. Signed-off-by: Josh --- .../App/AppStore/Fetcher/AppFetcherTest.php | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php index c212356125082..8e1485113b035 100644 --- a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php +++ b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php @@ -2249,4 +2249,92 @@ public function testGetAppsAllowlistCustomAppstore(): void { $this->assertEquals(count($apps), 1); $this->assertEquals($apps[0]['id'], 'contacts'); } + + public function testGetAppliesAllowlistToStaleCachedData(): void { + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config + ->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config + ->method('getSystemValue') + ->willReturnCallback(function (string $key, mixed $default = null): mixed { + if ($key === 'appsallowlist') { + return ['allowed_app']; + } + + return $default; + }); + + $this->config + ->method('getAppValue') + ->willReturnCallback(function (string $app, string $key, string $default): string { + if ($key === 'appstore-fetcher-lastFailure') { + return (string)time(); + } + + return $default; + }); + + $file = $this->createMock(ISimpleFile::class); + $folder = $this->createMock(ISimpleFolder::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with('apps.json') + ->willReturn($file); + + $now = time(); + + $file + ->expects($this->once()) + ->method('getContent') + ->willReturn(json_encode([ + 'timestamp' => $now - 3601, + 'data' => [ + [ + 'id' => 'allowed_app', + ], + [ + 'id' => 'blocked_app', + ], + ], + 'ncversion' => '11.0.0.2', + ])); + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturn($now); + + $this->clientService + ->expects($this->never()) + ->method('newClient'); + + $this->registry + ->expects($this->once()) + ->method('delegateHasValidSubscription') + ->willReturn(true); + + $this->assertSame([ + [ + 'id' => 'allowed_app', + ], + ], $this->fetcher->get()); + } } From a66dfa47fa3bf2497811955776a95896773ac991 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 10 Aug 2026 21:59:20 -0400 Subject: [PATCH 5/8] test(appstore): cover stale cache filtering in AppDiscoverFetcher Verify that stale discover data still removes expired entries after the base fetcher falls back from a failed refresh. Assisted-by: Copilot:gpt-5.6-luna Signed-off-by: Josh --- .../Fetcher/AppDiscoverFetcherTest.php | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/lib/App/AppStore/Fetcher/AppDiscoverFetcherTest.php b/tests/lib/App/AppStore/Fetcher/AppDiscoverFetcherTest.php index 49314d5128357..afae8445784e7 100644 --- a/tests/lib/App/AppStore/Fetcher/AppDiscoverFetcherTest.php +++ b/tests/lib/App/AppStore/Fetcher/AppDiscoverFetcherTest.php @@ -115,4 +115,84 @@ public static function dataGetETag(): array { 'numeric etag' => ['132', false, '{ "ETag": 132 }'], ]; } + + public function testGetFiltersExpiredEntriesFromStaleCachedData(): void { + $this->config + ->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default): string { + if ($key === 'version') { + return '11.0.0.2'; + } + + return $default; + }); + + $this->config + ->method('getSystemValueBool') + ->willReturnArgument(1); + + $this->config + ->method('getAppValue') + ->willReturnCallback(function (string $app, string $key, string $default): string { + if ($key === 'appstore-fetcher-lastFailure') { + return (string)time(); + } + + return $default; + }); + + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + + $folder + ->expects($this->once()) + ->method('getFile') + ->with('discover.json') + ->willReturn($file); + + $now = time(); + + $file + ->expects($this->once()) + ->method('getContent') + ->willReturn(json_encode([ + 'timestamp' => $now - 3601, + 'data' => [ + [ + 'type' => 'post', + 'id' => 'active-entry', + 'expiryDate' => date('c', $now + 3600), + ], + [ + 'type' => 'post', + 'id' => 'expired-entry', + 'expiryDate' => date('c', $now - 3600), + ], + ], + 'ncversion' => '11.0.0.2', + ])); + + $this->timeFactory + ->expects($this->exactly(2)) + ->method('getTime') + ->willReturn($now); + + $this->clientService + ->expects($this->never()) + ->method('newClient'); + + $this->assertSame([ + [ + 'type' => 'post', + 'id' => 'active-entry', + 'expiryDate' => date('c', $now + 3600), + ], + ], $this->fetcher->get()); + } } From 4d8c93c86da8fe7098054f12c96d1b09eacd7194 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 11 Aug 2026 11:50:49 -0400 Subject: [PATCH 6/8] fix(AppStore): resolve Psalm errors in Fetcher and harden cache fallback Replace the by-reference `$useCachedData` closure with a typed private `useCachedData()` method, fixing Psalm's inferred-type errors around `get()`'s `list` return type and slightly improving readability. Validate that decoded JSON is a list (not merely an array) at every point where untrusted data enters the contract: App Store responses, 304 conditional-request replays, and local cache files. Guard cache re-encoding and writes against failure without discarding valid fresh data, and expand the docblocks to document fallback behavior. Also refactor formatting and guard logic, including ordering and cross-method alignment, for clarity. Signed-off-by: Josh --- lib/private/App/AppStore/Fetcher/Fetcher.php | 252 +++++++++++++------ 1 file changed, 170 insertions(+), 82 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index 19bd65952ac21..21996f8d502db 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -32,20 +32,17 @@ abstract class Fetcher { public const INVALIDATE_AFTER_SECONDS = 3600; public const INVALIDATE_AFTER_SECONDS_UNSTABLE = 900; public const RETRY_AFTER_FAILURE_SECONDS = 300; - public const MAX_STALE_SECONDS = 604800; // 7 days + /** + * Maximum age of same-version cache data eligible for refresh failure fallback. + */ + public const MAX_STALE_SECONDS = 7 * 24 * 60 * 60; public const APP_STORE_URL = 'https://apps.nextcloud.com/api/v1'; - /** @var IAppData */ - protected $appData; - - /** @var string */ - protected $fileName; - /** @var string */ - protected $endpointName; - /** @var ?string */ - protected $version = null; - /** @var ?string */ - protected $channel = null; + protected IAppData $appData; + protected string $fileName; + protected string $endpointName; + protected ?string $version = null; + protected ?string $channel = null; public function __construct( Factory $appDataFactory, @@ -59,21 +56,32 @@ public function __construct( } /** - * Fetches the response from the server + * Fetches and validates the response from the App Store server. * - * @param string $ETag - The ETag of the cached response - * @param string $content - The content of the response - * @param bool $allowUnstable - Allow unstable releases + * A successful response contains a list of App Store entries and cache + * metadata. A suppressed, failed, or invalid refresh returns an empty + * array, allowing get() to consider an eligible stale cache. * - * @return array{data: list, ETag?: string, timestamp: int, ncversion: string}|array + * @param string $ETag The ETag of the cached response, if available. + * @param string $content The serialized cached response data used for a + * 304 Not Modified response. + * @param bool $allowUnstable Whether unstable releases should be requested. + * + * @return array{ + * data: list, + * ETag?: string, + * timestamp: int, + * ncversion: string + * }|array */ - protected function fetch($ETag, $content, $allowUnstable = false): array { + protected function fetch(string $ETag, string $content, bool $allowUnstable = false): array { $appstoreEnabled = $this->config->getSystemValueBool('appstoreenabled', true); - if ((int)$this->config->getAppValue('settings', 'appstore-fetcher-lastFailure', '0') > time() - self::RETRY_AFTER_FAILURE_SECONDS) { + if (!$appstoreEnabled) { return []; } - if (!$appstoreEnabled) { + $lastFailure = (int)$this->config->getAppValue('settings', 'appstore-fetcher-lastFailure', '0'); + if ($lastFailure > (time() - self::RETRY_AFTER_FAILURE_SECONDS)) { return []; } @@ -87,10 +95,11 @@ protected function fetch($ETag, $content, $allowUnstable = false): array { ]; } - if ($this->config->getSystemValueString('appstoreurl', self::APP_STORE_URL) === self::APP_STORE_URL) { - // If we have a valid subscription key, send it to the appstore + $appStoreUrl = $this->config->getSystemValueString('appstoreurl', self::APP_STORE_URL); + if ($appStoreUrl === self::APP_STORE_URL && $this->registry->delegateHasValidSubscription()) { $subscriptionKey = $this->config->getAppValue('support', 'subscription_key'); - if ($this->registry->delegateHasValidSubscription() && $subscriptionKey) { + + if ($subscriptionKey) { $options['headers'] ??= []; $options['headers']['X-NC-Subscription-Key'] = $subscriptionKey; } @@ -107,11 +116,25 @@ protected function fetch($ETag, $content, $allowUnstable = false): array { $responseJson = []; if ($response->getStatusCode() === Http::STATUS_NOT_MODIFIED) { - $responseJson['data'] = json_decode($content, true); + // Reuse the locally cached data after the server confirms the ETag is unchanged. + $decoded = json_decode($content, true); + if (!is_array($decoded) || !array_is_list($decoded)) { + return []; + } + + /** @var list $decoded */ + $responseJson['data'] = $decoded; } else { - $responseJson['data'] = json_decode($response->getBody(), true); + $decoded = json_decode($response->getBody(), true); + if (!is_array($decoded) || !array_is_list($decoded)) { + return []; + } + + /** @var list $decoded */ + $responseJson['data'] = $decoded; $ETag = $response->getHeader('ETag'); } + $this->config->deleteAppValue('settings', 'appstore-fetcher-lastFailure'); $responseJson['timestamp'] = $this->timeFactory->getTime(); @@ -124,65 +147,63 @@ protected function fetch($ETag, $content, $allowUnstable = false): array { } /** - * Returns the array with the entries on the appstore server + * Returns App Store entries, using the cache when appropriate. + * + * Fresh, same-version cache data is returned immediately. When refreshing + * stale cache data fails, valid same-version data may be used as a + * fallback while it is no older than MAX_STALE_SECONDS. * - * @param bool $allowUnstable - Allow unstable releases + * Cache data from another Nextcloud version, missing or invalid cache + * data, and cache data older than MAX_STALE_SECONDS are not used as + * fallbacks. + * + * A valid empty response from the App Store is returned and written to + * the cache as an empty list; invalid responses are treated as refresh + * failures. + * + * @param bool $allowUnstable Whether unstable releases should be included * @return list */ - public function get($allowUnstable = false): array { + public function get(bool $allowUnstable = false): array { $appstoreEnabled = $this->config->getSystemValueBool('appstoreenabled', true); - $internetAvailable = $this->config->getSystemValueBool('has_internet_connection', true); - $isDefaultAppStore = $this->config->getSystemValueString('appstoreurl', self::APP_STORE_URL) === self::APP_STORE_URL; - - if (!$appstoreEnabled || (!$internetAvailable && $isDefaultAppStore)) { - $this->logger->info('AppStore is disabled or this instance has no Internet connection to access the default app store', ['app' => 'appstoreFetcher']); + if (!$appstoreEnabled) { + $this->logger->info('The appstore is disabled', ['app' => 'appstoreFetcher']); return []; } - $rootFolder = $this->appData->getFolder('/'); + $internetAvailable = $this->config->getSystemValueBool('has_internet_connection', true); + $appStoreUrl = $this->config->getSystemValueString('appstoreurl', self::APP_STORE_URL); + if (!$internetAvailable && $appStoreUrl === self::APP_STORE_URL) { + $this->logger->info( + 'The default app store cannot be accessed since Internet connectivity is disabled on this instance', + ['app' => 'appstoreFetcher'] + ); + return []; + } $ETag = ''; $content = ''; + /** @var ?list $sameVersionCachedData */ $sameVersionCachedData = null; $sameVersionCacheTimestamp = null; - $useCachedData = function () use (&$sameVersionCachedData, &$sameVersionCacheTimestamp): array { - $now = $this->timeFactory->getTime(); - - if ($sameVersionCachedData === null || $sameVersionCacheTimestamp === null) { - return []; - } - - if ($sameVersionCacheTimestamp >= ($now - self::MAX_STALE_SECONDS)) { - $this->logger->warning( - 'Could not refresh appstore cache, using stale data', - ['app' => 'appstoreFetcher'] - ); - - return $sameVersionCachedData; - } - - $this->logger->warning( - 'Could not refresh appstore cache and cached data is too old', - [ - 'app' => 'appstoreFetcher', - 'cacheAge' => $now - $sameVersionCacheTimestamp, - ] - ); - - return []; - }; - + $rootFolder = $this->appData->getFolder('/'); try { - // File does already exists + // Read the existing cache file. $file = $rootFolder->getFile($this->fileName); $jsonBlob = json_decode($file->getContent(), true); if (is_array($jsonBlob)) { - // No caching when the version has been updated + // Only use cache data generated for the current Nextcloud version. if (isset($jsonBlob['ncversion']) && $jsonBlob['ncversion'] === $this->getVersion()) { - if (isset($jsonBlob['data']) && is_array($jsonBlob['data'])) { - $sameVersionCachedData = $jsonBlob['data']; + if ( + isset($jsonBlob['data']) + && is_array($jsonBlob['data']) + && array_is_list($jsonBlob['data']) + ) { + /** @var list $cachedData */ + $cachedData = $jsonBlob['data']; + $sameVersionCachedData = $cachedData; } if (isset($jsonBlob['timestamp']) && is_numeric($jsonBlob['timestamp'])) { @@ -205,50 +226,117 @@ public function get($allowUnstable = false): array { return $sameVersionCachedData; } - if (isset($jsonBlob['ETag'])) { + // Reuse the ETag only when valid same-version cached data is available. + if ($sameVersionCachedData !== null && isset($jsonBlob['ETag'])) { $ETag = $jsonBlob['ETag']; - $content = json_encode($jsonBlob['data']); + try { + $content = json_encode($sameVersionCachedData, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + $this->logger->warning( + 'Could not re-encode cached appstore data for conditional request', + ['app' => 'appstoreFetcher', 'exception' => $e] + ); + $ETag = ''; + $content = ''; + } } } } } catch (NotFoundException $e) { - // File does not already exist + // Create the cache file when it does not already exist. $file = $rootFolder->newFile($this->fileName); } catch (GenericFileException $e) { try { $file->delete(); } catch (\Exception) { - $this->logger->error('Could not read appstore cache file', ['app' => 'appstoreFetcher', 'exception' => $e]); + $this->logger->error( + 'Could not read appstore cache file', + ['app' => 'appstoreFetcher', 'exception' => $e] + ); return []; } - $this->logger->warning('Could not read appstore cache file, it will be refreshed', ['app' => 'appstoreFetcher', 'exception' => $e]); + $this->logger->warning( + 'Could not read appstore cache file, it will be refreshed', + ['app' => 'appstoreFetcher', 'exception' => $e] + ); $file = $rootFolder->newFile($this->fileName); } - // Refresh the file content try { $responseJson = $this->fetch($ETag, $content, $allowUnstable); - // On refresh failure, fallback to the stale but otherwise valid, - // same-version cached data, provided it is no older than - // MAX_STALE_SECONDS. An empty data array is valid and must be - // written to the cache. - if (!isset($responseJson['data']) || !is_array($responseJson['data'])) { - return $useCachedData(); + // An empty list is a valid successful response. Missing or invalid response + // data is treated as a failed refresh and falls back to eligible cached data. + if ( + !isset($responseJson['data']) + || !is_array($responseJson['data']) + || !array_is_list($responseJson['data']) + ) { + return $this->useCachedData($sameVersionCachedData, $sameVersionCacheTimestamp); } - $file->putContent(json_encode($responseJson)); - return $responseJson['data']; + /** @var list $responseData */ + $responseData = $responseJson['data']; + + try { + $file->putContent(json_encode($responseJson, JSON_THROW_ON_ERROR)); + } catch (\Exception $e) { + // Return fresh data even when updating the cache fails, but log for admin visibility. + $this->logger->warning( + 'Could not write appstore cache file: ' . $e->getMessage(), + ['app' => 'appstoreFetcher'] + ); + } + + return $responseData; } catch (ConnectException $e) { - $this->logger->warning('Could not connect to appstore: ' . $e->getMessage(), ['app' => 'appstoreFetcher']); - return $useCachedData(); + // Handle connection exceptions that escape an overridden or future fetch(). + $this->logger->warning( + 'Could not connect to appstore: ' . $e->getMessage(), + ['app' => 'appstoreFetcher'] + ); + + return $this->useCachedData($sameVersionCachedData, $sameVersionCacheTimestamp); } catch (\Exception $e) { $this->logger->warning($e->getMessage(), [ 'exception' => $e, 'app' => 'appstoreFetcher', ]); - return $useCachedData(); + + return $this->useCachedData($sameVersionCachedData, $sameVersionCacheTimestamp); + } + } + + /** + * @param ?list $sameVersionCachedData + * @param ?int $sameVersionCacheTimestamp + * @return list + */ + private function useCachedData(?array $sameVersionCachedData, ?int $sameVersionCacheTimestamp): array { + $now = $this->timeFactory->getTime(); + + if ($sameVersionCachedData === null || $sameVersionCacheTimestamp === null) { + return []; } + + if ($sameVersionCacheTimestamp >= ($now - self::MAX_STALE_SECONDS)) { + $this->logger->warning( + 'Could not refresh appstore cache, using stale data', + ['app' => 'appstoreFetcher'] + ); + + return $sameVersionCachedData; + } + + $this->logger->warning( + 'Could not refresh appstore cache and cached data is too old', + [ + 'app' => 'appstoreFetcher', + 'cacheAge' => $now - $sameVersionCacheTimestamp, + ] + ); + + return []; } /** From 1bc3cced2789b7a682b83f38a777431b71458375 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 11 Aug 2026 12:01:56 -0400 Subject: [PATCH 7/8] fix(AppStore): align AppFetcher overrides with Fetcher Add parameter typing and AppFetcher-specific docblocks, forward the allow-unstable flag to the parent fetcher, and document compatibility and allowlist filtering behavior. Signed-off-by: Josh --- lib/private/App/AppStore/Fetcher/AppFetcher.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/AppFetcher.php b/lib/private/App/AppStore/Fetcher/AppFetcher.php index a39bcfd2e3184..9d8e201436cbc 100644 --- a/lib/private/App/AppStore/Fetcher/AppFetcher.php +++ b/lib/private/App/AppStore/Fetcher/AppFetcher.php @@ -51,13 +51,13 @@ public function __construct( } /** - * Only returns the latest compatible app release in the releases array + * Fetches app data and keeps only the latest compatible release for each app. * * @inheritDoc */ #[\Override] - protected function fetch($ETag, $content, $allowUnstable = false): array { - $response = parent::fetch($ETag, $content); + protected function fetch(string $ETag, string $content, bool $allowUnstable = false): array { + $response = parent::fetch($ETag, $content, $allowUnstable); if (!isset($response['data']) || $response['data'] === null) { $this->logger->warning('Response from appstore is invalid, apps could not be retrieved. Try again later.', ['app' => 'appstoreFetcher']); @@ -152,8 +152,14 @@ public function setVersion(string $version, string $fileName = 'apps.json', bool $this->ignoreMaxVersion = $ignoreMaxVersion; } + /** + * Returns apps compatible with the current Nextcloud and PHP versions, + * optionally restricted by the configured app allowlist. + * + * @inheritDoc + */ #[\Override] - public function get($allowUnstable = false): array { + public function get(bool $allowUnstable = false): array { $allowPreReleases = $allowUnstable || $this->getChannel() === 'beta' || $this->getChannel() === 'daily' || $this->getChannel() === 'git'; $apps = parent::get($allowPreReleases); From ecd7fbb85657eb8b82b9a088b9d94753b2fd62cc Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 11 Aug 2026 12:14:45 -0400 Subject: [PATCH 8/8] fix(AppStore): clarify AppDiscoverFetcher filtering behavior Add typing to match parent class and docs explaining that upcoming-entry filtering is handled locally while the base fetcher retains its stable cache policy. Improve comments describing expired, future-dated, and malformed entries. Signed-off-by: Josh --- .../AppStore/Fetcher/AppDiscoverFetcher.php | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/AppDiscoverFetcher.php b/lib/private/App/AppStore/Fetcher/AppDiscoverFetcher.php index 3a91037454d45..9ee829163684c 100644 --- a/lib/private/App/AppStore/Fetcher/AppDiscoverFetcher.php +++ b/lib/private/App/AppStore/Fetcher/AppDiscoverFetcher.php @@ -17,7 +17,7 @@ use Psr\Log\LoggerInterface; /** - * Fetch app discover section entries from the app store + * Fetches and filters App Store discover-section entries. * * @psalm-import-type AppStoreFetcherDiscoverElement from ResponseDefinitions * @template-extends Fetcher @@ -49,18 +49,23 @@ public function __construct( } /** - * Get the app discover section entries + * Returns discover-section entries, optionally including upcoming entries. * - * @param bool $allowUnstable Include also upcoming entries + * Expired entries are always excluded. Entries with a future start date + * are included only when `$allowUnstable` is true. + * + * @param bool $allowUnstable Whether to include upcoming entries * @return list */ #[\Override] - public function get($allowUnstable = false): array { + public function get(bool $allowUnstable = false): array { + // The base fetcher is always called with the stable cache policy; + // $allowUnstable controls filtering of future-dated entries below. $entries = parent::get(false); $now = new DateTimeImmutable(); return array_values(array_filter($entries, function (array $entry) use ($now, $allowUnstable) { - // Always remove expired entries + // Always exclude expired entries. if (isset($entry['expiryDate'])) { try { $expiryDate = new DateTimeImmutable($entry['expiryDate']); @@ -73,7 +78,7 @@ public function get($allowUnstable = false): array { } } - // If not include upcoming entries, check for upcoming dates and remove those entries + // Exclude future-dated entries unless upcoming entries were requested. if (!$allowUnstable && isset($entry['date'])) { try { $date = new DateTimeImmutable($entry['date']); @@ -85,7 +90,8 @@ public function get($allowUnstable = false): array { return false; } } - // Otherwise the entry is not time limited and should stay + + // Entries without a relevant date remain eligible. return true; })); } @@ -101,7 +107,7 @@ public function getETag(): ?string { return (string)$jsonBlob['ETag']; } } catch (\Throwable $e) { - // ignore + // ETag lookup is best effort. } return null; }