Skip to content

Commit b002443

Browse files
miaulalalamiaulalala
authored
Merge pull request #60198 from nextcloud/fix/noid/theming-broken-images-32-0-9
fix(theming): fix broken custom images introduced by #58224
2 parents d153156 + 6404d75 commit b002443

12 files changed

Lines changed: 163 additions & 90 deletions

‎REUSE.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ SPDX-FileCopyrightText = "2016 ownCloud, Inc., 2016-2024 Nextcloud translators"
1010
SPDX-License-Identifier = "AGPL-3.0-only OR AGPL-3.0-or-later"
1111

1212
[[annotations]]
13-
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"]
13+
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"]
1414
precedence = "aggregate"
1515
SPDX-FileCopyrightText = "2016-2024 Nextcloud translators"
1616
SPDX-License-Identifier = "AGPL-3.0-or-later"

‎apps/theming/lib/Controller/ThemingController.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,13 @@ public function getImage(string $key, bool $useSvg = true) {
372372
$csp->allowInlineStyle();
373373
$response->setContentSecurityPolicy($csp);
374374
$response->cacheFor(3600);
375-
$response->addHeader('Content-Type', $file->getMimeType());
375+
// The original stored file has no extension (e.g. "logo"), so getMimeType() returns
376+
// application/octet-stream for it. Use the config-stored MIME type for the original
377+
// file, and getMimeType() only for converted files which have a proper extension.
378+
$mimeType = $file->getName() === $key
379+
? $this->appConfig->getAppValueString($key . 'Mime', '')
380+
: $file->getMimeType();
381+
$response->addHeader('Content-Type', $mimeType);
376382
$response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"');
377383
return $response;
378384
}
@@ -450,7 +456,7 @@ public function getThemeStylesheet(string $themeId, bool $plain = false, bool $w
450456
#[BruteForceProtection(action: 'manifest')]
451457
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
452458
public function getManifest(string $app): JSONResponse {
453-
$cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0');
459+
$cacheBusterValue = $this->appConfig->getAppValueString('cachebuster', '0');
454460
if ($app === 'core' || $app === 'settings') {
455461
$name = $this->themingDefaults->getName();
456462
$shortName = $this->themingDefaults->getName();

‎apps/theming/lib/ImageManager.php‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
use OCA\Theming\AppInfo\Application;
1010
use OCA\Theming\Service\BackgroundService;
11+
use OCP\AppFramework\Services\IAppConfig;
1112
use OCP\Files\IAppData;
1213
use OCP\Files\NotFoundException;
1314
use OCP\Files\NotPermittedException;
@@ -30,6 +31,7 @@ public function __construct(
3031
private LoggerInterface $logger,
3132
private ITempManager $tempManager,
3233
private BackgroundService $backgroundService,
34+
private IAppConfig $appConfig,
3335
) {
3436
}
3537

@@ -40,7 +42,7 @@ public function __construct(
4042
* @return string the image url
4143
*/
4244
public function getImageUrl(string $key): string {
43-
$cacheBusterCounter = $this->config->getAppValue(Application::APP_ID, 'cachebuster', '0');
45+
$cacheBusterCounter = (string)$this->appConfig->getAppValueInt(ConfigLexicon::CACHE_BUSTER);
4446
if ($this->hasImage($key)) {
4547
return $this->urlGenerator->linkToRoute('theming.Theming.getImage', [ 'key' => $key ]) . '?v=' . $cacheBusterCounter;
4648
} elseif ($key === 'backgroundDark' && $this->hasImage('background')) {
@@ -85,31 +87,14 @@ public function getImageUrlAbsolute(string $key): string {
8587
public function getImage(string $key, bool $useSvg = true): ISimpleFile {
8688
$mime = $this->config->getAppValue('theming', $key . 'Mime', '');
8789
$folder = $this->getRootFolder()->getFolder('images');
88-
$useSvg = $useSvg && $this->canConvert('SVG');
8990

9091
if ($mime === '' || !$folder->fileExists($key)) {
9192
throw new NotFoundException();
9293
}
93-
// if SVG was requested and is supported
94-
if ($useSvg) {
95-
if (!$folder->fileExists($key . '.svg')) {
96-
try {
97-
$finalIconFile = new \Imagick();
98-
$finalIconFile->setBackgroundColor('none');
99-
$finalIconFile->readImageBlob($folder->getFile($key)->getContent());
100-
$finalIconFile->setImageFormat('SVG');
101-
$svgFile = $folder->newFile($key . '.svg');
102-
$svgFile->putContent($finalIconFile->getImageBlob());
103-
return $svgFile;
104-
} catch (\ImagickException $e) {
105-
$this->logger->info('The image was requested to be no SVG file, but converting it to SVG failed: ' . $e->getMessage());
106-
}
107-
} else {
108-
return $folder->getFile($key . '.svg');
109-
}
110-
}
111-
// if SVG was not requested, but PNG is supported
112-
if (!$useSvg && $this->canConvert('PNG')) {
94+
// only convert SVG originals to PNG when SVG output is not desired;
95+
// converting raster images to SVG produces broken output and is not useful
96+
$isOriginalSvg = ($mime === 'image/svg+xml' || $mime === 'image/svg');
97+
if ($isOriginalSvg && !$useSvg && $this->canConvert('SVG') && $this->canConvert('PNG')) {
11398
if (!$folder->fileExists($key . '.png')) {
11499
try {
115100
$finalIconFile = new \Imagick();
@@ -120,13 +105,12 @@ public function getImage(string $key, bool $useSvg = true): ISimpleFile {
120105
$pngFile->putContent($finalIconFile->getImageBlob());
121106
return $pngFile;
122107
} catch (\ImagickException $e) {
123-
$this->logger->info('The image was requested to be no SVG file, but converting it to PNG failed: ' . $e->getMessage());
108+
$this->logger->info('Converting SVG to PNG failed: ' . $e->getMessage());
124109
}
125110
} else {
126111
return $folder->getFile($key . '.png');
127112
}
128113
}
129-
// fallback to the original file
130114
return $folder->getFile($key);
131115
}
132116

@@ -157,7 +141,7 @@ public function getCustomImages(): array {
157141
* @throws NotPermittedException
158142
*/
159143
public function getCacheFolder(): ISimpleFolder {
160-
$cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0');
144+
$cacheBusterValue = (string)$this->appConfig->getAppValueInt(ConfigLexicon::CACHE_BUSTER);
161145
try {
162146
$folder = $this->getRootFolder()->getFolder($cacheBusterValue);
163147
} catch (NotFoundException $e) {
@@ -214,6 +198,12 @@ public function delete(string $key): void {
214198
} catch (NotFoundException $e) {
215199
} catch (NotPermittedException $e) {
216200
}
201+
try {
202+
$file = $this->getRootFolder()->getFolder('images')->getFile($key . '.svg');
203+
$file->delete();
204+
} catch (NotFoundException $e) {
205+
} catch (NotPermittedException $e) {
206+
}
217207

218208
if ($key === 'logo') {
219209
$this->config->deleteAppValue('theming', 'logoDimensions');

‎apps/theming/src/components/admin/FileInputField.vue‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,13 @@ const emit = defineEmits<{
2929
3030
const isSaving = ref(false)
3131
const mime = ref(loadState<AdminThemingParameters>('theming', 'adminThemingParameters')[props.name + 'Mime'] as string)
32+
const cacheKey = ref(Date.now())
3233
3334
const inputElement = useTemplateRef('input')
3435
3536
const background = computed(() => {
3637
const baseUrl = generateUrl('/apps/theming/image/{key}', { key: props.name })
37-
return `url(${baseUrl}?v=${Date.now()}&m=${encodeURIComponent(mime.value)})`
38+
return `url(${baseUrl}?v=${cacheKey.value}&m=${encodeURIComponent(mime.value)})`
3839
})
3940
4041
/**
@@ -75,6 +76,7 @@ async function onChange() {
7576
},
7677
})
7778
mime.value = file.type
79+
cacheKey.value = Date.now()
7880
emit('updated')
7981
} catch (error) {
8082
if (isAxiosError(error) && error.response?.status === 422) {

‎apps/theming/tests/Controller/ThemingControllerTest.php‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,29 @@ public function testGetLogo(): void {
669669
}
670670

671671

672+
public function testGetLogoOriginalFile(): void {
673+
$file = $this->createMock(ISimpleFile::class);
674+
$file->method('getName')->willReturn('logo');
675+
$file->method('getMTime')->willReturn(42);
676+
$this->imageManager->expects($this->once())
677+
->method('getImage')
678+
->willReturn($file);
679+
$this->appConfig
680+
->expects($this->once())
681+
->method('getAppValueString')
682+
->with('logoMime', '')
683+
->willReturn('image/png');
684+
685+
@$expected = new FileDisplayResponse($file);
686+
$expected->cacheFor(3600);
687+
$expected->addHeader('Content-Type', 'image/png');
688+
$expected->addHeader('Content-Disposition', 'attachment; filename="logo"');
689+
$csp = new ContentSecurityPolicy();
690+
$csp->allowInlineStyle();
691+
$expected->setContentSecurityPolicy($csp);
692+
@$this->assertEquals($expected, $this->themingController->getImage('logo', false));
693+
}
694+
672695
public function testGetLoginBackgroundNotExistent(): void {
673696
$this->imageManager->method('getImage')
674697
->with($this->equalTo('background'))
@@ -711,10 +734,10 @@ public static function dataGetManifest(): array {
711734

712735
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetManifest')]
713736
public function testGetManifest(bool $standalone): void {
714-
$this->config
737+
$this->appConfig
715738
->expects($this->once())
716-
->method('getAppValue')
717-
->with('theming', 'cachebuster', '0')
739+
->method('getAppValueString')
740+
->with('cachebuster', '0')
718741
->willReturn('0');
719742
$this->themingDefaults
720743
->expects($this->any())

0 commit comments

Comments
 (0)