Bug 390452

Hi Jens, thanks a lot for this test. We understand that this is indeed a problem -- thanks for letting us know.

We're relying on the Qt framework's packaging of WebKit for HTML rendering. Can you please specify which Qt version and on which platform did you use in this test? The upstream situation with QtWebKit is, well, complicated, so this is an important piece of information for us. Please note that there are at least two implementations of "QtWebKit", one based on the official repositories and the other based on [1]. Some Linux distributions have switched to using this other WebKit.

It's documented [2] that DNS prefetching should be disabled by default in the official (and obsolete) Qt module. I also checked the code in latest upstream's git, and in there that attribute also defaults to false. We have so far relied on these effects (and some ad-hoc manual checks which were done at design & implementation time) to ensure that we don't leak these data. Too bad that it apparently fails now.

[1] https://github.com/annulen/webkit
[2] http://doc.qt.io/archives/qt-5.5/qwebsettings.html#WebAttribute-enum

For the tests we used Debian GNU/Linux 9.3 with the libqt5webkit5:amd64 (version 5.7.1+dfsg-1) package installed.

Note easy prefetching of <a href links or images does not work in Trojitá, neither does:
<link href="http://tracking-id.attacker.com" rel="prefetch">

But prefetching can be re-enabled either in the HTTP header (which is hard for email) or in the HTML content itself by the spammer:
<meta http-equiv="x-dns-prefetch-control" content="on">

I can reproduce it on OpenBSD -current with qtwebkit 5.9.0 (pretty sure this is still the official/original QtWebkit, not the fork). I started looking at some QtWebkit code to figure out what is going on but not done yet.

I looked at the QtWebkit source code and this piece of code caught my attention:

  4729	void Document::initDNSPrefetch()
  4730	{
  4731	    Settings* settings = this->settings();
  4732	
  4733	    m_haveExplicitlyDisabledDNSPrefetch = false;
  4734	    m_isDNSPrefetchEnabled = settings && settings->dnsPrefetchingEnabled() && securityOrigin()->protocol() == "http";
  4735	
  4736	    // Inherit DNS prefetch opt-out from parent frame    
  4737	    if (Document* parent = parentDocument()) {
  4738	        if (!parent->isDNSPrefetchEnabled())
  4739	            m_isDNSPrefetchEnabled = false;
  4740	    }
  4741	}
  4742	
  4743	void Document::parseDNSPrefetchControlHeader(const String& dnsPrefetchControl)
  4744	{
  4745	    if (equalIgnoringCase(dnsPrefetchControl, "on") && !m_haveExplicitlyDisabledDNSPrefetch) {
  4746	        m_isDNSPrefetchEnabled = true;
  4747	        return;
  4748	    }
  4749	
  4750	    m_isDNSPrefetchEnabled = false;
  4751	    m_haveExplicitlyDisabledDNSPrefetch = true;
  4752	}

Source: https://github.com/qt/qtwebkit/blob/5.9/Source/WebCore/dom/Document.cpp#L4729

The way I read the code: when initializing the DNS prefetch code, the settings
regarding DNS prefetching are taken into account (line 4734), but when DNS
prefetching is turned on by the "website", for instance with a header or the
http-equiv meta tag, the settings are ignored and DNS prefetching is turned on
regardless (line 4746).

To verify my assumption, I applied the diff found at the bottom to the QtWebkit
code and observed stdout while running trojita. When opening one of those
emails in Trojita, this is what I see on stdout:

    initDNSPrefetch(): DNS prefetching disabled in settings
    initDNSPrefetch(): DNS prefetching disabled in settings
    Processing HTTP equiv:
    parseDNSPrefetchControlHeader(): turning DNS prefetching on

This seems to confirm my understanding of the code. So the way I see it, there
is no easy, straightforward way for us to disable this behavior in (Qt)Webkit.
Do you agree with my analysis?



Index: Source/WebCore/dom/Document.cpp
--- Source/WebCore/dom/Document.cpp.orig
+++ Source/WebCore/dom/Document.cpp
@@ -28,6 +28,8 @@
 #include "config.h"
 #include "Document.h"
 
+#include <iostream>
+
 #include "AXObjectCache.h"
 #include "AnimationController.h"
 #include "Attr.h"
@@ -2824,6 +2826,8 @@ void Document::processHttpEquiv(const String& equiv, c
 
     Frame* frame = this->frame();
 
+    cout << "Processing HTTP equiv:" << endl;
+
     if (equalIgnoringCase(equiv, "default-style")) {
         // The preferred style set has been overridden as per section 
         // 14.3.2 of the HTML4.0 specification.  We need to update the
@@ -4732,6 +4736,10 @@ void Document::initDNSPrefetch()
 
     m_haveExplicitlyDisabledDNSPrefetch = false;
     m_isDNSPrefetchEnabled = settings && settings->dnsPrefetchingEnabled() && securityOrigin()->protocol() == "http";
+    if (settings && settings->dnsPrefetchingEnabled())
+        cout << "initDNSPrefetch(): DNS prefetching enabled in settings" << endl;
+    else
+        cout << "initDNSPrefetch(): DNS prefetching disabled in settings" << endl;
 
     // Inherit DNS prefetch opt-out from parent frame    
     if (Document* parent = parentDocument()) {
@@ -4743,6 +4751,7 @@ void Document::initDNSPrefetch()
 void Document::parseDNSPrefetchControlHeader(const String& dnsPrefetchControl)
 {
     if (equalIgnoringCase(dnsPrefetchControl, "on") && !m_haveExplicitlyDisabledDNSPrefetch) {
+        cout << "parseDNSPrefetchControlHeader(): turning DNS prefetching on" << endl;
         m_isDNSPrefetchEnabled = true;
         return;
     }

The way the code looks, injecting

<meta http-equiv="x-dns-prefetch-control" content="off">

to the top of any html mail should do.
Otherwise the behavior seems intended. You can configure the default behavior, but the webpage has the last word on this.

---
ceterum censeo: dropping html mail support would reliably fix this :-P

(In reply to Thomas Lübking from comment #5)
> The way the code looks, injecting
> 
> <meta http-equiv="x-dns-prefetch-control" content="off">
> 
> to the top of any html mail should do.

I thought of that as well this morning. Hopefully I'll have time tonight to submit a patch, let's see.

> ceterum censeo: dropping html mail support would reliably fix this :-P

Hehe.

Hello everyone,

At the bottom is a diff that contains my failed attempts at solving the problem.
Here is a description of those attempts:

1) Connect a slot to the QNetworkAccessManager::finished signal in
EmbeddedWebView that sets the "X-DNS-Prefetch-Control: off" header. Reason it
fails: the setRawHeader() method is protected so we cannot call it from
EmbeddedWebView.

2) Set the "X-DNS-Prefetch-Control: off" header in MsgPartNetworkReply. Reason
it fails: either QtWebkit does not see the header or it does not use it the
same way it uses an equivalent meta http-equiv tag. Either way, DNS prefetching
is not disabled.

3) Connect a slot to the QWebView::loadFinished signal in SimplePartWidget that
prepends the HTML with the meta http-equiv HTML tag to disable DNS prefetching.
Reason it fails: the page is rendered before the loadFinished signal is emitted
and the page us replaced. As such, the DNS requests are still performed before
the "fixed" page is put in place.

Does anyone have a better approach?

--

diff --git a/src/Gui/EmbeddedWebView.cpp b/src/Gui/EmbeddedWebView.cpp
index 6c530595..73a79d0b 100644
--- a/src/Gui/EmbeddedWebView.cpp
+++ b/src/Gui/EmbeddedWebView.cpp
@@ -75,6 +75,8 @@ EmbeddedWebView::EmbeddedWebView(QWidget *parent, QNetworkAccessManager *network
     setPage(new ErrorCheckingPage(this));
     page()->setNetworkAccessManager(networkManager);
 
+    connect(networkManager, &QNetworkAccessManager::finished, this, &EmbeddedWebView::slotReplyFinished);
+
     QWebSettings *s = settings();
     s->setAttribute(QWebSettings::JavascriptEnabled, false);
     s->setAttribute(QWebSettings::JavaEnabled, false);
@@ -177,6 +179,12 @@ void EmbeddedWebView::handlePageLoadFinished()
     page()->setLinkDelegationPolicy(QWebPage::DelegateAllLinks);
 }
 
+void EmbeddedWebView::slotReplyFinished(QNetworkReply *reply)
+{
+    // XXX: setRawHeader() is protected so we're unable to use it here
+//    reply->setRawHeader(QByteArrayLiteral("X-DNS-Prefetch-Control"), QByteArrayLiteral("off"));
+}
+
 void EmbeddedWebView::changeEvent(QEvent *e)
 {
     QWebView::changeEvent(e);
diff --git a/src/Gui/EmbeddedWebView.h b/src/Gui/EmbeddedWebView.h
index 9ac83d15..b8001ebd 100644
--- a/src/Gui/EmbeddedWebView.h
+++ b/src/Gui/EmbeddedWebView.h
@@ -78,6 +78,7 @@ private slots:
     void autoScroll();
     void slotLinkClicked(const QUrl &url);
     void handlePageLoadFinished();
+    void slotReplyFinished(QNetworkReply *reply);
 private:
     QWidget *m_scrollParent;
     int m_scrollParentPadding;
diff --git a/src/Gui/SimplePartWidget.cpp b/src/Gui/SimplePartWidget.cpp
index de4d9a2d..f3f58f98 100644
--- a/src/Gui/SimplePartWidget.cpp
+++ b/src/Gui/SimplePartWidget.cpp
@@ -71,6 +71,8 @@ SimplePartWidget::SimplePartWidget(QWidget *parent, Imap::Network::MsgPartNetAcc
             QWebSettings *s = settings();
             s->setFontFamily(QWebSettings::StandardFont, font.family());
         }
+    } else {
+        connect(this, &QWebView::loadFinished, this, &SimplePartWidget::slotMarkupPage);
     }
     load(url);
 
@@ -107,6 +109,21 @@ SimplePartWidget::SimplePartWidget(QWidget *parent, Imap::Network::MsgPartNetAcc
     }
 }
 
+void SimplePartWidget::slotMarkupPage()
+{
+    // NOTICE "single shot", we get a recursion otherwise!
+    disconnect(this, &QWebView::loadFinished, this, &SimplePartWidget::slotMarkupPage);
+
+    // If there's no data, don't try to "fix it up"
+    if (!m_partIndex.isValid() || !m_partIndex.data(Imap::Mailbox::RoleIsFetched).toBool())
+        return;
+
+    // and finally set the page
+    static QString header(QStringLiteral("<meta http-equiv=\"x-dns-prefetch-control\" content=\"off\">"));
+    page()->mainFrame()->setHtml(header + m_partIndex.data(Imap::Mailbox::RolePartUnicodeText).toString());
+    qDebug() << "replaced the text";
+}
+
 void SimplePartWidget::slotMarkupPlainText()
 {
     // NOTICE "single shot", we get a recursion otherwise!
diff --git a/src/Gui/SimplePartWidget.h b/src/Gui/SimplePartWidget.h
index 14162e0e..3b3cb677 100644
--- a/src/Gui/SimplePartWidget.h
+++ b/src/Gui/SimplePartWidget.h
@@ -66,6 +66,7 @@ public:
     void buildContextMenu(const QPoint &point, QMenu &menu) const;
 private slots:
     void slotFileNameRequested(QString *fileName);
+    void slotMarkupPage();
     void slotMarkupPlainText();
     void slotDownloadPart();
     void slotDownloadMessage();
diff --git a/src/Imap/Network/MsgPartNetworkReply.cpp b/src/Imap/Network/MsgPartNetworkReply.cpp
index 1135650e..275f1555 100644
--- a/src/Imap/Network/MsgPartNetworkReply.cpp
+++ b/src/Imap/Network/MsgPartNetworkReply.cpp
@@ -44,6 +44,9 @@ MsgPartNetworkReply::MsgPartNetworkReply(MsgPartNetAccessManager *parent, const
     url.setPath(part.data(Imap::Mailbox::RolePartPathToPart).toString());
     setUrl(url);
 
+    qDebug() << "MsgPartNetworkReply: X-DNS-Prefetch-Control: off";
+    setRawHeader(QByteArrayLiteral("X-DNS-Prefetch-Control"), QByteArrayLiteral("off"));
+
     setOpenMode(QIODevice::ReadOnly | QIODevice::Unbuffered);
     Q_ASSERT(part.isValid());
 
@@ -107,6 +110,8 @@ void MsgPartNetworkReply::slotMyDataChanged()
     } else {
         setHeader(QNetworkRequest::ContentTypeHeader, mimeType);
     }
+    qDebug() << "MsgPartNetworkReply: X-DNS-Prefetch-Control: off";
+    setRawHeader(QByteArrayLiteral("X-DNS-Prefetch-Control"), QByteArrayLiteral("off"));
     setFinished(true);
     emit readyRead();
     emit finished();

> Does anyone have a better approach?

Patch QtWebKit and send that patch upstream, please. This is clearly their bug.

> Otherwise the behavior seems intended. You can configure the default behavior, but the webpage has the last word on this.

Which looks like a reasonable behavior for a web browser to my eyes untrained on today's browsers' safety, but it's definitely a wrong behavior for a reusable component.

Caspar, did you have luck reporting this bug to the Qt upstream?

(In reply to Jan Kundrát from comment #9)
> Caspar, did you have luck reporting this bug to the Qt upstream?

I'm sorry, no, not yet. I'll open a bug report tonight and I'll keep you posted.

(In reply to Caspar Schutijser from comment #10)
> I'll open a bug report tonight and I'll keep you
> posted.

https://bugreports.qt.io/browse/QTBUG-67068

Jens reported the bug to WebKit: https://bugs.webkit.org/show_bug.cgi?id=182924

That change will be backported to QtWebKit: https://bugreports.qt.io/browse/QTBUG-67068?focusedCommentId=395494&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-395494

Jens, thanks for reporting the bug.