fix(message): render received external SVG images

[joeldj-nl]

Summary

Received emails containing external SVG images did not render. The message showed only the broken-image placeholder and the image's alt text or name. External images are fetched through the image proxy, which served every response as application/octet-stream. Browsers happily sniff raster formats (PNG/JPEG/GIF) out of an tag regardless of content type, but they refuse to render SVG unless it is served as image/svg+xml, so SVG logos and graphics stayed blank.

Changes

• Detect SVG markup in proxied responses and serve it with the image/svg+xml content type so browsers render it.
• Add an SvgSanitizer service that strips active content (scripts, event handlers, external/javascript: references) and rejects DOCTYPE/entity declarations, as defence in depth against XXE and stored XSS even though the context does not execute scripts.

The first screenshot shows the issue; the second screenshot shows it has been resolved.

Assisted-by: Claude:claude-opus-4-8

Summary by CodeRabbit

Release Notes

• New Features

  • SVG content is now automatically detected and sanitized during proxying operations. Dangerous elements and inline handlers are removed while safe graphics and styling are preserved for secure display.

• Tests

  • Added comprehensive unit tests covering SVG sanitization behavior and security edge cases.

[ChristophWurst]

Thanks for the contribution

[ChristophWurst]

@coderabbitai review and scan for new vulnerabilities

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the list of changed files in this PR
gh pr view 13142 --repo nextcloud/mail --json files --jq '.files[].path'

Length of output: 416

---

🏁 Script executed:

#!/bin/bash
# Read the new SvgSanitizer and any modified proxy/controller files
fd -e php . lib/ --exec echo {} \; | grep -i 'svg\|proxy\|image' | sort
cat -n lib/Service/SvgSanitizer.php 2>/dev/null || echo "NOT FOUND"

Length of output: 4248

---

🏁 Script executed:

#!/bin/bash
# Find and read all relevant new/changed files
fd -e php . lib/ | xargs grep -l -i 'svg\|SvgSanitizer' 2>/dev/null | sort

# Also find proxy-related files
fd -e php . lib/ | xargs grep -l -i 'proxy\|imageproxy\|image_proxy' 2>/dev/null | sort

Length of output: 730

---

🏁 Script executed:

#!/bin/bash
cat -n lib/Controller/ProxyController.php

Length of output: 6202

---

🏁 Script executed:

#!/bin/bash
cat -n tests/Unit/Service/SvgSanitizerTest.php
cat -n tests/Unit/Controller/ProxyControllerTest.php

Length of output: 14218

---

---

@ChristophWurst Overall the implementation is solid — HMAC gating, LIBXML_NONET, DOCTYPE/entity rejection, event-handler stripping, and namespace-aware element removal are all good defence-in-depth measures. A few security gaps are worth closing before merge:

---

🔴 Security findings

1. src attribute not stripped (SVG <image> – tracking / SSRF when navigated directly)

SvgSanitizer::stripDangerousAttributes() only checks href and xlink:href. SVG 2.0 <image> also accepts a plain src attribute that some browsers honour. When a user opens the proxy URL directly (non-<img> context), an unstripped <image src="https://tracker.example/pixel.gif"/> would trigger an outbound request.

// lib/Service/SvgSanitizer.php  Line 100
if (in_array($name, ['href', 'xlink:href'], true) && !str_starts_with($value, '#')) {

Suggested fix – extend the stripped attribute list and tighten the check to cover all URL-bearing SVG attributes:

private const URL_ATTRIBUTES = ['href', 'xlink:href', 'src', 'action', 'formaction'];

// in stripDangerousAttributes():
if (in_array($name, self::URL_ATTRIBUTES, true) && !str_starts_with($value, '#')) {
    $element->removeAttributeNode($attribute);
    continue;
}

---

2. <style> elements and style="" attributes not sanitized (CSS url() exfiltration on direct navigation)

The sanitizer removes dangerous elements and attributes but leaves <style> blocks and inline style="" attributes intact. CSS url() expressions can load external resources when the SVG is opened directly via the proxy URL (not as an <img> source). Example:

<svg xmlns="http://www.w3.org/2000/svg">
  <style>rect { fill: url(https://tracker.example/pixel.gif) }</style>
  <rect width="100" height="100"/>
</svg>

Suggested fix – add style to FORBIDDEN_ELEMENTS or walk <style> text nodes and strip url(...) expressions that reference non-fragment URLs:

private const FORBIDDEN_ELEMENTS = [
    'script', 'style',           // ← add style
    'foreignObject', 'handler', 'listener', 'set',
];

If preserving safe style rules matters (e.g., fill: red), a regex pass over <style> text content can strip only url(...) values that do not begin with #.

---

3. No size limit before DOMDocument::loadXML() (potential DoS)

A malicious upstream server could return a huge payload that passes looksLikeSvg(). loadXML() will attempt to hold the whole document in memory. Consider capping the input size before parsing:

// lib/Service/SvgSanitizer.php  – top of sanitize()
private const MAX_SVG_BYTES = 2 * 1024 * 1024; // 2 MiB

public function sanitize(string $svg): string {
    if (trim($svg) === '' || strlen($svg) > self::MAX_SVG_BYTES) {
        return '';
    }
    // …

---

🟡 Minor / behavioural findings

4. Empty body served on sanitizer rejection

When sanitize() returns '' (DOCTYPE detected, parse failure, etc.) the proxy still returns:

return new ProxyDownloadResponse($content, $src, 'image/svg+xml');
// ProxyController.php  Line 131 — $content is '' at this point

This sends a 200 with an empty image/svg+xml body instead of the blocked-image.png fallback. Consider:

if ($this->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');
}

---

5. looksLikeSvg() — <!-- prefix is overly broad

// ProxyController.php  Line 142
|| str_starts_with($start, '<!--')

Any response starting with an HTML comment (common in HTML pages) combined with any embedded <svg> element will pass through to the DOM parser, which will reject it and return ''. No security impact, but it means tracking-pixel HTML pages are unnecessarily parsed. Consider dropping the <!-- branch or checking that <svg follows before the first non-comment, non-PI token.

---

✅ What looks good

Concern	Status
HMAC validation before proxying	✅
LIBXML_NONET prevents network fetch during parse	✅
DOCTYPE / <!ENTITY pre-rejected by regex	✅
on* event-handler attributes stripped	✅
Namespace-aware element removal (local-name())	✅
External href / xlink:href stripped	✅
Rate-limiting (#[UserRateLimit])	✅
LocalServerException caught (SSRF guard)	✅
Test coverage for sanitizer + controller	✅

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new SvgSanitizer service is added that parses SVG via DOMDocument, rejects DOCTYPE/ENTITY payloads, removes forbidden active elements via XPath, and strips dangerous attributes. ProxyController is updated to inject this service, detect SVG responses with a heuristic, sanitize them, and return image/svg+xml instead of application/octet-stream.

Changes

SVG Sanitization in ProxyController

Layer / File(s)	Summary
SvgSanitizer service lib/Service/SvgSanitizer.php	New class with FORBIDDEN_ELEMENTS constant, sanitize() method (DOCTYPE/ENTITY rejection, XML parsing with LIBXML_NONET, XPath element removal, attribute stripping, serialization), and stripDangerousAttributes() removing on* handlers and non-fragment href/xlink:href.
ProxyController SVG detection and wiring lib/Controller/ProxyController.php	Adds SvgSanitizer constructor injection and property, updates proxy response path to cast body to string, detect SVG via new looksLikeSvg() heuristic (XML/comment prologues or <svg prefix), sanitize and serve as image/svg+xml, or fall back to application/octet-stream.
Unit tests tests/Unit/Service/SvgSanitizerTest.php, tests/Unit/Controller/ProxyControllerTest.php	New SvgSanitizerTest covers script removal, event handler stripping, external href blocking, same-document href preservation, DOCTYPE rejection, invalid markup, and safe attribute retention. ProxyControllerTest passes the SvgSanitizer mock to all controller constructions and adds an assertion for Content-Type: image/svg+xml.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 Hop hop, no scripts shall sneak through here,
I sniff each SVG with a twitching ear.
DOCTYPE? Rejected! onload? Away!
Only safe shapes and circles may stay.
With XPath and libxml I sanitize the stream,
And serve clean SVGs — a rabbit's dream! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(message): render received external SVG images' directly and specifically describes the main change—enabling SVG rendering in received emails by fixing the content-type issue, which is the core objective of the PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (1)

lib/Controller/ProxyController.php (1)

137-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move SVG detection heuristic out of the controller.

looksLikeSvg() is business logic and makes ProxyController less of a thin HTTP handler. Consider moving this heuristic into SvgSanitizer (or a dedicated detector service) and call it from here.

As per coding guidelines, lib/Controller/**/*.php: “Controllers must be thin HTTP handlers; business logic must live in services”.

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 26c52dd2-524b-4c89-9e7f-305d15c812e0

📥 Commits

Reviewing files that changed from the base of the PR and between 9808f3e and ccb4252.

📒 Files selected for processing (4)

• lib/Controller/ProxyController.php
• lib/Service/SvgSanitizer.php
• tests/Unit/Controller/ProxyControllerTest.php
• tests/Unit/Service/SvgSanitizerTest.php

[ChristophWurst]

Yes

[joeldj-nl]

The recommended changes have been implemented.

[ChristophWurst]

@coderabbitai re-review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat -n lib/Service/SvgSanitizer.php
echo "---"
cat -n lib/Controller/ProxyController.php

Length of output: 11368

---

---

@ChristophWurst All five previously flagged issues have been resolved in the current implementation:

#	Finding	Status
1	src attribute not stripped	✅ Fixed — src, action, formaction added to URL_ATTRIBUTES constant (Line 34)
2	<style> / style="" CSS url() not sanitized	✅ Fixed — <style> text content and inline style attributes are both run through stripCssUrls(), which replaces external references with none while preserving same-document url(#…) fragments
3	No size limit before DOMDocument::loadXML()	✅ Fixed — 2 MiB cap via MAX_SVG_BYTES constant, checked before parsing (Line 45)
4	Empty image/svg+xml response on sanitizer rejection	✅ Fixed — ProxyController now checks $sanitized === '' and falls back to blocked-image.png with application/octet-stream
5	<!-- prefix in looksLikeSvg() overly broad	✅ Fixed — <!-- branch removed; detection now only triggers on <?xml or <svg prefix

The implementation is clean and all security measures are in place.

[approve]