Merge pull request #60198 from nextcloud/fix/noid/theming-broken-images-32-0-9

fix(theming): fix broken custom images introduced by #58224
This commit is contained in:
Anna 2026-05-08 12:40:00 +02:00 committed by GitHub
commit b00244391c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 163 additions and 90 deletions

View file

@ -10,7 +10,7 @@ SPDX-FileCopyrightText = "2016 ownCloud, Inc., 2016-2024 Nextcloud translators"
SPDX-License-Identifier = "AGPL-3.0-only OR AGPL-3.0-or-later"
[[annotations]]
path = ["apps/cloud_federation_api/l10n/**.js", "apps/cloud_federation_api/l10n/**.json", "apps/contactsinteraction/l10n/**.js", "apps/contactsinteraction/l10n/**.json", "apps/dashboard/l10n/**.js", "apps/dashboard/l10n/**.json", "apps/files_reminders/l10n/**.js", "apps/files_reminders/l10n/**.json", "apps/lookup_server_connector/l10n/**.js", "apps/lookup_server_connector/l10n/**.json", "apps/profile/l10n/**.js", "apps/profile/l10n/**.json", "apps/sharebymail/l10n/**.js", "apps/oauth2/l10n/**.js", "apps/oauth2/l10n/**.json", "apps/sharebymail/l10n/**.json", "apps/theming/l10n/**.js", "apps/theming/l10n/**.json", "apps/twofactor_backupcodes/l10n/**.js", "apps/twofactor_backupcodes/l10n/**.json", "apps/user_status/l10n/**.js", "apps/user_status/l10n/**.json", "apps/weather_status/l10n/**.js", "apps/weather_status/l10n/**.json", "apps/webhook_listeners/l10n/**.js", "apps/webhook_listeners/l10n/**.json", "apps/workflowengine/l10n/**.js", "apps/workflowengine/l10n/**.json"]
path = ["apps/appstore/l10n/**.js", "apps/appstore/l10n/**.json", "apps/cloud_federation_api/l10n/**.js", "apps/cloud_federation_api/l10n/**.json", "apps/contactsinteraction/l10n/**.js", "apps/contactsinteraction/l10n/**.json", "apps/dashboard/l10n/**.js", "apps/dashboard/l10n/**.json", "apps/files_reminders/l10n/**.js", "apps/files_reminders/l10n/**.json", "apps/lookup_server_connector/l10n/**.js", "apps/lookup_server_connector/l10n/**.json", "apps/profile/l10n/**.js", "apps/profile/l10n/**.json", "apps/sharebymail/l10n/**.js", "apps/oauth2/l10n/**.js", "apps/oauth2/l10n/**.json", "apps/sharebymail/l10n/**.json", "apps/theming/l10n/**.js", "apps/theming/l10n/**.json", "apps/twofactor_backupcodes/l10n/**.js", "apps/twofactor_backupcodes/l10n/**.json", "apps/user_status/l10n/**.js", "apps/user_status/l10n/**.json", "apps/weather_status/l10n/**.js", "apps/weather_status/l10n/**.json", "apps/webhook_listeners/l10n/**.js", "apps/webhook_listeners/l10n/**.json", "apps/workflowengine/l10n/**.js", "apps/workflowengine/l10n/**.json"]
precedence = "aggregate"
SPDX-FileCopyrightText = "2016-2024 Nextcloud translators"
SPDX-License-Identifier = "AGPL-3.0-or-later"

View file

@ -372,7 +372,13 @@ class ThemingController extends Controller {
$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;
}
@ -450,7 +456,7 @@ class ThemingController extends Controller {
#[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();

View file

@ -8,6 +8,7 @@ namespace OCA\Theming;
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 @@ class ImageManager {
private LoggerInterface $logger,
private ITempManager $tempManager,
private BackgroundService $backgroundService,
private IAppConfig $appConfig,
) {
}
@ -40,7 +42,7 @@ class ImageManager {
* @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 @@ class ImageManager {
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 @@ class ImageManager {
$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 @@ class ImageManager {
* @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 @@ class ImageManager {
} 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');

View file

@ -29,12 +29,13 @@ const emit = defineEmits<{
const isSaving = ref(false)
const mime = ref(loadState<AdminThemingParameters>('theming', 'adminThemingParameters')[props.name + 'Mime'] as string)
const cacheKey = ref(Date.now())
const inputElement = useTemplateRef('input')
const background = computed(() => {
const baseUrl = generateUrl('/apps/theming/image/{key}', { key: props.name })
return `url(${baseUrl}?v=${Date.now()}&m=${encodeURIComponent(mime.value)})`
return `url(${baseUrl}?v=${cacheKey.value}&m=${encodeURIComponent(mime.value)})`
})
/**
@ -75,6 +76,7 @@ async function onChange() {
},
})
mime.value = file.type
cacheKey.value = Date.now()
emit('updated')
} catch (error) {
if (isAxiosError(error) && error.response?.status === 422) {

View file

@ -669,6 +669,29 @@ class ThemingControllerTest extends TestCase {
}
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'))
@ -711,10 +734,10 @@ class ThemingControllerTest extends TestCase {
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: '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())

View file

@ -9,6 +9,7 @@ namespace OCA\Theming\Tests;
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 @@ class ImageManagerTest extends TestCase {
$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 @@ class ImageManagerTest extends TestCase {
$this->logger,
$this->tempManager,
$backgroundService,
$this->appConfig,
);
$this->appData
->expects($this->any())
@ -79,26 +83,14 @@ class ImageManagerTest extends TestCase {
->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 @@ class ImageManagerTest extends TestCase {
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 @@ class ImageManagerTest extends TestCase {
}
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 @@ class ImageManagerTest extends TestCase {
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 @@ class ImageManagerTest extends TestCase {
}
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 @@ class ImageManagerTest extends TestCase {
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 @@ class ImageManagerTest extends TestCase {
}
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 @@ class ImageManagerTest extends TestCase {
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 @@ class ImageManagerTest extends TestCase {
$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);

View file

@ -2594,11 +2594,6 @@
<code><![CDATA[getAppValue]]></code>
</DeprecatedMethod>
</file>
<file src="apps/theming/lib/Controller/ThemingController.php">
<DeprecatedMethod>
<code><![CDATA[getAppValue]]></code>
</DeprecatedMethod>
</file>
<file src="apps/theming/lib/Controller/UserThemeController.php">
<DeprecatedMethod>
<code><![CDATA[getUserValue]]></code>

View file

@ -1,5 +1,5 @@
/* extracted by css-entry-points-plugin */
@import './theming-theming-settings-admin-ljDcYh_G.chunk.css';
@import './theming-theming-settings-admin-B-lUvcqL.chunk.css';
@import './common-Web-C_oBIsvc.chunk.css';
@import './common-ArrowRight-D7L4ZBkR.chunk.css';
@import './common-mdi-Jq77EThs.chunk.css';

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

View file

@ -1 +1 @@
h3[data-v-a9baec70],h4[data-v-a9baec70]{font-weight:700}h4[data-v-a9baec70],h5[data-v-a9baec70]{margin-block-start:12px}.info-note[data-v-a9baec70]{color:var(--color-text-maxcontrast)}._adminSectionTheming_1p8pd_2{max-width:650px;display:flex;flex-direction:column;gap:calc(2 * var(--default-grid-baseline))}._colorPickerField_o0yey_2{display:flex;flex-direction:column}._colorPickerField__row_o0yey_7{display:flex;flex-direction:row;align-items:center;gap:calc(1.5 * var(--default-grid-baseline))}._colorPickerField__button_o0yey_14{min-width:clamp(200px,25vw,300px)!important}._colorPickerField__description_o0yey_18{color:var(--color-text-maxcontrast);margin-block:calc(.5 * var(--default-grid-baseline)) var(--default-grid-baseline)}._colorPickerField__description_o0yey_18:empty{display:none}._fileInputField_1u7zs_2{display:flex;flex-direction:row;flex-wrap:wrap;align-items:center;gap:calc(1.5 * var(--default-grid-baseline))}._fileInputField__button_1u7zs_10{min-width:clamp(200px,25vw,300px)!important}._fileInputField__preview_1u7zs_14{height:var(--clickable-area-large);width:calc(var(--clickable-area-large) / 9 * 16);background:var(--v64992c7e);background-size:contain;background-repeat:no-repeat;background-position:center;border:2px solid var(--color-border-maxcontrast);border-radius:var(--border-radius-element)}._adminSectionThemingAdvanced_dyaj4_2{display:flex;flex-direction:column;gap:calc(2 * var(--default-grid-baseline));max-width:650px}
h3[data-v-a9baec70],h4[data-v-a9baec70]{font-weight:700}h4[data-v-a9baec70],h5[data-v-a9baec70]{margin-block-start:12px}.info-note[data-v-a9baec70]{color:var(--color-text-maxcontrast)}._adminSectionTheming_1p8pd_2{max-width:650px;display:flex;flex-direction:column;gap:calc(2 * var(--default-grid-baseline))}._colorPickerField_o0yey_2{display:flex;flex-direction:column}._colorPickerField__row_o0yey_7{display:flex;flex-direction:row;align-items:center;gap:calc(1.5 * var(--default-grid-baseline))}._colorPickerField__button_o0yey_14{min-width:clamp(200px,25vw,300px)!important}._colorPickerField__description_o0yey_18{color:var(--color-text-maxcontrast);margin-block:calc(.5 * var(--default-grid-baseline)) var(--default-grid-baseline)}._colorPickerField__description_o0yey_18:empty{display:none}._fileInputField_1u7zs_2{display:flex;flex-direction:row;flex-wrap:wrap;align-items:center;gap:calc(1.5 * var(--default-grid-baseline))}._fileInputField__button_1u7zs_10{min-width:clamp(200px,25vw,300px)!important}._fileInputField__preview_1u7zs_14{height:var(--clickable-area-large);width:calc(var(--clickable-area-large) / 9 * 16);background:var(--v6c51c3da);background-size:contain;background-repeat:no-repeat;background-position:center;border:2px solid var(--color-border-maxcontrast);border-radius:var(--border-radius-element)}._adminSectionThemingAdvanced_dyaj4_2{display:flex;flex-direction:column;gap:calc(2 * var(--default-grid-baseline));max-width:650px}

View file

@ -1078,6 +1078,11 @@ class Server extends ServerContainer implements IServerContainer {
$c->get(LoggerInterface::class),
$c->get(ITempManager::class),
$backgroundService,
new AppConfig(
$c->get(IConfig::class),
$c->get(IAppConfig::class),
'theming',
),
);
return new ThemingDefaults(
new AppConfig(