Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVG logo URL embedded in emails return SVG content, despite useSvg=0 #13895

Closed
agross opened this issue Jan 29, 2019 · 28 comments
Closed

SVG logo URL embedded in emails return SVG content, despite useSvg=0 #13895

agross opened this issue Jan 29, 2019 · 28 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: theming needs info

Comments

@agross
Copy link

agross commented Jan 29, 2019

The logo I uploaded to Nextcloud via the Theming section is returned as SVG. The URL embedded in the mail template is: https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=21

$ curl 'https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=21'
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 303.11 59.689">...

Steps to reproduce

  1. Add SVG logo via Theming
  2. Send email (Activity Notification in my case)
  3. Open in Gmail or Spark (in my case)

Expected behaviour

Logo should be displayed

Actual behaviour

I get the broken image placeholder

Server configuration

Operating system: Linux/docker

Web server: nginx

Database: MariaDB

PHP version: 7.2.14

Nextcloud version: 15.0.2

Updated from an older Nextcloud/ownCloud or fresh install: oC 10 -> NC 12, 13, 14, 15. After that to logo was added.

Where did you install Nextcloud from: docker

List of activated apps:

App list
Enabled:
  - accessibility: 1.1.0
  - activity: 2.8.2
  - admin_audit: 1.5.0
  - bruteforcesettings: 1.3.0
  - camerarawpreviews: 0.6.3
  - checksum: 0.4.2
  - cloud_federation_api: 0.1.0
  - comments: 1.5.0
  - dav: 1.8.1
  - federatedfilesharing: 1.5.0
  - federation: 1.5.0
  - files: 1.10.0
  - files_external: 1.6.0
  - files_fulltextsearch: 1.2.3
  - files_pdfviewer: 1.4.0
  - files_rightclick: 0.11.0
  - files_sharing: 1.7.0
  - files_texteditor: 2.7.0
  - files_trashbin: 1.5.0
  - files_versions: 1.8.0
  - files_videoplayer: 1.4.0
  - firstrunwizard: 2.4.0
  - fulltextsearch: 1.2.3
  - fulltextsearch_elasticsearch: 1.2.2
  - gallery: 18.2.0
  - impersonate: 1.2.0
  - logreader: 2.0.0
  - lookup_server_connector: 1.3.0
  - nextcloud_announcements: 1.4.0
  - notifications: 2.3.0
  - oauth2: 1.3.0
  - password_policy: 1.5.0
  - provisioning_api: 1.5.0
  - serverinfo: 1.5.0
  - sharebymail: 1.5.0
  - support: 1.0.0
  - survey_client: 1.3.0
  - systemtags: 1.5.0
  - theming: 1.6.0
  - twofactor_backupcodes: 1.4.1
  - twofactor_totp: 2.1.0
  - updatenotification: 1.5.0
  - workflowengine: 1.5.0
Disabled:
  - encryption
  - user_ldap

Nextcloud configuration:

Config report
{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "version": "15.0.2.0",
        "installed": true,
        "maintenance": false,
        "singleuser": false,
        "htaccess.RewriteBase": "\/",
        "memcache.local": "\\OC\\Memcache\\APCu",
        "apps_paths": [
            {
                "path": "\/var\/www\/html\/apps",
                "url": "\/apps",
                "writable": false
            },
            {
                "path": "\/var\/www\/html\/custom_apps",
                "url": "\/custom_apps",
                "writable": true
            }
        ],
        "dbtype": "mysql",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "owncloud.home.therightstuff.de",
            "cloud.grossweber.com"
        ],
        "overwrite.cli.url": "https:\/\/owncloud.home.therightstuff.de",
        "loglevel": 2,
        "logtimezone": "Europe\/Berlin",
        "log_rotate_size": 10485760,
        "log_rotate_sizerotation": 3,
        "mail_smtpmode": "smtp",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpport": "587",
        "mail_smtpsecure": "tls",
        "mail_smtpauthtype": "LOGIN",
        "mail_smtpauth": 1,
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "skeletondirectory": "",
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "port": 6379,
            "password": "***REMOVED SENSITIVE VALUE***"
        },
        "trashbin_retention_obligation": "auto, 30",
        "versions_retention_obligation": "auto, 3",
        "trusted_proxies": "***REMOVED SENSITIVE VALUE***",
        "data-fingerprint": "68bc4c4bc6106a1c8c88e19ad0230ab7"
    }
}

Are you using external storage, if yes which one: No

Are you using encryption: No

Are you using an external user-backend, if yes which one: No

Logs

Web server log

Web server log
"GET /apps/theming/image/logo?useSvg=0&v=21 HTTP/1.1" 200 10336 "-" "Mozilla/5.0 (Windows NT 5.1; rv:11.0) Gecko Firefox/11.0 (via ggpht.com GoogleImageProxy)"

Nextcloud log (data/nextcloud.log)

Nextcloud log
Nothing of relevance
@agross agross added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Jan 29, 2019
@kesselb
Copy link
Contributor

kesselb commented Jan 29, 2019

Is the ImageMagick extension installed and enabled? If not the conversion is not possible.

@agross
Copy link
Author

agross commented Jan 29, 2019

@danielkesselberg Which extension? An app?

@kesselb
Copy link
Contributor

kesselb commented Jan 29, 2019

ImageMagick is a extension for php similar to GD but with support for SVG. If you are using the nextcloud/docke image this extension should be present (https://github.com/nextcloud/docker/blob/250802c70679b72457e1b85f301e1a503517f780/15.0/apache/Dockerfile#L65).

image

Do you see the notice above?

@agross
Copy link
Author

agross commented Jan 29, 2019

The docker container is based on nextcloud:apache. The notice above is displayed, but not highlighted, so I missed it. What needs to be done to enable the extension?

@kesselb
Copy link
Contributor

kesselb commented Jan 29, 2019

Ah. ImageMagick is included but without svg support. nextcloud/docker#594 should do the trick. I'm closing this here.

@kesselb kesselb closed this as completed Jan 29, 2019
@agross
Copy link
Author

agross commented Jan 30, 2019

I installed libmagickcore-6.q16-3-extra and I also get a favicon based on the logo (which is unreadable, BTW). But when requesting https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=24 I still get SVG contents.

@fuse314
Copy link

fuse314 commented Feb 1, 2019

I don't know if libmagickcore*-extra must be added before compiling imagemagick.
You can see the supported formats by imagemagick in phpinfo() ( search for ImageMagick supported formats ). It should display the SVG type there...

@kesselb
Copy link
Contributor

kesselb commented Feb 1, 2019

I don't know if libmagickcore*-extra must be added before compiling imagemagick.

Yes. A custom dockerfile is the way to go. There are concerns about the security of ImageMagick #13099. I understand this is a bit frustrating for new users 😞

@agross
Copy link
Author

agross commented Feb 1, 2019

I don't know if libmagickcore*-extra must be added before compiling imagemagick.

I added libmagickcore*-extra via apt in a custom Dockerfile. ImageMagick was already installed via nextcloud:apache (my base image).

I noticed a slight change in UI after adding the libmagickcore*-extra package which seems to be related to the SVG support being present. The favicon changes depending on the app I'm using:

  • When I browse Files, I see the Files app icon (a folder) instead of Nextcoud's favicon (three circles),
  • for Activity I see the bolt icon
  • etc.

From my perspective Nextcloud's image handling capabilities have been extended after adding the package via apt, without requiring to actually compile ImageMagick again.

@kesselb
Copy link
Contributor

kesselb commented Feb 3, 2019

$ curl -I "https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=24"
HTTP/2 200 
content-type: image/png
content-length: 10336

@agross
Copy link
Author

agross commented Feb 3, 2019

$ curl  "https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=24"
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 303.11 59.689"><path fill="#ffffff"
...

Correct Content-Type, but contents are SVG.

@kesselb
Copy link
Contributor

kesselb commented Feb 3, 2019

@juliushaertl 🤔

@kesselb kesselb reopened this Feb 3, 2019
@juliushaertl
Copy link
Member

The icon files are not regenerated automatically if the additional imagemagick package is installed, so you need to change some value of the theming app in order to trigger a rebuild of the icons.

@agross
Copy link
Author

agross commented Feb 3, 2019

so you need to change some value of the theming app in order to trigger a rebuild of the icons.

I removed the custom logo and added the one I downloaded before. The now-current URL is https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=27 and it's still returned as SVG. Is deleting + adding not enough of a change?

@agross
Copy link
Author

agross commented Feb 3, 2019

I also tried changing the Color value, still SVG. https://cloud.grossweber.com/apps/theming/image/logo?useSvg=0&v=29

@PalmtopTiger
Copy link

The problem is that ImageMagick rejects SVG without XML declaration. The class OC\Preview\SVG contains a workaround for this,

@agross
Copy link
Author

agross commented Feb 4, 2019

@PalmtopTiger Yep, adding the XML declaration and reuploading fixed it. Thanks for the pointer in the right direction!

Should we leave this issue open to track a general fix for the future?

@kesselb
Copy link
Contributor

kesselb commented Jan 16, 2020

Index: apps/theming/lib/Controller/ThemingController.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- apps/theming/lib/Controller/ThemingController.php	(revision 5de3ea04170afd25a31f249a922feb3f9b189242)
+++ apps/theming/lib/Controller/ThemingController.php	(date 1579201520818)
@@ -383,6 +383,14 @@
 			return new NotFoundResponse();
 		}
 
+		if ($useSvg) {
+			$content = $file->getContent();
+			if (strpos($content, '<?xml') !== 0) {
+				$content = '<?xml version="1.0" encoding="UTF-8" standalone="no"?>' . $content;
+				$file->putContent($content);
+			}
+		}
+
 		$response = new FileDisplayResponse($file);
 		$csp = new Http\ContentSecurityPolicy();
 		$csp->allowInlineStyle();

Something like that could work. But does that make sense? There is no conversion from png to svg. Just the Content-Type header is changed. It seems to me that something else is broken. A request with useSvg=0 should not return a svg.

cc @juliushaertl 😕

@skjnldsv
Copy link
Member

Paging Dr @juliushaertl 📟

@ghost
Copy link

ghost commented May 10, 2020

This issue has been automatically marked as stale because it has not had recent activity and seems to be missing some essential information. It will be closed if no further activity occurs. Thank you for your contributions.

@ghost ghost added the stale Ticket or PR with no recent activity label May 10, 2020
@agross
Copy link
Author

agross commented May 10, 2020

Marking issues stale does not solve them.

@ghost ghost removed the stale Ticket or PR with no recent activity label May 10, 2020
@skjnldsv
Copy link
Member

@agross but it ping everyone mentioned and make sure it's not forgotten ;)

@juliushaertl
Copy link
Member

I'd say we should rather check during upload of the logo if imagick actually considers it a valid svg than manually rewriting it as in the patch from #13895 (comment)

@ghost
Copy link

ghost commented Jun 14, 2020

This issue has been automatically marked as stale because it has not had recent activity and seems to be missing some essential information. It will be closed if no further activity occurs. Thank you for your contributions.

@ghost ghost added the stale Ticket or PR with no recent activity label Jun 14, 2020
@agross
Copy link
Author

agross commented Jun 14, 2020

Again, not gone stale.

@ghost ghost removed the stale Ticket or PR with no recent activity label Jun 14, 2020
@skjnldsv skjnldsv added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap needs info labels Jul 30, 2020
@kbzowski
Copy link

The problem still occurs in Nextcloud v23.0.2
#26871 looks like good solution but needs to be adapted to the current version.

@ChristianKrausse
Copy link

The problem still occurs in Nextcloud Hub 3 (25.0.1)

@szaimen
Copy link
Contributor

szaimen commented Jan 9, 2023

Hi, please update to 24.0.8 or better 25.0.2 and report back if it fixes the issue. Thank you!

@szaimen szaimen added needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 1. to develop Accepted and waiting to be taken care of labels Jan 9, 2023
@szaimen szaimen closed this as completed Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: theming needs info
Projects
None yet
Development

No branches or pull requests

10 participants