From 056bbaa92169152ca96466615b1cd77a272998c6 Mon Sep 17 00:00:00 2001 From: Steven Noonan Date: Wed, 4 Apr 2018 19:18:58 -0700 Subject: [PATCH] Improve fetch favicon (#1786) * Eliminate dependency on libcurl in favor of Qt5Network code * Supports older Qt versions without QNetworkRequest::FollowRedirectsAttribute * 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: 1) Direct to fully-qualified domain (e.g. https://foo.bar.example.com/favicon.ico) 2) Direct to 2nd-level domain (e.g. https://example.com/favicon.ico) 3) 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 --- src/CMakeLists.txt | 3 - src/gui/EditWidgetIcons.cpp | 213 +++++++++++++++++++++++++----------- src/gui/EditWidgetIcons.h | 33 +++++- src/main.cpp | 17 +++ 4 files changed, 194 insertions(+), 72 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b3c7a650f..bb1949dfc 100644 --- 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 index af4476ac3..837d6ef5c 100644 --- a/src/gui/EditWidgetIcons.cpp +++ b/src/gui/EditWidgetIcons.cpp @@ -21,7 +21,6 @@ #include #include -#include #include "core/Config.h" #include "core/Group.h" @@ -31,9 +30,7 @@ #include "gui/MessageBox.h" #ifdef WITH_XC_NETWORKING -#include -#include "core/AsyncTask.h" -#undef MessageBox +#include #endif IconStruct::IconStruct() @@ -42,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)) { @@ -136,7 +154,7 @@ 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); @@ -144,87 +162,152 @@ void EditWidgetIcons::setUrl(const QString& url) #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; + } + + QUrl convertVariantToUrl(QVariant var) + { + QUrl url; + if (var.canConvert()) + url = var.value(); + return url; + } + + QUrl getRedirectTarget(QNetworkReply *reply) + { + QVariant var = reply->attribute(QNetworkRequest::RedirectionTargetAttribute); + QUrl url = convertVariantToUrl(var); + return url; + } +} +#endif + void EditWidgetIcons::downloadFavicon() { #ifdef WITH_XC_NETWORKING m_ui->faviconButton->setDisabled(true); - QUrl url = QUrl(m_url); - url.setPath("/favicon.ico"); + m_redirects = 0; + 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); + QUrl redirectTarget = getRedirectTarget(m_reply); + + m_reply->deleteLater(); + m_reply = nullptr; + + if (!error) { + if (redirectTarget.isValid()) { + // Redirected, we need to follow it, or fall through if we have + // done too many redirects already. + if (m_redirects < 5) { + m_redirects++; + if (redirectTarget.isRelative()) + redirectTarget = m_fetchUrl.resolved(redirectTarget); + startFetchFavicon(redirectTarget); + return; + } + } else { + // No redirect, and we theoretically have some icon data now. + 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()) { + m_redirects = 0; + 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 } +void EditWidgetIcons::fetchCanceled() +{ #ifdef WITH_XC_NETWORKING -namespace { -std::size_t writeCurlResponse(char* ptr, std::size_t size, std::size_t nmemb, void* data) -{ - QByteArray* response = static_cast(data); - std::size_t realsize = size * nmemb; - response->append(ptr, realsize); - return realsize; -} -} - -QImage EditWidgetIcons::fetchFavicon(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()); - } + m_reply->abort(); #endif - - // Perform the request in another thread - CURLcode result = AsyncTask::runAndWaitForFuture([curl]() { - return curl_easy_perform(curl); - }); - - if (result == CURLE_OK) { - image.loadFromData(imagedata); - } - - curl_easy_cleanup(curl); - } - - return image; } + +void EditWidgetIcons::startFetchFavicon(const QUrl& url) +{ +#ifdef WITH_XC_NETWORKING + m_bytesReceived.clear(); + + m_fetchUrl = url; + + QNetworkRequest request(url); + + m_reply = m_netMgr.get(request); + connect(m_reply, &QNetworkReply::finished, this, &EditWidgetIcons::fetchFinished); + connect(m_reply, &QIODevice::readyRead, this, &EditWidgetIcons::fetchReadyRead); + + 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); + + progress->show(); +#else + Q_UNUSED(url); #endif +} void EditWidgetIcons::addCustomIconFromFile() { diff --git a/src/gui/EditWidgetIcons.h b/src/gui/EditWidgetIcons.h index 7b5edf80c..82fcdeeb5 100644 --- a/src/gui/EditWidgetIcons.h +++ b/src/gui/EditWidgetIcons.h @@ -21,7 +21,9 @@ #include #include +#include #include +#include #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 @@ signals: 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,15 @@ private: const QScopedPointer m_ui; Database* m_database; Uuid m_currentUuid; - QString m_url; +#ifdef WITH_XC_NETWORKING + QUrl m_url; + QUrl m_fetchUrl; + QList m_urlsToTry; + QByteArray m_bytesReceived; + QNetworkAccessManager m_netMgr; + QNetworkReply *m_reply; + int m_redirects; +#endif DefaultIconModel* const m_defaultIconModel; CustomIconModel* const m_customIconModel; diff --git a/src/main.cpp b/src/main.cpp index b3b607f25..2ccbaf257 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -45,6 +45,21 @@ Q_IMPORT_PLUGIN(QXcbIntegrationPlugin) #endif #endif +static inline void earlyQNetworkAccessManagerWorkaround() +{ + // When QNetworkAccessManager is instantiated it regularly starts polling + // all network interfaces to see if anything changes and if so, what. This + // creates a latency spike every 10 seconds on Mac OS 10.12+ and Windows 7 >= + // when on a wifi connection. + // So here we disable it for lack of better measure. + // This will also cause this message: QObject::startTimer: Timers cannot + // have negative intervals + // For more info see: + // - https://bugreports.qt.io/browse/QTBUG-40332 + // - https://bugreports.qt.io/browse/QTBUG-46015 + qputenv("QT_BEARER_POLL_TIMEOUT", QByteArray::number(-1)); +} + int main(int argc, char** argv) { #ifdef QT_NO_DEBUG @@ -52,6 +67,8 @@ int main(int argc, char** argv) #endif Tools::setupSearchPaths(); + earlyQNetworkAccessManagerWorkaround(); + Application app(argc, argv); Application::setApplicationName("keepassxc"); Application::setApplicationVersion(KEEPASSX_VERSION);