The body of a notification may also contain hyperlinks

[rbuj]

Tests:

$ notify-send "title" "<a href=\"https://mate-desktop.org/es/\">MATE</a> <i>italic</i> <b>bold</b>"
$ notify-send "title" "<a href=\"https://mate-desktop.org/es/\">MATE</a> <i>italic</i> <b>bold</b> <qwert>asdf</qwert>"
$ notify-send "title" "<i>italic</i> <b>bold</b>"

Closes #118

[lukefromdc]

What exactly does this fix or do? Title of this PR just describes what GTK is doing

[rbuj]

The body of a notification may also contain hyperlinks.

I'm sorry, I'll change the title.

The current code only checks for any errors in the text that will be displayed on the body section which is rendered in a GtkLabel, but it can also use the element a (HTML hyperlink) in addition to the Pango Markup Language.

[rbuj]

The message body is also quoted from now.

[cwendling]

wait… does this new version really still allows e.g. links -- or any markup, actually? notify-send '<a href="foo.bar">test</a>' I didn't test yet, but I would expect getting the literal markup with this, doesn't it?

[rbuj]

undo the last change as it did not render the message body

[rbuj]

@cwendling where is the failure? It's renderend as markup text:

$ notify-send "title" "<i>italic</i> <b>bold</b> &gt; &lt; &amp;" 

It's renderend as plain text:

$ notify-send "title" "<i>italic</i> <b>bold</b> > < &"

[rbuj]

test: well formated

$  notify-send "title" "<i>italic</i> <b>bold</b> <a href=\"uri\">link</a>."

test: plain

$  notify-send "title" "<i>italic</i> <b>bold</b> <a href=\"uri\">link</a>. <test>The test</test>"

test: unescaped

$ notify-send "title" "<i>italic</i> <b>bold</b> <a href=\"test\">test</a> << >>"

[cwendling]

@cwendling where is the failure?

You fixed it since then reverting a change :)

But the problem with <img> is that, according to the spec (as linked in https://github.com/mate-desktop/mate-notification-daemon/issues/118#issuecomment-271165021):

Notification servers that do not support these tags should filter them out.

This means your approach of displaying as plain text if GtkLabel fails to parse does not fix cases where GtkLabel doesn't handle the markup -- and that's the case for <img> tags. The spec is clear that given e.g. <i>foo</i> <a href="bar.baz">bar</a> <img src="bar.baz/img" alt=""/>, we should render it as:

foo bar

as we don't support images, not as:

<i>foo</i> <a href="bar.baz">bar</a> <img src="bar.baz/img" alt=""/>

Really, what we should do is follow the spec and filter-out stuff we don't know. Yes, handling invalid XML as plain text makes sense, but valid XML with unsupported tags (especially ones from the spec) should still be supported.

[rbuj]

@cwendling done

[raveit65]

Last commit breaks standard notification theme, here with menta gtk-theme.

[rbuj]

@raveit65 Did you restart the m-s-d daemon?

[rbuj]

The standard theme seems that it's not working... WIP

[raveit65]

Same result after restarting m-s-d daemon. But i think the notification daemon is not always running and independent from m-s-d daemon. It is only called via dbus from the application.

[raveit65]

Btw. this is with f30.

[rave@mother ~]$ rpm -qa libxml2
libxml2-2.9.9-2.fc30.x86_64

[rbuj]

@raveit65 I think it's fixed now.

diff --git a/src/themes/standard/Makefile.am b/src/themes/standard/Makefile.am
index 9ae648a..42476a4 100644
--- a/src/themes/standard/Makefile.am
+++ b/src/themes/standard/Makefile.am
@@ -5,7 +5,7 @@ engine_LTLIBRARIES = libstandard.la
 libstandard_la_SOURCES = theme.c
 libstandard_la_CFLAGS = $(WARN_CFLAGS)
 libstandard_la_LDFLAGS = -module -avoid-version -no-undefined
-libstandard_la_LIBADD  = $(NOTIFICATION_DAEMON_LIBS)
+libstandard_la_LIBADD  = $(NOTIFICATION_DAEMON_LIBS) $(THEME_LIBS)

 AM_CPPFLAGS = $(NOTIFICATION_DAEMON_CFLAGS) $(THEME_CFLAGS)

[raveit65]

@cwendling Any concerns left or do you like to approve it?

[raveit65]

For some notification are hanging now sometimes and they run with high cpu load, most time fedoras abrt notification triggers the issue.

[root@mother rave]# ps aux | grep notification
rave     10991  100  0.1 569620 46288 ?        Rsl  15:14   0:07 /usr/libexec/mate-notification-daemon
root     11001  0.0  0.0 215732   892 pts/2    S+   15:14   0:00 grep --color=auto notification
[root@mother rave]# top -p 10991

top - 15:14:54 up  3:14,  1 user,  load average: 0,88, 0,76, 0,56
Tasks:   1 total,   1 running,   0 sleeping,   0 stopped,   0 zombie
%Cpu(s):  7,5 us,  0,5 sy,  0,0 ni, 91,4 id,  0,5 wa,  0,1 hi,  0,1 si,  0,0 st
MiB Mem :  32118,2 total,  20841,1 free,   2764,7 used,   8512,4 buff/cache
MiB Swap:      0,0 total,      0,0 free,      0,0 used.  28059,8 avail Mem 

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND    
10991 rave      20   0  569780  46424  28760 R 100,0   0,1   0:50.44 mate-noti+ 

I need to find out which commit cause the issue. I am testing https://github.com/mate-desktop/mate-notification-daemon/pull/172 too. So please do not merge for the moment.

[rbuj]

I didn't see any CPU high consumption on the stress test below:

test-notify-send.sh

#!/usr/bin/bash
notify-send -u critical -i dialog-information -t 1000 "Hello" "<i>italic</i> <b>bold</b> World!!\nLorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
sleep 1s

$ time seq 1 150 | parallel -j 5 ./test-notify-send.sh
real    0m33,243s
user    0m0,974s
sys 0m2,076s

[raveit65]

You need to trigger a notification from abrt (fedora bug reporting tool) Try to confirm this issue https://github.com/mate-desktop/mate-panel/issues/1017

1. remove all locations from clock-applet
2. restart the panel
3. add a new location
4. boom, clock-applet crashes and trigger a abrt alarm with notification If you use a self compiled rpm you need to edit abrt-action-save-package-data.conf

   
   [root@mother rave]# cat /etc/abrt/abrt-action-save-package-data.conf 
   # Configuration for the ABRT daemon
   # ---------------------------------
   #
   # This config file is empty by default which means that the preconfigured
   # default values will be used by ABRT. If you wish to override the default
   # settings, you can do so here.
   #
   # For details regarding the contents of this file and the defaults, see
   # `man 5 abrt-action-save-package-data.conf`.

OpenGPGCheck = no

Otherwise abrt will ignore your self compiled rpm.
Or `ProcessUnpackaged = yes/no` for packages installed by hand, see man abrt-action-save-package-data.conf

[rbuj]

You can send SIGSEGV to any process for triggering ABRT.

[robert@localhost ~]$ ps ax | grep marco
   1417 ?        Sl     0:00 marco
   1961 pts/1    S+     0:00 grep --color=auto marco
[robert@localhost ~]$ kill -SIGSEGV 1417

[raveit65]

cool, nice hint. I could solve the problem by rebuilding with your update from other PR. https://github.com/mate-desktop/mate-notification-daemon/pull/172

[cwendling]

I think the code should really be factorized rather than be duplicated in all themes. It's not a super-trivial piece of code anymore, so IMO it really warrant a helper function.

Also, ideally I think the code should filter nodes it accepts (e.g. Pango markup + a) rather than filter out nodes it knows it doesn't handle (img here). This would be a lot more robust, e.g. with random HTML tags. Never Trust User Input™. E.g. in overly-simplified pseudo-code:

valid_tags = [TEXT_NODE, 'a', 'b', 'big', 'i', 's', 'span', 'sup', 'sub', 'small', 'tt', 'u']
for node in input:
    if node.type not in valid_tags:
        del node

[rbuj]

@cwendling interesting ... but I don't see the pseudocode for validating the elements' attributes, do you think they don't need be validated? Have you considered defining a dtd or to use a xslt? ...

I think we don't need to implement a new DOM parser for GtkLabel markup which may conflict with gtk_label_set_markup

[rbuj]

Do you agree to validate the notification body twice consuming unnecessary time and resources?

[cwendling]

@cwendling interesting ... but I don't see the pseudocode for validating the elements' attributes, do you think they don't need be validated? Have you considered defining a dtd or to use a xslt? ...

I didn't think attributes mattered much, but maybe the do. Using a DTD is tempting, and libxml should be able to do the validation right away. However, we don't only want to validate the input, but strip the unsupported tags (at least that's what I understand the spec suggests), so we support notifications with e.g. img or other unknown tags, just without the specific behavior of those tags.

Basically what we want is that for any tag that don't validate the DTD, replace it with its child nodes (e.g. the text node inside, or anything else), and do that recursively (check the contents as well). I'm not sure libxml has this kind of built-in API that given a DTD and an XML input it gives you the valid part of the tree, or tell you what is or isn't valid?

Alternatively we could be less forgiving and have DTD for the spec, and only strip img tags as we know the support the others from the spec. And refuse anything that doesn't validate the DTD.

BTW I see that my pseudo-code example was covering more than it should, as I used the Pango markup as a base, whereas I should have used the spec, which only allows for b, i, u, a and img.

I think we don't need to implement a new DOM parser for GtkLabel markup which may conflict with gtk_label_set_markup

Here again, we don't only need to validate, but strip stuff. And for validating it could be nice to use GtkLabel's API, but AFAICT it doesn't have any: what you used before is apparently a side effect of the implementation, but nothing seems to suggest it was intentional.

Do you agree to validate the notification body twice consuming unnecessary time and resources?

I really don't think it matters, especially not when trading this for better safety and better support of the spec.

[rbuj]

@cwendling I don't want to waste more time with this. Do you see any impediment to merge this PR? After that, I will be happy to evaluate your RFE with your PR.

[sc0w]

New warnings in the logs with this PR:

coco-theme.c:531:3: warning: Value stored to 'size' is never read
                size = xmlNodeDump(buf, doc, xmlDocGetRootElement (doc), 0, 0);
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

nodoka-theme.c:916:3: warning: Value stored to 'size' is never read
                size = xmlNodeDump(buf, doc, xmlDocGetRootElement (doc), 0, 0);
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

theme.c:540:3: warning: Value stored to 'size' is never read
                size = xmlNodeDump(buf, doc, xmlDocGetRootElement (doc), 0, 0);
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

theme.c:888:3: warning: Value stored to 'size' is never read
                size = xmlNodeDump(buf, doc, xmlDocGetRootElement (doc), 0, 0);
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[rbuj]

@sc0w done

[cwendling]

@cwendling I don't want to waste more time with this.

Fair enough.

Do you see any impediment to merge this PR?

I'm frightened by the code duplication. Would it be hard to move it to a helper function instead?

[rbuj]

@cwendling I don't think that the best option is to use a helper function. Feel free to open a new PR. I prefer a subclass.

[raveit65]

Just rebased to master to fix travis archlinux build.

[raveit65]

I am fine with merging it like it is, a refactoring can be done with another PR. This is a long outstanding fix and should be cherry-picked to stable 1.24 branch. @cwendling OK?

[cwendling]

If you want, I'm just not sure this refactoring will happen… OTOH that might be a incentive to make me do it :D

I just quickly hacked an XSLT stylesheet as per @rbuj's idea, and that should do the work (disclaimer: I'm not very good at XSLT, but I think it should work):

<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">
<xsl:output method="xml" indent="no" encoding="UTF-8"/>

<!-- helper template to copy a node and apply templates on its children -->
<xsl:template name="copy-and-recurse">
  <xsl:copy>
    <xsl:apply-templates select="@* | node()"/>
  </xsl:copy>
</xsl:template>

<!-- text nodes -->
<xsl:template match="node()">
  <xsl:call-template name="copy-and-recurse"/>
</xsl:template>

<!-- img is not supported yet, replace it by its alternative text, if any -->
<xsl:template match="img">
  <xsl:value-of select="@alt"/>
</xsl:template>

<!-- links -->
<xsl:template match="a | a/@href">
  <xsl:call-template name="copy-and-recurse"/>
</xsl:template>

<!-- bold, italics, underline -->
<xsl:template match="b | i | u">
  <xsl:call-template name="copy-and-recurse"/>
</xsl:template>

<!-- removes unsupported tags and attributes -->
<xsl:template match="* | @*">
  <xsl:apply-templates select="node()"/>
</xsl:template>

</xsl:stylesheet>

It seems to work, given the input:

<root><!-- XML data requires a root element, this will be stripped -->

<a href="foo" alt="whatever">hello</a>
<i title="test">bar</i> lala

<img src="foo" alt="bar"/>

test: <zz>yeah</zz>
<yy>
  <a href="lala">lala.com</a>
  <b>important</b>
</yy>

</root>

…and processing it (here using xsltproc as an example, but we'd do that with libxslt):

$ xsltproc notification.xsl input.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!-- XML data requires a root element, this will be stripped -->

<a href="foo">hello</a>
<i>bar</i> lala

bar

test: yeah

  <a href="lala">lala.com</a>
  <b>important</b>

I'm not 100% sure it's the best approach, but it has the nice property of separating the rules and the code applying them, so that might be a good way to go. An it's fairly easy to alter as needed in the future.

[raveit65]

If you want,.....

No, i am happy with current state of PR :P

[rbuj]

ping

[cwendling]

FWIW I have an impl of my suggestion almost ready, I'll try and finish it "soon" whenever I have time and propose it.

[rbuj]

@cwendling Nice, today I received a notification which is related to the days I contributed to LO with XSL https://bugs.documentfoundation.org/show_bug.cgi?id=66305