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

Improve html message rendering #4003

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Nov 5, 2020

Fixes #3076

  1. Allow more CSS properties in html messages: see doSetupTricky() and doSetupProprietary() for details.
  2. Proxy Content-Type header (fallback to application/octet-stream). Some browsers (e.g. chromium based) refuse to display svgs in img elements unless they are served with the correct Content-Type header.

EDIT: 2. is not secure

@st3iny
Copy link
Member Author

st3iny commented Nov 5, 2020

I tested with real world newsletter/notification emails in my personal mail account and most of them are displayed correctly now. More testing is appreciated.

cc @MrManor @jancborchardt

@ChristophWurst
Copy link
Member

http://htmlpurifier.org/live/configdoc/plain.html

This parameter determines whether or not to allow "tricky" CSS properties and values. Tricky CSS properties/values can drastically modify page layout or be used for deceptive practices but do not directly constitute a security risk. For example, display:none; is considered a tricky property that will only be allowed if this directive is set to true.

👍

Whether or not to allow proprietary elements and attributes in your documents, as per HTMLPurifier_HTMLModule_Proprietary. Warning: This can cause your documents to stop validating!

👍 I'm fine with slightly invalid HTML. The browsers should be able to handle it.

@ChristophWurst
Copy link
Member

So I'm generally for this change, but we should be careful with the mime types and only allow a few safe ones that are used for common image types.

@ChristophWurst ChristophWurst added this to the v1.7 milestone Nov 5, 2020
@ChristophWurst
Copy link
Member

/backport to stable1.6

@st3iny st3iny force-pushed the fix/3076/improve-html-message-rendering branch from 606d236 to 175240a Compare November 5, 2020 14:18
@ChristophWurst
Copy link
Member

Some browsers (e.g. chromium based) refuse to display svgs in img elements unless they are served with the correct Content-Type header.

Now I'm not sure if we actually want to allow this. Did you see legit emails that use svg? There's some security implications with those. I'm checking with @rullzer if allowing them is OK or not.

I've found https://css-tricks.com/a-guide-on-svg-support-in-email/ on this tipic and it sounds like we'd not be the only client to not render the SVGs.

@st3iny
Copy link
Member Author

st3iny commented Nov 5, 2020

The notifications sent by the nextcloud server contain svgs.

Turns out that svgs are a security threat because they may contain scripts. If we really want to proxy svgs we should sanitize them.

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the fix/3076/improve-html-message-rendering branch from 175240a to 71931bc Compare November 5, 2020 15:03
@ChristophWurst
Copy link
Member

The notifications sent by the nextcloud server contain svgs.

Oops. With the insights from https://css-tricks.com/a-guide-on-svg-support-in-email/ it sounds like we should fix that in server.

Turns out that svgs are a security threat because they may contain scripts. If we really want to proxy svgs we should sanitize them.

Indeed. I see you updated your branch already. Let's skip the potentially dangerous SVG part for now. We can think about sanitization in a follow-up step.

@ChristophWurst ChristophWurst merged commit 2a4f99f into master Nov 5, 2020
@ChristophWurst ChristophWurst deleted the fix/3076/improve-html-message-rendering branch November 5, 2020 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some issues with HTML mail display
2 participants