We allow HTML in Notifications and QtQuick Text will even load remote images which poses a privacy threat.
The network access manager factory we install is ineffective as Plasma uses a shared engine nowadays and whenever a new QmlObject shared engine is created, its setupBindings will re-install the KIO access factory.
Details
Details
5.8 branch on Fabian's request as this is a security issue
Can no longer cause network requests by sending a notification with <img src="http://..."> or <span style="background: url(http://...)">.
(Btw I noticed that setupBindings is called >100 times on Plasma startup, setting up the very same QML engine over and over again, including creating a KIO NAM factory, KLocalizedContext and KIcon image provider)
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| applets/notifications/plugin/textsanitizer.cpp | ||
|---|---|---|
| 45 ↗ | (On Diff #16691) | (Already noted on IRC) This doesn't do what it's intended to do, findBlockByNumber is needed here.
results in
|
Comment Actions
Addressing the text first:
The network access manager factory we install is ineffective as Plasma uses a shared engine nowadays and whenever a new QmlObject shared engine is created, its setupBindings will re-install the KIO access factory.
There used to be a .desktop flag for that..
(Btw I noticed that setupBindings is called >100 times on Plasma startup, setting up the very same QML engine over and over again, including creating a KIO NAM factory, KLocalizedContext and KIcon image provider)
calling setupBindings() is needed as we install the ki18n on the context (which is different), but you're right that the stuff with the engine could indeed be done once.
Comment Actions
hypothetically (and this would still require a Qt change) if we could override loadResource would that be enough for doing what this is doing?
| applets/notifications/plugin/textsanitizer.cpp | ||
|---|---|---|
| 41 | just make it on the stack? | |
| 59 | format might not be valid here, does that cause a problem? | |
| 60 | probably better to do do: !backgroundUrl.isEmpty() && !isLocalFile.. QTextFormat was still doing some processing at least. | |
| 86 | (untested) Make a QTextCursor on qqdoc->textDocument() and then you can insertFragment. Removing images becomes a lot easier then, as you just skip that fragment rather than trying to remove text. | |
Comment Actions
To be honest, having a look at https://developer.gnome.org/notification-spec/#markup we should just filter out everything other than b i u a and img with local src
What do you think?
Comment Actions
should just filter out everything other than b i u a and img with local src
Sure, but how would you achieve this? Regular expressions aren't suitable for HTML. It's not like I went this overly complicated way for no reason.
Comment Actions
By doing something similar to what https://cgit.kde.org/knotifications.git/tree/src/notifybypopup.cpp#n790 does?
This would fail with "invalid but webbrowser-parseable" html, but i guess we don't really care about it either, no?
Comment Actions
This would fail with "invalid but webbrowser-parseable" html, but i guess we don't really care about it either, no?
That wouldn't solve anything as <img src="whatever" > without </img> would fail the XML remover but still end up getting loaded.
You could need to signal that it wasn't strippable, and the QtQuick item could load in plain text, but then the patch is getting more complex.
My comment looks surprisngly sensible, I'll follow that up tomorrow eve and see if it works. I'll run the test fvgot has in the earlier comment.
Any reason we're not doing the filtering at the dataengine level?
It's accessing the text document of the TextEdit; which avoids a double text document parse.
Though for the size of a notification at the frequency we get them, that's not a huge problem.
Comment Actions
I think QXmlStreamReader may be the way to go, I wasn't aware on how strict the spec is wrt HTML support.
David, an <img> opening tag is still valid XHTML, you might receive a document ended prematurely error at the end since there won't be a closing tag but definitely worth a shot filtering out any tags other than <b> etc and stripping all attributes other than src with local URL, especially the style attribute.
Comment Actions
Also, if we did the filtering on dataengine side we could easily backport that to 5.8 since it hardly changed in the meantime whereas the applet underwent significant modifications.
Comment Actions
FYI, started making changes (as I know Kai is away). Will be done by tomorrow eve with autotest suite.