Skip to content

Commit 512769b

Browse files
miaulalalamiaulalala
committed
fix(theming): fix broken custom images introduced in 32.0.9
Three bugs were introduced by the backport of #55132 (PR #58224): 1. ImageManager::getImage() incorrectly tried to convert any uploaded raster image (PNG, JPEG) to SVG when imagick SVG support was available. Imagick can read SVGs but reliably fails writing raster→SVG, causing an ImagickException that silently fell through to bug 2. 2. ThemingController::getImage() changed Content-Type from the config- stored MIME to $file->getMimeType(). The original stored file has no extension (named "logo", not "logo.png"), so detectPath() returned application/octet-stream, causing browsers to not render the image. 3. ImageManager::delete() cleaned up logo and logo.png but not logo.svg, leaving stale SVG conversions that would be served after image updates. Fixes #60146 AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com>
1 parent 57a2630 commit 512769b

4 files changed

Lines changed: 101 additions & 42 deletions

File tree

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

Lines changed: 7 additions & 1 deletion
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->config->getAppValue($this->appName, $key . 'Mime', '')
380+
: $file->getMimeType();
381+
$response->addHeader('Content-Type', $mimeType);
376382
$response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"');
377383
return $response;
378384
}

‎apps/theming/lib/ImageManager.php‎

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -85,31 +85,14 @@ public function getImageUrlAbsolute(string $key): string {
8585
public function getImage(string $key, bool $useSvg = true): ISimpleFile {
8686
$mime = $this->config->getAppValue('theming', $key . 'Mime', '');
8787
$folder = $this->getRootFolder()->getFolder('images');
88-
$useSvg = $useSvg && $this->canConvert('SVG');
8988

9089
if ($mime === '' || !$folder->fileExists($key)) {
9190
throw new NotFoundException();
9291
}
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')) {
92+
// only convert SVG originals to PNG when SVG output is not desired;
93+
// converting raster images to SVG produces broken output and is not useful
94+
$isOriginalSvg = ($mime === 'image/svg+xml' || $mime === 'image/svg');
95+
if ($isOriginalSvg && !$useSvg && $this->canConvert('SVG') && $this->canConvert('PNG')) {
11396
if (!$folder->fileExists($key . '.png')) {
11497
try {
11598
$finalIconFile = new \Imagick();
@@ -120,13 +103,12 @@ public function getImage(string $key, bool $useSvg = true): ISimpleFile {
120103
$pngFile->putContent($finalIconFile->getImageBlob());
121104
return $pngFile;
122105
} catch (\ImagickException $e) {
123-
$this->logger->info('The image was requested to be no SVG file, but converting it to PNG failed: ' . $e->getMessage());
106+
$this->logger->info('Converting SVG to PNG failed: ' . $e->getMessage());
124107
}
125108
} else {
126109
return $folder->getFile($key . '.png');
127110
}
128111
}
129-
// fallback to the original file
130112
return $folder->getFile($key);
131113
}
132114

@@ -214,6 +196,12 @@ public function delete(string $key): void {
214196
} catch (NotFoundException $e) {
215197
} catch (NotPermittedException $e) {
216198
}
199+
try {
200+
$file = $this->getRootFolder()->getFolder('images')->getFile($key . '.svg');
201+
$file->delete();
202+
} catch (NotFoundException $e) {
203+
} catch (NotPermittedException $e) {
204+
}
217205

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

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

Lines changed: 23 additions & 0 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->config
680+
->expects($this->any())
681+
->method('getAppValue')
682+
->with('theming', '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'))

‎apps/theming/tests/ImageManagerTest.php‎

Lines changed: 60 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -79,26 +79,14 @@ public function mockGetImage($key, $file) {
7979
->with('logo')
8080
->willThrowException(new NotFoundException());
8181
} else {
82-
$file->expects($this->once())
83-
->method('getContent')
84-
->willReturn(file_get_contents(__DIR__ . '/../../../tests/data/testimage.png'));
85-
$folder->expects($this->exactly(2))
82+
$folder->expects($this->once())
8683
->method('fileExists')
87-
->willReturnMap([
88-
['logo', true],
89-
['logo.png', false],
90-
]);
84+
->with('logo')
85+
->willReturn(true);
9186
$folder->expects($this->once())
9287
->method('getFile')
9388
->with('logo')
9489
->willReturn($file);
95-
$newFile = $this->createMock(ISimpleFile::class);
96-
$folder->expects($this->once())
97-
->method('newFile')
98-
->with('logo.png')
99-
->willReturn($newFile);
100-
$newFile->expects($this->once())
101-
->method('putContent');
10290
$this->rootFolder->expects($this->once())
10391
->method('getFolder')
10492
->with('images')
@@ -149,15 +137,69 @@ public function testGetImageUrlAbsolute(): void {
149137
}
150138

151139
public function testGetImage(): void {
152-
$this->checkImagick();
153140
$this->config->expects($this->once())
154-
->method('getAppValue')->with('theming', 'logoMime', false)
155-
->willReturn('png');
141+
->method('getAppValue')->with('theming', 'logoMime', '')
142+
->willReturn('image/png');
156143
$file = $this->createMock(ISimpleFile::class);
157144
$this->mockGetImage('logo', $file);
158145
$this->assertEquals($file, $this->imageManager->getImage('logo', false));
159146
}
160147

148+
public function testGetImageSvgToSvg(): void {
149+
$this->config->expects($this->once())
150+
->method('getAppValue')->with('theming', 'logoMime', '')
151+
->willReturn('image/svg+xml');
152+
$folder = $this->createMock(ISimpleFolder::class);
153+
$file = $this->createMock(ISimpleFile::class);
154+
$folder->expects($this->once())
155+
->method('fileExists')
156+
->with('logo')
157+
->willReturn(true);
158+
$folder->expects($this->once())
159+
->method('getFile')
160+
->with('logo')
161+
->willReturn($file);
162+
$this->rootFolder->expects($this->once())
163+
->method('getFolder')
164+
->with('images')
165+
->willReturn($folder);
166+
$this->assertEquals($file, $this->imageManager->getImage('logo', true));
167+
}
168+
169+
public function testGetImageSvgToPng(): void {
170+
$this->checkImagick();
171+
$this->config->expects($this->once())
172+
->method('getAppValue')->with('theming', 'logoMime', '')
173+
->willReturn('image/svg+xml');
174+
$folder = $this->createMock(ISimpleFolder::class);
175+
$svgFile = $this->createMock(ISimpleFile::class);
176+
$pngFile = $this->createMock(ISimpleFile::class);
177+
$svgFile->expects($this->once())
178+
->method('getContent')
179+
->willReturn(file_get_contents(__DIR__ . '/../../../core/img/logo/logo.svg'));
180+
$folder->expects($this->exactly(2))
181+
->method('fileExists')
182+
->willReturnMap([
183+
['logo', true],
184+
['logo.png', false],
185+
]);
186+
$folder->expects($this->once())
187+
->method('getFile')
188+
->with('logo')
189+
->willReturn($svgFile);
190+
$folder->expects($this->once())
191+
->method('newFile')
192+
->with('logo.png')
193+
->willReturn($pngFile);
194+
$pngFile->expects($this->once())
195+
->method('putContent');
196+
$this->rootFolder->expects($this->once())
197+
->method('getFolder')
198+
->with('images')
199+
->willReturn($folder);
200+
$this->assertEquals($pngFile, $this->imageManager->getImage('logo', false));
201+
}
202+
161203

162204
public function testGetImageUnset(): void {
163205
$this->expectException(NotFoundException::class);

0 commit comments

Comments
 (0)