Skip to content

Commit 638b2c2

Browse files
miaulalalanextcloud-command
miaulalala
authored and
nextcloud-command
committed
fix(theming): fix broken custom images introduced by #58224
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 - Updating unit tests for ImageManager and ThemingController - Updating psalm baseline for stable32 Backport of #60198 AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com> Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
1 parent d0c2d97 commit 638b2c2

9 files changed

Lines changed: 169 additions & 90 deletions

File tree

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,13 @@ public function getImage(string $key, bool $useSvg = true) {
356356
$csp->allowInlineStyle();
357357
$response->setContentSecurityPolicy($csp);
358358
$response->cacheFor(3600);
359-
$response->addHeader('Content-Type', $file->getMimeType());
359+
// The original stored file has no extension (e.g. "logo"), so getMimeType() returns
360+
// application/octet-stream for it. Use the config-stored MIME type for the original
361+
// file, and getMimeType() only for converted files which have a proper extension.
362+
$mimeType = $file->getName() === $key
363+
? $this->appConfig->getAppValueString($key . 'Mime', '')
364+
: $file->getMimeType();
365+
$response->addHeader('Content-Type', $mimeType);
360366
$response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"');
361367
return $response;
362368
}
@@ -435,7 +441,7 @@ public function getThemeStylesheet(string $themeId, bool $plain = false, bool $w
435441
#[BruteForceProtection(action: 'manifest')]
436442
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
437443
public function getManifest(string $app): JSONResponse {
438-
$cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0');
444+
$cacheBusterValue = $this->appConfig->getAppValueString('cachebuster', '0');
439445
if ($app === 'core' || $app === 'settings') {
440446
$name = $this->themingDefaults->getName();
441447
$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: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@
4545
:class="{
4646
'field__preview--logoheader': name === 'logoheader',
4747
'field__preview--favicon': name === 'favicon',
48-
}" />
48+
}"
49+
:style="previewStyle" />
4950

5051
<NcNoteCard v-if="errorMessage"
5152
type="error"
@@ -124,6 +125,7 @@ export default {
124125
data() {
125126
return {
126127
showLoading: false,
128+
cacheKey: Date.now(),
127129
acceptMime: (allowedMimeTypes[this.name]
128130
|| ['image/jpeg', 'image/png', 'image/gif', 'image/webp']).join(','),
129131
}
@@ -145,6 +147,13 @@ export default {
145147
}
146148
return false
147149
},
150+
151+
previewStyle() {
152+
const url = generateUrl('/apps/theming/image/{key}', { key: this.name })
153+
return {
154+
backgroundImage: `url(${url}?v=${this.cacheKey}&m=${encodeURIComponent(this.mimeValue)})`,
155+
}
156+
},
148157
},
149158
150159
methods: {
@@ -167,6 +176,7 @@ export default {
167176
this.showLoading = true
168177
const { data } = await axios.post(url, formData)
169178
this.showLoading = false
179+
this.cacheKey = Date.now()
170180
this.$emit('update:mime-value', file.type)
171181
this.$emit('uploaded', data.data.url)
172182
this.handleSuccess()

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

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

668668

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

709732
#[\PHPUnit\Framework\Attributes\DataProvider('dataGetManifest')]
710733
public function testGetManifest(bool $standalone): void {
711-
$this->config
734+
$this->appConfig
712735
->expects($this->once())
713-
->method('getAppValue')
714-
->with('theming', 'cachebuster', '0')
736+
->method('getAppValueString')
737+
->with('cachebuster', '0')
715738
->willReturn('0');
716739
$this->themingDefaults
717740
->expects($this->any())

0 commit comments

Comments
 (0)