D10188: Sanitise notification HTML

Fri Feb 2 16:16:16 UTC 2018

broulik added a comment.

  Thanks for taking care of this.

INLINE COMMENTS

> notificationsanitizer.cpp:45
> +
> +    QXmlStreamReader r(QStringLiteral("<html>") + t + QStringLiteral("</html>"));
> +    QString result;

We need a `QXmlStreamEntityResolver` like `KNotification` has otherwise HTML entities like `Ä` (for `Ä`) will error out.

> notificationsanitizer.cpp:72
> +
> +                out.writeAttribute(QStringLiteral("alt"), alt);
> +            }

Don't write `alt` if it doesn't have one?

> notificationsanitizer.h:2
> +/*
> + *   Copyright (C) 2017 David Edmundson <davidedmundson at kde.org>
> + *

2018

> notificationsengine.cpp:265
>      QString bodyFinal = (partOf == 0 ? body : _body);
> -    // First trim whitespace from beginning and end
> -    bodyFinal = bodyFinal.trimmed();
> -    // Now replace all \ns with <br/>
> -    bodyFinal = bodyFinal.replace(QLatin1String("\n"), QLatin1String("<br/>"));
> -    // Now remove all inner whitespace (\ns are already <br/>s
> -    bodyFinal = bodyFinal.simplified();
> -    // Finally, check if we don't have multiple <br/>s following,
> -    // can happen for example when "\n       \n" is sent, this replaces
> -    // all <br/>s in succsession with just one
> -    bodyFinal.replace(QRegularExpression(QStringLiteral("<br/>\\s*<br/>(\\s|<br/>)*")), QLatin1String("<br/>"));
> -    // This fancy RegExp escapes every occurence of & since QtQuick Text will blatantly cut off
> -    // text where it finds a stray ampersand.
> -    // Only &{apos, quot, gt, lt, amp}; as well as &#123 character references will be allowed
> -    bodyFinal.replace(QRegularExpression(QStringLiteral("&(?!(?:apos|quot|[gl]t|amp);|#)")), QLatin1String("&"));
> -    // The Text.StyledText format handles only html3.2 stuff and ' is html4 stuff
> -    // so we need to replace it here otherwise it will not render at all.
> -    bodyFinal.replace(QLatin1String("'"), QChar('\''));
> +    bodyFinal = NotificationSanitizer::parse(bodyFinal);
>  

Won't you end up with piles of `<html>` tags since `_body` is the body text of the notification it would group to.

  <html>
  <html>
  old notification
  </html>
  new notification
  </html>

Not that it really matters, though.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D10188

To: davidedmundson, #plasma, fvogt
Cc: broulik, aacid, fvogt, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180202/30521cdc/attachment.html>