fix(message): render received external SVG images by joeldj-nl · Pull Request #13142 · nextcloud/mail

joeldj-nl · 2026-06-22T15:29:35Z

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 automatically detected during proxying and sanitized before being served.
- Sanitized SVGs are returned with the correct image/svg+xml content type; unsafe/invalid content is replaced with a blocked-image fallback.
Bug Fixes
- Improved handling of proxy responses that don’t return valid image payloads, reducing failures and ensuring consistent output.
Tests
- Added unit tests for SVG sanitization security rules and updated controller tests to verify sanitized SVG delivery.

External images are fetched through the image proxy and served with a generic application/octet-stream content type. Browsers sniff raster formats in <img> 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 <img> context does not execute scripts. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

ChristophWurst · 2026-06-23T11:03:27Z

Thanks for the contribution

ChristophWurst · 2026-06-23T11:04:00Z

@coderabbitai review and scan for new vulnerabilities

coderabbitai · 2026-06-23T11:06:43Z

🧩 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 · 2026-06-23T11:13:21Z

📝 Walkthrough

Walkthrough

A new SvgSanitizer service parses and filters SVG markup. ProxyController now validates proxied bodies, sanitizes detected SVG responses, centralizes blocked-image loading, and returns sanitized SVG or fallback content with appropriate response types.

Changes

SVG Sanitization Feature

Layer / File(s)	Summary
SvgSanitizer service `lib/Service/SvgSanitizer.php`	Adds XML parsing, unsafe element and attribute removal, DOCTYPE/ENTITY rejection, and sanitized SVG serialization.
ProxyController SVG response handling `lib/Controller/ProxyController.php`	Injects `SvgSanitizer`, validates response bodies, sanitizes SVG content, handles empty sanitization results, and centralizes blocked-image loading with `ServiceException` failures.
Sanitization and proxy integration tests `tests/Unit/Service/SvgSanitizerTest.php`, `tests/Unit/Controller/ProxyControllerTest.php`	Covers SVG filtering, unsafe markup rejection, safe graphics preservation, SVG response headers, and updated controller dependency construction.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: christophwurst, gretad, kesselb

🚥 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 clearly matches the main change: fixing rendering of received external SVG images.
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.

✨ 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

_{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 · 2026-06-23T11:16:59Z

Yes

- Extend URL attribute stripping to cover src, action, formaction - Strip external CSS url() references from <style> elements and style="" attributes while preserving fragment references (url(#...)) - Reject SVG payloads exceeding 2 MiB before DOM parsing (DoS guard) - Fall back to blocked-image.png instead of serving empty body when sanitizer rejects an SVG document - Drop misleading <!-- branch from looksLikeSvg() heuristic - Move looksLikeSvg() from ProxyController into SvgSanitizer service Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

joeldj-nl · 2026-06-23T11:33:40Z

The recommended changes have been implemented.

ChristophWurst · 2026-06-23T11:48:40Z

@coderabbitai re-review

coderabbitai · 2026-06-23T11:55:05Z

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]

github-actions · 2026-07-07T05:13:49Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

ChristophWurst

👍 otherwise

thanks

ChristophWurst · 2026-07-21T18:44:45Z

The branch needs an update because there is a conflict

Signed-off-by: Joël de Jager <contact@joeldejager.nl>

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

…oeldj-nl/ncmail into fix/render-received-svg-images

joeldj-nl · 2026-07-21T20:05:22Z

The branch needs an update because there is a conflict

Updated!

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

ChristophWurst · 2026-07-22T11:56:00Z

@coderabbitai review again

coderabbitai · 2026-07-22T11:56:11Z

@ChristophWurst The Comment images show the original rendering failure: one displays a broken-image icon with the alt text main.svg, and the other displays a grey placeholder labeled 400×600. I’ll re-review the updated branch, including the conflict resolution, for regressions and new security issues.

✅ 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

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 537ec8a0-7baa-4b21-b7c6-209350983019

📥 Commits

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

📒 Files selected for processing (1)

lib/Controller/ProxyController.php

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

joeldj-nl · 2026-07-22T12:41:55Z

@ChristophWurst the new findings by CodeRabbitAI have been resolved.

kesselb · 2026-07-24T16:35:41Z

Thanks for your pr 👍

We reject SVG's with XLS/Transform in server (c.f. nextcloud/server#59835) and should apply the same hardening here.

Signed-off-by: Joël de Jager <contact@joeldejager.nl>

joeldj-nl · 2026-07-24T18:32:44Z

@kesselb thanks for your input. This should be fixed now.

Copilot

Pull request overview

This PR fixes external SVG images not rendering in proxied email content by detecting SVG payloads and serving them as image/svg+xml, adding an SVG sanitization layer to reduce XSS/XXE risk when proxying untrusted markup.

Changes:

Add SvgSanitizer service to detect SVG markup and strip active/dangerous content.
Update ProxyController to read streamed proxy responses safely and to sanitize/serve SVG with the correct content type.
Add unit tests for the sanitizer and for proxying sanitized SVG responses.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`lib/Controller/ProxyController.php`	Reads stream bodies, detects SVG responses, sanitizes them, and serves with `image/svg+xml` (fallbacks to blocked image on unsafe SVG).
`lib/Service/SvgSanitizer.php`	New sanitizer that parses SVG with libxml hardening and strips scripts/event handlers/external references.
`tests/Unit/Controller/ProxyControllerTest.php`	Adds test coverage for streamed bodies and SVG sanitization/content-type behavior in the proxy.
`tests/Unit/Service/SvgSanitizerTest.php`	Adds unit tests for sanitizer security rules and SVG detection heuristics.

+	private function stripCssUrls(string $css): string {
+		return preg_replace('/url\s*\((?!\s*[\'"]?#)[^)]*\)/i', 'none', $css) ?? $css;
+	}


kesselb · 2026-07-31T17:34:10Z

+
+		$xpath = new DOMXPath($dom);
+
+		// Remove processing instructions (e.g. <?xml-stylesheet type="text/xsl"?>).


The ?> ends php and make the linter fails.

kesselb · 2026-07-31T17:35:42Z

+	 *                parsed safely
+	 */
+	public function sanitize(string $svg): string {
+		if (trim($svg) === '' || strlen($svg) > self::MAX_SVG_BYTES) {


Please take a look at nextcloud/server#62162 and check if we have those cases covered.

kesselb · 2026-07-31T17:47:12Z

@@ -95,17 +101,49 @@ public function proxy(string $src, ?int $id, ?string $hmac): Response {
 		try {
 			$response = $client->get($src);
 			$content = $response->getBody();


$content is only a resource with a call like $client->get($src, ['stream' => true]). The actual return for the current request is string.

Please drop is_resource fallback.

To make static code analysis happy add:

// Not a stream request, so the body is a string, never a resource. assert(is_string($content));

joeldj-nl requested review from ChristophWurst, GretaD and kesselb as code owners June 22, 2026 15:29

ChristophWurst added 3. to review enhancement labels Jun 23, 2026

coderabbitai Bot reviewed Jun 23, 2026

View reviewed changes

github-actions Bot added the feedback-requested label Jul 7, 2026

ChristophWurst reviewed Jul 15, 2026

View reviewed changes

Comment thread lib/Controller/ProxyController.php Outdated

ChristophWurst reviewed Jul 15, 2026

View reviewed changes

joeldj-nl added 3 commits July 21, 2026 21:45

Merge branch 'main' into fix/render-received-svg-images

23b68c9

Signed-off-by: Joël de Jager <contact@joeldejager.nl>

fix(proxy): handle blocked image read failures

80a7a9e

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

Merge branch 'fix/render-received-svg-images' of https://github.com/j…

3bef3c7

…oeldj-nl/ncmail into fix/render-received-svg-images

joeldj-nl requested a review from ChristophWurst July 22, 2026 08:38

ChristophWurst reviewed Jul 22, 2026

View reviewed changes

Comment thread lib/Controller/ProxyController.php Outdated

fixup! fix(message): render received external SVG images

a47fda5

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

joeldj-nl requested a review from ChristophWurst July 22, 2026 11:21

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread lib/Controller/ProxyController.php Outdated

Comment thread lib/Controller/ProxyController.php

Comment thread lib/Controller/ProxyController.php

fixup! fix(message): render received external SVG images

431b9bf

Assisted-by: Codex:gpt-5 Signed-off-by: Joël de Jager <contact@joeldejager.nl>

fixup! fix(proxy): close SVG sanitizer security gaps

c637d51

Signed-off-by: Joël de Jager <contact@joeldejager.nl>

kesselb requested a review from Copilot July 31, 2026 17:19

Copilot started reviewing on behalf of kesselb July 31, 2026 17:20 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Comment thread lib/Controller/ProxyController.php

Comment thread lib/Service/SvgSanitizer.php

Comment on lines +160 to +162

private function stripCssUrls(string $css): string {

return preg_replace('/url\s*$(?!\s*[\'"]?#)[^)]*$/i', 'none', $css) ?? $css;

}

kesselb reviewed Jul 31, 2026

View reviewed changes


		$xpath = new DOMXPath($dom);

		// Remove processing instructions (e.g. <?xml-stylesheet type="text/xsl"?>).

Uh oh!

Conversation

joeldj-nl commented Jun 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Summary by CodeRabbit

Release Notes

Uh oh!

ChristophWurst commented Jun 23, 2026

Uh oh!

ChristophWurst commented Jun 23, 2026

Uh oh!

coderabbitai Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔴 Security findings

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

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

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

🟡 Minor / behavioural findings

4. Empty body served on sanitizer rejection

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

✅ What looks good

Uh oh!

coderabbitai Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristophWurst commented Jun 23, 2026

Uh oh!

joeldj-nl commented Jun 23, 2026

Uh oh!

ChristophWurst commented Jun 23, 2026

Uh oh!

coderabbitai Bot commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jul 21, 2026

Uh oh!

joeldj-nl commented Jul 21, 2026

Uh oh!

Uh oh!

ChristophWurst commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joeldj-nl commented Jul 22, 2026

Uh oh!

kesselb commented Jul 24, 2026

Uh oh!

joeldj-nl commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

joeldj-nl commented Jun 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading

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

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

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

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

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading