Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace qhttp client with curl for favicon downloading #1460

Merged
merged 3 commits into from
Feb 7, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ git:

before_install:
- if [ "$TRAVIS_OS_NAME" = "linux" ]; then sudo apt-get -qq update; fi
- if [ "$TRAVIS_OS_NAME" = "linux" ]; then sudo apt-get -qq install cmake3 libclang-common-3.5-dev libxi-dev qtbase5-dev libqt5x11extras5-dev qttools5-dev qttools5-dev-tools libgcrypt20-dev zlib1g-dev libxtst-dev xvfb libyubikey-dev libykpers-1-dev; fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason to keep this file at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, now is a good time to trash it!

- if [ "$TRAVIS_OS_NAME" = "linux" ]; then sudo apt-get -qq install cmake libclang-common-3.5-dev libxi-dev qtbase5-dev libqt5x11extras5-dev qttools5-dev qttools5-dev-tools libgcrypt20-dev zlib1g-dev libxtst-dev xvfb libyubikey-dev libykpers-1-dev libcurl4-openssl-dev; fi
- if [ "$TRAVIS_OS_NAME" = "osx" ]; then brew update; fi
- if [ "$TRAVIS_OS_NAME" = "osx" ]; then brew ls | grep -wq cmake || brew install cmake; fi
- if [ "$TRAVIS_OS_NAME" = "osx" ]; then brew ls | grep -wq qt5 || brew install qt5; fi
Expand Down
5 changes: 3 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

FROM ubuntu:14.04

ENV REBUILD_COUNTER=4
ENV REBUILD_COUNTER=5

ENV QT5_VERSION=59
ENV QT5_PPA_VERSION=${QT5_VERSION}2
Expand Down Expand Up @@ -51,7 +51,8 @@ RUN set -x \
libxtst-dev \
mesa-common-dev \
libyubikey-dev \
libykpers-1-dev
libykpers-1-dev \
libcurl4-openssl-dev

ENV CMAKE_PREFIX_PATH="/opt/qt${QT5_VERSION}/lib/cmake"
ENV CMAKE_INCLUDE_PATH="/opt/libgcrypt20-18/include:/opt/gpg-error-127/include"
Expand Down
1 change: 1 addition & 0 deletions ci/trusty/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ RUN set -x \
libgcrypt20-18-dev \
libargon2-0-dev \
libsodium-dev \
libcurl4-openssl-dev \
qt${QT5_VERSION}base \
qt${QT5_VERSION}tools \
qt${QT5_VERSION}x11extras \
Expand Down
1 change: 1 addition & 0 deletions snapcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ parts:
- libxtst-dev
- libyubikey-dev
- libykpers-1-dev
- libcurl4-openssl-dev
- libsodium-dev
stage-packages:
- dbus
Expand Down
21 changes: 8 additions & 13 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,10 @@ add_feature_info(KeePassHTTP WITH_XC_HTTP "Browser integration compatible with C
add_feature_info(SSHAgent WITH_XC_SSHAGENT "SSH agent integration compatible with KeeAgent")
add_feature_info(YubiKey WITH_XC_YUBIKEY "YubiKey HMAC-SHA1 challenge-response")

if(WITH_XC_HTTP)
add_subdirectory(http)
set(keepasshttp_LIB keepasshttp)
endif()
add_subdirectory(http)
if(WITH_XC_NETWORKING)
add_subdirectory(http/qhttp)
set(keepassxcnetwork_LIB qhttp Qt5::Network)
find_package(CURL REQUIRED)
set(keepassxcnetwork_LIB curl)
Copy link
Member

@phoerious phoerious Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ${CURL_LIBRARIES} instead of hardcoding the library name.

I think you should also remove this line altogether and use ${CURL_LIBRARIES} directly in target_link_libraries. Then we can get rid of ${keepassxchttp_LIB} when we drop KeepassHTTP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, duh good point

endif()

set(BROWSER_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/browser)
Expand Down Expand Up @@ -251,23 +248,21 @@ endif()
add_library(autotype STATIC ${autotype_SOURCES})
target_link_libraries(autotype Qt5::Core Qt5::Widgets)

set(autotype_LIB autotype)

add_library(keepassx_core STATIC ${keepassx_SOURCES})

set_target_properties(keepassx_core PROPERTIES COMPILE_DEFINITIONS KEEPASSX_BUILDING_CORE)
target_link_libraries(keepassx_core
${keepassxcbrowser_LIB}
${keepasshttp_LIB}
autotype
${keepassxchttp_LIB}
${keepassxcnetwork_LIB}
${autotype_LIB}
${keepassxcbrowser_LIB}
${sshagent_LIB}
${YUBIKEY_LIBRARIES}
${ZXCVBN_LIBRARIES}
Qt5::Core
Qt5::Network
Qt5::Concurrent
Qt5::Widgets
${YUBIKEY_LIBRARIES}
${ZXCVBN_LIBRARIES}
${ARGON2_LIBRARIES}
${GCRYPT_LIBRARIES}
${GPGERROR_LIBRARIES}
Expand Down
161 changes: 64 additions & 97 deletions src/gui/EditWidgetIcons.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@
#include "gui/MessageBox.h"

#ifdef WITH_XC_NETWORKING
#include "http/qhttp/qhttpclient.hpp"
#include "http/qhttp/qhttpclientresponse.hpp"

using namespace qhttp::client;
#include <QFuture>
#include <QtConcurrent>
#include <curl/curl.h>
#undef MessageBox
#endif

IconStruct::IconStruct()
Expand All @@ -49,10 +49,6 @@ EditWidgetIcons::EditWidgetIcons(QWidget* parent)
, m_database(nullptr)
, m_defaultIconModel(new DefaultIconModel(this))
, m_customIconModel(new CustomIconModel(this))
#ifdef WITH_XC_NETWORKING
, m_fallbackToGoogle(true)
, m_redirectCount(0)
#endif
{
m_ui->setupUi(this);

Expand Down Expand Up @@ -148,7 +144,6 @@ void EditWidgetIcons::setUrl(const QString& url)
#ifdef WITH_XC_NETWORKING
m_url = url;
m_ui->faviconButton->setVisible(!url.isEmpty());
resetFaviconDownload();
#else
Q_UNUSED(url);
m_ui->faviconButton->setVisible(false);
Expand All @@ -158,107 +153,79 @@ void EditWidgetIcons::setUrl(const QString& url)
void EditWidgetIcons::downloadFavicon()
{
#ifdef WITH_XC_NETWORKING
m_ui->faviconButton->setDisabled(true);

QUrl url = QUrl(m_url);
url.setPath("/favicon.ico");
fetchFavicon(url);
#endif
}

#ifdef WITH_XC_NETWORKING
void EditWidgetIcons::fetchFavicon(const QUrl& url)
{
if (nullptr == m_httpClient) {
m_httpClient = new QHttpClient(this);
}

bool requestMade = m_httpClient->request(qhttp::EHTTP_GET, url, [this, url](QHttpResponse* response) {
if (m_database == nullptr) {
return;
}

response->collectData();
response->onEnd([this, response, &url]() {
int status = response->status();
if (200 == status) {
QImage image;
image.loadFromData(response->collectedData());

if (!image.isNull()) {
addCustomIcon(image);
resetFaviconDownload();
} else {
fetchFaviconFromGoogle(url.host());
}
} else if (301 == status || 302 == status) {
// Check if server has sent a redirect
QUrl possibleRedirectUrl(response->headers().value("location", ""));
if (!possibleRedirectUrl.isEmpty() && possibleRedirectUrl != m_redirectUrl && m_redirectCount < 3) {
resetFaviconDownload(false);
m_redirectUrl = possibleRedirectUrl;
++m_redirectCount;
fetchFavicon(m_redirectUrl);
} else {
// website is trying to redirect to itself or
// maximum number of redirects has been reached, fall back to Google
fetchFaviconFromGoogle(url.host());
}
} else {
fetchFaviconFromGoogle(url.host());
}
});
});

if (!requestMade) {
resetFaviconDownload();
return;
}

m_httpClient->setConnectingTimeOut(5000, [this]() {
QUrl tempurl = QUrl(m_url);
if (tempurl.scheme() == "http") {
resetFaviconDownload();
emit messageEditEntry(tr("Unable to fetch favicon.") + "\n" +
tr("Hint: You can enable Google as a fallback under Tools>Settings>Security"),
MessageWidget::Error);
// Attempt to simply load the favicon.ico file
QImage image = fetchFavicon(url);
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 {
tempurl.setScheme("http");
m_url = tempurl.url();
tempurl.setPath("/favicon.ico");
fetchFavicon(tempurl);
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(true);
m_ui->faviconButton->setDisabled(false);
#endif
}

void EditWidgetIcons::fetchFaviconFromGoogle(const QString& domain)
#ifdef WITH_XC_NETWORKING
size_t writeCurlResponse(char* ptr, size_t size, size_t nmemb, void* data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::size_t
Function should also be wrapped in anonymous namespace.

{
if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool() && m_fallbackToGoogle) {
resetFaviconDownload();
m_fallbackToGoogle = false;
QUrl faviconUrl = QUrl("https://www.google.com/s2/favicons");
faviconUrl.setQuery("domain=" + QUrl::toPercentEncoding(domain));
fetchFavicon(faviconUrl);
} else {
resetFaviconDownload();
emit messageEditEntry(tr("Unable to fetch favicon."), MessageWidget::Error);
}
QByteArray* response = static_cast<QByteArray*>(data);
size_t realsize = size * nmemb;
response->append(ptr, realsize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we know the real size, you can use response->reserve() to pre-allocate memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually reserve doesn't make sense in this context because we are adding the new data received to an existing buffer. The append function is accurate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I somehow added a loop in my head, which isn't actually there.

return realsize;
}

void EditWidgetIcons::resetFaviconDownload(bool clearRedirect)
QImage EditWidgetIcons::fetchFavicon(const QUrl& url)
{
if (clearRedirect) {
m_redirectUrl.clear();
m_redirectCount = 0;
}
QImage image;
CURL* curl = curl_easy_init();
if (curl != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_FOLLOWLOCATION, 1L);
curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 5L);
curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1L);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &imagedata);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &writeCurlResponse);

// Perform the request in another thread
QFuture<CURLcode> future = QtConcurrent::run([curl]() {
return curl_easy_perform(curl);
});

QEventLoop loop;
QFutureWatcher<CURLcode> watcher;
connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
watcher.setFuture(future);
loop.exec();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have helper functions for this now.


curl_easy_cleanup(curl);

if (nullptr != m_httpClient) {
m_httpClient->deleteLater();
m_httpClient = nullptr;
if (future.result() == CURLE_OK) {
image.loadFromData(imagedata);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe show an error message otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a worker function, the error messages are handled in the parent caller (downloadFavicon())

}

m_fallbackToGoogle = true;
m_ui->faviconButton->setDisabled(false);
return image;
}
#endif

Expand All @@ -281,7 +248,7 @@ void EditWidgetIcons::addCustomIconFromFile()
}
}

void EditWidgetIcons::addCustomIcon(const QImage &icon)
void EditWidgetIcons::addCustomIcon(const QImage& icon)
{
if (m_database) {
Uuid uuid = m_database->metadata()->findCustomIcon(icon);
Expand Down
18 changes: 1 addition & 17 deletions src/gui/EditWidgetIcons.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,6 @@ class Database;
class DefaultIconModel;
class CustomIconModel;

#ifdef WITH_XC_NETWORKING
namespace qhttp {
namespace client {
class QHttpClient;
}
}
#endif

namespace Ui {
class EditWidgetIcons;
}
Expand Down Expand Up @@ -74,9 +66,7 @@ public slots:
private slots:
void downloadFavicon();
#ifdef WITH_XC_NETWORKING
void fetchFavicon(const QUrl& url);
void fetchFaviconFromGoogle(const QString& domain);
void resetFaviconDownload(bool clearRedirect = true);
QImage fetchFavicon(const QUrl& url);
#endif
void addCustomIconFromFile();
void addCustomIcon(const QImage& icon);
Expand All @@ -93,12 +83,6 @@ private slots:
QString m_url;
DefaultIconModel* const m_defaultIconModel;
CustomIconModel* const m_customIconModel;
#ifdef WITH_XC_NETWORKING
QUrl m_redirectUrl;
bool m_fallbackToGoogle;
unsigned short m_redirectCount;
qhttp::client::QHttpClient* m_httpClient = nullptr;
#endif

Q_DISABLE_COPY(EditWidgetIcons)
};
Expand Down
10 changes: 5 additions & 5 deletions src/gui/MainWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
class HttpPlugin: public ISettingsPage
{
public:
HttpPlugin(DatabaseTabWidget * tabWidget)
HttpPlugin(DatabaseTabWidget* tabWidget)
{
m_service = new Service(tabWidget);
}
Expand All @@ -88,18 +88,18 @@ class HttpPlugin: public ISettingsPage

QWidget * createWidget() override
{
OptionDialog * dlg = new OptionDialog();
OptionDialog* dlg = new OptionDialog();
QObject::connect(dlg, SIGNAL(removeSharedEncryptionKeys()), m_service, SLOT(removeSharedEncryptionKeys()));
QObject::connect(dlg, SIGNAL(removeStoredPermissions()), m_service, SLOT(removeStoredPermissions()));
return dlg;
}

void loadSettings(QWidget * widget) override
void loadSettings(QWidget* widget) override
{
qobject_cast<OptionDialog*>(widget)->loadSettings();
}

void saveSettings(QWidget * widget) override
void saveSettings(QWidget* widget) override
{
qobject_cast<OptionDialog*>(widget)->saveSettings();
if (HttpSettings::isEnabled())
Expand All @@ -108,7 +108,7 @@ class HttpPlugin: public ISettingsPage
m_service->stop();
}
private:
Service *m_service;
Service* m_service;
};
#endif

Expand Down
6 changes: 5 additions & 1 deletion src/http/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
if(WITH_XC_HTTP)
add_subdirectory(qhttp)

include_directories(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR})

set(keepasshttp_SOURCES
Expand All @@ -13,5 +15,7 @@ if(WITH_XC_HTTP)
)

add_library(keepasshttp STATIC ${keepasshttp_SOURCES})
target_link_libraries(keepasshttp qhttp Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network)
target_link_libraries(keepasshttp PUBLIC qhttp Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network)

set(keepassxchttp_LIB keepasshttp PARENT_SCOPE)
endif()