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

Escape HTML Tags from Linux Notifications (electron) #3564

Merged

Conversation

@t3chguy
Copy link
Collaborator

t3chguy commented Apr 4, 2017

https://developer.gnome.org/notification-spec/ lists HTML tags which cause special behaviour, but KDE/Plasma for instance simply removes anything that looks like a HTML Tag, https://github.com/electron/electron/blob/master/docs/tutorial/desktop-environment-integration.md#linux says that all Electron supported Linux notification systems (via libnotify) follow this markup spec. Examples of how this issue manifests below:

screenshot_20170403_103437

After fix (different message, this data would previously be in italics):

screenshot_20170403_105057


Tested on Windows for sanity's sake
Tested on KDE Neon (KDE/Plasma) 16.04

Needs testing on some other DMs/Distros

Sample test data:

  • <i>Foo</i>
  • <hello>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy force-pushed the t3chguy:t3chguy/malformed-notifications-gnome-kde branch from cfc7508 to 99923b7 Apr 12, 2017
@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented Apr 15, 2017

Tested on Mac, Windows, Linux (KDE, LXDE, GNOME, XFCE, MATE)

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 22, 2017

So this ends up with raw HTML tags being displayed in the notif? I'm confused if we're trying to show markup from the HTML message (in which case, why don't we just notify the non-HTML message), or are we trying to show literal HTML markup from a text message?

@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented Apr 22, 2017

@ara4n rather than raw HTML, its anything enclosed in < and > prior to this would be invisible in a notification on Linux, and <b> etc would actually make it stylized.
Inconsistent as it'd only happen in Linux and it had the potential to malform notifications to the point that they have no content.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 22, 2017

ok, so if the notif system takes the plain text message body (ie not the formatted one) and renders it as if it were HTML, don't we need to escape it fully? eg turn & into & etc? in which case we should use a proper html escaping function rather than just regexping the right angled brackets...

@t3chguy

This comment has been minimized.

Copy link
Collaborator Author

t3chguy commented Apr 22, 2017

As far as I am aware, it doesn't render it fully as HTML, just does basic tag parsing itself.

The following tags should be supported by the notification server. Though it is optional, it is recommended. Notification servers that do not support these tags should filter them out.
<b> ... </b>	Bold
<i> ... </i>	Italic
<u> ... </u>	Underline
<a href="..."> ... </a>	Hyperlink
<img src="..." alt="..."/>	Image
A full-blown HTML implementation is not required of this spec
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 22, 2017

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@ara4n ara4n merged commit 4864716 into vector-im:develop Apr 22, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 22, 2017

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.