favicons: improve favicon fetching process

Numerous improvements: * Eliminate dependency on libcurl. We already depend on Qt5Network, so why not use the HTTP fetching from there? * Show a progress dialog when downloading the favicon. The main utility of this is giving the user the option to cancel a download attempt (e.g. if it's taking too long). Canceling will try the next fallback URL in the list. * Try three different ways to obtain the favicon, in this order: * Direct to fully-qualified domain name (e.g. https://foo.bar.example.com/favicon.ico) * Direct to 2nd-level domain name (e.g. https://example.com/favicon.ico) * Google lookup for 2nd-level domain name (if enabled in settings) I changed the Google lookup, because a match is more likely to be found for the 2nd level domain than for the fully-qualified name. Google's error behavior is strange. If it doesn't find a match, it doesn't return an error. Instead, it returns a generic default icon, which is not really the desired result. This also means that unless we have some way to detect that we've received the generic icon, we can't fall back to any alternatives. Signed-off-by: Steven Noonan <[email protected]>
keepassxreboot · Mar 28, 2018 · 54b5470 · 54b5470
1 parent 73cbaf7
commit 54b5470
Show file tree

Hide file tree

Showing 3 changed files with 139 additions and 66 deletions.
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -202,9 +202,6 @@ add_feature_info(SSHAgent WITH_XC_SSHAGENT "SSH agent integration compatible wit
 add_feature_info(YubiKey WITH_XC_YUBIKEY "YubiKey HMAC-SHA1 challenge-response")
 
 add_subdirectory(http)
-if(WITH_XC_NETWORKING)
-  find_package(CURL REQUIRED)
-endif()
 
 set(BROWSER_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/browser)
 add_subdirectory(browser)

diff --git a/src/gui/EditWidgetIcons.cpp b/src/gui/EditWidgetIcons.cpp
@@ -30,9 +30,7 @@
 #include "gui/MessageBox.h"
 
 #ifdef WITH_XC_NETWORKING
-#include <curl/curl.h>
-#include "core/AsyncTask.h"
-#undef MessageBox
+#include <QtNetwork>
 #endif
 
 IconStruct::IconStruct()
@@ -41,10 +39,31 @@ IconStruct::IconStruct()
 {
 }
 
+UrlFetchProgressDialog::UrlFetchProgressDialog(const QUrl &url, QWidget *parent)
+  : QProgressDialog(parent)
+{
+    setWindowTitle(tr("Download Progress"));
+    setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint);
+    setLabelText(tr("Downloading %1.").arg(url.toDisplayString()));
+    setMinimum(0);
+    setValue(0);
+    setMinimumDuration(0);
+    setMinimumSize(QSize(400, 75));
+}
+
+void UrlFetchProgressDialog::networkReplyProgress(qint64 bytesRead, qint64 totalBytes)
+{
+    setMaximum(totalBytes);
+    setValue(bytesRead);
+}
+
 EditWidgetIcons::EditWidgetIcons(QWidget* parent)
     : QWidget(parent)
     , m_ui(new Ui::EditWidgetIcons())
     , m_database(nullptr)
+#ifdef WITH_XC_NETWORKING
+    , m_reply(nullptr)
+#endif
     , m_defaultIconModel(new DefaultIconModel(this))
     , m_customIconModel(new CustomIconModel(this))
 {
@@ -135,95 +154,129 @@ void EditWidgetIcons::load(const Uuid& currentUuid, Database* database, const Ic
 void EditWidgetIcons::setUrl(const QString& url)
 {
 #ifdef WITH_XC_NETWORKING
-    m_url = url;
+    m_url = QUrl(url);
     m_ui->faviconButton->setVisible(!url.isEmpty());
 #else
     Q_UNUSED(url);
     m_ui->faviconButton->setVisible(false);
 #endif
 }
 
+#ifdef WITH_XC_NETWORKING
+namespace {
+    // Try to get the 2nd level domain of the host part of a QUrl. For example,
+    // "foo.bar.example.com" would become "example.com", and "foo.bar.example.co.uk"
+    // would become "example.co.uk".
+    QString getSecondLevelDomain(QUrl url)
+    {
+        QString fqdn = url.host();
+        fqdn.truncate(fqdn.length() - url.topLevelDomain().length());
+        QStringList parts = fqdn.split('.');
+        QString newdom = parts.takeLast() + url.topLevelDomain();
+        return newdom;
+    }
+}
+#endif
+
 void EditWidgetIcons::downloadFavicon()
 {
 #ifdef WITH_XC_NETWORKING
     m_ui->faviconButton->setDisabled(true);
 
-    QUrl url = QUrl(m_url);
-    url.setPath("/favicon.ico");
+    m_urlsToTry.clear();
+
+    QString fullyQualifiedDomain = m_url.host();
+    QString secondLevelDomain = getSecondLevelDomain(m_url);
+
     // Attempt to simply load the favicon.ico file
-    QImage image = fetchFavicon(url);
+    if (fullyQualifiedDomain != secondLevelDomain) {
+        m_urlsToTry.append(QUrl(m_url.scheme() + "://" + fullyQualifiedDomain + "/favicon.ico"));
+    }
+    m_urlsToTry.append(QUrl(m_url.scheme() + "://" + secondLevelDomain + "/favicon.ico"));
+
+    // Try to use Google fallback, if enabled
+    if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool()) {
+        QUrl urlGoogle = QUrl("https://www.google.com/s2/favicons");
+
+        urlGoogle.setQuery("domain=" + QUrl::toPercentEncoding(secondLevelDomain));
+        m_urlsToTry.append(urlGoogle);
+    }
+
+    startFetchFavicon(m_urlsToTry.takeFirst());
+#endif
+}
+
+void EditWidgetIcons::fetchReadyRead()
+{
+#ifdef WITH_XC_NETWORKING
+    m_bytesReceived += m_reply->readAll();
+#endif
+}
+
+void EditWidgetIcons::fetchFinished()
+{
+#ifdef WITH_XC_NETWORKING
+    QImage image;
+    bool googleFallbackEnabled = config()->get("security/IconDownloadFallbackToGoogle", false).toBool();
+    bool error = (m_reply->error() != QNetworkReply::NoError);
+
+    m_reply->deleteLater();
+    m_reply = nullptr;
+
+    if (!error) {
+        image.loadFromData(m_bytesReceived);
+    }
+
     if (!image.isNull()) {
         addCustomIcon(image);
-    } else if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool()) {
-        QUrl faviconUrl = QUrl("https://www.google.com/s2/favicons");
-        faviconUrl.setQuery("domain=" + QUrl::toPercentEncoding(url.host()));
-        // Attempt to load favicon from Google
-        image = fetchFavicon(faviconUrl);
-        if (!image.isNull()) {
-            addCustomIcon(image);
+    } else if (!m_urlsToTry.empty()) {
+        startFetchFavicon(m_urlsToTry.takeFirst());
+        return;
+    } else {
+        if (!googleFallbackEnabled) {
+            emit messageEditEntry(tr("Unable to fetch favicon.") + "\n" +
+                                  tr("Hint: You can enable Google as a fallback under Tools>Settings>Security"),
+                                  MessageWidget::Error);
         } else {
             emit messageEditEntry(tr("Unable to fetch favicon."), MessageWidget::Error);
         }
-    } else {
-        emit messageEditEntry(tr("Unable to fetch favicon.") + "\n" +
-                              tr("Hint: You can enable Google as a fallback under Tools>Settings>Security"),
-                              MessageWidget::Error);
     }
 
     m_ui->faviconButton->setDisabled(false);
 #endif
 }
 
-#ifdef WITH_XC_NETWORKING
-namespace {
-std::size_t writeCurlResponse(char* ptr, std::size_t size, std::size_t nmemb, void* data)
+void EditWidgetIcons::fetchCanceled()
 {
-    QByteArray* response = static_cast<QByteArray*>(data);
-    std::size_t realsize = size * nmemb;
-    response->append(ptr, realsize);
-    return realsize;
-}
+#ifdef WITH_XC_NETWORKING
+    m_reply->abort();
+#endif
 }
 
-QImage EditWidgetIcons::fetchFavicon(const QUrl& url)
+void EditWidgetIcons::startFetchFavicon(const QUrl& url)
 {
-    QImage image;
-    CURL* curl = curl_easy_init();
-    if (curl) {
-        QByteArray imagedata;
-        QByteArray baUrl = url.url().toLatin1();
-
-        curl_easy_setopt(curl, CURLOPT_URL, baUrl.data());
-        curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 5L);
-        curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
-        curl_easy_setopt(curl, CURLOPT_USERAGENT, "curl");
-        curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
-        curl_easy_setopt(curl, CURLOPT_TIMEOUT, 10L);
-        curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1L);
-        curl_easy_setopt(curl, CURLOPT_WRITEDATA, &imagedata);
-        curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &writeCurlResponse);
-#ifdef Q_OS_WIN
-        const QDir appDir = QFileInfo(QCoreApplication::applicationFilePath()).absoluteDir();
-        if (appDir.exists("ssl\\certs")) {
-            curl_easy_setopt(curl, CURLOPT_CAINFO, (appDir.absolutePath() + "\\ssl\\certs\\ca-bundle.crt").toLatin1().data());
-        }
-#endif
+#ifdef WITH_XC_NETWORKING
+    m_bytesReceived.clear();
 
-        // Perform the request in another thread
-        CURLcode result = AsyncTask::runAndWaitForFuture([curl]() {
-            return curl_easy_perform(curl);
-        });
+    QNetworkRequest request(url);
+    request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true);
+    request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy);
 
-        if (result == CURLE_OK) {
-            image.loadFromData(imagedata);
-        }
+    m_reply = m_netMgr.get(request);
+    connect(m_reply, &QNetworkReply::finished, this, &EditWidgetIcons::fetchFinished);
+    connect(m_reply, &QIODevice::readyRead, this, &EditWidgetIcons::fetchReadyRead);
 
-        curl_easy_cleanup(curl);
-    }
+    UrlFetchProgressDialog *progress = new UrlFetchProgressDialog(url, this);
+    progress->setAttribute(Qt::WA_DeleteOnClose);
+    connect(m_reply, &QNetworkReply::finished, progress, &QProgressDialog::hide);
+    connect(m_reply, &QNetworkReply::downloadProgress, progress, &UrlFetchProgressDialog::networkReplyProgress);
+    connect(progress, &QProgressDialog::canceled, this, &EditWidgetIcons::fetchCanceled);
 
-    return image;
-}
+    progress->show();
+#else
+    Q_UNUSED(url);
 #endif
+}
 
 void EditWidgetIcons::addCustomIconFromFile()
 {

diff --git a/src/gui/EditWidgetIcons.h b/src/gui/EditWidgetIcons.h
@@ -21,7 +21,9 @@
 
 #include <QWidget>
 #include <QSet>
+#include <QProgressDialog>
 #include <QUrl>
+#include <QNetworkAccessManager>
 
 #include "config-keepassx.h"
 #include "core/Global.h"
@@ -31,6 +33,9 @@
 class Database;
 class DefaultIconModel;
 class CustomIconModel;
+#ifdef WITH_XC_NETWORKING
+class QNetworkReply;
+#endif
 
 namespace Ui {
     class EditWidgetIcons;
@@ -44,6 +49,17 @@ struct IconStruct
     int number;
 };
 
+class UrlFetchProgressDialog : public QProgressDialog
+{
+    Q_OBJECT
+
+public:
+    explicit UrlFetchProgressDialog(const QUrl &url, QWidget *parent = nullptr);
+
+public slots:
+    void networkReplyProgress(qint64 bytesRead, qint64 totalBytes);
+};
+
 class EditWidgetIcons : public QWidget
 {
     Q_OBJECT
@@ -65,9 +81,10 @@ public slots:
 
 private slots:
     void downloadFavicon();
-#ifdef WITH_XC_NETWORKING
-    QImage fetchFavicon(const QUrl& url);
-#endif
+    void startFetchFavicon(const QUrl& url);
+    void fetchFinished();
+    void fetchReadyRead();
+    void fetchCanceled();
     void addCustomIconFromFile();
     void addCustomIcon(const QImage& icon);
     void removeCustomIcon();
@@ -80,7 +97,13 @@ private slots:
     const QScopedPointer<Ui::EditWidgetIcons> m_ui;
     Database* m_database;
     Uuid m_currentUuid;
-    QString m_url;
+#ifdef WITH_XC_NETWORKING
+    QUrl m_url;
+    QList<QUrl> m_urlsToTry;
+    QByteArray m_bytesReceived;
+    QNetworkAccessManager m_netMgr;
+    QNetworkReply *m_reply;
+#endif
     DefaultIconModel* const m_defaultIconModel;
     CustomIconModel* const m_customIconModel;