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;
@@ -37,6 +38,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 +49,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 +59,7 @@ public function __construct(string $appName,
 		$this->logger = $logger;
 		$this->hmacGenerator = $hmacGenerator;
 		$this->mailManager = $mailManager;
+		$this->svgSanitizer = $svgSanitizer;
 		$this->userId = $userId;
 	}
 
@@ -112,6 +116,22 @@ 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->svgSanitizer->looksLikeSvg($content)) {
+			$sanitized = $this->svgSanitizer->sanitize($content);
+			if ($sanitized === '') {
+				$content = (string)file_get_contents(__DIR__ . '/../../img/blocked-image.png');
+				return new ProxyDownloadResponse($content, $src, 'application/octet-stream');
+			}
+			return new ProxyDownloadResponse($sanitized, $src, 'image/svg+xml');
+		}
+
 		return new ProxyDownloadResponse($content, $src, 'application/octet-stream');
 	}
 }

lib/Service/SvgSanitizer.php

@@ -0,0 +1,143 @@
+<?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',
+	];
+
+	/** Attributes that carry URL references and must not point off-document. */
+	private const URL_ATTRIBUTES = ['href', 'xlink:href', 'src', 'action', 'formaction'];
+
+	/** Reject payloads larger than this to prevent DoS via oversized documents. */
+	private const MAX_SVG_BYTES = 2 * 1024 * 1024;
+
+	/**
+	 * @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) === '' || strlen($svg) > self::MAX_SVG_BYTES) {
+			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);
+				}
+			}
+		}
+
+		// Sanitise <style> element content: strip external CSS url() references.
+		$styleNodes = $xpath->query('//*[local-name() = "style"]');
+		if ($styleNodes !== false) {
+			foreach ($styleNodes as $node) {
+				$node->textContent = $this->stripCssUrls($node->textContent);
+			}
+		}
+
+		$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;
+	}
+
+	/**
+	 * Heuristically decide whether the given bytes are an SVG document.
+	 */
+	public function looksLikeSvg(string $content): bool {
+		$start = ltrim($content);
+		$hasSvgPrologue = str_starts_with($start, '<?xml')
+			|| stripos($start, '<svg') === 0;
+		return $hasSvgPrologue && stripos($content, '<svg') !== false;
+	}
+
+	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, self::URL_ATTRIBUTES, true) && !str_starts_with($value, '#')) {
+				$element->removeAttributeNode($attribute);
+				continue;
+			}
+
+			// Strip external CSS url() references from inline style attributes.
+			if ($name === 'style') {
+				$element->setAttribute('style', $this->stripCssUrls($value));
+			}
+		}
+	}
+
+	/**
+	 * Replace CSS url() references that point outside the document with 'none'.
+	 * Fragment references (url(#…)) are preserved for gradients and masks.
+	 */
+	private function stripCssUrls(string $css): string {
+		return preg_replace('/url\s*\((?!\s*[\'"]?#)[^)]*\)/i', 'none', $css) ?? $css;
+	}
+}

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,127 @@ 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('looksLikeSvg')
+			->with($content)
+			->willReturn(true);
+		$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 testProxyServesBlockedImageWhenSvgSanitizerReturnsEmpty(): void {
+		$src = 'https://example.com/malicious.svg';
+		$id = 1;
+		$validHmac = 'valid-hmac-hash';
+		$content = '<svg xmlns="http://www.w3.org/2000/svg"><!DOCTYPE evil></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('looksLikeSvg')
+			->with($content)
+			->willReturn(true);
+		$this->svgSanitizer->expects($this->once())
+			->method('sanitize')
+			->with($content)
+			->willReturn('');
+		$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('application/octet-stream', $response->getHeaders()['Content-Type']);
 	}
 
 	public function testProxyWithInvalidHmac(): void {
@@ -172,6 +293,7 @@ public function testProxyWithInvalidHmac(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
@@ -199,6 +321,7 @@ public function testProxyWithMissingHmacParameters(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);
 
@@ -231,6 +354,7 @@ public function testProxyWithMessageNotOwnedByUser(): void {
 			$this->hmacGenerator,
 			$this->logger,
 			$this->mailManager,
+			$this->svgSanitizer,
 			$this->userId,
 		);