From ccb4252b582ca25e3764c284208f4207e7f78c6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20de=20Jager?= Date: Mon, 22 Jun 2026 16:27:45 +0200 Subject: [PATCH 1/2] fix(message): render received external SVG images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External images are fetched through the image proxy and served with a generic application/octet-stream content type. Browsers sniff raster formats in tags, but they refuse to render SVG unless it is served as image/svg+xml, so SVG logos stayed blank and only the alt text and placeholder were shown. Detect SVG markup in the proxied response and serve it as a sanitised image/svg+xml document. A new SvgSanitizer service strips active content (scripts, event handlers, external/javascript references) and rejects DOCTYPE/entity declarations as XXE defence in depth, even though the context does not execute scripts. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Joël de Jager --- lib/Controller/ProxyController.php | 30 +++++ lib/Service/SvgSanitizer.php | 105 ++++++++++++++++++ tests/Unit/Controller/ProxyControllerTest.php | 64 +++++++++++ tests/Unit/Service/SvgSanitizerTest.php | 82 ++++++++++++++ 4 files changed, 281 insertions(+) create mode 100644 lib/Service/SvgSanitizer.php create mode 100644 tests/Unit/Service/SvgSanitizerTest.php diff --git a/lib/Controller/ProxyController.php b/lib/Controller/ProxyController.php index 3b44a15254..812037b4f2 100644 --- a/lib/Controller/ProxyController.php +++ b/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 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-) 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, '/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('/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. ). + 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); + } + } + } +} diff --git a/tests/Unit/Controller/ProxyControllerTest.php b/tests/Unit/Controller/ProxyControllerTest.php index ee6ed5cf0a..0c956fb6b7 100644 --- a/tests/Unit/Controller/ProxyControllerTest.php +++ b/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 = ''; + $sanitized = ''; + $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, ); diff --git a/tests/Unit/Service/SvgSanitizerTest.php b/tests/Unit/Service/SvgSanitizerTest.php new file mode 100644 index 0000000000..f4d576a9e9 --- /dev/null +++ b/tests/Unit/Service/SvgSanitizerTest.php @@ -0,0 +1,82 @@ +sanitizer = new SvgSanitizer(); + } + + public function testRemovesScript(): void { + $svg = ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('assertStringContainsString('sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('onload', $result); + $this->assertStringContainsString('width="10"', $result); + } + + public function testRemovesExternalReference(): void { + $svg = '' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('evil.example', $result); + $this->assertStringContainsString(''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringContainsString('#icon', $result); + } + + public function testRejectsDoctypeAndEntities(): void { + $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 = '' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringContainsString('assertStringContainsString('fill="red"', $result); + } +} From f9279d60e804e8d96ec60eaaa6727248b2bb35ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20de=20Jager?= Date: Tue, 23 Jun 2026 13:31:46 +0200 Subject: [PATCH 2/2] fix(proxy): close SVG sanitizer security gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extend URL attribute stripping to cover src, action, formaction - Strip external CSS url() references from ' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringNotContainsString('tracker.example', $result); + $this->assertStringContainsString('rect { fill: red; stroke: blue }' + . ''; + + $result = $this->sanitizer->sanitize($svg); + + $this->assertStringContainsString('fill: red', $result); + } + + public function testRejectsOversizedInput(): void { + $svg = str_repeat('a', 2 * 1024 * 1024 + 1); + + $this->assertSame('', $this->sanitizer->sanitize($svg)); + } + + public function testLooksLikeSvgWithDirectSvgTag(): void { + $this->assertTrue($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgWithXmlPrologue(): void { + $this->assertTrue($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgReturnsFalseForHtml(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg('')); + } + + public function testLooksLikeSvgReturnsFalseForRasterImage(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg("\x89PNG\r\n")); + } + + public function testLooksLikeSvgReturnsFalseForHtmlComment(): void { + $this->assertFalse($this->sanitizer->looksLikeSvg('')); + } }