From 3551e61d70ae67aec529218b7f57ff338211e24e Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 7 May 2026 12:09:28 +0200 Subject: [PATCH 1/6] fix(theming): fix broken custom images introduced in 32.0.9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #58224 introduced a raster→SVG conversion path in ImageManager::getImage() that breaks display of custom theming images. The root cause is a three-part bug chain: 1. getImage() attempted to convert raster images (PNG/JPEG) to SVG format, which Imagick cannot do meaningfully and produces broken output. 2. getMimeType() returns 'application/octet-stream' for extensionless stored files, so the Content-Type response header was wrong. 3. Stale .svg cache files persisted after image replacement, causing subsequent requests to serve the wrong format. Fix by: - Restricting the Imagick conversion to SVG→PNG only (not raster→SVG) - Reading the stored MIME type from IAppConfig for extensionless files in ThemingController::getImage() - Deleting .svg cache files in ImageManager::delete() - Injecting IAppConfig into ImageManager and reading the cachebuster via IAppConfig::getAppValueInt() so the URL returned after upload always carries the freshly-incremented value (IConfig::getAppValue() can return a stale cached value within the same request) - Updating the FileInputField Vue component to use a reactive cacheKey ref that increments on every upload, so the thumbnail refreshes even when the MIME type of the new image is the same as the old one AI-Assisted-By: Claude Sonnet 4.6 Signed-off-by: Anna Larch --- .../lib/Controller/ThemingController.php | 10 ++++- apps/theming/lib/ImageManager.php | 40 +++++++------------ lib/private/Server.php | 5 +++ 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 3b83e755b65da..0692528987b96 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -356,7 +356,13 @@ public function getImage(string $key, bool $useSvg = true) { $csp->allowInlineStyle(); $response->setContentSecurityPolicy($csp); $response->cacheFor(3600); - $response->addHeader('Content-Type', $file->getMimeType()); + // The original stored file has no extension (e.g. "logo"), so getMimeType() returns + // application/octet-stream for it. Use the config-stored MIME type for the original + // file, and getMimeType() only for converted files which have a proper extension. + $mimeType = $file->getName() === $key + ? $this->appConfig->getAppValueString($key . 'Mime', '') + : $file->getMimeType(); + $response->addHeader('Content-Type', $mimeType); $response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"'); return $response; } @@ -435,7 +441,7 @@ public function getThemeStylesheet(string $themeId, bool $plain = false, bool $w #[BruteForceProtection(action: 'manifest')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] public function getManifest(string $app): JSONResponse { - $cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0'); + $cacheBusterValue = $this->appConfig->getAppValueString('cachebuster', '0'); if ($app === 'core' || $app === 'settings') { $name = $this->themingDefaults->getName(); $shortName = $this->themingDefaults->getName(); diff --git a/apps/theming/lib/ImageManager.php b/apps/theming/lib/ImageManager.php index 1979656dd1e82..ad3903f148e9b 100644 --- a/apps/theming/lib/ImageManager.php +++ b/apps/theming/lib/ImageManager.php @@ -8,6 +8,7 @@ use OCA\Theming\AppInfo\Application; use OCA\Theming\Service\BackgroundService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; @@ -30,6 +31,7 @@ public function __construct( private LoggerInterface $logger, private ITempManager $tempManager, private BackgroundService $backgroundService, + private IAppConfig $appConfig, ) { } @@ -40,7 +42,7 @@ public function __construct( * @return string the image url */ public function getImageUrl(string $key): string { - $cacheBusterCounter = $this->config->getAppValue(Application::APP_ID, 'cachebuster', '0'); + $cacheBusterCounter = (string)$this->appConfig->getAppValueInt(ConfigLexicon::CACHE_BUSTER); if ($this->hasImage($key)) { return $this->urlGenerator->linkToRoute('theming.Theming.getImage', [ 'key' => $key ]) . '?v=' . $cacheBusterCounter; } elseif ($key === 'backgroundDark' && $this->hasImage('background')) { @@ -85,31 +87,14 @@ public function getImageUrlAbsolute(string $key): string { public function getImage(string $key, bool $useSvg = true): ISimpleFile { $mime = $this->config->getAppValue('theming', $key . 'Mime', ''); $folder = $this->getRootFolder()->getFolder('images'); - $useSvg = $useSvg && $this->canConvert('SVG'); if ($mime === '' || !$folder->fileExists($key)) { throw new NotFoundException(); } - // if SVG was requested and is supported - if ($useSvg) { - if (!$folder->fileExists($key . '.svg')) { - try { - $finalIconFile = new \Imagick(); - $finalIconFile->setBackgroundColor('none'); - $finalIconFile->readImageBlob($folder->getFile($key)->getContent()); - $finalIconFile->setImageFormat('SVG'); - $svgFile = $folder->newFile($key . '.svg'); - $svgFile->putContent($finalIconFile->getImageBlob()); - return $svgFile; - } catch (\ImagickException $e) { - $this->logger->info('The image was requested to be no SVG file, but converting it to SVG failed: ' . $e->getMessage()); - } - } else { - return $folder->getFile($key . '.svg'); - } - } - // if SVG was not requested, but PNG is supported - if (!$useSvg && $this->canConvert('PNG')) { + // only convert SVG originals to PNG when SVG output is not desired; + // converting raster images to SVG produces broken output and is not useful + $isOriginalSvg = ($mime === 'image/svg+xml' || $mime === 'image/svg'); + if ($isOriginalSvg && !$useSvg && $this->canConvert('SVG') && $this->canConvert('PNG')) { if (!$folder->fileExists($key . '.png')) { try { $finalIconFile = new \Imagick(); @@ -120,13 +105,12 @@ public function getImage(string $key, bool $useSvg = true): ISimpleFile { $pngFile->putContent($finalIconFile->getImageBlob()); return $pngFile; } catch (\ImagickException $e) { - $this->logger->info('The image was requested to be no SVG file, but converting it to PNG failed: ' . $e->getMessage()); + $this->logger->info('Converting SVG to PNG failed: ' . $e->getMessage()); } } else { return $folder->getFile($key . '.png'); } } - // fallback to the original file return $folder->getFile($key); } @@ -157,7 +141,7 @@ public function getCustomImages(): array { * @throws NotPermittedException */ public function getCacheFolder(): ISimpleFolder { - $cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0'); + $cacheBusterValue = (string)$this->appConfig->getAppValueInt(ConfigLexicon::CACHE_BUSTER); try { $folder = $this->getRootFolder()->getFolder($cacheBusterValue); } catch (NotFoundException $e) { @@ -214,6 +198,12 @@ public function delete(string $key): void { } catch (NotFoundException $e) { } catch (NotPermittedException $e) { } + try { + $file = $this->getRootFolder()->getFolder('images')->getFile($key . '.svg'); + $file->delete(); + } catch (NotFoundException $e) { + } catch (NotPermittedException $e) { + } if ($key === 'logo') { $this->config->deleteAppValue('theming', 'logoDimensions'); diff --git a/lib/private/Server.php b/lib/private/Server.php index c8df46826cd7b..7ff163cd08868 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1030,6 +1030,11 @@ public function __construct($webRoot, \OC\Config $config) { $c->get(LoggerInterface::class), $c->get(ITempManager::class), $backgroundService, + new AppConfig( + $c->get(IConfig::class), + $c->get(IAppConfig::class), + 'theming', + ), ); return new ThemingDefaults( $c->get(\OCP\IConfig::class), From e32189490b3508ad6eb8343c224d6f51e12dc63d Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 7 May 2026 12:09:35 +0200 Subject: [PATCH 2/6] test(theming): update tests for ImageManager and ThemingController fixes - ImageManagerTest: inject IAppConfig mock, switch cachebuster assertions from IConfig::getAppValue to IAppConfig::getAppValueInt, add testGetImageSvgToSvg and testGetImageSvgToPng, update mockGetImage to reflect the corrected getImage() logic - ThemingControllerTest: update getImage and getManifest tests to use IAppConfig::getAppValueString for MIME type and cachebuster lookups, add testGetLogoOriginalFile for the extensionless-file MIME path AI-Assisted-By: Claude Sonnet 4.6 Signed-off-by: Anna Larch --- .../Controller/ThemingControllerTest.php | 29 +++- apps/theming/tests/ImageManagerTest.php | 150 ++++++++++++------ 2 files changed, 127 insertions(+), 52 deletions(-) diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 24b53b53d5190..2b684eb8c3dc8 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -666,6 +666,29 @@ public function testGetLogo(): void { } + public function testGetLogoOriginalFile(): void { + $file = $this->createMock(ISimpleFile::class); + $file->method('getName')->willReturn('logo'); + $file->method('getMTime')->willReturn(42); + $this->imageManager->expects($this->once()) + ->method('getImage') + ->willReturn($file); + $this->appConfig + ->expects($this->once()) + ->method('getAppValueString') + ->with('logoMime', '') + ->willReturn('image/png'); + + @$expected = new FileDisplayResponse($file); + $expected->cacheFor(3600); + $expected->addHeader('Content-Type', 'image/png'); + $expected->addHeader('Content-Disposition', 'attachment; filename="logo"'); + $csp = new ContentSecurityPolicy(); + $csp->allowInlineStyle(); + $expected->setContentSecurityPolicy($csp); + @$this->assertEquals($expected, $this->themingController->getImage('logo', false)); + } + public function testGetLoginBackgroundNotExistent(): void { $this->imageManager->method('getImage') ->with($this->equalTo('background')) @@ -708,10 +731,10 @@ public static function dataGetManifest(): array { #[\PHPUnit\Framework\Attributes\DataProvider('dataGetManifest')] public function testGetManifest(bool $standalone): void { - $this->config + $this->appConfig ->expects($this->once()) - ->method('getAppValue') - ->with('theming', 'cachebuster', '0') + ->method('getAppValueString') + ->with('cachebuster', '0') ->willReturn('0'); $this->themingDefaults ->expects($this->any()) diff --git a/apps/theming/tests/ImageManagerTest.php b/apps/theming/tests/ImageManagerTest.php index 0c4d555cc00ef..4e9da5986784f 100644 --- a/apps/theming/tests/ImageManagerTest.php +++ b/apps/theming/tests/ImageManagerTest.php @@ -9,6 +9,7 @@ use OCA\Theming\ImageManager; use OCA\Theming\Service\BackgroundService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; @@ -29,6 +30,7 @@ class ImageManagerTest extends TestCase { private LoggerInterface&MockObject $logger; private ITempManager&MockObject $tempManager; private ISimpleFolder&MockObject $rootFolder; + private IAppConfig&MockObject $appConfig; protected ImageManager $imageManager; protected function setUp(): void { @@ -41,6 +43,7 @@ protected function setUp(): void { $this->tempManager = $this->createMock(ITempManager::class); $this->rootFolder = $this->createMock(ISimpleFolder::class); $backgroundService = $this->createMock(BackgroundService::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->imageManager = new ImageManager( $this->config, $this->appData, @@ -49,6 +52,7 @@ protected function setUp(): void { $this->logger, $this->tempManager, $backgroundService, + $this->appConfig, ); $this->appData ->expects($this->any()) @@ -79,26 +83,14 @@ public function mockGetImage($key, $file) { ->with('logo') ->willThrowException(new NotFoundException()); } else { - $file->expects($this->once()) - ->method('getContent') - ->willReturn(file_get_contents(__DIR__ . '/../../../tests/data/testimage.png')); - $folder->expects($this->exactly(2)) + $folder->expects($this->once()) ->method('fileExists') - ->willReturnMap([ - ['logo', true], - ['logo.png', false], - ]); + ->with('logo') + ->willReturn(true); $folder->expects($this->once()) ->method('getFile') ->with('logo') ->willReturn($file); - $newFile = $this->createMock(ISimpleFile::class); - $folder->expects($this->once()) - ->method('newFile') - ->with('logo.png') - ->willReturn($newFile); - $newFile->expects($this->once()) - ->method('putContent'); $this->rootFolder->expects($this->once()) ->method('getFolder') ->with('images') @@ -108,12 +100,14 @@ public function mockGetImage($key, $file) { public function testGetImageUrl(): void { $this->checkImagick(); - $this->config->expects($this->exactly(2)) + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); + $this->config->expects($this->once()) ->method('getAppValue') - ->willReturnMap([ - ['theming', 'cachebuster', '0', '0'], - ['theming', 'logoMime', '', '0'], - ]); + ->with('theming', 'logoMime', '') + ->willReturn('image/png'); $this->urlGenerator->expects($this->once()) ->method('linkToRoute') ->willReturn('url-to-image'); @@ -121,12 +115,14 @@ public function testGetImageUrl(): void { } public function testGetImageUrlDefault(): void { - $this->config->expects($this->exactly(2)) + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); + $this->config->expects($this->once()) ->method('getAppValue') - ->willReturnMap([ - ['theming', 'cachebuster', '0', '0'], - ['theming', 'logoMime', '', ''], - ]); + ->with('theming', 'logoMime', '') + ->willReturn(''); $this->urlGenerator->expects($this->once()) ->method('imagePath') ->with('core', 'logo/logo.png') @@ -136,12 +132,14 @@ public function testGetImageUrlDefault(): void { public function testGetImageUrlAbsolute(): void { $this->checkImagick(); - $this->config->expects($this->exactly(2)) + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); + $this->config->expects($this->once()) ->method('getAppValue') - ->willReturnMap([ - ['theming', 'cachebuster', '0', '0'], - ['theming', 'logoMime', '', ''], - ]); + ->with('theming', 'logoMime', '') + ->willReturn(''); $this->urlGenerator->expects($this->any()) ->method('getAbsoluteUrl') ->willReturn('url-to-image-absolute?v=0'); @@ -149,15 +147,69 @@ public function testGetImageUrlAbsolute(): void { } public function testGetImage(): void { - $this->checkImagick(); $this->config->expects($this->once()) - ->method('getAppValue')->with('theming', 'logoMime', false) - ->willReturn('png'); + ->method('getAppValue')->with('theming', 'logoMime', '') + ->willReturn('image/png'); $file = $this->createMock(ISimpleFile::class); $this->mockGetImage('logo', $file); $this->assertEquals($file, $this->imageManager->getImage('logo', false)); } + public function testGetImageSvgToSvg(): void { + $this->config->expects($this->once()) + ->method('getAppValue')->with('theming', 'logoMime', '') + ->willReturn('image/svg+xml'); + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + $folder->expects($this->once()) + ->method('fileExists') + ->with('logo') + ->willReturn(true); + $folder->expects($this->once()) + ->method('getFile') + ->with('logo') + ->willReturn($file); + $this->rootFolder->expects($this->once()) + ->method('getFolder') + ->with('images') + ->willReturn($folder); + $this->assertEquals($file, $this->imageManager->getImage('logo', true)); + } + + public function testGetImageSvgToPng(): void { + $this->checkImagick(); + $this->config->expects($this->once()) + ->method('getAppValue')->with('theming', 'logoMime', '') + ->willReturn('image/svg+xml'); + $folder = $this->createMock(ISimpleFolder::class); + $svgFile = $this->createMock(ISimpleFile::class); + $pngFile = $this->createMock(ISimpleFile::class); + $svgFile->expects($this->once()) + ->method('getContent') + ->willReturn(file_get_contents(__DIR__ . '/../../../core/img/logo/logo.svg')); + $folder->expects($this->exactly(2)) + ->method('fileExists') + ->willReturnMap([ + ['logo', true], + ['logo.png', false], + ]); + $folder->expects($this->once()) + ->method('getFile') + ->with('logo') + ->willReturn($svgFile); + $folder->expects($this->once()) + ->method('newFile') + ->with('logo.png') + ->willReturn($pngFile); + $pngFile->expects($this->once()) + ->method('putContent'); + $this->rootFolder->expects($this->once()) + ->method('getFolder') + ->with('images') + ->willReturn($folder); + $this->assertEquals($pngFile, $this->imageManager->getImage('logo', false)); + } + public function testGetImageUnset(): void { $this->expectException(NotFoundException::class); @@ -170,10 +222,10 @@ public function testGetImageUnset(): void { public function testGetCacheFolder(): void { $folder = $this->createMock(ISimpleFolder::class); - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('theming', 'cachebuster', '0') - ->willReturn('0'); + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); $this->rootFolder->expects($this->once()) ->method('getFolder') ->with('0') @@ -182,10 +234,10 @@ public function testGetCacheFolder(): void { } public function testGetCacheFolderCreate(): void { $folder = $this->createMock(ISimpleFolder::class); - $this->config->expects($this->exactly(2)) - ->method('getAppValue') - ->with('theming', 'cachebuster', '0') - ->willReturn('0'); + $this->appConfig->expects($this->exactly(2)) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); $this->rootFolder->expects($this->exactly(2)) ->method('getFolder') ->with('0') @@ -261,10 +313,10 @@ public function testSetCachedImageCreate(): void { private function setupCacheFolder() { $folder = $this->createMock(ISimpleFolder::class); - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('theming', 'cachebuster', '0') - ->willReturn('0'); + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(0); $this->rootFolder->expects($this->once()) ->method('getFolder') ->with('0') @@ -286,10 +338,10 @@ public function testCleanup(): void { $folders[0]->expects($this->once())->method('delete'); $folders[1]->expects($this->once())->method('delete'); $folders[2]->expects($this->never())->method('delete'); - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('theming', 'cachebuster', '0') - ->willReturn('2'); + $this->appConfig->expects($this->once()) + ->method('getAppValueInt') + ->with('cachebuster') + ->willReturn(2); $this->rootFolder->expects($this->once()) ->method('getDirectoryListing') ->willReturn($folders); From 2364b49d4ab247e749082709b213dc31b2eeb448 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 7 May 2026 12:09:40 +0200 Subject: [PATCH 3/6] chore(psalm): update deprecated method baseline - Remove stale ThemingController entry (deprecated IConfig::getAppValue calls replaced with IAppConfig::getAppValueString) - Add CommentsEventListener::getEvent() (pre-existing deprecated usage not previously baselined) AI-Assisted-By: Claude Sonnet 4.6 Signed-off-by: Anna Larch From 75a9ff681a650576212461dbcb7a931a4bbfb4d2 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Fri, 8 May 2026 18:32:51 +0200 Subject: [PATCH 4/6] chore(psalm): update deprecated method baseline - Remove stale ThemingController entry (deprecated IConfig::getAppValue calls replaced with IAppConfig::getAppValueString) - Add CommentsEventListener::getEvent(), Activity/Listener::getEvent(), and Notification/Listener::getEvent() (pre-existing deprecated usage not previously baselined) Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- build/psalm-baseline.xml | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index b92298da7d122..f2495972d9143 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -43,6 +43,9 @@ + + + @@ -51,6 +54,9 @@ + + + @@ -62,6 +68,10 @@ + + + + @@ -2324,13 +2334,6 @@ - - - - - - - From d09be0b5195ea23c82a531ec5290b23be5289489 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Fri, 8 May 2026 18:35:13 +0200 Subject: [PATCH 5/6] fix(theming): use fully-qualified \OCP\IConfig in Server.php DI IConfig has no use-import in Server.php; the rest of the file uses \OCP\IConfig::class directly. Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- lib/private/Server.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Server.php b/lib/private/Server.php index 7ff163cd08868..0fc9787e332f6 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1031,7 +1031,7 @@ public function __construct($webRoot, \OC\Config $config) { $c->get(ITempManager::class), $backgroundService, new AppConfig( - $c->get(IConfig::class), + $c->get(\OCP\IConfig::class), $c->get(IAppConfig::class), 'theming', ), From 55837bc38a2b34e8f7de5f30f5228cecc1bcac2c Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Fri, 8 May 2026 19:04:56 +0200 Subject: [PATCH 6/6] chore(psalm): remove getEvent DeprecatedMethod entries from baseline Psalm does not flag these as violations on stable32; the entries are not needed. Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- build/psalm-baseline.xml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index f2495972d9143..410c062fd51a1 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -43,9 +43,6 @@ - - - @@ -54,9 +51,6 @@ - - - @@ -68,10 +62,6 @@ - - - -