fix(message): render received external SVG images

lib/Controller/ProxyController.php

@@ -13,6 +13,7 @@
 use OCA\Mail\Html\ProxyHmacGenerator;
 use OCA\Mail\Http\ProxyDownloadResponse;
 use OCA\Mail\Service\MailManager;
+use OCA\Mail\Service\SvgSanitizer;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
@@ -28,6 +29,9 @@
 use Psr\Log\LoggerInterface;
 use function file_get_contents;
 use function hash_equals;
+use function ltrim;
+use function stripos;
+use function str_starts_with;
 
 #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
 class ProxyController extends Controller {
@@ -37,6 +41,7 @@ class ProxyController extends Controller {
 	private LoggerInterface $logger;
 	private ProxyHmacGenerator $hmacGenerator;
 	private MailManager $mailManager;
+	private SvgSanitizer $svgSanitizer;
 	private ?string $userId;
 
 	public function __construct(string $appName,
@@ -47,6 +52,7 @@ public function __construct(string $appName,
 		ProxyHmacGenerator $hmacGenerator,
 		LoggerInterface $logger,
 		MailManager $mailManager,
+		SvgSanitizer $svgSanitizer,
 		?string $userId) {
 		parent::__construct($appName, $request);
 		$this->request = $request;
@@ -56,6 +62,7 @@ public function __construct(string $appName,
 		$this->logger = $logger;
 		$this->hmacGenerator = $hmacGenerator;
 		$this->mailManager = $mailManager;
+		$this->svgSanitizer = $svgSanitizer;
 		$this->userId = $userId;
 	}
 
@@ -112,6 +119,29 @@ public function proxy(string $src, ?int $id, ?string $hmac): Response {
 			$content = file_get_contents(__DIR__ . '/../../img/blocked-image.png');
 		}
 
+		$content = (string)$content;
+
+		// Browsers sniff raster image formats in <img> tags, but they refuse to
+		// render SVG unless it is served with the image/svg+xml content type.
+		// Detect and sanitise SVG markup so external SVG logos are displayed
+		// instead of staying blank. Sanitising also strips any active content in
+		// case the response is fetched through a direct (non-<img>) navigation.
+		if ($this->looksLikeSvg($content)) {
+			$content = $this->svgSanitizer->sanitize($content);
+			return new ProxyDownloadResponse($content, $src, 'image/svg+xml');
+		}
+
 		return new ProxyDownloadResponse($content, $src, 'application/octet-stream');
 	}
+
+	/**
+	 * Heuristically decide whether the given bytes are an SVG document.
+	 */
+	private function looksLikeSvg(string $content): bool {
+		$start = ltrim($content);
+		$hasSvgPrologue = str_starts_with($start, '<?xml')
+			|| str_starts_with($start, '<!--')
+			|| stripos($start, '<svg') === 0;
+		return $hasSvgPrologue && stripos($content, '<svg') !== false;
+	}
 }

lib/Service/SvgSanitizer.php

@@ -0,0 +1,105 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Mail\Service;
+
+use DOMAttr;
+use DOMDocument;
+use DOMElement;
+use DOMXPath;
+
+/**
+ * Removes active content from SVG markup before it is embedded into or sent
+ * with a message. SVGs are rendered in an <img>/CID context where scripts do
+ * not execute, but they are still sanitised as defence in depth: any document
+ * that cannot be parsed safely is dropped entirely.
+ */
+class SvgSanitizer {
+	/** Elements that can carry or execute active content. */
+	private const FORBIDDEN_ELEMENTS = [
+		'script',
+		'foreignObject',
+		'handler',
+		'listener',
+		'set',
+	];
+
+	/**
+	 * @param string $svg The raw (decoded) SVG markup
+	 * @return string The sanitised markup, or an empty string if it cannot be
+	 *                parsed safely
+	 */
+	public function sanitize(string $svg): string {
+		if (trim($svg) === '') {
+			return '';
+		}
+
+		// A DOCTYPE or entity declaration is not needed for plain SVG graphics
+		// and is a common XXE / entity-expansion vector. Reject such documents.
+		if (preg_match('/<!DOCTYPE|<!ENTITY/i', $svg) === 1) {
+			return '';
+		}
+
+		$dom = new DOMDocument();
+		$previousErrors = libxml_use_internal_errors(true);
+		// LIBXML_NONET forbids any network access while parsing.
+		$loaded = $dom->loadXML($svg, LIBXML_NONET);
+		libxml_clear_errors();
+		libxml_use_internal_errors($previousErrors);
+
+		if (!$loaded || $dom->documentElement === null) {
+			return '';
+		}
+
+		$xpath = new DOMXPath($dom);
+
+		// Remove dangerous elements. Matching on the local name catches them
+		// regardless of any namespace prefix (e.g. <x:script>).
+		foreach (self::FORBIDDEN_ELEMENTS as $tag) {
+			$nodes = $xpath->query('//*[local-name() = "' . $tag . '"]');
+			if ($nodes !== false) {
+				foreach (iterator_to_array($nodes) as $node) {
+					$node->parentNode?->removeChild($node);
+				}
+			}
+		}
+
+		$elements = $xpath->query('//*');
+		if ($elements !== false) {
+			foreach ($elements as $element) {
+				if ($element instanceof DOMElement) {
+					$this->stripDangerousAttributes($element);
+				}
+			}
+		}
+
+		$result = $dom->saveXML($dom->documentElement);
+		return $result === false ? '' : $result;
+	}
+
+	private function stripDangerousAttributes(DOMElement $element): void {
+		/** @var DOMAttr $attribute */
+		foreach (iterator_to_array($element->attributes) as $attribute) {
+			$name = strtolower($attribute->nodeName);
+			$value = trim($attribute->nodeValue ?? '');
+
+			// Inline event handlers (onload, onclick, …).
+			if (str_starts_with($name, 'on')) {
+				$element->removeAttributeNode($attribute);
+				continue;
+			}
+
+			// Only allow same-document references; strip javascript:, external
+			// and data: URLs from links and resource references.
+			if (in_array($name, ['href', 'xlink:href'], true) && !str_starts_with($value, '#')) {
+				$element->removeAttributeNode($attribute);
+			}
+		}
+	}
+}

tests/Unit/Controller/ProxyControllerTest.php

@@ -15,6 +15,7 @@
 use OCA\Mail\Html\ProxyHmacGenerator;
 use OCA\Mail\Http\ProxyDownloadResponse;
 use OCA\Mail\Service\MailManager;
+use OCA\Mail\Service\SvgSanitizer;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Response;
@@ -53,6 +54,9 @@ class ProxyControllerTest extends TestCase {
 	/** @var MailManager|MockObject */
 	private $mailManager;
 
+	/** @var SvgSanitizer|MockObject */
+	private $svgSanitizer;
+
 	private string $userId = 'user';
 
 	/** @var ProxyController */
@@ -68,6 +72,7 @@ protected function setUp(): void {
 		$this->clientService = $this->createMock(IClientService::class);
 		$this->hmacGenerator = $this->createMock(ProxyHmacGenerator::class);
 		$this->mailManager = $this->createMock(MailManager::class);
+		$this->svgSanitizer = $this->createMock(SvgSanitizer::class);
 		$this->logger = new NullLogger();
 	}
 
@@ -90,6 +95,7 @@ public function testProxyWithoutCookies(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
@@ -136,12 +142,67 @@ public function testProxy(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
+			$this->userId,
+		);
+
+		$response = $this->controller->proxy($src, $id, $validHmac);
+
+		$this->assertInstanceOf(ProxyDownloadResponse::class, $response);
+	}
+
+	public function testProxySanitizesAndServesSvg(): void {
+		$src = 'https://example.com/logo.svg';
+		$id = 1;
+		$validHmac = 'valid-hmac-hash';
+		$content = '<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>';
+		$sanitized = '<svg xmlns="http://www.w3.org/2000/svg"></svg>';
+		$httpResponse = $this->createMock(IResponse::class);
+		$this->request->expects(self::once())
+			->method('passesStrictCookieCheck')
+			->willReturn(true);
+		$this->session->expects($this->once())
+			->method('close');
+		$this->hmacGenerator->expects($this->once())
+			->method('generate')
+			->with($id, $src)
+			->willReturn($validHmac);
+		$this->mailManager->expects($this->once())
+			->method('getMessage')
+			->with($this->userId, $id);
+		$client = $this->getMockBuilder(IClient::class)->getMock();
+		$this->clientService->expects($this->once())
+			->method('newClient')
+			->willReturn($client);
+		$client->expects($this->once())
+			->method('get')
+			->with($src)
+			->willReturn($httpResponse);
+		$httpResponse->expects($this->once())
+			->method('getBody')
+			->willReturn($content);
+		$this->svgSanitizer->expects($this->once())
+			->method('sanitize')
+			->with($content)
+			->willReturn($sanitized);
+		$this->controller = new ProxyController(
+			$this->appName,
+			$this->request,
+			$this->urlGenerator,
+			$this->session,
+			$this->clientService,
+			$this->hmacGenerator,
+			$this->logger,
+			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
 		$response = $this->controller->proxy($src, $id, $validHmac);
 
 		$this->assertInstanceOf(ProxyDownloadResponse::class, $response);
+		$this->assertSame('image/svg+xml', $response->getHeaders()['Content-Type']);
+		$this->assertSame($sanitized, $response->render());
 	}
 
 	public function testProxyWithInvalidHmac(): void {
@@ -172,6 +233,7 @@ public function testProxyWithInvalidHmac(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
@@ -199,6 +261,7 @@ public function testProxyWithMissingHmacParameters(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
@@ -231,6 +294,7 @@ public function testProxyWithMessageNotOwnedByUser(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 

tests/Unit/Service/SvgSanitizerTest.php

@@ -0,0 +1,82 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Mail\Tests\Unit\Service;
+
+use ChristophWurst\Nextcloud\Testing\TestCase;
+use OCA\Mail\Service\SvgSanitizer;
+
+class SvgSanitizerTest extends TestCase {
+	private SvgSanitizer $sanitizer;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->sanitizer = new SvgSanitizer();
+	}
+
+	public function testRemovesScript(): void {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script><rect/></svg>';
+
+		$result = $this->sanitizer->sanitize($svg);
+
+		$this->assertStringNotContainsString('<script', $result);
+		$this->assertStringContainsString('<rect', $result);
+	}
+
+	public function testRemovesEventHandlers(): void {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg"><rect onload="evil()" width="10"/></svg>';
+
+		$result = $this->sanitizer->sanitize($svg);
+
+		$this->assertStringNotContainsString('onload', $result);
+		$this->assertStringContainsString('width="10"', $result);
+	}
+
+	public function testRemovesExternalReference(): void {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">'
+			. '<a xlink:href="https://evil.example"><rect/></a></svg>';
+
+		$result = $this->sanitizer->sanitize($svg);
+
+		$this->assertStringNotContainsString('evil.example', $result);
+		$this->assertStringContainsString('<rect', $result);
+	}
+
+	public function testKeepsSameDocumentReference(): void {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">'
+			. '<use xlink:href="#icon"/></svg>';
+
+		$result = $this->sanitizer->sanitize($svg);
+
+		$this->assertStringContainsString('#icon', $result);
+	}
+
+	public function testRejectsDoctypeAndEntities(): void {
+		$svg = '<!DOCTYPE svg [<!ENTITY xxe SYSTEM "file:///etc/passwd">]>'
+			. '<svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>';
+
+		$this->assertSame('', $this->sanitizer->sanitize($svg));
+	}
+
+	public function testReturnsEmptyForInvalidMarkup(): void {
+		$this->assertSame('', $this->sanitizer->sanitize('this is not <<< svg'));
+		$this->assertSame('', $this->sanitizer->sanitize(''));
+	}
+
+	public function testKeepsSafeGraphics(): void {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 10 10">'
+			. '<circle cx="5" cy="5" r="4" fill="red"/></svg>';
+
+		$result = $this->sanitizer->sanitize($svg);
+
+		$this->assertStringContainsString('<circle', $result);
+		$this->assertStringContainsString('fill="red"', $result);
+	}
+}