From 52ca11addd000f3df3ca42d1b2aaa9b438de9c54 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Sun, 10 May 2026 12:19:43 +0200 Subject: [PATCH 1/2] fix(appstore): catch GenericFileException when reading cache file in Fetcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the appstore cache file exists but getContent() throws a GenericFileException (I/O error or OS-level permission failure), explicitly delete the file and recreate it before writing fresh data — mirroring the NotFoundException recovery path. If deletion itself fails, return [] cleanly. Previously, the unhandled exception caused the entire apps settings page to crash. The new test covers both the recovery path and deletion failure. Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- lib/private/App/AppStore/Fetcher/Fetcher.php | 11 ++- .../lib/App/AppStore/Fetcher/FetcherBase.php | 84 +++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index 3304b414ad747..c6510ff6d8d64 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -12,6 +12,7 @@ use OC\Files\AppData\Factory; use OCP\AppFramework\Http; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Files\GenericFileException; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Http\Client\IClientService; @@ -161,7 +162,15 @@ public function get($allowUnstable = false) { } } } catch (NotFoundException $e) { - // File does not already exists + // File does not already exist + $file = $rootFolder->newFile($this->fileName); + } catch (GenericFileException $e) { + $this->logger->warning('Could not read appstore cache file, it will be refreshed', ['app' => 'appstoreFetcher', 'exception' => $e]); + try { + $file->delete(); + } catch (\Exception $deleteException) { + return []; + } $file = $rootFolder->newFile($this->fileName); } diff --git a/tests/lib/App/AppStore/Fetcher/FetcherBase.php b/tests/lib/App/AppStore/Fetcher/FetcherBase.php index 4784e76d574f6..bdff8b3181c72 100644 --- a/tests/lib/App/AppStore/Fetcher/FetcherBase.php +++ b/tests/lib/App/AppStore/Fetcher/FetcherBase.php @@ -13,6 +13,7 @@ use OC\Files\AppData\AppData; use OC\Files\AppData\Factory; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Files\GenericFileException; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; @@ -683,4 +684,87 @@ public function testFetchAfterUpgradeNoETag(): void { ]; $this->assertSame($expected, $this->fetcher->get()); } + + public function testGetWithUnreadableCacheFileRecreatesAndFetches(): void { + $this->config + ->method('getSystemValueString') + ->willReturnCallback(function ($var, $default) { + if ($var === 'appstoreurl') { + return 'https://apps.nextcloud.com/api/v1'; + } elseif ($var === 'version') { + return '11.0.0.2'; + } + return $default; + }); + $this->config->method('getSystemValueBool') + ->willReturnArgument(1); + + $folder = $this->createMock(ISimpleFolder::class); + $corruptedFile = $this->createMock(ISimpleFile::class); + $freshFile = $this->createMock(ISimpleFile::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($corruptedFile); + $corruptedFile + ->expects($this->once()) + ->method('getContent') + ->willThrowException(new GenericFileException()); + $corruptedFile + ->expects($this->once()) + ->method('delete'); + $folder + ->expects($this->once()) + ->method('newFile') + ->with($this->fileName) + ->willReturn($freshFile); + $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) + ->willReturn($response); + $response + ->expects($this->once()) + ->method('getBody') + ->willReturn('[{"id":"MyNewApp", "foo": "foo"}, {"id":"bar"}]'); + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; + $freshFile + ->expects($this->once()) + ->method('putContent') + ->with($fileData); + $freshFile + ->expects($this->once()) + ->method('getContent') + ->willReturn($fileData); + $this->timeFactory + ->expects($this->once()) + ->method('getTime') + ->willReturn(1502); + + $expected = [ + [ + 'id' => 'MyNewApp', + 'foo' => 'foo', + ], + [ + 'id' => 'bar', + ], + ]; + $this->assertSame($expected, $this->fetcher->get()); + } } From b723c9424625ff105330fb7f69ad64a7996e6943 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 19 May 2026 16:52:52 +0200 Subject: [PATCH 2/2] fix(appstore): address review comments on GenericFileException handling - Attempt delete before logging the warning, so the warning only fires when we know recovery will succeed - Log an error (not silently return) when delete itself fails - Use catch (\Exception) without variable (PHP 8) - Replace willReturnArgument(1) with explicit willReturn(true) in test - Add blank lines between logical blocks in test for readability Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- lib/private/App/AppStore/Fetcher/Fetcher.php | 5 +++-- tests/lib/App/AppStore/Fetcher/FetcherBase.php | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index c6510ff6d8d64..739c0b294d95b 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -165,12 +165,13 @@ public function get($allowUnstable = false) { // File does not already exist $file = $rootFolder->newFile($this->fileName); } catch (GenericFileException $e) { - $this->logger->warning('Could not read appstore cache file, it will be refreshed', ['app' => 'appstoreFetcher', 'exception' => $e]); try { $file->delete(); - } catch (\Exception $deleteException) { + } catch (\Exception) { + $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]); $file = $rootFolder->newFile($this->fileName); } diff --git a/tests/lib/App/AppStore/Fetcher/FetcherBase.php b/tests/lib/App/AppStore/Fetcher/FetcherBase.php index bdff8b3181c72..ca0eba78692fb 100644 --- a/tests/lib/App/AppStore/Fetcher/FetcherBase.php +++ b/tests/lib/App/AppStore/Fetcher/FetcherBase.php @@ -697,7 +697,7 @@ public function testGetWithUnreadableCacheFileRecreatesAndFetches(): void { return $default; }); $this->config->method('getSystemValueBool') - ->willReturnArgument(1); + ->willReturn(true); $folder = $this->createMock(ISimpleFolder::class); $corruptedFile = $this->createMock(ISimpleFile::class); @@ -724,6 +724,7 @@ public function testGetWithUnreadableCacheFileRecreatesAndFetches(): void { ->method('newFile') ->with($this->fileName) ->willReturn($freshFile); + $client = $this->createMock(IClient::class); $this->clientService ->expects($this->once()) @@ -742,6 +743,7 @@ public function testGetWithUnreadableCacheFileRecreatesAndFetches(): void { $response->method('getHeader') ->with($this->equalTo('ETag')) ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; $freshFile ->expects($this->once())